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/03/09 18:18:29 UTC
[GitHub] [knox] pzampino opened a new pull request #284: KNOX-2266 - Tokens
Should Include a Unique Identifier
pzampino opened a new pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284
## What changes were proposed in this pull request?
JWTs issued and validated by Knox now include a unique identifier as a private claim. This is mostly to guarantee token uniqueness, even for multiple requests within the same second.
Further, the TokenStateService has been updated to leverage this unique identifier as the key for handling token state. This identifier is less susceptible to the nuances of some storage mechanisms.
## How was this patch tested?
Mulitple existing tests were modified to accommodate this change while ensuring the maintenance of existing behavior. TokenServiceResourceTest#testConcurrentGetToken was added to validate these changes. I've also done a bit of manual testing.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390320621
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
+ expiration = Long.parseLong(exp);
+ }
+ }
+
+ if (expiration == -1) {
Review comment:
Sometimes -1 is treated as a special value, like unlimited expiry. I do agree that in context of Knox token (which we control) -1 = expired token
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390059082
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
Review comment:
I don't agree that toString() is a replacement for getMessage(), but I will account for the possibility of a null result from getMessage().
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390034382
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -184,33 +195,22 @@ public long renewToken(final String token, long renewInterval) throws UnknownTok
@Override
public void revokeToken(final JWTToken token) throws UnknownTokenException {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
- revokeToken(token.getPayload());
+ revokeToken(TokenUtils.getTokenId(token));
}
@Override
- public void revokeToken(final String token) throws UnknownTokenException {
+ public void revokeToken(final String tokenId) throws UnknownTokenException {
/* no reason to keep revoked tokens around */
- removeToken(token);
- log.revokedToken(TokenUtils.getTokenDisplayText(token));
+ removeToken(tokenId);
+ log.revokedToken(tokenId);
}
@Override
public boolean isExpired(final JWTToken token) throws UnknownTokenException {
- return isExpired(token.getPayload());
- }
-
- @Override
- public boolean isExpired(final String token) throws UnknownTokenException {
- boolean isExpired;
- isExpired = isUnknown(token); // Check if the token exist
- if (!isExpired) {
- // If it not unknown, check its expiration
- isExpired = (getTokenExpiration(token) <= System.currentTimeMillis());
- }
- return isExpired;
+ return getTokenExpiration(token) <= System.currentTimeMillis();
Review comment:
This method would throw `UnknownTokenException` if a token is not found. Exceptions are expensive (filling up stack trace and unwinding it every time it is passed to a new method) so should `getTokenExpiration(token)` return some negative value instead?
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390035230
##########
File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
##########
@@ -246,9 +249,14 @@ public Response renew(String token) {
String renewer = SubjectUtils.getCurrentEffectivePrincipalName();
if (allowedRenewers.contains(renewer)) {
try {
+ JWTToken jwt = new JWTToken(token);
// If renewal fails, it should be an exception
- expiration = tokenStateService.renewToken(token,
+ expiration = tokenStateService.renewToken(jwt,
renewInterval.orElse(tokenStateService.getDefaultRenewInterval()));
+ log.renewedToken(getTopologyName(), TokenUtils.getTokenDisplayText(token), TokenUtils.getTokenId(jwt));
+ } catch (ParseException e) {
+ log.invalidToken(getTopologyName(), TokenUtils.getTokenDisplayText(token), e);
+ error = e.getMessage();
Review comment:
e.getMessage() will throw NPE in case message is null.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390063658
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
+ expiration = Long.parseLong(exp);
+ }
+ }
+
+ if (expiration == -1) {
Review comment:
If that code means (-1 = now), that seems quite strange to me, and I don't think Knox tokens require such a convention.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390030016
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
Review comment:
e.getMessage() can throw a NPE in case there is no message, e.toString() is safer.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390319159
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -184,33 +195,22 @@ public long renewToken(final String token, long renewInterval) throws UnknownTok
@Override
public void revokeToken(final JWTToken token) throws UnknownTokenException {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
- revokeToken(token.getPayload());
+ revokeToken(TokenUtils.getTokenId(token));
}
@Override
- public void revokeToken(final String token) throws UnknownTokenException {
+ public void revokeToken(final String tokenId) throws UnknownTokenException {
/* no reason to keep revoked tokens around */
- removeToken(token);
- log.revokedToken(TokenUtils.getTokenDisplayText(token));
+ removeToken(tokenId);
+ log.revokedToken(tokenId);
}
@Override
public boolean isExpired(final JWTToken token) throws UnknownTokenException {
- return isExpired(token.getPayload());
- }
-
- @Override
- public boolean isExpired(final String token) throws UnknownTokenException {
- boolean isExpired;
- isExpired = isUnknown(token); // Check if the token exist
- if (!isExpired) {
- // If it not unknown, check its expiration
- isExpired = (getTokenExpiration(token) <= System.currentTimeMillis());
- }
- return isExpired;
+ return getTokenExpiration(token) <= System.currentTimeMillis();
Review comment:
Yup, it does.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390058084
##########
File path: gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java
##########
@@ -246,9 +249,14 @@ public Response renew(String token) {
String renewer = SubjectUtils.getCurrentEffectivePrincipalName();
if (allowedRenewers.contains(renewer)) {
try {
+ JWTToken jwt = new JWTToken(token);
// If renewal fails, it should be an exception
- expiration = tokenStateService.renewToken(token,
+ expiration = tokenStateService.renewToken(jwt,
renewInterval.orElse(tokenStateService.getDefaultRenewInterval()));
+ log.renewedToken(getTopologyName(), TokenUtils.getTokenDisplayText(token), TokenUtils.getTokenId(jwt));
+ } catch (ParseException e) {
+ log.invalidToken(getTopologyName(), TokenUtils.getTokenDisplayText(token), e);
+ error = e.getMessage();
Review comment:
Yes, getMessage() may return null. I will add logic to handle that, but I don't believe replacing it with toString() is equivalent.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390032171
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
+ expiration = Long.parseLong(exp);
+ }
+ }
+
+ if (expiration == -1) {
Review comment:
-1 seems like a valid value for expiration (might not be now but in the future and for different types of tokens) , Azure uses it https://github.com/apache/hadoop/pull/1872/files#diff-c20196161db7a6b5e0f9d390ab924209R397.
----------------------------------------------------------------
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 #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390058899
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -97,48 +94,62 @@ public long getDefaultMaxLifetimeDuration() {
@Override
public void addToken(final JWTToken token, long issueTime) {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token cannot be null.");
}
- addToken(token.getPayload(), issueTime, token.getExpiresDate().getTime());
+ addToken(TokenUtils.getTokenId(token), issueTime, token.getExpiresDate().getTime());
}
@Override
- public void addToken(final String token, long issueTime, long expiration) {
- addToken(token, issueTime, expiration, getDefaultMaxLifetimeDuration());
+ public void addToken(final String tokenId, long issueTime, long expiration) {
+ addToken(tokenId, issueTime, expiration, getDefaultMaxLifetimeDuration());
}
@Override
- public void addToken(final String token,
- long issueTime,
- long expiration,
- long maxLifetimeDuration) {
- if (!isValidIdentifier(token)) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ public void addToken(final String tokenId,
+ long issueTime,
+ long expiration,
+ long maxLifetimeDuration) {
+ if (!isValidIdentifier(tokenId)) {
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
synchronized (tokenExpirations) {
- tokenExpirations.put(token, expiration);
+ tokenExpirations.put(tokenId, expiration);
}
- setMaxLifetime(token, issueTime, maxLifetimeDuration);
- log.addedToken(TokenUtils.getTokenDisplayText(token), getTimestampDisplay(expiration));
+ setMaxLifetime(tokenId, issueTime, maxLifetimeDuration);
+ log.addedToken(tokenId, getTimestampDisplay(expiration));
}
@Override
- public long getTokenExpiration(final String token) throws UnknownTokenException {
- long expiration;
-
+ public long getTokenExpiration(final JWT token) throws UnknownTokenException {
+ long expiration = -1;
try {
- validateToken(token);
- } catch (final UnknownTokenException e) {
- /* if token permissiveness is enabled we check JWT token expiration when the token state is unknown */
- if (permissiveValidationEnabled && getJWTTokenExpiration(token).isPresent()) {
- return getJWTTokenExpiration(token).getAsLong();
- } else {
+ expiration = getTokenExpiration(TokenUtils.getTokenId(token));
+ } catch (UnknownTokenException e) {
+ if (permissiveValidationEnabled) {
+ String exp = token.getExpires();
+ if (exp != null) {
+ log.permissiveTokenHandling(TokenUtils.getTokenId(token), e.getMessage());
+ expiration = Long.parseLong(exp);
+ }
+ }
+
+ if (expiration == -1) {
Review comment:
In terms of milliseconds since the epoch, I don't know what -1 could mean as a valid expiration time. I'm interested to understand the Azure treatment 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] pzampino commented on a change in pull request #284:
KNOX-2266 - Tokens Should Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284#discussion_r390058525
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -184,33 +195,22 @@ public long renewToken(final String token, long renewInterval) throws UnknownTok
@Override
public void revokeToken(final JWTToken token) throws UnknownTokenException {
if (token == null) {
- throw new IllegalArgumentException("Token data cannot be null.");
+ throw new IllegalArgumentException("Token identifier cannot be null.");
}
- revokeToken(token.getPayload());
+ revokeToken(TokenUtils.getTokenId(token));
}
@Override
- public void revokeToken(final String token) throws UnknownTokenException {
+ public void revokeToken(final String tokenId) throws UnknownTokenException {
/* no reason to keep revoked tokens around */
- removeToken(token);
- log.revokedToken(TokenUtils.getTokenDisplayText(token));
+ removeToken(tokenId);
+ log.revokedToken(tokenId);
}
@Override
public boolean isExpired(final JWTToken token) throws UnknownTokenException {
- return isExpired(token.getPayload());
- }
-
- @Override
- public boolean isExpired(final String token) throws UnknownTokenException {
- boolean isExpired;
- isExpired = isUnknown(token); // Check if the token exist
- if (!isExpired) {
- // If it not unknown, check its expiration
- isExpired = (getTokenExpiration(token) <= System.currentTimeMillis());
- }
- return isExpired;
+ return getTokenExpiration(token) <= System.currentTimeMillis();
Review comment:
Since I think the UnknownTokenException scenario should be exceptional, I think it's ok as it is. It's not intended to be a regularly occurring logic path. Does that make sense?
----------------------------------------------------------------
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 merged pull request #284: KNOX-2266 - Tokens Should
Include a Unique Identifier
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #284: KNOX-2266 - Tokens Should Include a Unique Identifier
URL: https://github.com/apache/knox/pull/284
----------------------------------------------------------------
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