User authentication over secure connection #23

bpintea · 2018-09-17T20:47:21Z

This PR adds support for user authenticated and secure connection to a server, both implemented through libcurl. A DB connection can now:

make use of a username and (optional) password to authenticate to the server (atm Elasticsearch only supports Basic auth).
be encrypted using SSL. A "secure" parameter controls the list of checks that are enabled in the SSL connection establishment, ranging incrementally through:
- simply use SSL, no checks;
- check CA; for this check a local file with server's public CA can be provided;
- check the hostname in the received cert;
- check the revocation chain (if avail).

The connection test part of a DSN validation now also includes a simple SELECT 0 query, to check if user auth works and that a server with the SQL plug in is indeed running on the other side.

The request/response handling has been also refactor to: (a) break the functionality into smaller function and (b) allow for libcurl's error to bubble up into the diagnostics eventually reported to the user.

- Consider and use the user id and password in DSN: - Feed these params to libcurl. The authentication mechansim is currently set to "any" (Elasticsearch seems to only support the HTTP Basic mechanism). - Change the initial connection testing to actually perform a query ("SELECT 0"), rather then a simple socket connection estiablishemnt check.

This should allow easier spotting of the build phases

- some test files still need migration to project's style

- configure the needed params for libcurl to be able to connect to a SSL-enabled Elasticsearch. This includes settings for: * checking the CA, hostname, revokation info; these params corespond to a "secure" connection string integer parameter. * reading a public CA file (if server's cert is not part of PKI). - register a debug call back to libcurl; - refactor part of HTTP request/response handling, covering also the initial connection test; - add a few "offline" unit test for a small part of the refactored code (the error handling).

droberts195

My main comments are about whether password should be optional in the config if username is specified. The other's are minor nits.

droberts195 · 2018-09-19T11:56:21Z

driver/connect.c

+				ERRH(dbc, "libcurl: failed to enable host check.");
+				goto err;
+			}
+			/* verify the revokation chain? */


Even though "revoke" is spelt with a "k", "revocation" is spelt with a "c". English is very inconsistent.

Thanks, will fix.

droberts195 · 2018-09-19T12:00:36Z

driver/connect.c

+		res = curl_easy_setopt(curl, CURLOPT_HTTPAUTH,
+				/* libcurl (7.61.0) won't pick Basic auth over SSL when
+				 * _ANY is used. -- ??? XXX */
+				dbc->secure ? CURLAUTH_BASIC : CURLAUTH_ANY);


Given that basic auth is all ES supports, would it be safer to always use CURLAUTH_BASIC in this code branch? If ES did ever add another method I think it would be preferable to have to change this code than to have it silently attempt to use some other authentication method with likely incorrect parameters.

Libcurl will always try to use the safest method if more are available. Without SSL, the next best thing that Elasticsearch could add is Digest, which would work with the same given credentials.
So if that's ever going to be a possibility, ANY would be preferable here (in the unlikely scenario of an old driver, but new Elasticsearch).
In fact, "any" other case than Basic, that would also work would be preferable.
Now, given the fact that other and better mechanisms are available/worked on (Kerberos, LDAP), the chance of this case is rather small.

have it silently attempt to use some other authentication method with likely incorrect parameters.

I don't think there would be a lot of downside (in terms of auth data leakage, at least).

