Add support framework for Kerberos Realm #31023

bizybot · 2018-06-01T06:06:00Z

This change adds the framework to support Kerberos authN in elasticsearch.
ES is the service protected by Kerberos, each ES service node will have its
own keytab. Keytab is the file with Service principal name and encrypted key.
This can be then used to validate the authenticator coming in the request.
This change only adds support for SPNEGO mechanism and uses JGSS.
JVM options -Djava.security.krb5.conf can be used to specify krb5.conf with
additional settings if required.

For Kerberos Realm,

KerberosRealmSettings: Captures settings required for Kerberos
Usually keytab (stored in the config), cache settings and krb debug flag
KerberosAuthenticationToken: Handles extraction of token from request
Extracts the token from request header:
"Authorization: Negotiate "
If any error condition occurs, throws Exception with Rest status 401
Also adds response header "WWW-Authenticate: Negotiate"
KerberosTicketValidator: Used for kerberos ticket validation and
gss context establishment.
On service side, we need to login first, uses Jaas to complete service login.
To avoid more file configurations, we generate the JAAS configuration with
required modules in memory. The token extracted from authnToken is
passed on to GSSContext which uses service credentials (keytab) to verify
the passed token and generates output token. If GSS context is established
it returns tuple of client-username and out token (can be empty). If out token
is present but context is yet not established then it will return tuple with no
username and out token. The out token needs to be returned as response
header 401 and "WWW-Authenticate: Negotiate " for ongoing
negotiation. This will continue till either it fails or successful authentication on
context establishment.
Changes in plugin-security policy to add required permissions
Few settings like Jaas config and kerberos keytab access requires permissions.

For testing,

KerberosTestCase is the base class to start/stop kdc server
and build test settings. SimpleKdcLdapServer(sourced from Hadoop) is a wrapper around
SimpleKdcServer(ApacheDS), which simplifies in memory testing with KDC and uses
in memory ldap server as its backend.

This commit adds framework to support Kerberos authN in elasticsearch. For Kerberos Realm, KerberosRealmSettings: Captures settings required for Kerberos KerberosAuthenticationToken: Handles extraction of token from request KerberosTicketValidator: Used for kerberos ticket validation and gss context establishment. Changes in plugin-security policy to add required permissions For testing, KerberosTestCase is the base class to start/stop kdc server and build test settings. MiniKdc(Hadoop) is a wrapper around SimpleKdcServer(ApacheDS), simplifies in memory testing with KDC.

elasticmachine · 2018-06-01T06:06:01Z

Pinging @elastic/es-security

The change removes MiniKdc dependency and disabling of license check for the same.

bizybot · 2018-06-04T04:35:00Z

@elasticmachine test this, please.

tvernum · 2018-06-04T07:59:12Z

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

