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 2020/02/25 21:13:28 UTC
[GitHub] [knox] moresandeep opened a new pull request #274: KNOX-2212 -
Token permissiveness
moresandeep opened a new pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274
## What changes were proposed in this pull request?
When `gateway.knox.token.permissive.failure.enabled` property is set to true in gateway-site.xml (by default it is false) **and** `knox.token.exp.server-managed` is set to true in the descriptor (server managed token state is enabled) then, when Knox encounters a valid token which is not stored in it's token state Knox uses the expiry date of the JWT token instead of flagging it as "Unknown Token"
## How was this patch tested?
This patch was locally tested
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385200021
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
I **do not** see this converting an UnknownTokenException to an IllegalArgumentException. Am I missing something?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385336747
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -249,8 +249,10 @@
private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.interval";
private static final String KNOX_TOKEN_EVICTION_GRACE_PERIOD = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.grace.period";
+ private static final String KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.permissive.failure.enabled";
Review comment:
Sure, I'll change it to `.knox.token.permissive.validation`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385336020
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -92,9 +93,17 @@ protected long getMaxLifetime(final String token) {
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration = 0;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
+ .containsIgnoreCase(e.toString(), "Unknown token")) {
Review comment:
I see the confusion now, I started working on it before "UnknownTokenException" was introduced so we have "IllegalArgumentException" this is an artifact of that. I will change it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384132552
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -92,9 +93,17 @@ protected long getMaxLifetime(final String token) {
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration = 0;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
+ .containsIgnoreCase(e.toString(), "Unknown token")) {
Review comment:
What? We catch an `UnknownTokenException` and then are doing a string check on the error message? Why?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385197908
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -118,9 +125,17 @@ public void addToken(final String token,
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
Review comment:
The unknown token condition is an important distinction from a known-but-invalid token case; Hence, the exception.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385200626
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java
##########
@@ -63,4 +63,10 @@
@Message(level = MessageLevel.ERROR, text = "Error occurred evicting token {0}")
void errorEvictingTokens(@StackTrace(level = MessageLevel.DEBUG) Exception e);
+ @Message(level = MessageLevel.ERROR, text = "Error occurred while parsing JWT token, cause: {0}")
+ void errorParsingToken(String cause);
+
+ @Message(level = MessageLevel.DEBUG, text = "Token permissiveness is enabled, expiration for token {0} is {1}")
Review comment:
Again, I think _permissive validation_ is a better description
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385336952
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -249,8 +249,10 @@
private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.interval";
private static final String KNOX_TOKEN_EVICTION_GRACE_PERIOD = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.grace.period";
+ private static final String KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.permissive.failure.enabled";
private static final long KNOX_TOKEN_EVICTION_INTERVAL_DEFAULT = TimeUnit.MINUTES.toSeconds(5);
private static final long KNOX_TOKEN_EVICTION_GRACE_PERIOD_DEFAULT = TimeUnit.MINUTES.toSeconds(5);
+ private static final boolean KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED_DEFAULT = false;
Review comment:
Yup, I'll fix this.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385786676
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
You could also just make it [Long](https://docs.oracle.com/javase/8/docs/api/java/lang/Long.html), and return null in the case where it cannot be parsed. The OptionalLong might be better though.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385781280
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
There could be some case where 0 might be a legal value. On the same lines I can just make return type [OptionalInt](https://docs.oracle.com/javase/8/docs/api/java/util/OptionalInt.html).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385195059
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -249,8 +249,10 @@
private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.interval";
private static final String KNOX_TOKEN_EVICTION_GRACE_PERIOD = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.grace.period";
+ private static final String KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.permissive.failure.enabled";
private static final long KNOX_TOKEN_EVICTION_INTERVAL_DEFAULT = TimeUnit.MINUTES.toSeconds(5);
private static final long KNOX_TOKEN_EVICTION_GRACE_PERIOD_DEFAULT = TimeUnit.MINUTES.toSeconds(5);
+ private static final boolean KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED_DEFAULT = false;
Review comment:
Again, I think we should treat this in terms of validation rather than failure.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep merged pull request #274: KNOX-2212 - Token
permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384168240
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
True, that would be ugly, but we only throw two types of exceptions here `UnknownTokenException ` and `IllegalArgumentException`. `ParseException` is not a Runtime exception so we need to wrap it up in one of the two types, which leaves us with `IllegalArgumentException`. The stack trace will atleast let users know what the original error was.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384133911
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
So this throws a new exception? Where does this exception end up going? I think this will convert an `UnknownTokenException` to a runtime `IllegalArgumentException` and so this will cause weird failures up the stack?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385316073
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
Right but the method signature where all this is called is:
`public long getTokenExpiration(final String token) throws UnknownTokenException {`
So why would we throw an `IllegalArgumentException`? Its an unchecked runtimeexception so do we know how that bubbles back up to the caller? Is every user of `DefaultTokenStateService#getTokenExpiration` going to know to check for `IllegalArgumentException`?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385195850
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -16,15 +16,19 @@
*/
package org.apache.knox.gateway.services.token.impl;
+import org.apache.commons.lang3.StringUtils;
Review comment:
I recall comments from Kevin discouraging the use of this, but I don't recall the rationale.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385781280
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
There could be some case where 0 might be a legal value. On the same lines I can just make return type [OptionalLong](https://docs.oracle.com/javase/8/docs/api/java/util/OptionalLong.html).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385347313
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
So to fix this I can think of either of two ways
1. We would need to update the method signature for `getTokenExpiration()` to throw `UnknownTokenException` and `IllegalArgumentException`. Now this change would cause updating a lot of method signatures which I am hesitant to do as it affects a lot of parts that we know work well.
2. `getJWTTokenExpiration()` method can throw `UnknownTokenException`. We do not need to check if the token is not null here (since it was done by calling method currently using it) so we can get rid of one of two `IllegalArgumentException`. And then wrap the `ParseException` into `UnknownTokenException` which I am not sure is right.
@risdenk @pzampino thoughts ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385221731
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -16,15 +16,19 @@
*/
package org.apache.knox.gateway.services.token.impl;
+import org.apache.commons.lang3.StringUtils;
Review comment:
It was more around removing the commons-lang3 dependency if it could be avoided. I only bring it up if we don't need commons-lang3 everywhere. commons-lang3 wasn't added in this PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384164524
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -118,9 +125,17 @@ public void addToken(final String token,
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
Review comment:
I mean we could move this one level up and include it in a different function.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384162938
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -92,9 +93,17 @@ protected long getMaxLifetime(final String token) {
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration = 0;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
+ .containsIgnoreCase(e.toString(), "Unknown token")) {
Review comment:
Because this is how the validate() function let's you know that the TokenStateService does not have the token, by throwing UnknownTokenException.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385768402
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
What if, rather than throwing the IllegalArgumentException, a value (e.g., 0) was returned? This would be handled as treating the token as if it is expired, which is primarily used to determine if it is (still) valid; We might have to change the resulting log message(s), but I think treating it as invalid would work.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384772404
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -118,9 +125,17 @@ public void addToken(final String token,
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
Review comment:
Actually moving this code in a separate function would be more untidy (common code in this case would be just `validateToken(token)` and try catch). I don't like the fact that `validateToken(token)` throws an exception instead of returning `true` or `false` that would be a major change to do here. The main function that calculates JWT value `getJWTTokenExpiration(token)` is common for both implementation. Let me know if you still feel strongly about it.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r384133140
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -118,9 +125,17 @@ public void addToken(final String token,
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
Review comment:
We do the exact same thing twice? Why?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385227048
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
I see what you're saying, but really the UnknownTokenException is being handled, and the fact that the token is actually invalid is a different 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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385791872
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
Cool, will do.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385224019
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -310,4 +325,30 @@ protected boolean needsEviction(final String token) throws UnknownTokenException
return tokenExpirations.keySet().stream().collect(Collectors.toList());
}
+ /**
+ * A function that returns the JWT token expiration. This is only called when
+ * gateway.knox.token.permissive.failure.enabled property is set to true.
+ *
+ * @param token token to be verified and saved
+ */
+ protected long getJWTTokenExpiration(final String token) {
+ if (!isValidIdentifier(token)) {
+ throw new IllegalArgumentException("Token data cannot be null.");
+ }
+ JWT jwt;
+ try {
+ jwt = new JWTToken(token);
+ } catch (final ParseException e) {
+ log.errorParsingToken(e.toString());
+ throw new IllegalArgumentException(e);
Review comment:
So its subtle. `getJWTTokenExpiration` is called from the following code:
```
try {
validateToken(token);
} catch (final UnknownTokenException e) {
/* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
if (permissiveFailureEnabled && StringUtils
.containsIgnoreCase(e.toString(), "Unknown token")) {
return getJWTTokenExpiration(token);
} else {
throw e;
}
}
```
What this means is that originally the `validateToken` call would throw `UnknownTokenException`. With this change, the `UnknownTokenException` is caught and `getJWTTokenExpiration` is called. If `getJWTTokenExpiration` fails, the code throws `IllegalArgumentException`
So it has the same effect of converting `UnknownTokenException` to `IllegalArgumentException` based on the settings introduced.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385194987
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -92,9 +93,17 @@ protected long getMaxLifetime(final String token) {
@Override
public long getTokenExpiration(final String token) throws UnknownTokenException {
long expiration = 0;
-
- validateToken(token);
-
+ try {
+ validateToken(token);
+ } catch (final UnknownTokenException e) {
+ /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
+ if (permissiveFailureEnabled && StringUtils
+ .containsIgnoreCase(e.toString(), "Unknown token")) {
Review comment:
To Kevin's point, why do you need to check the message if it's an UnknownTokenException?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #274:
KNOX-2212 - Token permissiveness
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #274: KNOX-2212 - Token permissiveness
URL: https://github.com/apache/knox/pull/274#discussion_r385194104
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -249,8 +249,10 @@
private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.interval";
private static final String KNOX_TOKEN_EVICTION_GRACE_PERIOD = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.eviction.grace.period";
+ private static final String KNOX_TOKEN_PERMISSIVE_FAILURE_ENABLED = GATEWAY_CONFIG_FILE_PREFIX + ".knox.token.permissive.failure.enabled";
Review comment:
I don't love this config property name. Perhaps, **.knox.token.permissive** is sufficient ? Or, **.knox.token.permissive.validation** ?
----------------------------------------------------------------
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
With regards,
Apache Git Services