[Kerberos] Add Kerberos Realm by bizybot · Pull Request #31761 · elastic/elasticsearch

bizybot · 2018-07-03T13:41:58Z

This commit adds authentication realm for handling Kerberos
authentication by spnego mechanism.
The class KerberosRealm authenticates user for given kerberos
ticket after validating the ticket using KerberosTicketValidator.
It uses native role mapping store to find user details and
then creates an authenticated User.
On successful authentication, it will return populated User
object with roles. On failure to authenticate, it will terminate
authentication process with a failure message. The failure could be
due to gss context negotiation failure requiring further
negotiation and it might return outToken to be communicated with
peer as value for header WWW-Authenticate in the form
'Negotiate oYH1MIHyoAMK...'. There could be other failures like
JAAS login exception or GSS Exception which will terminate the
authentication process.

As KerberosRealm can terminate authentication process during
context negotiation with some outToken, the header value for
WWW-Authenticate needs to be preserved. Earlier the behavior
was to overwrite all the headers as defined in authentication
failure handler in my last commit. Negotiate does maintain kind
of state over HTTP and so we have to handle this in a special way.
For this, I have added a special check for if exception has header
'WWW-Authenticate' with 'Negotiate ' scheme and token, it will
not be overwritten.

We want Kerberos to be a platinum feature, so it is not
included as part of standard types similar to SAML.

TODO: Support for user lookup from other realms like AD/LDAP.
Authorizing realms feature is work in progress, once completed
I will add the support to KerberosRealm. I have a TODO note in
the source code.

This commit adds authentication realm for handling Kerberos authentication by spnego mechanism. The class `KerberosRealm` authenticates user for given kerberos ticket after validating the ticket using `KerberosTicketValidator`. It uses native role mapping store to find user details and then creates an authenticated `User`. On successful authentication, it will return populated `User` object with roles. On failure to authenticate, it will terminate authentication process with a failure message. The failure could be due to gss context negotiation failure requiring further negotiation and it might return outToken to be communicated with peer as value for header `WWW-Authenticate` in the form 'Negotiate oYH1MIHyoAMK...'. There could be other failures like JAAS login exception or GSS Exception which will terminate the authentication process. As KerberosRealm can terminate authentication process during context negotiation with some outToken, the header value for `WWW-Authenticate` needs to be preserved. Earlier the behavior was to overwrite all the headers as defined in authentication failure handler in my last commit. Negotiate does maintain kind of state over HTTP and so we have to handle this in a special way. For this, I have added a special check for if exception has header 'WWW-Authenticate' with 'Negotiate ' scheme and token, it will not be overwritten. We want Kerberos to be a platinum feature, so it is not included as part of standard types similar to SAML. TODO: Support for user lookup from other realms like AD/LDAP. Authorizing realms feature is work in progress, once completed I will add the support to KerberosRealm. I have a TODO note in source code.

elasticmachine · 2018-07-03T13:42:00Z

Pinging @elastic/es-security

tvernum

I haven't read through the tests yet.

tvernum · 2018-07-04T08:10:58Z

...gin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java

                                    logger.info("Authentication of [{}] was terminated by realm [{}] - {}",
                                            authenticationToken.principal(), realm.name(), result.getMessage());
-                                    userListener.onFailure(Exceptions.authenticationError(result.getMessage(), result.getException()));
+                                    userListener.onFailure(result.getException());


This change doesn't seem right - result.getException can be null, and is for the reserved realm.

yes, you are right. The reason I made this change was to avoid wrapping of ElasticsearchSecurityException which was thrown from KerberosRealm. It would not be a problem for other realms but for KerberosRealm there is a case where we do not want to do that. Like when it terminates authentication process with a token to be sent to peer for further gss negotiation. The header 'WWW-Authenticate', in this case, is present in thrown exception but if we wrap it then the header will be lost.
I did not think about exception being null, so now modified it to create authentication error if the exception is null. Thank you.

