From 4d931ba5104fd659e620df635b5b40e61caa941b Mon Sep 17 00:00:00 2001 From: Alan Wilkie Date: Mon, 12 Sep 2022 10:04:04 +1000 Subject: [PATCH 1/3] Allow extra args to be provided to the OIDC auth endpoint --- .../common/crds/k8s.nginx.org_policies.yaml | 2 + .../crds/k8s.nginx.org_policies.yaml | 2 + docs/content/configuration/policy-resource.md | 1 + internal/configs/oidc/openid_connect.js | 4 ++ internal/configs/version2/http.go | 1 + .../version2/nginx-plus.virtualserver.tmpl | 1 + internal/configs/virtualserver.go | 1 + pkg/apis/configuration/v1/types.go | 1 + pkg/apis/configuration/validation/policy.go | 15 ++++++ .../configuration/validation/policy_test.go | 54 +++++++++++++++++++ 10 files changed, 82 insertions(+) diff --git a/deployments/common/crds/k8s.nginx.org_policies.yaml b/deployments/common/crds/k8s.nginx.org_policies.yaml index 7569669673..7233477b77 100644 --- a/deployments/common/crds/k8s.nginx.org_policies.yaml +++ b/deployments/common/crds/k8s.nginx.org_policies.yaml @@ -112,6 +112,8 @@ spec: properties: authEndpoint: type: string + authExtraArgs: + type: string clientID: type: string clientSecret: diff --git a/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml b/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml index 7569669673..7233477b77 100644 --- a/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml +++ b/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml @@ -112,6 +112,8 @@ spec: properties: authEndpoint: type: string + authExtraArgs: + type: string clientID: type: string clientSecret: diff --git a/docs/content/configuration/policy-resource.md b/docs/content/configuration/policy-resource.md index a7ef34f0d8..6860c6c4a3 100644 --- a/docs/content/configuration/policy-resource.md +++ b/docs/content/configuration/policy-resource.md @@ -343,6 +343,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw |``clientID`` | The client ID provided by your OpenID Connect provider. | ``string`` | Yes | |``clientSecret`` | The name of the Kubernetes secret that stores the client secret provided by your OpenID Connect provider. It must be in the same namespace as the Policy resource. The secret must be of the type ``nginx.org/oidc``, and the secret under the key ``client-secret``, otherwise the secret will be rejected as invalid. | ``string`` | Yes | |``authEndpoint`` | URL for the authorization endpoint provided by your OpenID Connect provider. | ``string`` | Yes | +|``authExtraArgs`` | Extra URL arguments to pass to the authorization endpoint provided by your OpenID Connect provider. Arguments must be URL encoded, multiple arguments may be provided by concatenating them with ``&``, for example ``arg1=value1&arg2=value2`` | ``string`` | No | |``tokenEndpoint`` | URL for the token endpoint provided by your OpenID Connect provider. | ``string`` | Yes | |``jwksURI`` | URL for the JSON Web Key Set (JWK) document provided by your OpenID Connect provider. | ``string`` | Yes | |``scope`` | List of OpenID Connect scopes. Possible values are ``openid``, ``profile``, ``email``, ``address`` and ``phone``. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``. The default is ``openid``. | ``string`` | No | diff --git a/internal/configs/oidc/openid_connect.js b/internal/configs/oidc/openid_connect.js index 6110be5764..b997441bb8 100644 --- a/internal/configs/oidc/openid_connect.js +++ b/internal/configs/oidc/openid_connect.js @@ -268,6 +268,10 @@ function getAuthZArgs(r) { var nonceHash = h.digest('base64url'); var authZArgs = "?response_type=code&scope=" + r.variables.oidc_scopes + "&client_id=" + r.variables.oidc_client + "&redirect_uri=" + r.variables.redirect_base + r.variables.redir_location + "&nonce=" + nonceHash; + if (r.variables.oidc_authz_extra_args) { + authZArgs += "&" + r.variables.oidc_authz_extra_args; + } + r.headersOut['Set-Cookie'] = [ "auth_redir=" + r.variables.request_uri + "; " + r.variables.oidc_cookie_flags, "auth_nonce=" + noncePlain + "; " + r.variables.oidc_cookie_flags diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index ca410ee184..d99b4e700b 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -118,6 +118,7 @@ type OIDC struct { TokenEndpoint string RedirectURI string ZoneSyncLeeway int + AuthExtraArgs string } // WAF defines WAF configuration. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 62fbe645b1..9d99dab886 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -76,6 +76,7 @@ server { set $zone_sync_leeway {{ $oidc.ZoneSyncLeeway }}; set $oidc_authz_endpoint "{{ $oidc.AuthEndpoint }}"; + set $oidc_authz_extra_args "{{ $oidc.AuthExtraArgs }}"; set $oidc_token_endpoint "{{ $oidc.TokenEndpoint }}"; set $oidc_jwt_keyfile "{{ $oidc.JwksURI }}"; set $oidc_scopes "{{ $oidc.Scope }}"; diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index bece3e1353..813e5f70b9 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1029,6 +1029,7 @@ func (p *policiesCfg) addOIDCConfig( oidcPolCfg.oidc = &version2.OIDC{ AuthEndpoint: oidc.AuthEndpoint, + AuthExtraArgs: oidc.AuthExtraArgs, TokenEndpoint: oidc.TokenEndpoint, JwksURI: oidc.JWKSURI, ClientID: oidc.ClientID, diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 01c42d385c..f7bfd3215d 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -480,6 +480,7 @@ type OIDC struct { Scope string `json:"scope"` RedirectURI string `json:"redirectURI"` ZoneSyncLeeway *int `json:"zoneSyncLeeway"` + AuthExtraArgs string `json:"authExtraArgs"` } // WAF defines an WAF policy. diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 846fb03f6a..369accbcdd 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -250,6 +250,10 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { allErrs = append(allErrs, validatePositiveIntOrZero(*oidc.ZoneSyncLeeway, fieldPath.Child("zoneSyncLeeway"))...) } + if oidc.AuthExtraArgs != "" { + allErrs = append(allErrs, validateQueryString(oidc.AuthExtraArgs, fieldPath.Child("authExtraArgs"))...) + } + allErrs = append(allErrs, validateURL(oidc.AuthEndpoint, fieldPath.Child("authEndpoint"))...) allErrs = append(allErrs, validateURL(oidc.TokenEndpoint, fieldPath.Child("tokenEndpoint"))...) allErrs = append(allErrs, validateURL(oidc.JWKSURI, fieldPath.Child("jwksURI"))...) @@ -367,6 +371,17 @@ func validateURL(name string, fieldPath *field.Path) field.ErrorList { return allErrs } +func validateQueryString(queryString string, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + + _, err := url.ParseQuery(queryString) + if err != nil { + return append(allErrs, field.Invalid(fieldPath, queryString, err.Error())) + } + + return allErrs +} + func validatePortNumber(port string, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} portInt, _ := strconv.Atoi(port) diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go index 47b063a76a..687a093f6a 100644 --- a/pkg/apis/configuration/validation/policy_test.go +++ b/pkg/apis/configuration/validation/policy_test.go @@ -47,6 +47,7 @@ func TestValidatePolicy(t *testing.T) { Spec: v1.PolicySpec{ OIDC: &v1.OIDC{ AuthEndpoint: "https://foo.bar/auth", + AuthExtraArgs: "foo=bar", TokenEndpoint: "https://foo.bar/token", JWKSURI: "https://foo.bar/certs", ClientID: "random-string", @@ -210,6 +211,24 @@ func TestValidatePolicyFails(t *testing.T) { enableOIDC: true, msg: "OIDC policy with invalid ZoneSyncLeeway", }, + { + policy: &v1.Policy{ + Spec: v1.PolicySpec{ + OIDC: &v1.OIDC{ + AuthEndpoint: "https://foo.bar/auth", + AuthExtraArgs: "foo;bar", + TokenEndpoint: "https://foo.bar/token", + JWKSURI: "https://foo.bar/certs", + ClientID: "random-string", + ClientSecret: "random-secret", + Scope: "openid", + }, + }, + }, + isPlus: true, + enableOIDC: true, + msg: "OIDC policy with invalid AuthExtraArgs", + }, } for _, test := range tests { err := ValidatePolicy(test.policy, test.isPlus, test.enableOIDC, test.enableAppProtect) @@ -872,6 +891,7 @@ func TestValidateOIDCValid(t *testing.T) { { oidc: &v1.OIDC{ AuthEndpoint: "https://accounts.google.com/o/oauth2/v2/auth", + AuthExtraArgs: "foo=bar", TokenEndpoint: "https://oauth2.googleapis.com/token", JWKSURI: "https://www.googleapis.com/oauth2/v3/certs", ClientID: "random-string", @@ -897,6 +917,7 @@ func TestValidateOIDCValid(t *testing.T) { { oidc: &v1.OIDC{ AuthEndpoint: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/auth", + AuthExtraArgs: "kc_idp_hint=foo", TokenEndpoint: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/token", JWKSURI: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/certs", ClientID: "bar", @@ -1012,6 +1033,18 @@ func TestValidateOIDCInvalid(t *testing.T) { }, msg: "invalid chars in clientID", }, + { + oidc: &v1.OIDC{ + AuthEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth", + AuthExtraArgs: "foo;bar", + TokenEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token", + JWKSURI: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs", + ClientID: "foobar", + ClientSecret: "secret", + Scope: "openid", + }, + msg: "invalid chars in authExtraArgs", + }, { oidc: &v1.OIDC{ AuthEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth", @@ -1097,6 +1130,27 @@ func TestValidateURL(t *testing.T) { } } +func TestValidateQueryStringt(t *testing.T) { + t.Parallel() + validInput := []string{"foo=bar", "foo", "foo=bar&baz=zot", "foo=bar&foo=baz", "foo=bar%3Bbaz"} + + for _, test := range validInput { + allErrs := validateQueryString(test, field.NewPath("authExtraArgs")) + if len(allErrs) != 0 { + t.Errorf("validateQueryString(%q) returned errors %v for valid input", allErrs, test) + } + } + + invalidInput := []string{"foo=bar;baz"} + + for _, test := range invalidInput { + allErrs := validateQueryString(test, field.NewPath("authExtraArgs")) + if len(allErrs) == 0 { + t.Errorf("validateQueryString(%q) didn't return error for invalid input", test) + } + } +} + func TestValidateWAF(t *testing.T) { t.Parallel() tests := []struct { From b5852623fe7bfa2423d4a1413298a5600f5e9d6d Mon Sep 17 00:00:00 2001 From: Alan Wilkie Date: Tue, 3 Jan 2023 13:30:08 +1100 Subject: [PATCH 2/3] Make authExtraArgs policy items a list --- .../common/crds/k8s.nginx.org_policies.yaml | 4 +++- .../crds/k8s.nginx.org_policies.yaml | 4 +++- docs/content/configuration/policy-resource.md | 2 +- internal/configs/virtualserver.go | 6 +++++- pkg/apis/configuration/v1/types.go | 18 +++++++++--------- pkg/apis/configuration/validation/policy.go | 4 ++-- .../configuration/validation/policy_test.go | 10 +++++----- 7 files changed, 28 insertions(+), 20 deletions(-) diff --git a/deployments/common/crds/k8s.nginx.org_policies.yaml b/deployments/common/crds/k8s.nginx.org_policies.yaml index 68b6254d01..1693d834e4 100644 --- a/deployments/common/crds/k8s.nginx.org_policies.yaml +++ b/deployments/common/crds/k8s.nginx.org_policies.yaml @@ -113,7 +113,9 @@ spec: authEndpoint: type: string authExtraArgs: - type: string + type: array + items: + type: string clientID: type: string clientSecret: diff --git a/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml b/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml index 68b6254d01..1693d834e4 100644 --- a/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml +++ b/deployments/helm-chart/crds/k8s.nginx.org_policies.yaml @@ -113,7 +113,9 @@ spec: authEndpoint: type: string authExtraArgs: - type: string + type: array + items: + type: string clientID: type: string clientSecret: diff --git a/docs/content/configuration/policy-resource.md b/docs/content/configuration/policy-resource.md index b5043fbdc0..e9a0654d0a 100644 --- a/docs/content/configuration/policy-resource.md +++ b/docs/content/configuration/policy-resource.md @@ -343,7 +343,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw |``clientID`` | The client ID provided by your OpenID Connect provider. | ``string`` | Yes | |``clientSecret`` | The name of the Kubernetes secret that stores the client secret provided by your OpenID Connect provider. It must be in the same namespace as the Policy resource. The secret must be of the type ``nginx.org/oidc``, and the secret under the key ``client-secret``, otherwise the secret will be rejected as invalid. | ``string`` | Yes | |``authEndpoint`` | URL for the authorization endpoint provided by your OpenID Connect provider. | ``string`` | Yes | -|``authExtraArgs`` | Extra URL arguments to pass to the authorization endpoint provided by your OpenID Connect provider. Arguments must be URL encoded, multiple arguments may be provided by concatenating them with ``&``, for example ``arg1=value1&arg2=value2`` | ``string`` | No | +|``authExtraArgs`` | A list of extra URL arguments to pass to the authorization endpoint provided by your OpenID Connect provider. Arguments must be URL encoded, multiple arguments may be included in the list, for example ``[ arg1=value1, arg2=value2 ]`` | ``string[]`` | No | |``tokenEndpoint`` | URL for the token endpoint provided by your OpenID Connect provider. | ``string`` | Yes | |``jwksURI`` | URL for the JSON Web Key Set (JWK) document provided by your OpenID Connect provider. | ``string`` | Yes | |``scope`` | List of OpenID Connect scopes. Possible values are ``openid``, ``profile``, ``email``, ``address`` and ``phone``. The scope ``openid`` always needs to be present and others can be added concatenating them with a ``+`` sign, for example ``openid+profile+email``. The default is ``openid``. | ``string`` | No | diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index ead1ba7052..f90a79ff7d 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1032,10 +1032,14 @@ func (p *policiesCfg) addOIDCConfig( if scope == "" { scope = "openid" } + authExtraArgs := "" + if oidc.AuthExtraArgs != nil { + authExtraArgs = strings.Join(oidc.AuthExtraArgs, "&") + } oidcPolCfg.oidc = &version2.OIDC{ AuthEndpoint: oidc.AuthEndpoint, - AuthExtraArgs: oidc.AuthExtraArgs, + AuthExtraArgs: authExtraArgs, TokenEndpoint: oidc.TokenEndpoint, JwksURI: oidc.JWKSURI, ClientID: oidc.ClientID, diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index f7bfd3215d..85b70a2a4e 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -472,15 +472,15 @@ type EgressMTLS struct { // OIDC defines an Open ID Connect policy. type OIDC struct { - AuthEndpoint string `json:"authEndpoint"` - TokenEndpoint string `json:"tokenEndpoint"` - JWKSURI string `json:"jwksURI"` - ClientID string `json:"clientID"` - ClientSecret string `json:"clientSecret"` - Scope string `json:"scope"` - RedirectURI string `json:"redirectURI"` - ZoneSyncLeeway *int `json:"zoneSyncLeeway"` - AuthExtraArgs string `json:"authExtraArgs"` + AuthEndpoint string `json:"authEndpoint"` + TokenEndpoint string `json:"tokenEndpoint"` + JWKSURI string `json:"jwksURI"` + ClientID string `json:"clientID"` + ClientSecret string `json:"clientSecret"` + Scope string `json:"scope"` + RedirectURI string `json:"redirectURI"` + ZoneSyncLeeway *int `json:"zoneSyncLeeway"` + AuthExtraArgs []string `json:"authExtraArgs"` } // WAF defines an WAF policy. diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 369accbcdd..c8af54191b 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -250,8 +250,8 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { allErrs = append(allErrs, validatePositiveIntOrZero(*oidc.ZoneSyncLeeway, fieldPath.Child("zoneSyncLeeway"))...) } - if oidc.AuthExtraArgs != "" { - allErrs = append(allErrs, validateQueryString(oidc.AuthExtraArgs, fieldPath.Child("authExtraArgs"))...) + if oidc.AuthExtraArgs != nil { + allErrs = append(allErrs, validateQueryString(strings.Join(oidc.AuthExtraArgs, "&"), fieldPath.Child("authExtraArgs"))...) } allErrs = append(allErrs, validateURL(oidc.AuthEndpoint, fieldPath.Child("authEndpoint"))...) diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go index 687a093f6a..5268c61d24 100644 --- a/pkg/apis/configuration/validation/policy_test.go +++ b/pkg/apis/configuration/validation/policy_test.go @@ -47,7 +47,7 @@ func TestValidatePolicy(t *testing.T) { Spec: v1.PolicySpec{ OIDC: &v1.OIDC{ AuthEndpoint: "https://foo.bar/auth", - AuthExtraArgs: "foo=bar", + AuthExtraArgs: []string{"foo=bar"}, TokenEndpoint: "https://foo.bar/token", JWKSURI: "https://foo.bar/certs", ClientID: "random-string", @@ -216,7 +216,7 @@ func TestValidatePolicyFails(t *testing.T) { Spec: v1.PolicySpec{ OIDC: &v1.OIDC{ AuthEndpoint: "https://foo.bar/auth", - AuthExtraArgs: "foo;bar", + AuthExtraArgs: []string{"foo;bar"}, TokenEndpoint: "https://foo.bar/token", JWKSURI: "https://foo.bar/certs", ClientID: "random-string", @@ -891,7 +891,7 @@ func TestValidateOIDCValid(t *testing.T) { { oidc: &v1.OIDC{ AuthEndpoint: "https://accounts.google.com/o/oauth2/v2/auth", - AuthExtraArgs: "foo=bar", + AuthExtraArgs: []string{"foo=bar", "baz=zot"}, TokenEndpoint: "https://oauth2.googleapis.com/token", JWKSURI: "https://www.googleapis.com/oauth2/v3/certs", ClientID: "random-string", @@ -917,7 +917,7 @@ func TestValidateOIDCValid(t *testing.T) { { oidc: &v1.OIDC{ AuthEndpoint: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/auth", - AuthExtraArgs: "kc_idp_hint=foo", + AuthExtraArgs: []string{"kc_idp_hint=foo"}, TokenEndpoint: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/token", JWKSURI: "http://keycloak.default.svc.cluster.local:8080/auth/realms/master/protocol/openid-connect/certs", ClientID: "bar", @@ -1036,7 +1036,7 @@ func TestValidateOIDCInvalid(t *testing.T) { { oidc: &v1.OIDC{ AuthEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/auth", - AuthExtraArgs: "foo;bar", + AuthExtraArgs: []string{"foo;bar"}, TokenEndpoint: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/token", JWKSURI: "http://127.0.0.1:8080/auth/realms/master/protocol/openid-connect/certs", ClientID: "foobar", From 22ef431ca87d985d26ff5d2a068ec2261f4261e8 Mon Sep 17 00:00:00 2001 From: Luca Comellini Date: Tue, 10 Jan 2023 18:18:36 -0800 Subject: [PATCH 3/3] Update codegen --- pkg/apis/configuration/v1/zz_generated.deepcopy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 862df0103e..aa65af4686 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -454,6 +454,11 @@ func (in *OIDC) DeepCopyInto(out *OIDC) { *out = new(int) **out = **in } + if in.AuthExtraArgs != nil { + in, out := &in.AuthExtraArgs, &out.AuthExtraArgs + *out = make([]string, len(*in)) + copy(*out, *in) + } return }