KNOX-3073 - Token verification fallback to Knox keys behavior should be configurable

[pzampino]

What changes were proposed in this pull request?

Added a topology-level JWTProvider param for enabling token signature verification fallback to Knox's key(s) when a PEM and/or JWKS URL(s) are explicitly configured for the verification. By default, if one or both of those explicit methods is configured, the verification will not fallback to Knox's key(s).

How was this patch tested?

Unit tests and manual testing was performed to validate the flow and conditions.

[smolnar82]

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.

[pzampino]

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?

[smolnar82]

Hi @pzampino!

Yes, I'm fine with those new proposed variable names, they sound more accurate.

Thanks for your reply!

[pzampino]

@smolnar82 Thank you for the thoughtful review. I've made the agreed upon changes.