tvernum · 2018-07-04T08:12:43Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/InternalRealms.java

+    private static final Set<String> TYPES_DEPEND_ON_THIRD_PARTY_SOURCES = new HashSet<>();
+    static {
+        Collections.addAll(TYPES_DEPEND_ON_THIRD_PARTY_SOURCES, SamlRealmSettings.TYPE, KerberosRealmSettings.TYPE);
+    }


Why use a static block to initialise this?
Sets.newHashSet can turn this into a 1 liner, and then you can wrap it in unmodifiableSet

Thanks, still getting used to these additional helpers. I had a look at CollectionUtils but then there is also a Sets class.

tvernum · 2018-07-04T08:16:52Z

...c/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosAuthenticationToken.java

+     * @param args the arguments for the message
+     * @return instance of {@link ElasticsearchSecurityException}
+     */
+    static ElasticsearchSecurityException unauthorized(final String message, final Throwable cause, final String outToken,


I'm worried that this method has too many arguments to also have a varargs args.
It will be easy to accidentally pass things in the wrong order.
Maybe it should be split up so that it the authenticate-negotiate handling takes an ElasticsearchSecurityException directly rather than the args to create one.

True, wanted to split but was being lazy. I have split this into two as suggested. Thanks.

tvernum · 2018-07-04T08:24:27Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+
+    @Override
+    public void authenticate(final AuthenticationToken token, final ActionListener<AuthenticationResult> listener) {
+        if (token instanceof KerberosAuthenticationToken) {


I think this should be an assert instead. You should never get passed a token you don't support.

I was referring SamlRealm during implementation and went with that but I do see support checks in AuthenticationService so added an assert. Thank you.

tvernum · 2018-07-04T08:25:58Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+        if (token instanceof KerberosAuthenticationToken) {
+            final KerberosAuthenticationToken kerbAuthnToken = (KerberosAuthenticationToken) token;
+            final Path keytabPath =
+                    config.env().configFile().resolve(KerberosRealmSettings.HTTP_SERVICE_KEYTAB_PATH.get(config.settings()));


Is there a reason to resolve this on every call to authenticate? The setting will never change, so we can resolve & store the Path in the constructor.

No, earlier I was trying to make debug flag dynamic but then Jay's suggestion was to not make it unless required. So yes this can be set once in the constructor, changes done. Thank you.

tvernum · 2018-07-04T08:27:52Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+        }
+    }
+
+    private void buildUser(final Tuple<String, String> userPrincipalNameOutToken, final ActionListener<AuthenticationResult> listener) {


I would prefer that you take 2 separate arguments rather than a Tuple

Thanks, done.

tvernum · 2018-07-04T08:30:02Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+                        } else {
+                            /**
+                             * Ongoing context establishment, terminate with UNAUTHORIZED and outToken
+                             */


This comment isn't very informative. Can you include an explanation about why you're choosing to terminate instead of just calling unsuccessful

Updated the comment with information why terminate than unsuccessful. Thank you.

tvernum · 2018-07-04T08:34:57Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+                            listener.onResponse(AuthenticationResult.terminate(errorMessage,
+                                    unauthorized(errorMessage, null, userPrincipalNameOutToken.v2())));
+                        }
+                    }, (e) -> {


nit brackets

Removed, Thanks.

tvernum · 2018-07-04T08:37:30Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+
+    private void handleException(Exception e, final ActionListener<AuthenticationResult> listener) {
+        if (e instanceof LoginException) {
+            logger.error("failed to authenticate user, service login failure", e);


I don't think we want to log an error everytime a login fails do we?
It seems quite verbose, especially since the AuthenticationService will log this as well.

No, I see we are logging it in AuthenticationService. Removed these log statements.

tvernum · 2018-07-04T08:42:42Z

...in/java/org/elasticsearch/xpack/security/authc/kerberos/support/KerberosTicketValidator.java

+                actionListener.onFailure((GSSException) pve.getCause());
            }
-            throw ExceptionsHelper.convertToRuntime(pve.getException());
+            actionListener.onFailure(ExceptionsHelper.convertToRuntime(pve.getException()));


Do you need to convert it to runtime now that it's not being thrown?

No need, thank you.

Also refactored to break KerberosRealm Tests to avoid big test class.

bizybot · 2018-07-04T16:31:29Z

Hi, @tvernum I have addressed your review comments. I have also split one big test file for KerberosRealm into multiple based on test scenario's. Please take a look at the changes when you get some time. Appreciate your feedback. Thank you.

tvernum

LGTM

tvernum · 2018-07-09T04:23:21Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+                 */
+                containsNegotiateWithToken =
+                        ese.getHeader("WWW-Authenticate").stream()
+                                .anyMatch((s) -> s != null && s.toLowerCase(Locale.ENGLISH).contains("negotiate "));


We typically use Locale.ROOT rather than ENGLISH for case conversion.

Oh, and nit on the unnecessary brackets :)

Thanks, did the change and now configured IDE :)

tvernum · 2018-07-09T04:28:29Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosRealm.java

+            listener.onResponse(AuthenticationResult.success(user));
+        } else {
+            /**
+             * TODO: bizybot AD/LDAP user lookup if lookup realm configured


Quick comment, not a review issue: This TODO is in the wrong spot - you'll want to handle lookup-realms regardless of the cache status.

jaymode

I left a couple of small comments. Other than those this LGTM

jaymode · 2018-07-09T15:33:17Z

...in/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java

+                 */
+                containsNegotiateWithToken =
+                        ese.getHeader("WWW-Authenticate").stream()
+                                .anyMatch(s -> s != null && s.toLowerCase(Locale.ROOT).contains("negotiate "));


rather than use .toLowerCase shouldn't we use regionMatches?

Thanks, addressed this.

jaymode · 2018-07-09T15:37:39Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/InternalRealms.java

+     * The list of all realm types, which are those that have extensive interaction
+     * with third party sources
+     */
+    private static final Set<String> TYPES_DEPEND_ON_THIRD_PARTY_SOURCES =


I'd just remove this and change standard types to explicitly list the types. Collections.unmodifiableSet(NativeRealmSettings.TYPE, FileRealmSettings.TYPE, LdapRealmSettings.AD_TYPE, LdapRealmSettings.LDAP_TYPE, PkiRealmSettings.TYPE);

Thanks, yes it wasn't used anywhere modified it as suggested.

bizybot added >feature :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Jul 3, 2018

bizybot requested review from jaymode and tvernum July 3, 2018 13:41

Yogesh Gaikwad added 3 commits July 4, 2018 01:13

[Kerberos] Remove unused import

38dd7ea

[Kerberos] Address review comments

cbbefd1

Merge branch 'kerberos/sync' into kerberos/addkerbrealm

50729e6

tvernum reviewed Jul 4, 2018

View reviewed changes

[Kerberos] Address review comments from Tim

87af0e2

Also refactored to break KerberosRealm Tests to avoid big test class.

tvernum approved these changes Jul 9, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits July 9, 2018 18:17

[Kerberos] Address review comments

8c9eefe

Merge branch 'kerberos/sync' into kerberos/addkerbrealm

5b2ece1

jaymode approved these changes Jul 9, 2018

View reviewed changes

[Kerberos] Address review comments from Jay

391807b

bizybot merged commit 24a3f16 into elastic:feature/kerberos Jul 10, 2018

bizybot deleted the kerberos/addkerbrealm branch July 10, 2018 01:08

Conversation

bizybot commented Jul 3, 2018

Uh oh!

elasticmachine commented Jul 3, 2018

Uh oh!

tvernum 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!

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!

bizybot commented Jul 4, 2018

Uh oh!

tvernum 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!

jaymode 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development