From 949b1c90347c8774ed9a26636e5a0030558d5de8 Mon Sep 17 00:00:00 2001 From: Sandeep More Date: Mon, 3 Feb 2020 15:21:25 -0500 Subject: [PATCH 1/4] KNOX-2207 - TokenStateService revocation should remove persisted token state --- .../impl/AliasBasedTokenStateService.java | 25 ++++++++++++++--- .../token/impl/DefaultTokenStateService.java | 28 +++++++++++-------- .../token/impl/TokenStateServiceMessages.java | 2 +- 3 files changed, 38 insertions(+), 17 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java index b5b1010f20..f6f053d037 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java @@ -104,14 +104,15 @@ public long getTokenExpiration(final String token) { @Override public void revokeToken(final String token) { - // Record the revocation by setting the expiration to -1 - updateExpiration(token, -1L); + /* no reason to keep revoked tokens around */ + removeRevokedExpiredToken(token); log.revokedToken(getTokenDisplayText(token)); } @Override protected boolean isRevoked(final String token) { - return (getTokenExpiration(token) < 0); + /* since we no longer maintain revoked tokens, revoked tokens are unknown tokens */ + return isUnknown(token); } @Override @@ -125,11 +126,27 @@ protected boolean isUnknown(final String token) { return isUnknown; } + @Override + protected void removeRevokedExpiredToken(final String token) { + if (isUnknown(token)) { + log.unknownToken(getTokenDisplayText(token)); + throw new IllegalArgumentException("Unknown or revoked token."); + } + + try { + aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token); + aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max"); + } catch (AliasServiceException e) { + log.failedToUpdateTokenExpiration(e); + } + + } + @Override protected void updateExpiration(final String token, long expiration) { if (isUnknown(token)) { log.unknownToken(getTokenDisplayText(token)); - throw new IllegalArgumentException("Unknown token."); + throw new IllegalArgumentException("Unknown or revoked token."); } try { diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java index 77ab5a47b5..c33a1185d4 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java @@ -23,10 +23,8 @@ import org.apache.knox.gateway.services.security.token.impl.JWTToken; import java.util.HashMap; -import java.util.HashSet; import java.util.Locale; import java.util.Map; -import java.util.Set; /** * In-Memory authentication token state management implementation. @@ -43,8 +41,6 @@ public class DefaultTokenStateService implements TokenStateService { private final Map tokenExpirations = new HashMap<>(); - private final Set revokedTokens = new HashSet<>(); - private final Map maxTokenLifetimes = new HashMap<>(); @@ -160,7 +156,8 @@ public void revokeToken(final JWTToken token) { @Override public void revokeToken(final String token) { validateToken(token); - revokedTokens.add(token); + /* no reason to keep revoked tokens around */ + removeRevokedExpiredToken(token); log.revokedToken(getTokenDisplayText(token)); } @@ -208,6 +205,18 @@ protected void updateExpiration(final String token, long expiration) { } } + protected void removeRevokedExpiredToken(final String token) { + if (!isValidIdentifier(token)) { + throw new IllegalArgumentException("Token data cannot be null."); + } + synchronized (tokenExpirations) { + tokenExpirations.remove(token); + } + synchronized (maxTokenLifetimes) { + maxTokenLifetimes.remove(token); + } + } + protected boolean hasRemainingRenewals(final String token, long renewInterval) { // Is the current time + 30-second buffer + the renewal interval is less than the max lifetime for the token? return ((System.currentTimeMillis() + 30000 + renewInterval) < getMaxLifetime(token)); @@ -222,7 +231,7 @@ protected long getMaxLifetime(final String token) { } protected boolean isRevoked(final String token) { - return revokedTokens.contains(token); + return !tokenExpirations.containsKey(token); } protected boolean isValidIdentifier(final String token) { @@ -256,12 +265,7 @@ protected void validateToken(final String token, boolean includeRevocation) thro // First, make sure the token is one we know about if (isUnknown(token)) { log.unknownToken(getTokenDisplayText(token)); - throw new IllegalArgumentException("Unknown token"); - } - - // Then, make sure it has not been revoked - if (includeRevocation && isRevoked(token)) { - throw new IllegalArgumentException("The specified token has been revoked"); + throw new IllegalArgumentException("Unknown or revoked token"); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java index 8b7d9b5c40..dc66558fd0 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java @@ -33,7 +33,7 @@ public interface TokenStateServiceMessages { @Message(level = MessageLevel.DEBUG, text = "Revoked token {0}") void revokedToken(String tokenDisplayText); - @Message(level = MessageLevel.DEBUG, text = "Unknown token {0}") + @Message(level = MessageLevel.DEBUG, text = "Unknown or revoked token {0}") void unknownToken(String tokenDisplayText); @Message(level = MessageLevel.ERROR, text = "The renewal limit for the token ({0}) has been exceeded.") From 32002259868e427afeafbe119f1ebf7707f6caaa Mon Sep 17 00:00:00 2001 From: Sandeep More Date: Tue, 4 Feb 2020 16:06:58 -0500 Subject: [PATCH 2/4] KNOX-2207 - Review changes --- .../impl/AliasBasedTokenStateService.java | 17 +++--------- .../token/impl/DefaultTokenStateService.java | 27 +++++++------------ .../token/impl/TokenStateServiceMessages.java | 2 +- 3 files changed, 15 insertions(+), 31 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java index f6f053d037..6d29cae1c3 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java @@ -105,16 +105,10 @@ public long getTokenExpiration(final String token) { @Override public void revokeToken(final String token) { /* no reason to keep revoked tokens around */ - removeRevokedExpiredToken(token); + removeToken(token); log.revokedToken(getTokenDisplayText(token)); } - @Override - protected boolean isRevoked(final String token) { - /* since we no longer maintain revoked tokens, revoked tokens are unknown tokens */ - return isUnknown(token); - } - @Override protected boolean isUnknown(final String token) { boolean isUnknown = false; @@ -127,11 +121,8 @@ protected boolean isUnknown(final String token) { } @Override - protected void removeRevokedExpiredToken(final String token) { - if (isUnknown(token)) { - log.unknownToken(getTokenDisplayText(token)); - throw new IllegalArgumentException("Unknown or revoked token."); - } + protected void removeToken(final String token) { + validateToken(token); try { aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token); @@ -146,7 +137,7 @@ protected void removeRevokedExpiredToken(final String token) { protected void updateExpiration(final String token, long expiration) { if (isUnknown(token)) { log.unknownToken(getTokenDisplayText(token)); - throw new IllegalArgumentException("Unknown or revoked token."); + throw new IllegalArgumentException("Unknown token."); } try { diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java index c33a1185d4..e2b1a49b3f 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java @@ -155,9 +155,8 @@ public void revokeToken(final JWTToken token) { @Override public void revokeToken(final String token) { - validateToken(token); /* no reason to keep revoked tokens around */ - removeRevokedExpiredToken(token); + removeToken(token); log.revokedToken(getTokenDisplayText(token)); } @@ -168,15 +167,15 @@ public boolean isExpired(final JWTToken token) { @Override public boolean isExpired(final String token) { - boolean isExpired; + boolean isUnknownToken; - isExpired = isRevoked(token); // Check if it has been revoked first - if (!isExpired) { - // If it has not been revoked, check its expiration - isExpired = (getTokenExpiration(token) <= System.currentTimeMillis()); + isUnknownToken = isUnknown(token); // Check if the token exist + if (!isUnknownToken) { + // If it not unknown, check its expiration + isUnknownToken = (getTokenExpiration(token) <= System.currentTimeMillis()); } - return isExpired; + return isUnknownToken; } protected void setMaxLifetime(final String token, long issueTime, long maxLifetimeDuration) { @@ -205,10 +204,8 @@ protected void updateExpiration(final String token, long expiration) { } } - protected void removeRevokedExpiredToken(final String token) { - if (!isValidIdentifier(token)) { - throw new IllegalArgumentException("Token data cannot be null."); - } + protected void removeToken(final String token) { + validateToken(token); synchronized (tokenExpirations) { tokenExpirations.remove(token); } @@ -230,10 +227,6 @@ protected long getMaxLifetime(final String token) { return result; } - protected boolean isRevoked(final String token) { - return !tokenExpirations.containsKey(token); - } - protected boolean isValidIdentifier(final String token) { return token != null && !token.isEmpty(); } @@ -265,7 +258,7 @@ protected void validateToken(final String token, boolean includeRevocation) thro // First, make sure the token is one we know about if (isUnknown(token)) { log.unknownToken(getTokenDisplayText(token)); - throw new IllegalArgumentException("Unknown or revoked token"); + throw new IllegalArgumentException("Unknown token"); } } diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java index dc66558fd0..8b7d9b5c40 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java @@ -33,7 +33,7 @@ public interface TokenStateServiceMessages { @Message(level = MessageLevel.DEBUG, text = "Revoked token {0}") void revokedToken(String tokenDisplayText); - @Message(level = MessageLevel.DEBUG, text = "Unknown or revoked token {0}") + @Message(level = MessageLevel.DEBUG, text = "Unknown token {0}") void unknownToken(String tokenDisplayText); @Message(level = MessageLevel.ERROR, text = "The renewal limit for the token ({0}) has been exceeded.") From e0bec607432137f5d72599c535bd49268d85ea17 Mon Sep 17 00:00:00 2001 From: Sandeep More Date: Wed, 5 Feb 2020 11:24:36 -0500 Subject: [PATCH 3/4] KNOX-2207 - Review changes --- .../token/impl/DefaultTokenStateService.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java index e2b1a49b3f..89f9ccb963 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java @@ -167,15 +167,16 @@ public boolean isExpired(final JWTToken token) { @Override public boolean isExpired(final String token) { - boolean isUnknownToken; - - isUnknownToken = isUnknown(token); // Check if the token exist - if (!isUnknownToken) { - // If it not unknown, check its expiration - isUnknownToken = (getTokenExpiration(token) <= System.currentTimeMillis()); + try { + validateToken(token); + } catch (final IllegalArgumentException e) { + if (e.toString().contains("Token data cannot be null.") || e.toString().contains("Unknown token")) { + return true; + } else { + throw e; + } } - - return isUnknownToken; + return (getTokenExpiration(token) <= System.currentTimeMillis()); } protected void setMaxLifetime(final String token, long issueTime, long maxLifetimeDuration) { From 583b74662051082081c549977d2ef7f2fb590b94 Mon Sep 17 00:00:00 2001 From: Sandeep More Date: Wed, 5 Feb 2020 13:14:41 -0500 Subject: [PATCH 4/4] KNOX-2207 - Review changes --- .../token/impl/DefaultTokenStateService.java | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java index 89f9ccb963..e15815486c 100644 --- a/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java +++ b/gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java @@ -167,16 +167,13 @@ public boolean isExpired(final JWTToken token) { @Override public boolean isExpired(final String token) { - try { - validateToken(token); - } catch (final IllegalArgumentException e) { - if (e.toString().contains("Token data cannot be null.") || e.toString().contains("Unknown token")) { - return true; - } else { - throw e; - } + boolean isExpired; + isExpired = isUnknown(token); // Check if the token exist + if (!isExpired) { + // If it not unknown, check its expiration + isExpired = (getTokenExpiration(token) <= System.currentTimeMillis()); } - return (getTokenExpiration(token) <= System.currentTimeMillis()); + return isExpired; } protected void setMaxLifetime(final String token, long issueTime, long maxLifetimeDuration) {