You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@knox.apache.org by pz...@apache.org on 2021/03/12 20:15:54 UTC
[knox] branch master updated: KNOX-2547 - Token-based providers
should perform signature verification last (#410)
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 007024f KNOX-2547 - Token-based providers should perform signature verification last (#410)
007024f is described below
commit 007024fc6eb9516b6060f665bdf50a9ff00a2a15
Author: Phil Zampino <pz...@apache.org>
AuthorDate: Fri Mar 12 15:15:45 2021 -0500
KNOX-2547 - Token-based providers should perform signature verification last (#410)
---
.../federation/jwt/filter/AbstractJWTFilter.java | 57 +++++++++-----
.../provider/federation/AbstractJWTFilterTest.java | 92 ++++++++++++++++++++++
.../JWTAsHTTPBasicCredsFederationFilterTest.java | 4 +-
.../federation/JWTFederationFilterTest.java | 4 +-
.../provider/federation/SSOCookieProviderTest.java | 4 +-
5 files changed, 134 insertions(+), 27 deletions(-)
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 f9b9edc..2f1b864 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
@@ -84,7 +84,7 @@ public abstract class AbstractJWTFilter implements Filter {
public static final String JWT_DEFAULT_SIGALG = "RS256";
public static final String JWT_VERIFIED_CACHE_MAX = "jwt.verified.cache.max";
- public static final int JWT_VERIFIED_CACHE_MAX_DEFAULT = 100;
+ public static final int JWT_VERIFIED_CACHE_MAX_DEFAULT = 250;
static JWTMessages log = MessagesFactory.get( JWTMessages.class );
private static AuditService auditService = AuditServiceFactory.getAuditService();
@@ -299,31 +299,37 @@ public abstract class AbstractJWTFilter implements Filter {
// the designated expiration time
try {
if (tokenIsStillValid(token)) {
- // Verify the token signature
- if (verifyToken(token)) {
- boolean audValid = validateAudiences(token);
- if (audValid) {
- Date nbf = token.getNotBeforeDate();
- if (nbf == null || new Date().after(nbf)) {
+ boolean audValid = validateAudiences(token);
+ if (audValid) {
+ Date nbf = token.getNotBeforeDate();
+ if (nbf == null || new Date().after(nbf)) {
+ if (verifyTokenSignature(token)) {
return true;
} else {
- log.notBeforeCheckFailed();
- handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
- "Bad request: the NotBefore check failed");
+ log.failedToVerifyTokenSignature(tokenId, displayableToken);
+ handleValidationError(request, response, HttpServletResponse.SC_UNAUTHORIZED, null);
}
} else {
- log.failedToValidateAudience(tokenId, displayableToken);
+ log.notBeforeCheckFailed();
handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
- "Bad request: missing required token audience");
+ "Bad request: the NotBefore check failed");
}
} else {
- log.failedToVerifyTokenSignature(tokenId, displayableToken);
- handleValidationError(request, response, HttpServletResponse.SC_UNAUTHORIZED, null);
+ log.failedToValidateAudience(tokenId, displayableToken);
+ handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
+ "Bad request: missing required token audience");
}
} else {
log.tokenHasExpired(tokenId, displayableToken);
+
+ // 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");
+
}
} catch (UnknownTokenException e) {
log.unableToVerifyExpiration(e);
@@ -336,13 +342,13 @@ public abstract class AbstractJWTFilter implements Filter {
return false;
}
- protected boolean verifyToken(final JWT token) {
+ protected boolean verifyTokenSignature(final JWT token) {
boolean verified;
String tokenId = TokenUtils.getTokenId(token);
// Check if the token has already been verified
- verified = hasTokenBeenVerified(tokenId);
+ verified = hasSignatureBeenVerified(tokenId);
// If it has not yet been verified, then perform the verification now
if (!verified) {
@@ -372,7 +378,7 @@ public abstract class AbstractJWTFilter implements Filter {
}
if (verified) { // If successful, record the verification for future reference
- recordTokenVerification(tokenId);
+ recordSignatureVerification(tokenId);
}
}
@@ -380,25 +386,34 @@ public abstract class AbstractJWTFilter implements Filter {
}
/**
- * Determine if the specified token has previously been successfully verified.
+ * Determine if the specified token signature has previously been successfully verified.
*
* @param tokenId The unique identifier for a token.
*
* @return true, if the specified token has been previously verified; Otherwise, false.
*/
- protected boolean hasTokenBeenVerified(final String tokenId) {
+ protected boolean hasSignatureBeenVerified(final String tokenId) {
return (verifiedTokens.getIfPresent(tokenId) != null);
}
/**
- * Record a successful token verification.
+ * Record a successful token signature verification.
*
* @param tokenId The unique identifier for the token which has been successfully verified.
*/
- protected void recordTokenVerification(final String tokenId) {
+ protected void recordSignatureVerification(final String tokenId) {
verifiedTokens.put(tokenId, true);
}
+ /**
+ * Explicitly evict the signature verification record from the cache if it exists.
+ *
+ * @param tokenId The token whose signature verification record should be evicted.
+ */
+ protected void removeSignatureVerificationRecord(final String tokenId) {
+ verifiedTokens.asMap().remove(tokenId);
+ }
+
protected abstract void handleValidationError(HttpServletRequest request, HttpServletResponse response, int status,
String error) throws IOException;
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 300cbe2..20324d6 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
@@ -1031,6 +1031,91 @@ public abstract class AbstractJWTFilterTest {
}
}
+ @Test
+ public void testExpiredTokensEvictedFromSignatureVerificationCache() throws Exception {
+ try {
+
+ final String principalAlice = "alice";
+
+ Properties props = getProperties();
+ props.put(AbstractJWTFilter.JWT_EXPECTED_SIGALG, "RS512");
+ props.put(AbstractJWTFilter.JWT_VERIFIED_CACHE_MAX, "1");
+ handler.init(new TestFilterConfig(props));
+ Assert.assertEquals("Expected no token verification calls yet.",
+ 0, ((TokenVerificationCounter) handler).getVerificationCount());
+
+ long expiration = new Date().getTime() + TimeUnit.SECONDS.toMillis(2);
+ final SignedJWT jwt_alice = getJWT(AbstractJWTFilter.JWT_DEFAULT_ISSUER,
+ principalAlice,
+ new Date(expiration),
+ new Date(),
+ privateKey,
+ JWSAlgorithm.RS512.getName());
+
+ HttpServletRequest request = createMockRequest(jwt_alice);
+ HttpServletResponse response = createMockResponse();
+ EasyMock.replay(request, response);
+
+ TestFilterChain chain = new TestFilterChain();
+ handler.doFilter(request, response, chain);
+ Assert.assertTrue("doFilterCalled should not be false.", chain.doFilterCalled);
+ Assert.assertEquals("The signature verification record for the token should have been added.",
+ 1, getSignatureVerificationCacheSize());
+
+ // Do it again, after the token has expired
+ request = createMockRequest(jwt_alice);
+ response = createMockResponse();
+ EasyMock.replay(request, response);
+
+ // Wait for the token to expire
+ while (System.currentTimeMillis() < expiration) {
+ try {
+ Thread.sleep(500);
+ } catch (InterruptedException e) {
+ //
+ }
+ }
+
+ chain = new TestFilterChain();
+ handler.doFilter(request, response, chain);
+ Assert.assertFalse("doFilterCalled should be false since the token is expired.", chain.doFilterCalled);
+
+ Assert.assertEquals("The signature verification record for the expired token should have been removed.",
+ 0, getSignatureVerificationCacheSize());
+
+ } catch (ServletException se) {
+ fail("Should NOT have thrown a ServletException.");
+ }
+ }
+
+ private HttpServletRequest createMockRequest(final SignedJWT jwt) {
+ 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).anyTimes();
+ return request;
+ }
+
+ private HttpServletResponse createMockResponse() throws Exception {
+ HttpServletResponse response = EasyMock.createNiceMock(HttpServletResponse.class);
+ EasyMock.expect(response.encodeRedirectURL(SERVICE_URL)).andReturn(SERVICE_URL);
+ EasyMock.expect(response.getOutputStream()).andAnswer(() -> new ServletOutputStream() {
+ @Override
+ public void write(int b) {
+ }
+
+ @Override
+ public void setWriteListener(WriteListener arg0) {
+ }
+
+ @Override
+ public boolean isReady() {
+ return false;
+ }
+ }).anyTimes();
+ return response;
+ }
+
private void doRepeatTestVerificationOptimization(final HttpServletRequest request,
final HttpServletResponse response,
final SignedJWT jwt,
@@ -1064,6 +1149,13 @@ public abstract class AbstractJWTFilterTest {
cache.cleanUp();
}
+ private long getSignatureVerificationCacheSize() throws Exception {
+ Field f = handler.getClass().getSuperclass().getSuperclass().getDeclaredField("verifiedTokens");
+ f.setAccessible(true);
+ Cache<String, Boolean> cache = (Cache<String, Boolean>) f.get(handler);
+ return cache.estimatedSize();
+ }
+
protected Properties getProperties() {
Properties props = new Properties();
props.setProperty(
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 ae99d99..b9b52fd 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
@@ -60,8 +60,8 @@ public class JWTAsHTTPBasicCredsFederationFilterTest extends AbstractJWTFilterTe
}
@Override
- protected void recordTokenVerification(String tokenId) {
- super.recordTokenVerification(tokenId);
+ protected void recordSignatureVerification(String tokenId) {
+ super.recordSignatureVerification(tokenId);
verifiedCount++;
}
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTest.java b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTest.java
index 8a630aa..ca8b53b 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/JWTFederationFilterTest.java
@@ -57,8 +57,8 @@ public class JWTFederationFilterTest extends AbstractJWTFilterTest {
}
@Override
- protected void recordTokenVerification(final String tokenId) {
- super.recordTokenVerification(tokenId);
+ protected void recordSignatureVerification(final String tokenId) {
+ super.recordSignatureVerification(tokenId);
verificationCount++;
}
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java
index 2977c84..5c932b1 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/SSOCookieProviderTest.java
@@ -311,8 +311,8 @@ public class SSOCookieProviderTest extends AbstractJWTFilterTest {
}
@Override
- protected void recordTokenVerification(String tokenId) {
- super.recordTokenVerification(tokenId);
+ protected void recordSignatureVerification(String tokenId) {
+ super.recordSignatureVerification(tokenId);
verificationCount++;
}