diff --git a/CHANGELOG.md b/CHANGELOG.md index effa411d..452d644b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ## Changes since v7.2.1 +- [#1418](https://github.com/oauth2-proxy/oauth2-proxy/pull/1418) Support for passing arbitrary query parameters through from `/oauth2/start` to the identity provider's login URL. Configuration settings control which parameters are passed by default and precisely which values can be overridden per-request (@ianroberts) - [#1559](https://github.com/oauth2-proxy/oauth2-proxy/pull/1559) Introduce ProviderVerifier to clean up OIDC discovery code (@JoelSpeed) - [#1561](https://github.com/oauth2-proxy/oauth2-proxy/pull/1561) Add ppc64le support (@mgiessing) - [#1563](https://github.com/oauth2-proxy/oauth2-proxy/pull/1563) Ensure claim extractor does not attempt profile call when URL is empty (@JoelSpeed) diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md index 154d4f8c..3fae9938 100644 --- a/docs/docs/configuration/alpha_config.md +++ b/docs/docs/configuration/alpha_config.md @@ -264,6 +264,85 @@ make up the header value | `jwtKeyFile` | _string_ | JWTKeyFile is a path to the private key file in PEM format used to sign the JWT | | `pubjwkURL` | _string_ | PubJWKURL is the JWK pubkey access endpoint | +### LoginURLParameter + +(**Appears on:** [Provider](#provider)) + +LoginURLParameter is the configuration for a single query parameter that +can be passed through from the `/oauth2/start` endpoint to the IdP login +URL. The "default" option specifies the default value or values (if any) +that will be passed to the IdP for this parameter, and "allow" is a list +of options for ways in which this parameter can be set or overridden via +the query string to `/oauth2/start`. +If _only_ a default is specified and no "allow" then the parameter is +effectively fixed - the default value will always be used and anything +passed to the start URL will be ignored. If _only_ "allow" is specified +but no default then the parameter will only be passed on to the IdP if +the caller provides it, and no value will be sent otherwise. + +Examples: + +A parameter whose value is fixed + +``` +name: organization +default: +- myorg +``` + +A parameter that is not passed by default, but may be set to one of a +fixed set of values + +``` +name: prompt +allow: +- value: login +- value: consent +- value: select_account +``` + +A parameter that is passed by default but may be overridden by one of +a fixed set of values + +``` +name: prompt +default: ["login"] +allow: +- value: consent +- value: select_account +``` + +A parameter that may be overridden, but only by values that match a +regular expression. For example to restrict `login_hint` to email +addresses in your organization's domain: + +``` +name: login_hint +allow: +- pattern: '^[^@]*@example\.com$' +# this allows at most one "@" sign, and requires "example.com" domain. +``` + +Note that the YAML rules around exactly which characters are allowed +and/or require escaping in different types of string literals are +convoluted. For regular expressions the single quoted form is simplest +as backslash is not considered to be an escape character. Alternatively +use the "chomped block" format `|-`: + +``` +- pattern: |- + ^[^@]*@example\.com$ +``` + +The hyphen is important, a `|` block would have a trailing newline +character. + +| Field | Type | Description | +| ----- | ---- | ----------- | +| `name` | _string_ | Name specifies the name of the query parameter. | +| `default` | _[]string_ | _(Optional)_ Default specifies a default value or values that will be
passed to the IdP if not overridden. | +| `allow` | _[[]URLParameterRule](#urlparameterrule)_ | _(Optional)_ Allow specifies rules about how the default (if any) may be
overridden via the query string to `/oauth2/start`. Only
values that match one or more of the allow rules will be
forwarded to the IdP. | + ### OIDCOptions (**Appears on:** [Provider](#provider)) @@ -309,15 +388,13 @@ Provider holds all configuration for a single provider | `name` | _string_ | Name is the providers display name
if set, it will be shown to the users in the login page. | | `caFiles` | _[]string_ | CAFiles is a list of paths to CA certificates that should be used when connecting to the provider.
If not specified, the default Go trust sources are used instead | | `loginURL` | _string_ | LoginURL is the authentication endpoint | +| `loginURLParameters` | _[[]LoginURLParameter](#loginurlparameter)_ | LoginURLParameters defines the parameters that can be passed from the start URL to the IdP login URL | | `redeemURL` | _string_ | RedeemURL is the token redemption endpoint | | `profileURL` | _string_ | ProfileURL is the profile access endpoint | | `resource` | _string_ | ProtectedResource is the resource that is protected (Azure AD and ADFS only) | | `validateURL` | _string_ | ValidateURL is the access token validation endpoint | | `scope` | _string_ | Scope is the OAuth scope specification | -| `prompt` | _string_ | Prompt is OIDC prompt | -| `approvalPrompt` | _string_ | ApprovalPrompt is the OAuth approval_prompt
default is set to 'force' | | `allowedGroups` | _[]string_ | AllowedGroups is a list of restrict logins to members of this group | -| `acrValues` | _string_ | AcrValues is a string of acr values | ### ProviderType #### (`string` alias) @@ -377,6 +454,20 @@ as well as an optional minimal TLS version that is acceptable. | `Cert` | _[SecretSource](#secretsource)_ | Cert is the TLS certificate data to use.
Typically this will come from a file. | | `MinVersion` | _string_ | MinVersion is the minimal TLS version that is acceptable.
E.g. Set to "TLS1.3" to select TLS version 1.3 | +### URLParameterRule + +(**Appears on:** [LoginURLParameter](#loginurlparameter)) + +URLParameterRule represents a rule by which query parameters +passed to the `/oauth2/start` endpoint are checked to determine whether +they are valid overrides for the given parameter passed to the IdP's +login URL. Either Value or Pattern should be supplied, not both. + +| Field | Type | Description | +| ----- | ---- | ----------- | +| `value` | _string_ | A Value rule matches just this specific value | +| `pattern` | _string_ | A Pattern rule gives a regular expression that must be matched by
some substring of the value. The expression is _not_ automatically
anchored to the start and end of the value, if you _want_ to restrict
the whole parameter value you must anchor it yourself with `^` and `$`. | + ### Upstream (**Appears on:** [UpstreamConfig](#upstreamconfig)) diff --git a/main_test.go b/main_test.go index 0ca8e745..dd995a68 100644 --- a/main_test.go +++ b/main_test.go @@ -68,7 +68,6 @@ providers: ID: google=oauth2-proxy clientSecret: b2F1dGgyLXByb3h5LWNsaWVudC1zZWNyZXQK clientID: oauth2-proxy - approvalPrompt: force azureConfig: tenant: common oidcConfig: @@ -78,6 +77,10 @@ providers: insecureSkipNonce: true audienceClaims: [aud] extraAudiences: [] + loginURLParameters: + - name: approval_prompt + default: + - force ` const testCoreConfig = ` @@ -154,7 +157,9 @@ redirect_url="http://localhost:4180/oauth2/callback" ExtraAudiences: []string{}, InsecureSkipNonce: true, }, - ApprovalPrompt: "force", + LoginURLParameters: []options.LoginURLParameter{ + {Name: "approval_prompt", Default: []string{"force"}}, + }, }, } return opts diff --git a/oauthproxy.go b/oauthproxy.go index c715303e..3287dadb 100644 --- a/oauthproxy.go +++ b/oauthproxy.go @@ -611,6 +611,7 @@ func (p *OAuthProxy) SignIn(rw http.ResponseWriter, req *http.Request) { if p.SkipProviderButton { p.OAuthStart(rw, req) } else { + // TODO - should we pass on /oauth2/sign_in query params to /oauth2/start? p.SignInPage(rw, req, http.StatusOK) } } @@ -671,6 +672,12 @@ func (p *OAuthProxy) SignOut(rw http.ResponseWriter, req *http.Request) { // OAuthStart starts the OAuth2 authentication flow func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) { + // start the flow permitting login URL query parameters to be overridden from the request URL + p.doOAuthStart(rw, req, req.URL.Query()) +} + +func (p *OAuthProxy) doOAuthStart(rw http.ResponseWriter, req *http.Request, overrides url.Values) { + extraParams := p.provider.Data().LoginURLParams(overrides) prepareNoCache(rw) csrf, err := cookies.NewCSRF(p.CookieOptions) @@ -692,6 +699,7 @@ func (p *OAuthProxy) OAuthStart(rw http.ResponseWriter, req *http.Request) { callbackRedirect, encodeState(csrf.HashOAuthState(), appRedirect), csrf.HashOIDCNonce(), + extraParams, ) if _, err := csrf.SetCookie(rw, req); err != nil { @@ -871,7 +879,10 @@ func (p *OAuthProxy) Proxy(rw http.ResponseWriter, req *http.Request) { logger.Printf("No valid authentication in request. Initiating login.") if p.SkipProviderButton { - p.OAuthStart(rw, req) + // start OAuth flow, but only with the default login URL params - do not + // consider this request's query params as potential overrides, since + // the user did not explicitly start the login flow + p.doOAuthStart(rw, req, nil) } else { p.SignInPage(rw, req, http.StatusForbidden) } diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go index 121724c6..3233c233 100644 --- a/pkg/apis/options/legacy_options.go +++ b/pkg/apis/options/legacy_options.go @@ -632,10 +632,7 @@ func (l *LegacyProvider) convert() (Providers, error) { ProtectedResource: l.ProtectedResource, ValidateURL: l.ValidateURL, Scope: l.Scope, - Prompt: l.Prompt, - ApprovalPrompt: l.ApprovalPrompt, AllowedGroups: l.AllowedGroups, - AcrValues: l.AcrValues, } // This part is out of the switch section for all providers that support OIDC @@ -708,6 +705,24 @@ func (l *LegacyProvider) convert() (Providers, error) { provider.ID = l.ProviderType + "=" + l.ClientID } + // handle AcrValues, Prompt and ApprovalPrompt + var urlParams []LoginURLParameter + if l.AcrValues != "" { + urlParams = append(urlParams, LoginURLParameter{Name: "acr_values", Default: []string{l.AcrValues}}) + } + switch { + case l.Prompt != "": + urlParams = append(urlParams, LoginURLParameter{Name: "prompt", Default: []string{l.Prompt}}) + case l.ApprovalPrompt != "": + urlParams = append(urlParams, LoginURLParameter{Name: "approval_prompt", Default: []string{l.ApprovalPrompt}}) + default: + // match legacy behaviour by default - if neither prompt nor approval_prompt + // specified, use approval_prompt=force + urlParams = append(urlParams, LoginURLParameter{Name: "approval_prompt", Default: []string{"force"}}) + } + + provider.LoginURLParameters = urlParams + providers = append(providers, provider) return providers, nil diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go index 5c684ff9..2e11e329 100644 --- a/pkg/apis/options/legacy_options_test.go +++ b/pkg/apis/options/legacy_options_test.go @@ -118,6 +118,9 @@ var _ = Describe("Legacy Options", func() { opts.Providers[0].OIDCConfig.InsecureSkipNonce = true opts.Providers[0].OIDCConfig.AudienceClaims = []string{"aud"} opts.Providers[0].OIDCConfig.ExtraAudiences = []string{} + opts.Providers[0].LoginURLParameters = []LoginURLParameter{ + {Name: "approval_prompt", Default: []string{"force"}}, + } converted, err := legacyOpts.ToOptions() Expect(err).ToNot(HaveOccurred()) @@ -891,21 +894,41 @@ var _ = Describe("Legacy Options", func() { // Non defaults for these options clientID := "abcd" + defaultURLParams := []LoginURLParameter{ + {Name: "approval_prompt", Default: []string{"force"}}, + } + defaultProvider := Provider{ - ID: "google=" + clientID, - ClientID: clientID, - Type: "google", + ID: "google=" + clientID, + ClientID: clientID, + Type: "google", + LoginURLParameters: defaultURLParams, } defaultLegacyProvider := LegacyProvider{ ClientID: clientID, ProviderType: "google", } - displayNameProvider := Provider{ - ID: "displayName", - Name: "displayName", + defaultProviderWithPrompt := Provider{ + ID: "google=" + clientID, ClientID: clientID, Type: "google", + LoginURLParameters: []LoginURLParameter{ + {Name: "prompt", Default: []string{"switch_user"}}, + }, + } + defaultLegacyProviderWithPrompt := LegacyProvider{ + ClientID: clientID, + ProviderType: "google", + Prompt: "switch_user", + } + + displayNameProvider := Provider{ + ID: "displayName", + Name: "displayName", + ClientID: clientID, + Type: "google", + LoginURLParameters: defaultURLParams, } displayNameLegacyProvider := LegacyProvider{ @@ -923,6 +946,7 @@ var _ = Describe("Legacy Options", func() { ServiceAccountJSON: "test.json", Groups: []string{"1", "2"}, }, + LoginURLParameters: defaultURLParams, } internalConfigLegacyProvider := LegacyProvider{ @@ -950,6 +974,11 @@ var _ = Describe("Legacy Options", func() { expectedProviders: Providers{defaultProvider}, errMsg: "", }), + Entry("with prompt setting", &convertProvidersTableInput{ + legacyProvider: defaultLegacyProviderWithPrompt, + expectedProviders: Providers{defaultProviderWithPrompt}, + errMsg: "", + }), Entry("with provider display name", &convertProvidersTableInput{ legacyProvider: displayNameLegacyProvider, expectedProviders: Providers{displayNameProvider}, diff --git a/pkg/apis/options/login_url_parameters.go b/pkg/apis/options/login_url_parameters.go new file mode 100644 index 00000000..2ebe550a --- /dev/null +++ b/pkg/apis/options/login_url_parameters.go @@ -0,0 +1,101 @@ +package options + +// LoginURLParameter is the configuration for a single query parameter that +// can be passed through from the `/oauth2/start` endpoint to the IdP login +// URL. The "default" option specifies the default value or values (if any) +// that will be passed to the IdP for this parameter, and "allow" is a list +// of options for ways in which this parameter can be set or overridden via +// the query string to `/oauth2/start`. +// If _only_ a default is specified and no "allow" then the parameter is +// effectively fixed - the default value will always be used and anything +// passed to the start URL will be ignored. If _only_ "allow" is specified +// but no default then the parameter will only be passed on to the IdP if +// the caller provides it, and no value will be sent otherwise. +// +// Examples: +// +// A parameter whose value is fixed +// +// ``` +// name: organization +// default: +// - myorg +// ``` +// +// A parameter that is not passed by default, but may be set to one of a +// fixed set of values +// +// ``` +// name: prompt +// allow: +// - value: login +// - value: consent +// - value: select_account +// ``` +// +// A parameter that is passed by default but may be overridden by one of +// a fixed set of values +// +// ``` +// name: prompt +// default: ["login"] +// allow: +// - value: consent +// - value: select_account +// ``` +// +// A parameter that may be overridden, but only by values that match a +// regular expression. For example to restrict `login_hint` to email +// addresses in your organization's domain: +// +// ``` +// name: login_hint +// allow: +// - pattern: '^[^@]*@example\.com$' +// # this allows at most one "@" sign, and requires "example.com" domain. +// ``` +// +// Note that the YAML rules around exactly which characters are allowed +// and/or require escaping in different types of string literals are +// convoluted. For regular expressions the single quoted form is simplest +// as backslash is not considered to be an escape character. Alternatively +// use the "chomped block" format `|-`: +// +// ``` +// - pattern: |- +// ^[^@]*@example\.com$ +// ``` +// +// The hyphen is important, a `|` block would have a trailing newline +// character. +type LoginURLParameter struct { + // Name specifies the name of the query parameter. + Name string `json:"name"` + + // Default specifies a default value or values that will be + // passed to the IdP if not overridden. + //+optional + Default []string `json:"default,omitempty"` + + // Allow specifies rules about how the default (if any) may be + // overridden via the query string to `/oauth2/start`. Only + // values that match one or more of the allow rules will be + // forwarded to the IdP. + //+optional + Allow []URLParameterRule `json:"allow,omitempty"` +} + +// URLParameterRule represents a rule by which query parameters +// passed to the `/oauth2/start` endpoint are checked to determine whether +// they are valid overrides for the given parameter passed to the IdP's +// login URL. Either Value or Pattern should be supplied, not both. +type URLParameterRule struct { + // A Value rule matches just this specific value + Value *string `json:"value,omitempty"` + + // A Pattern rule gives a regular expression that must be matched by + // some substring of the value. The expression is _not_ automatically + // anchored to the start and end of the value, if you _want_ to restrict + // the whole parameter value you must anchor it yourself with `^` and `$`. + Pattern *string `json:"pattern,omitempty"` +} diff --git a/pkg/apis/options/providers.go b/pkg/apis/options/providers.go index 2631e32b..5eae8516 100644 --- a/pkg/apis/options/providers.go +++ b/pkg/apis/options/providers.go @@ -62,6 +62,8 @@ type Provider struct { // LoginURL is the authentication endpoint LoginURL string `json:"loginURL,omitempty"` + // LoginURLParameters defines the parameters that can be passed from the start URL to the IdP login URL + LoginURLParameters []LoginURLParameter `json:"loginURLParameters,omitempty"` // RedeemURL is the token redemption endpoint RedeemURL string `json:"redeemURL,omitempty"` // ProfileURL is the profile access endpoint @@ -72,16 +74,8 @@ type Provider struct { ValidateURL string `json:"validateURL,omitempty"` // Scope is the OAuth scope specification Scope string `json:"scope,omitempty"` - // Prompt is OIDC prompt - Prompt string `json:"prompt,omitempty"` - // ApprovalPrompt is the OAuth approval_prompt - // default is set to 'force' - ApprovalPrompt string `json:"approvalPrompt,omitempty"` // AllowedGroups is a list of restrict logins to members of this group AllowedGroups []string `json:"allowedGroups,omitempty"` - - // AcrValues is a string of acr values - AcrValues string `json:"acrValues,omitempty"` } // ProviderType is used to enumerate the different provider type options @@ -243,9 +237,7 @@ type LoginGovOptions struct { func providerDefaults() Providers { providers := Providers{ { - Type: "google", - Prompt: "", // Change to "login" when ApprovalPrompt officially deprecated - ApprovalPrompt: "force", + Type: "google", AzureConfig: AzureOptions{ Tenant: "common", }, diff --git a/providers/adfs.go b/providers/adfs.go index 844b2d29..65fade84 100644 --- a/providers/adfs.go +++ b/providers/adfs.go @@ -61,8 +61,7 @@ func NewADFSProvider(p *ProviderData, opts options.ADFSOptions) *ADFSProvider { // GetLoginURL Override to double encode the state parameter. If not query params are lost // More info here: https://docs.microsoft.com/en-us/powerapps/maker/portals/configure/configure-saml2-settings -func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string) string { - extraParams := url.Values{} +func (p *ADFSProvider) GetLoginURL(redirectURI, state, nonce string, extraParams url.Values) string { if !p.SkipNonce { extraParams.Add("nonce", nonce) } diff --git a/providers/adfs_test.go b/providers/adfs_test.go index 19356b75..fbd48495 100755 --- a/providers/adfs_test.go +++ b/providers/adfs_test.go @@ -168,7 +168,7 @@ var _ = Describe("ADFS Provider Tests", func() { Scope: "", }, options.ADFSOptions{SkipScope: true}) - result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "") + result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", url.Values{}) Expect(result).NotTo(ContainSubstring("scope=")) }) }) @@ -189,7 +189,7 @@ var _ = Describe("ADFS Provider Tests", func() { }, options.ADFSOptions{}) Expect(p.Data().Scope).To(Equal(in.expectedScope)) - result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "") + result := p.GetLoginURL("https://example.com/adfs/oauth2/", "", "", url.Values{}) Expect(result).To(ContainSubstring("scope=" + url.QueryEscape(in.expectedScope))) }, Entry("should add slash", scopeTableInput{ diff --git a/providers/azure.go b/providers/azure.go index 9c955a67..0620d2a8 100644 --- a/providers/azure.go +++ b/providers/azure.go @@ -103,8 +103,7 @@ func overrideTenantURL(current, defaultURL *url.URL, tenant, path string) { } } -func (p *AzureProvider) GetLoginURL(redirectURI, state, _ string) string { - extraParams := url.Values{} +func (p *AzureProvider) GetLoginURL(redirectURI, state, _ string, extraParams url.Values) string { if p.ProtectedResource != nil && p.ProtectedResource.String() != "" { extraParams.Add("resource", p.ProtectedResource.String()) } diff --git a/providers/azure_test.go b/providers/azure_test.go index 794ed570..6d96ec00 100644 --- a/providers/azure_test.go +++ b/providers/azure_test.go @@ -348,7 +348,7 @@ func TestAzureProviderRedeem(t *testing.T) { func TestAzureProviderProtectedResourceConfigured(t *testing.T) { p := testAzureProvider("", options.AzureOptions{}) p.ProtectedResource, _ = url.Parse("http://my.resource.test") - result := p.GetLoginURL("https://my.test.app/oauth", "", "") + result := p.GetLoginURL("https://my.test.app/oauth", "", "", url.Values{}) assert.Contains(t, result, "resource="+url.QueryEscape("http://my.resource.test")) } diff --git a/providers/logingov.go b/providers/logingov.go index 117d2735..14fed649 100644 --- a/providers/logingov.go +++ b/providers/logingov.go @@ -270,9 +270,8 @@ func (p *LoginGovProvider) Redeem(ctx context.Context, _, code string) (*session } // GetLoginURL overrides GetLoginURL to add login.gov parameters -func (p *LoginGovProvider) GetLoginURL(redirectURI, state, _ string) string { - extraParams := url.Values{} - if p.AcrValues == "" { +func (p *LoginGovProvider) GetLoginURL(redirectURI, state, _ string, extraParams url.Values) string { + if len(extraParams["acr_values"]) == 0 { acr := "http://idmanagement.gov/ns/assurance/loa/1" extraParams.Add("acr_values", acr) } diff --git a/providers/logingov_test.go b/providers/logingov_test.go index fe371c9d..c8f9f162 100644 --- a/providers/logingov_test.go +++ b/providers/logingov_test.go @@ -337,7 +337,7 @@ func TestLoginGovProviderBadNonce(t *testing.T) { func TestLoginGovProviderGetLoginURL(t *testing.T) { p, _, _ := newLoginGovProvider() - result := p.GetLoginURL("http://redirect/", "", "") + result := p.GetLoginURL("http://redirect/", "", "", url.Values{}) assert.Contains(t, result, "acr_values="+url.QueryEscape("http://idmanagement.gov/ns/assurance/loa/1")) assert.Contains(t, result, "nonce=fakenonce") } diff --git a/providers/oidc.go b/providers/oidc.go index 50ea8412..9c0fc630 100644 --- a/providers/oidc.go +++ b/providers/oidc.go @@ -34,8 +34,7 @@ func NewOIDCProvider(p *ProviderData, opts options.OIDCOptions) *OIDCProvider { var _ Provider = (*OIDCProvider)(nil) // GetLoginURL makes the LoginURL with optional nonce support -func (p *OIDCProvider) GetLoginURL(redirectURI, state, nonce string) string { - extraParams := url.Values{} +func (p *OIDCProvider) GetLoginURL(redirectURI, state, nonce string, extraParams url.Values) string { if !p.SkipNonce { extraParams.Add("nonce", nonce) } diff --git a/providers/oidc_test.go b/providers/oidc_test.go index 1341830a..203b86a9 100644 --- a/providers/oidc_test.go +++ b/providers/oidc_test.go @@ -96,11 +96,11 @@ func TestOIDCProviderGetLoginURL(t *testing.T) { nonce := base64.RawURLEncoding.EncodeToString(n) // SkipNonce defaults to true - skipNonce := provider.GetLoginURL("http://redirect/", "", nonce) + skipNonce := provider.GetLoginURL("http://redirect/", "", nonce, url.Values{}) assert.NotContains(t, skipNonce, "nonce") provider.SkipNonce = false - withNonce := provider.GetLoginURL("http://redirect/", "", nonce) + withNonce := provider.GetLoginURL("http://redirect/", "", nonce, url.Values{}) assert.Contains(t, withNonce, fmt.Sprintf("nonce=%s", nonce)) } diff --git a/providers/provider_data.go b/providers/provider_data.go index 9bcb8729..c6860282 100644 --- a/providers/provider_data.go +++ b/providers/provider_data.go @@ -7,6 +7,7 @@ import ( "io/ioutil" "net/http" "net/url" + "regexp" "strings" "github.com/coreos/go-oidc/v3/oidc" @@ -32,15 +33,10 @@ type ProviderData struct { ProfileURL *url.URL ProtectedResource *url.URL ValidateURL *url.URL - // Auth request params & related, see - //https://openid.net/specs/openid-connect-basic-1_0.html#rfc.section.2.1.1.1 - AcrValues string - ApprovalPrompt string // NOTE: Renamed to "prompt" in OAuth2 - ClientID string - ClientSecret string - ClientSecretFile string - Scope string - Prompt string + ClientID string + ClientSecret string + ClientSecretFile string + Scope string // Common OIDC options for any OIDC-based providers to consume AllowUnverifiedEmail bool @@ -54,6 +50,8 @@ type ProviderData struct { AllowedGroups map[string]struct{} getAuthorizationHeaderFunc func(string) http.Header + loginURLParameterDefaults url.Values + loginURLParameterOverrides map[string]*regexp.Regexp } // Data returns the ProviderData @@ -73,6 +71,100 @@ func (p *ProviderData) GetClientSecret() (clientSecret string, err error) { return string(fileClientSecret), nil } +// LoginURLParams returns the parameter values that should be passed to the IdP +// login URL. This is the default set of parameters configured for this provider, +// optionally overridden by the given overrides (typically from the URL of the +// /oauth2/start request) according to the configured rules for this provider. +func (p *ProviderData) LoginURLParams(overrides url.Values) url.Values { + // the returned url.Values may be modified later in the request handling process + // so shallow clone the default map + params := url.Values{} + for k, v := range p.loginURLParameterDefaults { + params[k] = v + } + if len(overrides) > 0 { + for param, re := range p.loginURLParameterOverrides { + if reqValues, ok := overrides[param]; ok { + actualValues := make([]string, 0, len(reqValues)) + for _, val := range reqValues { + if re.MatchString(val) { + actualValues = append(actualValues, val) + } + } + if len(actualValues) > 0 { + params.Del(param) + params[param] = actualValues + } + } + } + } + return params +} + +// Compile the given set of LoginURLParameter options into the internal defaults +// and regular expressions used to validate any overrides. +func (p *ProviderData) compileLoginParams(paramConfig []options.LoginURLParameter) []error { + var errs []error + p.loginURLParameterDefaults = url.Values{} + p.loginURLParameterOverrides = make(map[string]*regexp.Regexp) + + for _, param := range paramConfig { + if p.seenParameter(param.Name) { + errs = append(errs, fmt.Errorf("parameter %s provided more than once in loginURLParameters", param.Name)) + } else { + // record default if parameter declares one + if len(param.Default) > 0 { + p.loginURLParameterDefaults[param.Name] = param.Default + } + // record allow rules if any + if len(param.Allow) > 0 { + errs = p.convertAllowRules(errs, param) + } + } + } + return errs +} + +// Converts the list of allow rules for the given parameter into a regexp +// and store it for use at runtime when validating overrides of that parameter. +func (p *ProviderData) convertAllowRules(errs []error, param options.LoginURLParameter) []error { + var allowREs []string + for idx, rule := range param.Allow { + if (rule.Value == nil) == (rule.Pattern == nil) { + errs = append(errs, fmt.Errorf("rule %d in LoginURLParameter %s must have exactly one of value or pattern", idx, param.Name)) + } else { + allowREs = append(allowREs, regexpForRule(rule)) + } + } + if re, err := regexp.Compile(strings.Join(allowREs, "|")); err != nil { + errs = append(errs, err) + } else { + p.loginURLParameterOverrides[param.Name] = re + } + return errs +} + +// Check whether we have already processed a configuration for the given parameter name +func (p *ProviderData) seenParameter(name string) bool { + _, seenDefault := p.loginURLParameterDefaults[name] + _, seenOverride := p.loginURLParameterOverrides[name] + return seenDefault || seenOverride +} + +// Generate a validating regular expression pattern for a given URLParameterRule. +// If the rule is for a fixed value then returns a regexp that matches exactly +// that value, if the rule is itself a regexp just use that as-is. +func regexpForRule(rule options.URLParameterRule) string { + if rule.Value != nil { + // convert literal value into an equivalent regexp, + // anchored at start and end + return "^" + regexp.QuoteMeta(*rule.Value) + "$" + } + // just use the pattern as-is, but wrap in a non-capture group + // to avoid any possibility of confusing the outer disjunction. + return "(?:" + *rule.Pattern + ")" +} + // setAllowedGroups organizes a group list into the AllowedGroups map // to be consumed by Authorize implementations func (p *ProviderData) setAllowedGroups(groups []string) { diff --git a/providers/provider_data_test.go b/providers/provider_data_test.go index 40892cbc..838c061b 100644 --- a/providers/provider_data_test.go +++ b/providers/provider_data_test.go @@ -8,10 +8,14 @@ import ( "encoding/json" "errors" "fmt" + "net/url" "strings" "testing" "time" + "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/options" + "github.com/stretchr/testify/assert" + "github.com/coreos/go-oidc/v3/oidc" "github.com/golang-jwt/jwt" "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/sessions" @@ -498,3 +502,113 @@ func TestProviderData_checkNonce(t *testing.T) { }) } } + +func TestProviderData_loginURLParameters(t *testing.T) { + + testCases := []struct { + name string + overrides url.Values + has url.Values + notHas []string + }{ + { + name: "no overrides", + overrides: url.Values{}, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"default-value"}, + }, + notHas: []string{"enum_no_default", "free_no_default"}, + }, + { + name: "attempt to override fixed value", + overrides: url.Values{"fixed": {"another-value"}}, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"default-value"}, + }, + notHas: []string{"enum_no_default", "free_no_default"}, + }, + { + name: "set one allowed and one forbidden enum", + overrides: url.Values{ + "enum_no_default": {"allowed1", "not-allowed"}, + }, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"default-value"}, + "enum_no_default": {"allowed1"}, + }, + notHas: []string{"free_no_default"}, + }, + { + name: "replace default value", + overrides: url.Values{"free_with_default": {"something-else"}}, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"something-else"}, + }, + notHas: []string{"enum_no_default", "free_no_default"}, + }, + { + name: "set free text value", + overrides: url.Values{"free_no_default": {"some-value"}}, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"default-value"}, + "free_no_default": {"some-value"}, + }, + notHas: []string{"enum_no_default"}, + }, + { + name: "attempt to set unapproved parameter", + overrides: url.Values{"malicious_value": {"evil"}}, + has: url.Values{ + "fixed": {"fixed-value"}, + "enum_with_default": {"default-value"}, + "free_with_default": {"default-value"}, + }, + notHas: []string{"enum_no_default", "free_no_default"}, + }, + } + + // fixed list of two allowed values + allowed1 := "allowed1" + allowed2 := "allowed2" + allowEnum := []options.URLParameterRule{ + {Value: &allowed1}, + {Value: &allowed2}, + } + // regex that will allow anything + anything := "^.*$" + allowAnything := []options.URLParameterRule{ + {Pattern: &anything}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // set up LoginURLParameters for testing + data := ProviderData{} + data.compileLoginParams([]options.LoginURLParameter{ + {Name: "fixed", Default: []string{"fixed-value"}}, + {Name: "enum_with_default", Default: []string{"default-value"}, Allow: allowEnum}, + {Name: "enum_no_default", Allow: allowEnum}, + {Name: "free_with_default", Default: []string{"default-value"}, Allow: allowAnything}, + {Name: "free_no_default", Allow: allowAnything}, + }) + + redirectParams := data.LoginURLParams(tc.overrides) + for _, k := range tc.notHas { + assert.NotContains(t, redirectParams, k) + } + for k, vs := range tc.has { + actualVals := redirectParams[k] + assert.ElementsMatch(t, vs, actualVals) + } + }) + } +} diff --git a/providers/provider_default.go b/providers/provider_default.go index 7a641b1e..af88f0fe 100644 --- a/providers/provider_default.go +++ b/providers/provider_default.go @@ -33,8 +33,7 @@ var ( ) // GetLoginURL with typical oauth parameters -func (p *ProviderData) GetLoginURL(redirectURI, state, _ string) string { - extraParams := url.Values{} +func (p *ProviderData) GetLoginURL(redirectURI, state, _ string, extraParams url.Values) string { loginURL := makeLoginURL(p, redirectURI, state, extraParams) return loginURL.String() } diff --git a/providers/provider_default_test.go b/providers/provider_default_test.go index ce6eea4f..75b4f5d3 100644 --- a/providers/provider_default_test.go +++ b/providers/provider_default_test.go @@ -2,7 +2,6 @@ package providers import ( "context" - "net/url" "testing" "time" @@ -30,33 +29,6 @@ func TestRefresh(t *testing.T) { assert.Equal(t, ErrNotImplemented, err) } -func TestAcrValuesNotConfigured(t *testing.T) { - p := &ProviderData{ - LoginURL: &url.URL{ - Scheme: "http", - Host: "my.test.idp", - Path: "/oauth/authorize", - }, - } - - result := p.GetLoginURL("https://my.test.app/oauth", "", "") - assert.NotContains(t, result, "acr_values") -} - -func TestAcrValuesConfigured(t *testing.T) { - p := &ProviderData{ - LoginURL: &url.URL{ - Scheme: "http", - Host: "my.test.idp", - Path: "/oauth/authorize", - }, - AcrValues: "testValue", - } - - result := p.GetLoginURL("https://my.test.app/oauth", "", "") - assert.Contains(t, result, "acr_values=testValue") -} - func TestProviderDataEnrichSession(t *testing.T) { g := NewWithT(t) p := &ProviderData{} diff --git a/providers/providers.go b/providers/providers.go index 2993dc2c..58faf049 100644 --- a/providers/providers.go +++ b/providers/providers.go @@ -14,7 +14,7 @@ import ( // Provider represents an upstream identity provider implementation type Provider interface { Data() *ProviderData - GetLoginURL(redirectURI, finalRedirect string, nonce string) string + GetLoginURL(redirectURI, finalRedirect string, nonce string, extraParams url.Values) string Redeem(ctx context.Context, redirectURI, code string) (*sessions.SessionState, error) // Deprecated: Migrate to EnrichSession GetEmailAddress(ctx context.Context, s *sessions.SessionState) (string, error) @@ -70,9 +70,6 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, ClientID: providerConfig.ClientID, ClientSecret: providerConfig.ClientSecret, ClientSecretFile: providerConfig.ClientSecretFile, - Prompt: providerConfig.Prompt, - ApprovalPrompt: providerConfig.ApprovalPrompt, - AcrValues: providerConfig.AcrValues, } needsVerifier, err := providerRequiresOIDCProviderVerifier(providerConfig.Type) @@ -122,6 +119,9 @@ func newProviderDataFromConfig(providerConfig options.Provider) (*ProviderData, errs = append(errs, fmt.Errorf("could not parse %s URL: %v", name, err)) } } + // handle LoginURLParameters + errs = append(errs, p.compileLoginParams(providerConfig.LoginURLParameters)...) + if len(errs) > 0 { return nil, k8serrors.NewAggregate(errs) } diff --git a/providers/util.go b/providers/util.go index 0507dde0..115c29c7 100644 --- a/providers/util.go +++ b/providers/util.go @@ -38,14 +38,6 @@ func makeLoginURL(p *ProviderData, redirectURI, state string, extraParams url.Va a := *p.LoginURL params, _ := url.ParseQuery(a.RawQuery) params.Set("redirect_uri", redirectURI) - if p.AcrValues != "" { - params.Add("acr_values", p.AcrValues) - } - if p.Prompt != "" { - params.Set("prompt", p.Prompt) - } else { // Legacy variant of the prompt param: - params.Set("approval_prompt", p.ApprovalPrompt) - } params.Add("scope", p.Scope) params.Set("client_id", p.ClientID) params.Set("response_type", "code")