-
Notifications
You must be signed in to change notification settings - Fork 212
Expose DCR config in operator CRD for OAuth2 upstreams #5069
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: issue_5039_dcr-2c
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -273,6 +273,21 @@ const ( | |||||
| ) | ||||||
|
|
||||||
| // UpstreamProviderConfig defines configuration for an upstream Identity Provider. | ||||||
| // | ||||||
| // Exactly one of OIDCConfig or OAuth2Config must be set and must match the | ||||||
| // declared Type: oidc-typed providers set OIDCConfig, oauth2-typed providers | ||||||
| // set OAuth2Config. The CEL rule below enforces the pairing at admission; the | ||||||
| // matching Go-level check in validateUpstreamProvider provides defense-in-depth | ||||||
| // for stored objects. | ||||||
| // | ||||||
| // The rule is structured as a chain of equality checks ending in an explicit | ||||||
| // `false`, so adding a new UpstreamProviderType value without extending this | ||||||
| // rule fails admission instead of silently demanding the OAuth2 shape. When | ||||||
| // adding a new type, extend both this rule and validateUpstreamProvider. | ||||||
| // | ||||||
| // +kubebuilder:validation:XValidation:rule="self.type == 'oidc' ? (has(self.oidcConfig) && !has(self.oauth2Config)) : self.type == 'oauth2' ? (has(self.oauth2Config) && !has(self.oidcConfig)) : false",message="type must be 'oidc' or 'oauth2'; oidcConfig must be set when type is 'oidc' and oauth2Config must be set when type is 'oauth2' (and the other must not be set)" | ||||||
| // | ||||||
| //nolint:lll // CEL validation rules exceed line length limit | ||||||
| type UpstreamProviderConfig struct { | ||||||
| // Name uniquely identifies this upstream provider. | ||||||
| // Used for routing decisions and session binding in multi-upstream scenarios. | ||||||
|
|
@@ -354,6 +369,14 @@ type OIDCUpstreamConfig struct { | |||||
|
|
||||||
| // OAuth2UpstreamConfig contains configuration for pure OAuth 2.0 providers. | ||||||
| // OAuth 2.0 providers require explicit endpoint configuration. | ||||||
| // | ||||||
| // Exactly one of ClientID or DCRConfig must be set: ClientID is used when the | ||||||
| // client is pre-provisioned out of band, DCRConfig enables RFC 7591 Dynamic | ||||||
| // Client Registration at runtime. | ||||||
| // | ||||||
| // +kubebuilder:validation:XValidation:rule="(has(self.clientId) && size(self.clientId) > 0) ? !has(self.dcrConfig) : has(self.dcrConfig)",message="exactly one of clientId or dcrConfig must be set" | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F10 / LOW / consensus 7/10 — The OAuth2 XOR rule treats Polish only. If you want the outer error to fire first, add a non-empty subfield check (e.g. |
||||||
| // | ||||||
| //nolint:lll // CEL validation rules exceed line length limit | ||||||
| type OAuth2UpstreamConfig struct { | ||||||
| // AuthorizationEndpoint is the URL for the OAuth authorization endpoint. | ||||||
| // +kubebuilder:validation:Required | ||||||
|
|
@@ -374,8 +397,10 @@ type OAuth2UpstreamConfig struct { | |||||
| UserInfo *UserInfoConfig `json:"userInfo,omitempty"` | ||||||
|
|
||||||
| // ClientID is the OAuth 2.0 client identifier registered with the upstream IDP. | ||||||
| // +kubebuilder:validation:Required | ||||||
| ClientID string `json:"clientId"` | ||||||
| // Mutually exclusive with DCRConfig: when DCRConfig is set, ClientID is obtained | ||||||
| // at runtime via RFC 7591 Dynamic Client Registration and must be left empty. | ||||||
| // +optional | ||||||
| ClientID string `json:"clientId,omitempty"` | ||||||
|
|
||||||
| // ClientSecretRef references a Kubernetes Secret containing the OAuth 2.0 client secret. | ||||||
| // Optional for public clients using PKCE instead of client secret. | ||||||
|
|
@@ -410,6 +435,81 @@ type OAuth2UpstreamConfig struct { | |||||
| // +kubebuilder:validation:MaxProperties=16 | ||||||
| // +optional | ||||||
| AdditionalAuthorizationParams map[string]string `json:"additionalAuthorizationParams,omitempty"` | ||||||
|
|
||||||
| // DCRConfig enables RFC 7591 Dynamic Client Registration against the upstream | ||||||
| // authorization server. When set, the client credentials are obtained at | ||||||
| // runtime rather than being pre-provisioned, and ClientID must be left empty. | ||||||
| // Mutually exclusive with ClientID. | ||||||
| // +optional | ||||||
| DCRConfig *DCRUpstreamConfig `json:"dcrConfig,omitempty"` | ||||||
| } | ||||||
|
|
||||||
| // DCRUpstreamConfig configures RFC 7591 Dynamic Client Registration for an | ||||||
| // OAuth 2.0 upstream. When present on an OAuth2 upstream, the authserver | ||||||
| // performs registration at runtime to obtain client credentials, replacing | ||||||
| // the need to pre-provision a ClientID. | ||||||
| // | ||||||
| // Exactly one of DiscoveryURL or RegistrationEndpoint must be set. DiscoveryURL | ||||||
| // points at an RFC 8414 / OIDC Discovery document from which the registration | ||||||
| // endpoint is resolved; RegistrationEndpoint is used directly when the upstream | ||||||
| // does not publish discovery metadata. | ||||||
| // | ||||||
| // The XOR rule uses has() alone (not has() + size() > 0) to keep the rule's | ||||||
| // estimated CEL cost under the apiserver's per-rule static budget. With | ||||||
| // `omitempty` on both fields, an unset field is absent on the wire and has() | ||||||
| // returns false; the explicit-empty-string edge case is rejected at reconcile | ||||||
| // time by ValidateOAuth2DCRConfig. | ||||||
| // | ||||||
| // +kubebuilder:validation:XValidation:rule="has(self.discoveryUrl) != has(self.registrationEndpoint)",message="exactly one of discoveryUrl or registrationEndpoint must be set" | ||||||
| // | ||||||
| //nolint:lll // CEL validation rules exceed line length limit | ||||||
| type DCRUpstreamConfig struct { | ||||||
| // DiscoveryURL is the RFC 8414 / OIDC Discovery document URL. The resolver | ||||||
| // issues a single GET against this URL (no well-known-path fallback) and | ||||||
| // reads registration_endpoint, authorization_endpoint, token_endpoint, | ||||||
| // token_endpoint_auth_methods_supported, and scopes_supported from the | ||||||
| // response. | ||||||
| // Mutually exclusive with RegistrationEndpoint. | ||||||
| // MaxLength bounds CEL cost estimation on the parent struct's | ||||||
| // XValidation rule and matches the conventional URL length cap. | ||||||
| // +optional | ||||||
| // +kubebuilder:validation:Pattern=`^https?://.*$` | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F2 / MEDIUM / consensus 9/10 — DCR URL pattern allows The Separately, the existing
Suggested change
Apply the same change at line 488 for |
||||||
| // +kubebuilder:validation:MaxLength=2048 | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F6 / LOW / consensus 8/10 — The comment above ("MaxLength bounds CEL cost estimation on the parent struct's XValidation rule") implies the cap is needed because of CEL cost. The parent rule is Fix the source comment, then re-run |
||||||
| DiscoveryURL string `json:"discoveryUrl,omitempty"` | ||||||
|
|
||||||
| // RegistrationEndpoint is the RFC 7591 registration endpoint URL used | ||||||
| // directly, bypassing discovery. When using this field, the caller is | ||||||
| // expected to also supply AuthorizationEndpoint, TokenEndpoint, and an | ||||||
| // explicit Scopes list on the parent OAuth2UpstreamConfig. | ||||||
| // Mutually exclusive with DiscoveryURL. | ||||||
| // MaxLength bounds CEL cost estimation on the parent struct's | ||||||
| // XValidation rule and matches the conventional URL length cap. | ||||||
| // +optional | ||||||
| // +kubebuilder:validation:Pattern=`^https?://.*$` | ||||||
| // +kubebuilder:validation:MaxLength=2048 | ||||||
| RegistrationEndpoint string `json:"registrationEndpoint,omitempty"` | ||||||
|
|
||||||
| // InitialAccessTokenRef is an optional reference to a Kubernetes Secret | ||||||
| // carrying an RFC 7591 §3 initial access token. When set, the resolver | ||||||
| // presents the token value as a Bearer credential on the registration | ||||||
| // request. Mirrors the ClientSecretRef pattern. | ||||||
| // +optional | ||||||
| InitialAccessTokenRef *SecretKeyRef `json:"initialAccessTokenRef,omitempty"` | ||||||
|
|
||||||
| // SoftwareID is the RFC 7591 "software_id" registration metadata value, | ||||||
| // identifying the client software independent of any particular | ||||||
| // registration instance. Typically a UUID or short identifier. | ||||||
| // +optional | ||||||
| // +kubebuilder:validation:MaxLength=255 | ||||||
| SoftwareID string `json:"softwareId,omitempty"` | ||||||
|
|
||||||
| // SoftwareStatement is the RFC 7591 "software_statement" JWT asserting | ||||||
| // metadata about the client software, signed by a party the authorization | ||||||
| // server trusts. Bounded to 4096 characters to prevent unbounded | ||||||
| // user input from inflating the CR beyond etcd object limits. | ||||||
| // +optional | ||||||
| // +kubebuilder:validation:MaxLength=4096 | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F1 / MEDIUM / consensus 10/10 — softwareStatement MaxLength=4096 may reject realistic signed JWTs (raised by CRD-specialist + security-specialist) Real-world RFC 7591 software_statements with embedded Either raise the limit (e.g. to 16384 — still well under etcd object limits) or document the cap as a deliberate constraint with operator-facing guidance for what to do when their IdP exceeds it. |
||||||
| SoftwareStatement string `json:"softwareStatement,omitempty"` | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F11 / LOW / consensus 7/10 — softwareStatement is plaintext in CR spec; no Secret-ref alternative (raised by security-specialist) RFC 7591 §2.3 software_statements are signed JWTs that authenticate the client software to the AS. Some deployments treat them as bearer-style attestations and rotate them like secrets. The current placement makes them visible to anyone with Consider an alternate |
||||||
| } | ||||||
|
|
||||||
| // TokenResponseMapping maps non-standard token response fields to standard OAuth 2.0 fields | ||||||
|
|
@@ -956,10 +1056,62 @@ func (*MCPExternalAuthConfig) validateUpstreamProvider(index int, provider *Upst | |||||
| return fmt.Errorf("%s: unsupported provider type: %s", prefix, provider.Type) | ||||||
| } | ||||||
|
|
||||||
| // Validate OAuth2-specific DCR / ClientID constraints (defense-in-depth with CEL). | ||||||
| if provider.Type == UpstreamProviderTypeOAuth2 && provider.OAuth2Config != nil { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F12 / LOW / consensus 7/10 — Dead defensive nil-check on The pairing check at line 1052 already guarantees: if Type is OAuth2 and we're past that check,
Suggested change
|
||||||
| if err := ValidateOAuth2DCRConfig(prefix, provider.OAuth2Config); err != nil { | ||||||
| return err | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Validate additionalAuthorizationParams does not contain reserved keys | ||||||
| return ValidateAdditionalAuthorizationParams(prefix, provider.AdditionalAuthorizationParams()) | ||||||
| } | ||||||
|
|
||||||
| // ValidateOAuth2DCRConfig enforces the mutual exclusivity between ClientID and | ||||||
| // DCRConfig and, when DCRConfig is present, between DiscoveryURL and | ||||||
| // RegistrationEndpoint. These rules mirror the CEL validation on | ||||||
| // OAuth2UpstreamConfig and DCRUpstreamConfig and provide defense-in-depth for | ||||||
| // stored objects (e.g., objects stored before CEL rules were added or | ||||||
| // validated through code paths that bypass admission). | ||||||
| // | ||||||
| // The prefix is embedded in error messages to identify the offending upstream | ||||||
| // (e.g., "upstreamProviders[i]"). Pass an empty string when the caller already | ||||||
| // wraps the error with the upstream identifier, to avoid duplicating it. | ||||||
| // Exported so the controllerutil conversion layer can reuse the same | ||||||
| // invariants when building runtime configs, rejecting malformed objects at | ||||||
| // reconcile time rather than waiting until the authserver process parses them. | ||||||
| func ValidateOAuth2DCRConfig(prefix string, cfg *OAuth2UpstreamConfig) error { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F3 / MEDIUM / consensus 8/10 —
Add the rejection here and a matching CEL counterpart on |
||||||
| hasClientID := cfg.ClientID != "" | ||||||
| hasDCR := cfg.DCRConfig != nil | ||||||
|
|
||||||
| scoped := scopedFieldPath(prefix, "oauth2Config") | ||||||
| if hasClientID == hasDCR { | ||||||
| return fmt.Errorf("%s: exactly one of clientId or dcrConfig must be set", scoped) | ||||||
| } | ||||||
|
|
||||||
| if !hasDCR { | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| hasDiscoveryURL := cfg.DCRConfig.DiscoveryURL != "" | ||||||
| hasRegistrationEndpoint := cfg.DCRConfig.RegistrationEndpoint != "" | ||||||
| if hasDiscoveryURL == hasRegistrationEndpoint { | ||||||
| return fmt.Errorf("%s.dcrConfig: exactly one of discoveryUrl or registrationEndpoint must be set", scoped) | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // scopedFieldPath joins a non-empty prefix to a field name with a dot. When | ||||||
| // the prefix is empty, it returns just the field name so callers that already | ||||||
| // supply their own scope (e.g., a wrapping `fmt.Errorf("upstream %q: %w", ...)`) | ||||||
| // don't end up with a leading dot. | ||||||
| func scopedFieldPath(prefix, field string) string { | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F4 / MEDIUM / consensus 8/10 — The Two cleaner shapes:
Either change makes |
||||||
| if prefix == "" { | ||||||
| return field | ||||||
| } | ||||||
| return prefix + "." + field | ||||||
| } | ||||||
|
|
||||||
| // AdditionalAuthorizationParams returns the additional authorization parameters | ||||||
| // from whichever upstream config is set, or nil if none. | ||||||
| func (p *UpstreamProviderConfig) AdditionalAuthorizationParams() map[string]string { | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -571,6 +571,125 @@ func TestMCPExternalAuthConfig_validateUpstreamProvider(t *testing.T) { | |
| }, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F15 / LOW / consensus 7/10 — Boundary length tests missing for softwareStatement and URLs (raised by test-specialist) The CRD imposes |
||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with valid DCRConfig (discoveryUrl only)", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "dcr-discovery", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| DCRConfig: &DCRUpstreamConfig{ | ||
| DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", | ||
| }, | ||
| }, | ||
| }, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with valid DCRConfig (registrationEndpoint only)", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "dcr-endpoint", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| Scopes: []string{"openid"}, | ||
| DCRConfig: &DCRUpstreamConfig{ | ||
| RegistrationEndpoint: "https://idp.example.com/register", | ||
| }, | ||
| }, | ||
| }, | ||
| expectErr: false, | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with DCRConfig and non-empty ClientID", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "dcr-with-clientid", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| ClientID: "pre-provisioned-id", | ||
| DCRConfig: &DCRUpstreamConfig{ | ||
| DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", | ||
| }, | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "exactly one of clientId or dcrConfig must be set", | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with DCRConfig specifying both endpoints", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "dcr-both-endpoints", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| DCRConfig: &DCRUpstreamConfig{ | ||
| DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", | ||
| RegistrationEndpoint: "https://idp.example.com/register", | ||
| }, | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "exactly one of discoveryUrl or registrationEndpoint must be set", | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with DCRConfig specifying neither endpoint", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "dcr-neither-endpoint", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| DCRConfig: &DCRUpstreamConfig{}, | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "exactly one of discoveryUrl or registrationEndpoint must be set", | ||
| }, | ||
| { | ||
| name: "OAuth2 provider with neither ClientID nor DCRConfig", | ||
| provider: UpstreamProviderConfig{ | ||
| Name: "oauth2-missing-auth", | ||
| Type: UpstreamProviderTypeOAuth2, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| // Pin the scoped prefix so a rename of the oauth2Config label surfaces as a test failure. | ||
| errMsg: "oauth2Config: exactly one of clientId or dcrConfig must be set", | ||
| }, | ||
| { | ||
| // Regression guard: conversion only maps DCRConfig in the OAuth2 | ||
| // branch, so an OIDC-typed provider carrying OAuth2Config/DCRConfig | ||
| // must be rejected at validate time rather than silently dropped. | ||
| name: "OIDC provider with OAuth2Config carrying DCRConfig is rejected", | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. F7 / LOW / consensus 8/10 — Test name claims to assert DCR-specific rejection but actually fires the OIDC discriminator (raised by test-specialist) The case has The case adds no DCR-specific coverage beyond the existing OIDC-discriminator test at line 192. Either rename it to reflect what it actually checks, or reshape it to a true DCR-leakage guard by also setting a valid |
||
| provider: UpstreamProviderConfig{ | ||
| Name: "mismatched-oidc", | ||
| Type: UpstreamProviderTypeOIDC, | ||
| OAuth2Config: &OAuth2UpstreamConfig{ | ||
| AuthorizationEndpoint: "https://idp.example.com/authorize", | ||
| TokenEndpoint: "https://idp.example.com/token", | ||
| UserInfo: &UserInfoConfig{EndpointURL: "https://idp.example.com/userinfo"}, | ||
| DCRConfig: &DCRUpstreamConfig{ | ||
| DiscoveryURL: "https://idp.example.com/.well-known/openid-configuration", | ||
| }, | ||
| }, | ||
| }, | ||
| expectErr: true, | ||
| errMsg: "oidcConfig must be set when type is 'oidc' and must not be set otherwise", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F9 / LOW / consensus 7/10 — CEL discriminator and Go validator emit different messages for the same root cause (raised by CRD-specialist)
CEL (this line):
"type must be 'oidc' or 'oauth2'; oidcConfig must be set when type is 'oidc' and oauth2Config must be set when type is 'oauth2' (and the other must not be set)".Go (line 1050):
"oidcConfig must be set when type is 'oidc' and must not be set otherwise".Same root cause, two different diagnostics depending on which gate fires first. Pick one wording (the CEL one is more informative) and align both. The accompanying test at line 326 of types_test.go pins the Go-validator wording, so updating either side requires syncing the test.