OIDC CodeAuthenticationMechanism should not fail silently

[rgmz]

Description

When users are redirected back to Quarkus after authentication, it's possible for their request to 'fail silently' (return a 401 with no body) if there's an issue calling the token endpoint. Because no body is returned, and errors are logged at debug level, it can be difficult to troubleshoot the 401.

One could argue that this doesn't need to be logged, but in my experience it's confusing to see a "401" and not be able to tell if it's coming from the application or the IdP. I encountered this issue after following the "Using OpenID Connect (OIDC) to Protect Web Applications..." and accidentally setting quarkus.oidc.secret instead of quarkus.oidc.credentials.secret; it wasn't obvious that "401" mean "error trying to authenticate because of invalid credentials".

2021-11-16 16:29:25,837 DEBUG [io.qua.oid.run.OidcProviderClient] (vert.x-eventloop-thread-1) Request has failed: status: 401, error message: {"error_description":"Invalid client or client credentials.","error":"invalid_client"}
2021-11-16 16:29:25,837 DEBUG [io.qua.oid.run.CodeAuthenticationMechanism] (vert.x-eventloop-thread-1) Exception during the code to token exchange: {"error_description":"Invalid client or client credentials.","error":"invalid_client"}

Implementation ideas

Two potential ways to make this more obvious:

1. Log failures at the WARN level instead of DEBUG
   

   quarkus/extensions/oidc/runtime/src/main/java/io/quarkus/oidc/runtime/CodeAuthenticationMechanism.java

   Line 300 in 2e8c6a8

   LOG.debugf("Exception during the code to token exchange: %s", tOuter.getMessage());

2. (Any security concerns?) Include the body when an unexpected error occurs (not sure if there are security concerns for this)
3. (May not be practical) Validate the quarkus configuration to check for a client secret... assuming all IdPs require one. :)
4. (May not be practical) Return a 5xx error if there's an unexpected authentication error. For example, Quarkus throws an error if you provide an invalid auth-server-url:

   io.quarkus.oidc.OIDCException: OIDC server is not available at the '...' URL. Please make sure it is correct.

[quarkus-bot]

/cc @pedroigor, @sberyozkin

[sberyozkin]

Hi @rgmz

Thanks for opening this issue, but IMHO it won't need to be addressed.
The same guide has a section, How to check the errors in the logs (it is less visible in the 2.4-LATEST guide) - 401 does warrant checking the logs.

Let me also comment further

Log failures at the WARN level instead of DEBUG

We don't do it for the security errors

(Any security concerns?) Include the body when an unexpected error occurs (not sure if there are security concerns for this)

Yes, that would be giving the attacker too much information and helping with trying more variations.

(May not be practical) Validate the quarkus configuration to check for a client secret... assuming all IdPs require one. :)

It is a good idea but only if we could know it was necessary to always have a client_secret - besides there could be other reasons why the code flow can fail to complete.

(May not be practical) Return a 5xx error

This is similar to the earlier comment - the flow may fail to complete for many other reasons, or the secret may be set but the secret value mistyped.

IMHO checking the logs at the debug level is a reasonable but also secure approach.

However, you've reminded me about the problem with the https://quarkus.io/version/main/guides/security-openid-connect-web-authentication guide: it should really not use a public client in the quickstart - Quarkus endpoints are acting as the confidential clients so they really should have the client secrets set - so I'll address this particular guide issue (which should prevent similar typos in the future) in order to resolve this issue

Thanks

[sberyozkin]

Marking it as a OIDC web-app guide bug

[rgmz]

@sberyozkin: Thanks for the quick reply; I'll provide a thoughtful response later.

I also noticed that the UserInfo section of that guide refers to quarkus.oidc.user-info-required, which isn't a known configuration key (it should be quarkus.oidc.authenticaton.user-info-required).

If I noticed any other issues with the OIDC docs, would you prefer that I open separate issues for each, or note them in the existing meta-issue (#20036)?

[sberyozkin]

Hi @rgmz Oh, thanks for spotting it :-). It would be fine to open dedicated issues, the epic issue is a high level one to improve the security docs. If you'd like, please create the one for the userinfo typo, a PR will be welcome too