From 967dd1f297a0076f1190fd3e9eb9f73fc08b07b4 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 30 Apr 2025 14:53:04 +0100 Subject: [PATCH 01/41] Fix leading whitespace in tmpl files --- internal/configs/version2/nginx.virtualserver.tmpl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 59a86b5e52..35376ecf88 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -424,7 +424,7 @@ server { {{- with $ssl := $s.SSL }} {{ if $ssl.HTTP2 }} - location @grpc_deadline_exceeded { + location @grpc_deadline_exceeded { default_type application/grpc; add_header content-type application/grpc; add_header grpc-status 4; @@ -480,6 +480,6 @@ server { return 204; } - {{ end }} + {{ end }} {{ end }} } From 21281bff221933ff46857c059098ed7cdc769946 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 30 Apr 2025 16:21:29 +0100 Subject: [PATCH 02/41] Add PKCE Enabled flag --- internal/configs/version2/http.go | 1 + .../version2/nginx-plus.virtualserver.tmpl | 2 +- internal/configs/version2/template_helper.go | 18 ++++++++++++++++++ internal/configs/virtualserver.go | 1 + pkg/apis/configuration/v1/types.go | 1 + 5 files changed, 22 insertions(+), 1 deletion(-) diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 818546b557..16bc5f2dc8 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -153,6 +153,7 @@ type OIDC struct { ZoneSyncLeeway int AuthExtraArgs string AccessTokenEnable bool + PKCEEnabled bool } // APIKey holds API key configuration. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index f71a5aab74..a17610d03e 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -97,7 +97,7 @@ server { {{- with $oidc := $s.OIDC }} include oidc/oidc.conf; - set $oidc_pkce_enable 0; + set $oidc_pkce_enable "{{ boolToInteger $oidc.PKCEEnabled }}"; set $oidc_client_auth_method "client_secret_post"; set $oidc_logout_redirect "{{ $oidc.PostLogoutRedirectURI }}"; set $oidc_hmac_key "{{ $s.VSName }}"; diff --git a/internal/configs/version2/template_helper.go b/internal/configs/version2/template_helper.go index 667f158e94..a31003e928 100644 --- a/internal/configs/version2/template_helper.go +++ b/internal/configs/version2/template_helper.go @@ -231,6 +231,23 @@ func makeServerName(s StreamServer) string { return fmt.Sprintf("server_name \"%s\";", s.ServerName) } +// boolToInteger will do the following conversions: +// false -> 0 +// true -> 1 +// +// An example for where it's used is the OIDC PKCE Enable flag. +func boolToInteger(b bool) int { + // The compiler currently only optimizes this form. + // See issue 6011. + var i int + if b { + i = 1 + } else { + i = 0 + } + return i +} + var helperFunctions = template.FuncMap{ "headerListToCIMap": headerListToCIMap, "hasCIKey": hasCIKey, @@ -246,4 +263,5 @@ var helperFunctions = template.FuncMap{ "makeHeaderQueryValue": makeHeaderQueryValue, "makeTransportListener": makeTransportListener, "makeServerName": makeServerName, + "boolToInteger": boolToInteger, } diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 1c4e11d5d6..68cbb9cbd5 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1419,6 +1419,7 @@ func (p *policiesCfg) addOIDCConfig( PostLogoutRedirectURI: postLogoutRedirectURI, ZoneSyncLeeway: generateIntFromPointer(oidc.ZoneSyncLeeway, 200), AccessTokenEnable: oidc.AccessTokenEnable, + PKCEEnabled: oidc.PKCEEnabled, } oidcPolCfg.key = polKey } diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 8f93dfbe96..e75469dd1b 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -685,6 +685,7 @@ type OIDC struct { ZoneSyncLeeway *int `json:"zoneSyncLeeway"` AuthExtraArgs []string `json:"authExtraArgs"` AccessTokenEnable bool `json:"accessTokenEnable"` + PKCEEnabled bool `json:"pkceEnabled"` } // WAF defines an WAF policy. From 18dca5ce10250b9922ea94d9a953c36b83be66ce Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 12:33:52 +0100 Subject: [PATCH 03/41] Implement pkce in configs --- internal/configs/oidc/oidc_common.conf | 2 -- internal/configs/oidc/oidc_pkce_supplements.conf | 8 ++++++++ internal/configs/version2/nginx-plus.virtualserver.tmpl | 4 ++++ 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 internal/configs/oidc/oidc_pkce_supplements.conf diff --git a/internal/configs/oidc/oidc_common.conf b/internal/configs/oidc/oidc_common.conf index 26ef21b05b..30d5d37a5d 100644 --- a/internal/configs/oidc/oidc_common.conf +++ b/internal/configs/oidc/oidc_common.conf @@ -20,7 +20,6 @@ proxy_cache_path /var/cache/nginx/jwk levels=1 keys_zone=jwk:64k max_size=1m; keyval_zone zone=oidc_id_tokens:1M timeout=1h sync; keyval_zone zone=oidc_access_tokens:1M timeout=1h sync; keyval_zone zone=refresh_tokens:1M timeout=8h sync; -#keyval_zone zone=oidc_pkce:128K timeout=90s sync; # Temporary storage for PKCE code verifier. keyval $cookie_auth_token $session_jwt zone=oidc_id_tokens; # Exchange cookie for ID token(JWT) keyval $cookie_auth_token $access_token zone=oidc_access_tokens; # Exchange cookie for access token @@ -28,7 +27,6 @@ keyval $cookie_auth_token $refresh_token zone=refresh_tokens; # Exchange coo keyval $request_id $new_session zone=oidc_id_tokens; # For initial session creation keyval $request_id $new_access_token zone=oidc_access_tokens; keyval $request_id $new_refresh zone=refresh_tokens; # '' -#keyval $pkce_id $pkce_code_verifier zone=oidc_pkce; auth_jwt_claim_set $jwt_audience aud; # In case aud is an array js_import oidc from oidc/openid_connect.js; diff --git a/internal/configs/oidc/oidc_pkce_supplements.conf b/internal/configs/oidc/oidc_pkce_supplements.conf new file mode 100644 index 0000000000..668b817f46 --- /dev/null +++ b/internal/configs/oidc/oidc_pkce_supplements.conf @@ -0,0 +1,8 @@ +# https://nginx.org/en/docs/http/ngx_http_keyval_module.html +# context: http + +# keyval_zone keyval_zone zone=name:size [state=file] [timeout=time] [type=string|ip|prefix] [sync]; +keyval_zone zone=oidc_pkce:128K timeout=90s sync; # Temporary storage for PKCE code verifier. + +# keyval key $variable zone +keyval $pkce_id $pkce_code_verifier zone=oidc_pkce; diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index a17610d03e..a6782b3a7f 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -82,6 +82,10 @@ match {{ $m.Name }} { proxy_cache_path /var/cache/nginx/jwks_uri_{{$s.VSName}} levels=1 keys_zone=jwks_uri_{{$s.VSName}}:1m max_size=10m; {{- end }} +{{- if $s.OIDC.PKCEEnabled }} +include oidc/oidc_pkce_supplements.conf +{{- end }} + server { {{- if $s.Gunzip }} gunzip on; From f30cae3c02c590741da9961c239baed43e538af2 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 13:44:49 +0100 Subject: [PATCH 04/41] Add check for OIDC to guard for a nil pointer --- internal/configs/version2/nginx-plus.virtualserver.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index a6782b3a7f..53b3339cbc 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -82,7 +82,7 @@ match {{ $m.Name }} { proxy_cache_path /var/cache/nginx/jwks_uri_{{$s.VSName}} levels=1 keys_zone=jwks_uri_{{$s.VSName}}:1m max_size=10m; {{- end }} -{{- if $s.OIDC.PKCEEnabled }} +{{- if and $s.OIDC $s.OIDC.PKCEEnabled }} include oidc/oidc_pkce_supplements.conf {{- end }} From 3c2b1fc98dd67d22779c22390fbccf6f6c982028 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 13:47:26 +0100 Subject: [PATCH 05/41] Fix some whitespace alignment in tmpl files --- internal/configs/version2/nginx-plus.virtualserver.tmpl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 53b3339cbc..11d57fdf1b 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -368,7 +368,7 @@ server { {{- if $hc.GRPCPass }} type=grpc{{- if $hc.GRPCStatus }} grpc_status={{ $hc.GRPCStatus }}{{- end -}} {{- if $hc.GRPCService }} grpc_service={{ $hc.GRPCService }}{{- end -}}{{ end -}}; - } + } {{- end }} {{- range $e := $s.ErrorPageLocations }} @@ -676,7 +676,7 @@ server { {{- with $ssl := $s.SSL }} {{ if $ssl.HTTP2 }} - location @grpc_deadline_exceeded { + location @grpc_deadline_exceeded { default_type application/grpc; add_header content-type application/grpc; add_header grpc-status 4; @@ -732,6 +732,6 @@ server { return 204; } - {{ end }} + {{ end }} {{ end }} } From 96e1aa0f5946e1bfbc88316e264243a0828c6288 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 13:58:17 +0100 Subject: [PATCH 06/41] Update snapshots to realign with whitespaces --- .../version2/__snapshots__/templates_test.snap | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index d2d9ce4cdc..df946bc7d8 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -335,7 +335,7 @@ server { proxy_pass http://coffee-v2; health_check uri=/ port=50 interval=5s jitter=0s fails=1 passes=1 mandatory persistent keepalive_time=60s; - } + } location @hc-tea { grpc_connect_timeout ; @@ -344,7 +344,7 @@ server { grpc_pass grpc://tea-v3; health_check port=50 interval=5s jitter=0s fails=1 passes=1 type=grpc grpc_status=12 grpc_service=tea-servicev2; - } + } location @vs_cafe_cafe_vsr_tea_tea_tea__tea_error_page_0 { default_type "application/json"; @@ -761,7 +761,7 @@ server { proxy_pass http://coffee-v2; health_check uri=/ port=50 interval=5s jitter=0s fails=1 passes=1 mandatory persistent keepalive_time=60s; - } + } location @hc-tea { grpc_connect_timeout ; @@ -770,7 +770,7 @@ server { grpc_pass grpc://tea-v3; health_check port=50 interval=5s jitter=0s fails=1 passes=1 type=grpc grpc_status=12 grpc_service=tea-servicev2; - } + } location @vs_cafe_cafe_vsr_tea_tea_tea__tea_error_page_0 { default_type "application/json"; @@ -1438,7 +1438,7 @@ server { return 204; } - + } @@ -3303,7 +3303,7 @@ server { return 204; } - + } @@ -3402,7 +3402,7 @@ server { proxy_pass http://coffee-v2; health_check uri=/ port=50 interval=5s jitter=0s fails=1 passes=1 mandatory persistent keepalive_time=60s; - } + } location @hc-tea { grpc_connect_timeout ; @@ -3411,7 +3411,7 @@ server { grpc_pass grpc://tea-v3; health_check port=50 interval=5s jitter=0s fails=1 passes=1 type=grpc grpc_status=12 grpc_service=tea-servicev2; - } + } location @vs_cafe_cafe_vsr_tea_tea_tea__tea_error_page_0 { default_type "application/json"; From 4a800a7f5ccff919d89cd6dd2f94da1f2677274d Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 14:59:01 +0100 Subject: [PATCH 07/41] Add tests for PKCE enabled true --- .../__snapshots__/templates_test.snap | 48 +++++++++++++++++++ internal/configs/version2/templates_test.go | 37 ++++++++++++++ internal/configs/virtualserver_test.go | 1 + 3 files changed, 86 insertions(+) diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index df946bc7d8..971f237543 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -1316,6 +1316,54 @@ server { --- +[TestExecuteVirtualServerTemplateWithOIDCAndPKCEPolicyNGINXPlus - 1] + +include oidc/oidc_pkce_supplements.conf + +server { + listen 80 proxy_protocol; + listen [::]:80 proxy_protocol; + + + server_name example.com; + status_zone example.com; + set $resource_type "virtualserver"; + set $resource_name ""; + set $resource_namespace ""; + include oidc/oidc.conf; + + set $oidc_pkce_enable "1"; + set $oidc_client_auth_method "client_secret_post"; + set $oidc_logout_redirect ""; + set $oidc_hmac_key ""; + set $zone_sync_leeway 0; + + set $oidc_authz_endpoint ""; + set $oidc_authz_extra_args ""; + set $oidc_token_endpoint ""; + set $oidc_end_session_endpoint ""; + set $oidc_jwt_keyfile ""; + set $oidc_scopes ""; + set $oidc_client ""; + set $oidc_client_secret ""; + set $redir_location ""; + + server_tokens ""; + + + + + location / { + set $service ""; + status_zone ""; + + + set $default_connection_header close; + } +} + +--- + [TestExecuteVirtualServerTemplate_RendersOSSTemplateWithHTTP2Off - 1] server { diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index 53729954d3..4b5cff7ff0 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -809,6 +809,27 @@ func TestExecuteVirtualServerTemplate_WithCustomOIDCRedirectLocation(t *testing. if !bytes.Contains(got, []byte(expectedRedirVar)) { t.Errorf("Should set $redir_location to custom value: %s", expectedRedirVar) } +} + +func TestExecuteVirtualServerTemplateWithOIDCAndPKCEPolicyNGINXPlus(t *testing.T) { + t.Parallel() + + e := newTmplExecutorNGINXPlus(t) + got, err := e.ExecuteVirtualServerTemplate(&virtualServerCfgWithOIDCAndPKCETurnedOn) + if err != nil { + t.Error(err) + } + + want := "include oidc/oidc_pkce_supplements.conf" + want2 := "include oidc/oidc.conf;" + + if !bytes.Contains(got, []byte(want)) { + t.Errorf("want %q in generated template", want) + } + + if !bytes.Contains(got, []byte(want2)) { + t.Errorf("want %q in generated template", want2) + } snaps.MatchSnapshot(t, string(got)) t.Log(string(got)) @@ -2416,6 +2437,22 @@ var ( }, } + virtualServerCfgWithOIDCAndPKCETurnedOn = VirtualServerConfig{ + Server: Server{ + ServerName: "example.com", + StatusZone: "example.com", + ProxyProtocol: true, + OIDC: &OIDC{ + PKCEEnabled: true, + }, + Locations: []Location{ + { + Path: "/", + }, + }, + }, + } + transportServerCfg = TransportServerConfig{ Upstreams: []StreamUpstream{ { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index cd5170afdd..746860bd2f 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -10652,6 +10652,7 @@ func TestGeneratePoliciesFails(t *testing.T) { EndSessionEndpoint: "http://foo.com/bar", PostLogoutRedirectURI: "/_logout", AccessTokenEnable: true, + PKCEEnabled: true, }, }, }, From 4f00a17b34acb0aff46744c0722ed337bd5624d7 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 8 May 2025 15:15:33 +0100 Subject: [PATCH 08/41] Update CRDs based on policy files --- config/crd/bases/k8s.nginx.org_policies.yaml | 2 ++ deploy/crds.yaml | 2 ++ 2 files changed, 4 insertions(+) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index da36ddeb89..2aee1a56d7 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -163,6 +163,8 @@ spec: type: string jwksURI: type: string + pkceEnabled: + type: boolean postLogoutRedirectURI: type: string redirectURI: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 9d59787abd..dab7788e68 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -325,6 +325,8 @@ spec: type: string jwksURI: type: string + pkceEnabled: + type: boolean postLogoutRedirectURI: type: string redirectURI: From 812a9d2b5c05b9289dc6737027a2d0a92abff844 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 13 May 2025 13:55:26 +0200 Subject: [PATCH 09/41] Add pkceEnabled to oidc pytest setup yaml --- tests/data/oidc/oidc.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/data/oidc/oidc.yaml b/tests/data/oidc/oidc.yaml index 990924f3de..95fe430dc4 100644 --- a/tests/data/oidc/oidc.yaml +++ b/tests/data/oidc/oidc.yaml @@ -12,3 +12,4 @@ spec: endSessionEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/logout scope: openid+profile+email accessTokenEnable: true + pkceEnabled: true From db28908e6ee59ace73f9be4ef4aad54730480bab Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 13 May 2025 16:22:20 +0200 Subject: [PATCH 10/41] Terminate include directive with a ; --- internal/configs/version2/__snapshots__/templates_test.snap | 2 +- internal/configs/version2/nginx-plus.virtualserver.tmpl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index 971f237543..0f51bdf4d6 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -1318,7 +1318,7 @@ server { [TestExecuteVirtualServerTemplateWithOIDCAndPKCEPolicyNGINXPlus - 1] -include oidc/oidc_pkce_supplements.conf +include oidc/oidc_pkce_supplements.conf; server { listen 80 proxy_protocol; diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 11d57fdf1b..8a7f6bbc75 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -83,7 +83,7 @@ proxy_cache_path /var/cache/nginx/jwks_uri_{{$s.VSName}} levels=1 keys_zone=jwks {{- end }} {{- if and $s.OIDC $s.OIDC.PKCEEnabled }} -include oidc/oidc_pkce_supplements.conf +include oidc/oidc_pkce_supplements.conf; {{- end }} server { From b70f3ffdc148329182dbf1e83e930ef602f134ce Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 14 May 2025 12:42:56 +0200 Subject: [PATCH 11/41] Set pkce enabled to an int instead of a string --- internal/configs/version2/nginx-plus.virtualserver.tmpl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 8a7f6bbc75..8466b617aa 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -101,7 +101,7 @@ server { {{- with $oidc := $s.OIDC }} include oidc/oidc.conf; - set $oidc_pkce_enable "{{ boolToInteger $oidc.PKCEEnabled }}"; + set $oidc_pkce_enable {{ boolToInteger $oidc.PKCEEnabled }}; set $oidc_client_auth_method "client_secret_post"; set $oidc_logout_redirect "{{ $oidc.PostLogoutRedirectURI }}"; set $oidc_hmac_key "{{ $s.VSName }}"; From d81eea1d04fa7fc9511c08480efe9a3da26d8cf0 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 16:47:35 +0200 Subject: [PATCH 12/41] OIDC test doesn't need pkce enabled --- tests/data/oidc/oidc.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/data/oidc/oidc.yaml b/tests/data/oidc/oidc.yaml index 95fe430dc4..990924f3de 100644 --- a/tests/data/oidc/oidc.yaml +++ b/tests/data/oidc/oidc.yaml @@ -12,4 +12,3 @@ spec: endSessionEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/logout scope: openid+profile+email accessTokenEnable: true - pkceEnabled: true From 0348ef25ba6c8983c189e153d911b6eb8b51a2f0 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 16:49:09 +0200 Subject: [PATCH 13/41] Add PKCE pytest --- tests/data/oidc/pkce.yaml | 15 +++ tests/suite/test_pkce.py | 229 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 244 insertions(+) create mode 100644 tests/data/oidc/pkce.yaml create mode 100644 tests/suite/test_pkce.py diff --git a/tests/data/oidc/pkce.yaml b/tests/data/oidc/pkce.yaml new file mode 100644 index 0000000000..bcf3c1cc3d --- /dev/null +++ b/tests/data/oidc/pkce.yaml @@ -0,0 +1,15 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: oidc-policy +spec: + oidc: + clientID: nginx-plus-pkce + clientSecret: oidc-secret + authEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/auth + tokenEndpoint: http://keycloak.default.svc.cluster.local:8080/realms/master/protocol/openid-connect/token + jwksURI: http://keycloak.default.svc.cluster.local:8080/realms/master/protocol/openid-connect/certs + endSessionEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/logout + scope: openid+profile+email + accessTokenEnable: true + pkceEnabled: true diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py new file mode 100644 index 0000000000..aab5438ade --- /dev/null +++ b/tests/suite/test_pkce.py @@ -0,0 +1,229 @@ +import secrets + +import pytest +import requests +import yaml +from playwright.sync_api import Error, sync_playwright +from settings import DEPLOYMENTS, TEST_DATA +from suite.utils.policy_resources_utils import delete_policy +from suite.utils.resources_utils import ( + create_example_app, + create_items_from_yaml, + create_secret, + create_secret_from_yaml, + delete_common_app, + delete_secret, + replace_configmap_from_yaml, + wait_before_test, + wait_until_all_pods_are_ready, +) +from suite.utils.vs_vsr_resources_utils import ( + create_virtual_server_from_yaml, + delete_virtual_server, + patch_virtual_server_from_yaml, +) + +username = "nginx-user-" + secrets.token_hex(4) +password = secrets.token_hex(8) +keycloak_src = f"{TEST_DATA}/oidc/keycloak.yaml" +keycloak_vs_src = f"{TEST_DATA}/oidc/virtual-server-idp.yaml" +oidc_secret_src = f"{TEST_DATA}/oidc/client-secret.yaml" +pkce_pol_src = f"{TEST_DATA}/oidc/pkce.yaml" +oidc_vs_src = f"{TEST_DATA}/oidc/virtual-server.yaml" +orig_vs_src = f"{TEST_DATA}/virtual-server-tls/standard/virtual-server.yaml" +cm_src = f"{TEST_DATA}/oidc/nginx-config.yaml" +cm_zs_src = f"{TEST_DATA}/oidc/nginx-config-zs.yaml" +orig_cm_src = f"{DEPLOYMENTS}/common/nginx-config.yaml" +svc_src = f"{TEST_DATA}/oidc/nginx-ingress-headless.yaml" + + +class KeycloakSetupPKCE: + """ + Attributes: + secret (str): + """ + + def __init__(self, secret): + self.secret = secret + + +@pytest.fixture(scope="class") +def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): + + # Create Keycloak resources and setup Keycloak idp with PKCE enabled + secret_name = create_secret_from_yaml( + kube_apis.v1, virtual_server_setup.namespace, f"{TEST_DATA}/virtual-server-tls/tls-secret.yaml" + ) + keycloak_address = "keycloak.example.com" + create_example_app(kube_apis, "keycloak", test_namespace) + wait_before_test() + wait_until_all_pods_are_ready(kube_apis.v1, test_namespace) + keycloak_vs_name = create_virtual_server_from_yaml(kube_apis.custom_objects, keycloak_vs_src, test_namespace) + wait_before_test() + + # Get token + url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/realms/master/protocol/openid-connect/token" + headers = {"Host": keycloak_address, "Content-Type": "application/x-www-form-urlencoded"} + data = {"username": "admin", "password": "admin", "grant_type": "password", "client_id": "admin-cli"} + + response = requests.post(url, headers=headers, data=data, verify=False) + token = response.json()["access_token"] + + # Create a user and set credentials + create_user_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/admin/realms/master/users" + headers = {"Content-Type": "application/json", "Authorization": f"Bearer {token}", "Host": keycloak_address} + user_payload = { + "username": username, + "enabled": True, + "credentials": [{"type": "password", "value": password, "temporary": False}], + } + response = requests.post(create_user_url, headers=headers, json=user_payload, verify=False) + + # Create client "nginx-plus" + create_client_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/admin/realms/master/clients" + client_payload = { + "clientId": "nginx-plus-pkce", + "redirectUris": ["https://virtual-server-tls.example.com:443/_codexch"], + "standardFlowEnabled": True, + "directAccessGrantsEnabled": False, + "publicClient": True, + "attributes": { + "post.logout.redirect.uris": "https://virtual-server-tls.example.com:443/*", + "pkce.code.challenge.method": "S256", + }, + "protocol": "openid-connect", + } + client_resp = requests.post(create_client_url, headers=headers, json=client_payload, verify=False) + client_resp.raise_for_status() + # let's check that it's a 201 somehow + + print(f"Keycloak setup complete.") + + def fin(): + if request.config.getoption("--skip-fixture-teardown") == "no": + print("Delete Keycloak resources") + delete_virtual_server(kube_apis.custom_objects, keycloak_vs_name, test_namespace) + delete_common_app(kube_apis, "keycloak", test_namespace) + delete_secret(kube_apis.v1, secret_name, test_namespace) + + request.addfinalizer(fin) + + # base64 encoded "pkce-doesnt-support-client-secrets" + return KeycloakSetupPKCE("cGtjZS1kb2VzbnQtc3VwcG9ydC1jbGllbnQtc2VjcmV0cw==") + + +@pytest.mark.oidc +@pytest.mark.skip_for_nginx_oss +@pytest.mark.parametrize( + "crd_ingress_controller, virtual_server_setup", + [ + ( + { + "type": "complete", + "extra_args": [ + f"-enable-oidc", + ], + }, + {"example": "virtual-server-tls", "app_type": "simple"}, + ) + ], + indirect=True, +) +class TestPKCE: + @pytest.mark.parametrize("configmap", [cm_src, cm_zs_src]) + def test_pkce( + self, + request, + kube_apis, + ingress_controller_endpoint, + ingress_controller_prerequisites, + crd_ingress_controller, + test_namespace, + virtual_server_setup, + keycloak_setup, + configmap, + ): + + print(f"Create oidc secret") + with open(oidc_secret_src) as f: + secret_data = yaml.safe_load(f) + secret_data["data"]["client-secret"] = keycloak_setup.secret + secret_name = create_secret(kube_apis.v1, test_namespace, secret_data) + + print(f"Create oidc policy") + with open(pkce_pol_src) as f: + doc = yaml.safe_load(f) + pol = doc["metadata"]["name"] + doc["spec"]["oidc"]["tokenEndpoint"] = doc["spec"]["oidc"]["tokenEndpoint"].replace("default", test_namespace) + doc["spec"]["oidc"]["jwksURI"] = doc["spec"]["oidc"]["jwksURI"].replace("default", test_namespace) + kube_apis.custom_objects.create_namespaced_custom_object("k8s.nginx.org", "v1", test_namespace, "policies", doc) + print(f"Policy created with name {pol}") + wait_before_test() + + print(f"Create virtual server") + patch_virtual_server_from_yaml( + kube_apis.custom_objects, virtual_server_setup.vs_name, oidc_vs_src, test_namespace + ) + wait_before_test() + print(f"Update nginx configmap") + replace_configmap_from_yaml( + kube_apis.v1, + ingress_controller_prerequisites.config_map["metadata"]["name"], + ingress_controller_prerequisites.namespace, + configmap, + ) + wait_before_test() + + if configmap == cm_src: + print(f"Create headless service") + create_items_from_yaml(kube_apis, svc_src, ingress_controller_prerequisites.namespace) + + with sync_playwright() as playwright: + run_oidc(playwright.chromium, ingress_controller_endpoint.public_ip, ingress_controller_endpoint.port_ssl) + + replace_configmap_from_yaml( + kube_apis.v1, + ingress_controller_prerequisites.config_map["metadata"]["name"], + ingress_controller_prerequisites.namespace, + cm_src, + ) + delete_secret(kube_apis.v1, secret_name, test_namespace) + delete_policy(kube_apis.custom_objects, pol, test_namespace) + patch_virtual_server_from_yaml( + kube_apis.custom_objects, virtual_server_setup.vs_name, orig_vs_src, test_namespace + ) + + +def run_oidc(browser_type, ip_address, port): + + browser = browser_type.launch(headless=True, args=[f"--host-resolver-rules=MAP * {ip_address}:{port}"]) + context = browser.new_context(ignore_https_errors=True) + + try: + page = context.new_page() + + page.goto("https://virtual-server-tls.example.com") + page.wait_for_selector('input[name="username"]') + page.fill('input[name="username"]', username) + page.wait_for_selector('input[name="password"]', timeout=5000) + page.fill('input[name="password"]', password) + + with page.expect_navigation(): + page.click('input[type="submit"]') + page.wait_for_load_state("load") + page_text = page.text_content("body") + fields_to_check = [ + "Server address:", + "Server name:", + "Date:", + "Request ID:", + ] + for field in fields_to_check: + assert field in page_text, f"'{field}' not found in page text" + + except Error as e: + assert False, f"Error: {e}" + + finally: + context.close() + browser.close() From 730635e06a26029b9ade4bad4a65145dfdf33c4b Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 17:06:50 +0200 Subject: [PATCH 14/41] Update snapshot after changing a str -> int --- internal/configs/version2/__snapshots__/templates_test.snap | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index 0f51bdf4d6..df3f494012 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -1332,7 +1332,7 @@ server { set $resource_namespace ""; include oidc/oidc.conf; - set $oidc_pkce_enable "1"; + set $oidc_pkce_enable 1; set $oidc_client_auth_method "client_secret_post"; set $oidc_logout_redirect ""; set $oidc_hmac_key ""; From 510902063ff916570472a699aedf77ff09569585 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 17:54:16 +0200 Subject: [PATCH 15/41] oidc and pkce pytest fixture scope to function --- tests/suite/test_oidc.py | 2 +- tests/suite/test_pkce.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/suite/test_oidc.py b/tests/suite/test_oidc.py index 70f7aa2704..9bde872958 100644 --- a/tests/suite/test_oidc.py +++ b/tests/suite/test_oidc.py @@ -48,7 +48,7 @@ def __init__(self, secret): self.secret = secret -@pytest.fixture(scope="class") +@pytest.fixture(scope="function") def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): # Create Keycloak resources and setup Keycloak idp diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py index aab5438ade..1498d80d42 100644 --- a/tests/suite/test_pkce.py +++ b/tests/suite/test_pkce.py @@ -47,7 +47,7 @@ def __init__(self, secret): self.secret = secret -@pytest.fixture(scope="class") +@pytest.fixture(scope="function") def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): # Create Keycloak resources and setup Keycloak idp with PKCE enabled From 97737b67ddcb41d73c821d3f0c5298ca3538c71e Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 18:39:43 +0200 Subject: [PATCH 16/41] OIDC tests should be class fixtured --- tests/suite/test_oidc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suite/test_oidc.py b/tests/suite/test_oidc.py index 9bde872958..70f7aa2704 100644 --- a/tests/suite/test_oidc.py +++ b/tests/suite/test_oidc.py @@ -48,7 +48,7 @@ def __init__(self, secret): self.secret = secret -@pytest.fixture(scope="function") +@pytest.fixture(scope="class") def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): # Create Keycloak resources and setup Keycloak idp From 318ea4592a6a6bddb4fddb169f255685038e32d0 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 19:08:55 +0200 Subject: [PATCH 17/41] Remove a parameter from pkce test --- tests/suite/test_pkce.py | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py index 1498d80d42..9a73b33c06 100644 --- a/tests/suite/test_pkce.py +++ b/tests/suite/test_pkce.py @@ -4,11 +4,10 @@ import requests import yaml from playwright.sync_api import Error, sync_playwright -from settings import DEPLOYMENTS, TEST_DATA +from settings import TEST_DATA from suite.utils.policy_resources_utils import delete_policy from suite.utils.resources_utils import ( create_example_app, - create_items_from_yaml, create_secret, create_secret_from_yaml, delete_common_app, @@ -31,9 +30,7 @@ pkce_pol_src = f"{TEST_DATA}/oidc/pkce.yaml" oidc_vs_src = f"{TEST_DATA}/oidc/virtual-server.yaml" orig_vs_src = f"{TEST_DATA}/virtual-server-tls/standard/virtual-server.yaml" -cm_src = f"{TEST_DATA}/oidc/nginx-config.yaml" cm_zs_src = f"{TEST_DATA}/oidc/nginx-config-zs.yaml" -orig_cm_src = f"{DEPLOYMENTS}/common/nginx-config.yaml" svc_src = f"{TEST_DATA}/oidc/nginx-ingress-headless.yaml" @@ -130,7 +127,7 @@ def fin(): indirect=True, ) class TestPKCE: - @pytest.mark.parametrize("configmap", [cm_src, cm_zs_src]) + @pytest.mark.parametrize("configmap", [cm_zs_src]) def test_pkce( self, request, @@ -174,19 +171,9 @@ def test_pkce( ) wait_before_test() - if configmap == cm_src: - print(f"Create headless service") - create_items_from_yaml(kube_apis, svc_src, ingress_controller_prerequisites.namespace) - with sync_playwright() as playwright: run_oidc(playwright.chromium, ingress_controller_endpoint.public_ip, ingress_controller_endpoint.port_ssl) - replace_configmap_from_yaml( - kube_apis.v1, - ingress_controller_prerequisites.config_map["metadata"]["name"], - ingress_controller_prerequisites.namespace, - cm_src, - ) delete_secret(kube_apis.v1, secret_name, test_namespace) delete_policy(kube_apis.custom_objects, pol, test_namespace) patch_virtual_server_from_yaml( From 84b29771ee2bd8ad8eb95a768433698c116743ae Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 19:29:49 +0200 Subject: [PATCH 18/41] pkce test fixture should also be class scoped --- tests/suite/test_pkce.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py index 9a73b33c06..0cfde63222 100644 --- a/tests/suite/test_pkce.py +++ b/tests/suite/test_pkce.py @@ -44,7 +44,7 @@ def __init__(self, secret): self.secret = secret -@pytest.fixture(scope="function") +@pytest.fixture(scope="class") def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): # Create Keycloak resources and setup Keycloak idp with PKCE enabled From f0d13c66a3447915f7ead35126617822a1f47380 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Fri, 16 May 2025 19:38:46 +0200 Subject: [PATCH 19/41] Add debug prints --- tests/suite/test_oidc.py | 5 +++++ tests/suite/test_pkce.py | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/tests/suite/test_oidc.py b/tests/suite/test_oidc.py index 70f7aa2704..1ba52cb27e 100644 --- a/tests/suite/test_oidc.py +++ b/tests/suite/test_oidc.py @@ -199,6 +199,11 @@ def run_oidc(browser_type, ip_address, port): page = context.new_page() page.goto("https://virtual-server-tls.example.com") + + print("\n\n\n") + print(page.content()) + print("\n\n\n") + page.wait_for_selector('input[name="username"]') page.fill('input[name="username"]', username) page.wait_for_selector('input[name="password"]', timeout=5000) diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py index 0cfde63222..174c059ad3 100644 --- a/tests/suite/test_pkce.py +++ b/tests/suite/test_pkce.py @@ -190,6 +190,11 @@ def run_oidc(browser_type, ip_address, port): page = context.new_page() page.goto("https://virtual-server-tls.example.com") + + print("\n\n\n") + print(page.content()) + print("\n\n\n") + page.wait_for_selector('input[name="username"]') page.fill('input[name="username"]', username) page.wait_for_selector('input[name="password"]', timeout=5000) From 67b8385d0c449ce6ff1a7a5f47e16c1c61495390 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 20 May 2025 10:20:38 +0200 Subject: [PATCH 20/41] Merge pkce test into oidc test file --- tests/suite/test_oidc.py | 33 ++++-- tests/suite/test_pkce.py | 221 --------------------------------------- 2 files changed, 27 insertions(+), 227 deletions(-) delete mode 100644 tests/suite/test_pkce.py diff --git a/tests/suite/test_oidc.py b/tests/suite/test_oidc.py index 1ba52cb27e..da4f55d7f2 100644 --- a/tests/suite/test_oidc.py +++ b/tests/suite/test_oidc.py @@ -14,6 +14,7 @@ create_secret_from_yaml, delete_common_app, delete_secret, + delete_service, replace_configmap_from_yaml, wait_before_test, wait_until_all_pods_are_ready, @@ -30,6 +31,7 @@ keycloak_vs_src = f"{TEST_DATA}/oidc/virtual-server-idp.yaml" oidc_secret_src = f"{TEST_DATA}/oidc/client-secret.yaml" oidc_pol_src = f"{TEST_DATA}/oidc/oidc.yaml" +pkce_pol_src = f"{TEST_DATA}/oidc/pkce.yaml" oidc_vs_src = f"{TEST_DATA}/oidc/virtual-server.yaml" orig_vs_src = f"{TEST_DATA}/virtual-server-tls/standard/virtual-server.yaml" cm_src = f"{TEST_DATA}/oidc/nginx-config.yaml" @@ -81,6 +83,23 @@ def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoi } response = requests.post(create_user_url, headers=headers, json=user_payload, verify=False) + # Create client "nginx-plus-pkce" for the pkce test + create_pkce_client_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/admin/realms/master/clients" + pkce_client_payload = { + "clientId": "nginx-plus-pkce", + "redirectUris": ["https://virtual-server-tls.example.com:443/_codexch"], + "standardFlowEnabled": True, + "directAccessGrantsEnabled": False, + "publicClient": True, + "attributes": { + "post.logout.redirect.uris": "https://virtual-server-tls.example.com:443/*", + "pkce.code.challenge.method": "S256", + }, + "protocol": "openid-connect", + } + pkce_client_resp = requests.post(create_pkce_client_url, headers=headers, json=pkce_client_payload, verify=False) + pkce_client_resp.raise_for_status() + # Create client "nginx-plus" and get secret create_client_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/realms/master/clients-registrations/default" client_payload = { @@ -128,6 +147,7 @@ def fin(): ) class TestOIDC: @pytest.mark.parametrize("configmap", [cm_src, cm_zs_src]) + @pytest.mark.parametrize("oidcYaml", [oidc_pol_src, pkce_pol_src]) def test_oidc( self, request, @@ -139,6 +159,7 @@ def test_oidc( virtual_server_setup, keycloak_setup, configmap, + oidcYaml, ): print(f"Create oidc secret") with open(oidc_secret_src) as f: @@ -147,7 +168,7 @@ def test_oidc( secret_name = create_secret(kube_apis.v1, test_namespace, secret_data) print(f"Create oidc policy") - with open(oidc_pol_src) as f: + with open(oidcYaml) as f: doc = yaml.safe_load(f) pol = doc["metadata"]["name"] doc["spec"]["oidc"]["tokenEndpoint"] = doc["spec"]["oidc"]["tokenEndpoint"].replace("default", test_namespace) @@ -188,6 +209,11 @@ def test_oidc( patch_virtual_server_from_yaml( kube_apis.custom_objects, virtual_server_setup.vs_name, orig_vs_src, test_namespace ) + if configmap == cm_src: + with open(svc_src) as f: + headless_svc = yaml.safe_load(f) + headless_name = headless_svc["metadata"]["name"] + delete_service(kube_apis.v1, headless_name, ingress_controller_prerequisites.namespace) def run_oidc(browser_type, ip_address, port): @@ -199,11 +225,6 @@ def run_oidc(browser_type, ip_address, port): page = context.new_page() page.goto("https://virtual-server-tls.example.com") - - print("\n\n\n") - print(page.content()) - print("\n\n\n") - page.wait_for_selector('input[name="username"]') page.fill('input[name="username"]', username) page.wait_for_selector('input[name="password"]', timeout=5000) diff --git a/tests/suite/test_pkce.py b/tests/suite/test_pkce.py deleted file mode 100644 index 174c059ad3..0000000000 --- a/tests/suite/test_pkce.py +++ /dev/null @@ -1,221 +0,0 @@ -import secrets - -import pytest -import requests -import yaml -from playwright.sync_api import Error, sync_playwright -from settings import TEST_DATA -from suite.utils.policy_resources_utils import delete_policy -from suite.utils.resources_utils import ( - create_example_app, - create_secret, - create_secret_from_yaml, - delete_common_app, - delete_secret, - replace_configmap_from_yaml, - wait_before_test, - wait_until_all_pods_are_ready, -) -from suite.utils.vs_vsr_resources_utils import ( - create_virtual_server_from_yaml, - delete_virtual_server, - patch_virtual_server_from_yaml, -) - -username = "nginx-user-" + secrets.token_hex(4) -password = secrets.token_hex(8) -keycloak_src = f"{TEST_DATA}/oidc/keycloak.yaml" -keycloak_vs_src = f"{TEST_DATA}/oidc/virtual-server-idp.yaml" -oidc_secret_src = f"{TEST_DATA}/oidc/client-secret.yaml" -pkce_pol_src = f"{TEST_DATA}/oidc/pkce.yaml" -oidc_vs_src = f"{TEST_DATA}/oidc/virtual-server.yaml" -orig_vs_src = f"{TEST_DATA}/virtual-server-tls/standard/virtual-server.yaml" -cm_zs_src = f"{TEST_DATA}/oidc/nginx-config-zs.yaml" -svc_src = f"{TEST_DATA}/oidc/nginx-ingress-headless.yaml" - - -class KeycloakSetupPKCE: - """ - Attributes: - secret (str): - """ - - def __init__(self, secret): - self.secret = secret - - -@pytest.fixture(scope="class") -def keycloak_setup(request, kube_apis, test_namespace, ingress_controller_endpoint, virtual_server_setup): - - # Create Keycloak resources and setup Keycloak idp with PKCE enabled - secret_name = create_secret_from_yaml( - kube_apis.v1, virtual_server_setup.namespace, f"{TEST_DATA}/virtual-server-tls/tls-secret.yaml" - ) - keycloak_address = "keycloak.example.com" - create_example_app(kube_apis, "keycloak", test_namespace) - wait_before_test() - wait_until_all_pods_are_ready(kube_apis.v1, test_namespace) - keycloak_vs_name = create_virtual_server_from_yaml(kube_apis.custom_objects, keycloak_vs_src, test_namespace) - wait_before_test() - - # Get token - url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/realms/master/protocol/openid-connect/token" - headers = {"Host": keycloak_address, "Content-Type": "application/x-www-form-urlencoded"} - data = {"username": "admin", "password": "admin", "grant_type": "password", "client_id": "admin-cli"} - - response = requests.post(url, headers=headers, data=data, verify=False) - token = response.json()["access_token"] - - # Create a user and set credentials - create_user_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/admin/realms/master/users" - headers = {"Content-Type": "application/json", "Authorization": f"Bearer {token}", "Host": keycloak_address} - user_payload = { - "username": username, - "enabled": True, - "credentials": [{"type": "password", "value": password, "temporary": False}], - } - response = requests.post(create_user_url, headers=headers, json=user_payload, verify=False) - - # Create client "nginx-plus" - create_client_url = f"https://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port_ssl}/admin/realms/master/clients" - client_payload = { - "clientId": "nginx-plus-pkce", - "redirectUris": ["https://virtual-server-tls.example.com:443/_codexch"], - "standardFlowEnabled": True, - "directAccessGrantsEnabled": False, - "publicClient": True, - "attributes": { - "post.logout.redirect.uris": "https://virtual-server-tls.example.com:443/*", - "pkce.code.challenge.method": "S256", - }, - "protocol": "openid-connect", - } - client_resp = requests.post(create_client_url, headers=headers, json=client_payload, verify=False) - client_resp.raise_for_status() - # let's check that it's a 201 somehow - - print(f"Keycloak setup complete.") - - def fin(): - if request.config.getoption("--skip-fixture-teardown") == "no": - print("Delete Keycloak resources") - delete_virtual_server(kube_apis.custom_objects, keycloak_vs_name, test_namespace) - delete_common_app(kube_apis, "keycloak", test_namespace) - delete_secret(kube_apis.v1, secret_name, test_namespace) - - request.addfinalizer(fin) - - # base64 encoded "pkce-doesnt-support-client-secrets" - return KeycloakSetupPKCE("cGtjZS1kb2VzbnQtc3VwcG9ydC1jbGllbnQtc2VjcmV0cw==") - - -@pytest.mark.oidc -@pytest.mark.skip_for_nginx_oss -@pytest.mark.parametrize( - "crd_ingress_controller, virtual_server_setup", - [ - ( - { - "type": "complete", - "extra_args": [ - f"-enable-oidc", - ], - }, - {"example": "virtual-server-tls", "app_type": "simple"}, - ) - ], - indirect=True, -) -class TestPKCE: - @pytest.mark.parametrize("configmap", [cm_zs_src]) - def test_pkce( - self, - request, - kube_apis, - ingress_controller_endpoint, - ingress_controller_prerequisites, - crd_ingress_controller, - test_namespace, - virtual_server_setup, - keycloak_setup, - configmap, - ): - - print(f"Create oidc secret") - with open(oidc_secret_src) as f: - secret_data = yaml.safe_load(f) - secret_data["data"]["client-secret"] = keycloak_setup.secret - secret_name = create_secret(kube_apis.v1, test_namespace, secret_data) - - print(f"Create oidc policy") - with open(pkce_pol_src) as f: - doc = yaml.safe_load(f) - pol = doc["metadata"]["name"] - doc["spec"]["oidc"]["tokenEndpoint"] = doc["spec"]["oidc"]["tokenEndpoint"].replace("default", test_namespace) - doc["spec"]["oidc"]["jwksURI"] = doc["spec"]["oidc"]["jwksURI"].replace("default", test_namespace) - kube_apis.custom_objects.create_namespaced_custom_object("k8s.nginx.org", "v1", test_namespace, "policies", doc) - print(f"Policy created with name {pol}") - wait_before_test() - - print(f"Create virtual server") - patch_virtual_server_from_yaml( - kube_apis.custom_objects, virtual_server_setup.vs_name, oidc_vs_src, test_namespace - ) - wait_before_test() - print(f"Update nginx configmap") - replace_configmap_from_yaml( - kube_apis.v1, - ingress_controller_prerequisites.config_map["metadata"]["name"], - ingress_controller_prerequisites.namespace, - configmap, - ) - wait_before_test() - - with sync_playwright() as playwright: - run_oidc(playwright.chromium, ingress_controller_endpoint.public_ip, ingress_controller_endpoint.port_ssl) - - delete_secret(kube_apis.v1, secret_name, test_namespace) - delete_policy(kube_apis.custom_objects, pol, test_namespace) - patch_virtual_server_from_yaml( - kube_apis.custom_objects, virtual_server_setup.vs_name, orig_vs_src, test_namespace - ) - - -def run_oidc(browser_type, ip_address, port): - - browser = browser_type.launch(headless=True, args=[f"--host-resolver-rules=MAP * {ip_address}:{port}"]) - context = browser.new_context(ignore_https_errors=True) - - try: - page = context.new_page() - - page.goto("https://virtual-server-tls.example.com") - - print("\n\n\n") - print(page.content()) - print("\n\n\n") - - page.wait_for_selector('input[name="username"]') - page.fill('input[name="username"]', username) - page.wait_for_selector('input[name="password"]', timeout=5000) - page.fill('input[name="password"]', password) - - with page.expect_navigation(): - page.click('input[type="submit"]') - page.wait_for_load_state("load") - page_text = page.text_content("body") - fields_to_check = [ - "Server address:", - "Server name:", - "Date:", - "Request ID:", - ] - for field in fields_to_check: - assert field in page_text, f"'{field}' not found in page text" - - except Error as e: - assert False, f"Error: {e}" - - finally: - context.close() - browser.close() From 71d4ddccc12a8675caa97e57dc6271276d1948bc Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 20 May 2025 10:38:00 +0200 Subject: [PATCH 21/41] Add unit tests for the bool to int util function --- .../configs/version2/template_helper_test.go | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/internal/configs/version2/template_helper_test.go b/internal/configs/version2/template_helper_test.go index 58d166657d..cc9a53067b 100644 --- a/internal/configs/version2/template_helper_test.go +++ b/internal/configs/version2/template_helper_test.go @@ -815,3 +815,31 @@ func newReplaceAll(t *testing.T) *template.Template { } return tmpl } + +func TestBoolToInt(t *testing.T) { + cases := []struct { + name string + in bool + want int + }{ + { + name: "converts true to 1", + in: true, + want: 1, + }, + { + name: "converts false to 0", + in: false, + want: 0, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := boolToInteger(tc.in) + if got != tc.want { + t.Errorf("boolToInteger(%v) returned %v but expected %v.", tc.in, got, tc.want) + } + }) + } +} From df5d0e41cf2249a07ee034117e5dc5c1f9c8610f Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 27 May 2025 10:33:11 +0100 Subject: [PATCH 22/41] Add docs to create keycloak client via api --- .../custom-resources/oidc/keycloak_setup.md | 52 ++++++++++++++++--- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/examples/custom-resources/oidc/keycloak_setup.md b/examples/custom-resources/oidc/keycloak_setup.md index 570778e365..e7d9a49fce 100644 --- a/examples/custom-resources/oidc/keycloak_setup.md +++ b/examples/custom-resources/oidc/keycloak_setup.md @@ -42,14 +42,50 @@ Steps: curl -sS -k -X POST -d '{ "username": "nginx-user", "enabled": true, "credentials":[{"type": "password", "value": "test", "temporary": false}]}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/admin/realms/master/users ``` -1. Create the client `nginx-plus` and retrieve the secret: +1. Create the client `nginx-plus`: - ```console - SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` - ``` +{{< tabs name="create-keycloak-client" >}} - If everything went well you should have the secret stored in $SECRET. To double check run: +{{%tab name="Standard OIDC client `nginx-ingress`"%}} - ```console - echo $SECRET - ``` +Use the following command to create an OIDC client that does not use PKCE: + +```console +SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` +``` + +If everything went well, you should have the secret stored in $SECRET. To double-check, run: + +```console +echo $SECRET +``` + +{{%/tab%}} + +{{%tab name="PKCE Client `nginx-ingress-pkce`"%}} + +Use the following command to create an OIDC client that uses PKCE: + +```console +curl -sS -k -H "Content-Type: application/json" -H "Authorization: Bearer ${TOKEN}" \ +--data '{ + "clientId": "nginx-plus", + "enabled": true, + "standardFlowEnabled": true, + "directAccessGrantsEnabled": false, + "publicClient": true, + "redirectUris": [ + "https://webapp.example.com:443/_codexch" + ], + "attributes": { + "pkce.code.challenge.method":"S256", + "post.logout.redirect.uris": "https://webapp.example.com:443/*" + }, + "protocol": "openid-connect" +}' \ +https://${KEYCLOAK_ADDRESS}/admin/realms/master/clients +``` + +{{%/tab%}} + +{{< /tabs >}} From 94c59be90e768396a8dd70b8017293917aa10ee3 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Tue, 27 May 2025 10:41:47 +0100 Subject: [PATCH 23/41] Reword options because no tabs --- .../custom-resources/oidc/keycloak_setup.md | 86 ++++++++----------- 1 file changed, 37 insertions(+), 49 deletions(-) diff --git a/examples/custom-resources/oidc/keycloak_setup.md b/examples/custom-resources/oidc/keycloak_setup.md index e7d9a49fce..6b8cf5eef5 100644 --- a/examples/custom-resources/oidc/keycloak_setup.md +++ b/examples/custom-resources/oidc/keycloak_setup.md @@ -21,7 +21,7 @@ Steps: KEYCLOAK_ADDRESS=keycloak.example.com ``` -1. Retrieve the access token and store it into a shell variable: +2. Retrieve the access token and store it into a shell variable: ```console TOKEN=`curl -sS -k --data "username=admin&password=admin&grant_type=password&client_id=admin-cli" "https://${KEYCLOAK_ADDRESS}/realms/master/protocol/openid-connect/token" | jq -r .access_token` @@ -36,56 +36,44 @@ Steps: ***Note***: The access token lifespan is very short. If it expires between commands, retrieve it again with the command above. -1. Create the user `nginx-user`: +3. Create the user `nginx-user`: ```console curl -sS -k -X POST -d '{ "username": "nginx-user", "enabled": true, "credentials":[{"type": "password", "value": "test", "temporary": false}]}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/admin/realms/master/users ``` -1. Create the client `nginx-plus`: - -{{< tabs name="create-keycloak-client" >}} - -{{%tab name="Standard OIDC client `nginx-ingress`"%}} - -Use the following command to create an OIDC client that does not use PKCE: - -```console -SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` -``` - -If everything went well, you should have the secret stored in $SECRET. To double-check, run: - -```console -echo $SECRET -``` - -{{%/tab%}} - -{{%tab name="PKCE Client `nginx-ingress-pkce`"%}} - -Use the following command to create an OIDC client that uses PKCE: - -```console -curl -sS -k -H "Content-Type: application/json" -H "Authorization: Bearer ${TOKEN}" \ ---data '{ - "clientId": "nginx-plus", - "enabled": true, - "standardFlowEnabled": true, - "directAccessGrantsEnabled": false, - "publicClient": true, - "redirectUris": [ - "https://webapp.example.com:443/_codexch" - ], - "attributes": { - "pkce.code.challenge.method":"S256", - "post.logout.redirect.uris": "https://webapp.example.com:443/*" - }, - "protocol": "openid-connect" -}' \ -https://${KEYCLOAK_ADDRESS}/admin/realms/master/clients -``` - -{{%/tab%}} - -{{< /tabs >}} +4. Create the client `nginx-plus`: + + 1. If you are not using PKCE, use the following command to create an OIDC client that does not use PKCE: + + ```console + SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` + ``` + + If everything went well, you should have the secret stored in $SECRET. To double-check, run: + + ```console + echo $SECRET + ``` + + 2. Use the following command to create an OIDC client that uses PKCE: + + ```console + curl -sS -k -H "Content-Type: application/json" -H "Authorization: Bearer ${TOKEN}" \ + --data '{ + "clientId": "nginx-plus", + "enabled": true, + "standardFlowEnabled": true, + "directAccessGrantsEnabled": false, + "publicClient": true, + "redirectUris": [ + "https://webapp.example.com:443/_codexch" + ], + "attributes": { + "pkce.code.challenge.method":"S256", + "post.logout.redirect.uris": "https://webapp.example.com:443/*" + }, + "protocol": "openid-connect" + }' \ + https://${KEYCLOAK_ADDRESS}/admin/realms/master/clients + ``` From 7544728bd0fef6823640eb630cc3c2f5ad984472 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 09:05:24 +0100 Subject: [PATCH 24/41] OIDC example deploy keycloak into nginx-ingress ns --- examples/custom-resources/oidc/keycloak.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/custom-resources/oidc/keycloak.yaml b/examples/custom-resources/oidc/keycloak.yaml index 0e879dfa11..ac13098488 100644 --- a/examples/custom-resources/oidc/keycloak.yaml +++ b/examples/custom-resources/oidc/keycloak.yaml @@ -16,7 +16,7 @@ apiVersion: apps/v1 kind: Deployment metadata: name: keycloak - namespace: default + namespace: nginx-ingress labels: app: keycloak spec: From 1466bae97cc60e43fb82296da53d943ff9066d30 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 09:09:31 +0100 Subject: [PATCH 25/41] Add plus-mgmt-configmap.yaml to instructions --- examples/custom-resources/oidc/README.md | 12 ++++++++++++ .../includes/installation/manifests/deployment.md | 1 + 2 files changed, 13 insertions(+) diff --git a/examples/custom-resources/oidc/README.md b/examples/custom-resources/oidc/README.md index 9fd03fa70b..da53129950 100644 --- a/examples/custom-resources/oidc/README.md +++ b/examples/custom-resources/oidc/README.md @@ -5,6 +5,18 @@ application using an OpenID Connect policy and [Keycloak](https://www.keycloak.o **Note**: The KeyCloak container does not support IPv6 environments. +**Note**: This example assumes that your default namespace is set to `default`. You can check this with + +```shell +kubectl config view --minify | grep namespace +``` + +If it's not empty, and anything other than `default`, you can set to `default` with the following command: + +```shell +kubectl config set-context --namespace default --current +``` + ## Prerequisites 1. Follow the [installation](https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-manifests/) diff --git a/site/content/includes/installation/manifests/deployment.md b/site/content/includes/installation/manifests/deployment.md index 3786a2256b..b32955a69f 100644 --- a/site/content/includes/installation/manifests/deployment.md +++ b/site/content/includes/installation/manifests/deployment.md @@ -15,6 +15,7 @@ When you deploy NGINX Ingress Controller as a Deployment, Kubernetes automatical - For NGINX Plus, run: ```shell + kubectl apply -f deployments/common/plus-mgmt-configmap.yaml kubectl apply -f deployments/deployment/nginx-plus-ingress.yaml ``` From 76ad8bcbd0c476fe37ae083ef241e5dc7215dca7 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 12:29:41 +0100 Subject: [PATCH 26/41] Redo list numbers in oidc example readme --- examples/custom-resources/oidc/README.md | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/examples/custom-resources/oidc/README.md b/examples/custom-resources/oidc/README.md index da53129950..a293ae2a37 100644 --- a/examples/custom-resources/oidc/README.md +++ b/examples/custom-resources/oidc/README.md @@ -22,7 +22,7 @@ kubectl config set-context --namespace default --current 1. Follow the [installation](https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-manifests/) instructions to deploy the Ingress Controller. This example requires that the HTTPS port of the Ingress Controller is `443`. -1. Save the public IP address of the Ingress Controller into `/etc/hosts` of your machine: +2. Save the public IP address of the Ingress Controller into `/etc/hosts` of your machine: ```text ... @@ -59,7 +59,7 @@ kubectl apply -f webapp.yaml kubectl apply -f keycloak.yaml ``` -1. Create a VirtualServer resource for Keycloak: +2. Create a VirtualServer resource for Keycloak: ```console kubectl apply -f virtual-server-idp.yaml @@ -71,13 +71,13 @@ To set up Keycloak: 1. Follow the steps in the "Configuring Keycloak" [section of the documentation](https://docs.nginx.com/nginx/deployment-guides/single-sign-on/keycloak/#configuring-keycloak): 1. To connect to Keycloak, use `https://keycloak.example.com`. - 1. Make sure to save the client secret for NGINX-Plus client to the `SECRET` shell variable: + 2. Make sure to save the client secret for NGINX-Plus client to the `SECRET` shell variable: ```console SECRET=value ``` -1. Alternatively, [execute the commands](./keycloak_setup.md). +2. Alternatively, [execute the commands](./keycloak_setup.md). ## Step 5 - Deploy the Client Secret @@ -87,9 +87,9 @@ To set up Keycloak: echo -n $SECRET | base64 ``` -1. Edit `client-secret.yaml`, replacing `` with the encoded secret. +2. Edit `client-secret.yaml`, replacing `` with the encoded secret. -1. Create a secret with the name `oidc-secret` that will be used by the OIDC policy: +3. Create a secret with the name `oidc-secret` that will be used by the OIDC policy: ```console kubectl apply -f client-secret.yaml @@ -134,9 +134,9 @@ Note that the VirtualServer references the policy `oidc-policy` created in Step 1. Open a web browser and navigate to the URL of the web application: `https://webapp.example.com`. You will be redirected to Keycloak. -1. Log in with the username and password for the user you created in Keycloak, `nginx-user` and `test`. +2. Log in with the username and password for the user you created in Keycloak, `nginx-user` and `test`. ![keycloak](./keycloak.png) -1. Once logged in, you will be redirected to the web application and get a response from it. Notice the field `User ID` +3. Once logged in, you will be redirected to the web application and get a response from it. Notice the field `User ID` in the response, this will match the ID for your user in Keycloak. ![webapp](./webapp.png) ## Step 10 - Log Out @@ -144,5 +144,5 @@ in the response, this will match the ID for your user in Keycloak. ![webapp](./w 1. To log out, navigate to `https://webapp.example.com/logout`. Your session will be terminated, and you will be redirected to the default post logout URI `https://webapp.example.com/_logout`. ![logout](./logout.png) -1. To confirm that you have been logged out, navigate to `https://webapp.example.com`. You will be redirected to +2. To confirm that you have been logged out, navigate to `https://webapp.example.com`. You will be redirected to Keycloak to log in again. From e1ab42679c268255210f35aa01e56f26bebb3232 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 12:29:56 +0100 Subject: [PATCH 27/41] Reset keycloak to be in default namespace --- examples/custom-resources/oidc/keycloak.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/custom-resources/oidc/keycloak.yaml b/examples/custom-resources/oidc/keycloak.yaml index ac13098488..0e879dfa11 100644 --- a/examples/custom-resources/oidc/keycloak.yaml +++ b/examples/custom-resources/oidc/keycloak.yaml @@ -16,7 +16,7 @@ apiVersion: apps/v1 kind: Deployment metadata: name: keycloak - namespace: nginx-ingress + namespace: default labels: app: keycloak spec: From 8c8e56c4d07bb3dababb1bf2ae4556f12516be67 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 12:32:23 +0100 Subject: [PATCH 28/41] Add note on not using client secret for PKCE --- examples/custom-resources/oidc/README.md | 3 +++ internal/configs/virtualserver.go | 8 ++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/examples/custom-resources/oidc/README.md b/examples/custom-resources/oidc/README.md index a293ae2a37..d1197c598d 100644 --- a/examples/custom-resources/oidc/README.md +++ b/examples/custom-resources/oidc/README.md @@ -81,6 +81,9 @@ To set up Keycloak: ## Step 5 - Deploy the Client Secret +**Note**: If you're using PKCE, skip this step. PKCE clients do not have client secrets. Applying this will result +in a broken deployment. + 1. Encode the secret, obtained in the previous step: ```console diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 68cbb9cbd5..a75a685100 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -34,6 +34,7 @@ const ( splitClientsKeyValZoneSize = "100k" splitClientAmountWhenWeightChangesDynamicReload = 101 defaultLogOutput = "syslog:server=localhost:514" + pkceClientValue = "cGtjZS1oYXMtbm8tc2VjcmV0" // "pkce-has-no-secret" base64 encoded ) var grpcConflictingErrors = map[int]bool{ @@ -1372,6 +1373,7 @@ func (p *policiesCfg) addOIDCConfig( } else { secretKey := fmt.Sprintf("%v/%v", polNamespace, oidc.ClientSecret) secretRef := secretRefs[secretKey] + clientSecret := []byte(pkceClientValue) var secretType api_v1.SecretType if secretRef.Secret != nil { @@ -1381,13 +1383,15 @@ func (p *policiesCfg) addOIDCConfig( res.addWarningf("OIDC policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeOIDC) res.isError = true return res - } else if secretRef.Error != nil { + } else if secretRef.Error != nil && !oidc.PKCEEnabled { res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true return res } - clientSecret := secretRef.Secret.Data[ClientSecretKey] + if !oidc.PKCEEnabled { + clientSecret = secretRef.Secret.Data[ClientSecretKey] + } redirectURI := oidc.RedirectURI if redirectURI == "" { From 81ef4a141a5ea7c6132e1ad06088e4fc3338aa3b Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 13:58:04 +0100 Subject: [PATCH 29/41] Move applying the plus mgmt to common resources --- .../includes/installation/create-common-resources.md | 6 ++++++ site/content/includes/installation/manifests/deployment.md | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/site/content/includes/installation/create-common-resources.md b/site/content/includes/installation/create-common-resources.md index d30f555255..5ebea07176 100644 --- a/site/content/includes/installation/create-common-resources.md +++ b/site/content/includes/installation/create-common-resources.md @@ -18,6 +18,12 @@ In this section, you'll create resources that most NGINX Ingress Controller inst kubectl apply -f deployments/common/nginx-config.yaml ``` + If you're using NGINX Plus, you will also need to apply the management config map as well + + ```shell + kubectl apply -f deployments/common/plus-mgmt-configmap.yaml + ``` + 3. Create an `IngressClass` resource. NGINX Ingress Controller won't start without an `IngressClass` resource. ```shell diff --git a/site/content/includes/installation/manifests/deployment.md b/site/content/includes/installation/manifests/deployment.md index b32955a69f..3786a2256b 100644 --- a/site/content/includes/installation/manifests/deployment.md +++ b/site/content/includes/installation/manifests/deployment.md @@ -15,7 +15,6 @@ When you deploy NGINX Ingress Controller as a Deployment, Kubernetes automatical - For NGINX Plus, run: ```shell - kubectl apply -f deployments/common/plus-mgmt-configmap.yaml kubectl apply -f deployments/deployment/nginx-plus-ingress.yaml ``` From 41016cd09a4d918359a78407a5f8d637bf3aa3c4 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 16:54:06 +0100 Subject: [PATCH 30/41] Add pkceEnabled to policy resource doc --- site/content/configuration/policy-resource.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/site/content/configuration/policy-resource.md b/site/content/configuration/policy-resource.md index 727509c481..4633b7f128 100644 --- a/site/content/configuration/policy-resource.md +++ b/site/content/configuration/policy-resource.md @@ -649,6 +649,7 @@ spec: endSessionEndpoint: https://idp.example.com/openid-connect/logout postLogoutRedirectURI: / accessTokenEnable: true + pkceEnabled: false ``` NGINX Plus will pass the ID of an authenticated user to the backend in the HTTP header `username`. @@ -689,6 +690,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw |``postLogoutRedirectURI`` | URI to redirect to after the logout has been performed. Requires ``endSessionEndpoint``. The default is ``/_logout``. | ``string`` | No | |``zoneSyncLeeway`` | Specifies the maximum timeout in milliseconds for synchronizing ID/access tokens and shared values between Ingress Controller pods. The default is ``200``. | ``int`` | No | |``accessTokenEnable`` | Option of whether Bearer token is used to authorize NGINX to access protected backend. | ``boolean`` | No | +|``pkceEnabled`` | Switches Proof Key for Code Exchange on. The OpenID client needs to be in public mode. `clientSecret` is not used in this mode. | ``boolean`` | No | {{% /table %}} {{< note >}} From 878cf384573653d7414c05c582f1b7a7c487d975 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Wed, 28 May 2025 17:00:53 +0100 Subject: [PATCH 31/41] Rename pkceEnabled from past to present tense --- config/crd/bases/k8s.nginx.org_policies.yaml | 2 +- deploy/crds.yaml | 2 +- internal/configs/version2/http.go | 2 +- internal/configs/version2/nginx-plus.virtualserver.tmpl | 4 ++-- internal/configs/version2/templates_test.go | 2 +- internal/configs/virtualserver.go | 6 +++--- internal/configs/virtualserver_test.go | 2 +- pkg/apis/configuration/v1/types.go | 2 +- site/content/configuration/policy-resource.md | 4 ++-- tests/data/oidc/pkce.yaml | 2 +- 10 files changed, 14 insertions(+), 14 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 2aee1a56d7..d44aab7672 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -163,7 +163,7 @@ spec: type: string jwksURI: type: string - pkceEnabled: + pkceEnable: type: boolean postLogoutRedirectURI: type: string diff --git a/deploy/crds.yaml b/deploy/crds.yaml index dab7788e68..c079eb106b 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -325,7 +325,7 @@ spec: type: string jwksURI: type: string - pkceEnabled: + pkceEnable: type: boolean postLogoutRedirectURI: type: string diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 16bc5f2dc8..3f27badf1c 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -153,7 +153,7 @@ type OIDC struct { ZoneSyncLeeway int AuthExtraArgs string AccessTokenEnable bool - PKCEEnabled bool + PKCEEnable bool } // APIKey holds API key configuration. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 8466b617aa..8d04789e14 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -82,7 +82,7 @@ match {{ $m.Name }} { proxy_cache_path /var/cache/nginx/jwks_uri_{{$s.VSName}} levels=1 keys_zone=jwks_uri_{{$s.VSName}}:1m max_size=10m; {{- end }} -{{- if and $s.OIDC $s.OIDC.PKCEEnabled }} +{{- if and $s.OIDC $s.OIDC.PKCEEnable }} include oidc/oidc_pkce_supplements.conf; {{- end }} @@ -101,7 +101,7 @@ server { {{- with $oidc := $s.OIDC }} include oidc/oidc.conf; - set $oidc_pkce_enable {{ boolToInteger $oidc.PKCEEnabled }}; + set $oidc_pkce_enable {{ boolToInteger $oidc.PKCEEnable }}; set $oidc_client_auth_method "client_secret_post"; set $oidc_logout_redirect "{{ $oidc.PostLogoutRedirectURI }}"; set $oidc_hmac_key "{{ $s.VSName }}"; diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index 4b5cff7ff0..ae02e5f264 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -2443,7 +2443,7 @@ var ( StatusZone: "example.com", ProxyProtocol: true, OIDC: &OIDC{ - PKCEEnabled: true, + PKCEEnable: true, }, Locations: []Location{ { diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index a75a685100..fc18441095 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1383,13 +1383,13 @@ func (p *policiesCfg) addOIDCConfig( res.addWarningf("OIDC policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeOIDC) res.isError = true return res - } else if secretRef.Error != nil && !oidc.PKCEEnabled { + } else if secretRef.Error != nil && !oidc.PKCEEnable { res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true return res } - if !oidc.PKCEEnabled { + if !oidc.PKCEEnable { clientSecret = secretRef.Secret.Data[ClientSecretKey] } @@ -1423,7 +1423,7 @@ func (p *policiesCfg) addOIDCConfig( PostLogoutRedirectURI: postLogoutRedirectURI, ZoneSyncLeeway: generateIntFromPointer(oidc.ZoneSyncLeeway, 200), AccessTokenEnable: oidc.AccessTokenEnable, - PKCEEnabled: oidc.PKCEEnabled, + PKCEEnable: oidc.PKCEEnable, } oidcPolCfg.key = polKey } diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 746860bd2f..f5a0696757 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -10652,7 +10652,7 @@ func TestGeneratePoliciesFails(t *testing.T) { EndSessionEndpoint: "http://foo.com/bar", PostLogoutRedirectURI: "/_logout", AccessTokenEnable: true, - PKCEEnabled: true, + PKCEEnable: true, }, }, }, diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index e75469dd1b..5f379f31a7 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -685,7 +685,7 @@ type OIDC struct { ZoneSyncLeeway *int `json:"zoneSyncLeeway"` AuthExtraArgs []string `json:"authExtraArgs"` AccessTokenEnable bool `json:"accessTokenEnable"` - PKCEEnabled bool `json:"pkceEnabled"` + PKCEEnable bool `json:"pkceEnable"` } // WAF defines an WAF policy. diff --git a/site/content/configuration/policy-resource.md b/site/content/configuration/policy-resource.md index 4633b7f128..470b126d1d 100644 --- a/site/content/configuration/policy-resource.md +++ b/site/content/configuration/policy-resource.md @@ -649,7 +649,7 @@ spec: endSessionEndpoint: https://idp.example.com/openid-connect/logout postLogoutRedirectURI: / accessTokenEnable: true - pkceEnabled: false + pkceEnable: false ``` NGINX Plus will pass the ID of an authenticated user to the backend in the HTTP header `username`. @@ -690,7 +690,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw |``postLogoutRedirectURI`` | URI to redirect to after the logout has been performed. Requires ``endSessionEndpoint``. The default is ``/_logout``. | ``string`` | No | |``zoneSyncLeeway`` | Specifies the maximum timeout in milliseconds for synchronizing ID/access tokens and shared values between Ingress Controller pods. The default is ``200``. | ``int`` | No | |``accessTokenEnable`` | Option of whether Bearer token is used to authorize NGINX to access protected backend. | ``boolean`` | No | -|``pkceEnabled`` | Switches Proof Key for Code Exchange on. The OpenID client needs to be in public mode. `clientSecret` is not used in this mode. | ``boolean`` | No | +|``pkceEnable`` | Switches Proof Key for Code Exchange on. The OpenID client needs to be in public mode. `clientSecret` is not used in this mode. | ``boolean`` | No | {{% /table %}} {{< note >}} diff --git a/tests/data/oidc/pkce.yaml b/tests/data/oidc/pkce.yaml index bcf3c1cc3d..ed8c1f3ef9 100644 --- a/tests/data/oidc/pkce.yaml +++ b/tests/data/oidc/pkce.yaml @@ -12,4 +12,4 @@ spec: endSessionEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/logout scope: openid+profile+email accessTokenEnable: true - pkceEnabled: true + pkceEnable: true From 20cc31229f8b21978a687f86719a8f165df91b45 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 29 May 2025 11:39:36 +0100 Subject: [PATCH 32/41] Fix product name in example readme --- examples/custom-resources/oidc/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/custom-resources/oidc/README.md b/examples/custom-resources/oidc/README.md index d1197c598d..69e06273a5 100644 --- a/examples/custom-resources/oidc/README.md +++ b/examples/custom-resources/oidc/README.md @@ -20,8 +20,8 @@ kubectl config set-context --namespace default --current ## Prerequisites 1. Follow the [installation](https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-manifests/) - instructions to deploy the Ingress Controller. This example requires that the HTTPS port of the Ingress Controller is - `443`. + instructions to deploy NGINX Ingress Controller. This example requires that the HTTPS port of the Ingress + Controller is `443`. 2. Save the public IP address of the Ingress Controller into `/etc/hosts` of your machine: ```text From 9b9103163ed2d087cf072ec53f5ab39b2ba4b64f Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 29 May 2025 11:39:58 +0100 Subject: [PATCH 33/41] Change console code type to shell --- examples/custom-resources/oidc/README.md | 20 +++++++++---------- .../custom-resources/oidc/keycloak_setup.md | 14 ++++++------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/examples/custom-resources/oidc/README.md b/examples/custom-resources/oidc/README.md index 69e06273a5..3a500ed13c 100644 --- a/examples/custom-resources/oidc/README.md +++ b/examples/custom-resources/oidc/README.md @@ -39,7 +39,7 @@ kubectl config set-context --namespace default --current Create a secret with the TLS certificate and key that will be used for TLS termination of the web application and Keycloak: -```console +```shell kubectl apply -f tls-secret.yaml ``` @@ -47,7 +47,7 @@ kubectl apply -f tls-secret.yaml Create the application deployment and service: -```console +```shell kubectl apply -f webapp.yaml ``` @@ -55,13 +55,13 @@ kubectl apply -f webapp.yaml 1. Create the Keycloak deployment and service: - ```console + ```shell kubectl apply -f keycloak.yaml ``` 2. Create a VirtualServer resource for Keycloak: - ```console + ```shell kubectl apply -f virtual-server-idp.yaml ``` @@ -73,7 +73,7 @@ To set up Keycloak: 1. To connect to Keycloak, use `https://keycloak.example.com`. 2. Make sure to save the client secret for NGINX-Plus client to the `SECRET` shell variable: - ```console + ```shell SECRET=value ``` @@ -86,7 +86,7 @@ in a broken deployment. 1. Encode the secret, obtained in the previous step: - ```console + ```shell echo -n $SECRET | base64 ``` @@ -94,7 +94,7 @@ in a broken deployment. 3. Create a secret with the name `oidc-secret` that will be used by the OIDC policy: - ```console + ```shell kubectl apply -f client-secret.yaml ``` @@ -111,7 +111,7 @@ Steps: 1. Apply the ConfigMap `nginx-config.yaml`, which contains `zone-sync` configuration parameter that enable zone synchronization and the resolver using the kube-dns service. - ```console + ```shell kubectl apply -f nginx-config.yaml ``` @@ -119,7 +119,7 @@ Steps: Create a policy with the name `oidc-policy` that references the secret from the previous step: -```console +```shell kubectl apply -f oidc.yaml ``` @@ -127,7 +127,7 @@ kubectl apply -f oidc.yaml Create a VirtualServer resource for the web application: -```console +```shell kubectl apply -f virtual-server.yaml ``` diff --git a/examples/custom-resources/oidc/keycloak_setup.md b/examples/custom-resources/oidc/keycloak_setup.md index 6b8cf5eef5..d46e140ba1 100644 --- a/examples/custom-resources/oidc/keycloak_setup.md +++ b/examples/custom-resources/oidc/keycloak_setup.md @@ -17,19 +17,19 @@ Steps: 1. Save the address of Keycloak into a shell variable: - ```console + ```shell KEYCLOAK_ADDRESS=keycloak.example.com ``` 2. Retrieve the access token and store it into a shell variable: - ```console + ```shell TOKEN=`curl -sS -k --data "username=admin&password=admin&grant_type=password&client_id=admin-cli" "https://${KEYCLOAK_ADDRESS}/realms/master/protocol/openid-connect/token" | jq -r .access_token` ``` Ensure the request was successful and the token is stored in the shell variable by running: - ```console + ```shell echo $TOKEN ``` @@ -38,7 +38,7 @@ Steps: 3. Create the user `nginx-user`: - ```console + ```shell curl -sS -k -X POST -d '{ "username": "nginx-user", "enabled": true, "credentials":[{"type": "password", "value": "test", "temporary": false}]}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/admin/realms/master/users ``` @@ -46,19 +46,19 @@ Steps: 1. If you are not using PKCE, use the following command to create an OIDC client that does not use PKCE: - ```console + ```shell SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` ``` If everything went well, you should have the secret stored in $SECRET. To double-check, run: - ```console + ```shell echo $SECRET ``` 2. Use the following command to create an OIDC client that uses PKCE: - ```console + ```shell curl -sS -k -H "Content-Type: application/json" -H "Authorization: Bearer ${TOKEN}" \ --data '{ "clientId": "nginx-plus", From 46286b0b3f2e255c6240be34361c6775992e598a Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 29 May 2025 15:24:43 +0100 Subject: [PATCH 34/41] Turn choice into unordered list --- examples/custom-resources/oidc/keycloak_setup.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/examples/custom-resources/oidc/keycloak_setup.md b/examples/custom-resources/oidc/keycloak_setup.md index d46e140ba1..278e0fe32c 100644 --- a/examples/custom-resources/oidc/keycloak_setup.md +++ b/examples/custom-resources/oidc/keycloak_setup.md @@ -44,7 +44,7 @@ Steps: 4. Create the client `nginx-plus`: - 1. If you are not using PKCE, use the following command to create an OIDC client that does not use PKCE: + - If you are not using PKCE, use the following command to create an OIDC client that does not use PKCE: ```shell SECRET=`curl -sS -k -X POST -d '{ "clientId": "nginx-plus", "redirectUris": ["https://webapp.example.com:443/_codexch"], "attributes": {"post.logout.redirect.uris": "https://webapp.example.com:443/*"}}' -H "Content-Type:application/json" -H "Authorization: bearer ${TOKEN}" https://${KEYCLOAK_ADDRESS}/realms/master/clients-registrations/default | jq -r .secret` @@ -56,7 +56,7 @@ Steps: echo $SECRET ``` - 2. Use the following command to create an OIDC client that uses PKCE: + - Or if you are using PKCE with OIDC, use the following command to create the client: ```shell curl -sS -k -H "Content-Type: application/json" -H "Authorization: Bearer ${TOKEN}" \ From 690344881cc913643217133a92fabf65ecb92eca Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 2 Jun 2025 10:46:22 +0100 Subject: [PATCH 35/41] Replace const default pkce secret with init val --- internal/configs/virtualserver.go | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index fc18441095..97d58ed56c 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -2,7 +2,9 @@ package configs import ( "context" + "crypto/rand" "crypto/sha256" + "encoding/base64" "encoding/hex" "fmt" "net/url" @@ -34,7 +36,6 @@ const ( splitClientsKeyValZoneSize = "100k" splitClientAmountWhenWeightChangesDynamicReload = 101 defaultLogOutput = "syslog:server=localhost:514" - pkceClientValue = "cGtjZS1oYXMtbm8tc2VjcmV0" // "pkce-has-no-secret" base64 encoded ) var grpcConflictingErrors = map[int]bool{ @@ -68,6 +69,16 @@ var incompatibleLBMethodsForSlowStart = map[string]bool{ "random two least_time=last_byte": true, } +// pkceDefaultValue is a placeholder base64 encoded random string of +// bytes that will be used as a secret for OIDC when PKCE is used. If +// PKCE is not used, this will be overridden. +// +// Technically, we could use a constant here, but code scanning tools +// flag that up as an issue. +// +// The initial value of this var is declared in the init() function. +var pkceDefaultValue []byte + // MeshPodOwner contains the type and name of the K8s resource that owns the pod. // This owner information is needed for NGINX Service Mesh metrics. type MeshPodOwner struct { @@ -106,6 +117,17 @@ type VirtualServerEx struct { ZoneSync bool } +// init function with the sole purpose of generating a random set of +// bytes and base64 encoding that and saving it to the default var. +func init() { + randomBytes := make([]byte, 32) // Generate 32 random bytes + _, err := rand.Read(randomBytes) + if err != nil { + os.Exit(1) + } + pkceDefaultValue = []byte(base64.StdEncoding.EncodeToString(randomBytes)) +} + func (vsx *VirtualServerEx) String() string { if vsx == nil { return "" @@ -1373,7 +1395,7 @@ func (p *policiesCfg) addOIDCConfig( } else { secretKey := fmt.Sprintf("%v/%v", polNamespace, oidc.ClientSecret) secretRef := secretRefs[secretKey] - clientSecret := []byte(pkceClientValue) + clientSecret := pkceDefaultValue var secretType api_v1.SecretType if secretRef.Secret != nil { From 873b0fc79fb7e6bdb4a5a163d3bcf2b41ba3bd84 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Thu, 5 Jun 2025 19:13:23 +0100 Subject: [PATCH 36/41] Change pkce and client secret validations --- internal/configs/virtualserver.go | 34 ++++++++++----------- internal/k8s/controller.go | 4 +++ pkg/apis/configuration/validation/policy.go | 26 ++++++++++++++-- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 97d58ed56c..fe4526b703 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1394,25 +1394,25 @@ func (p *policiesCfg) addOIDCConfig( } } else { secretKey := fmt.Sprintf("%v/%v", polNamespace, oidc.ClientSecret) - secretRef := secretRefs[secretKey] - clientSecret := pkceDefaultValue + secretRef, ok := secretRefs[secretKey] + clientSecret := []byte("") - var secretType api_v1.SecretType - if secretRef.Secret != nil { - secretType = secretRef.Secret.Type - } - if secretType != "" && secretType != secrets.SecretTypeOIDC { - res.addWarningf("OIDC policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeOIDC) - res.isError = true - return res - } else if secretRef.Error != nil && !oidc.PKCEEnable { - res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) - res.isError = true - return res - } + if ok { + clientSecret = pkceDefaultValue - if !oidc.PKCEEnable { - clientSecret = secretRef.Secret.Data[ClientSecretKey] + var secretType api_v1.SecretType + if secretRef.Secret != nil { + secretType = secretRef.Secret.Type + } + if secretType != "" && secretType != secrets.SecretTypeOIDC { + res.addWarningf("OIDC policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeOIDC) + res.isError = true + return res + } else if secretRef.Error != nil && !oidc.PKCEEnable { + res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) + res.isError = true + return res + } } redirectURI := oidc.RedirectURI diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 3435e07de1..dcdca18b94 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -2832,6 +2832,10 @@ func (lbc *LoadBalancerController) addOIDCSecretRefs(secretRefs map[string]*secr continue } + if pol.Spec.OIDC.PKCEEnable { + continue + } + secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.OIDC.ClientSecret) secretRef := lbc.secretStore.GetSecret(secretKey) diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index c687a43938..0e48dc2c84 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -265,15 +265,15 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { if oidc.ClientID == "" { return field.ErrorList{field.Required(fieldPath.Child("clientID"), "")} } - if oidc.ClientSecret == "" { - return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")} - } if oidc.EndSessionEndpoint == "" && oidc.PostLogoutRedirectURI != "" { msg := "postLogoutRedirectURI can only be set when endSessionEndpoint is set" return field.ErrorList{field.Forbidden(fieldPath.Child("postLogoutRedirectURI"), msg)} } allErrs := field.ErrorList{} + + allErrs = append(allErrs, validatePKCE(oidc.PKCEEnable, oidc.ClientSecret, fieldPath.Child("clientSecret"))...) + if oidc.Scope != "" { allErrs = append(allErrs, validateOIDCScope(oidc.Scope, fieldPath.Child("scope"))...) } @@ -470,6 +470,26 @@ func validateOIDCScope(scope string, fieldPath *field.Path) field.ErrorList { return nil } +// validatePKCE checks for the duo of PKCEEnable and clientSecret settings. +// +// - yes PKCE and not empty client secret is bad, because PKCE does not use +// client secret +// - no PKCE and empty client secret is bad because standard OIDC uses client +// secrets +func validatePKCE(PKCEEnable bool, clientSecret string, + clientSecretPath *field.Path, +) field.ErrorList { + if !PKCEEnable && clientSecret == "" { + return field.ErrorList{field.Required(clientSecretPath, "clientSecret is required when PKCE is not used")} + } + + if PKCEEnable && clientSecret != "" { + return field.ErrorList{field.Forbidden(clientSecretPath, "clientSecret cannot be used when PKCE is used")} + } + + return nil +} + func validateURL(name string, fieldPath *field.Path) field.ErrorList { u, err := url.Parse(name) if err != nil { From 9f7c90f90170d62a4fbb21613b4155210b1ee46a Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 9 Jun 2025 09:32:55 +0100 Subject: [PATCH 37/41] Update snapshots --- charts/tests/__snapshots__/helmunit_test.snap | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/charts/tests/__snapshots__/helmunit_test.snap b/charts/tests/__snapshots__/helmunit_test.snap index 3ebe121d45..78d45f7e8c 100755 --- a/charts/tests/__snapshots__/helmunit_test.snap +++ b/charts/tests/__snapshots__/helmunit_test.snap @@ -442,8 +442,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -911,8 +909,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -1449,8 +1445,6 @@ spec: - -weight-changes-dynamic-reload=false - -agent=true - -agent-instance-group=app-protect-waf-agentv2-nginx-ingress-controller - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -1965,7 +1959,6 @@ spec: mountPath: /opt/app_protect/config - name: app-protect-bundles mountPath: /etc/app_protect/bundles - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -2547,7 +2540,6 @@ spec: mountPath: /opt/app_protect/config - name: app-protect-bundles mountPath: /etc/app_protect/bundles - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -2961,8 +2953,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -3406,8 +3396,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -3851,8 +3839,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -4297,8 +4283,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -4763,8 +4747,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -5210,8 +5192,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -5672,8 +5652,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -6141,8 +6119,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -6620,8 +6596,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -7080,8 +7054,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -7540,8 +7512,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 @@ -8010,8 +7980,6 @@ spec: - -ssl-dynamic-reload=true - -enable-telemetry-reporting=true - -weight-changes-dynamic-reload=false - - minReadySeconds: 0 /-/-/-/ # Source: nginx-ingress/templates/controller-ingress-class.yaml apiVersion: networking.k8s.io/v1 From e4956a11bff8efa28fbab40402f6e424b88edb36 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 9 Jun 2025 09:35:10 +0100 Subject: [PATCH 38/41] Do not use default client secret --- internal/configs/virtualserver.go | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index fe4526b703..44cec67078 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -2,9 +2,7 @@ package configs import ( "context" - "crypto/rand" "crypto/sha256" - "encoding/base64" "encoding/hex" "fmt" "net/url" @@ -69,16 +67,6 @@ var incompatibleLBMethodsForSlowStart = map[string]bool{ "random two least_time=last_byte": true, } -// pkceDefaultValue is a placeholder base64 encoded random string of -// bytes that will be used as a secret for OIDC when PKCE is used. If -// PKCE is not used, this will be overridden. -// -// Technically, we could use a constant here, but code scanning tools -// flag that up as an issue. -// -// The initial value of this var is declared in the init() function. -var pkceDefaultValue []byte - // MeshPodOwner contains the type and name of the K8s resource that owns the pod. // This owner information is needed for NGINX Service Mesh metrics. type MeshPodOwner struct { @@ -117,17 +105,6 @@ type VirtualServerEx struct { ZoneSync bool } -// init function with the sole purpose of generating a random set of -// bytes and base64 encoding that and saving it to the default var. -func init() { - randomBytes := make([]byte, 32) // Generate 32 random bytes - _, err := rand.Read(randomBytes) - if err != nil { - os.Exit(1) - } - pkceDefaultValue = []byte(base64.StdEncoding.EncodeToString(randomBytes)) -} - func (vsx *VirtualServerEx) String() string { if vsx == nil { return "" @@ -1398,8 +1375,6 @@ func (p *policiesCfg) addOIDCConfig( clientSecret := []byte("") if ok { - clientSecret = pkceDefaultValue - var secretType api_v1.SecretType if secretRef.Secret != nil { secretType = secretRef.Secret.Type From 4d4da9fdc2c3be74a4e02440cfb6e5c185cba671 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 9 Jun 2025 10:19:58 +0100 Subject: [PATCH 39/41] Add more validation and tests --- internal/configs/virtualserver.go | 10 +++ internal/configs/virtualserver_test.go | 95 ++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 44cec67078..c0bf3e5733 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1387,7 +1387,17 @@ func (p *policiesCfg) addOIDCConfig( res.addWarningf("OIDC policy %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true return res + } else if oidc.PKCEEnable { + res.addWarningf("OIDC policy %s has a secret and PKCE enabled. Secrets can't be used with PKCE", polKey) + res.isError = true + return res } + + clientSecret = secretRef.Secret.Data[ClientSecretKey] + } else if !oidc.PKCEEnable { + res.addWarningf("Client secret is required for OIDC policy %s when not using PKCE", polKey) + res.isError = true + return res } redirectURI := oidc.RedirectURI diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index f5a0696757..e183b55f15 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -11139,6 +11139,101 @@ func TestGeneratePoliciesFails(t *testing.T) { expectedOidc: &oidcPolicyCfg{}, msg: "multi waf", }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "oidc-policy", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/oidc-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "oidc-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + OIDC: &conf_v1.OIDC{ + ClientSecret: "oidc-secret", + AuthEndpoint: "https://foo.com/auth", + TokenEndpoint: "https://foo.com/token", + JWKSURI: "https://foo.com/certs", + EndSessionEndpoint: "https://foo.com/logout", + PostLogoutRedirectURI: "/_logout", + ClientID: "foo", + AccessTokenEnable: true, + PKCEEnable: true, + }, + }, + }, + }, + policyOpts: policyOptions{ + secretRefs: map[string]*secrets.SecretReference{ + "default/oidc-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeOIDC, + Data: map[string][]byte{ + "client-secret": []byte("super_secret_123"), + }, + }, + }, + }, + }, + context: "route", + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `OIDC policy default/oidc-policy has a secret and PKCE enabled. Secrets can't be used with PKCE`, + }, + }, + expectedOidc: &oidcPolicyCfg{}, + msg: "oidc pkce yes secret yes", + }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "oidc-policy", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/oidc-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "oidc-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + OIDC: &conf_v1.OIDC{ + AuthEndpoint: "https://foo.com/auth", + TokenEndpoint: "https://foo.com/token", + JWKSURI: "https://foo.com/certs", + EndSessionEndpoint: "https://foo.com/logout", + PostLogoutRedirectURI: "/_logout", + ClientID: "foo", + AccessTokenEnable: true, + PKCEEnable: false, + }, + }, + }, + }, + context: "route", + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `Client secret is required for OIDC policy default/oidc-policy when not using PKCE`, + }, + }, + expectedOidc: &oidcPolicyCfg{}, + msg: "oidc pkce no secret no", + }, } for _, test := range tests { From 6b22f2ac27545d3a7256a763708b400f3205de45 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 9 Jun 2025 10:28:46 +0100 Subject: [PATCH 40/41] Add note to OIDC policy docs about pkce-clientsecret --- site/content/configuration/policy-resource.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/content/configuration/policy-resource.md b/site/content/configuration/policy-resource.md index 470b126d1d..552edc2526 100644 --- a/site/content/configuration/policy-resource.md +++ b/site/content/configuration/policy-resource.md @@ -679,7 +679,7 @@ The OIDC policy defines a few internal locations that can't be customized: `/_jw |Field | Description | Type | Required | | ---| ---| ---| --- | |``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 | +|``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. If PKCE is enabled, this should be not configured. | ``string`` | Yes | |``authEndpoint`` | URL for the authorization endpoint provided by your OpenID Connect provider. | ``string`` | Yes | |``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 | From 515beb9fe085da52157db69754e5ec87aeb65f34 Mon Sep 17 00:00:00 2001 From: Gabor Javorszky Date: Mon, 9 Jun 2025 10:57:24 +0100 Subject: [PATCH 41/41] Remove clientsecret from e2e test for pkce --- tests/data/oidc/pkce.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/data/oidc/pkce.yaml b/tests/data/oidc/pkce.yaml index ed8c1f3ef9..92af0c1305 100644 --- a/tests/data/oidc/pkce.yaml +++ b/tests/data/oidc/pkce.yaml @@ -5,7 +5,6 @@ metadata: spec: oidc: clientID: nginx-plus-pkce - clientSecret: oidc-secret authEndpoint: https://keycloak.example.com/realms/master/protocol/openid-connect/auth tokenEndpoint: http://keycloak.default.svc.cluster.local:8080/realms/master/protocol/openid-connect/token jwksURI: http://keycloak.default.svc.cluster.local:8080/realms/master/protocol/openid-connect/certs