Conversation
…sertion (envoyproxy#46699) Commit Message: oauth2: add assertion_audience and source key_id from the token secret Adds `assertion_audience` to `PrivateKeyJwtConfig`, setting the `aud` claim of the JWT client assertion. RFC 7523 Section 3 requires `aud` to identify the authorization server and permits the token endpoint URL; RFC 9126 Section 2.1 recommends the authorization server's issuer identifier and FAPI 2.0 Section 5.3.3.1 requires it. When unset, the `aud` claim remains the configured `token_endpoint` URI. Also allows `token_secret` to be supplied as a multi-entry generic secret with a `private_key` entry holding the PEM-encoded key and an optional `key_id` entry. When the `key_id` entry is present, its value is set as the `kid` header parameter of the client assertion per RFC 7515 Section 4.1.4, which providers use to select the correct verification key when a client has more than one key registered. Single-value secrets continue to hold the key directly and emit no `kid` header, so existing configurations are unaffected. Supporting this required exposing the named entries of a multi-entry `GenericSecret` through `ThreadLocalGenericSecretProvider`, which previously only surfaced the single-value form. That change is additive: the existing accessor is unchanged and existing consumers are unaffected. Additional Description: Private key JWT client authentication was requested in envoyproxy#40127 and implemented in envoyproxy#44005. When testing that support against ID-Porten (Norway's public sector IDP for norwegian citizens), token requests were rejected for two reasons: the provider expects its issuer identifier as the assertion audience rather than the token endpoint URL, and it requires a `kid` header to select the verification key among the several keys registered for the client. Neither value is configurable today. The `aud` claim is derived from `token_endpoint` and no `kid` header is emitted at all. The two values are sourced differently because they change on different terms. The assertion audience identifies the configured authorization server, so it belongs in filter configuration alongside the endpoints it refers to. The key ID identifies the signing key, so it belongs in the secret that carries that key: when the key rotates, the new key ID arrives with it, and the two can never be observed out of step. This PR was developed with AI assistance. I have reviewed and fully understand all of the code being submitted. Risk Level: Low (small optional feature; new fields are opt-in and defaults preserve existing behavior) Testing: Unit tests. * `test/common/secret/secret_provider_impl_test.cc` (new): named-entry lookup for single-value and multi-entry secrets, absent entries, empty secrets, and the unreadable-entry error path. * `filter_test`: `SDSSecretReader` resolving the key and key ID from a multi-entry secret, a single-value secret yielding no key ID, and a missing client secret provider; the `kid` header present when the secret carries a key ID and absent when it does not; audience defaulting to the token endpoint URI, an explicitly configured audience, and decoding the generated assertion to assert the resulting `aud` claim. * `client_assertion_test`: `kid` present when supplied, omitted when empty, and JSON-escaped so a value from a secret store cannot break out of the JWT header. * `config_test`: loading a configuration that sets `assertion_audience`. Docs Changes: API documentation inline with the proto in `api/envoy/extensions/filters/http/oauth2/v3/oauth.proto`. The new `assertion_audience` field, and the `token_secret` field documenting both accepted secret forms and the `private_key` / `key_id` entry names. No changes to docs/root were needed. Release Notes: Added `changelogs/current/new_features/oauth2__assertion-audience-and-key-id.rst`. Platform Specific Features: N/A Runtime guard: N/A. Both additions are opt-in and preserve existing behavior for configurations that do not use them. Fixes #Issue: N/A — follow-up to envoyproxy#40127 / envoyproxy#44005, which are already closed. API Considerations(https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md): * Feature enablement: both additions are opt-in. Unset `assertion_audience` preserves the current `aud` value (the `token_endpoint` URI), and a single-value `token_secret` is read exactly as today with no `kid` header emitted. No existing user is affected without a config change. * WKT wrapping: `assertion_audience` is a `string` where empty unambiguously means "unset"; there is no plausible future default that would require a wrapper type. * Validation: no protoc-gen-validate rules are added. `aud` may be any string per RFC 7519, so constraining it in the API would risk rejecting legitimate provider configurations. The `key_id` entry is data inside a secret rather than a config field and is not validated at config load. * Existing field semantics: `token_secret` now accepts the multi-entry `GenericSecret` form in addition to the single value form. `GenericSecret` already defines the multi-entry form for distributing associated secrets together, and the entry names are documented on the field. cc @kanurag94 --------- Signed-off-by: Lars Sørensen <larssorensen98@hotmail.com> (cherry picked from commit e06565d) Signed-off-by: Lars Sørensen <54811127+larsore@users.noreply.github.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Contributor
Author
|
/retest |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #46699.
Nominated via
backport/reviewon the original PR; opening this so the stable maintainers can evaluate it directly.This adds API surface to a stable branch, which I recognise is the main argument against it. The case for considering it anyway:
private_key_jwtshipped in 1.39.0 but cannot be used at all with providers that require the authorization server's issuer identifier as the assertion audience (mandated by FAPI 2.0 §5.3.3.1) or akidheader to select among multiple registered keys. ID-Porten, the national identity provider for Norwegian citizens, requires both. Both additions are opt-in and inert unless configured, so no existing 1.39 configuration changes behaviour.Not a clean pick: 1.39 predates the exception-free
FilterConfigmigration (#46323) and theASSERT_OKtest migration, so the tests were adapted to the branch's existing constructor and assertion style. Non-test changes apply unmodified.//test/extensions/filters/http/oauth2/...and//test/common/secret/...pass against the 1.39 base.