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/06/02 01:49:35 UTC
[GitHub] [knox] pzampino opened a new pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
pzampino opened a new pull request #337:
URL: https://github.com/apache/knox/pull/337
… frequently
## What changes were proposed in this pull request?
The goal of these changes is to reduce the number of times during a token state reaper iteration that the keystore file is loaded and subsequently written. This change reduces that number from (1 + 2-to-4-per-token) to (1 + 0-to-1-per-token + 1). So, for 100 expired tokens and 100 unexpired tokens, that means going from 601 loads to 2 loads (if all the token state aliases are cached) or up to 202 in the absolute worst case when none of the token state aliases is cached.
Part of the solution includes adding bulk alias/key removal method to the AliasService and KeystoreService respectively.
## How was this patch tested?
I've added multiple tests to exercise these changes: AliasBasedTokenStateServiceTest, DefaultTokenStateServiceTest, DefaultKeystoreServiceTest, RemoteAliasServiceTest, and ZookeeperRemoteAliasServiceTest. I'm continuing manual testing, but wanted to get the review process started as soon as possible.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r434053333
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##########
@@ -357,6 +359,30 @@ public void removeCredentialForCluster(String clusterName, String alias) throws
}
}
+ @Override
+ public void removeCredentialsForCluster(String clusterName, Set<String> aliases) throws KeystoreServiceException {
+ synchronized (this) {
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ // Delete all the entries
+ for (String alias : aliases) {
+ if (ks.containsAlias(alias)) {
+ ks.deleteEntry(alias);
+ }
+ removeFromCache(clusterName, alias);
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433856451
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -319,16 +366,17 @@ protected void evictExpiredTokens() {
*/
protected boolean needsEviction(final String tokenId) throws UnknownTokenException {
// If the expiration time(+ grace period) has already passed, it should be considered expired
- long expirationWithGrace = getTokenExpiration(tokenId) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
+ long expirationWithGrace = getTokenExpiration(tokenId, false) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
return (expirationWithGrace <= System.currentTimeMillis());
}
/**
* Get a list of tokens
*
- * @return List of tokens
+ * @return
*/
protected List<String> getTokens() {
- return new ArrayList<>(tokenExpirations.keySet());
+ return tokenExpirations.keySet().stream().collect(Collectors.toList());
Review comment:
Yes, good catch
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433859014
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -254,7 +291,7 @@ protected void removeToken(final String tokenId) throws UnknownTokenException {
protected boolean hasRemainingRenewals(final String tokenId, long renewInterval) {
// Is the current time + 30-second buffer + the renewal interval is less than the max lifetime for the token?
- return ((System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(30) + renewInterval) < getMaxLifetime(tokenId));
+ return ((System.currentTimeMillis() + 30000 + renewInterval) < getMaxLifetime(tokenId));
Review comment:
The change to the literal value was unintentional
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r434062618
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -237,6 +238,13 @@ public void removeAliasForCluster(final String clusterName, final String alias)
}
}
+ @Override
+ public void removeAliasesForCluster(String clusterName, Set<String> aliases) throws AliasServiceException {
Review comment:
yup, this was just a note :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r434900949
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -135,6 +138,25 @@ public JWT issueToken(Principal p, List<String> audiences, String algorithm, lon
return issueToken(p, audiences, algorithm, expires, null, null, null);
}
+ private RSAPrivateKey getSigningKey(final String signingKeystoreName,
+ final String signingKeystoreAlias,
+ final char[] signingKeystorePassphrase)
+ throws KeystoreServiceException, TokenServiceException {
+
+ if (signingKeystorePassphrase != null) {
+ char[] passphrase;
+ try {
+ passphrase = getSigningKeyPassphrase(signingKeystorePassphrase);
+ } catch (AliasServiceException e) {
+ throw new TokenServiceException(e);
+ }
Review comment:
+1
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -177,11 +192,7 @@ public JWT issueToken(Principal p, List<String> audiences, String algorithm, lon
}
private char[] getSigningKeyPassphrase(char[] signingKeyPassphrase) throws AliasServiceException {
Review comment:
+1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433675431
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -319,16 +366,17 @@ protected void evictExpiredTokens() {
*/
protected boolean needsEviction(final String tokenId) throws UnknownTokenException {
// If the expiration time(+ grace period) has already passed, it should be considered expired
- long expirationWithGrace = getTokenExpiration(tokenId) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
+ long expirationWithGrace = getTokenExpiration(tokenId, false) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod);
return (expirationWithGrace <= System.currentTimeMillis());
}
/**
* Get a list of tokens
*
- * @return List of tokens
+ * @return
*/
protected List<String> getTokens() {
- return new ArrayList<>(tokenExpirations.keySet());
+ return tokenExpirations.keySet().stream().collect(Collectors.toList());
Review comment:
Should not we synchronize here too?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
Review comment:
`Map.replace` also runs a `containsKey`:
```
The default implementation is equivalent to, for this map:
if (map.containsKey(key)) {
return map.put(key, value);
} else
return null;
```
I think a simple `put` is enough: you want to make sure you have the given expiration in `tokenExpirations`
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -109,30 +133,52 @@ public long getTokenExpiration(final String tokenId) throws UnknownTokenExceptio
@Override
protected boolean isUnknown(final String tokenId) {
- boolean isUnknown = false;
- try {
- isUnknown = (aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId) == null);
- } catch (AliasServiceException e) {
- log.errorAccessingTokenState(tokenId, e);
+ boolean isUnknown = super.isUnknown(tokenId);
+
+ // If it's not in the cache, then check the underlying alias
+ if (isUnknown) {
+ try {
+ isUnknown = (aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId) == null);
+ } catch (AliasServiceException e) {
+ log.errorAccessingTokenState(tokenId, e);
+ }
}
return isUnknown;
}
@Override
protected void removeToken(final String tokenId) throws UnknownTokenException {
- validateToken(tokenId);
-
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId);
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId + TOKEN_MAX_LIFETIME_POSTFIX);
log.removedTokenState(tokenId);
} catch (AliasServiceException e) {
log.failedToRemoveTokenState(tokenId, e);
}
+ super.removeToken(tokenId);
+ }
Review comment:
Why not invoke the new method as `removeTokens(Collections.singleton(tokenId))`?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -254,7 +291,7 @@ protected void removeToken(final String tokenId) throws UnknownTokenException {
protected boolean hasRemainingRenewals(final String tokenId, long renewInterval) {
// Is the current time + 30-second buffer + the renewal interval is less than the max lifetime for the token?
- return ((System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(30) + renewInterval) < getMaxLifetime(tokenId));
+ return ((System.currentTimeMillis() + 30000 + renewInterval) < getMaxLifetime(tokenId));
Review comment:
Why is this 30 seconds buffer added?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
}
}
protected void removeToken(final String tokenId) throws UnknownTokenException {
validateToken(tokenId);
+ removeTokenState(tokenId);
+ }
+
+ /**
+ * Bulk removal of the specified tokens.
+ *
+ * @param tokenIds The unique identifiers of the tokens whose state should be removed.
+ *
+ * @throws UnknownTokenException
+ */
+ protected void removeTokens(final Set<String> tokenIds) throws UnknownTokenException {
+ // Duplicating the logic from removeToken(String) here because this method is supposed to be an optimization for
+ // sub-classes that access an external store, for which bulk token removal performs better than individual removal.
+ // Sub-classes will have implemented removeToken(String), and calling that here will undo any optimizations provided
+ // by the sub-class's implementation of this method.
+ for (String tokenId : tokenIds) {
+ removeTokenState(tokenId);
+ }
+ }
+
+ private void removeTokenState(final String tokenId) {
Review comment:
I'm not sure if I agree that any non-optimized code should be left if we already know that code piece will result in causing performance issues.
My two cents here:
1. in `removeToken` I'd call `removeTokens(Collections.singleton(tokenId))`
2. in `removeTokens` I'd **not** call a synchronized block `N` times. Instead, you may synchronize once, fetch the keyset and call the `removeAll` on that key set -> that collection is
> backed by the map, so changes to the map are reflected in the set, and vice-versa.
Something like this:
```
synchronized (tokenExpirations) {
tokenExpirations.keySet().removeAll(tokenIds);
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433853293
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
Review comment:
Yes, I had meant to return to this after verifying that the replace invocation that was already there is not needed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433858751
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -254,7 +291,7 @@ protected void removeToken(final String tokenId) throws UnknownTokenException {
protected boolean hasRemainingRenewals(final String tokenId, long renewInterval) {
// Is the current time + 30-second buffer + the renewal interval is less than the max lifetime for the token?
- return ((System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(30) + renewInterval) < getMaxLifetime(tokenId));
+ return ((System.currentTimeMillis() + 30000 + renewInterval) < getMaxLifetime(tokenId));
Review comment:
It is not added as part of this change, and I would have to revisit why it is there.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433875101
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##########
@@ -357,6 +359,30 @@ public void removeCredentialForCluster(String clusterName, String alias) throws
}
}
+ @Override
+ public void removeCredentialsForCluster(String clusterName, Set<String> aliases) throws KeystoreServiceException {
+ synchronized (this) {
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ // Delete all the entries
+ for (String alias : aliases) {
+ if (ks.containsAlias(alias)) {
+ ks.deleteEntry(alias);
+ }
+ removeFromCache(clusterName, alias);
Review comment:
Instead of invalidating one entry at a time we can bulk invalidate them using [invalidateAll(keys)](https://www.javadoc.io/doc/com.github.ben-manes.caffeine/caffeine/1.2.0/com/github/benmanes/caffeine/cache/Cache.html#invalidateAll-java.lang.Iterable-) API
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -237,6 +238,13 @@ public void removeAliasForCluster(final String clusterName, final String alias)
}
}
+ @Override
+ public void removeAliasesForCluster(String clusterName, Set<String> aliases) throws AliasServiceException {
Review comment:
Not related to review but just a side note: we should make some calls to RemoteAliasService non blocking, it would help a lot.
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -143,15 +189,21 @@ protected void updateExpiration(final String tokenId, long expiration) {
@Override
protected List<String> getTokens() {
- List<String> allAliases = new ArrayList<>();
+ List<String> tokenIds = null;
+
try {
- allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
- /* only get the aliases that represent tokens and extract the current list of tokens */
- allAliases = allAliases.stream().filter(a -> a.contains(TOKEN_MAX_LIFETIME_POSTFIX)).map(a -> a.substring(0, a.indexOf(TOKEN_MAX_LIFETIME_POSTFIX)))
- .collect(Collectors.toList());
+ List<String> allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
+
+ // Filter for the token state aliases, and extract the token ID
+ tokenIds = allAliases.stream()
+ .filter(a -> a.contains(TOKEN_MAX_LIFETIME_POSTFIX))
+ .map(a -> a.substring(0, a.indexOf(TOKEN_MAX_LIFETIME_POSTFIX)))
+ .collect(Collectors.toList());
} catch (AliasServiceException e) {
- log.errorEvictingTokens(e);
+ e.printStackTrace(); // TODO: PJZ: Logging
Review comment:
Why are we throwing a stack trace? if this is for debugging can we wrap it up in logger and then throw instead of using e.printStackTrace().
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r434053520
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -237,6 +238,13 @@ public void removeAliasForCluster(final String clusterName, final String alias)
}
}
+ @Override
+ public void removeAliasesForCluster(String clusterName, Set<String> aliases) throws AliasServiceException {
Review comment:
Out of scope for this change IMO
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433855809
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -237,12 +249,37 @@ protected boolean isUnknown(final String token) {
protected void updateExpiration(final String tokenId, long expiration) {
synchronized (tokenExpirations) {
- tokenExpirations.replace(tokenId, expiration);
+ if (tokenExpirations.containsKey(tokenId)) {
+ tokenExpirations.replace(tokenId, expiration);
+ } else {
+ tokenExpirations.put(tokenId, expiration);
+ }
}
}
protected void removeToken(final String tokenId) throws UnknownTokenException {
validateToken(tokenId);
+ removeTokenState(tokenId);
+ }
+
+ /**
+ * Bulk removal of the specified tokens.
+ *
+ * @param tokenIds The unique identifiers of the tokens whose state should be removed.
+ *
+ * @throws UnknownTokenException
+ */
+ protected void removeTokens(final Set<String> tokenIds) throws UnknownTokenException {
+ // Duplicating the logic from removeToken(String) here because this method is supposed to be an optimization for
+ // sub-classes that access an external store, for which bulk token removal performs better than individual removal.
+ // Sub-classes will have implemented removeToken(String), and calling that here will undo any optimizations provided
+ // by the sub-class's implementation of this method.
+ for (String tokenId : tokenIds) {
+ removeTokenState(tokenId);
+ }
+ }
+
+ private void removeTokenState(final String tokenId) {
Review comment:
+1
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433919383
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -143,15 +189,21 @@ protected void updateExpiration(final String tokenId, long expiration) {
@Override
protected List<String> getTokens() {
- List<String> allAliases = new ArrayList<>();
+ List<String> tokenIds = null;
+
try {
- allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
- /* only get the aliases that represent tokens and extract the current list of tokens */
- allAliases = allAliases.stream().filter(a -> a.contains(TOKEN_MAX_LIFETIME_POSTFIX)).map(a -> a.substring(0, a.indexOf(TOKEN_MAX_LIFETIME_POSTFIX)))
- .collect(Collectors.toList());
+ List<String> allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
+
+ // Filter for the token state aliases, and extract the token ID
+ tokenIds = allAliases.stream()
+ .filter(a -> a.contains(TOKEN_MAX_LIFETIME_POSTFIX))
+ .map(a -> a.substring(0, a.indexOf(TOKEN_MAX_LIFETIME_POSTFIX)))
+ .collect(Collectors.toList());
} catch (AliasServiceException e) {
- log.errorEvictingTokens(e);
+ e.printStackTrace(); // TODO: PJZ: Logging
Review comment:
This is an oversight. Thanks for catching it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r434880140
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -177,11 +192,7 @@ public JWT issueToken(Principal p, List<String> audiences, String algorithm, lon
}
private char[] getSigningKeyPassphrase(char[] signingKeyPassphrase) throws AliasServiceException {
Review comment:
No longer throws `AliasServiceException`
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java
##########
@@ -135,6 +138,25 @@ public JWT issueToken(Principal p, List<String> audiences, String algorithm, lon
return issueToken(p, audiences, algorithm, expires, null, null, null);
}
+ private RSAPrivateKey getSigningKey(final String signingKeystoreName,
+ final String signingKeystoreAlias,
+ final char[] signingKeystorePassphrase)
+ throws KeystoreServiceException, TokenServiceException {
+
+ if (signingKeystorePassphrase != null) {
+ char[] passphrase;
+ try {
+ passphrase = getSigningKeyPassphrase(signingKeystorePassphrase);
+ } catch (AliasServiceException e) {
+ throw new TokenServiceException(e);
+ }
Review comment:
Since `getSigningKeyPassphrase` no longer throws `AliasServiceException ` -> the try/catch block is unnecesarry.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino merged pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino merged pull request #337:
URL: https://github.com/apache/knox/pull/337
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433922377
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/ZookeeperRemoteAliasService.java
##########
@@ -237,6 +238,13 @@ public void removeAliasForCluster(final String clusterName, final String alias)
}
}
+ @Override
+ public void removeAliasesForCluster(String clusterName, Set<String> aliases) throws AliasServiceException {
Review comment:
I'm not sure what you mean here. Can you elaborate?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on pull request #337:
URL: https://github.com/apache/knox/pull/337#issuecomment-638506230
> Besides my review comments below I'm not sure if I got the idea of the `unpersistedState` collection in `AliasBasedTokenStateService`. Could you please update the PR description to make it cleaner for me :)
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433848858
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -109,30 +133,52 @@ public long getTokenExpiration(final String tokenId) throws UnknownTokenExceptio
@Override
protected boolean isUnknown(final String tokenId) {
- boolean isUnknown = false;
- try {
- isUnknown = (aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId) == null);
- } catch (AliasServiceException e) {
- log.errorAccessingTokenState(tokenId, e);
+ boolean isUnknown = super.isUnknown(tokenId);
+
+ // If it's not in the cache, then check the underlying alias
+ if (isUnknown) {
+ try {
+ isUnknown = (aliasService.getPasswordFromAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId) == null);
+ } catch (AliasServiceException e) {
+ log.errorAccessingTokenState(tokenId, e);
+ }
}
return isUnknown;
}
@Override
protected void removeToken(final String tokenId) throws UnknownTokenException {
- validateToken(tokenId);
-
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId);
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, tokenId + TOKEN_MAX_LIFETIME_POSTFIX);
log.removedTokenState(tokenId);
} catch (AliasServiceException e) {
log.failedToRemoveTokenState(tokenId, e);
}
+ super.removeToken(tokenId);
+ }
Review comment:
Yes, that would be better than invoking the individual removal method twice, now that there is the bulk removal method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #337: KNOX-2375 - Token state eviction should access the keystore file less…
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #337:
URL: https://github.com/apache/knox/pull/337#discussion_r433921301
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultKeystoreService.java
##########
@@ -357,6 +359,30 @@ public void removeCredentialForCluster(String clusterName, String alias) throws
}
}
+ @Override
+ public void removeCredentialsForCluster(String clusterName, Set<String> aliases) throws KeystoreServiceException {
+ synchronized (this) {
+ KeyStore ks = getCredentialStoreForCluster(clusterName);
+ if (ks != null) {
+ try {
+ // Delete all the entries
+ for (String alias : aliases) {
+ if (ks.containsAlias(alias)) {
+ ks.deleteEntry(alias);
+ }
+ removeFromCache(clusterName, alias);
Review comment:
Is there any benefit in doing it this way?
----------------------------------------------------------------
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