+    private String base64EncodedTicket;
+
+    public KerberosAuthenticationToken(final String base64EncodedToken) {
+        this.principalName = UNAUTHENTICATED_PRINCIPAL_NAME;


Why do we have a field that is always set to a constant value (that in itself is extremely generic)?

Yeah, we do not need it. Thanks.

tvernum · 2018-06-04T08:00:02Z

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

+        try {
+            decodedKerberosTicket = Base64.getDecoder().decode(base64Token);
+        } catch (IllegalArgumentException iae) {
+            rootCause = iae;


This flow is pretty weird - can we just handle the exception here?

Addressed this, Thanks.

tvernum · 2018-06-04T08:00:58Z

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

+            throw unauthorized("invalid negotiate authentication header value, could not decode base64 token {}", rootCause,
+                    base64EncodedToken);
+        }
+        return new KerberosAuthenticationToken(base64EncodedToken);


Why do we base64 decode the token, and then throw the result away? Shouldn't this token use the byte[]?

Yes, did not realize I was again decoding in the ticket validator, good catch. Thank you.

tvernum · 2018-06-04T08:03:22Z

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

+
+    private static ElasticsearchSecurityException unauthorized(final String message, final Throwable cause, final Object... args) {
+        ElasticsearchSecurityException ese = new ElasticsearchSecurityException(message, RestStatus.UNAUTHORIZED, cause, args);
+        ese.addHeader(WWW_AUTHENTICATE, NEGOTIATE_AUTH_HEADER);


Is this right? (It might be, I just want to know that we've thought about it).
If we receive an invalid Kerberos token, we request to negotiate again - are you sure that's what we want?

Yes, this is according to RFC2616 https://tools.ietf.org/html/rfc2616#section-10.4.2, though the expectation mentioned is to respond possibly with a different scheme if it was refused for presented credentials. During extraction of the token, we haven't actually tried to validate/authenticate the credentials yet but the header itself was malformed. Looked at the Tomcat Spnego Authenticator, it responds with the same scheme, so I went with the decision.
We could respond with a different scheme but not sure if we should be doing that. For example, Kibana, in this case, would respond with login page along with www-authenticate negotiate header is what I understood from the discussion here

tvernum · 2018-06-04T08:13:36Z

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

+            loginContext = serviceLogin(servicePrincipalName, keyTabPath.toString(), config.settings());
+            // create credentials
+            GSSCredential serviceCreds = createCredentials(servicePrincipalName, nameType, gssManager, loginContext);
+            // create gss context


I don't feel like these comments add anything. They're just repeating what is already clear from the method being called.

True, removed those. Thanks.

tvernum · 2018-06-04T08:14:42Z

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

+                base64OutToken = Base64.getEncoder().encodeToString(outToken);
+            }
+            LOGGER.debug("validateTicket isGSSContextEstablished = {}, username = {}, outToken = {}", gssContext.isEstablished(),
+                    gssContext.getSrcName().toString(), base64OutToken);


This should probably be a trace log.

Change done, Thanks.

tvernum · 2018-06-04T08:16:36Z

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

+                });
+            } catch (PrivilegedActionException e) {
+                RuntimeException rte = ExceptionsHelper.convertToRuntime((Exception) ExceptionsHelper.unwrapCause(e));
+                LOGGER.log(Level.DEBUG, "Could not dispose GSS Context", rte);


We use .debug, not .log

Change done, Thanks.

tvernum · 2018-06-04T08:17:45Z

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

+                });
+            } catch (PrivilegedActionException e) {
+                RuntimeException rte = ExceptionsHelper.convertToRuntime((Exception) ExceptionsHelper.unwrapCause(e));
+                LOGGER.log(Level.DEBUG, "Could not close LoginContext", rte);


Change done, Thanks.

tvernum · 2018-06-04T08:24:12Z

...t/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosAuthenticationTokenTests.java

+            public void describeTo(Description description) {
+            }
+        });
+        KerberosAuthenticationToken.extractToken(threadContext);


Our standard pattern is to use LucenseTestCase.expectThrows.
Is there a reason you're using a Rule instead?

Addressed this. Thank you.

tvernum · 2018-06-04T08:25:43Z

...t/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosAuthenticationTokenTests.java

