Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MESOS-9499 extended URI syntax to support any Zookeeper authentication schemes #324

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

MESOS-9499 extended URI syntax to support any Zookeeper authentication schemes #324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions include/mesos/zookeeper/authentication.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ struct Authentication
: scheme(_scheme),
credentials(_credentials)
{
// TODO(benh): Fix output operator below once this changes.
CHECK_EQ(scheme, "digest") << "Unsupported authentication scheme";
}

const std::string scheme;
Expand All @@ -62,9 +60,7 @@ inline std::ostream& operator<<(
std::ostream& stream,
const Authentication& authentication)
{
// TODO(benh): Fix this once we support more than just 'digest'.
CHECK_EQ(authentication.scheme, "digest");
return stream << authentication.credentials;
return stream << authentication.scheme << "!" << authentication.credentials;
}

} // namespace zookeeper {
Expand Down
38 changes: 29 additions & 9 deletions include/mesos/zookeeper/url.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class URL
public:
static Try<URL> parse(const std::string& url);

static const char* scheme()
static const char* urlScheme()
{
return "zk://";
}
Expand All @@ -63,10 +63,11 @@ class URL
: servers(_servers),
path(_path) {}

URL(const std::string& credentials,
URL(const std::string& scheme,
const std::string& credentials,
const std::string& _servers,
const std::string& _path)
: authentication(Authentication("digest", credentials)),
: authentication(Authentication(scheme, credentials)),
servers(_servers),
path(_path) {}
};
Expand All @@ -76,10 +77,10 @@ inline Try<URL> URL::parse(const std::string& url)
{
std::string s = strings::trim(url);

if (!strings::startsWith(s, URL::scheme())) {
if (!strings::startsWith(s, URL::urlScheme())) {
return Error("Expecting 'zk://' at the beginning of the URL");
}
s = s.substr(5);
s = s.substr(strlen(urlScheme()));

// Look for the trailing '/' (if any), that's where the path starts.
std::string path;
Expand All @@ -101,16 +102,35 @@ inline Try<URL> URL::parse(const std::string& url)
// Look for the trailing '@' (if any), that's where servers starts.
size_t index = s.find_last_of('@');

if (index != std::string::npos) {
return URL(s.substr(0, index), s.substr(index + 1), path);
} else {
if (index == std::string::npos)
return URL(s, path);

std::string servers = s.substr(index + 1);
std::string auth = s.substr(0, index);

size_t schemeDelimiter = auth.find_first_of('!');

// If there is not '!' in URL scheme is "digest" and everything before '@' is credentials
std::string scheme = "digest";
std::string credentials = auth;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding the authentication scheme into this location gives us a small-ish patch, but has downsides:

  • The addition of an exclamation mark delimiter invalidates the URI schema, according to https://tools.ietf.org/html/rfc1034#section-3.5 . Only letters, digits, and dashes are allowed in a "host" field. If we (Mesos) ever change our URI parsing logic, we would need to rip out this code.
  • Zookeeper's pluggable system does not seem to limit the characters in an authentication scheme (as far as I know). If a plugin's scheme has an @ in the name for whatever reason, the URI parsing logic here would fail spectacularly 😈 .

My recommendation is to add an optional master/agent flag which specifies the ZK authentication scheme.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not in the hostname. Look:
example: zk://simple!login:password@host:port/path
hostname starts after '@'.
Everything after zk:// and before '@' has nothing to do with DNS and hostnames.

Check https://tools.ietf.org/html/rfc3986
Page 49 describes allowed delimiters in this part, explicitly including '!'.
Section 3.2.1 describes this part as "User Information".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed that. However, if we use a ! as a delimiter, that means the user info cannot have exclamation marks. So something like:

zk://user:pass!word@host:port/path

Would give us a "authentication scheme" of user:pass, which would be incorrect.

Copy link
Author

@grok45 grok45 Jan 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. But it is an obvious side effect of design decision to pass authentication information as URL. Which was already in place.

It is also not a big problem. We can allow escape it somehow. Like "\!".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we can choose another delimiter symbol, RFS allows to use several symbols.


if(schemeDelimiter != std::string::npos) {
if(schemeDelimiter == 0)
return Error("Expecting Zookeeper authentication schemee before '!' in the URL");

if(schemeDelimiter == auth.length()-1)
return Error("Expecting credentials after '!' in the URL");

scheme = auth.substr(0, schemeDelimiter);
credentials = auth.substr(schemeDelimiter+1);
}

return URL(scheme, credentials, servers, path);
}

inline std::ostream& operator<<(std::ostream& stream, const URL& url)
{
stream << URL::scheme();
stream << URL::urlScheme();
if (url.authentication.isSome()) {
stream << url.authentication.get() << "@";
}
Expand Down