Repository navigation
Surface Kubernetes IdP key-loading failures and fix a resulting NPE - #51686
Conversation
37dfe39 to
dea7902
Compare
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improve observability and failure handling for Kubernetes IdP JWKS loading by surfacing key-loading failures distinctly from signature mismatches, and by failing fast on missing jwks_uri to avoid opaque NPEs.
Changes:
- Fail fast with a descriptive
IOExceptionwhen OIDC discovery lacksjwks_uri. - Log JWKS/key-loading failures at
WARN(and returnfalse) while leaving routine signature failures non-noisy.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| services/src/main/java/org/keycloak/broker/kubernetes/KubernetesJwksEndpointLoader.java | Adds explicit null-check for jwks_uri and throws a descriptive exception early. |
| services/src/main/java/org/keycloak/broker/kubernetes/KubernetesIdentityProvider.java | Separates key-loading failures into a WARN log path to distinguish infra/config issues from bad signatures. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } catch (Exception e) { | ||
| // Indicative of a infrastructure/configuration and can't be | ||
| // triggered by a client - worth WARN visibility. | ||
| LOGGER.warn("Failed to load public keys for Kubernetes IdP", e); |
There was a problem hiding this comment.
@onematchfox, other failures are at the debug level, so let's use the same.
The message should probably include the issuer; otherwise, it won't help much to identify the failing issue/U 10000 RL.
There was a problem hiding this comment.
Agreed on adding more details to make the alert more actionable (done and pushed).
However, I disagree with reducing the level to debug as that defeats the point of the PR to begin with (the error is currently logged at debug level). This specific failure - the JWKS/key-loading call itself throwing - is an infra/config problem (bad issuer URL, network/DNS failure, failed auth, TLS issue, etc.) that an admin needs to notice, not a normal signature-verification failure a client can trigger by presenting a bad token. I even wonder whether this shouldn't be raised to error rather than just warn since by the logging docs
ERROR | A significant error or problem leading to the inability to process requests.
WARN | A non-critical error or problem that might not require immediate correction.
A JWKS load failure doesn't just degrade something in the background - it makes this client-authentication request unprocessable. Perhaps the logging should be changed to match what JWTClientAuthenticator does here (which incidentally provides precedent for logging at error on every failed attempt).
Leaving it at debug means it stays invisible unless someone already suspects this component and enables debug logging and I don't really want to redeploy my production instance with debug logging enabled to have to determine find out that login issues come down to failures to obtain the JWKS data. The other debug-level logs in this method are genuine "token didn't verify" cases and I think it's correct for those to stay quiet.
WDYT?
There was a problem hiding this comment.
I see your point, but the same applies to other messages currently set to debug level. I would like to skip discussing (valid) changes to these messages now and just keep the same pattern as others.
We need to do this carefully because we want to avoid impacting existing deployments and flooding production logs.
Let us discuss reviewing the log level as a follow-up.
There was a problem hiding this comment.
I would like to skip discussing (valid) changes to these messages now
Not 100% sure I'm following you here. Which changes are valid in your opinion? Just the changes to prevent an NPE in services/src/main/java/org/keycloak/broker/kubernetes/KubernetesJwksEndpointLoader.java? Those were a secondary concern - but I can completely see them being their own PR if that's your preference. As for the changes to this file (i.e. reducing the log to debug) - in that case these changes are not needed at all - the existing handling here is more than sufficient to identify the issue (if debug logging is enabled).
We need to do this carefully because we want to avoid impacting existing deployments and flooding production logs.
Completely agree. Which is why the log level change was scoped specifically to issues in obtaining JWKS material rather than more broadly within the existing handling (linked above).
There was a problem hiding this comment.
Which changes are valid in your opinion? Just the changes to prevent an NPE in services/src/main/java/org/keycloak/broker/kubernetes/KubernetesJwksEndpointLoader.java?
Yes. But I guess I missed the primary goal of your PR ...
As for the changes to this file (i.e. reducing the log to debug) - in that case these changes are not needed at all - the existing handling here is more than sufficient to identify the issue (if debug logging is enabled).
Perhaps this is what I'm missing. If you are happy enabling debugging only for this specific category (org.keycloak.broker.kubernetes.KubernetesIdentityProvider), it works for me to just rely on the existing handling.
There was a problem hiding this comment.
Perhaps this is what I'm missing. If you are happy enabling debugging only for this specific category (org.keycloak.broker.kubernetes.KubernetesIdentityProvider), it works for me to just rely on the existing handling.
Yeah, that's the thing - I'm not happy with having to enable debug logging to figure out this kind of error. 😅 Currently if the JWKS cannot be retrieved for whatever reason anyone trying to login receives a generic "Invalid client or Invalid client credentials" and no error whatsoever will be logged (assuming this is a production service that doesn't have debug logging enabled of course). This is misleading since there can be nothing wrong with the client credentials to begin with. Failing to retrieve the JWKS is a server error (it can't be triggered by a client) and IMO, this sort of error shouldn't require redeploying a production service with debug logging enabled just to find out that's there's a problem.
P.s. There are more details of the reason why I ran into this in the first place in the issue I logged - #51682 - in case you missed that.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesJwksEndpointLoader.java:52
- The new guard handles
null, but an empty/blankjwks_uriwill still flow into request/URI-building and likely fail later with a less actionable error. Consider treating blank values the same as missing (e.g., checkisBlank()as well) and adjust the exception message accordingly.
String jwksUri = wellKnownReqest.asJson(OIDCConfigurationRepresentation.class).getJwksUri();
if (jwksUri == null) {
throw new IOException("OIDC discovery document from " + wellKnownEndpoint + " did not include a jwks_uri");
}
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesIdentityProvider.java:57
- Correct the grammar in the comment: “a infrastructure” → “an infrastructure”.
// Indicative of a infrastructure/configuration and can't be
// triggered by a client - worth WARN visibility.
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesIdentityProvider.java:58
- The WARN log message may be hard to correlate in multi-tenant/multi-IdP setups. Consider including non-sensitive context (e.g., issuer URL and/or internal IdP id / realm id) in the message so operators can quickly identify which Kubernetes IdP instance is failing to load keys.
LOGGER.warn("Failed to load public keys for Kubernetes IdP", e);
dea7902 to
bc80256
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesIdentityProvider.java:58
- This warning is still client-triggerable: during an IdP outage, each assertion matching the configured issuer/subject reaches this catch, and failed key loads do not create a cache entry (
InfinispanPublicKeyStorageProvider.java:244-273). That lets unauthenticated requests emit an unthrottled WARN stack trace; rate-limit duplicate failures permodelKey(for example, first failure at WARN and repeats at DEBUG until the retry interval expires).
// Indicative of a infrastructure/configuration and can't be
// triggered by a client - worth WARN visibility.
LOGGER.warn("Failed to load public keys for Kubernetes IdP", e);
There was a problem hiding this comment.
Thanks, @onematchfox. I think GH Copilot made a valid point.
| } catch (Exception e) { | ||
| // Indicative of a infrastructure/configuration and can't be | ||
| // triggered by a client - worth WARN visibility. | ||
| LOGGER.warn("Failed to load public keys for Kubernetes IdP", e); |
There was a problem hiding this comment.
@onematchfox, other failures are at the debug level, so let's use the same.
The message should probably include the issuer; otherwise, it won't help much to identify the failing issue/URL.
`KubernetesIdentityProvider.verifySignature()` logged any caught exception from loading the JWKS at `DEBUG` only, indistinguishable from a routine signature mismatch and invisible without enabling `DEBUG` logging. Split the two: a failure to load the keys themselves (an infra/config problem, not something a client can trigger) now logs at `WARN`, while a routine bad-signature/malformed token stays at `DEBUG` so spoofing attempts don't spam the log. Separately, `KubernetesJwksEndpointLoader` silently let a null `jwks_uri` propagate into request-building code, surfacing several calls later as an opaque `NullPointerException` deep inside `java.net.URI`. It now fails fast with a descriptive `IOException` instead. This PR was written with the help of AI. Fixes keycloak#51682 Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
bc80256 to
df0d26f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
services/src/main/java/org/keycloak/broker/kubernetes/KubernetesJwksEndpointLoader.java:51
- Please add a Kubernetes client-auth test where discovery omits
jwks_uri, verifying that authentication fails cleanly through this new branch. Existing coverage only serves a valid discovery document, so the regression from an opaque NPE to the intended loader failure is currently unprotected.
if (jwksUri == null) {
throw new IOException("OIDC discovery document from " + wellKnownEndpoint + " did not include a jwks_uri");
|
Thanks, @onematchfox ! |
KubernetesIdentityProvider.verifySignature()logged any caught exception from loading the JWKS atDEBUGonly, indistinguishable from a routine signature mismatch and invisible without enablingDEBUGlogging.Split the two: a failure to load the keys themselves (an infra/config problem, not something a client can trigger) now logs at
WARN, while a routine bad-signature/malformed token stays atDEBUGso spoofing attempts don't spam the log.Separately,
KubernetesJwksEndpointLoadersilently let a nulljwks_uripropagate into request-building code, surfacing several calls later as an opaqueNullPointerExceptiondeep insidejava.net.URI. It now fails fast with a descriptiveIOExceptioninstead.This PR was written with the help of AI.
Fixes #51682