feat(auth): add Microsoft Entra ID support with On-Behalf-Of token exchange#953
feat(auth): add Microsoft Entra ID support with On-Behalf-Of token exchange#953manusa merged 1 commit intocontainers:mainfrom
Conversation
5f76f86 to
c419416
Compare
Cali0707
left a comment
There was a problem hiding this comment.
Thanks for all the hard work getting entra id to work @nader-ziada !
I did a first pass looking through the PR, left some comments/questions
…change Add support for Microsoft Entra ID (Azure AD) as an OIDC provider add JWT client assertion support for Entra ID OBO flow Implement RFC 7523 JWT Client Assertion for Microsoft Entra ID On-Behalf-Of token exchange, enabling certificate-based authentication as an alternative to client secrets. Signed-off-by: Nader Ziada <nziada@redhat.com>
|
Positives:
Minor observations (non-blocking):
|
manusa
left a comment
There was a problem hiding this comment.
Hey @nader-ziada, thanks for the great work on this. The security posture throughout (fail-closed on token exchange failure, opt-in trust_proxy_headers, path-traversal guards) is spot on.
One optional cleanup if you feel like it: IsRequireOAuth() looks like dead code now. After moving Derived() over to ResolveClusterAuthMode(), I couldn't find any remaining callers.
- Interface:
pkg/api/config.go:24 - Impl:
pkg/config/config.go:394
$ grep -rn 'IsRequireOAuth()'
pkg/api/config.go:24: IsRequireOAuth() bool
pkg/config/config.go:394:func (c *StaticConfig) IsRequireOAuth() bool {
The RequireOAuth field and require_oauth TOML key are obviously still needed, only the method. Feel free to drop it in this PR or leave it for a follow-up, whichever you prefer.
Thanks again!
Cali0707
left a comment
There was a problem hiding this comment.
LGTM, thanks @nader-ziada !!
The IsRequireOAuth() method and AuthProvider interface became dead code in containers#953 when Derived() moved to ResolveClusterAuthMode(). The RequireOAuth field and require_oauth TOML key are still needed — only the method was unreferenced. Refs containers#953 Signed-off-by: Marc Nuri <marc@marcnuri.com>
* refactor(config): drop unused IsRequireOAuth method The IsRequireOAuth() method and AuthProvider interface became dead code in #953 when Derived() moved to ResolveClusterAuthMode(). The RequireOAuth field and require_oauth TOML key are still needed — only the method was unreferenced. Refs #953 Signed-off-by: Marc Nuri <marc@marcnuri.com> * test(auth): add Entra ID / OBO follow-up coverage - Strengthen the OBO happy path: assert the exchanged token — not the original user token — reaches the backend, replacing the removed log substring assertion with an observable behavior check. - Add TestAuthorizationOidcTokenExchangeSSE that drives the same flow over mcp.SSEClientTransport. SSE relies on AuthorizationMiddleware injecting the validated bearer into the request context; StreamableHTTP propagates headers directly, so the context-injection path was previously untested (#1043). - Add TrustProxyHeadersSuite covering client.address and url.scheme gating in RequestMiddleware. Assertions read OTel span attributes from an in-memory recorder installed via TestMain (#1044). Closes #1043 Closes #1044 Signed-off-by: Marc Nuri <marc@marcnuri.com> --------- Signed-off-by: Marc Nuri <marc@marcnuri.com>
Add support for Microsoft Entra ID (Azure AD) as an OIDC provider
Changes:
entra-obotoken exchange strategy for On-Behalf-Of flowcluster_auth_modethat has two values:passthroughandkubeconfig