smolnar82 commented on code in PR #949:
URL: https://github.com/apache/knox/pull/949#discussion_r1830444857


##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/AbstractJWTFilter.java:
##########
@@ -512,17 +520,22 @@ protected boolean verifyTokenSignature(final JWT token) {
     // If it has not yet been verified, then perform the verification now
     if (!verified) {
       try {
+        boolean hasPem  = false;
+        boolean hasJWKS = false;
+
         if (publicKey != null) {
+          hasPem = true;
           verified = authority.verifyToken(token, publicKey);
           log.pemVerificationResultMessage(verified);
         }
 
         if (!verified && expectedJWKSUrls != null && 
!expectedJWKSUrls.isEmpty()) {
+          hasJWKS = true;
           verified = authority.verifyToken(token, expectedJWKSUrls, 
expectedSigAlg, allowedJwsTypes);
           log.jwksVerificationResultMessage(verified);
         }
 
-        if(!verified) {
+        if(!verified && ((!hasPem && !hasJWKS) || isJwtInstanceKeyFallback)) {

Review Comment:
   I think `hasPEM` and `hasJWKS` are redundant (or need a better name).
   First, `hasJWKS` is misleading, because it's only set to `true` when
   - no PEM configured
   - PEM configured, but the PEM verification attempt failed
   
   So even if JWKS is configured, it might remain `false`.
   
   If I were you, I'd only rely on the already existing class members as 
follows:
   - new private method: `private boolean configuredPEM() { return publicKey != 
null; }`
   - new private method `private boolean configuredJWKS() { return  
expectedJWKSUrls != null && !expectedJWKSUrls.isEmpty(); }`
   - then, you could use these new methods in lines 526 and 532 as well as in 
the new condition in line 538. Even better, I'd create another new private 
method for that purpose:
   ```
   private boolean verifyInstanceKeys() { //name might be changing
     return isJwtInstanceKeyFallback || (!hasPEM() && !hasJWKS());
   }
   ```
   So the updated condition would look like this: `if (!verified && 
verifyInstanceKeys()) {`
   Thus, the new boolean variables can be removed.



-- 
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

Reply via email to