You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2021/03/31 19:11:40 UTC
[GitHub] [knox] pzampino opened a new pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
pzampino opened a new pull request #427:
URL: https://github.com/apache/knox/pull/427
## What changes were proposed in this pull request?
Added some checks for missing Knox token UUID claim around the signature verification caching (which was added as part of KNOX-2544) to avoid NullPointerException when JWTs which were not issued by Knox (but which Knox can verify) are received.
## How was this patch tested?
Added org.apache.knox.gateway.provider.federation.AbstractJWTFilterTest#testJWTWithoutKnoxUUIDClaim() to reproduce the NPE condition and then to verify the fix. Ran all other existing tests as part of the build.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605716533
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -407,7 +415,7 @@ protected boolean verifyTokenSignature(final JWT token) {
String tokenId = TokenUtils.getTokenId(token);
// Check if the token has already been verified
- verified = hasSignatureBeenVerified(tokenId);
+ verified = (tokenId != null) && hasSignatureBeenVerified(tokenId);
Review comment:
This is a valid point, and one that I would argue can be addressed as part of revisiting the the signature verification caching implementation altogether.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] lmccay commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605728662
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -385,12 +387,18 @@ protected boolean validateToken(final HttpServletRequest request,
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");
Review comment:
okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] lmccay commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605728496
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -69,4 +69,8 @@
text = "The configuration value ({0}) for maximum token verification cache is invalid; Using the default value." )
void invalidVerificationCacheMaxConfiguration(String value);
+ @Message( level = MessageLevel.ERROR,
Review comment:
okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] lmccay commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605730041
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -407,7 +415,7 @@ protected boolean verifyTokenSignature(final JWT token) {
String tokenId = TokenUtils.getTokenId(token);
// Check if the token has already been verified
- verified = hasSignatureBeenVerified(tokenId);
+ verified = (tokenId != null) && hasSignatureBeenVerified(tokenId);
Review comment:
I can buy that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] lmccay commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
lmccay commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605678987
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -69,4 +69,8 @@
text = "The configuration value ({0}) for maximum token verification cache is invalid; Using the default value." )
void invalidVerificationCacheMaxConfiguration(String value);
+ @Message( level = MessageLevel.ERROR,
Review comment:
Should this really be considered an error?
We actually have support for 3rd party tokens which relying on this KnoxToken specific claim breaks backward compatibility for. I think this is a warning at best and we can extend the message to indicate that it is assumed to be a 3rd party token.
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -385,12 +387,18 @@ protected boolean validateToken(final HttpServletRequest request,
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");
Review comment:
It seems has though we are assuming that if token state server is enabled for a given KnoxToken service that it cannot also accept 3rd party tokens. Is this a necessary requirement? Would falling back to only using the token expiry be acceptable? It is sort of unclear how we would verify the signature given today's approach to signing keys but if we were to add support for jwks for getting the public key for verification, it seems that we could fully support 3rd party tokens as well as KnoxTokens in the same provider deployment. If we waited for the jwks support to make that work it may be harder to find and fix these.
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -407,7 +415,7 @@ protected boolean verifyTokenSignature(final JWT token) {
String tokenId = TokenUtils.getTokenId(token);
// Check if the token has already been verified
- verified = hasSignatureBeenVerified(tokenId);
+ verified = (tokenId != null) && hasSignatureBeenVerified(tokenId);
Review comment:
if we do decide to support 3rd party tokens, we would likely want to make sure that such tokens also benefit from the caching. Which seems to imply that we would either need to derive a tokenId from the 3rd party token somehow or use the actual token as the id. The latter would have uniqueness implications across jobs but may be okay for this optimization. If the token is exactly the same then it is the same user and created at exactly the same time with the exact same expiry?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605715619
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java
##########
@@ -385,12 +387,18 @@ protected boolean validateToken(final HttpServletRequest request,
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");
Review comment:
Again, this code is not applicable to the cases where a JWT is provided for authentication; It is only for those cases where ONLY the token ID is provided, in which case the server-managed token state is absolutely necessary since there is nothing about the ID which can be verified by itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #427:
URL: https://github.com/apache/knox/pull/427#discussion_r605713638
##########
File path: gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java
##########
@@ -69,4 +69,8 @@
text = "The configuration value ({0}) for maximum token verification cache is invalid; Using the default value." )
void invalidVerificationCacheMaxConfiguration(String value);
+ @Message( level = MessageLevel.ERROR,
Review comment:
The context from which this is logged is not applicable to the JWT scenario. It is only ever logged when the token ID is used as a passcode, which requires server-managed token state to be enabled because we can't authenticate based on the ID alone (we need the expiry/metadata). In this context, I believe this is an error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino merged pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #427:
URL: https://github.com/apache/knox/pull/427
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] lmccay commented on pull request #427: KNOX-2566 - JWT Token Signature Verification Caching NPE
Posted by GitBox <gi...@apache.org>.
lmccay commented on pull request #427:
URL: https://github.com/apache/knox/pull/427#issuecomment-811970157
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org