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/30 12:52:54 UTC
[knox] branch master updated: KNOX-2561 - Unique token identifiers
must be truncated when logged now that they can be used as secrets (#425)
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 ac80f0a KNOX-2561 - Unique token identifiers must be truncated when logged now that they can be used as secrets (#425)
ac80f0a is described below
commit ac80f0a712f38fc8f41b52816ce73db8507e7249
Author: Phil Zampino <pz...@apache.org>
AuthorDate: Tue Mar 30 08:52:43 2021 -0400
KNOX-2561 - Unique token identifiers must be truncated when logged now that they can be used as secrets (#425)
---
.../federation/jwt/filter/AbstractJWTFilter.java | 9 ++-
.../jwt/filter/AccessTokenFederationFilter.java | 7 +-
.../provider/federation/CommonJWTFilterTest.java | 6 +-
.../token/impl/DefaultTokenStateServiceTest.java | 3 +-
.../token/impl/ZookeeperTokenStateServiceTest.java | 12 +--
.../security/token/UnknownTokenException.java | 8 +-
.../java/org/apache/knox/gateway/util/Tokens.java | 19 +++++
.../org/apache/knox/gateway/util/TokensTest.java | 85 ++++++++++++++++++++++
8 files changed, 132 insertions(+), 17 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 c1a1056..fbbe3ce 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
@@ -325,6 +325,7 @@ public abstract class AbstractJWTFilter implements Filter {
final FilterChain chain, final JWT token)
throws IOException, ServletException {
final String tokenId = TokenUtils.getTokenId(token);
+ final String displayableTokenId = Tokens.getTokenIDDisplayText(tokenId);
final String displayableToken = Tokens.getTokenDisplayText(token.toString());
// confirm that issuer matches the intended target
if (expectedIssuer.equals(token.getIssuer())) {
@@ -340,7 +341,7 @@ public abstract class AbstractJWTFilter implements Filter {
if (verifyTokenSignature(token)) {
return true;
} else {
- log.failedToVerifyTokenSignature(tokenId, displayableToken);
+ log.failedToVerifyTokenSignature(displayableToken, displayableTokenId);
handleValidationError(request, response, HttpServletResponse.SC_UNAUTHORIZED, null);
}
} else {
@@ -349,12 +350,12 @@ public abstract class AbstractJWTFilter implements Filter {
"Bad request: the NotBefore check failed");
}
} else {
- log.failedToValidateAudience(tokenId, displayableToken);
+ log.failedToValidateAudience(displayableToken, displayableTokenId);
handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
"Bad request: missing required token audience");
}
} else {
- log.tokenHasExpired(displayableToken, tokenId);
+ 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
@@ -387,7 +388,7 @@ public abstract class AbstractJWTFilter implements Filter {
if (tokenIsStillValid(tokenId)) {
return true;
} else {
- log.tokenHasExpired(tokenId);
+ log.tokenHasExpired(Tokens.getTokenIDDisplayText(tokenId));
handleValidationError(request, response, HttpServletResponse.SC_BAD_REQUEST,
"Bad request: token has expired");
}
diff --git a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AccessTokenFederationFilter.java b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AccessTokenFederationFilter.java
index ea67c01..0f427a1 100644
--- a/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AccessTokenFederationFilter.java
+++ b/gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AccessTokenFederationFilter.java
@@ -92,6 +92,7 @@ public class AccessTokenFederationFilter implements Filter {
}
final String tokenId = TokenUtils.getTokenId(token);
+ final String displayableTokenId = Tokens.getTokenIDDisplayText(tokenId);
final String displayableToken = Tokens.getTokenDisplayText(token.toString());
if (verified) {
try {
@@ -100,11 +101,11 @@ public class AccessTokenFederationFilter implements Filter {
Subject subject = createSubjectFromToken(token);
continueWithEstablishedSecurityContext(subject, (HttpServletRequest)request, (HttpServletResponse)response, chain);
} else {
- log.failedToValidateAudience(tokenId, displayableToken);
+ log.failedToValidateAudience(displayableToken, displayableTokenId);
sendUnauthorized(response);
}
} else {
- log.tokenHasExpired(tokenId, displayableToken);
+ log.tokenHasExpired(displayableToken, displayableTokenId);
sendUnauthorized(response);
}
} catch (UnknownTokenException e) {
@@ -112,7 +113,7 @@ public class AccessTokenFederationFilter implements Filter {
sendUnauthorized(response);
}
} else {
- log.failedToVerifyTokenSignature(tokenId, displayableToken);
+ log.failedToVerifyTokenSignature(displayableToken, displayableTokenId);
sendUnauthorized(response);
}
} else {
diff --git a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/CommonJWTFilterTest.java b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/CommonJWTFilterTest.java
index 7e8353a..c78a956 100644
--- a/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/CommonJWTFilterTest.java
+++ b/gateway-provider-security-jwt/src/test/java/org/apache/knox/gateway/provider/federation/CommonJWTFilterTest.java
@@ -39,6 +39,7 @@ import java.io.IOException;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
+import java.util.UUID;
import static org.easymock.EasyMock.anyObject;
import static org.junit.Assert.assertFalse;
@@ -128,11 +129,12 @@ public class CommonJWTFilterTest {
@Test(expected = UnknownTokenException.class)
public void testIsStillValidUnknownToken() throws Exception {
TokenStateService tss = EasyMock.createNiceMock(TokenStateService.class);
+ final String tokenId = UUID.randomUUID().toString();
EasyMock.expect(tss.getTokenExpiration(anyObject(JWT.class)))
- .andThrow(new UnknownTokenException("eyjhbgcioi1234567890neg"))
+ .andThrow(new UnknownTokenException(tokenId))
.anyTimes();
EasyMock.expect(tss.getTokenExpiration(anyObject(String.class)))
- .andThrow(new UnknownTokenException("eyjhbgcioi1234567890neg"))
+ .andThrow(new UnknownTokenException(tokenId))
.anyTimes();
EasyMock.replay(tss);
diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java
index 1ed01ec..7aa1fac 100644
--- a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java
+++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateServiceTest.java
@@ -43,6 +43,7 @@ import org.apache.knox.gateway.services.security.token.TokenUtils;
import org.apache.knox.gateway.services.security.token.UnknownTokenException;
import org.apache.knox.gateway.services.security.token.impl.JWT;
import org.apache.knox.gateway.services.security.token.impl.JWTToken;
+import org.apache.knox.gateway.util.Tokens;
import org.easymock.EasyMock;
import org.junit.BeforeClass;
import org.junit.Rule;
@@ -233,7 +234,7 @@ public class DefaultTokenStateServiceTest {
// Expect the renew call to fail since the token should have been evicted
final UnknownTokenException e = assertThrows(UnknownTokenException.class, () -> tss.renewToken(token));
- assertEquals("Unknown token: " + TokenUtils.getTokenId(token), e.getMessage());
+ assertEquals("Unknown token: " + Tokens.getTokenIDDisplayText(TokenUtils.getTokenId(token)), e.getMessage());
} finally {
tss.stop();
}
diff --git a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/ZookeeperTokenStateServiceTest.java b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/ZookeeperTokenStateServiceTest.java
index a31aa47..0e99013 100644
--- a/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/ZookeeperTokenStateServiceTest.java
+++ b/gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/ZookeeperTokenStateServiceTest.java
@@ -33,6 +33,7 @@ import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.UUID;
import org.apache.curator.test.InstanceSpec;
import org.apache.curator.test.TestingCluster;
@@ -112,17 +113,18 @@ public class ZookeeperTokenStateServiceTest {
public void testRetry() throws Exception {
final ZookeeperTokenStateService zktokenStateServiceNode1 = setupZkTokenStateService(LONG_TOKEN_STATE_ALIAS_PERSISTENCE_INTERVAL);
final ZookeeperTokenStateService zktokenStateServiceNode2 = setupZkTokenStateService(LONG_TOKEN_STATE_ALIAS_PERSISTENCE_INTERVAL);
- zktokenStateServiceNode1.addToken("node1Token", 10L, 2000L);
- final long expiration = zktokenStateServiceNode2.getTokenExpiration("node1Token");
+ final String tokenId = UUID.randomUUID().toString();
+ zktokenStateServiceNode1.addToken(tokenId, 10L, 2000L);
+ final long expiration = zktokenStateServiceNode2.getTokenExpiration(tokenId);
Thread.sleep(LONG_TOKEN_STATE_ALIAS_PERSISTENCE_INTERVAL * 1000);
assertEquals(2000L, expiration);
final String userName = "testUser";
final String comment = "This is my test comment";
- zktokenStateServiceNode1.addMetadata("node1Token", new TokenMetadata(userName, comment));
+ zktokenStateServiceNode1.addMetadata(tokenId, new TokenMetadata(userName, comment));
Thread.sleep(LONG_TOKEN_STATE_ALIAS_PERSISTENCE_INTERVAL * 1000);
- assertEquals(userName, zktokenStateServiceNode2.getTokenMetadata("node1Token").getUserName());
- assertEquals(comment, zktokenStateServiceNode2.getTokenMetadata("node1Token").getComment());
+ assertEquals(userName, zktokenStateServiceNode2.getTokenMetadata(tokenId).getUserName());
+ assertEquals(comment, zktokenStateServiceNode2.getTokenMetadata(tokenId).getComment());
}
@Test
diff --git a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/UnknownTokenException.java b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/UnknownTokenException.java
index 1a8c4e0..6a0f8ff 100644
--- a/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/UnknownTokenException.java
+++ b/gateway-spi/src/main/java/org/apache/knox/gateway/services/security/token/UnknownTokenException.java
@@ -16,9 +16,12 @@
*/
package org.apache.knox.gateway.services.security.token;
+import org.apache.knox.gateway.util.Tokens;
+
public class UnknownTokenException extends Exception {
- private String tokenId;
+ private final String tokenId;
+ private final String tokenIdDisplay;
/**
*
@@ -26,6 +29,7 @@ public class UnknownTokenException extends Exception {
*/
public UnknownTokenException(final String tokenId) {
this.tokenId = tokenId;
+ this.tokenIdDisplay = Tokens.getTokenIDDisplayText(tokenId);
}
public String getTokenId() {
@@ -34,7 +38,7 @@ public class UnknownTokenException extends Exception {
@Override
public String getMessage() {
- return "Unknown token: " + tokenId;
+ return "Unknown token: " + tokenIdDisplay;
}
}
diff --git a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/Tokens.java b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/Tokens.java
index 77e2918..52b7a35 100644
--- a/gateway-util-common/src/main/java/org/apache/knox/gateway/util/Tokens.java
+++ b/gateway-util-common/src/main/java/org/apache/knox/gateway/util/Tokens.java
@@ -39,4 +39,23 @@ public class Tokens {
return displayText;
}
+ /**
+ * Get a String derived from a Knox token UUID String, which is suitable for presentation (e.g., logging) without
+ * compromising security.
+ *
+ * @param uuid A Knox token UUID String.
+ *
+ * @return An abbreviated form of the specified UUID String.
+ */
+ public static String getTokenIDDisplayText(final String uuid) {
+ String displayText = null;
+ if (uuid != null && uuid.length() == 36 && uuid.contains("-")) {
+ displayText = String.format(Locale.ROOT,
+ "%s...%s",
+ uuid.substring(0, uuid.indexOf('-')),
+ uuid.substring(uuid.lastIndexOf('-') + 1));
+ }
+ return displayText;
+ }
+
}
diff --git a/gateway-util-common/src/test/java/org/apache/knox/gateway/util/TokensTest.java b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/TokensTest.java
new file mode 100644
index 0000000..0d54375
--- /dev/null
+++ b/gateway-util-common/src/test/java/org/apache/knox/gateway/util/TokensTest.java
@@ -0,0 +1,85 @@
+/*
+ *
+ * * Licensed to the Apache Software Foundation (ASF) under one or more
+ * * contributor license agreements. See the NOTICE file distributed with this
+ * * work for additional information regarding copyright ownership. The ASF
+ * * licenses this file to you under the Apache License, Version 2.0 (the
+ * * "License"); you may not use this file except in compliance with the License.
+ * * You may obtain a copy of the License at
+ * *
+ * * http://www.apache.org/licenses/LICENSE-2.0
+ * *
+ * * Unless required by applicable law or agreed to in writing, software
+ * * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * * License for the specific language governing permissions and limitations under
+ * * the License.
+ *
+ */
+package org.apache.knox.gateway.util;
+
+import org.junit.Test;
+
+import java.util.UUID;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+public class TokensTest {
+
+ @Test
+ public void testTokenIdDisplayText() {
+ doTestTokenDisplay(UUID.randomUUID().toString());
+ }
+
+ @Test
+ public void testTokenIdDisplayTextEmptyUUID() {
+ doTestTokenDisplay("", true);
+ }
+
+ @Test
+ public void testTokenIdDisplayTextNullUUID() {
+ doTestTokenDisplay(null, true);
+ }
+
+ @Test
+ public void testTokenIdDisplayTextShortUUID() {
+ final String tokenId = UUID.randomUUID().toString();
+ doTestTokenDisplay(tokenId.substring(1), true);
+ }
+
+ @Test
+ public void testTokenIdDisplayTextInvalidUUID() {
+ final String tokenId = UUID.randomUUID().toString();
+ // Strip the '-' from the token ID
+ char[] invalid = new char[tokenId.length() - 4];
+ int count = 0;
+ for (int i = 0 ; i < tokenId.length(); i++) {
+ char c = tokenId.charAt(i);
+ if (c != '-') {
+ invalid[count++] = tokenId.charAt(i);
+ }
+ }
+ // Invoke the test with the invalid ID
+ doTestTokenDisplay(new String(invalid), true);
+ }
+
+ private void doTestTokenDisplay(final String tokenId) {
+ doTestTokenDisplay(tokenId, false);
+ }
+
+ private void doTestTokenDisplay(final String tokenId, final Boolean invalidID) {
+ String displayableTokenId = Tokens.getTokenIDDisplayText(tokenId);
+ if (invalidID) {
+ assertNull("Expected null because the tokenId is invalid.", displayableTokenId);
+ } else {
+ assertNotNull(displayableTokenId);
+ assertTrue(displayableTokenId.length() < tokenId.length());
+ assertEquals("Unexpected result for displayable token UUID.",
+ tokenId.substring(0, tokenId.indexOf('-')) + "..." + tokenId.substring(tokenId.lastIndexOf('-') + 1),
+ displayableTokenId);
+ }
+ }
+}