[ https://issues.apache.org/jira/browse/HADOOP-11717?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14365135#comment-14365135 ]
Kai Zheng commented on HADOOP-11717: ------------------------------------ More comments to complete the review, mainly about tests. 1. The new handler looks little heavier to me. One thing we can do for now is to have a utility class like {{JwtTokenUtil}} and remove at least {{getPublicKey}} related logic and variables there. So some related tests like {{testValidPEM}} will not need a handler instance. How about having {{parsePublicKey}} (instead of {{getPublicKey}}), {{parseAudiences}}, and {{parseJwtToken}} (even trivial, restore JWT from a string like from cookie) for the new utility class if it sounds better to have ? 2. It would be good not to couple the new test with {{KerberosSecurityTestcase}} since all the test cases won't relate to Kerberos at all. 3. For all the handler really logic tests, better to move the following to {{setup}} or {{teardown}}. {code} JWTRedirectAuthenticationHandler handler = new JWTRedirectAuthenticationHandler(); ... handler.destroy(); {code} 4. All the handler logic tests are different in token preparing. It's possible to have the following in a function where token is a parameter to avoid repeating. {code} + Properties props = getProperties(); + handler.init(props); + + SignedJWT jwt = getJWT("bob", new Date(new Date().getTime() + 5000), + privateKey); + + Cookie cookie = new Cookie("hadoop-jwt", jwt.serialize()); + HttpServletRequest request = Mockito.mock(HttpServletRequest.class); + Mockito.when(request.getCookies()).thenReturn(new Cookie[] { cookie }); + Mockito.when(request.getRequestURL()).thenReturn( + new StringBuffer(SERVICE_URL)); + HttpServletResponse response = Mockito.mock(HttpServletResponse.class); + Mockito.when(response.encodeRedirectURL(SERVICE_URL)).thenReturn( + SERVICE_URL); + + AuthenticationToken token = handler.alternateAuthenticate(request, + response); {code} 5. In the tests, we have repeated values like "bar", "bob" here and there. How about having variables for them ? 6. In the following codes, {{aud}} and {{sigInput}} aren't really used. {code} + List<String> aud = new ArrayList<String>(); + aud.add("bar"); + claimsSet.setAudience("bar"); + + JWSHeader header = new JWSHeader.Builder(JWSAlgorithm.RS256).build(); + + SignedJWT signedJWT = new SignedJWT(header, claimsSet); + Base64URL sigInput = Base64URL.encode(signedJWT.getSigningInput()); + JWSSigner signer = new RSASSASigner(privateKey); + + signedJWT.sign(signer); + + return signedJWT; {code} > Add Redirecting WebSSO behavior with JWT Token in Hadoop Auth > ------------------------------------------------------------- > > Key: HADOOP-11717 > URL: https://issues.apache.org/jira/browse/HADOOP-11717 > Project: Hadoop Common > Issue Type: Improvement > Components: security > Reporter: Larry McCay > Assignee: Larry McCay > Attachments: HADOOP-11717-1.patch, HADOOP-11717-2.patch, > HADOOP-11717-3.patch, HADOOP-11717-4.patch, HADOOP-11717-5.patch > > > Extend AltKerberosAuthenticationHandler to provide WebSSO flow for UIs. > The actual authentication is done by some external service that the handler > will redirect to when there is no hadoop.auth cookie and no JWT token found > in the incoming request. > Using JWT provides a number of benefits: > * It is not tied to any specific authentication mechanism - so buys us many > SSO integrations > * It is cryptographically verifiable for determining whether it can be trusted > * Checking for expiration allows for a limited lifetime and window for > compromised use > This will introduce the use of nimbus-jose-jwt library for processing, > validating and parsing JWT tokens. -- This message was sent by Atlassian JIRA (v6.3.4#6332)