+    public void testEqualsHashCode() {
+        final KerberosAuthenticationToken kerberosAuthenticationToken = new KerberosAuthenticationToken("base64EncodedToken");
+        EqualsHashCodeTestUtils.checkEqualsAndHashCode(kerberosAuthenticationToken, (original) -> {
+            return new KerberosAuthenticationToken((String) original.credentials());


Can you include a Mutator in this test as well ?

Added additional checks with Mutator, Thank you.

Added few tests, modified KerberosAuthenticationToken to keep decoded bytes. Corresponding changes in KerberosTicketValidator and tests.

This was due to less minimum ticket lifetime set on kdc.

jaymode

I took a first pass and left comments. Please let me know when this is ready for review again.

jaymode · 2018-06-06T15:27:27Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+    public static final String TYPE = "kerberos";
+
+    /**
+     * Kerberos Key tab for Elasticsearch HTTP Service and Kibana HTTP Service<br>


I wouldn't mention kibana here since nothing is specific to kibana

Sure, removed it. Thanks.

jaymode · 2018-06-06T15:27:41Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+     * Uses single key tab for multiple service accounts.
+     */
+    public static final Setting<String> HTTP_SERVICE_KEYTAB_PATH =
+            Setting.simpleString("http.service.keytab.path", Setting.Property.NodeScope);


can we just use keytab.path?

yes, done. Thanks.

jaymode · 2018-06-06T15:28:26Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+     */
+    public static final Setting<String> HTTP_SERVICE_KEYTAB_PATH =
+            Setting.simpleString("http.service.keytab.path", Setting.Property.NodeScope);
+    public static final Setting<Boolean> SETTING_KRB_DEBUG_ENABLE =


this setting should not be dynamic

My understanding of Dynamic is it would not require a restart of the node as these settings can be changed from API. As this is debugging log to enable verbose logging on the node for jvm kerb module, we could turn on and off after capturing the logs was what I thought and made it dynamic. Could you please let me know if this is not advisable for some reason? Thanks.

There needs to be a settings update consumer to handle the update from the update settings api. There is not one in this PR and we can always make this dynamic later on if we find the need. No other realm has dynamic settings either iirc

Removed Dynamic, will need to explore how Dynamic works. Thanks for the pointer.

For dynamic, you need the ClusterSettings instance and have to register a settings update consumer for the setting. Take a look at IPFilter for an example of where we do this in the security code.

jaymode · 2018-06-06T15:29:27Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+    // Cache
+    private static final TimeValue DEFAULT_TTL = TimeValue.timeValueMinutes(20);
+    public static final Setting<TimeValue> CACHE_TTL_SETTING = Setting.timeSetting("cache.ttl", DEFAULT_TTL, Setting.Property.NodeScope);
+    private static final int DEFAULT_MAX_USERS = 100_000; // 100k users


can you put the private static final values together and the public ones together rather than intermixing

done thanks.

jaymode · 2018-06-06T15:30:25Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+            Setting.intSetting("cache.max_users", DEFAULT_MAX_USERS, Setting.Property.NodeScope);
+
+    private KerberosRealmSettings() {
+        /* Empty private constructor */


this comment isn't necessary and doesn't add much. Either remove it or add value by saying this class should never be instantiated.

Removed it. Thanks.

jaymode · 2018-06-06T16:34:36Z

...c/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SimpleKdcLdapServer.java

+ * {@link InMemoryDirectoryServer}.<br>
+ * Starts in memory Ldap server and then uses it as backend for Kdc Server.
+ */
+@SuppressForbidden(reason = "Uses Apache Kdc which requires usage of java.io.File")


do we really need this? Can you point me to where this trips the forbidden check? We should try to limit the suppress forbidden annotations to the smallest amount of code possible and this is on the whole class

I have moved to the methods where it is doing Path.toFile() instead of whole class. Thanks.

jaymode · 2018-06-06T16:38:21Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+        try {
+            oid = new Oid("1.3.6.1.5.5.2");
+        } catch (GSSException gsse) {
+            ExceptionsHelper.convertToRuntime(gsse);


missing throw

Removed this, using the constant from KerberosTicketValidator. Thanks.

jaymode · 2018-06-06T16:38:45Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+        try {
+            oldUseSubjectCredsOnlyFlag = getAndSetSystemProperty("javax.security.auth.useSubjectCredsOnly", "true");
+
+            LOGGER.info("SpnegoClient with princName : {}", userPrincipalName);


s/princName/principal name

Done, Thanks.

jaymode · 2018-06-06T16:40:14Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+        return Base64.getEncoder().encodeToString(outToken);
+    }
+
+    public void close() throws LoginException, GSSException, PrivilegedActionException {


should we make it implement closeable? Also mention that it should be closed in javadocs

Yes, did that, Thanks.

javadocs are missing about needing to be closed

Done, Thanks

jaymode · 2018-06-06T16:40:56Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+    }
+
+    /**
+     * Instead of jaas.conf, this requires refresh of {@link Configuration}.


add complete javadocs

Done, thanks.

This commit addresses review comments. Also during testing found Arabic "ar" locale having problem with SimpleKdcServer so added a check for it. Removed usage of Junit rules, using expectThrows instead. Add missing java docs.

bizybot · 2018-06-08T07:57:53Z

@tvernum @jaymode I have addressed your review comments. I think this is ready for another review.

Important comments that I would want you guys to look at :-

In KerberosAuthenticationToken when throwing ElasticsearchSecurityException for invalid authz token for 'Negotiate' scheme I am setting header 'WWW-Authenticate: Negotiate'. You both had comments around it so reiterating with my reasoning here as it seems important.
According to RFC2616, though the expectation mentioned is to respond possibly with a different scheme if it was refused for presented credentials. During extraction of the token, we haven't actually tried to validate/authenticate the credentials yet but the header itself was malformed for given scheme. Looked at the Tomcat Spnego Authenticator, it also responds with the same scheme, so I went with the decision.
We could respond with a different scheme but not sure if we want to do that.
It would have been good if RFC defined something that lets the client know of all supported auth schemes. Please comment with your thoughts around it if we want to do it differently.

For the comment on adding bootstrap check for Kerberos about keytab exists during startup, I will address it in the next PR.
Thank you.

Other languages that do not work with Apache SimpleKdcServer "ja", "th", "hi". Changing the default during testing.

KdcConfigKey does not specify locale during getProperyKey causing problem loading ldap backend properties. This is workaround as it writes the key in the same locale as expected for that test by using same property key

jaymode

I went through it again and left more feedback. Please let me know when its ready to be reviewed again.

jaymode · 2018-06-08T14:07:55Z

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

@@ -18,43 +17,53 @@
 import java.util.Objects;

 /**
- * Holds on to base 64 encoded ticket, also helps extracting token from
- * {@link ThreadContext}
+ * This class represents AuthenticationToken for Kerberos authentication using


s/represents/represents an

Thanks, done.

jaymode · 2018-06-08T14:09:19Z

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

- * Holds on to base 64 encoded ticket, also helps extracting token from
- * {@link ThreadContext}
+ * This class represents AuthenticationToken for Kerberos authentication using
+ * SPNEGO mechanism. The token stores base 64 decoded token bytes, extracted


s/SPNEGO mechanism/SPNEGO

Thanks, done.

jaymode · 2018-06-13T16:20:06Z

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

+            return null;
+        }
+
+        if (authorizationHeader.regionMatches(IGNORE_CASE_AUTH_HEADER_MATCH, 0, NEGOTIATE_AUTH_HEADER.trim(), 0,


why do we call trim on a constant value? There should be a whitespace after Negotiate for it to be valid

Yes, you are right, we need not do it. actually not sure why I did that and the check should as "(n|N)egotiate ". I have also added the change in UsernamePasswordToken to do the check as "(b|B)asic ". Corrected it. Thank you.

jaymode · 2018-06-13T16:20:43Z

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

+            return null;
+        }
+
+        final String base64EncodedToken = authorizationHeader.substring(NEGOTIATE_AUTH_HEADER.trim().length()).trim();


same thing here with trim. We shouldn't be calling trim all the time on a constant value

Thanks, done.

jaymode · 2018-06-13T16:21:22Z

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

+
+    @Override
+    public String principal() {
+        return "<Unauthenticated Principal Name>";


remove Name

Removed it. Thanks.

jaymode · 2018-06-13T17:40:16Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+/**
+ * This class is used as a Spnego client and handles spnego interactions using
+ * GSS context negotiation.<br>
+ * Not thread safe


why isn't this class thread safe?

It internally uses GSSContext which maintains sequencing for avoiding replay attacks, in this class to verify mutual authentication I am keeping the context available, so it is advisable to not use this instance across threads as GSSContext also mentions this. I have added the java doc for it. I wanted to elaborate on it but forgot to add TODO. Thanks.

jaymode · 2018-06-13T17:40:50Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+        return Base64.getEncoder().encodeToString(outToken);
+    }
+
+    public void close() throws LoginException, GSSException, PrivilegedActionException {


javadocs are missing about needing to be closed

jaymode · 2018-06-13T17:41:18Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+     * @throws LoginException
+     */
+    private static LoginContext loginUsingPassword(final String principal, final SecureString password) throws LoginException {
+        final Set<Principal> principals = new HashSet<>();


use Collections.singleton

Sorry for these ones they were already pointed in earlier review. Thanks.

jaymode · 2018-06-13T17:41:28Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+        final Set<Principal> principals = new HashSet<>();
+        principals.add(new KerberosPrincipal(principal));
+
+        final Subject subject = new Subject(false, principals, new HashSet<Object>(), new HashSet<Object>());


use Collections.emptySet

Sorry for these ones they were already pointed in earlier review. Thanks.

jaymode · 2018-06-13T17:43:06Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

+
+                @Override
+                @SuppressForbidden(
+                        reason = "For testing application provides credentials, needs sys prop javax.security.auth.useSubjectCredsOnly")


This statement is pretty vague. Is this method really only used for a single system property?

Yes just wanted to do it for single sys property, updated the method name and modified it to be used to set reset same property. Thanks.

- Removed unwanted trim() and corrected the check for Negotiate - corrected basic authorization check to be case insensitive - Add missing documentation - Upgrade Apache SimpleKdcServer and its dependencies

bizybot · 2018-06-20T00:29:07Z

Hi, @tvernum @jaymode I have addressed your review comments. I think this is ready for another review. Thank you for your time.

jaymode · 2018-06-19T20:37:05Z

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

@@ -32,8 +30,10 @@

    public static final String WWW_AUTHENTICATE = "WWW-Authenticate";
    public static final String AUTH_HEADER = "Authorization";
-    public static final String NEGOTIATE_AUTH_HEADER = "Negotiate ";
+    public static final String NEGOTIATE_SCHEME_NAME = "Negotiate";
+    public static final String NEGOTIATE_AUTH_HEADER = NEGOTIATE_SCHEME_NAME + " ";


This isn't the header. This is the prefix for the value of the authorization header. Either rename the variable or remove it

Renamed it. Thanks.

jaymode · 2018-06-19T20:38:14Z

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

@@ -173,7 +173,7 @@ private static GSSCredential createCredentials(final GSSManager gssManager,

    /**
     * Privileged wrapper for closing GSSContext, does not throw exceptions but logs
-     * them as warning.
+     * them as debug message.


s/debug/a debug

Changed it, Thanks.

jaymode · 2018-06-19T20:38:27Z

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

@@ -192,7 +192,7 @@ private static void privilegedDisposeNoThrow(final GSSContext gssContext) {

    /**
     * Privileged wrapper for closing LoginContext, does not throw exceptions but
-     * logs them as warning.
+     * logs them as debug message.


s/debug/a debug

Changed it, Thanks.

jaymode · 2018-06-19T20:42:01Z

...rity/src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/SpnegoClient.java

        String retVal = null;
        try {
            retVal = AccessController.doPrivileged(new PrivilegedExceptionAction<String>() {

                @Override
                @SuppressForbidden(
-                        reason = "For testing application provides credentials, needs sys prop javax.security.auth.useSubjectCredsOnly")
+                        reason = "For tests where we provide credentials, need to set reset javax.security.auth.useSubjectCredsOnly")


s/set reset/set and reset

Changed it, Thanks.

jaymode · 2018-06-19T20:43:48Z

...src/main/java/org/elasticsearch/xpack/core/security/authc/support/UsernamePasswordToken.java

+        if (Strings.isNullOrEmpty(headerValue)) {
+            return null;
+        }
+        if (headerValue.regionMatches(IGNORE_CASE_AUTH_HEADER_MATCH, 0, BASIC_AUTH_PREFIX, 0,


this change should be made in a separate PR against master. It is unrelated to kerberos and technically fixes a bug but is hidden here. Lets open up a new PR for this

Yes, just was plain lazy. I will create a bug and a PR to address this. Thanks.

jaymode · 2018-06-19T20:53:38Z

.../src/test/java/org/elasticsearch/xpack/security/authc/kerberos/support/KerberosTestCase.java

+        /*
+         * arabic and other languages have problem due to handling of GeneralizedTime in
+         * SimpleKdcServer For more look at :
+         * org.apache.kerby.asn1.type.Asn1GeneralizedTime#toBytes()


There is non-standard support for UTF-8 as implemented by Microsoft. This is controlled using the sun.security.krb5.msinterop.kstring system property. This was added in JDK7 https://bugs.openjdk.java.net/browse/JDK-2182089 and will become the default in JDK 11 https://bugs.openjdk.java.net/browse/JDK-8200152. From that issue:

In fact, RFC 3961 requires it to be UTF-8 string, and MIT krb5 also uses UTF-8. It's time to change the default to UTF-8. The system property can be kept.

jaymode · 2018-06-20T18:50:18Z

...t/java/org/elasticsearch/xpack/security/authc/kerberos/KerberosAuthenticationTokenTests.java

+        final KerberosAuthenticationToken kerbAuthnToken = KerberosAuthenticationToken.extractToken(authzHeader);
+        assertNotNull(kerbAuthnToken);
+        assertEquals(UNAUTHENTICATED_PRINCIPAL_NAME, kerbAuthnToken.principal());
+        assertTrue(kerbAuthnToken.credentials() instanceof byte[]);


this would be better as assertThat(kerbAuthnToken.credentials(), instanceOf(byte[].class)). This is because the error message will be much better than is false expected true

- Removed basic authentication bug fix, will create a bug and raise PR against that. - Corrected variable name - Corrected statements in comments and javadoc - Readability in tests

case insensitive handling of basic auth header will be addressed in separate PR.

bizybot · 2018-06-22T01:05:46Z

Hi, @jaymode I have addressed your review comments. I think this is ready for another review. Thank you for your time.

tvernum

LGTM, but I left a couple of suggestions

tvernum · 2018-06-22T03:11:59Z

...rc/main/java/org/elasticsearch/xpack/core/security/authc/kerberos/KerberosRealmSettings.java

+        settings.add(CACHE_TTL_SETTING);
+        settings.add(CACHE_MAX_USERS_SETTING);
+        settings.add(SETTING_KRB_DEBUG_ENABLE);
+        return settings;


This is fine - and you don't need to change it, but you can use Sets.newHashSet here, which can be a little more readable.

Thanks, have addressed this.

tvernum · 2018-06-22T03:13:53Z

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

+    // authorization scheme check is case-insensitive
+    private static final boolean IGNORE_CASE_AUTH_HEADER_MATCH = true;
+
+    private final byte[] base64DecodedToken;


It seems strange to name this variable based on the format it used to be in.
I think tokenBytes or even decodedToken would make more sense.

Renamed it. Thanks.

tvernum · 2018-06-22T03:18:07Z

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

+
+    @Override
+    public String principal() {
+        return "<Unauthenticated Principal>";


The way this was done in SAML with "<unauthenticated-saml-user>" has proven to be confusing for users. The log messages report a token for <unauthenticated-saml-user> and they get worried that authentication failed, or something like that.

I think we're probably better of with something like "<Kerberos Token>" instead.

Perhaps we should have a separate PR to allow this to be null, for tokens that don't know their principal yet.

Yeah, it can be confusing, I think null makes sense and then create authenticated token with correct principal once authentication is successful so we have the principal name, where required, say logging etc. I will take it up as separate PR. Thanks.

tvernum · 2018-06-22T03:22:29Z

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

+            if (pve.getCause() instanceof GSSException) {
+                throw (GSSException) pve.getCause();
+            }
+            throw ExceptionsHelper.convertToRuntime((Exception) ExceptionsHelper.unwrapCause(pve));


unwrapCause doesn't do what you think it does.
You just want pve.getException()

I saw the implementation for unwrapCause totally different than what I assumed. As it was taking Throwable assumed must be unwrapping root cause and I can convert to runtime. Thanks. Addressed this.

tvernum · 2018-06-22T03:23:22Z

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

+                    return null;
+                });
+            } catch (PrivilegedActionException e) {
+                LOGGER.debug("Could not dispose GSS Context", (Exception) ExceptionsHelper.unwrapCause(e));


Use e.getException() instead of unwrapCause

Thanks, addressed this.

tvernum · 2018-06-22T03:23:37Z

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

+                    return null;
+                });
+            } catch (PrivilegedActionException e) {
+                LOGGER.debug("Could not close LoginContext", (Exception) ExceptionsHelper.unwrapCause(e));


Use e.getException() instead of unwrapCause

Thanks, addressed this.

tvernum · 2018-06-22T03:25:53Z

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

+     *         closed using {@link LoginContext#logout()} after usage.
+     * @throws PrivilegedActionException when privileged action threw exception
+     */
+    private static LoginContext serviceLogin(final String keytabFilePath, final boolean krbDebug)


Nit: You're inconsistent in whether it's a keytab or a keyTab. I have no opinion, but pick one :)

Thanks, I see that. Some jaas properties refer it as keyTab and some places keytab. I will make it consistent.

jaymode

I left a suggestion. LGTM

jaymode · 2018-06-22T19:09:32Z

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

+        }
+    }
+
+    private String base64Encode(final byte[] outToken) {


can you rename this method? This has specific behavior for an outgoing token; ie check for null/empty

bizybot added >feature WIP :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 labels Jun 1, 2018

bizybot requested review from tvernum and jaymode June 1, 2018 06:06

[Kerberos] Remove MiniKdc dependency

6211040

The change removes MiniKdc dependency and disabling of license check for the same.

tvernum reviewed Jun 4, 2018

View reviewed changes

Yogesh Gaikwad added 8 commits June 5, 2018 08:52

[Kerberos] Address review comments

29ef31c

Added few tests, modified KerberosAuthenticationToken to keep decoded bytes. Corresponding changes in KerberosTicketValidator and tests.

[Kerberos] Fix the test, removed duplicate code

4fe4f54

[Kerberos] Address random test failure

542c41d

This was due to less minimum ticket lifetime set on kdc.

Merge branch 'master' into kerberos/ticketvalidator

778592b

[Kerberos] Line length check failed

067a863

[Kerberos] Did not check the results for javadoc, fixed it.

837532e

Merge branch 'master' into kerberos/ticketvalidator

6c04be8

Merge branch 'master' into kerberos/ticketvalidator

431c73f

jaymode removed WIP v6.4.0 labels Jun 6, 2018

jaymode requested changes Jun 6, 2018

View reviewed changes

Yogesh Gaikwad added 3 commits June 8, 2018 13:34

[Kerberos] Address review comments.

b4f2f00

This commit addresses review comments. Also during testing found Arabic "ar" locale having problem with SimpleKdcServer so added a check for it. Removed usage of Junit rules, using expectThrows instead. Add missing java docs.

[Kerberos] Method rename, close ldapconn after usage

9e11414

[Kerberos] reverting change done to make ticket validator final

27440a0

Yogesh Gaikwad added 4 commits June 9, 2018 10:23

[Kerberos] Add unsupported languages during test

ad13149

Other languages that do not work with Apache SimpleKdcServer "ja", "th", "hi". Changing the default during testing.

Merge branch 'master' into kerberos/ticketvalidator

891a60b

Merge branch 'master' into kerberos/ticketvalidator

e18d732

jaymode requested changes Jun 13, 2018

View reviewed changes

Yogesh Gaikwad added 5 commits June 15, 2018 14:11

[Kerberos] Address review comments from Jay

9fdf182

- Removed unwanted trim() and corrected the check for Negotiate - corrected basic authorization check to be case insensitive - Add missing documentation - Upgrade Apache SimpleKdcServer and its dependencies

[Kerberos] Forgot to add one changed file

b8f68e1

Merge branch 'master' into kerberos/ticketvalidator

4839543

[Kerberos] Address review comment

2db7f0c

[Kerberos] Remove extra full stop in comment.

5f142f1

[Kerberos] Update java doc to close SpnegoClient after usage

6bdc904

jaymode requested changes Jun 20, 2018

View reviewed changes

Yogesh Gaikwad added 5 commits June 21, 2018 07:54

[Kerberos] Address review comments.

fd02cbe

- Removed basic authentication bug fix, will create a bug and raise PR against that. - Corrected variable name - Corrected statements in comments and javadoc - Readability in tests

[Kerberos] Revert UsernamePasswordTokenTests

72d33f0

case insensitive handling of basic auth header will be addressed in separate PR.

[Kerberos] Revert exactly as it was

b256f11

Merge branch 'master' into kerberos/ticketvalidator

b8c2c6a

[Kerberos] Correct javadoc

1c83bfb

tvernum approved these changes Jun 22, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits June 22, 2018 14:45

[Kerberos] Address review comments from Tim

28e7fe4

[Kerberos] Fix the failing test

f70d0b7

jaymode approved these changes Jun 22, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits June 23, 2018 17:23

[Kerberos] Address review comments

a324de1

Merge branch 'master' into kerberos/ticketvalidator

0d37491

bizybot merged commit 52d7701 into elastic:feature/kerberos Jun 23, 2018

bizybot deleted the kerberos/ticketvalidator branch June 23, 2018 15:30

Add support framework for Kerberos Realm #31023

Add support framework for Kerberos Realm #31023

Conversation

bizybot commented Jun 1, 2018 • edited Loading

elasticmachine commented Jun 1, 2018

bizybot commented Jun 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot commented Jun 8, 2018

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot Jun 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot commented Jun 20, 2018

Choose a reason for hiding this comment

bizybot commented Jun 1, 2018 •

edited

Loading

bizybot Jun 4, 2018 •

edited

Loading

bizybot Jun 4, 2018 •

edited

Loading

bizybot Jun 15, 2018 •

edited

Loading