feat: Add optional OIDC token audience and issuer verification - #6670
Conversation
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6670 +/- ##
=======================================
Coverage 46.44% 46.44%
=======================================
Files 414 414
Lines 50134 50134
Branches 7173 7173
=======================================
Hits 23285 23285
Misses 25212 25212
Partials 1637 1637
*This pull request uses carry forward flags. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
ntkathole
left a comment
There was a problem hiding this comment.
Let's have it in operator as well
|
Thanks for the quick review. Operator follow-up PR coming right up: adding audience and issuer to the OIDC secret property whitelist so they flow into the generated feature_store.yaml auth section, plus tests. |
|
Operator follow-up is tracked in #6676 and the change is ready on my side (whitelist additions for audience and issuer in the OIDC Secret, tests, and docs; verify_ssl and ca_cert_path stay CR fields since they already flow through the CRD). One question on sequencing: happy for this one to merge now with the operator PR following right behind, or would you rather hold it and land the two together? Either works for me. |
|
will merge this, @aniketpalu fyi |
The OIDC token parser verifies signature (JWKS via discovery) and expiry, but never the audience or issuer: any validly-signed, unexpired token from the configured IdP authenticates regardless of which resource it was minted for, leaving RBAC role matching as the only gate. Add two optional OidcAuthConfig fields, audience and issuer, both unset by default. When set, the corresponding claim must match or the token is rejected at authentication; when unset, the decode options are identical to before, so existing deployments are unaffected. Opt-in rather than strict-by-default because IdPs commonly mint tokens whose claims differ from the discovery metadata: Entra ID issues v1.0 tokens (iss under sts.windows.net, api:// audience) that are validated against a v2.0 discovery URL, which works because discovery is used only to source JWKS keys. A dedicated test pins that setup so it cannot silently regress. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Cover the new audience and issuer options in the OIDC authorization page, including the caveat that expected values must match the token's claims rather than the discovery document (the Entra ID v1.0-token case). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
fad9e3b to
67074d0
Compare
Follow-up to feast-dev#6670, requested in review: add audience and issuer to OidcOptionalSecretProperties so operators can set them in the referenced OIDC Secret and have them flow into the generated feature_store.yaml auth section, enabling the new opt-in claim verification on the feature server. Absent keys change nothing. Documents the two optional Secret keys in the operator security guide with a pointer to the OIDC authorization page for the token-claims vs discovery-metadata caveat. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
) * feat: Pass optional OIDC audience and issuer through the operator Follow-up to #6670, requested in review: add audience and issuer to OidcOptionalSecretProperties so operators can set them in the referenced OIDC Secret and have them flow into the generated feature_store.yaml auth section, enabling the new opt-in claim verification on the feature server. Absent keys change nothing. Documents the two optional Secret keys in the operator security guide with a pointer to the OIDC authorization page for the token-claims vs discovery-metadata caveat. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * fix: Coerce numeric OIDC audience and issuer Secret values to strings Secret values are YAML-parsed on extraction, so an all-digits audience (e.g. a numeric IdP application ID) arrives as a Go int, renders unquoted in the generated feature_store.yaml, and fails the SDK's Optional[str] validation at server startup. Coerce the two claim keys back to strings in the allowlist copy, with a regression test. Also extend the reconcile-level envtest fixtures with both keys so they exercise real Secret extraction and YAML serialization rather than only the mocked property map. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * docs: Sharpen operator OIDC audience/issuer docs and fix dead links State precisely which claims are and are not checked, scope the checks to the standard OIDC/JWKS path (ServiceAccount and intra-server tokens follow separate paths), distinguish the Secret's issuer key from the CR's issuerUrl, and document three operational caveats: pre-existing Secret keys activate on operator upgrade, the IdP must mint matching claims for Feast's own client token flows, and Secret edits apply on the next reconcile. Point the two dead ../reference/auth links at real pages and add the new optional keys to the linked sample Secret as commented lines. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
…-dev#6670) * feat: Add optional OIDC token audience and issuer verification The OIDC token parser verifies signature (JWKS via discovery) and expiry, but never the audience or issuer: any validly-signed, unexpired token from the configured IdP authenticates regardless of which resource it was minted for, leaving RBAC role matching as the only gate. Add two optional OidcAuthConfig fields, audience and issuer, both unset by default. When set, the corresponding claim must match or the token is rejected at authentication; when unset, the decode options are identical to before, so existing deployments are unaffected. Opt-in rather than strict-by-default because IdPs commonly mint tokens whose claims differ from the discovery metadata: Entra ID issues v1.0 tokens (iss under sts.windows.net, api:// audience) that are validated against a v2.0 discovery URL, which works because discovery is used only to source JWKS keys. A dedicated test pins that setup so it cannot silently regress. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * docs: Document optional OIDC audience and issuer verification Cover the new audience and issuer options in the OIDC authorization page, including the caveat that expected values must match the token's claims rather than the discovery document (the Entra ID v1.0-token case). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
…ast-dev#6677) * feat: Pass optional OIDC audience and issuer through the operator Follow-up to feast-dev#6670, requested in review: add audience and issuer to OidcOptionalSecretProperties so operators can set them in the referenced OIDC Secret and have them flow into the generated feature_store.yaml auth section, enabling the new opt-in claim verification on the feature server. Absent keys change nothing. Documents the two optional Secret keys in the operator security guide with a pointer to the OIDC authorization page for the token-claims vs discovery-metadata caveat. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * fix: Coerce numeric OIDC audience and issuer Secret values to strings Secret values are YAML-parsed on extraction, so an all-digits audience (e.g. a numeric IdP application ID) arrives as a Go int, renders unquoted in the generated feature_store.yaml, and fails the SDK's Optional[str] validation at server startup. Coerce the two claim keys back to strings in the allowlist copy, with a regression test. Also extend the reconcile-level envtest fixtures with both keys so they exercise real Secret extraction and YAML serialization rather than only the mocked property map. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * docs: Sharpen operator OIDC audience/issuer docs and fix dead links State precisely which claims are and are not checked, scope the checks to the standard OIDC/JWKS path (ServiceAccount and intra-server tokens follow separate paths), distinguish the Secret's issuer key from the CR's issuerUrl, and document three operational caveats: pre-existing Secret keys activate on operator upgrade, the IdP must mint matching claims for Feast's own client token flows, and Secret edits apply on the next reconcile. Point the two dead ../reference/auth links at real pages and add the new optional keys to the linked sample Secret as commented lines. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
What this PR does / why we need it
Adds optional audience and issuer verification to the OIDC token parser. Today the parser verifies signature and expiry only, so any validly-signed, unexpired token from the configured IdP authenticates regardless of which resource it was minted for; RBAC role matching is the only remaining gate.
Two new
OidcAuthConfigfields,audienceandissuer, both unset by default:When set,
jwt.decoderuns withverify_aud/verify_issenabled against the configured values, and a mismatched or missing claim rejects the token at authentication. When unset, the decode options are identical to before, so existing deployments see no behavior change; all pre-existing tests are untouched and passing.Opt-in rather than strict-by-default because IdPs commonly mint tokens whose claims differ from their discovery metadata. Entra ID issues v1.0 tokens (
issundersts.windows.net,api://audience) that are routinely validated against a v2.0 discovery URL, which works because discovery is only used to source JWKS keys.test_oidc_default_supports_v1_tokens_against_v2_discoverypins that setup so a future refactor cannot silently regress it. Operators state expected values explicitly rather than the code inferring them from the discovery document, for the same reason.Testing: 11 new unit tests. Config-to-decode fidelity (the verified decode enables exactly the checks the config provides), end-to-end RS256-signed tokens through the real
jwt.decode(match accepted, mismatch rejected, missing claim rejected, default permissive), and the v1/v2 pin.tests/unit/permissions: 307 passing; ruff and mypy clean on the changed files.Docs: the OIDC section of
authz_manager.mddocuments the new options and warns that expected values must match the token's claims, not the discovery document.Not included: the feast-operator's OIDC secret-key whitelist (
OidcOptionalSecretProperties) does not yet passaudience/issuerthrough to the generatedfeature_store.yaml. Happy to follow up separately if there is interest in exposing these via the operator CRD.Which issue(s) this PR fixes
Fixes #6669