From a87bae636b3aab402c3a9da51e7bab8a3a54c612 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Wed, 29 Jan 2025 12:22:48 +0000 Subject: [PATCH 01/13] generate auth_jwt_claim_set directive Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- internal/configs/version2/http.go | 2 +- .../version2/nginx-plus.virtualserver.tmpl | 2 +- internal/configs/version2/templates_test.go | 2 +- internal/configs/virtualserver.go | 46 +++++++++---- internal/configs/virtualserver_test.go | 68 +++++++++++++++++++ pkg/apis/configuration/v1/types.go | 32 ++++++--- 6 files changed, 127 insertions(+), 25 deletions(-) diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 152706fba6..7d5e7f7e3e 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -32,7 +32,7 @@ type VirtualServerConfig struct { // AuthJwtClaimSet defines the values for the `auth_jwt_claim_set` directive type AuthJwtClaimSet struct { Variable string - Claims string + Claim string } // Upstream defines an upstream. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 4ac0967984..41c36b6249 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -51,7 +51,7 @@ split_clients {{ $sc.Source }} {{ $sc.Variable }} { {{- end }} {{- range $claim := .AuthJwtClaimSet }} -auth_jwt_claim_set {{ $claim.Variable }} {{ $claim.Claims}} +auth_jwt_claim_set {{ $claim.Variable }} {{ $claim.Claim}} {{- end }} {{- range $m := .Maps }} diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index 7dc0739a58..dc18064d8e 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -1577,7 +1577,7 @@ var ( AuthJwtClaimSet: []AuthJwtClaimSet{ { Variable: "$jwt_default_webapp_group_consumer_group_type", - Claims: "consumer_group type", + Claim: "consumer_group type", }, }, Maps: []Map{ diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 8535467b87..44f6e81f78 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -914,18 +914,19 @@ type apiKeyAuth struct { } type policiesCfg struct { - Allow []string - Deny []string - RateLimit rateLimit - JWTAuth jwtAuth - BasicAuth *version2.BasicAuth - IngressMTLS *version2.IngressMTLS - EgressMTLS *version2.EgressMTLS - OIDC bool - APIKey apiKeyAuth - WAF *version2.WAF - ErrorReturn *version2.Return - BundleValidator bundleValidator + Allow []string + Deny []string + RateLimit rateLimit + JWTAuth jwtAuth + AuthJwtClaimSets []*version2.AuthJwtClaimSet + BasicAuth *version2.BasicAuth + IngressMTLS *version2.IngressMTLS + EgressMTLS *version2.EgressMTLS + OIDC bool + APIKey apiKeyAuth + WAF *version2.WAF + ErrorReturn *version2.Return + BundleValidator bundleValidator } type bundleValidator interface { @@ -1011,6 +1012,10 @@ func (p *policiesCfg) addRateLimitConfig( rlZoneName := fmt.Sprintf("pol_rl_%v_%v_%v_%v", polNamespace, polName, vsNamespace, vsName) p.RateLimit.Reqs = append(p.RateLimit.Reqs, generateLimitReq(rlZoneName, rateLimit)) p.RateLimit.Zones = append(p.RateLimit.Zones, generateLimitReqZone(rlZoneName, rateLimit, podReplicas)) + if rateLimit.Condition != nil && rateLimit.Condition.JWT != nil { + p.AuthJwtClaimSets = append(p.AuthJwtClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) + } + //p.AuthJwtClaimSet = app if len(p.RateLimit.Reqs) == 1 { p.RateLimit.Options = generateLimitReqOptions(rateLimit) } else { @@ -1667,6 +1672,23 @@ func removeDuplicateLimitReqZones(rlz []version2.LimitReqZone) []version2.LimitR return result } +func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace string, vsName string) *version2.AuthJwtClaimSet { + return &version2.AuthJwtClaimSet{ + Variable: generateAuthJwtClaimSetVariable(jwtCondition.Claim, vsNamespace, vsName), + Claim: generateAuthJwtClaimSetClaim(jwtCondition.Claim), + } +} + +// TODO: process claim with spaces +func generateAuthJwtClaimSetVariable(claim string, vsNamespace string, vsName string) string { + return fmt.Sprintf("jwt_%v_%v_%v", vsNamespace, vsName, strings.Join(strings.Split(claim, "."), "_")) +} + +// TODO: process claim with spaces +func generateAuthJwtClaimSetClaim(claim string) string { + return strings.Join(strings.Split(claim, "."), " ") +} + func addPoliciesCfgToLocation(cfg policiesCfg, location *version2.Location) { location.Allow = cfg.Allow location.Deny = cfg.Deny diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 93af610053..058572ea30 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -9322,6 +9322,74 @@ func TestGenerateString(t *testing.T) { } } +func TestGenerateAuthJwtClaimSetVariable(t *testing.T) { + t.Parallel() + tests := []struct { + claim string + vsNamespace string + vsName string + expected string + }{ + { + claim: "consumer_group.type", + vsNamespace: "default", + vsName: "webapp", + expected: "jwt_default_webapp_consumer_group_type", + }, + { + claim: "type", + vsNamespace: "default", + vsName: "webapp", + expected: "jwt_default_webapp_type", + }, + { + claim: "a.b.c", + vsNamespace: "default", + vsName: "webapp", + expected: "jwt_default_webapp_a_b_c", + }, + } + + for _, test := range tests { + result := generateAuthJwtClaimSetVariable(test.claim, test.vsNamespace, test.vsName) + if result != test.expected { + t.Errorf("generateAuthJwtClaimSetVariable() return %v but expected %v", result, test.expected) + } + } +} + +func TestGenerateAuthJwtClaimSetClaim(t *testing.T) { + t.Parallel() + tests := []struct { + claim string + expected string + }{ + { + claim: "consumer_group.type", + expected: "consumer_group type", + }, + { + claim: "consumer_group.type", + expected: "consumer_group type", + }, + { + claim: "type", + expected: "type", + }, + { + claim: "a.b.c", + expected: "a b c", + }, + } + + for _, test := range tests { + result := generateAuthJwtClaimSetClaim(test.claim) + if result != test.expected { + t.Errorf("generateAuthJwtClaimSetClaim() return %v but expected %v", result, test.expected) + } + } +} + func TestGenerateSnippets(t *testing.T) { t.Parallel() tests := []struct { diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index cac87569ab..9829dd7407 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -600,16 +600,28 @@ type AccessControl struct { // RateLimit defines a rate limit policy. type RateLimit struct { - Rate string `json:"rate"` - Key string `json:"key"` - Delay *int `json:"delay"` - NoDelay *bool `json:"noDelay"` - Burst *int `json:"burst"` - ZoneSize string `json:"zoneSize"` - DryRun *bool `json:"dryRun"` - LogLevel string `json:"logLevel"` - RejectCode *int `json:"rejectCode"` - Scale bool `json:"scale"` + Rate string `json:"rate"` + Key string `json:"key"` + Delay *int `json:"delay"` + NoDelay *bool `json:"noDelay"` + Burst *int `json:"burst"` + ZoneSize string `json:"zoneSize"` + DryRun *bool `json:"dryRun"` + LogLevel string `json:"logLevel"` + RejectCode *int `json:"rejectCode"` + Scale bool `json:"scale"` + Condition *RateLimitCondition `json:"condition"` +} + +// TODO: add valition at CRD level +type RateLimitCondition struct { + JWT *JWTCondition `json:"jwt"` + Default bool `json:"default"` +} + +type JWTCondition struct { + Claim string `json:"claim"` + Match string `json:"match"` } // JWTAuth holds JWT authentication configuration. From 029e02ea2e16a743c7539b547f11cfdbf3a62e25 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Wed, 29 Jan 2025 17:43:53 +0000 Subject: [PATCH 02/13] add auth_jwt_claim_set to spec, route, subroute Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- internal/configs/version2/http.go | 6 +-- .../version2/nginx-plus.virtualserver.tmpl | 2 +- internal/configs/version2/templates_test.go | 2 +- internal/configs/virtualserver.go | 43 ++++++++++++++----- internal/configs/virtualserver_test.go | 1 + 5 files changed, 38 insertions(+), 16 deletions(-) diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 7d5e7f7e3e..4b6a80b13f 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -18,7 +18,7 @@ type VirtualServerConfig struct { KeyVals []KeyVal LimitReqZones []LimitReqZone Maps []Map - AuthJwtClaimSet []AuthJwtClaimSet + AuthJWTClaimSets []AuthJWTClaimSet Server Server SpiffeCerts bool SpiffeClientCerts bool @@ -29,8 +29,8 @@ type VirtualServerConfig struct { StaticSSLPath string } -// AuthJwtClaimSet defines the values for the `auth_jwt_claim_set` directive -type AuthJwtClaimSet struct { +// AuthJWTClaimSet defines the values for the `auth_jwt_claim_set` directive +type AuthJWTClaimSet struct { Variable string Claim string } diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 41c36b6249..a57e96ffdf 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -50,7 +50,7 @@ split_clients {{ $sc.Source }} {{ $sc.Variable }} { } {{- end }} -{{- range $claim := .AuthJwtClaimSet }} +{{- range $claim := .AuthJWTClaimSets }} auth_jwt_claim_set {{ $claim.Variable }} {{ $claim.Claim}} {{- end }} diff --git a/internal/configs/version2/templates_test.go b/internal/configs/version2/templates_test.go index dc18064d8e..5aa546fc76 100644 --- a/internal/configs/version2/templates_test.go +++ b/internal/configs/version2/templates_test.go @@ -1574,7 +1574,7 @@ var ( }, }, Upstreams: []Upstream{}, - AuthJwtClaimSet: []AuthJwtClaimSet{ + AuthJWTClaimSets: []AuthJWTClaimSet{ { Variable: "$jwt_default_webapp_group_consumer_group_type", Claim: "consumer_group type", diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 44f6e81f78..db83227d70 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -453,9 +453,12 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( var statusMatches []version2.StatusMatch var healthChecks []version2.HealthCheck var limitReqZones []version2.LimitReqZone + var authJWTClaimSets []*version2.AuthJWTClaimSet limitReqZones = append(limitReqZones, policiesCfg.RateLimit.Zones...) + authJWTClaimSets = append(authJWTClaimSets, policiesCfg.AuthJWTClaimSets...) + // generate upstreams for VirtualServer for _, u := range vsEx.VirtualServer.Spec.Upstreams { @@ -606,6 +609,8 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } limitReqZones = append(limitReqZones, routePoliciesCfg.RateLimit.Zones...) + authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.AuthJWTClaimSets...) + dosRouteCfg := generateDosCfg(dosResources[r.Path]) if len(r.Matches) > 0 { @@ -747,6 +752,8 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( limitReqZones = append(limitReqZones, routePoliciesCfg.RateLimit.Zones...) + authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.AuthJWTClaimSets...) + dosRouteCfg := generateDosCfg(dosResources[r.Path]) if len(r.Matches) > 0 { @@ -828,12 +835,13 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( }) vsCfg := version2.VirtualServerConfig{ - Upstreams: upstreams, - SplitClients: splitClients, - Maps: maps, - StatusMatches: statusMatches, - LimitReqZones: removeDuplicateLimitReqZones(limitReqZones), - HTTPSnippets: httpSnippets, + Upstreams: upstreams, + SplitClients: splitClients, + Maps: maps, + StatusMatches: statusMatches, + LimitReqZones: removeDuplicateLimitReqZones(limitReqZones), + AuthJWTClaimSets: removeDuplicateAuthJWTClaimSets(authJWTClaimSets), + HTTPSnippets: httpSnippets, Server: version2.Server{ ServerName: vsEx.VirtualServer.Spec.Host, Gunzip: vsEx.VirtualServer.Spec.Gunzip, @@ -918,7 +926,7 @@ type policiesCfg struct { Deny []string RateLimit rateLimit JWTAuth jwtAuth - AuthJwtClaimSets []*version2.AuthJwtClaimSet + AuthJWTClaimSets []*version2.AuthJWTClaimSet BasicAuth *version2.BasicAuth IngressMTLS *version2.IngressMTLS EgressMTLS *version2.EgressMTLS @@ -1013,9 +1021,8 @@ func (p *policiesCfg) addRateLimitConfig( p.RateLimit.Reqs = append(p.RateLimit.Reqs, generateLimitReq(rlZoneName, rateLimit)) p.RateLimit.Zones = append(p.RateLimit.Zones, generateLimitReqZone(rlZoneName, rateLimit, podReplicas)) if rateLimit.Condition != nil && rateLimit.Condition.JWT != nil { - p.AuthJwtClaimSets = append(p.AuthJwtClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) + p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) } - //p.AuthJwtClaimSet = app if len(p.RateLimit.Reqs) == 1 { p.RateLimit.Options = generateLimitReqOptions(rateLimit) } else { @@ -1672,8 +1679,22 @@ func removeDuplicateLimitReqZones(rlz []version2.LimitReqZone) []version2.LimitR return result } -func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace string, vsName string) *version2.AuthJwtClaimSet { - return &version2.AuthJwtClaimSet{ +func removeDuplicateAuthJWTClaimSets(ajcs []*version2.AuthJWTClaimSet) []version2.AuthJWTClaimSet { + encountered := make(map[string]bool) + var result []version2.AuthJWTClaimSet + + for _, v := range ajcs { + if !encountered[v.Variable] { + encountered[v.Variable] = true + result = append(result, *v) + } + } + + return result +} + +func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace string, vsName string) *version2.AuthJWTClaimSet { + return &version2.AuthJWTClaimSet{ Variable: generateAuthJwtClaimSetVariable(jwtCondition.Claim, vsNamespace, vsName), Claim: generateAuthJwtClaimSetClaim(jwtCondition.Claim), } diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 058572ea30..53e1fbb110 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -8919,6 +8919,7 @@ func TestRemoveDuplicates(t *testing.T) { } } +// TODO: write test for removeDuplicateAuthJWTClaimSets func TestAddPoliciesCfgToLocations(t *testing.T) { t.Parallel() cfg := policiesCfg{ From 81e6368ad83564e86df26421361bb3dca204cf43 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 31 Jan 2025 11:54:33 +0000 Subject: [PATCH 03/13] add test, fix template, remove pointers, generate crd Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- config/crd/bases/k8s.nginx.org_policies.yaml | 12 + deploy/crds.yaml | 12 + .../__snapshots__/templates_test.snap | 2 +- .../version2/nginx-plus.virtualserver.tmpl | 2 +- internal/configs/virtualserver.go | 20 +- internal/configs/virtualserver_test.go | 301 +++++++++++++++++- pkg/apis/configuration/v1/types.go | 4 +- 7 files changed, 335 insertions(+), 18 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 7bf119c71b..2bb1348bbf 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -179,6 +179,18 @@ spec: properties: burst: type: integer + condition: + properties: + default: + type: boolean + jwt: + properties: + claim: + type: string + match: + type: string + type: object + type: object delay: type: integer dryRun: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index c6601ee07f..91f87821f5 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -341,6 +341,18 @@ spec: properties: burst: type: integer + condition: + properties: + default: + type: boolean + jwt: + properties: + claim: + type: string + match: + type: string + type: object + type: object delay: type: integer dryRun: diff --git a/internal/configs/version2/__snapshots__/templates_test.snap b/internal/configs/version2/__snapshots__/templates_test.snap index e3aef875f5..2dfe09753b 100644 --- a/internal/configs/version2/__snapshots__/templates_test.snap +++ b/internal/configs/version2/__snapshots__/templates_test.snap @@ -2263,7 +2263,7 @@ server { [TestExecuteVirtualServerTemplate_RendersTemplateWithRateLimitJWTClaim - 1] -auth_jwt_claim_set $jwt_default_webapp_group_consumer_group_type consumer_group type +auth_jwt_claim_set $jwt_default_webapp_group_consumer_group_type consumer_group type; map $jwt_default_webapp_group_consumer_group_type $rate_limit_default_webapp_group_consumer_group_type { default Group3; Gold Group1; diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index a57e96ffdf..49bd62712f 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -51,7 +51,7 @@ split_clients {{ $sc.Source }} {{ $sc.Variable }} { {{- end }} {{- range $claim := .AuthJWTClaimSets }} -auth_jwt_claim_set {{ $claim.Variable }} {{ $claim.Claim}} +auth_jwt_claim_set {{ $claim.Variable }} {{ $claim.Claim}}; {{- end }} {{- range $m := .Maps }} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index db83227d70..52344a3084 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -453,7 +453,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( var statusMatches []version2.StatusMatch var healthChecks []version2.HealthCheck var limitReqZones []version2.LimitReqZone - var authJWTClaimSets []*version2.AuthJWTClaimSet + var authJWTClaimSets []version2.AuthJWTClaimSet limitReqZones = append(limitReqZones, policiesCfg.RateLimit.Zones...) @@ -695,7 +695,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } locSnippets := r.LocationSnippets - // use the VirtualServer location snippet if the route does not define any + // use the VirtualServer location snippet if the route does not define any if r.LocationSnippets == "" { locSnippets = vsrLocationSnippetsFromVs[vsrNamespaceName] } @@ -926,7 +926,7 @@ type policiesCfg struct { Deny []string RateLimit rateLimit JWTAuth jwtAuth - AuthJWTClaimSets []*version2.AuthJWTClaimSet + AuthJWTClaimSets []version2.AuthJWTClaimSet BasicAuth *version2.BasicAuth IngressMTLS *version2.IngressMTLS EgressMTLS *version2.EgressMTLS @@ -1020,8 +1020,8 @@ func (p *policiesCfg) addRateLimitConfig( rlZoneName := fmt.Sprintf("pol_rl_%v_%v_%v_%v", polNamespace, polName, vsNamespace, vsName) p.RateLimit.Reqs = append(p.RateLimit.Reqs, generateLimitReq(rlZoneName, rateLimit)) p.RateLimit.Zones = append(p.RateLimit.Zones, generateLimitReqZone(rlZoneName, rateLimit, podReplicas)) - if rateLimit.Condition != nil && rateLimit.Condition.JWT != nil { - p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) + if rateLimit.Condition != nil && rateLimit.Condition.JWT.Claim != "" && rateLimit.Condition.JWT.Match != "" { + p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(rateLimit.Condition.JWT, vsNamespace, vsName)) } if len(p.RateLimit.Reqs) == 1 { p.RateLimit.Options = generateLimitReqOptions(rateLimit) @@ -1679,22 +1679,22 @@ func removeDuplicateLimitReqZones(rlz []version2.LimitReqZone) []version2.LimitR return result } -func removeDuplicateAuthJWTClaimSets(ajcs []*version2.AuthJWTClaimSet) []version2.AuthJWTClaimSet { +func removeDuplicateAuthJWTClaimSets(ajcs []version2.AuthJWTClaimSet) []version2.AuthJWTClaimSet { encountered := make(map[string]bool) var result []version2.AuthJWTClaimSet for _, v := range ajcs { if !encountered[v.Variable] { encountered[v.Variable] = true - result = append(result, *v) + result = append(result, v) } } return result } -func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace string, vsName string) *version2.AuthJWTClaimSet { - return &version2.AuthJWTClaimSet{ +func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace string, vsName string) version2.AuthJWTClaimSet { + return version2.AuthJWTClaimSet{ Variable: generateAuthJwtClaimSetVariable(jwtCondition.Claim, vsNamespace, vsName), Claim: generateAuthJwtClaimSetClaim(jwtCondition.Claim), } @@ -1702,7 +1702,7 @@ func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace stri // TODO: process claim with spaces func generateAuthJwtClaimSetVariable(claim string, vsNamespace string, vsName string) string { - return fmt.Sprintf("jwt_%v_%v_%v", vsNamespace, vsName, strings.Join(strings.Split(claim, "."), "_")) + return fmt.Sprintf("$jwt_%v_%v_%v", vsNamespace, vsName, strings.Join(strings.Split(claim, "."), "_")) } // TODO: process claim with spaces diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 53e1fbb110..2271f5e8c7 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6395,6 +6395,233 @@ func TestGenerateVirtualServerConfigAPIKeyClientMaps(t *testing.T) { } } +func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { + t.Parallel() + + dryRunOverride := true + rejectCodeOverride := 505 + + virtualServerEx := VirtualServerEx{ + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe", + Namespace: "default", + }, + Spec: conf_v1.VirtualServerSpec{ + Host: "cafe.example.com", + Policies: []conf_v1.PolicyReference{ + { + Name: "gold-rate-limit-policy", + }, + { + Name: "silver-rate-limit-policy", + }, + }, + Upstreams: []conf_v1.Upstream{ + { + Name: "tea", + Service: "tea-svc", + Port: 80, + }, + { + Name: "coffee", + Service: "coffee-svc", + Port: 80, + }, + }, + Routes: []conf_v1.Route{ + { + Path: "/tea", + Action: &conf_v1.Action{ + Pass: "tea", + }, + }, + { + Path: "/coffee", + Action: &conf_v1.Action{ + Pass: "coffee", + }, + }, + }, + }, + }, + Policies: map[string]*conf_v1.Policy{ + "default/gold-rate-limit-policy": { + Spec: conf_v1.PolicySpec{ + RateLimit: &conf_v1.RateLimit{ + Key: "test", + ZoneSize: "10M", + Rate: "10r/s", + DryRun: &dryRunOverride, + RejectCode: &rejectCodeOverride, + Condition: &conf_v1.RateLimitCondition{ + JWT: conf_v1.JWTCondition{ + Claim: "user_type.tier", + Match: "gold", + }, + }, + }, + }, + }, + "default/silver-rate-limit-policy": { + Spec: conf_v1.PolicySpec{ + RateLimit: &conf_v1.RateLimit{ + Key: "test", + ZoneSize: "20M", + Rate: "20r/s", + DryRun: &dryRunOverride, + //LogLevel: "info", + RejectCode: &rejectCodeOverride, + Condition: &conf_v1.RateLimitCondition{ + JWT: conf_v1.JWTCondition{ + Claim: "user_type.tier", + Match: "silver", + }, + }, + }, + }, + }, + }, + Endpoints: map[string][]string{ + "default/tea-svc:80": { + "10.0.0.20:80", + }, + "default/coffee-svc:80": { + "10.0.0.30:80", + }, + }, + } + + expected := version2.VirtualServerConfig{ + Maps: nil, + AuthJWTClaimSets: []version2.AuthJWTClaimSet{{Variable: "$jwt_default_cafe_user_type_tier", Claim: "user_type tier"}}, + + Upstreams: []version2.Upstream{ + { + UpstreamLabels: version2.UpstreamLabels{ + Service: "coffee-svc", + ResourceType: "virtualserver", + ResourceName: "cafe", + ResourceNamespace: "default", + }, + Name: "vs_default_cafe_coffee", + Servers: []version2.UpstreamServer{ + { + Address: "10.0.0.30:80", + }, + }, + Keepalive: 16, + }, + { + UpstreamLabels: version2.UpstreamLabels{ + Service: "tea-svc", + ResourceType: "virtualserver", + ResourceName: "cafe", + ResourceNamespace: "default", + }, + Name: "vs_default_cafe_tea", + Servers: []version2.UpstreamServer{ + { + Address: "10.0.0.20:80", + }, + }, + Keepalive: 16, + }, + }, + HTTPSnippets: []string{}, + LimitReqZones: []version2.LimitReqZone{ + {"test", "pol_rl_default_gold-rate-limit-policy_default_cafe", "10M", "10r/s"}, + {"test", "pol_rl_default_silver-rate-limit-policy_default_cafe", "20M", "20r/s"}, + }, + Server: version2.Server{ + JWTAuthList: nil, + JWTAuth: nil, + JWKSAuthEnabled: false, + ServerName: "cafe.example.com", + StatusZone: "cafe.example.com", + ProxyProtocol: true, + ServerTokens: "off", + RealIPHeader: "X-Real-IP", + SetRealIPFrom: []string{"0.0.0.0/0"}, + RealIPRecursive: true, + Snippets: []string{"# server snippet"}, + TLSPassthrough: true, + VSNamespace: "default", + VSName: "cafe", + APIKeyEnabled: false, + LimitReqs: []version2.LimitReq{ + {"pol_rl_default_gold-rate-limit-policy_default_cafe", 0, false, 0}, + {"pol_rl_default_silver-rate-limit-policy_default_cafe", 0, false, 0}, + }, + LimitReqOptions: version2.LimitReqOptions{ + DryRun: true, + LogLevel: "error", + RejectCode: 505, + }, + Locations: []version2.Location{ + { + Path: "/tea", + ProxyPass: "http://vs_default_cafe_tea", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, + HasKeepalive: true, + ProxySSLName: "tea-svc.default.svc", + ProxyPassRequestHeaders: true, + ProxySetHeaders: []version2.Header{{Name: "Host", Value: "$host"}}, + ServiceName: "tea-svc", + }, + { + Path: "/coffee", + ProxyPass: "http://vs_default_cafe_coffee", + ProxyNextUpstream: "error timeout", + ProxyNextUpstreamTimeout: "0s", + ProxyNextUpstreamTries: 0, + HasKeepalive: true, + ProxySSLName: "coffee-svc.default.svc", + ProxyPassRequestHeaders: true, + ProxySetHeaders: []version2.Header{{Name: "Host", Value: "$host"}}, + ServiceName: "coffee-svc", + }, + }, + }, + } + + baseCfgParams := ConfigParams{ + Context: context.Background(), + ServerTokens: "off", + Keepalive: 16, + ServerSnippets: []string{"# server snippet"}, + ProxyProtocol: true, + SetRealIPFrom: []string{"0.0.0.0/0"}, + RealIPHeader: "X-Real-IP", + RealIPRecursive: true, + } + + vsc := newVirtualServerConfigurator( + &baseCfgParams, + false, + false, + &StaticConfigParams{TLSPassthrough: true}, + false, + &fakeBV, + ) + + result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx, nil, nil) + + sort.Slice(result.Maps, func(i, j int) bool { + return result.Maps[i].Variable < result.Maps[j].Variable + }) + + if diff := cmp.Diff(expected, result); diff != "" { + t.Errorf("GenerateVirtualServerConfig() mismatch (-want +got):\n%s", diff) + } + + if len(warnings) != 0 { + t.Errorf("GenerateVirtualServerConfig returned warnings: %v", vsc.warnings) + } +} + func TestGeneratePolicies(t *testing.T) { t.Parallel() ownerDetails := policyOwnerDetails{ @@ -8877,7 +9104,7 @@ func TestGeneratePoliciesFails(t *testing.T) { } } -func TestRemoveDuplicates(t *testing.T) { +func TestRemoveDuplicateLimitReqZones(t *testing.T) { t.Parallel() tests := []struct { rlz []version2.LimitReqZone @@ -8920,6 +9147,72 @@ func TestRemoveDuplicates(t *testing.T) { } // TODO: write test for removeDuplicateAuthJWTClaimSets +func TestRemoveDuplicateAuthJWTClaimSets(t *testing.T) { + t.Parallel() + tests := []struct { + ajcs []version2.AuthJWTClaimSet + expected []version2.AuthJWTClaimSet + }{ + { + ajcs: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + }, + expected: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + }, + }, + { + ajcs: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + }, + expected: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + }, + }, + { + ajcs: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + { + Variable: "$jwt_default_webapp_user_group_type", + }, + }, + expected: []version2.AuthJWTClaimSet{ + { + Variable: "$jwt_default_webapp_consumer_group_type", + }, + { + Variable: "$jwt_default_webapp_user_group_type", + }, + }, + }, + } + for _, test := range tests { + result := removeDuplicateAuthJWTClaimSets(test.ajcs) + if !reflect.DeepEqual(result, test.expected) { + t.Errorf("removeDuplicateAuthJWTClaimSets() returned \n%v, but expected \n%v", result, test.expected) + } + } +} + func TestAddPoliciesCfgToLocations(t *testing.T) { t.Parallel() cfg := policiesCfg{ @@ -9335,19 +9628,19 @@ func TestGenerateAuthJwtClaimSetVariable(t *testing.T) { claim: "consumer_group.type", vsNamespace: "default", vsName: "webapp", - expected: "jwt_default_webapp_consumer_group_type", + expected: "$jwt_default_webapp_consumer_group_type", }, { claim: "type", vsNamespace: "default", vsName: "webapp", - expected: "jwt_default_webapp_type", + expected: "$jwt_default_webapp_type", }, { claim: "a.b.c", vsNamespace: "default", vsName: "webapp", - expected: "jwt_default_webapp_a_b_c", + expected: "$jwt_default_webapp_a_b_c", }, } diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 9829dd7407..1e528734f4 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -615,8 +615,8 @@ type RateLimit struct { // TODO: add valition at CRD level type RateLimitCondition struct { - JWT *JWTCondition `json:"jwt"` - Default bool `json:"default"` + JWT JWTCondition `json:"jwt"` + Default bool `json:"default"` } type JWTCondition struct { From 8af81368236767e9852105ab4761115351443d86 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 31 Jan 2025 12:36:37 +0000 Subject: [PATCH 04/13] rough validation regex's might need more work --- pkg/apis/configuration/v1/types.go | 37 +++++++++++++++++------------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 1e528734f4..fda4649647 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -600,27 +600,32 @@ type AccessControl struct { // RateLimit defines a rate limit policy. type RateLimit struct { - Rate string `json:"rate"` - Key string `json:"key"` - Delay *int `json:"delay"` - NoDelay *bool `json:"noDelay"` - Burst *int `json:"burst"` - ZoneSize string `json:"zoneSize"` - DryRun *bool `json:"dryRun"` - LogLevel string `json:"logLevel"` - RejectCode *int `json:"rejectCode"` - Scale bool `json:"scale"` - Condition *RateLimitCondition `json:"condition"` -} - -// TODO: add valition at CRD level + Rate string `json:"rate"` + Key string `json:"key"` + Delay *int `json:"delay"` + NoDelay *bool `json:"noDelay"` + Burst *int `json:"burst"` + ZoneSize string `json:"zoneSize"` + DryRun *bool `json:"dryRun"` + LogLevel string `json:"logLevel"` + RejectCode *int `json:"rejectCode"` + Scale bool `json:"scale"` + // +kubebuilder:validation:Optional + Condition *RateLimitCondition `json:"condition"` +} + type RateLimitCondition struct { - JWT JWTCondition `json:"jwt"` - Default bool `json:"default"` + JWT JWTCondition `json:"jwt"` + // +kubebuilder:validation:Optional + Default bool `json:"default"` } type JWTCondition struct { + // +kubebuilder:validation:Required + // +kubebuilder:validation:Pattern=^([^$\s"'])*$ Claim string `json:"claim"` + // +kubebuilder:validation:Required + // +kubebuilder:validation:Pattern=^([^$\s."'])*$ Match string `json:"match"` } From 45070894cba7472f74e70cccdf68e68127b8eac4 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 31 Jan 2025 12:38:56 +0000 Subject: [PATCH 05/13] update crds --- config/crd/bases/k8s.nginx.org_policies.yaml | 5 +++++ deploy/crds.yaml | 5 +++++ pkg/apis/configuration/v1/types.go | 4 ++-- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 2bb1348bbf..8e71ab5297 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -186,9 +186,14 @@ spec: jwt: properties: claim: + pattern: ^([^$\s"'])*$ type: string match: + pattern: ^([^$\s."'])*$ type: string + required: + - claim + - match type: object type: object delay: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 91f87821f5..b0fd47230b 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -348,9 +348,14 @@ spec: jwt: properties: claim: + pattern: ^([^$\s"'])*$ type: string match: + pattern: ^([^$\s."'])*$ type: string + required: + - claim + - match type: object type: object delay: diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index fda4649647..4e73b7b5c1 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -622,10 +622,10 @@ type RateLimitCondition struct { type JWTCondition struct { // +kubebuilder:validation:Required - // +kubebuilder:validation:Pattern=^([^$\s"'])*$ + // +kubebuilder:validation:Pattern=`^([^$\s"'])*$` Claim string `json:"claim"` // +kubebuilder:validation:Required - // +kubebuilder:validation:Pattern=^([^$\s."'])*$ + // +kubebuilder:validation:Pattern=`^([^$\s."'])*$` Match string `json:"match"` } From d6070e1a492836b9bbd521c71304d3f287baad71 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 31 Jan 2025 17:45:32 +0000 Subject: [PATCH 06/13] fix linting Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- internal/configs/virtualserver_test.go | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 2271f5e8c7..5ee91aa5c2 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6398,9 +6398,6 @@ func TestGenerateVirtualServerConfigAPIKeyClientMaps(t *testing.T) { func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { t.Parallel() - dryRunOverride := true - rejectCodeOverride := 505 - virtualServerEx := VirtualServerEx{ VirtualServer: &conf_v1.VirtualServer{ ObjectMeta: meta_v1.ObjectMeta{ @@ -6449,11 +6446,9 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { "default/gold-rate-limit-policy": { Spec: conf_v1.PolicySpec{ RateLimit: &conf_v1.RateLimit{ - Key: "test", - ZoneSize: "10M", - Rate: "10r/s", - DryRun: &dryRunOverride, - RejectCode: &rejectCodeOverride, + Key: "test", + ZoneSize: "10M", + Rate: "10r/s", Condition: &conf_v1.RateLimitCondition{ JWT: conf_v1.JWTCondition{ Claim: "user_type.tier", @@ -6469,9 +6464,6 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { Key: "test", ZoneSize: "20M", Rate: "20r/s", - DryRun: &dryRunOverride, - //LogLevel: "info", - RejectCode: &rejectCodeOverride, Condition: &conf_v1.RateLimitCondition{ JWT: conf_v1.JWTCondition{ Claim: "user_type.tier", @@ -6554,9 +6546,9 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { {"pol_rl_default_silver-rate-limit-policy_default_cafe", 0, false, 0}, }, LimitReqOptions: version2.LimitReqOptions{ - DryRun: true, + DryRun: false, LogLevel: "error", - RejectCode: 505, + RejectCode: 503, }, Locations: []version2.Location{ { From 2a20bc5e888d71390927967ecbb802b14fcadb50 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 31 Jan 2025 17:50:54 +0000 Subject: [PATCH 07/13] update codegen Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- config/crd/bases/k8s.nginx.org_policies.yaml | 2 + deploy/crds.yaml | 2 + internal/configs/virtualserver_test.go | 8 ++-- pkg/apis/configuration/v1/types.go | 3 ++ .../configuration/v1/zz_generated.deepcopy.go | 38 +++++++++++++++++++ 5 files changed, 49 insertions(+), 4 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 8e71ab5297..b3e83e0d7c 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -180,6 +180,8 @@ spec: burst: type: integer condition: + description: RateLimitCondition defines a condition for a rate + limit policy. properties: default: type: boolean diff --git a/deploy/crds.yaml b/deploy/crds.yaml index b0fd47230b..b32b954b2b 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -342,6 +342,8 @@ spec: burst: type: integer condition: + description: RateLimitCondition defines a condition for a rate + limit policy. properties: default: type: boolean diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 5ee91aa5c2..22c2be4238 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6522,8 +6522,8 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { }, HTTPSnippets: []string{}, LimitReqZones: []version2.LimitReqZone{ - {"test", "pol_rl_default_gold-rate-limit-policy_default_cafe", "10M", "10r/s"}, - {"test", "pol_rl_default_silver-rate-limit-policy_default_cafe", "20M", "20r/s"}, + {Key: "test", ZoneName: "pol_rl_default_gold-rate-limit-policy_default_cafe", ZoneSize: "10M", Rate: "10r/s"}, + {Key: "test", ZoneName: "pol_rl_default_silver-rate-limit-policy_default_cafe", ZoneSize: "20M", Rate: "20r/s"}, }, Server: version2.Server{ JWTAuthList: nil, @@ -6542,8 +6542,8 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { VSName: "cafe", APIKeyEnabled: false, LimitReqs: []version2.LimitReq{ - {"pol_rl_default_gold-rate-limit-policy_default_cafe", 0, false, 0}, - {"pol_rl_default_silver-rate-limit-policy_default_cafe", 0, false, 0}, + {ZoneName: "pol_rl_default_gold-rate-limit-policy_default_cafe", Burst: 0, NoDelay: false, Delay: 0}, + {ZoneName: "pol_rl_default_silver-rate-limit-policy_default_cafe", Burst: 0, NoDelay: false, Delay: 0}, }, LimitReqOptions: version2.LimitReqOptions{ DryRun: false, diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 4e73b7b5c1..6e21d1633e 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -614,12 +614,15 @@ type RateLimit struct { Condition *RateLimitCondition `json:"condition"` } +// RateLimitCondition defines a condition for a rate limit policy. type RateLimitCondition struct { JWT JWTCondition `json:"jwt"` // +kubebuilder:validation:Optional Default bool `json:"default"` } +// RateLimitCondition defines a condition for a rate limit by JWT claim. + type JWTCondition struct { // +kubebuilder:validation:Required // +kubebuilder:validation:Pattern=`^([^$\s"'])*$` diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index b617f3cb93..67f16d3ee5 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -515,6 +515,22 @@ func (in *JWTAuth) DeepCopy() *JWTAuth { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *JWTCondition) DeepCopyInto(out *JWTCondition) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTCondition. +func (in *JWTCondition) DeepCopy() *JWTCondition { + if in == nil { + return nil + } + out := new(JWTCondition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Listener) DeepCopyInto(out *Listener) { *out = *in @@ -870,6 +886,11 @@ func (in *RateLimit) DeepCopyInto(out *RateLimit) { *out = new(int) **out = **in } + if in.Condition != nil { + in, out := &in.Condition, &out.Condition + *out = new(RateLimitCondition) + **out = **in + } return } @@ -883,6 +904,23 @@ func (in *RateLimit) DeepCopy() *RateLimit { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RateLimitCondition) DeepCopyInto(out *RateLimitCondition) { + *out = *in + out.JWT = in.JWT + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RateLimitCondition. +func (in *RateLimitCondition) DeepCopy() *RateLimitCondition { + if in == nil { + return nil + } + out := new(RateLimitCondition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Route) DeepCopyInto(out *Route) { *out = *in From fd00573521838cfd73c5431bfcf482d014d3ce23 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 09:13:23 +0000 Subject: [PATCH 08/13] Correct JWTCondition comment --- pkg/apis/configuration/v1/types.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 6e21d1633e..f62107ff5b 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -621,8 +621,7 @@ type RateLimitCondition struct { Default bool `json:"default"` } -// RateLimitCondition defines a condition for a rate limit by JWT claim. - +// JWTCondition defines a condition for a rate limit by JWT claim. type JWTCondition struct { // +kubebuilder:validation:Required // +kubebuilder:validation:Pattern=`^([^$\s"'])*$` From 6a640f583b35e41862b56e009b59c9f4844e4f6d Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 10:24:00 +0000 Subject: [PATCH 09/13] make JWTCondition a pointer --- config/crd/bases/k8s.nginx.org_policies.yaml | 2 ++ deploy/crds.yaml | 2 ++ internal/configs/virtualserver.go | 2 +- internal/configs/virtualserver_test.go | 4 ++-- pkg/apis/configuration/v1/types.go | 2 +- pkg/apis/configuration/v1/zz_generated.deepcopy.go | 8 ++++++-- 6 files changed, 14 insertions(+), 6 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index b3e83e0d7c..65c164835c 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -186,6 +186,8 @@ spec: default: type: boolean jwt: + description: JWTCondition defines a condition for a rate limit + by JWT claim. properties: claim: pattern: ^([^$\s"'])*$ diff --git a/deploy/crds.yaml b/deploy/crds.yaml index b32b954b2b..982e3defdd 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -348,6 +348,8 @@ spec: default: type: boolean jwt: + description: JWTCondition defines a condition for a rate limit + by JWT claim. properties: claim: pattern: ^([^$\s"'])*$ diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 52344a3084..5af3d6dd3a 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1021,7 +1021,7 @@ func (p *policiesCfg) addRateLimitConfig( p.RateLimit.Reqs = append(p.RateLimit.Reqs, generateLimitReq(rlZoneName, rateLimit)) p.RateLimit.Zones = append(p.RateLimit.Zones, generateLimitReqZone(rlZoneName, rateLimit, podReplicas)) if rateLimit.Condition != nil && rateLimit.Condition.JWT.Claim != "" && rateLimit.Condition.JWT.Match != "" { - p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(rateLimit.Condition.JWT, vsNamespace, vsName)) + p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) } if len(p.RateLimit.Reqs) == 1 { p.RateLimit.Options = generateLimitReqOptions(rateLimit) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 22c2be4238..1ab187f1ce 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6450,7 +6450,7 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { ZoneSize: "10M", Rate: "10r/s", Condition: &conf_v1.RateLimitCondition{ - JWT: conf_v1.JWTCondition{ + JWT: &conf_v1.JWTCondition{ Claim: "user_type.tier", Match: "gold", }, @@ -6465,7 +6465,7 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { ZoneSize: "20M", Rate: "20r/s", Condition: &conf_v1.RateLimitCondition{ - JWT: conf_v1.JWTCondition{ + JWT: &conf_v1.JWTCondition{ Claim: "user_type.tier", Match: "silver", }, diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index f62107ff5b..9c577f8f1c 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -616,7 +616,7 @@ type RateLimit struct { // RateLimitCondition defines a condition for a rate limit policy. type RateLimitCondition struct { - JWT JWTCondition `json:"jwt"` + JWT *JWTCondition `json:"jwt"` // +kubebuilder:validation:Optional Default bool `json:"default"` } diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 67f16d3ee5..b7923f10dc 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -889,7 +889,7 @@ func (in *RateLimit) DeepCopyInto(out *RateLimit) { if in.Condition != nil { in, out := &in.Condition, &out.Condition *out = new(RateLimitCondition) - **out = **in + (*in).DeepCopyInto(*out) } return } @@ -907,7 +907,11 @@ func (in *RateLimit) DeepCopy() *RateLimit { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RateLimitCondition) DeepCopyInto(out *RateLimitCondition) { *out = *in - out.JWT = in.JWT + if in.JWT != nil { + in, out := &in.JWT, &out.JWT + *out = new(JWTCondition) + **out = **in + } return } From 8a62fb02c79e5297df6f8fc002674d765cd2e74c Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 10:24:34 +0000 Subject: [PATCH 10/13] add validation and tests for JWTCondition --- pkg/apis/configuration/validation/policy.go | 8 ++ .../configuration/validation/policy_test.go | 74 +++++++++++++++---- 2 files changed, 67 insertions(+), 15 deletions(-) diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 98d8626d08..903ee57793 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -151,6 +151,14 @@ func validateRateLimit(rateLimit *v1.RateLimit, fieldPath *field.Path, isPlus bo } } + if rateLimit.Condition != nil && rateLimit.Condition.JWT == nil { + allErrs = append(allErrs, field.Required(fieldPath.Child("jwt"), "jwt cannot be nil")) + } + + if rateLimit.Condition != nil && rateLimit.Condition.JWT != nil && !isPlus { + allErrs = append(allErrs, field.Forbidden(fieldPath.Child("condition.jwt"), "is only supported in NGINX Plus")) + } + return allErrs } diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go index 542bfaba24..0f3840e3d8 100644 --- a/pkg/apis/configuration/validation/policy_test.go +++ b/pkg/apis/configuration/validation/policy_test.go @@ -572,6 +572,7 @@ func TestValidateRateLimit_PassesOnValidInput(t *testing.T) { tests := []struct { rateLimit *v1.RateLimit + isPlus bool msg string }{ { @@ -580,7 +581,8 @@ func TestValidateRateLimit_PassesOnValidInput(t *testing.T) { ZoneSize: "10M", Key: "${request_uri}", }, - msg: "only required fields are set", + isPlus: false, + msg: "only required fields are set", }, { rateLimit: &v1.RateLimit{ @@ -594,14 +596,29 @@ func TestValidateRateLimit_PassesOnValidInput(t *testing.T) { LogLevel: "info", RejectCode: createPointerFromInt(505), }, - msg: "ratelimit all fields set", + isPlus: false, + msg: "ratelimit all fields set", + }, + { + rateLimit: &v1.RateLimit{ + Rate: "30r/m", + Key: "${request_uri}", + ZoneSize: "10M", + Condition: &v1.RateLimitCondition{ + JWT: &v1.JWTCondition{ + Claim: "sub", + Match: "Gold", + }, + Default: false, + }, + }, + isPlus: true, + msg: "ratelimit JWT Condition", }, } - isPlus := false - for _, test := range tests { - allErrs := validateRateLimit(test.rateLimit, field.NewPath("rateLimit"), isPlus) + allErrs := validateRateLimit(test.rateLimit, field.NewPath("rateLimit"), test.isPlus) if len(allErrs) > 0 { t.Errorf("validateRateLimit() returned errors %v for valid input for the case of %v", allErrs, test.msg) } @@ -622,56 +639,83 @@ func TestValidateRateLimit_FailsOnInvalidInput(t *testing.T) { t.Parallel() tests := []struct { rateLimit *v1.RateLimit + isPlus bool msg string }{ { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.Rate = "0r/s" }), - msg: "invalid rateLimit rate", + isPlus: false, + msg: "invalid rateLimit rate", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.Key = "${fail}" }), - msg: "invalid rateLimit key variable use", + isPlus: false, + msg: "invalid rateLimit key variable use", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.Delay = createPointerFromInt(0) }), - msg: "invalid rateLimit delay", + isPlus: false, + msg: "invalid rateLimit delay", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.Burst = createPointerFromInt(0) }), - msg: "invalid rateLimit burst", + isPlus: false, + msg: "invalid rateLimit burst", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.ZoneSize = "31k" }), - msg: "invalid rateLimit zoneSize", + isPlus: false, + msg: "invalid rateLimit zoneSize", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.RejectCode = createPointerFromInt(600) }), - msg: "invalid rateLimit rejectCode", + isPlus: false, + msg: "invalid rateLimit rejectCode", }, { rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { r.LogLevel = "invalid" }), - msg: "invalid rateLimit logLevel", + isPlus: false, + msg: "invalid rateLimit logLevel", + }, + { + rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { + r.Condition = &v1.RateLimitCondition{ + JWT: &v1.JWTCondition{ + Claim: "sub", + Match: "Gold", + }, + } + }), + isPlus: false, + msg: "must be plus", + }, + { + rateLimit: createInvalidRateLimit(func(r *v1.RateLimit) { + r.Condition = &v1.RateLimitCondition{ + Default: false, + } + }), + isPlus: true, + msg: "missing JWTCondition", }, } - isPlus := false - for _, test := range tests { - allErrs := validateRateLimit(test.rateLimit, field.NewPath("rateLimit"), isPlus) + allErrs := validateRateLimit(test.rateLimit, field.NewPath("rateLimit"), test.isPlus) if len(allErrs) == 0 { t.Errorf("validateRateLimit() returned no errors for invalid input for the case of %v", test.msg) } From 1c8bc0d8b8b5d8c362ed10a4cba0f310ce8d7d03 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 10:28:04 +0000 Subject: [PATCH 11/13] remove TODO --- internal/configs/virtualserver_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 1ab187f1ce..db32019c5d 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -9138,7 +9138,6 @@ func TestRemoveDuplicateLimitReqZones(t *testing.T) { } } -// TODO: write test for removeDuplicateAuthJWTClaimSets func TestRemoveDuplicateAuthJWTClaimSets(t *testing.T) { t.Parallel() tests := []struct { From 46958c1f2a151d2a04b35330ba67954459ece7e9 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Mon, 3 Feb 2025 11:34:32 +0000 Subject: [PATCH 12/13] move AuthJWTClaimSets to policyCfg.RateLimit --- internal/configs/virtualserver.go | 40 +++++++++++++------------- internal/configs/virtualserver_test.go | 2 -- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 5af3d6dd3a..985ae27cef 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -457,7 +457,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( limitReqZones = append(limitReqZones, policiesCfg.RateLimit.Zones...) - authJWTClaimSets = append(authJWTClaimSets, policiesCfg.AuthJWTClaimSets...) + authJWTClaimSets = append(authJWTClaimSets, policiesCfg.RateLimit.AuthJWTClaimSets...) // generate upstreams for VirtualServer for _, u := range vsEx.VirtualServer.Spec.Upstreams { @@ -609,7 +609,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } limitReqZones = append(limitReqZones, routePoliciesCfg.RateLimit.Zones...) - authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.AuthJWTClaimSets...) + authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.RateLimit.AuthJWTClaimSets...) dosRouteCfg := generateDosCfg(dosResources[r.Path]) @@ -752,7 +752,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( limitReqZones = append(limitReqZones, routePoliciesCfg.RateLimit.Zones...) - authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.AuthJWTClaimSets...) + authJWTClaimSets = append(authJWTClaimSets, routePoliciesCfg.RateLimit.AuthJWTClaimSets...) dosRouteCfg := generateDosCfg(dosResources[r.Path]) @@ -901,9 +901,10 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( // rateLimit hold the configuration for the ratelimiting Policy type rateLimit struct { - Reqs []version2.LimitReq - Zones []version2.LimitReqZone - Options version2.LimitReqOptions + Reqs []version2.LimitReq + Zones []version2.LimitReqZone + Options version2.LimitReqOptions + AuthJWTClaimSets []version2.AuthJWTClaimSet } // jwtAuth hold the configuration for the JWTAuth & JWKSAuth Policies @@ -922,19 +923,18 @@ type apiKeyAuth struct { } type policiesCfg struct { - Allow []string - Deny []string - RateLimit rateLimit - JWTAuth jwtAuth - AuthJWTClaimSets []version2.AuthJWTClaimSet - BasicAuth *version2.BasicAuth - IngressMTLS *version2.IngressMTLS - EgressMTLS *version2.EgressMTLS - OIDC bool - APIKey apiKeyAuth - WAF *version2.WAF - ErrorReturn *version2.Return - BundleValidator bundleValidator + Allow []string + Deny []string + RateLimit rateLimit + JWTAuth jwtAuth + BasicAuth *version2.BasicAuth + IngressMTLS *version2.IngressMTLS + EgressMTLS *version2.EgressMTLS + OIDC bool + APIKey apiKeyAuth + WAF *version2.WAF + ErrorReturn *version2.Return + BundleValidator bundleValidator } type bundleValidator interface { @@ -1021,7 +1021,7 @@ func (p *policiesCfg) addRateLimitConfig( p.RateLimit.Reqs = append(p.RateLimit.Reqs, generateLimitReq(rlZoneName, rateLimit)) p.RateLimit.Zones = append(p.RateLimit.Zones, generateLimitReqZone(rlZoneName, rateLimit, podReplicas)) if rateLimit.Condition != nil && rateLimit.Condition.JWT.Claim != "" && rateLimit.Condition.JWT.Match != "" { - p.AuthJWTClaimSets = append(p.AuthJWTClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) + p.RateLimit.AuthJWTClaimSets = append(p.RateLimit.AuthJWTClaimSets, generateAuthJwtClaimSet(*rateLimit.Condition.JWT, vsNamespace, vsName)) } if len(p.RateLimit.Reqs) == 1 { p.RateLimit.Options = generateLimitReqOptions(rateLimit) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index db32019c5d..45744e6434 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6483,11 +6483,9 @@ func TestGenerateVirtualServerConfigRateLimitPolicyAuthJwt(t *testing.T) { }, }, } - expected := version2.VirtualServerConfig{ Maps: nil, AuthJWTClaimSets: []version2.AuthJWTClaimSet{{Variable: "$jwt_default_cafe_user_type_tier", Claim: "user_type tier"}}, - Upstreams: []version2.Upstream{ { UpstreamLabels: version2.UpstreamLabels{ From 04fb2d2cabba22fd88fb495cb1cd4e9b19dd725a Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Tue, 4 Feb 2025 16:17:52 +0000 Subject: [PATCH 13/13] remove todo validation --- internal/configs/virtualserver.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 985ae27cef..a262fdd852 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1700,12 +1700,10 @@ func generateAuthJwtClaimSet(jwtCondition conf_v1.JWTCondition, vsNamespace stri } } -// TODO: process claim with spaces func generateAuthJwtClaimSetVariable(claim string, vsNamespace string, vsName string) string { return fmt.Sprintf("$jwt_%v_%v_%v", vsNamespace, vsName, strings.Join(strings.Split(claim, "."), "_")) } -// TODO: process claim with spaces func generateAuthJwtClaimSetClaim(claim string) string { return strings.Join(strings.Split(claim, "."), " ") }