smolnar82 commented on code in PR #924:
URL: https://github.com/apache/knox/pull/924#discussion_r1671617962
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenAuthorityService.java:
##########
@@ -83,6 +84,8 @@ public class DefaultTokenAuthorityService implements
JWTokenAuthority, Service {
// https://tools.ietf.org/html/rfc7518
private static final Set<String> SUPPORTED_PKI_SIG_ALGS = new
HashSet<>(Arrays.asList("RS256", "RS384", "RS512", "PS256", "PS384", "PS512"));
private static final Set<String> SUPPORTED_HMAC_SIG_ALGS = new
HashSet<>(Arrays.asList("HS256", "HS384", "HS512"));
+ /* cache JWKS endpoint for 2 hrs in case of outage */
+ private static final long OUTAGE_TTL = 2*60*60*1000;
Review Comment:
Having the default set to 2 hours is good, but, IMO, this should be
configurable on the topology level.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
@@ -110,7 +114,7 @@ public void init( FilterConfig filterConfig ) throws
ServletException {
// JWKSUrl
String oidcjwksurl = filterConfig.getInitParameter(JWKS_URL);
Review Comment:
IIWU, I'd introduce the new `knox.token.jwks.urls` config name now and mark
the previous one (`knox.token.jwks.url`) deprecated. Have the code handle both
for now and remove the support for the old one in the next release.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java:
##########
@@ -115,7 +116,7 @@ public abstract class AbstractJWTFilter implements Filter {
private String expectedIssuer;
private String expectedSigAlg;
protected String expectedPrincipalClaim;
- protected String expectedJWKSUrl;
+ protected Set<URI> expectedJWKSUrl = new HashSet();
Review Comment:
The class member should be renamed (even if the config name does not change
in this release) to `expectedJWKSUrls`.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/JWTMessages.java:
##########
@@ -123,4 +123,7 @@ public interface JWTMessages {
@Message( level = MessageLevel.INFO, text = "Token verification result using
knox signing cert, verified: {0}" )
void signingKeyVerificationResultMessage(boolean verified);
+
+ @Message(level = MessageLevel.ERROR, text = "Invalid URL ignored. Not a
valid JWKS url {0}")
+ void notValidJwksUrl(String jwksUrl);
Review Comment:
I'd call this method `invalidJwksUrl`.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTFederationFilter.java:
##########
@@ -149,6 +153,30 @@ public void init( FilterConfig filterConfig ) throws
ServletException {
configureExpectedParameters(filterConfig);
}
+ /**
+ * Helper function to extract URLs from given string
+ * in the form of https://url:port/contxt/.wellknown,
https://url2:port/contxt/.wellknown
+ * into expectedJWKSUrl URL set.
+ * @param oidcjwksurl
+ */
+ private Set<URI> getJwksUrlsFromConfig(final String oidcjwksurl) {
Review Comment:
The parameter should be renamed to `oidcJWKSUrls`.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@knox.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
