Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
pzampino merged PR #949: URL: https://github.com/apache/knox/pull/949 -- 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
Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
pzampino commented on code in PR #949:
URL: https://github.com/apache/knox/pull/949#discussion_r1835034993
##
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:
@smolnar82 Thank you for the thoughtful review. I've made the agreed upon
changes.
--
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
Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
smolnar82 commented on code in PR #949:
URL: https://github.com/apache/knox/pull/949#discussion_r1834501261
##
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:
Hi @pzampino!
Yes, I'm fine with those new proposed variable names, they sound more
accurate.
Thanks for your reply!
--
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
Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
pzampino commented on code in PR #949:
URL: https://github.com/apache/knox/pull/949#discussion_r1833401823
##
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:
The booleans are intended to indicate which methods have been attempted, and
they avoid additional (albeit minimal) method overhead; They're effectively
caching the result of the evaluations you're proposing to repeat. I do see your
point about hasJWKS being a little misleading, but I'm not convinced it matters
since we only really care if they're BOTH (PEM, JWKS) missing. I could be
persuaded to rename them to something like attemptedPEMVerification and
attemptedJwksVerification, which more accurately reflect their respective
intentions.
What do you think?
--
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
Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
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.
--
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
Re: [PR] KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable [knox]
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
