[
https://issues.apache.org/jira/browse/KNOX-3073?focusedWorklogId=942107&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-942107
]
ASF GitHub Bot logged work on KNOX-3073:
----------------------------------------
Author: ASF GitHub Bot
Created on: 06/Nov/24 11:01
Start Date: 06/Nov/24 11:01
Worklog Time Spent: 10m
Work Description: 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 || (!configuredPEM() && !configuredJWKS());
}
```
So the updated condition would look like this: `if (!verified &&
verifyInstanceKeys()) {`
Thus, the new boolean variables can be removed.
Issue Time Tracking
-------------------
Worklog Id: (was: 942107)
Time Spent: 0.5h (was: 20m)
> Token verification fallback to Knox keys behavior should configurable
> ---------------------------------------------------------------------
>
> Key: KNOX-3073
> URL: https://issues.apache.org/jira/browse/KNOX-3073
> Project: Apache Knox
> Issue Type: Improvement
> Components: Server
> Reporter: Philip Zampino
> Assignee: Philip Zampino
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> KNOX-3040
> ntroduced support for multiple token verification mechanisms (i.e., PEM,
> jwks) for the same topology (provider instance), falling back to Knox's own
> signing and TLS keys if any of those configured should fail.
> This behavior may not be expected by some, and we should provide the ability
> to control the fallback to the Knox keys.
> To support deployments expecting the previous behavior, there should be a
> provider param for indicating that the new fall-back behavior is desired
> (e.g., instance-keys-fallback=true), which defaults to false.
> Default Behavior:
> * Neither PEM nor jwks URL(s) is configured, attempt verification using (in
> order)
> ** Knox's signing key
> ** Knox's TLS key
> * Only PEM is configured: Knox will attempt verification using ONLY the
> configured PEM
> * Only jwks URL(s) are configured: Knox will attempt verification using ONLY
> the configured jwks URL(s)
> * PEM AND jwks URL(s) are configured: Knox will attempt verification using
> ONLY (in order)
> ** The configured PEM
> ** The configured jwks URL(s).
> instance-keys-fallback=true Behavior:
> * Same as default behavior except that in the cases where PEM and/or jwks
> URL(s) are configured and fail to verify, Knox will subsequently attempt
> verification using (in order):
> ** Knox's signing key
> ** Knox's TLS key
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
