[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=395370=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-395370 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 29/Feb/20 01:40 Start Date: 29/Feb/20 01:40 Worklog Time Spent: 10m Work Description: moresandeep commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 395370) Time Spent: 4h (was: 3h 50m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 4h > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394986=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394986
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 28/Feb/20 16:16
Start Date: 28/Feb/20 16:16
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394986)
Time Spent: 3h 50m (was: 3h 40m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 3h 50m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394984=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394984
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 28/Feb/20 16:13
Start Date: 28/Feb/20 16:13
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 394984)
Time Spent: 3h 40m (was: 3.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 3h 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394982=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394982
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 28/Feb/20 16:06
Start Date: 28/Feb/20 16:06
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 394982)
Time Spent: 3.5h (was: 3h 20m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 3.5h
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394962=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394962
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 28/Feb/20 15:44
Start Date: 28/Feb/20 15:44
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394962)
Time Spent: 3h 20m (was: 3h 10m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 3h 20m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394440=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394440
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 20:14
Start Date: 27/Feb/20 20:14
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 394440)
Time Spent: 3h 10m (was: 3h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 3h 10m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394432=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394432 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 19:53 Start Date: 27/Feb/20 19:53 Worklog Time Spent: 10m Work Description: moresandeep commented on 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 Issue Time Tracking --- Worklog Id: (was: 394432) Time Spent: 3h (was: 2h 50m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 3h > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394431=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394431 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 19:53 Start Date: 27/Feb/20 19:53 Worklog Time Spent: 10m Work Description: moresandeep commented on 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 Issue Time Tracking --- Worklog Id: (was: 394431) Time Spent: 2h 50m (was: 2h 40m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 2h 50m > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394430=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394430
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 19:51
Start Date: 27/Feb/20 19:51
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 394430)
Time Spent: 2h 40m (was: 2.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 2h 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394414=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394414
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 19:14
Start Date: 27/Feb/20 19:14
Worklog Time Spent: 10m
Work Description: risdenk commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394414)
Time Spent: 2.5h (was: 2h 20m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 2.5h
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394310=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394310
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 16:36
Start Date: 27/Feb/20 16:36
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394310)
Time Spent: 2h 20m (was: 2h 10m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 2h 20m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394304=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394304
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 16:32
Start Date: 27/Feb/20 16:32
Worklog Time Spent: 10m
Work Description: risdenk commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394304)
Time Spent: 2h (was: 1h 50m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 2h
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394305=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394305 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 16:32 Start Date: 27/Feb/20 16:32 Worklog Time Spent: 10m Work Description: risdenk commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 394305) Time Spent: 2h 10m (was: 2h) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 2h 10m > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394267=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394267
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 15:58
Start Date: 27/Feb/20 15:58
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394267)
Time Spent: 1h 40m (was: 1.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394265=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394265 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 15:58 Start Date: 27/Feb/20 15:58 Worklog Time Spent: 10m Work Description: pzampino commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 394265) Time Spent: 1.5h (was: 1h 20m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 1.5h > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394263=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394263 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 15:58 Start Date: 27/Feb/20 15:58 Worklog Time Spent: 10m Work Description: pzampino commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 394263) Time Spent: 1h 20m (was: 1h 10m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 1h 20m > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394264=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394264 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 27/Feb/20 15:58 Start Date: 27/Feb/20 15:58 Worklog Time Spent: 10m Work Description: pzampino commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 394264) Time Spent: 1.5h (was: 1h 20m) > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 1.5h > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394266=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394266
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 15:58
Start Date: 27/Feb/20 15:58
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394266)
Time Spent: 1h 40m (was: 1.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394269=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394269
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 15:58
Start Date: 27/Feb/20 15:58
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394269)
Time Spent: 1h 50m (was: 1h 40m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h 50m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=394268=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-394268
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 27/Feb/20 15:58
Start Date: 27/Feb/20 15:58
Worklog Time Spent: 10m
Work Description: pzampino commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 394268)
Time Spent: 1h 40m (was: 1.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=393755=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-393755
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 26/Feb/20 21:19
Start Date: 26/Feb/20 21:19
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 393755)
Time Spent: 1h 10m (was: 1h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=393013=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-393013
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 22:35
Start Date: 25/Feb/20 22:35
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 393013)
Time Spent: 1h (was: 50m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 1h
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=393010=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-393010
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 22:26
Start Date: 25/Feb/20 22:26
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 393010)
Time Spent: 50m (was: 40m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 50m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=393008=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-393008
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 22:23
Start Date: 25/Feb/20 22:23
Worklog Time Spent: 10m
Work Description: moresandeep commented on 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
Issue Time Tracking
---
Worklog Id: (was: 393008)
Time Spent: 40m (was: 0.5h)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 40m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=392943=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392943
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 21:22
Start Date: 25/Feb/20 21:22
Worklog Time Spent: 10m
Work Description: risdenk commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392943)
Time Spent: 0.5h (was: 20m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=392941=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392941
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 21:22
Start Date: 25/Feb/20 21:22
Worklog Time Spent: 10m
Work Description: risdenk commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392941)
Time Spent: 20m (was: 10m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[
https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=392942=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392942
]
ASF GitHub Bot logged work on KNOX-2212:
Author: ASF GitHub Bot
Created on: 25/Feb/20 21:22
Start Date: 25/Feb/20 21:22
Worklog Time Spent: 10m
Work Description: risdenk commented on 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:
us...@infra.apache.org
Issue Time Tracking
---
Worklog Id: (was: 392942)
Time Spent: 20m (was: 10m)
> TokenStateService should fail permissively
> --
>
> Key: KNOX-2212
> URL: https://issues.apache.org/jira/browse/KNOX-2212
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
>Affects Versions: 1.4.0
>Reporter: Philip Zampino
>Assignee: Sandeep More
>Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> While unlikely, it's possible that the TokenStateService may be presented
> with a token, for which it has no state, but which can be verified as a valid
> (origin, expiration, etc...) token. In these cases, rather than rejecting the
> token for lack of server-managed state, the token's state should be recorded,
> and the associated operation permitted.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Work logged] (KNOX-2212) TokenStateService should fail permissively
[ https://issues.apache.org/jira/browse/KNOX-2212?focusedWorklogId=392934=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-392934 ] ASF GitHub Bot logged work on KNOX-2212: Author: ASF GitHub Bot Created on: 25/Feb/20 21:13 Start Date: 25/Feb/20 21:13 Worklog Time Spent: 10m Work Description: moresandeep commented on 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: us...@infra.apache.org Issue Time Tracking --- Worklog Id: (was: 392934) Remaining Estimate: 0h Time Spent: 10m > TokenStateService should fail permissively > -- > > Key: KNOX-2212 > URL: https://issues.apache.org/jira/browse/KNOX-2212 > Project: Apache Knox > Issue Type: Bug > Components: Server >Affects Versions: 1.4.0 >Reporter: Philip Zampino >Assignee: Sandeep More >Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > While unlikely, it's possible that the TokenStateService may be presented > with a token, for which it has no state, but which can be verified as a valid > (origin, expiration, etc...) token. In these cases, rather than rejecting the > token for lack of server-managed state, the token's state should be recorded, > and the associated operation permitted. -- This message was sent by Atlassian Jira (v8.3.4#803005)