However: it's fixed to Basic if SSL is enabled (=99%+ of use cases, probably) and any change in available auth methods would require a change here too, so I'll fixed it. (I'll keep the note, though, since it cost me some time :-) )

droberts195 · 2018-09-19T12:13:55Z

driver/connect.c

+	}
+	if (secure < ESODBC_SEC_NONE || ESODBC_SEC_MAX <= secure) {
+		ERRH(dbc, "invalid secure param `" LWPDL "` (not within %d - %d).",
+			LWSTR(&attrs->secure), ESODBC_SEC_NONE, ESODBC_SEC_CHECK_HOST);


It might be more future-proof to use ESODBC_SEC_MAX - 1 rather than ESODBC_SEC_CHECK_HOST, otherwise whoever adds a new enum value needs to remember to change this line too and if they forget then the code will still compile but the error message will be inaccurate.

Good catch. Indeed, that was the intention of adding ESODBC_SEC_MAX. Thanks.

droberts195 · 2018-09-19T12:17:34Z

driver/connect.c

+				attrs->uid.cnt, LWSTR(&attrs->uid));
+			goto err;
+		}
+		if (attrs->pwd.cnt) {


Is it traditional in ODBC drivers to treat a missing password setting as a password that is an empty string. I think that's what's being done here. The alternative would be to force the user to explicitly set the password setting if username was set, even if the password was an empty string. I doubt many people use empty passwords, so this might be a friendlier way to fail if we expect most cases of username set but password not set are due to people forgetting to set the password rather than not having one. (This idea also requires that it's possible to explicitly set the password setting to an empty string. If not then the way you've got it is the only way.)

I was initially thinking that the standard doesn't necessarily require a non-empty password.
I now see that Elasticsearch'es API rejects an empty value Validation Failed: 1: passwords must be at least [6] characters long;.
However, I would think that the right place for this check is (going to be) in the DSN configuration part (the pending GUI to add).
The user could still then edit the registry, true, but that would be a deliberate change, rather than accidental usage of a system unsecure by default.

The driver is pretty much just a proxy for information, rather then a gateway, so I'd think not enforcing it also here would be OK, but don't feel very strong about it. If you still think it's better to restrict it, I'll make the change.

OK fair enough, but maybe you should make the INFOH(dbc, "no password for username " LCPDL "." on line 339 of connect.c into a warning or error so that it is more prominent in the log file if the user ever does get into a situation where username is set but not pasword?

Sure, it's logged as an error now.

droberts195 · 2018-09-19T12:19:30Z

driver/connect.c

+				goto err;
+			}
+		} else {
+			INFOH(dbc, "no password for username `" LCPDL "`.",


I left a comment in the bit where config options are copied to the database connection saying that if it's not possible in Elasticsearch to have a user without a password then it should be disallowed at the config reading stage. If that's the case then this should be changed to an assertion.

- removed the non-SSL case where _ANY would have been used. - fix a spelling error ('revokation'); - log the correct upper 'secure' (parameter) limit in error message.

- however, allow the call to proceed.

droberts195

LGTM

bpintea added 4 commits September 13, 2018 22:11

prefix build.bat messages by its name

c5570c2

This should allow easier spotting of the build phases

add modelines to test files

1ded820

- some test files still need migration to project's style

bpintea requested review from droberts195 and edsavage September 17, 2018 20:47

bpintea mentioned this pull request Sep 17, 2018

TODOs #2

Closed

fixed attach_error tests

507971e

droberts195 reviewed Sep 19, 2018

View reviewed changes

bpintea added 2 commits September 19, 2018 16:14

Always use (CURLAUTH_)BASIC auth method

8c4c99c

- removed the non-SSL case where _ANY would have been used. - fix a spelling error ('revokation'); - log the correct upper 'secure' (parameter) limit in error message.

report a user without a password as an error

de3b9b9

- however, allow the call to proceed.

droberts195 approved these changes Sep 19, 2018

View reviewed changes

bpintea merged commit b5a17cb into elastic:master Sep 19, 2018

bpintea deleted the feature/auth branch September 19, 2018 18:31

bpintea added >feature Applicable to PRs adding new functionality v6.5.0 labels May 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

User authentication over secure connection #23

User authentication over secure connection #23

Uh oh!

bpintea commented Sep 17, 2018

Uh oh!

droberts195 left a comment

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

bpintea Sep 19, 2018

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

bpintea Sep 19, 2018

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

bpintea Sep 19, 2018

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

bpintea Sep 19, 2018

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

bpintea Sep 19, 2018

Uh oh!

droberts195 Sep 19, 2018

Uh oh!

droberts195 left a comment

Uh oh!

Uh oh!

User authentication over secure connection #23

User authentication over secure connection #23

Uh oh!

Conversation

bpintea commented Sep 17, 2018

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!