From d9ad51fe5a7ff7b922a2cbc7b49a292d4e3b6fff Mon Sep 17 00:00:00 2001 From: Stefan Negrus Date: Wed, 27 Jun 2018 11:25:31 +0100 Subject: [PATCH 1/4] Check header look like JWT before processing --- .../auth/http/BearerTokenLoggingFilter.java | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java b/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java index 70fd8d3a..100949fb 100644 --- a/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java +++ b/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java @@ -17,6 +17,7 @@ package com.palantir.tokens.auth.http; import com.palantir.tokens.auth.AuthHeader; +import com.palantir.tokens.auth.BearerToken; import com.palantir.tokens.auth.UnverifiedJsonWebToken; import javax.annotation.Priority; import javax.ws.rs.Priorities; @@ -51,15 +52,17 @@ public final void filter(ContainerRequestContext requestContext) { return; } - try { - UnverifiedJsonWebToken jwt = UnverifiedJsonWebToken.of( - AuthHeader.valueOf(rawAuthHeader).getBearerToken()); + if (hasJwtStructure(rawAuthHeader)) { + try { + UnverifiedJsonWebToken jwt = UnverifiedJsonWebToken.of( + AuthHeader.valueOf(rawAuthHeader).getBearerToken()); - setUnverifiedContext(requestContext, USER_ID_KEY, jwt.getUnverifiedUserId()); - jwt.getUnverifiedSessionId().ifPresent(s -> setUnverifiedContext(requestContext, SESSION_ID_KEY, s)); - jwt.getUnverifiedTokenId().ifPresent(s -> setUnverifiedContext(requestContext, TOKEN_ID_KEY, s)); - } catch (Throwable t) { - log.debug("Unable to process auth header.", t); + setUnverifiedContext(requestContext, USER_ID_KEY, jwt.getUnverifiedUserId()); + jwt.getUnverifiedSessionId().ifPresent(s -> setUnverifiedContext(requestContext, SESSION_ID_KEY, s)); + jwt.getUnverifiedTokenId().ifPresent(s -> setUnverifiedContext(requestContext, TOKEN_ID_KEY, s)); + } catch (Throwable t) { + log.debug("Unable to process auth header.", t); + } } } @@ -69,6 +72,13 @@ private void clearMdc() { MDC.remove(TOKEN_ID_KEY); } + /** + * Based on the structure check from {@link UnverifiedJsonWebToken#of(BearerToken)}. + */ + private boolean hasJwtStructure(String rawAuthHeader) { + return rawAuthHeader.split("\\.").length == 3; + } + private void setUnverifiedContext(ContainerRequestContext requestContext, String key, String value) { MDC.put(key, value); requestContext.setProperty(getRequestPropertyKey(key), value); From 65b040bf1accc700af416ecbb2743070e8778dcb Mon Sep 17 00:00:00 2001 From: Stefan Negrus Date: Wed, 27 Jun 2018 14:41:02 +0100 Subject: [PATCH 2/4] Add "tryParse" method and logger to UnverifiedJsonWebToken Update the logic in the filter --- .../auth/http/BearerTokenLoggingFilter.java | 28 +++++-------------- .../tokens/auth/UnverifiedJsonWebToken.java | 19 +++++++++++++ 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java b/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java index 100949fb..83476b5d 100644 --- a/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java +++ b/auth-tokens-filter/src/main/java/com/palantir/tokens/auth/http/BearerTokenLoggingFilter.java @@ -16,9 +16,8 @@ package com.palantir.tokens.auth.http; -import com.palantir.tokens.auth.AuthHeader; -import com.palantir.tokens.auth.BearerToken; import com.palantir.tokens.auth.UnverifiedJsonWebToken; +import java.util.Optional; import javax.annotation.Priority; import javax.ws.rs.Priorities; import javax.ws.rs.container.ContainerRequestContext; @@ -52,18 +51,12 @@ public final void filter(ContainerRequestContext requestContext) { return; } - if (hasJwtStructure(rawAuthHeader)) { - try { - UnverifiedJsonWebToken jwt = UnverifiedJsonWebToken.of( - AuthHeader.valueOf(rawAuthHeader).getBearerToken()); - - setUnverifiedContext(requestContext, USER_ID_KEY, jwt.getUnverifiedUserId()); - jwt.getUnverifiedSessionId().ifPresent(s -> setUnverifiedContext(requestContext, SESSION_ID_KEY, s)); - jwt.getUnverifiedTokenId().ifPresent(s -> setUnverifiedContext(requestContext, TOKEN_ID_KEY, s)); - } catch (Throwable t) { - log.debug("Unable to process auth header.", t); - } - } + Optional parsedJwt = UnverifiedJsonWebToken.tryParse(rawAuthHeader); + parsedJwt.ifPresent(jwt -> { + setUnverifiedContext(requestContext, USER_ID_KEY, jwt.getUnverifiedUserId()); + jwt.getUnverifiedSessionId().ifPresent(s -> setUnverifiedContext(requestContext, SESSION_ID_KEY, s)); + jwt.getUnverifiedTokenId().ifPresent(s -> setUnverifiedContext(requestContext, TOKEN_ID_KEY, s)); + }); } private void clearMdc() { @@ -72,13 +65,6 @@ private void clearMdc() { MDC.remove(TOKEN_ID_KEY); } - /** - * Based on the structure check from {@link UnverifiedJsonWebToken#of(BearerToken)}. - */ - private boolean hasJwtStructure(String rawAuthHeader) { - return rawAuthHeader.split("\\.").length == 3; - } - private void setUnverifiedContext(ContainerRequestContext requestContext, String key, String value) { MDC.put(key, value); requestContext.setProperty(getRequestPropertyKey(key), value); diff --git a/auth-tokens/src/main/java/com/palantir/tokens/auth/UnverifiedJsonWebToken.java b/auth-tokens/src/main/java/com/palantir/tokens/auth/UnverifiedJsonWebToken.java index 05afe439..5b9ba46b 100644 --- a/auth-tokens/src/main/java/com/palantir/tokens/auth/UnverifiedJsonWebToken.java +++ b/auth-tokens/src/main/java/com/palantir/tokens/auth/UnverifiedJsonWebToken.java @@ -27,6 +27,8 @@ import java.util.Optional; import java.util.UUID; import org.immutables.value.Value; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Represents the parsed form of a JWT but does not verify the token signature. @@ -46,6 +48,8 @@ public abstract class UnverifiedJsonWebToken { .registerModule(new Jdk8Module()) .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); + private static final Logger log = LoggerFactory.getLogger(UnverifiedJsonWebToken.class); + /** * Returns the unverified user id, i.e., the "sub" (subject) field of the JWT. */ @@ -66,6 +70,21 @@ public abstract class UnverifiedJsonWebToken { @Value.Parameter public abstract Optional getUnverifiedTokenId(); + /** + * Does a lower cost check on the structure of string provided + * before attempting to create an {@link UnverifiedJsonWebToken}. + */ + public static Optional tryParse(String rawAuthHeader) { + if (rawAuthHeader.chars().filter(x -> x == '.').count() == 2) { + try { + return Optional.of(of(AuthHeader.valueOf(rawAuthHeader).getBearerToken())); + } catch (Throwable t) { + log.debug("Unable to process auth header.", t); + } + } + return Optional.empty(); + } + /** * Attempts to create an {@link UnverifiedJsonWebToken} from provided {@link BearerToken}. *

From 57ec6ba0c4b1c2bd89dd3a71872918c6a654283c Mon Sep 17 00:00:00 2001 From: Stefan Negrus Date: Wed, 27 Jun 2018 14:53:28 +0100 Subject: [PATCH 3/4] Add tests --- .../auth/UnverifiedJsonWebTokenTests.java | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java b/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java index 32810746..a76feacd 100644 --- a/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java +++ b/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java @@ -58,17 +58,25 @@ public void testAsJwt_validJwtFromSessionToken() { @Test public void testAsJwt_validJwtFromApiToken() { UnverifiedJsonWebToken token = UnverifiedJsonWebToken.of(API_TOKEN); - assertEquals(USERID, token.getUnverifiedUserId()); - assertEquals(Optional.empty(), token.getUnverifiedSessionId()); - assertEquals(Optional.of(TOKEN_ID), token.getUnverifiedTokenId()); + assertValidToken(token); } @Test public void testAsJwt_validJwtFromProxyToken() { UnverifiedJsonWebToken token = UnverifiedJsonWebToken.of(PROXY_TOKEN); - assertEquals(USERID, token.getUnverifiedUserId()); - assertEquals(Optional.empty(), token.getUnverifiedSessionId()); - assertEquals(Optional.of(TOKEN_ID), token.getUnverifiedTokenId()); + assertValidToken(token); + } + + @Test + public void testAsJwt_validJwtFromParsedToken() { + Optional token = UnverifiedJsonWebToken.tryParse(PROXY_TOKEN.getToken()); + assertValidToken(token.get()); + } + + @Test + public void invalidJwt_parseReturnsEmpty() { + Optional parsedJwt = UnverifiedJsonWebToken.tryParse(INVALID_BEARER_TOKEN.getToken()); + assertEquals(parsedJwt, Optional.empty()); } @Test @@ -90,4 +98,10 @@ public void invalidJwt_invalidPayloadToken() { assertEquals("Invalid JWT: cannot parse payload", e.getMessage()); } } + + private void assertValidToken(UnverifiedJsonWebToken token) { + assertEquals(USERID, token.getUnverifiedUserId()); + assertEquals(Optional.empty(), token.getUnverifiedSessionId()); + assertEquals(Optional.of(TOKEN_ID), token.getUnverifiedTokenId()); + } } From 5397c8851793871449fd0ad58c416fe5d41a2fd8 Mon Sep 17 00:00:00 2001 From: Stefan Negrus Date: Tue, 3 Jul 2018 14:38:36 +0100 Subject: [PATCH 4/4] Rename method, add missing test (address CR comments) --- .../tokens/auth/UnverifiedJsonWebTokenTests.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java b/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java index a76feacd..ded042c2 100644 --- a/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java +++ b/auth-tokens/src/test/java/com/palantir/tokens/auth/UnverifiedJsonWebTokenTests.java @@ -58,19 +58,19 @@ public void testAsJwt_validJwtFromSessionToken() { @Test public void testAsJwt_validJwtFromApiToken() { UnverifiedJsonWebToken token = UnverifiedJsonWebToken.of(API_TOKEN); - assertValidToken(token); + assertValidApiToken(token); } @Test public void testAsJwt_validJwtFromProxyToken() { UnverifiedJsonWebToken token = UnverifiedJsonWebToken.of(PROXY_TOKEN); - assertValidToken(token); + assertValidApiToken(token); } @Test public void testAsJwt_validJwtFromParsedToken() { Optional token = UnverifiedJsonWebToken.tryParse(PROXY_TOKEN.getToken()); - assertValidToken(token.get()); + assertValidApiToken(token.get()); } @Test @@ -79,6 +79,12 @@ public void invalidJwt_parseReturnsEmpty() { assertEquals(parsedJwt, Optional.empty()); } + @Test + public void invalidJwt_parseReturnsEmpty_validStructure() { + Optional parsedJwt = UnverifiedJsonWebToken.tryParse(INVALID_PAYLOAD_TOKEN.getToken()); + assertEquals(parsedJwt, Optional.empty()); + } + @Test public void invalidJwt_invalidNumberOfSegments() { try { @@ -99,7 +105,7 @@ public void invalidJwt_invalidPayloadToken() { } } - private void assertValidToken(UnverifiedJsonWebToken token) { + private void assertValidApiToken(UnverifiedJsonWebToken token) { assertEquals(USERID, token.getUnverifiedUserId()); assertEquals(Optional.empty(), token.getUnverifiedSessionId()); assertEquals(Optional.of(TOKEN_ID), token.getUnverifiedTokenId());