[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #274: KNOX-2212 - Token permissiveness
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:
us...@infra.apache.org
With regards,
Apache Git Services
