Implement acr_values for oidc

[matya]

Overview

This feature enables users of the OIDC authentication flow to specify ACR values, as outlined by the OIDC specification. This is mostly used when performing MFA and instructs the auth provider about the required methods (example for Okta).

The modification introduces a new optional configuration option called acr_values in the API, which provides the necessity interface to configure such values. Without this, no other way exists to enforce the security requirements that are enforced by the ACR values.

Design of Change

The change is aligned with existing parameters and settings within the codepaths, and present a diff with minimal complexity and easy testability, as the backend only needs to append another key=value pair to the querystring of the auth_url, which is fairly straigt forward using the already present implementation in the used hashicorp/cap package.

Related Issues/Pull Requests

[ ] Issue #275
[ ] PR #1234

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link
Example
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[ ] Backwards compatible

Documentation is WIP.

[hashicorp-cla]

All committers have signed the CLA.

[matya]

OTOH on second reading I am not sure this is not a per-role configuration.

[matya]

I reworked it to support acr_values on both config and role levels.

Please give some feedback if this would fit into the general design of the plugin/auth method.

[matya]

The tests have passed.

=== RUN   TestOIDC_AuthURL_acr_values
=== RUN   TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_config
=== RUN   TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_both_role_and_config
=== RUN   TestOIDC_AuthURL_acr_values/auth_URL_for_empty_role_acr_values
=== RUN   TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_role
--- PASS: TestOIDC_AuthURL_acr_values (1.50s)
    --- PASS: TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_config (0.75s)
    --- PASS: TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_both_role_and_config (0.21s)
    --- PASS: TestOIDC_AuthURL_acr_values/auth_URL_for_empty_role_acr_values (0.21s)
    --- PASS: TestOIDC_AuthURL_acr_values/auth_URL_with_acr_values_for_role (0.22s)

[matya]

Asking for some insights if there are any ideas on if/how you would accept a contribution for the support of acr values
@austingebauer

[austingebauer]

Hi @matya - Thanks for this contribution. I'm looking at this and will provide my thoughts.

[austingebauer]

@matya - I'm curious to hear your thoughts on how we should be processing the arc claim returned on ID tokens. I'm not seeing us validating it here or in the cap/oidc package, but I could be missing something.

My understanding from the OIDC spec is that requesting it via acr_values (as we are here) makes it a "voluntary claim". The spec isn't super clear on how to process the acr claim when looking at https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.

If the acr Claim was requested, the Client SHOULD check that the asserted Claim Value is appropriate. The meaning and processing of acr Claim Values is out of scope for this specification.

[matya]

@matya - I'm curious to hear your thoughts on how we should be processing the arc claim returned on ID tokens. I'm not seeing us validating it here or in the cap/oidc package, but I could be missing something.

My understanding from the OIDC spec is that requesting it via acr_values (as we are here) makes it a "voluntary claim". The spec isn't super clear on how to process the acr claim when looking at https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation.

Validating claims can already be performed by bound_claims so I was not sure if I wanted to pollute the config options with something like validate_acr as a bool, or add a mandatory validation of the returned acr value in the token.

So either

1. Documentation example of how to implement acr validation via existing methods, or
2. Explicit validation, or
3. Configurable (default true for safe-by-design) validation.

This also is the least to most effort for implementing.

[austingebauer]

@matya - I'm mostly looking for an answer on what is technically correct per the OIDC spec or common in other OIDC relying parties (i.e., clients) making use of acr_values. The OIDC spec says "the Client SHOULD check that the asserted Claim Value is appropriate." but also leaves the processing out of scope. I'll probe some other identity folks at HashiCorp and peek across other codebases to see.

[matya]

@matya - I'm mostly looking for an answer on what is technically correct per the OIDC spec or common in other OIDC relying parties (i.e., clients) making use of acr_values. The OIDC spec says "the Client SHOULD check that the asserted Claim Value is appropriate." but also leaves the processing out of scope. I'll probe some other identity folks at HashiCorp and peek across other codebases to see.

I have found some time to search the publicly available hashicorp repos. For now what I have found to be similar in function is this:
https://github.com/hashicorp/consul/blob/90117613e8891450d75b840c7d60b87382748fc6/internal/go-sso/oidcauth/oidc.go#L63

I didn't find any validation of validation for that feature. Maybe you find something else in other non public repos or if you find the time, can discuss it with the appropriate people.

[matya]

@austingebauer Sorry to bug you, just wanted to clarify if this is something that can be considered for contribution? I would start working on a PR-proposal on the documentation or rework it if any feedback could be provided.