-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix cryptic 'The supplier returned null
' message if OIDC server connection fails
#26661
Conversation
null
' message if OIDC server connection fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @famod
Looks like an exception is structured differently in case of the connection time out, as just recently we dealt with the issue where an |
Well, with this change you will see the requested endpoint in the trace. Is that too much? |
@famod Hmm, I haven't thought about it, while the current message is clearly not good enough, it will also a problem in that other case, the internal endpoint address leaking to to the client, see #26528 and the linked issue. |
Hum, after reading that issue I can't help thinking that crippling exception messages feels like fixing it on the wrong end. TBH, I don't like hiding such info via DEBUG and would rather like to see a WARN message. This shouldn't be any issue in terms of info leakage (somebody who can read your logs can probably also read your environment vars...). |
@famod No, the problem is about leaking it to the external client, with this PR, will the calling client get the internal address of Keycloak ? May be I am missing something ? |
@famod when As I said I agree we need to fix this cryptic message, my only concern is whether it will leak the internal KC address or not to the external client, this is why I suggested to report |
Well, my scenario is not like in #26314 (where the info leak seems to occur when an OIDC secured resource is involved). @RegisterRestClient(configKey = "my-connector")
@RegisterProvider(MyOidcClientRequestFilter.class) ( So, long story short: I cannot tell you how or whether at all this can happen if a REST resource is OIDC secured and cannot reach the respective OIDC server. I guess it's safest to proceed as you suggested: throw a more general exception but also log all the details as a warning. |
.onFailure().transform(t -> { | ||
LOG.warn("OIDC Server is not available:", t.getCause() != null ? t.getCause() : t); | ||
// don't wrap t to avoid information leak | ||
return new OidcClientException("OIDC Server is not available"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose OidcClientException
instead of OIDCException
because this class already throws OidcClientException
in multiple places, but not OIDCException
.
Please let me know in case you prefer OIDCException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@famod Ha-ha, I've only now realized you are fixing OidcClientImpl
and not OidcRecorder
:-), but your update is still important IMHO, I think it looks fine now, thanks
With the change now pushed, the original |
Is it really fixed ? Quarkus version: 2.11.1.Final
} My application.properties: Error: |
I was getting:
Only in the debugger I was able to see that the actual cause was: