ISSUE:569: Support Kerberos credentials based Basic Authentication #567

guruchai · 2019-07-03T16:54:59Z

Fixes #569

raju-saravanan · 2019-07-04T06:26:01Z

@guruchai: Can you create an ISSUE for this and explain what is being done in this PR?

guruchai · 2019-07-04T06:56:25Z

@guruchai: Can you create an ISSUE for this and explain what is being done in this PR?

Done. Thanks!

kamalcph

overall looks good to me. Left some minor comments to address.

kamalcph · 2019-07-08T09:33:28Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

@@ -0,0 +1,204 @@
+package com.hortonworks.registries.auth.server;


license is missing. Add license for all the newly added files.

kamalcph · 2019-07-08T09:34:38Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+    public static final String AUTHORIZATION_HEADER = "Authorization";
+    public static final String BASIC_AUTHENTICATION = "Basic";
+
+    private static final Logger LOG = LoggerFactory.getLogger(KerberosAuthenticationHandler.class);


Update the class name in the logger.

kamalcph · 2019-07-08T09:42:26Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+        }
+
+        if (LOG.isDebugEnabled()) {
+            LOG.debug("Ran provider.authenticate() and returned authentication for " +


Whether to check authentication is null before printing the log?

kamalcph · 2019-07-08T09:56:10Z

...test/java/com/hortonworks/registries/auth/server/TestKerberosBasicAuthenticationHandler.java

+    }
+
+    @Test
+    public void testRequestWithInvalidAuthorization() throws Exception {


testRequestWithInvalidAuthorization and testRequestWithInvalidCredentials looks similar.

Not exactly. testRequestWithInvalidAuthorization tests the invalidity of Authorization header. testRequestWithInvalidCredentials tests the invalidity of the credentials after validating the Authorization header. They are like 2 steps in parsing step.

kamalcph · 2019-07-08T10:00:27Z

...test/java/com/hortonworks/registries/auth/server/TestKerberosBasicAuthenticationHandler.java

+        AuthenticationToken authToken = handler.authenticate(request, response);
+
+        if (authToken != null) {
+            Assert.fail("AuthToken should have been null if the SPNEGO is disabled in KerberosAuthenticationLoginHandler.");


KerberosAuthenticationLoginHandler -> KerberosBasicAuthenticationHandler

guruchai · 2019-07-08T13:23:59Z

Thanks for the review, @kamalcph!! made the changes suggested.

kamalcph

some more minor comments.

kamalcph · 2019-07-08T13:43:44Z

...test/java/com/hortonworks/registries/auth/server/TestKerberosBasicAuthenticationHandler.java

@@ -0,0 +1,302 @@
+package com.hortonworks.registries.auth.server;


license is missing.

kamalcph · 2019-07-08T13:44:00Z

...test/java/com/hortonworks/registries/auth/server/TestKerberosBasicAuthenticationHandler.java

+    }
+
+    @Test
+    public void testRequestWithInvalidAuthorization() throws Exception {


kamalcph · 2019-07-08T13:44:20Z

common-auth/src/main/java/com/hortonworks/registries/auth/util/Utils.java

+                }
+            }
+            return true;
+        } else {


else block can be removed.

raju-saravanan

Overall looks good. Left some comments.

raju-saravanan · 2019-07-08T15:08:58Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+ */
+public class KerberosBasicAuthenticationHandler extends KerberosAuthenticationHandler {
+
+    public static final String LOGIN_ENABLED_CONFIG = "login.enabled";


Can you mention these props in the registry .yaml under the "servletFilters" section?

raju-saravanan · 2019-07-08T15:17:16Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+    public static final String BASIC_AUTHENTICATION = "Basic";
+
+    private static final Logger LOG = LoggerFactory.getLogger(KerberosBasicAuthenticationHandler.class);
+    private static final String METHOD = "POST";


Can we rename this to "HTTP_LOGIN_METHOD" or more something meaningful?

raju-saravanan · 2019-07-08T15:21:05Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+        try {
+            provider = new KerberosAuthenticationProvider();
+            SunJaasKerberosClient client = new SunJaasKerberosClient();
+            client.setDebug(true);


I am not sure how much logs the client will generate in debug mode. Can we enable this if debug mode is enabled for LOG?

raju-saravanan · 2019-07-08T15:22:31Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+        }
+        String credentials = authorization.split(BASIC_AUTHENTICATION)[1].trim();
+        byte[] credentialsArray = Base64.getDecoder().decode(credentials);
+        String[] userPassword = new String(credentialsArray, StandardCharsets.UTF_8).split(":");


Can we rename this to "principalAndPassword"? I was bit confused when rawPrincipal was extracted from "userPassword".

raju-saravanan · 2019-07-08T15:32:45Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+        return identity;
+    }
+
+    static class KerberosUserDetailsService implements UserDetailsService {


Do we need static keyword here?

static nested classes are usually preferred if the class is not using any of the fields from the enclosing class.

Agreed. I hurried up. Did not pay much attention. Considering no access to enclosing class, static should have been finer for this case!

raju-saravanan · 2019-07-08T15:44:32Z

...src/main/java/com/hortonworks/registries/auth/server/KerberosBasicAuthenticationHandler.java

+ * <li>login.enabled: a boolean string to indicate whether the enabling of Kerberos login.</li>
+ * </ul>
+ */
+public class KerberosBasicAuthenticationHandler extends KerberosAuthenticationHandler {


Can you talk about this authentication method in security.rst under docs pacakge ?

guruchai · 2019-07-09T09:24:37Z

Made the changes.
@raju-saravanan , @kamalcph, can we merge the PR if things look good?

kamalcph

LGTM.

raju-saravanan

+1 LGTM, Thanks @guruchai for this PR.

) * Bumping the version * CSP-51: Auth changes to support credentials based Kerberos Login * Changing nexus repo from http to https as http is not active * Added few comments and modified the class name * Addressed review comments * Addressed review comments and added some documentation

Bumping the version

2fe9f51

guruchai force-pushed the kerberos_login branch from 0c1211a to ba23d7d Compare July 3, 2019 18:12

CSP-51: Auth changes to support credentials based Kerberos Login

1c642ed

guruchai force-pushed the kerberos_login branch from ba23d7d to 1c642ed Compare July 3, 2019 18:21

Changing nexus repo from http to https as http is not active

ade1ee9

guruchai requested a review from raju-saravanan July 4, 2019 03:49

guruchai changed the title ~~Kerberos login~~ ISSUE:569: Kerberos login Jul 4, 2019

guruchai changed the title ~~ISSUE:569: Kerberos login~~ ISSUE:569: Support Kerberos credentials based Authentication Jul 4, 2019

guruchai changed the title ~~ISSUE:569: Support Kerberos credentials based Authentication~~ ISSUE:569: Support Kerberos credentials based Basic Authentication Jul 4, 2019

Added few comments and modified the class name

c045c5a

guruchai force-pushed the kerberos_login branch from fa4e134 to c045c5a Compare July 8, 2019 07:04

guruchai requested a review from kamalcph July 8, 2019 08:40

kamalcph reviewed Jul 8, 2019

View reviewed changes

Addressed review comments

d69c7ab

kamalcph reviewed Jul 8, 2019

View reviewed changes

raju-saravanan reviewed Jul 8, 2019

View reviewed changes

Addressed review comments and added some documentation

370b8b6

kamalcph approved these changes Jul 9, 2019

View reviewed changes

raju-saravanan approved these changes Jul 9, 2019

View reviewed changes

raju-saravanan merged commit 6e9b463 into master Jul 9, 2019

guruchai deleted the kerberos_login branch September 12, 2019 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE:569: Support Kerberos credentials based Basic Authentication #567

ISSUE:569: Support Kerberos credentials based Basic Authentication #567

guruchai commented Jul 3, 2019 •

edited by raju-saravanan

Loading

raju-saravanan commented Jul 4, 2019

guruchai commented Jul 4, 2019

kamalcph left a comment

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

guruchai Jul 8, 2019

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

guruchai commented Jul 8, 2019

kamalcph left a comment

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

kamalcph Jul 8, 2019

raju-saravanan left a comment

raju-saravanan Jul 8, 2019

raju-saravanan Jul 8, 2019

raju-saravanan Jul 8, 2019

raju-saravanan Jul 8, 2019

raju-saravanan Jul 8, 2019

kamalcph Jul 9, 2019

guruchai Jul 9, 2019

raju-saravanan Jul 8, 2019

guruchai commented Jul 9, 2019

kamalcph left a comment

raju-saravanan left a comment

		@@ -0,0 +1,204 @@
		package com.hortonworks.registries.auth.server;

		@@ -0,0 +1,302 @@
		package com.hortonworks.registries.auth.server;

ISSUE:569: Support Kerberos credentials based Basic Authentication #567

ISSUE:569: Support Kerberos credentials based Basic Authentication #567

Conversation

guruchai commented Jul 3, 2019 • edited by raju-saravanan Loading

raju-saravanan commented Jul 4, 2019

guruchai commented Jul 4, 2019

kamalcph 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

guruchai commented Jul 8, 2019

kamalcph 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

raju-saravanan 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

guruchai commented Jul 9, 2019

kamalcph left a comment

Choose a reason for hiding this comment

raju-saravanan left a comment

Choose a reason for hiding this comment

guruchai commented Jul 3, 2019 •

edited by raju-saravanan

Loading