Skip to content

Convert client_auth_method to a Symbol for quirked OmniAuth providers

What does this MR do?

Prior to this change, all configurations that followed the published method for configuring OpenIDConnect providers would POST the client id and secret as part of the body for authorization. This is not the recommended default method specified in the OIDC RFC.

The underlying infrastructure gem does allow for this to be configured by changing 'client_auth_method', however, due to the way the configuration values are saved and loaded, fidelity is lost since all hash values get turned into strings. The rack-oauth2 gem requires the client_auth_method to be specified as a Symbol, thus configuring any value here results in what what we're seeing (defaulting to POSTing in the request body).

This MR will quirk OpenIDConnect providers (and derivatives that use the OmniAuth::Strategies::OpenIDConnect strategy class) that leverage the rack-oauth2 gem for authorization requests by converting client_auth_method to a Symbol if it's specified as an argument to the provider. In this way, Gitlab will now support Basic Auth, JWT Bearer and mTLS options via OpenIDConnect

There is currently a PR for rack-oauth2 to convert client_auth_method to a Symbol if necessary that may make this MR moot.

Does this MR meet the acceptance criteria?

Conformity

Performance and testing

Should have no performance impact.

Test coverage for all OmniAuth providers would technically be necessary, but seems infeasible from a maintenance perspective. I think relying on the test coverage of the dependent gems is enough.

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team

Closes #64407 (closed)

Edited by Stan Hu

Merge request reports