knox-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From pzamp...@apache.org
Subject [knox] branch master updated: KNOX-2566 - JWT Token Signature Verification Caching NPE (#427)
Date Thu, 01 Apr 2021 20:16:50 GMT
This is an automated email from the ASF dual-hosted git repository.

pzampino pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/knox.git


The following commit(s) were added to refs/heads/master by this push:
     new bcce97b  KNOX-2566 - JWT Token Signature Verification Caching NPE (#427)
bcce97b is described below

commit bcce97b7b83d0086bba113321c8bfff50f1b3e8a
Author: Phil Zampino <pzampino@apache.org>
AuthorDate: Thu Apr 1 16:16:44 2021 -0400

    KNOX-2566 - JWT Token Signature Verification Caching NPE (#427)
---
 .../provider/federation/jwt/JWTMessages.java       |  4 ++
 .../federation/jwt/filter/AbstractJWTFilter.java   | 28 +++++---
 .../federation/jwt/filter/JWTFederationFilter.java |  3 +-
 .../provider/federation/AbstractJWTFilterTest.java | 79 +++++++++++++++++++---
 .../JWTAsHTTPBasicCredsFederationFilterTest.java   |  3 +-
 ...okenIDAsHTTPBasicCredsFederationFilterTest.java | 32 +++++++--
 6 files changed, 121 insertions(+), 28 deletions(-)

diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
index 688a2ab..8a01811 100644
--- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
+++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
@@ -69,4 +69,8 @@ public interface JWTMessages {
             text = "The configuration value ({0}) for maximum token verification cache is
invalid; Using the default value." )
   void invalidVerificationCacheMaxConfiguration(String value);
 
+  @Message( level = MessageLevel.ERROR,
+            text = "Missing token passcode." )
+  void missingTokenPasscode();
+
 }
diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
index 2e37526..45a3ce6 100644
--- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
+++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
@@ -357,10 +357,12 @@ public abstract class AbstractJWTFilter implements Filter {
         } else {
           log.tokenHasExpired(displayableToken, displayableTokenId);
 
-          // Explicitly evict the record of this token's signature verification (if present).
-          // There is no value in keeping this record for expired tokens, and explicitly
removing them may prevent
-          // records for other valid tokens from being prematurely evicted from the cache.
-          removeSignatureVerificationRecord(tokenId);
+          if (tokenId != null) {
+            // Explicitly evict the record of this token's signature verification (if present).
+            // There is no value in keeping this record for expired tokens, and explicitly
removing them may prevent
+            // records for other valid tokens from being prematurely evicted from the cache.
+            removeSignatureVerificationRecord(tokenId);
+          }
 
           handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
                                 "Bad request: token has expired");
@@ -385,12 +387,18 @@ public abstract class AbstractJWTFilter implements Filter {
 
     if (tokenStateService != null) {
       try {
-        if (tokenIsStillValid(tokenId)) {
-          return true;
+        if (tokenId != null) {
+          if (tokenIsStillValid(tokenId)) {
+            return true;
+          } else {
+            log.tokenHasExpired(Tokens.getTokenIDDisplayText(tokenId));
+            handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
+                    "Bad request: token has expired");
+          }
         } else {
-          log.tokenHasExpired(Tokens.getTokenIDDisplayText(tokenId));
+          log.missingTokenPasscode();
           handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
-                                "Bad request: token has expired");
+                                "Bad request: missing token passcode.");
         }
       } catch (UnknownTokenException e) {
         log.unableToVerifyExpiration(e);
@@ -407,7 +415,7 @@ public abstract class AbstractJWTFilter implements Filter {
     String tokenId = TokenUtils.getTokenId(token);
 
     // Check if the token has already been verified
-    verified = hasSignatureBeenVerified(tokenId);
+    verified = (tokenId != null) && hasSignatureBeenVerified(tokenId);
 
     // If it has not yet been verified, then perform the verification now
     if (!verified) {
@@ -436,7 +444,7 @@ public abstract class AbstractJWTFilter implements Filter {
         }
       }
 
-      if (verified) { // If successful, record the verification for future reference
+      if (verified && tokenId != null) { // If successful, record the verification
for future reference
         recordSignatureVerification(tokenId);
       }
     }
diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java
b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java
index a8d50df..aa82062 100644
--- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java
+++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java
@@ -167,8 +167,9 @@ public class JWTFederationFilter extends AbstractJWTFilter {
       final String credentials = new String(credDecoded, StandardCharsets.UTF_8);
       final String[] values = credentials.split(":", 2);
       String username = values[0];
+      String passcode = values[1].isEmpty() ? null : values[1];
       if (TOKEN.equalsIgnoreCase(username) || PASSCODE.equalsIgnoreCase(username)) {
-          parsed = Pair.of(TOKEN.equalsIgnoreCase(username) ? TokenType.JWT : TokenType.Passcode,
values[1]);
+          parsed = Pair.of(TOKEN.equalsIgnoreCase(username) ? TokenType.JWT : TokenType.Passcode,
passcode);
       }
 
       return parsed;
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/AbstractJWTFilterTest.java
b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/AbstractJWTFilterTest.java
index 6d71a18..16b1811 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/AbstractJWTFilterTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/AbstractJWTFilterTest.java
@@ -64,6 +64,7 @@ import java.security.cert.Certificate;
 import java.security.interfaces.RSAPrivateKey;
 import java.security.interfaces.RSAPublicKey;
 import java.text.MessageFormat;
+import java.time.Instant;
 import java.util.Date;
 import java.util.Enumeration;
 import java.util.Locale;
@@ -115,6 +116,50 @@ public abstract class AbstractJWTFilterTest  {
     handler.destroy();
   }
 
+  /**
+   * KNOX-2566
+   */
+  @Test
+  public void testJWTWithoutKnoxUUIDClaim() throws Exception {
+    doTestJWTWithoutKnoxUUIDClaim(Instant.now().toEpochMilli() + 5000);
+  }
+
+  /**
+   * KNOX-2566
+   * Covers the explicit removal of signature verification cache records for expired tokens.
+   */
+  @Test
+  public void testExpiredJWTWithoutKnoxUUIDClaim() throws Exception {
+    doTestJWTWithoutKnoxUUIDClaim(Instant.now().toEpochMilli() - 5000);
+  }
+
+  private void doTestJWTWithoutKnoxUUIDClaim(final long expiration) throws Exception {
+    Properties props = getProperties();
+    handler.init(new TestFilterConfig(props));
+
+    SignedJWT jwt = getJWT(AbstractJWTFilter.JWT_DEFAULT_ISSUER,
+                           "alice",
+                           "bar",
+                           new Date(expiration),
+                           new Date(),
+                           privateKey,
+                           JWSAlgorithm.RS256.getName(),
+                           null); // null knox ID so the claim will be omitted from the token
+
+    HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class);
+    setTokenOnRequest(request, jwt);
+
+    EasyMock.expect(request.getRequestURL()).andReturn(new StringBuffer(SERVICE_URL)).anyTimes();
+    EasyMock.expect(request.getQueryString()).andReturn(null);
+    HttpServletResponse response = EasyMock.createNiceMock(HttpServletResponse.class);
+    EasyMock.expect(response.encodeRedirectURL(SERVICE_URL)).andReturn(SERVICE_URL);
+    EasyMock.expect(response.getOutputStream()).andAnswer(DummyServletOutputStream::new).anyTimes();
+    EasyMock.replay(request, response);
+
+    TestFilterChain chain = new TestFilterChain();
+    handler.doFilter(request, response, chain);
+  }
+
   @Test
   public void testValidJWT() throws Exception {
     try {
@@ -154,7 +199,6 @@ public abstract class AbstractJWTFilterTest  {
 
       SignedJWT jwt = getJWT(AbstractJWTFilter.JWT_DEFAULT_ISSUER, "alice",
                              new Date(new Date().getTime() + 5000), privateKey);
-
       HttpServletRequest request = EasyMock.createNiceMock(HttpServletRequest.class);
       setTokenOnRequest(request, jwt);
 
@@ -880,17 +924,30 @@ public abstract class AbstractJWTFilterTest  {
   }
 
   protected SignedJWT getJWT(String issuer, String sub, String aud, Date expires, Date nbf,
RSAPrivateKey privateKey,
-                             String signatureAlgorithm)
+                             String signatureAlgorithm) throws Exception {
+    return getJWT(issuer, sub, aud, expires, nbf, privateKey, signatureAlgorithm, String.valueOf(UUID.randomUUID()));
+  }
+
+  protected SignedJWT getJWT(final String issuer,
+                             final String sub,
+                             final String aud,
+                             final Date expires,
+                             final Date nbf,
+                             final RSAPrivateKey privateKey,
+                             final String signatureAlgorithm,
+                             final String knoxId)
       throws Exception {
-    JWTClaimsSet claims = new JWTClaimsSet.Builder()
-                                          .issuer(issuer)
-                                          .subject(sub)
-                                          .audience(aud)
-                                          .expirationTime(expires)
-                                          .notBeforeTime(nbf)
-                                          .claim("scope", "openid")
-                                          .claim(JWTToken.KNOX_ID_CLAIM, String.valueOf(UUID.randomUUID()))
-                                          .build();
+    JWTClaimsSet.Builder builder = new JWTClaimsSet.Builder();
+    builder.issuer(issuer)
+            .subject(sub)
+            .audience(aud)
+            .expirationTime(expires)
+            .notBeforeTime(nbf)
+            .claim("scope", "openid");
+    if (knoxId != null) {
+      builder.claim(JWTToken.KNOX_ID_CLAIM, knoxId);
+    }
+    JWTClaimsSet claims = builder.build();
 
     JWSHeader header = new JWSHeader.Builder(JWSAlgorithm.parse(signatureAlgorithm)).build();
 
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTAsHTTPBasicCredsFederationFilterTest.java
b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTAsHTTPBasicCredsFederationFilterTest.java
index dbfe98f..0de8828 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTAsHTTPBasicCredsFederationFilterTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTAsHTTPBasicCredsFederationFilterTest.java
@@ -77,7 +77,8 @@ public class JWTAsHTTPBasicCredsFederationFilterTest extends AbstractJWTFilterTe
     protected void setTokenOnRequest(final HttpServletRequest request,
                                      final String             authUsername,
                                      final String             authPassword) {
-        final byte[] basicAuth = (authUsername + ":" + authPassword).getBytes(StandardCharsets.UTF_8);
+        final byte[] basicAuth =
+                (authUsername + ":" + (authPassword != null ? authPassword : "")).getBytes(StandardCharsets.UTF_8);
         final String authHeaderValue = "Basic " + Base64.getEncoder().encodeToString(basicAuth);
         EasyMock.expect((Object)request.getHeader("Authorization")).andReturn(authHeaderValue);
     }
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/TokenIDAsHTTPBasicCredsFederationFilterTest.java
b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/TokenIDAsHTTPBasicCredsFederationFilterTest.java
index a439143..157aabe 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/TokenIDAsHTTPBasicCredsFederationFilterTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/TokenIDAsHTTPBasicCredsFederationFilterTest.java
@@ -37,6 +37,7 @@ import javax.servlet.ServletException;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.text.ParseException;
+import java.time.Instant;
 import java.util.Date;
 import java.util.Locale;
 import java.util.Map;
@@ -224,6 +225,16 @@ public class TokenIDAsHTTPBasicCredsFederationFilterTest extends JWTAsHTTPBasicC
     }
 
     @Override
+    public void testJWTWithoutKnoxUUIDClaim() throws Exception {
+        // Override to disable N/A test
+    }
+
+    @Override
+    public void testExpiredJWTWithoutKnoxUUIDClaim() throws Exception {
+        // Override to disable N/A test
+    }
+
+    @Override
     public void testInvalidAudienceJWT() throws Exception {
         // Override to disable N/A test
     }
@@ -339,23 +350,34 @@ public class TokenIDAsHTTPBasicCredsFederationFilterTest extends JWTAsHTTPBasicC
             if (expiration == null || expiration.isEmpty()) {
                 expiration = "0";
             }
-            tokenExpirations.put(TokenUtils.getTokenId(token), Long.parseLong(expiration));
+            addToken(TokenUtils.getTokenId(token), Instant.now().toEpochMilli(), Long.parseLong(expiration));
+        }
+
+        private void addToken(String tokenId, long expiration) {
+            tokenExpirations.put(tokenId, expiration);
         }
 
         @Override
         public void addToken(String tokenId, long issueTime, long expiration) {
-            tokenExpirations.put(tokenId, expiration);
+            addToken(tokenId, expiration);
         }
 
         @Override
         public void addToken(String tokenId, long issueTime, long expiration, long maxLifetimeDuration)
{
-            tokenExpirations.put(tokenId, expiration);
+            addToken(tokenId, expiration);
         }
 
         @Override
         public boolean isExpired(JWTToken token) throws UnknownTokenException {
-            Long expiration = tokenExpirations.get(TokenUtils.getTokenId(token));
-            return (new Date(expiration).before(new Date()));
+            Long expiration;
+            String tokenId = TokenUtils.getTokenId(token);
+            if (tokenId != null) {
+                expiration = tokenExpirations.get(tokenId);
+            } else {
+                expiration = Long.parseLong(token.getExpires());
+            }
+
+            return Instant.ofEpochMilli(expiration).isBefore(Instant.now());
         }
 
         @Override

Mime
View raw message