From ba8f0c295298c10c53050b808835e8c7fbb36bb8 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Thu, 9 May 2024 11:03:15 +0100 Subject: [PATCH 01/36] always load njs module Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- internal/configs/version1/nginx-plus.tmpl | 2 -- internal/configs/version1/nginx.tmpl | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index e525a533c6..77a568f9f8 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -27,9 +27,7 @@ load_module modules/ngx_fips_check_module.so; {{$value}}{{end}} {{- end}} -{{- if .OIDC}} load_module modules/ngx_http_js_module.so; -{{- end}} events { worker_connections {{.WorkerConnections}}; diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index a3e34a94f9..e56b508af9 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -20,6 +20,8 @@ load_module modules/ngx_http_opentracing_module.so; {{$value}}{{end}} {{- end}} +load_module modules/ngx_http_js_module.so; + events { worker_connections {{.WorkerConnections}}; } From d046e57d37c3a08f078e0f3f34e2acb83dd97f66 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Mon, 20 May 2024 09:25:05 +0100 Subject: [PATCH 02/36] accept api key policy yaml 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 | 30 +++++++ deploy/crds.yaml | 30 +++++++ .../api-key/api-key-policy-2.yaml | 17 ++++ .../api-key/api-key-policy.yaml | 21 +++++ .../api-key/api-key-secret-1.yaml | 8 ++ .../api-key/api-key-secret-2.yaml | 8 ++ .../custom-resources/api-key/cafe-secret.yaml | 8 ++ .../api-key/cafe-virtual-server.yaml | 26 ++++++ examples/custom-resources/api-key/cafe.yaml | 65 ++++++++++++++ internal/configs/configurator.go | 10 +++ internal/configs/version2/http.go | 15 ++++ .../version2/nginx-plus.virtualserver.tmpl | 33 +++++++ internal/configs/virtualserver.go | 68 +++++++++++++++ internal/k8s/controller.go | 24 ++++++ internal/k8s/secrets/validation.go | 5 +- pkg/apis/configuration/v1/types.go | 22 +++++ .../configuration/v1/zz_generated.deepcopy.go | 86 +++++++++++++++++++ pkg/apis/configuration/validation/policy.go | 13 +++ 18 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 examples/custom-resources/api-key/api-key-policy-2.yaml create mode 100644 examples/custom-resources/api-key/api-key-policy.yaml create mode 100644 examples/custom-resources/api-key/api-key-secret-1.yaml create mode 100644 examples/custom-resources/api-key/api-key-secret-2.yaml create mode 100644 examples/custom-resources/api-key/cafe-secret.yaml create mode 100644 examples/custom-resources/api-key/cafe-virtual-server.yaml create mode 100644 examples/custom-resources/api-key/cafe.yaml diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index de6bef324b..90563ef69d 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -67,6 +67,36 @@ spec: type: string type: array type: object + apiKey: + properties: + clients: + items: + properties: + id: + type: string + secret: + type: string + type: object + type: array + rejectCodes: + properties: + noMatch: + type: integer + notSupplied: + type: integer + type: object + suppliedIn: + properties: + header: + items: + type: string + type: array + query: + items: + type: string + type: array + type: object + type: object basicAuth: description: |- BasicAuth holds HTTP Basic authentication configuration diff --git a/deploy/crds.yaml b/deploy/crds.yaml index aed959d337..4a54cea534 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -269,6 +269,36 @@ spec: type: string type: array type: object + apiKey: + properties: + clients: + items: + properties: + id: + type: string + secret: + type: string + type: object + type: array + rejectCodes: + properties: + noMatch: + type: integer + notSupplied: + type: integer + type: object + suppliedIn: + properties: + header: + items: + type: string + type: array + query: + items: + type: string + type: array + type: object + type: object basicAuth: description: |- BasicAuth holds HTTP Basic authentication configuration diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml new file mode 100644 index 0000000000..b2eb759348 --- /dev/null +++ b/examples/custom-resources/api-key/api-key-policy-2.yaml @@ -0,0 +1,17 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy-2 +spec: + apiKey: + suppliedIn: + header: + - "X-header-name" + query: + - "queryName" + rejectCodes: # optional + notSupplied: 408 + noMatch: 410 + clients: + - id: client2 + secret: api-key-client-secret-2 \ No newline at end of file diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml new file mode 100644 index 0000000000..9af347e922 --- /dev/null +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -0,0 +1,21 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy +spec: + apiKey: + suppliedIn: + header: + - "X-header-name" + - "apikey" + - "some-other-header" + query: + - "queryName" + rejectCodes: # optional + notSupplied: 401 + noMatch: 403 + clients: + - id: client1 + secret: api-key-client-secret-1 + - id: client2 + secret: api-key-client-secret-2 \ No newline at end of file diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml new file mode 100644 index 0000000000..998851888b --- /dev/null +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-1 +type: Opaque +data: + clientId: Y2xpZW50MQo= + key: cGFzc3dvcmQ= \ No newline at end of file diff --git a/examples/custom-resources/api-key/api-key-secret-2.yaml b/examples/custom-resources/api-key/api-key-secret-2.yaml new file mode 100644 index 0000000000..d997cd8352 --- /dev/null +++ b/examples/custom-resources/api-key/api-key-secret-2.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-2 +type: Opaque +data: + clientId: Y2xpZW50Mg== + key: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk \ No newline at end of file diff --git a/examples/custom-resources/api-key/cafe-secret.yaml b/examples/custom-resources/api-key/cafe-secret.yaml new file mode 100644 index 0000000000..8f9fd84855 --- /dev/null +++ b/examples/custom-resources/api-key/cafe-secret.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: cafe-secret +type: kubernetes.io/tls +data: + tls.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURMakNDQWhZQ0NRREFPRjl0THNhWFdqQU5CZ2txaGtpRzl3MEJBUXNGQURCYU1Rc3dDUVlEVlFRR0V3SlYKVXpFTE1Ba0dBMVVFQ0F3Q1EwRXhJVEFmQmdOVkJBb01HRWx1ZEdWeWJtVjBJRmRwWkdkcGRITWdVSFI1SUV4MApaREViTUJrR0ExVUVBd3dTWTJGbVpTNWxlR0Z0Y0d4bExtTnZiU0FnTUI0WERURTRNRGt4TWpFMk1UVXpOVm9YCkRUSXpNRGt4TVRFMk1UVXpOVm93V0RFTE1Ba0dBMVVFQmhNQ1ZWTXhDekFKQmdOVkJBZ01Ba05CTVNFd0h3WUQKVlFRS0RCaEpiblJsY201bGRDQlhhV1JuYVhSeklGQjBlU0JNZEdReEdUQVhCZ05WQkFNTUVHTmhabVV1WlhoaApiWEJzWlM1amIyMHdnZ0VpTUEwR0NTcUdTSWIzRFFFQkFRVUFBNElCRHdBd2dnRUtBb0lCQVFDcDZLbjdzeTgxCnAwanVKL2N5ayt2Q0FtbHNmanRGTTJtdVpOSzBLdGVjcUcyZmpXUWI1NXhRMVlGQTJYT1N3SEFZdlNkd0kyaloKcnVXOHFYWENMMnJiNENaQ0Z4d3BWRUNyY3hkam0zdGVWaVJYVnNZSW1tSkhQUFN5UWdwaW9iczl4N0RsTGM2SQpCQTBaalVPeWwwUHFHOVNKZXhNVjczV0lJYTVyRFZTRjJyNGtTa2JBajREY2o3TFhlRmxWWEgySTVYd1hDcHRDCm42N0pDZzQyZitrOHdnemNSVnA4WFprWldaVmp3cTlSVUtEWG1GQjJZeU4xWEVXZFowZXdSdUtZVUpsc202OTIKc2tPcktRajB2a29QbjQxRUUvK1RhVkVwcUxUUm9VWTNyemc3RGtkemZkQml6Rk8yZHNQTkZ4MkNXMGpYa05MdgpLbzI1Q1pyT2hYQUhBZ01CQUFFd0RRWUpLb1pJaHZjTkFRRUxCUUFEZ2dFQkFLSEZDY3lPalp2b0hzd1VCTWRMClJkSEliMzgzcFdGeW5acS9MdVVvdnNWQTU4QjBDZzdCRWZ5NXZXVlZycTVSSWt2NGxaODFOMjl4MjFkMUpINnIKalNuUXgrRFhDTy9USkVWNWxTQ1VwSUd6RVVZYVVQZ1J5anNNL05VZENKOHVIVmhaSitTNkZBK0NuT0Q5cm4yaQpaQmVQQ0k1ckh3RVh3bm5sOHl3aWozdnZRNXpISXV5QmdsV3IvUXl1aTlmalBwd1dVdlVtNG52NVNNRzl6Q1Y3ClBwdXd2dWF0cWpPMTIwOEJqZkUvY1pISWc4SHc5bXZXOXg5QytJUU1JTURFN2IvZzZPY0s3TEdUTHdsRnh2QTgKN1dqRWVxdW5heUlwaE1oS1JYVmYxTjM0OWVOOThFejM4Zk9USFRQYmRKakZBL1BjQytHeW1lK2lHdDVPUWRGaAp5UkU9Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K + tls.key: LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFb3dJQkFBS0NBUUVBcWVpcCs3TXZOYWRJN2lmM01wUHJ3Z0pwYkg0N1JUTnBybVRTdENyWG5LaHRuNDFrCkcrZWNVTldCUU5semtzQndHTDBuY0NObzJhN2x2S2wxd2k5cTIrQW1RaGNjS1ZSQXEzTVhZNXQ3WGxZa1YxYkcKQ0pwaVJ6ejBza0lLWXFHN1BjZXc1UzNPaUFRTkdZMURzcGRENmh2VWlYc1RGZTkxaUNHdWF3MVVoZHErSkVwRwp3SStBM0kreTEzaFpWVng5aU9WOEZ3cWJRcCt1eVFvT05uL3BQTUlNM0VWYWZGMlpHVm1WWThLdlVWQ2cxNWhRCmRtTWpkVnhGbldkSHNFYmltRkNaYkp1dmRySkRxeWtJOUw1S0Q1K05SQlAvazJsUkthaTAwYUZHTjY4NE93NUgKYzMzUVlzeFR0bmJEelJjZGdsdEkxNURTN3lxTnVRbWF6b1Z3QndJREFRQUJBb0lCQVFDUFNkU1luUXRTUHlxbApGZlZGcFRPc29PWVJoZjhzSStpYkZ4SU91UmF1V2VoaEp4ZG01Uk9ScEF6bUNMeUw1VmhqdEptZTIyM2dMcncyCk45OUVqVUtiL1ZPbVp1RHNCYzZvQ0Y2UU5SNThkejhjbk9SVGV3Y290c0pSMXBuMWhobG5SNUhxSkpCSmFzazEKWkVuVVFmY1hackw5NGxvOUpIM0UrVXFqbzFGRnM4eHhFOHdvUEJxalpzVjdwUlVaZ0MzTGh4bndMU0V4eUZvNApjeGI5U09HNU9tQUpvelN0Rm9RMkdKT2VzOHJKNXFmZHZ5dGdnOXhiTGFRTC94MGtwUTYyQm9GTUJEZHFPZVBXCktmUDV6WjYvMDcvdnBqNDh5QTFRMzJQem9idWJzQkxkM0tjbjMyamZtMUU3cHJ0V2wrSmVPRmlPem5CUUZKYk4KNHFQVlJ6NWhBb0dCQU50V3l4aE5DU0x1NFArWGdLeWNrbGpKNkY1NjY4Zk5qNUN6Z0ZScUowOXpuMFRsc05ybwpGVExaY3hEcW5SM0hQWU00MkpFUmgySi9xREZaeW5SUW8zY2czb2VpdlVkQlZHWTgrRkkxVzBxZHViL0w5K3l1CmVkT1pUUTVYbUdHcDZyNmpleHltY0ppbS9Pc0IzWm5ZT3BPcmxEN1NQbUJ2ek5MazRNRjZneGJYQW9HQkFNWk8KMHA2SGJCbWNQMHRqRlhmY0tFNzdJbUxtMHNBRzR1SG9VeDBlUGovMnFyblRuT0JCTkU0TXZnRHVUSnp5K2NhVQprOFJxbWRIQ2JIelRlNmZ6WXEvOWl0OHNaNzdLVk4xcWtiSWN1YytSVHhBOW5OaDFUanNSbmU3NFowajFGQ0xrCmhIY3FIMHJpN1BZU0tIVEU4RnZGQ3haWWRidUI4NENtWmlodnhicFJBb0dBSWJqcWFNWVBUWXVrbENkYTVTNzkKWVNGSjFKelplMUtqYS8vdER3MXpGY2dWQ0thMzFqQXdjaXowZi9sU1JxM0hTMUdHR21lemhQVlRpcUxmZVpxYwpSMGlLYmhnYk9jVlZrSkozSzB5QXlLd1BUdW14S0haNnpJbVpTMGMwYW0rUlk5WUdxNVQ3WXJ6cHpjZnZwaU9VCmZmZTNSeUZUN2NmQ21mb09oREN0enVrQ2dZQjMwb0xDMVJMRk9ycW40M3ZDUzUxemM1em9ZNDR1QnpzcHd3WU4KVHd2UC9FeFdNZjNWSnJEakJDSCtULzZzeXNlUGJKRUltbHpNK0l3eXRGcEFOZmlJWEV0LzQ4WGY2ME54OGdXTQp1SHl4Wlp4L05LdER3MFY4dlgxUE9ucTJBNWVpS2ErOGpSQVJZS0pMWU5kZkR1d29seHZHNmJaaGtQaS80RXRUCjNZMThzUUtCZ0h0S2JrKzdsTkpWZXN3WEU1Y1VHNkVEVXNEZS8yVWE3ZlhwN0ZjanFCRW9hcDFMU3crNlRYcDAKWmdybUtFOEFSek00NytFSkhVdmlpcS9udXBFMTVnMGtKVzNzeWhwVTl6WkxPN2x0QjBLSWtPOVpSY21Vam84UQpjcExsSE1BcWJMSjhXWUdKQ2toaVd4eWFsNmhZVHlXWTRjVmtDMHh0VGwvaFVFOUllTktvCi0tLS0tRU5EIFJTQSBQUklWQVRFIEtFWS0tLS0tCg== diff --git a/examples/custom-resources/api-key/cafe-virtual-server.yaml b/examples/custom-resources/api-key/cafe-virtual-server.yaml new file mode 100644 index 0000000000..d4b4ca1a9f --- /dev/null +++ b/examples/custom-resources/api-key/cafe-virtual-server.yaml @@ -0,0 +1,26 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServer +metadata: + name: cafe +spec: + host: cafe.example.com + tls: + secret: cafe-secret + policies: + - name: api-key-policy + upstreams: + - name: tea + service: tea-svc + port: 80 + - name: coffee + service: coffee-svc + port: 80 + routes: + - path: /tea + action: + pass: tea + - path: /coffee + action: + pass: coffee + policies: + - name: api-key-policy-2 diff --git a/examples/custom-resources/api-key/cafe.yaml b/examples/custom-resources/api-key/cafe.yaml new file mode 100644 index 0000000000..f049e8bf29 --- /dev/null +++ b/examples/custom-resources/api-key/cafe.yaml @@ -0,0 +1,65 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 2 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee-svc +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tea +spec: + replicas: 1 + selector: + matchLabels: + app: tea + template: + metadata: + labels: + app: tea + spec: + containers: + - name: tea + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: tea-svc +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: tea diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 9828c04a61..02bf5ea6b7 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -830,6 +830,12 @@ func (cnf *Configurator) addOrUpdateHtpasswdSecret(secret *api_v1.Secret) string return cnf.nginxManager.CreateSecret(name, data, nginx.HtpasswdSecretFileMode) } +func (cnf *Configurator) addOrUpdateAPIKeySecret(secret *api_v1.Secret) string { + name := objectMetaToFileName(&secret.ObjectMeta) + data := secret.Data[HtpasswdFileKey] + return cnf.nginxManager.CreateSecret(name, data, nginx.HtpasswdSecretFileMode) +} + // AddOrUpdateResources adds or updates configuration for resources. func (cnf *Configurator) AddOrUpdateResources(resources ExtendedResources, reloadIfUnchanged bool) (Warnings, error) { allWarnings := newWarnings() @@ -1990,6 +1996,10 @@ func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) string { case secrets.SecretTypeOIDC: // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. return "" + // how to update multiple secrets + case api_v1.SecretTypeOpaque: + glog.Infof(secret.Name) + return "" default: return cnf.addOrUpdateTLSSecret(secret) } diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index c79590fe5e..dec49bb3d7 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -84,6 +84,7 @@ type Server struct { IngressMTLS *IngressMTLS EgressMTLS *EgressMTLS OIDC *OIDC + APIKey *APIKey WAF *WAF Dos *Dos PoliciesErrorReturn *Return @@ -137,6 +138,19 @@ type OIDC struct { AccessTokenEnable bool } +type APIKey struct { + Header []string + Query []string + RejectCodeNotSupplied int + RejectCodeNoMatch int + Clients []Client +} + +type Client struct { + ClientID string + EncryptedKey string +} + // WAF defines WAF configuration. type WAF struct { Enable string @@ -197,6 +211,7 @@ type Location struct { BasicAuth *BasicAuth EgressMTLS *EgressMTLS OIDC bool + APIKey *APIKey WAF *WAF Dos *Dos PoliciesErrorReturn *Return diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index b943e2d227..efd33c664b 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -245,6 +245,22 @@ server { proxy_ssl_name {{ .SSLName }}; {{- end }} + {{- with $s.APIKey}} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + {{- range $header := .Header}} + # header: {{ $header }} + {{- end -}} + {{- range $query := .Query }} + # query: {{ $query }} + {{- end -}} + {{- range $client := .Clients }} + # client: {{ $client.ClientID }} + # secret: {{ $client.EncryptedKey }} + {{- end }} + {{- end }} + {{- with $s.WAF }} app_protect_enable {{ .Enable }}; {{ if .ApPolicy }} @@ -444,6 +460,23 @@ server { {{- end }} {{- end }} + + {{- with $l.APIKey}} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + {{- range $header := .Header}} + # header: {{ $header }} + {{- end -}} + {{- range $query := .Query }} + # query: {{ $query }} + {{- end -}} + {{- range $client := .Clients }} + # client: {{ $client.ClientID }} + # secret: {{ $client.EncryptedKey }} + {{- end }} + {{- end }} + {{- with $l.WAF }} app_protect_enable {{ .Enable }}; {{- if .ApPolicy }} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index c715e3410b..99ff521926 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1,6 +1,9 @@ package configs import ( + "crypto/sha256" + "encoding/base64" + "encoding/hex" "fmt" "net/url" "os" @@ -825,6 +828,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( JWKSAuthEnabled: policiesCfg.JWKSAuthEnabled, IngressMTLS: policiesCfg.IngressMTLS, EgressMTLS: policiesCfg.EgressMTLS, + APIKey: policiesCfg.APIKey, OIDC: vsc.oidcPolCfg.oidc, WAF: policiesCfg.WAF, Dos: dosCfg, @@ -858,6 +862,7 @@ type policiesCfg struct { IngressMTLS *version2.IngressMTLS EgressMTLS *version2.EgressMTLS OIDC bool + APIKey *version2.APIKey WAF *version2.WAF ErrorReturn *version2.Return BundleValidator bundleValidator @@ -1288,6 +1293,66 @@ func (p *policiesCfg) addOIDCConfig( return res } +func (p *policiesCfg) addAPIKeyConfig( + apiKey *conf_v1.APIKey, + polKey string, + polNamespace string, + secretRefs map[string]*secrets.SecretReference, +) *validationResults { + res := newValidationResults() + var clients []version2.Client + if p.APIKey != nil { + res.addWarningf( + "Multiple apiKey policies in the same context is not valid. APIKey policy %s will be ignored", + polKey, + ) + res.isError = true + return res + } else { + for _, client := range apiKey.Clients { + + secretKey := fmt.Sprintf("%v/%v", polNamespace, client.Secret) + secretRef := secretRefs[secretKey] + + var secretType api_v1.SecretType + if secretRef.Secret != nil { + secretType = secretRef.Secret.Type + } + if secretType != "" && secretType != api_v1.SecretTypeOpaque { + res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, api_v1.SecretTypeOpaque) + res.isError = true + return res + } else if secretRef.Error != nil { + res.addWarningf("API Key %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) + res.isError = true + return res + } + clientSecret := secretRef.Secret.Data["key"] // TODO: use const ClientSecretKey + + h := sha256.New() + h.Write([]byte(clientSecret)) + sha256Hash := hex.EncodeToString(h.Sum(nil)) + base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) + + glog.Infof("clientSecret %s", clientSecret) + glog.Infof("sha %s", sha256Hash) + glog.Infof("base64Str %s", base64Str) + clients = append(clients, version2.Client{ClientID: client.ID, EncryptedKey: sha256Hash}) // + } + + } + + p.APIKey = &version2.APIKey{ + Header: apiKey.SuppliedIn.Header, + Query: apiKey.SuppliedIn.Query, + RejectCodeNotSupplied: apiKey.RejectCodes.NotSupplied, + RejectCodeNoMatch: apiKey.RejectCodes.NoMatch, + Clients: clients, + } + return res + +} + func (p *policiesCfg) addWAFConfig( waf *conf_v1.WAF, polKey string, @@ -1418,6 +1483,8 @@ func (vsc *virtualServerConfigurator) generatePolicies( res = config.addEgressMTLSConfig(pol.Spec.EgressMTLS, key, polNamespace, policyOpts.secretRefs) case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) + case pol.Spec.APIKey != nil: + res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) case pol.Spec.WAF != nil: res = config.addWAFConfig(pol.Spec.WAF, key, polNamespace, policyOpts.apResources) default: @@ -1501,6 +1568,7 @@ func addPoliciesCfgToLocation(cfg policiesCfg, location *version2.Location) { location.EgressMTLS = cfg.EgressMTLS location.OIDC = cfg.OIDC location.WAF = cfg.WAF + location.APIKey = cfg.APIKey location.PoliciesErrorReturn = cfg.ErrorReturn } diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 43a0c4fb89..d9df838e89 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -3168,6 +3168,10 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. if err != nil { glog.Warningf("Error getting OIDC secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } + err = lbc.addAPIKeySecretRefs(virtualServerEx.SecretRefs, policies) + if err != nil { + glog.Warningf("Error getting APIKey secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) + } err = lbc.addWAFPolicyRefs(virtualServerEx.ApPolRefs, virtualServerEx.LogConfRefs, policies) if err != nil { @@ -3574,6 +3578,26 @@ func (lbc *LoadBalancerController) addOIDCSecretRefs(secretRefs map[string]*secr return nil } +func (lbc *LoadBalancerController) addAPIKeySecretRefs(secretRefs map[string]*secrets.SecretReference, policies []*conf_v1.Policy) error { + for _, pol := range policies { + if pol.Spec.APIKey == nil { + continue + } + + for _, client := range pol.Spec.APIKey.Clients { + secretKey := fmt.Sprintf("%v/%v", pol.Namespace, client.Secret) + secretRef := lbc.secretStore.GetSecret(secretKey) + + secretRefs[secretKey] = secretRef + + if secretRef.Error != nil { + return secretRef.Error + } + } + } + return nil +} + // addWAFPolicyRefs ensures the app protect resources that are referenced in policies exist. func (lbc *LoadBalancerController) addWAFPolicyRefs( apPolRef, logConfRef map[string]*unstructured.Unstructured, diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 0f2908a5f0..8da781eafb 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -131,7 +131,8 @@ func IsSupportedSecretType(secretType api_v1.SecretType) bool { secretType == SecretTypeCA || secretType == SecretTypeJWK || secretType == SecretTypeOIDC || - secretType == SecretTypeHtpasswd + secretType == SecretTypeHtpasswd || + secretType == api_v1.SecretTypeOpaque } // ValidateSecret validates the secret. If it is valid, the function returns nil. @@ -147,6 +148,8 @@ func ValidateSecret(secret *api_v1.Secret) error { return ValidateOIDCSecret(secret) case SecretTypeHtpasswd: return ValidateHtpasswdSecret(secret) + case api_v1.SecretTypeOpaque: + return nil } return fmt.Errorf("Secret is of the unsupported type %v", secret.Type) diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 067391cd65..d2c076ad20 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -577,6 +577,7 @@ type PolicySpec struct { EgressMTLS *EgressMTLS `json:"egressMTLS"` OIDC *OIDC `json:"oidc"` WAF *WAF `json:"waf"` + APIKey *APIKey `json:"apiKey"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object @@ -675,3 +676,24 @@ type SecurityLog struct { ApLogBundle string `json:"apLogBundle"` LogDest string `json:"logDest"` } + +type APIKey struct { + SuppliedIn SuppliedIn `json:"suppliedIn"` + RejectCodes RejectCodes `json:"rejectCodes,omitempty"` + Clients []Client `json:"clients"` +} + +type SuppliedIn struct { + Header []string `json:"header"` + Query []string `json:"query"` +} + +type RejectCodes struct { + NotSupplied int `json:"notSupplied"` + NoMatch int `json:"noMatch"` +} + +type Client struct { + ID string `json:"id"` + Secret string `json:"secret"` +} diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 52c5327146..e30e9ee168 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -9,6 +9,29 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *APIKey) DeepCopyInto(out *APIKey) { + *out = *in + in.SuppliedIn.DeepCopyInto(&out.SuppliedIn) + out.RejectCodes = in.RejectCodes + if in.Clients != nil { + in, out := &in.Clients, &out.Clients + *out = make([]Client, len(*in)) + copy(*out, *in) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new APIKey. +func (in *APIKey) DeepCopy() *APIKey { + if in == nil { + return nil + } + out := new(APIKey) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AccessControl) DeepCopyInto(out *AccessControl) { *out = *in @@ -178,6 +201,22 @@ func (in *CertManager) DeepCopy() *CertManager { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Client) DeepCopyInto(out *Client) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Client. +func (in *Client) DeepCopy() *Client { + if in == nil { + return nil + } + out := new(Client) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Condition) DeepCopyInto(out *Condition) { *out = *in @@ -689,6 +728,11 @@ func (in *PolicySpec) DeepCopyInto(out *PolicySpec) { *out = new(WAF) (*in).DeepCopyInto(*out) } + if in.APIKey != nil { + in, out := &in.APIKey, &out.APIKey + *out = new(APIKey) + (*in).DeepCopyInto(*out) + } return } @@ -857,6 +901,22 @@ 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 *RejectCodes) DeepCopyInto(out *RejectCodes) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RejectCodes. +func (in *RejectCodes) DeepCopy() *RejectCodes { + if in == nil { + return nil + } + out := new(RejectCodes) + 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 @@ -973,6 +1033,32 @@ func (in *Split) DeepCopy() *Split { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *SuppliedIn) DeepCopyInto(out *SuppliedIn) { + *out = *in + if in.Header != nil { + in, out := &in.Header, &out.Header + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Query != nil { + in, out := &in.Query, &out.Query + *out = make([]string, len(*in)) + copy(*out, *in) + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SuppliedIn. +func (in *SuppliedIn) DeepCopy() *SuppliedIn { + if in == nil { + return nil + } + out := new(SuppliedIn) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TLS) DeepCopyInto(out *TLS) { *out = *in diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index a762146446..f583f02d8b 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -72,6 +72,11 @@ func validatePolicySpec(spec *v1.PolicySpec, fieldPath *field.Path, isPlus, enab fieldCount++ } + if spec.APIKey != nil { + allErrs = append(allErrs, validateAPIKey(spec.APIKey, fieldPath.Child("apiKey"))...) + fieldCount++ + } + if spec.WAF != nil { if !isPlus { allErrs = append(allErrs, field.Forbidden(fieldPath.Child("waf"), "WAF is only supported in NGINX Plus")) @@ -277,6 +282,14 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { return append(allErrs, validateClientID(oidc.ClientID, fieldPath.Child("clientID"))...) } +func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { + + allErrs := field.ErrorList{} + + return allErrs + +} + func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} bundleMode := waf.ApBundle != "" From 94851fc7286d35045c51ff834e986f0b190b0f98 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 20 May 2024 08:27:00 +0000 Subject: [PATCH 03/36] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- examples/custom-resources/api-key/api-key-policy-2.yaml | 2 +- examples/custom-resources/api-key/api-key-policy.yaml | 2 +- examples/custom-resources/api-key/api-key-secret-1.yaml | 2 +- examples/custom-resources/api-key/api-key-secret-2.yaml | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml index b2eb759348..3f725f5c55 100644 --- a/examples/custom-resources/api-key/api-key-policy-2.yaml +++ b/examples/custom-resources/api-key/api-key-policy-2.yaml @@ -14,4 +14,4 @@ spec: noMatch: 410 clients: - id: client2 - secret: api-key-client-secret-2 \ No newline at end of file + secret: api-key-client-secret-2 diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml index 9af347e922..01026e5707 100644 --- a/examples/custom-resources/api-key/api-key-policy.yaml +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -18,4 +18,4 @@ spec: - id: client1 secret: api-key-client-secret-1 - id: client2 - secret: api-key-client-secret-2 \ No newline at end of file + secret: api-key-client-secret-2 diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml index 998851888b..89f95731fd 100644 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -5,4 +5,4 @@ metadata: type: Opaque data: clientId: Y2xpZW50MQo= - key: cGFzc3dvcmQ= \ No newline at end of file + key: cGFzc3dvcmQ= diff --git a/examples/custom-resources/api-key/api-key-secret-2.yaml b/examples/custom-resources/api-key/api-key-secret-2.yaml index d997cd8352..fcd5a2b7c5 100644 --- a/examples/custom-resources/api-key/api-key-secret-2.yaml +++ b/examples/custom-resources/api-key/api-key-secret-2.yaml @@ -5,4 +5,4 @@ metadata: type: Opaque data: clientId: Y2xpZW50Mg== - key: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk \ No newline at end of file + key: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk From 20f7d06ebadc0f13cd0f74ae16dcd0c4d5f01d98 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Mon, 20 May 2024 14:06:09 +0100 Subject: [PATCH 04/36] add njs and nginx config --- build/Dockerfile | 1 + internal/configs/njs/apikey_auth.js | 25 ++++++++ .../version2/nginx-plus.virtualserver.tmpl | 38 +++++++++-- .../configs/version2/nginx.virtualserver.tmpl | 55 ++++++++++++++++ internal/configs/version2/template_helper.go | 41 ++++++++---- internal/configs/virtualserver.go | 63 ++++++++++++++----- internal/configs/virtualserver_test.go | 18 ++++-- 7 files changed, 202 insertions(+), 39 deletions(-) create mode 100644 internal/configs/njs/apikey_auth.js diff --git a/build/Dockerfile b/build/Dockerfile index aa39ec3f44..9486ce1064 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -380,6 +380,7 @@ RUN --mount=type=bind,target=/tmp mkdir -p /var/lib/nginx /etc/nginx/secrets /et && [ -z "${BUILD_OS##*plus*}" ] && PLUS=-plus; cp -a /tmp/internal/configs/version1/nginx$PLUS.ingress.tmpl /tmp/internal/configs/version1/nginx$PLUS.tmpl \ /tmp/internal/configs/version2/nginx$PLUS.virtualserver.tmpl /tmp/internal/configs/version2/nginx$PLUS.transportserver.tmpl / \ && if [ -z "${BUILD_OS##*plus*}" ]; then mkdir -p /etc/nginx/state_files/; fi \ + && mkdir -p /etc/nginx/njs/ && cp -a /tmp/internal/configs/njs/* /etc/nginx/njs/ \ && chown -R 101:0 /etc/nginx /var/cache/nginx /var/lib/nginx /var/log/nginx /*.tmpl \ && chmod -R g=u /etc/nginx /var/cache/nginx /var/lib/nginx /var/log/nginx /*.tmpl \ && rm -f /etc/nginx/conf.d/* diff --git a/internal/configs/njs/apikey_auth.js b/internal/configs/njs/apikey_auth.js new file mode 100644 index 0000000000..787e53b8b3 --- /dev/null +++ b/internal/configs/njs/apikey_auth.js @@ -0,0 +1,25 @@ +const c = require("crypto"); + +function hash(r) { + const header_query_value = r.variables.header_query_value; + const hashed_value = c + .createHash("sha256") + .update(header_query_value) + .digest("hex"); + return hashed_value; +} + +function validate(r) { + const client_name = r.variables["apikey_auth_local_map"]; + const header_query_value = r.variables.header_query_value; + + if (!header_query_value) { + r.return(401, "401"); + } else if (!client_name) { + r.return(403, "403"); + } else { + r.return(204, "204"); + } +} + +export default { validate, hash }; diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index efd33c664b..844eb21fd6 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -1,4 +1,7 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version2.VirtualServerConfig*/ -}} + +js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + {{ range $u := .Upstreams }} upstream {{ $u.Name }} { zone {{ $u.Name }} {{ if ne $u.UpstreamZoneSize "0" }}{{ $u.UpstreamZoneSize }}{{ else }}512k{{ end }}; @@ -222,6 +225,13 @@ server { } {{- end }} + + # TODO: Do this conditionally + location = /_validate_apikey_njs { + internal; + js_content apikey_auth.validate; + } + {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; @@ -246,9 +256,17 @@ server { {{- end }} {{- with $s.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hash_fn_server apikey_auth.hash; + js_var $apikey_auth_token $hash_fn_server; + + set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy; #TODO: dont hardcode variable + + auth_request /_validate_apikey_njs; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} @@ -462,9 +480,17 @@ server { {{- with $l.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hash_fn_location apikey_auth.hash; + js_var $apikey_auth_token $hash_fn_location; + + set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy_2; #TODO: dont hardcode variable + + auth_request /_validate_apikey_njs; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index b558a65e2d..87de63471b 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -1,4 +1,5 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version2.VirtualServerConfig*/ -}} +js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed {{ range $u := .Upstreams }} upstream {{ $u.Name }} { {{- if ne $u.UpstreamZoneSize "0" }}zone {{ $u.Name }} {{ $u.UpstreamZoneSize }};{{ end }} @@ -143,11 +144,41 @@ server { {{ if $rl.Delay }} delay={{ $rl.Delay }}{{ end }}{{ if $rl.NoDelay }} nodelay{{ end }}; {{- end }} + # TODO: Do this conditionally + location = /_validate_apikey_njs { + internal; + js_content apikey_auth.validate; + } + {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; {{- end }} + {{- with $s.APIKey}} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hash_fn_server apikey_auth.hash; + js_var $apikey_auth_token $hash_fn_server; + + set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy; #TODO: dont hardcode variable + + auth_request /_validate_apikey_njs; + {{- range $header := .Header}} + # header: {{ $header }} + {{- end -}} + {{- range $query := .Query }} + # query: {{ $query }} + {{- end -}} + {{- range $client := .Clients }} + # client: {{ $client.ClientID }} + # secret: {{ $client.EncryptedKey }} + {{- end }} + {{- end }} + {{- with $s.EgressMTLS }} {{- if .Certificate }} proxy_ssl_certificate {{ makeSecretPath .Certificate $.StaticSSLPath "$secret_dir_path" $.DynamicSSLReloadEnabled }}; @@ -255,6 +286,30 @@ server { auth_basic_user_file {{ .Secret }}; {{- end }} + {{- with $l.APIKey}} + # API KEY POLICY + # not supplied: {{ .RejectCodeNotSupplied }} + # no match: {{ .RejectCodeNoMatch }} + + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hash_fn_location apikey_auth.hash; + js_var $apikey_auth_token $hash_fn_location; + + set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy_2; #TODO: dont hardcode variable + + auth_request /_validate_apikey_njs; + {{- range $header := .Header}} + # header: {{ $header }} + {{- end -}} + {{- range $query := .Query }} + # query: {{ $query }} + {{- end -}} + {{- range $client := .Clients }} + # client: {{ $client.ClientID }} + # secret: {{ $client.EncryptedKey }} + {{- end }} + {{- end }} + {{ $proxyOrGRPC := "proxy" }}{{ if $l.GRPCPass }}{{ $proxyOrGRPC = "grpc" }}{{ end }} {{- with $l.EgressMTLS }} diff --git a/internal/configs/version2/template_helper.go b/internal/configs/version2/template_helper.go index f49466875e..1f9356959f 100644 --- a/internal/configs/version2/template_helper.go +++ b/internal/configs/version2/template_helper.go @@ -1,6 +1,7 @@ package version2 import ( + "fmt" "strconv" "strings" "text/template" @@ -121,16 +122,34 @@ func makeHTTPSListener(s Server) string { return makeListener(https, s) } +func makeHeaderQueryValue(apiKey APIKey) string { + var parts []string + + for _, header := range apiKey.Header { + nginxHeader := strings.ReplaceAll(header, "-", "_") + nginxHeader = strings.ToLower(nginxHeader) + + parts = append(parts, fmt.Sprintf("${http_%s}", nginxHeader)) + } + + for _, query := range apiKey.Query { + parts = append(parts, fmt.Sprintf("${arg_%s}", query)) + } + + return fmt.Sprintf("\"%s\"", strings.Join(parts, "")) +} + var helperFunctions = template.FuncMap{ - "headerListToCIMap": headerListToCIMap, - "hasCIKey": hasCIKey, - "contains": strings.Contains, - "hasPrefix": strings.HasPrefix, - "hasSuffix": strings.HasSuffix, - "toLower": strings.ToLower, - "toUpper": strings.ToUpper, - "replaceAll": strings.ReplaceAll, - "makeHTTPListener": makeHTTPListener, - "makeHTTPSListener": makeHTTPSListener, - "makeSecretPath": commonhelpers.MakeSecretPath, + "headerListToCIMap": headerListToCIMap, + "hasCIKey": hasCIKey, + "contains": strings.Contains, + "hasPrefix": strings.HasPrefix, + "hasSuffix": strings.HasSuffix, + "toLower": strings.ToLower, + "toUpper": strings.ToUpper, + "replaceAll": strings.ReplaceAll, + "makeHTTPListener": makeHTTPListener, + "makeHTTPSListener": makeHTTPSListener, + "makeSecretPath": commonhelpers.MakeSecretPath, + "makeHeaderQueryValue": makeHeaderQueryValue, } diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 99ff521926..3729990ad8 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -393,6 +393,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( dosResources map[string]*appProtectDosResource, ) (version2.VirtualServerConfig, Warnings) { vsc.clearWarnings() + var maps []version2.Map useCustomListeners := false @@ -415,8 +416,9 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - policiesCfg := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) + policiesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) + maps = append(maps, mapsFromPolicies...) if policiesCfg.JWKSAuthEnabled { jwtAuthKey := policiesCfg.JWTAuth.Key policiesCfg.JWTAuthList = make(map[string]*version2.JWTAuth) @@ -510,7 +512,6 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( var internalRedirectLocations []version2.InternalRedirectLocation var returnLocations []version2.ReturnLocation var splitClients []version2.SplitClient - var maps []version2.Map var errorPageLocations []version2.ErrorPageLocation var keyValZones []version2.KeyValZone var keyVals []version2.KeyVal @@ -566,7 +567,9 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - routePoliciesCfg := vsc.generatePolicies(ownerDetails, r.Policies, vsEx.Policies, routeContext, policyOpts) + routePoliciesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, r.Policies, vsEx.Policies, routeContext, policyOpts) + maps = append(maps, mapsFromPolicies...) + if policiesCfg.OIDC { routePoliciesCfg.OIDC = policiesCfg.OIDC } @@ -696,7 +699,8 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policyRefs = r.Policies context = subRouteContext } - routePoliciesCfg := vsc.generatePolicies(ownerDetails, policyRefs, vsEx.Policies, context, policyOpts) + routePoliciesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, policyRefs, vsEx.Policies, context, policyOpts) + maps = append(maps, mapsFromPolicies...) if policiesCfg.OIDC { routePoliciesCfg.OIDC = policiesCfg.OIDC } @@ -1298,7 +1302,7 @@ func (p *policiesCfg) addAPIKeyConfig( polKey string, polNamespace string, secretRefs map[string]*secrets.SecretReference, -) *validationResults { +) (*validationResults, *version2.Map) { res := newValidationResults() var clients []version2.Client if p.APIKey != nil { @@ -1307,7 +1311,7 @@ func (p *policiesCfg) addAPIKeyConfig( polKey, ) res.isError = true - return res + return res, nil } else { for _, client := range apiKey.Clients { @@ -1321,11 +1325,11 @@ func (p *policiesCfg) addAPIKeyConfig( if secretType != "" && secretType != api_v1.SecretTypeOpaque { res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, api_v1.SecretTypeOpaque) res.isError = true - return res + return res, nil } else if secretRef.Error != nil { res.addWarningf("API Key %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true - return res + return res, nil } clientSecret := secretRef.Secret.Data["key"] // TODO: use const ClientSecretKey @@ -1342,6 +1346,25 @@ func (p *policiesCfg) addAPIKeyConfig( } + default_parameter := version2.Parameter{ + Value: "default", + Result: "\"\"", + } + + params := []version2.Parameter{default_parameter} + for _, client := range clients { + params = append(params, version2.Parameter{ + Value: fmt.Sprintf("\"%s\"", client.EncryptedKey), + Result: fmt.Sprintf("\"%s\"", client.ClientID), + }) + } + + shaToClient := &version2.Map{ + Source: "$apikey_auth_token", + Variable: strings.Replace(fmt.Sprintf("$apikey_auth_client_name_%s", strings.Split(polKey, "/")[1]), "-", "_", -1), + Parameters: params, + } + p.APIKey = &version2.APIKey{ Header: apiKey.SuppliedIn.Header, Query: apiKey.SuppliedIn.Query, @@ -1349,7 +1372,7 @@ func (p *policiesCfg) addAPIKeyConfig( RejectCodeNoMatch: apiKey.RejectCodes.NoMatch, Clients: clients, } - return res + return res, shaToClient } @@ -1441,8 +1464,9 @@ func (vsc *virtualServerConfigurator) generatePolicies( policies map[string]*conf_v1.Policy, context string, policyOpts policyOptions, -) policiesCfg { +) (policiesCfg, []version2.Map) { config := newPoliciesConfig(vsc.bundleValidator) + maps := []version2.Map{} for _, p := range policyRefs { polNamespace := p.Namespace @@ -1484,27 +1508,34 @@ func (vsc *virtualServerConfigurator) generatePolicies( case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: - res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) + res, shaToClientMap := config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) + if res != nil && !res.isError && shaToClientMap != nil { + maps = append(maps, *shaToClientMap) + } + case pol.Spec.WAF != nil: res = config.addWAFConfig(pol.Spec.WAF, key, polNamespace, policyOpts.apResources) default: res = newValidationResults() } - vsc.addWarnings(ownerDetails.owner, res.warnings) - if res.isError { + if res != nil && len(res.warnings) > 0 { + vsc.addWarnings(ownerDetails.owner, res.warnings) + } + + if res != nil && res.isError { return policiesCfg{ ErrorReturn: &version2.Return{Code: 500}, - } + }, maps } } else { vsc.addWarningf(ownerDetails.owner, "Policy %s is missing or invalid", key) return policiesCfg{ ErrorReturn: &version2.Return{Code: 500}, - } + }, maps } } - return *config + return *config, maps } func generateLimitReq(zoneName string, rateLimitPol *conf_v1.RateLimit) version2.LimitReq { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 640a86f831..c3951b16b6 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6284,7 +6284,9 @@ func TestGeneratePolicies(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) for _, test := range tests { - result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) + result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) + //TODO test maps + println(maps) // temporary print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) @@ -6377,10 +6379,12 @@ func TestGeneratePolicies_GeneratesWAFPolicyOnValidApBundle(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) - got := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) - got.BundleValidator = nil - if !cmp.Equal(tc.want, got) { - t.Error(cmp.Diff(tc.want, got)) + res, maps := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) + //TODO test maps + println(maps) // temporary print to use the variable + res.BundleValidator = nil + if !cmp.Equal(tc.want, res) { + t.Error(cmp.Diff(tc.want, res)) } }) } @@ -7722,7 +7726,9 @@ func TestGeneratePoliciesFails(t *testing.T) { vsc.oidcPolCfg = test.oidcPolCfg } - result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) + result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) + //TODO test maps + println(maps) // Temporty print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) From fd33e3b1c74a04136dae1f982473618564a3490b Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 22 May 2024 15:44:51 +0100 Subject: [PATCH 05/36] move js import to http --- internal/configs/version1/nginx-plus.tmpl | 2 ++ internal/configs/version1/nginx.tmpl | 4 ++++ internal/configs/version2/nginx-plus.virtualserver.tmpl | 1 - internal/configs/version2/nginx.virtualserver.tmpl | 1 - 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index 77a568f9f8..2de3a3d9e0 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -39,6 +39,8 @@ http { map_hash_max_size {{.MapHashMaxSize}}; map_hash_bucket_size {{.MapHashBucketSize}}; + js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + {{- if .HTTPSnippets}} {{range $value := .HTTPSnippets}} {{$value}}{{end}} diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index e56b508af9..ba268e36dc 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -1,4 +1,5 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version1.MainConfig*/ -}} + worker_processes {{.WorkerProcesses}}; {{- if .WorkerRlimitNofile}} worker_rlimit_nofile {{.WorkerRlimitNofile}};{{end}} @@ -32,6 +33,9 @@ http { map_hash_max_size {{.MapHashMaxSize}}; map_hash_bucket_size {{.MapHashBucketSize}}; + + js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + {{- if .HTTPSnippets}} {{range $value := .HTTPSnippets}} {{$value}}{{end}} diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 844eb21fd6..68e916770b 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -1,6 +1,5 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version2.VirtualServerConfig*/ -}} -js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed {{ range $u := .Upstreams }} upstream {{ $u.Name }} { diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 87de63471b..c043756487 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -1,5 +1,4 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version2.VirtualServerConfig*/ -}} -js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed {{ range $u := .Upstreams }} upstream {{ $u.Name }} { {{- if ne $u.UpstreamZoneSize "0" }}zone {{ $u.Name }} {{ $u.UpstreamZoneSize }};{{ end }} From 5ce5ac5569dd0a5034bd8793b5d286d7bb70214b Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 24 May 2024 10:20:04 +0100 Subject: [PATCH 06/36] update schema and allow update after NIC starts 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 | 11 +-- deploy/crds.yaml | 11 +-- .../api-key/api-key-policy-2.yaml | 5 +- .../api-key/api-key-policy.yaml | 6 +- .../api-key/api-key-secret-1.yaml | 4 +- .../api-key/api-key-secret-2.yaml | 3 +- internal/configs/configurator.go | 7 -- internal/configs/virtualserver.go | 69 +++++++++++-------- internal/k8s/controller.go | 21 ++++-- internal/k8s/secrets/validation.go | 9 ++- pkg/apis/configuration/v1/types.go | 6 +- .../configuration/v1/zz_generated.deepcopy.go | 21 ------ pkg/apis/configuration/validation/policy.go | 6 +- 13 files changed, 80 insertions(+), 99 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 90563ef69d..8c2b0abb89 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -69,15 +69,8 @@ spec: type: object apiKey: properties: - clients: - items: - properties: - id: - type: string - secret: - type: string - type: object - type: array + clientSecret: + type: string rejectCodes: properties: noMatch: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 4a54cea534..2a473b3c78 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -271,15 +271,8 @@ spec: type: object apiKey: properties: - clients: - items: - properties: - id: - type: string - secret: - type: string - type: object - type: array + clientSecret: + type: string rejectCodes: properties: noMatch: diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml index 3f725f5c55..45045fc43f 100644 --- a/examples/custom-resources/api-key/api-key-policy-2.yaml +++ b/examples/custom-resources/api-key/api-key-policy-2.yaml @@ -12,6 +12,5 @@ spec: rejectCodes: # optional notSupplied: 408 noMatch: 410 - clients: - - id: client2 - secret: api-key-client-secret-2 + clientSecret: api-key-client-secret-2 + diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml index 01026e5707..68a88f7a70 100644 --- a/examples/custom-resources/api-key/api-key-policy.yaml +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -14,8 +14,4 @@ spec: rejectCodes: # optional notSupplied: 401 noMatch: 403 - clients: - - id: client1 - secret: api-key-client-secret-1 - - id: client2 - secret: api-key-client-secret-2 + clientSecret: api-key-client-secret-1 diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml index 89f95731fd..2a71600625 100644 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -4,5 +4,5 @@ metadata: name: api-key-client-secret-1 type: Opaque data: - clientId: Y2xpZW50MQo= - key: cGFzc3dvcmQ= + client1: cGFzc3dvcmQ= + client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk diff --git a/examples/custom-resources/api-key/api-key-secret-2.yaml b/examples/custom-resources/api-key/api-key-secret-2.yaml index fcd5a2b7c5..e175e817b0 100644 --- a/examples/custom-resources/api-key/api-key-secret-2.yaml +++ b/examples/custom-resources/api-key/api-key-secret-2.yaml @@ -4,5 +4,4 @@ metadata: name: api-key-client-secret-2 type: Opaque data: - clientId: Y2xpZW50Mg== - key: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk + client1: cGFzc3dvcmQ= diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 02bf5ea6b7..1d569c4617 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -830,12 +830,6 @@ func (cnf *Configurator) addOrUpdateHtpasswdSecret(secret *api_v1.Secret) string return cnf.nginxManager.CreateSecret(name, data, nginx.HtpasswdSecretFileMode) } -func (cnf *Configurator) addOrUpdateAPIKeySecret(secret *api_v1.Secret) string { - name := objectMetaToFileName(&secret.ObjectMeta) - data := secret.Data[HtpasswdFileKey] - return cnf.nginxManager.CreateSecret(name, data, nginx.HtpasswdSecretFileMode) -} - // AddOrUpdateResources adds or updates configuration for resources. func (cnf *Configurator) AddOrUpdateResources(resources ExtendedResources, reloadIfUnchanged bool) (Warnings, error) { allWarnings := newWarnings() @@ -1996,7 +1990,6 @@ func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) string { case secrets.SecretTypeOIDC: // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. return "" - // how to update multiple secrets case api_v1.SecretTypeOpaque: glog.Infof(secret.Name) return "" diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 3729990ad8..c0e35fb845 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1307,43 +1307,44 @@ func (p *policiesCfg) addAPIKeyConfig( var clients []version2.Client if p.APIKey != nil { res.addWarningf( - "Multiple apiKey policies in the same context is not valid. APIKey policy %s will be ignored", + "Multiple APIKey policies in the same context is not valid. APIKey policy %s will be ignored", polKey, ) res.isError = true return res, nil - } else { - for _, client := range apiKey.Clients { - - secretKey := fmt.Sprintf("%v/%v", polNamespace, client.Secret) - secretRef := secretRefs[secretKey] + } - var secretType api_v1.SecretType - if secretRef.Secret != nil { - secretType = secretRef.Secret.Type - } - if secretType != "" && secretType != api_v1.SecretTypeOpaque { - res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, api_v1.SecretTypeOpaque) - res.isError = true - return res, nil - } else if secretRef.Error != nil { - res.addWarningf("API Key %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) - res.isError = true - return res, nil - } - clientSecret := secretRef.Secret.Data["key"] // TODO: use const ClientSecretKey + //if apiKey.ClientSecret != "" { + secretKey := fmt.Sprintf("%v/%v", polNamespace, apiKey.ClientSecret) + glog.Infof("secretKey: %v", secretKey) + secretRef := secretRefs[secretKey] + glog.Infof("secretRefs: %v", secretRefs) + var secretType api_v1.SecretType + glog.Infof("secret: %v", secretRef) + if secretRef.Secret != nil { + secretType = secretRef.Secret.Type + } + if secretType != "" && secretType != api_v1.SecretTypeOpaque { + res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, api_v1.SecretTypeOpaque) + res.isError = true + return res, nil + } else if secretRef.Error != nil { + res.addWarningf("API Key %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) + res.isError = true + return res, nil + } - h := sha256.New() - h.Write([]byte(clientSecret)) - sha256Hash := hex.EncodeToString(h.Sum(nil)) - base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) + for clientID, clientSecret := range secretRef.Secret.Data { - glog.Infof("clientSecret %s", clientSecret) - glog.Infof("sha %s", sha256Hash) - glog.Infof("base64Str %s", base64Str) - clients = append(clients, version2.Client{ClientID: client.ID, EncryptedKey: sha256Hash}) // - } + h := sha256.New() + h.Write([]byte(clientSecret)) + sha256Hash := hex.EncodeToString(h.Sum(nil)) + base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) + glog.Infof("clientSecret %s", clientSecret) + glog.Infof("sha %s", sha256Hash) + glog.Infof("base64Str %s", base64Str) + clients = append(clients, version2.Client{ClientID: clientID, EncryptedKey: sha256Hash}) // } default_parameter := version2.Parameter{ @@ -1509,6 +1510,16 @@ func (vsc *virtualServerConfigurator) generatePolicies( res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: res, shaToClientMap := config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) + // TODO: refactor + if res != nil && len(res.warnings) > 0 { + vsc.addWarnings(ownerDetails.owner, res.warnings) + } + + if res != nil && res.isError { + return policiesCfg{ + ErrorReturn: &version2.Return{Code: 500}, + }, maps + } if res != nil && !res.isError && shaToClientMap != nil { maps = append(maps, *shaToClientMap) } diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index d9df838e89..3304dad9cf 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -3295,6 +3295,12 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. if err != nil { glog.Warningf("Error getting OIDC secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) } + + err = lbc.addAPIKeySecretRefs(virtualServerEx.SecretRefs, vsRoutePolicies) + if err != nil { + glog.Warningf("Error getting APIKey secrets for VirtualServer %v/%v: %v", virtualServer.Namespace, virtualServer.Name, err) + } + } for _, vsr := range virtualServerRoutes { @@ -3584,16 +3590,15 @@ func (lbc *LoadBalancerController) addAPIKeySecretRefs(secretRefs map[string]*se continue } - for _, client := range pol.Spec.APIKey.Clients { - secretKey := fmt.Sprintf("%v/%v", pol.Namespace, client.Secret) - secretRef := lbc.secretStore.GetSecret(secretKey) + secretKey := fmt.Sprintf("%v/%v", pol.Namespace, pol.Spec.APIKey.ClientSecret) + secretRef := lbc.secretStore.GetSecret(secretKey) - secretRefs[secretKey] = secretRef + secretRefs[secretKey] = secretRef - if secretRef.Error != nil { - return secretRef.Error - } + if secretRef.Error != nil { + return secretRef.Error } + } return nil } @@ -3676,6 +3681,8 @@ func findPoliciesForSecret(policies []*conf_v1.Policy, secretNamespace string, s res = append(res, pol) } else if pol.Spec.OIDC != nil && pol.Spec.OIDC.ClientSecret == secretName && pol.Namespace == secretNamespace { res = append(res, pol) + } else if pol.Spec.APIKey != nil && pol.Spec.APIKey.ClientSecret == secretName && pol.Namespace == secretNamespace { + res = append(res, pol) } } diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 8da781eafb..518b7923bb 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -109,6 +109,13 @@ func ValidateOIDCSecret(secret *api_v1.Secret) error { return nil } +func ValidateAPIKeySecret(secret *api_v1.Secret) error { + if secret.Type != api_v1.SecretTypeOpaque { + return fmt.Errorf("APIKey secret must be of the type %v", SecretTypeOIDC) + } + return nil +} + // ValidateHtpasswdSecret validates the secret. If it is valid, the function returns nil. func ValidateHtpasswdSecret(secret *api_v1.Secret) error { if secret.Type != SecretTypeHtpasswd { @@ -149,7 +156,7 @@ func ValidateSecret(secret *api_v1.Secret) error { case SecretTypeHtpasswd: return ValidateHtpasswdSecret(secret) case api_v1.SecretTypeOpaque: - return nil + return ValidateAPIKeySecret(secret) } return fmt.Errorf("Secret is of the unsupported type %v", secret.Type) diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index d2c076ad20..f2e470814b 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -678,9 +678,9 @@ type SecurityLog struct { } type APIKey struct { - SuppliedIn SuppliedIn `json:"suppliedIn"` - RejectCodes RejectCodes `json:"rejectCodes,omitempty"` - Clients []Client `json:"clients"` + SuppliedIn SuppliedIn `json:"suppliedIn"` + RejectCodes RejectCodes `json:"rejectCodes,omitempty"` + ClientSecret string `json:"clientSecret"` } type SuppliedIn struct { diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index e30e9ee168..10760491f5 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -14,11 +14,6 @@ func (in *APIKey) DeepCopyInto(out *APIKey) { *out = *in in.SuppliedIn.DeepCopyInto(&out.SuppliedIn) out.RejectCodes = in.RejectCodes - if in.Clients != nil { - in, out := &in.Clients, &out.Clients - *out = make([]Client, len(*in)) - copy(*out, *in) - } return } @@ -201,22 +196,6 @@ func (in *CertManager) DeepCopy() *CertManager { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Client) DeepCopyInto(out *Client) { - *out = *in - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Client. -func (in *Client) DeepCopy() *Client { - if in == nil { - return nil - } - out := new(Client) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Condition) DeepCopyInto(out *Condition) { *out = *in diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index f583f02d8b..f4f074cf60 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -283,9 +283,13 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { } func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { - + if apiKey.ClientSecret == "" { + return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")} + } allErrs := field.ErrorList{} + allErrs = append(allErrs, validateSecretName(apiKey.ClientSecret, fieldPath.Child("clientSecret"))...) + return allErrs } From 4d2ee9da045fe3200ba29830d650c836a887142c Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 24 May 2024 09:20:27 +0000 Subject: [PATCH 07/36] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- examples/custom-resources/api-key/api-key-policy-2.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml index 45045fc43f..7ecf2452fc 100644 --- a/examples/custom-resources/api-key/api-key-policy-2.yaml +++ b/examples/custom-resources/api-key/api-key-policy-2.yaml @@ -13,4 +13,3 @@ spec: notSupplied: 408 noMatch: 410 clientSecret: api-key-client-secret-2 - From c529d8aa9efae414b845b1c9917ab4f5dca23c37 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 24 May 2024 16:41:49 +0100 Subject: [PATCH 08/36] remove hardcoded variable --- internal/configs/version2/http.go | 1 + .../configs/version2/nginx-plus.virtualserver.tmpl | 10 +++++----- internal/configs/version2/nginx.virtualserver.tmpl | 10 +++++----- internal/configs/virtualserver.go | 11 ++++++++--- 4 files changed, 19 insertions(+), 13 deletions(-) diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index dec49bb3d7..18e8338d5b 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -144,6 +144,7 @@ type APIKey struct { RejectCodeNotSupplied int RejectCodeNoMatch int Clients []Client + MapName string } type Client struct { diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 68e916770b..a16038332a 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -261,9 +261,9 @@ server { set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; js_set $hash_fn_server apikey_auth.hash; - js_var $apikey_auth_token $hash_fn_server; + js_var $apikey_auth_token_spec $hash_fn_server; - set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy; #TODO: dont hardcode variable + set $apikey_auth_local_map {{ .MapName }}; auth_request /_validate_apikey_njs; {{- range $header := .Header}} @@ -483,11 +483,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; js_set $hash_fn_location apikey_auth.hash; - js_var $apikey_auth_token $hash_fn_location; + js_var $apikey_auth_token_route $hash_fn_location; - set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy_2; #TODO: dont hardcode variable + set $apikey_auth_local_map {{ .MapName }}; auth_request /_validate_apikey_njs; {{- range $header := .Header}} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index c043756487..04ea966b30 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -161,9 +161,9 @@ server { set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; js_set $hash_fn_server apikey_auth.hash; - js_var $apikey_auth_token $hash_fn_server; + js_var $apikey_auth_token_spec $hash_fn_server; - set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy; #TODO: dont hardcode variable + set $apikey_auth_local_map {{ .MapName }}; auth_request /_validate_apikey_njs; {{- range $header := .Header}} @@ -290,11 +290,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; js_set $hash_fn_location apikey_auth.hash; - js_var $apikey_auth_token $hash_fn_location; + js_var $apikey_auth_token_route $hash_fn_location; - set $apikey_auth_local_map $apikey_auth_client_name_api_key_policy_2; #TODO: dont hardcode variable + set $apikey_auth_local_map {{ .MapName }}; auth_request /_validate_apikey_njs; {{- range $header := .Header}} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index c0e35fb845..a213bfe1fe 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1302,6 +1302,7 @@ func (p *policiesCfg) addAPIKeyConfig( polKey string, polNamespace string, secretRefs map[string]*secrets.SecretReference, + context string, ) (*validationResults, *version2.Map) { res := newValidationResults() var clients []version2.Client @@ -1360,9 +1361,12 @@ func (p *policiesCfg) addAPIKeyConfig( }) } + sourceName := "$apikey_auth_token_" + context + mapName := fmt.Sprintf("$apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) + shaToClient := &version2.Map{ - Source: "$apikey_auth_token", - Variable: strings.Replace(fmt.Sprintf("$apikey_auth_client_name_%s", strings.Split(polKey, "/")[1]), "-", "_", -1), + Source: sourceName, + Variable: mapName, Parameters: params, } @@ -1372,6 +1376,7 @@ func (p *policiesCfg) addAPIKeyConfig( RejectCodeNotSupplied: apiKey.RejectCodes.NotSupplied, RejectCodeNoMatch: apiKey.RejectCodes.NoMatch, Clients: clients, + MapName: mapName, } return res, shaToClient @@ -1509,7 +1514,7 @@ func (vsc *virtualServerConfigurator) generatePolicies( case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: - res, shaToClientMap := config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) + res, shaToClientMap := config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs, context) // TODO: refactor if res != nil && len(res.warnings) > 0 { vsc.addWarnings(ownerDetails.owner, res.warnings) From 7a579a9c56349683b8e2930b23ad31335ed2fc8a Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 24 May 2024 15:42:08 +0000 Subject: [PATCH 09/36] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- 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 a16038332a..e88e88fc26 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -487,7 +487,7 @@ server { js_set $hash_fn_location apikey_auth.hash; js_var $apikey_auth_token_route $hash_fn_location; - set $apikey_auth_local_map {{ .MapName }}; + set $apikey_auth_local_map {{ .MapName }}; auth_request /_validate_apikey_njs; {{- range $header := .Header}} From 30c019dc9ef9a9c7398e025963e754d3086226b4 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Tue, 28 May 2024 10:07:58 +0100 Subject: [PATCH 10/36] add updated njs --- internal/configs/njs/apikey_auth.js | 61 +++++++++++++------ .../version2/nginx-plus.virtualserver.tmpl | 33 +++++----- .../configs/version2/nginx.virtualserver.tmpl | 32 ++++++---- 3 files changed, 80 insertions(+), 46 deletions(-) diff --git a/internal/configs/njs/apikey_auth.js b/internal/configs/njs/apikey_auth.js index 787e53b8b3..9359df9249 100644 --- a/internal/configs/njs/apikey_auth.js +++ b/internal/configs/njs/apikey_auth.js @@ -1,25 +1,48 @@ const c = require("crypto"); -function hash(r) { - const header_query_value = r.variables.header_query_value; - const hashed_value = c - .createHash("sha256") - .update(header_query_value) - .digest("hex"); - return hashed_value; +function hash_route(r) { + const header_query_value = r.variables.header_query_value_route; + const hashed_value = c.createHash('sha256').update(header_query_value).digest('hex'); + return hashed_value; } -function validate(r) { - const client_name = r.variables["apikey_auth_local_map"]; - const header_query_value = r.variables.header_query_value; - - if (!header_query_value) { - r.return(401, "401"); - } else if (!client_name) { - r.return(403, "403"); - } else { - r.return(204, "204"); - } +function hash_spec(r) { + const header_query_value = r.variables.header_query_value_spec; + const hashed_value = c.createHash('sha256').update(header_query_value).digest('hex'); + return hashed_value; } -export default { validate, hash }; + + +function validate_route(r) { + const client_name = r.variables['apikey_auth_local_map_route']; + const header_query_value = r.variables.header_query_value_route; + + if (!header_query_value) { + r.return(401, "401") + } + else if (!client_name) { + r.return(403, "403") + } + else { + r.return(204, "204"); + } +} + +function validate_spec(r) { + const client_name = r.variables['apikey_auth_local_map_spec']; + const header_query_value = r.variables.header_query_value_spec; + + if (!header_query_value) { + r.return(401, "401") + } + else if (!client_name) { + r.return(403, "403") + } + else { + r.return(204, "204"); + } + +} + +export default { validate_route, validate_spec, hash_route, hash_spec }; diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index e88e88fc26..625cd260df 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -226,9 +226,16 @@ server { # TODO: Do this conditionally - location = /_validate_apikey_njs { + location = /_validate_apikey_njs_spec { internal; - js_content apikey_auth.validate; + js_content apikey_auth.validate_spec; + } + + + # TODO: Do this conditionally + location = /_validate_apikey_njs_route { + internal; + js_content apikey_auth.validate_route; } {{- with $s.BasicAuth }} @@ -259,13 +266,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; - js_set $hash_fn_server apikey_auth.hash; - js_var $apikey_auth_token_spec $hash_fn_server; - - set $apikey_auth_local_map {{ .MapName }}; - - auth_request /_validate_apikey_njs; + set $header_query_value_spec {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hashed_variable_spec apikey_auth.hash_spec; + js_var $apikey_auth_token_spec $hashed_variable_spec; + set $apikey_auth_local_map_spec {{ .MapName }}; + auth_request /_validate_apikey_njs_spec; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} @@ -483,13 +488,13 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_set $hash_fn_location apikey_auth.hash; - js_var $apikey_auth_token_route $hash_fn_location; + set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; + js_set $hashed_variable_route apikey_auth.hash_route; + js_var $apikey_auth_token_route $hashed_variable_route; - set $apikey_auth_local_map {{ .MapName }}; + set $apikey_auth_local_map_route {{ .MapName }}; - auth_request /_validate_apikey_njs; + auth_request /_validate_apikey_njs_route; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 04ea966b30..7757589ba1 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -144,9 +144,16 @@ server { {{- end }} # TODO: Do this conditionally - location = /_validate_apikey_njs { + location = /_validate_apikey_njs_spec { internal; - js_content apikey_auth.validate; + js_content apikey_auth.validate_spec; + } + + + # TODO: Do this conditionally + location = /_validate_apikey_njs_route { + internal; + js_content apikey_auth.validate_route; } {{- with $s.BasicAuth }} @@ -159,13 +166,12 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; - js_set $hash_fn_server apikey_auth.hash; - js_var $apikey_auth_token_spec $hash_fn_server; - - set $apikey_auth_local_map {{ .MapName }}; + set $header_query_value_spec {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_set $hashed_variable_spec apikey_auth.hash_spec; + js_var $apikey_auth_token_spec $hashed_variable_spec; + set $apikey_auth_local_map_spec {{ .MapName }}; + auth_request /_validate_apikey_njs_spec; - auth_request /_validate_apikey_njs; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} @@ -290,13 +296,13 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_set $hash_fn_location apikey_auth.hash; - js_var $apikey_auth_token_route $hash_fn_location; + set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; + js_set $hashed_variable_route apikey_auth.hash_route; + js_var $apikey_auth_token_route $hashed_variable_route; - set $apikey_auth_local_map {{ .MapName }}; + set $apikey_auth_local_map_route {{ .MapName }}; - auth_request /_validate_apikey_njs; + auth_request /_validate_apikey_njs_route; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} From 264332002e3aa72f28251c179fda36e3b46bd357 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Tue, 28 May 2024 10:35:51 +0100 Subject: [PATCH 11/36] move js set outside location --- internal/configs/version2/nginx-plus.virtualserver.tmpl | 4 +++- internal/configs/version2/nginx.virtualserver.tmpl | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 625cd260df..7cff676781 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -238,6 +238,9 @@ server { js_content apikey_auth.validate_route; } + # TODO: do this conditionally (if there is an apikey policy in a location) + js_set $hashed_variable_route apikey_auth.hash_route; + {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; @@ -489,7 +492,6 @@ server { # no match: {{ .RejectCodeNoMatch }} set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_set $hashed_variable_route apikey_auth.hash_route; js_var $apikey_auth_token_route $hashed_variable_route; set $apikey_auth_local_map_route {{ .MapName }}; diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 7757589ba1..c605fb4622 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -156,6 +156,10 @@ server { js_content apikey_auth.validate_route; } + + # TODO: do this conditionally (if there is an apikey policy in a location) + js_set $hashed_variable_route apikey_auth.hash_route; + {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; @@ -297,7 +301,6 @@ server { # no match: {{ .RejectCodeNoMatch }} set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_set $hashed_variable_route apikey_auth.hash_route; js_var $apikey_auth_token_route $hashed_variable_route; set $apikey_auth_local_map_route {{ .MapName }}; From c4b0bdfca5bf12c6e356b6e5aeff03e6d7ff4efd Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Wed, 29 May 2024 17:26:49 +0100 Subject: [PATCH 12/36] use nginx.org/apikey secret type Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- .../api-key/api-key-policy.yaml | 6 +- .../api-key/api-key-secret-1.yaml | 2 +- .../api-key/api-key-secret-2.yaml | 2 +- examples/custom-resources/api-key/cafe-2.yaml | 65 +++++++++++++++++++ .../api-key/cafe-virtual-server-2.yaml | 26 ++++++++ .../api-key/cafe-virtual-server.yaml | 15 +---- .../api-key/coffee-virtual-server-route.yaml | 17 +++++ .../api-key/tea-virtual-server-route.yaml | 14 ++++ internal/configs/configurator.go | 6 +- internal/configs/virtualserver.go | 13 ++-- internal/k8s/controller.go | 5 ++ internal/k8s/secrets/validation.go | 16 +++-- pkg/apis/configuration/v1/types.go | 4 +- .../configuration/v1/zz_generated.deepcopy.go | 28 +++++++- pkg/apis/configuration/validation/policy.go | 1 + 15 files changed, 189 insertions(+), 31 deletions(-) create mode 100644 examples/custom-resources/api-key/cafe-2.yaml create mode 100644 examples/custom-resources/api-key/cafe-virtual-server-2.yaml create mode 100644 examples/custom-resources/api-key/coffee-virtual-server-route.yaml create mode 100644 examples/custom-resources/api-key/tea-virtual-server-route.yaml diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml index 68a88f7a70..2c12b38923 100644 --- a/examples/custom-resources/api-key/api-key-policy.yaml +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -11,7 +11,7 @@ spec: - "some-other-header" query: - "queryName" - rejectCodes: # optional - notSupplied: 401 - noMatch: 403 +# rejectCodes: # optional +# notSupplied: 401 +# noMatch: 403 clientSecret: api-key-client-secret-1 diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml index 2a71600625..318529da6a 100644 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -2,7 +2,7 @@ apiVersion: v1 kind: Secret metadata: name: api-key-client-secret-1 -type: Opaque +type: nginx.org/apikey data: client1: cGFzc3dvcmQ= client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk diff --git a/examples/custom-resources/api-key/api-key-secret-2.yaml b/examples/custom-resources/api-key/api-key-secret-2.yaml index e175e817b0..bab71a1872 100644 --- a/examples/custom-resources/api-key/api-key-secret-2.yaml +++ b/examples/custom-resources/api-key/api-key-secret-2.yaml @@ -2,6 +2,6 @@ apiVersion: v1 kind: Secret metadata: name: api-key-client-secret-2 -type: Opaque +type: nginx.org/apikey data: client1: cGFzc3dvcmQ= diff --git a/examples/custom-resources/api-key/cafe-2.yaml b/examples/custom-resources/api-key/cafe-2.yaml new file mode 100644 index 0000000000..f049e8bf29 --- /dev/null +++ b/examples/custom-resources/api-key/cafe-2.yaml @@ -0,0 +1,65 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: coffee +spec: + replicas: 2 + selector: + matchLabels: + app: coffee + template: + metadata: + labels: + app: coffee + spec: + containers: + - name: coffee + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: coffee-svc +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: coffee +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + name: tea +spec: + replicas: 1 + selector: + matchLabels: + app: tea + template: + metadata: + labels: + app: tea + spec: + containers: + - name: tea + image: nginxdemos/nginx-hello:plain-text + ports: + - containerPort: 8080 +--- +apiVersion: v1 +kind: Service +metadata: + name: tea-svc +spec: + ports: + - port: 80 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: tea diff --git a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml new file mode 100644 index 0000000000..db55ebb5fc --- /dev/null +++ b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml @@ -0,0 +1,26 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServer +metadata: + name: cafe-2 +spec: + host: cafe-2.example.com + tls: + secret: cafe-secret + policies: + - name: api-key-policy + upstreams: + - name: tea + service: tea-svc + port: 80 + - name: coffee + service: coffee-svc + port: 80 + routes: + - path: /tea + action: + pass: tea + - path: /coffee + action: + pass: coffee + policies: + - name: api-key-policy-2 diff --git a/examples/custom-resources/api-key/cafe-virtual-server.yaml b/examples/custom-resources/api-key/cafe-virtual-server.yaml index d4b4ca1a9f..ecc0b61b6f 100644 --- a/examples/custom-resources/api-key/cafe-virtual-server.yaml +++ b/examples/custom-resources/api-key/cafe-virtual-server.yaml @@ -8,19 +8,8 @@ spec: secret: cafe-secret policies: - name: api-key-policy - upstreams: - - name: tea - service: tea-svc - port: 80 - - name: coffee - service: coffee-svc - port: 80 routes: - path: /tea - action: - pass: tea + route: default/tea - path: /coffee - action: - pass: coffee - policies: - - name: api-key-policy-2 + route: default/coffee diff --git a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml new file mode 100644 index 0000000000..456da973dd --- /dev/null +++ b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml @@ -0,0 +1,17 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServerRoute +metadata: + name: coffee +spec: + host: cafe.example.com + upstreams: + - name: coffee + service: coffee-svc + port: 80 + subroutes: + - path: /coffee + action: + pass: coffee + policies: + - name: api-key-policy-2 + diff --git a/examples/custom-resources/api-key/tea-virtual-server-route.yaml b/examples/custom-resources/api-key/tea-virtual-server-route.yaml new file mode 100644 index 0000000000..db445c0280 --- /dev/null +++ b/examples/custom-resources/api-key/tea-virtual-server-route.yaml @@ -0,0 +1,14 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServerRoute +metadata: + name: tea +spec: + host: cafe.example.com + upstreams: + - name: tea + service: tea-svc + port: 80 + subroutes: + - path: /tea + action: + pass: tea diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 1d569c4617..69ce005fbc 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -1990,8 +1990,10 @@ func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) string { case secrets.SecretTypeOIDC: // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. return "" - case api_v1.SecretTypeOpaque: - glog.Infof(secret.Name) + case secrets.SecretTypeAPIKey: + glog.Infof("adding apikey secret: %s", secret.Name) + // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. + return "" default: return cnf.addOrUpdateTLSSecret(secret) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index a213bfe1fe..2e925403f0 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -424,6 +424,10 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policiesCfg.JWTAuthList = make(map[string]*version2.JWTAuth) policiesCfg.JWTAuthList[jwtAuthKey] = policiesCfg.JWTAuth } + // + //if policiesCfg.APIKey { + // apiKey := policiesCfg.APIKey.MapName + //} dosCfg := generateDosCfg(dosResources[""]) @@ -867,6 +871,7 @@ type policiesCfg struct { EgressMTLS *version2.EgressMTLS OIDC bool APIKey *version2.APIKey + APIKeyList map[string]*version2.APIKey WAF *version2.WAF ErrorReturn *version2.Return BundleValidator bundleValidator @@ -1325,8 +1330,8 @@ func (p *policiesCfg) addAPIKeyConfig( if secretRef.Secret != nil { secretType = secretRef.Secret.Type } - if secretType != "" && secretType != api_v1.SecretTypeOpaque { - res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, api_v1.SecretTypeOpaque) + if secretType != "" && secretType != secrets.SecretTypeAPIKey { + res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeAPIKey) res.isError = true return res, nil } else if secretRef.Error != nil { @@ -1373,8 +1378,8 @@ func (p *policiesCfg) addAPIKeyConfig( p.APIKey = &version2.APIKey{ Header: apiKey.SuppliedIn.Header, Query: apiKey.SuppliedIn.Query, - RejectCodeNotSupplied: apiKey.RejectCodes.NotSupplied, - RejectCodeNoMatch: apiKey.RejectCodes.NoMatch, + RejectCodeNotSupplied: generateIntFromPointer(apiKey.RejectCodes.NotSupplied, 401), + RejectCodeNoMatch: generateIntFromPointer(apiKey.RejectCodes.NoMatch, 403), Clients: clients, MapName: mapName, } diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 3304dad9cf..9bd4374e84 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -3331,6 +3331,11 @@ func (lbc *LoadBalancerController) createVirtualServerEx(virtualServer *conf_v1. glog.Warningf("Error getting OIDC secrets for VirtualServerRoute %v/%v: %v", vsr.Namespace, vsr.Name, err) } + err = lbc.addAPIKeySecretRefs(virtualServerEx.SecretRefs, vsrSubroutePolicies) + if err != nil { + glog.Warningf("Error getting APIKey secrets for VirtualServerRoute %v/%v: %v", vsr.Namespace, vsr.Name, err) + } + err = lbc.addWAFPolicyRefs(virtualServerEx.ApPolRefs, virtualServerEx.LogConfRefs, vsrSubroutePolicies) if err != nil { glog.Warningf("Error getting WAF policies for VirtualServerRoute %v/%v: %v", vsr.Namespace, vsr.Name, err) diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 518b7923bb..3d0ed28169 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -34,6 +34,9 @@ const SecretTypeOIDC api_v1.SecretType = "nginx.org/oidc" //nolint:gosec // G101 // SecretTypeHtpasswd contains an htpasswd file for use in HTTP Basic authorization.. #nosec G101 const SecretTypeHtpasswd api_v1.SecretType = "nginx.org/htpasswd" // #nosec G101 +// SecretTypeAPIKey contains a list of client ID and key for API key authorization.. #nosec G101 +const SecretTypeAPIKey api_v1.SecretType = "nginx.org/apikey" // #nosec G101 + // ValidateTLSSecret validates the secret. If it is valid, the function returns nil. func ValidateTLSSecret(secret *api_v1.Secret) error { if secret.Type != api_v1.SecretTypeTLS { @@ -110,9 +113,13 @@ func ValidateOIDCSecret(secret *api_v1.Secret) error { } func ValidateAPIKeySecret(secret *api_v1.Secret) error { - if secret.Type != api_v1.SecretTypeOpaque { - return fmt.Errorf("APIKey secret must be of the type %v", SecretTypeOIDC) + if secret.Type != SecretTypeAPIKey { + return fmt.Errorf("APIKey secret must be of the type %v", SecretTypeAPIKey) } + //clientSecret, exists := secret.Data[ClientSecretKey] + //if !exists { + // return fmt.Errorf("OIDC secret must have the data field %v", ClientSecretKey) + //} return nil } @@ -139,7 +146,8 @@ func IsSupportedSecretType(secretType api_v1.SecretType) bool { secretType == SecretTypeJWK || secretType == SecretTypeOIDC || secretType == SecretTypeHtpasswd || - secretType == api_v1.SecretTypeOpaque + secretType == SecretTypeAPIKey + //secretType == api_v1.SecretTypeOpaque } // ValidateSecret validates the secret. If it is valid, the function returns nil. @@ -155,7 +163,7 @@ func ValidateSecret(secret *api_v1.Secret) error { return ValidateOIDCSecret(secret) case SecretTypeHtpasswd: return ValidateHtpasswdSecret(secret) - case api_v1.SecretTypeOpaque: + case SecretTypeAPIKey: return ValidateAPIKeySecret(secret) } diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index f2e470814b..4733e34960 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -689,8 +689,8 @@ type SuppliedIn struct { } type RejectCodes struct { - NotSupplied int `json:"notSupplied"` - NoMatch int `json:"noMatch"` + NotSupplied *int `json:"notSupplied"` + NoMatch *int `json:"noMatch"` } type Client struct { diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 10760491f5..339c9ee38b 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -13,7 +13,7 @@ import ( func (in *APIKey) DeepCopyInto(out *APIKey) { *out = *in in.SuppliedIn.DeepCopyInto(&out.SuppliedIn) - out.RejectCodes = in.RejectCodes + in.RejectCodes.DeepCopyInto(&out.RejectCodes) return } @@ -196,6 +196,22 @@ func (in *CertManager) DeepCopy() *CertManager { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Client) DeepCopyInto(out *Client) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Client. +func (in *Client) DeepCopy() *Client { + if in == nil { + return nil + } + out := new(Client) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Condition) DeepCopyInto(out *Condition) { *out = *in @@ -883,6 +899,16 @@ func (in *RateLimit) DeepCopy() *RateLimit { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *RejectCodes) DeepCopyInto(out *RejectCodes) { *out = *in + if in.NotSupplied != nil { + in, out := &in.NotSupplied, &out.NotSupplied + *out = new(int) + **out = **in + } + if in.NoMatch != nil { + in, out := &in.NoMatch, &out.NoMatch + *out = new(int) + **out = **in + } return } diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index f4f074cf60..d14ffa4423 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -283,6 +283,7 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { } func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { + //if api if apiKey.ClientSecret == "" { return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")} } From be1b05ed46b3ce2ee00c6a353dadce27d6f0c424 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 29 May 2024 16:27:18 +0000 Subject: [PATCH 13/36] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../custom-resources/api-key/coffee-virtual-server-route.yaml | 1 - 1 file changed, 1 deletion(-) diff --git a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml index 456da973dd..d24eca3e1a 100644 --- a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml +++ b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml @@ -14,4 +14,3 @@ spec: pass: coffee policies: - name: api-key-policy-2 - From 2a7f66a8d881f440322ba7be944dcb59cb101058 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 31 May 2024 14:46:04 +0100 Subject: [PATCH 14/36] simplify njs --- internal/configs/njs/apikey_auth.js | 38 ++++--------------- internal/configs/version1/nginx-plus.tmpl | 1 + internal/configs/version1/nginx.tmpl | 1 + .../version2/nginx-plus.virtualserver.tmpl | 33 +++++----------- .../configs/version2/nginx.virtualserver.tmpl | 33 ++++++---------- internal/configs/virtualserver.go | 9 ++--- internal/configs/virtualserver_test.go | 6 +-- internal/k8s/secrets/validation.go | 2 +- pkg/apis/configuration/validation/policy.go | 3 +- 9 files changed, 40 insertions(+), 86 deletions(-) diff --git a/internal/configs/njs/apikey_auth.js b/internal/configs/njs/apikey_auth.js index 9359df9249..459dba9cf8 100644 --- a/internal/configs/njs/apikey_auth.js +++ b/internal/configs/njs/apikey_auth.js @@ -1,37 +1,15 @@ -const c = require("crypto"); +const c = require('crypto') -function hash_route(r) { - const header_query_value = r.variables.header_query_value_route; +function hash(r) { + const header_query_value = r.variables.header_query_value; const hashed_value = c.createHash('sha256').update(header_query_value).digest('hex'); return hashed_value; } -function hash_spec(r) { - const header_query_value = r.variables.header_query_value_spec; - const hashed_value = c.createHash('sha256').update(header_query_value).digest('hex'); - return hashed_value; -} - - - -function validate_route(r) { - const client_name = r.variables['apikey_auth_local_map_route']; - const header_query_value = r.variables.header_query_value_route; - - if (!header_query_value) { - r.return(401, "401") - } - else if (!client_name) { - r.return(403, "403") - } - else { - r.return(204, "204"); - } -} - -function validate_spec(r) { - const client_name = r.variables['apikey_auth_local_map_spec']; - const header_query_value = r.variables.header_query_value_spec; +function validate(r) { + const client_name_map = r.variables['apikey_auth_local_map']; + const client_name = r.variables[client_name_map]; + const header_query_value = r.variables.header_query_value; if (!header_query_value) { r.return(401, "401") @@ -45,4 +23,4 @@ function validate_spec(r) { } -export default { validate_route, validate_spec, hash_route, hash_spec }; +export default { validate, hash }; diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index 2de3a3d9e0..a0bb9a78a0 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -40,6 +40,7 @@ http { map_hash_bucket_size {{.MapHashBucketSize}}; js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + js_set $apikey_auth_hash apikey_auth.hash; {{- if .HTTPSnippets}} {{range $value := .HTTPSnippets}} diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index ba268e36dc..fc298fd814 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -35,6 +35,7 @@ http { js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + js_set $apikey_auth_hash apikey_auth.hash; {{- if .HTTPSnippets}} {{range $value := .HTTPSnippets}} diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 7cff676781..162c82df1b 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -224,23 +224,12 @@ server { } {{- end }} - - # TODO: Do this conditionally - location = /_validate_apikey_njs_spec { - internal; - js_content apikey_auth.validate_spec; - } - - # TODO: Do this conditionally - location = /_validate_apikey_njs_route { + location = /_validate_apikey_njs { internal; - js_content apikey_auth.validate_route; + js_content apikey_auth.validate; } - # TODO: do this conditionally (if there is an apikey policy in a location) - js_set $hashed_variable_route apikey_auth.hash_route; - {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; @@ -269,11 +258,10 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value_spec {{ makeHeaderQueryValue $s.APIKey | printf }}; - js_set $hashed_variable_spec apikey_auth.hash_spec; - js_var $apikey_auth_token_spec $hashed_variable_spec; - set $apikey_auth_local_map_spec {{ .MapName }}; - auth_request /_validate_apikey_njs_spec; + js_var $apikey_auth_local_map "{{ .MapName}}"; + js_var $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_var $apikey_auth_token $apikey_auth_hash; + auth_request /_validate_apikey_njs; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} @@ -491,12 +479,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_var $apikey_auth_token_route $hashed_variable_route; - - set $apikey_auth_local_map_route {{ .MapName }}; + set $apikey_auth_local_map "{{ .MapName }}"; + set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; + set $apikey_auth_token $apikey_auth_hash; + auth_request /_validate_apikey_njs; - auth_request /_validate_apikey_njs_route; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index c605fb4622..e65014e9f3 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -144,22 +144,12 @@ server { {{- end }} # TODO: Do this conditionally - location = /_validate_apikey_njs_spec { + location = /_validate_apikey_njs { internal; - js_content apikey_auth.validate_spec; + js_content apikey_auth.validate; } - # TODO: Do this conditionally - location = /_validate_apikey_njs_route { - internal; - js_content apikey_auth.validate_route; - } - - - # TODO: do this conditionally (if there is an apikey policy in a location) - js_set $hashed_variable_route apikey_auth.hash_route; - {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; auth_basic_user_file {{ .Secret }}; @@ -170,11 +160,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value_spec {{ makeHeaderQueryValue $s.APIKey | printf }}; - js_set $hashed_variable_spec apikey_auth.hash_spec; - js_var $apikey_auth_token_spec $hashed_variable_spec; - set $apikey_auth_local_map_spec {{ .MapName }}; - auth_request /_validate_apikey_njs_spec; + js_var $apikey_auth_local_map "{{ .MapName}}"; + js_var $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + js_var $apikey_auth_token $apikey_auth_hash; + auth_request /_validate_apikey_njs; + {{- range $header := .Header}} # header: {{ $header }} @@ -300,12 +290,11 @@ server { # not supplied: {{ .RejectCodeNotSupplied }} # no match: {{ .RejectCodeNoMatch }} - set $header_query_value_route {{ makeHeaderQueryValue $l.APIKey | printf }}; - js_var $apikey_auth_token_route $hashed_variable_route; - - set $apikey_auth_local_map_route {{ .MapName }}; + set $apikey_auth_local_map "{{ .MapName }}"; + set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; + set $apikey_auth_token $apikey_auth_hash; + auth_request /_validate_apikey_njs; - auth_request /_validate_apikey_njs_route; {{- range $header := .Header}} # header: {{ $header }} {{- end -}} diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 2e925403f0..6c06c54678 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1320,7 +1320,7 @@ func (p *policiesCfg) addAPIKeyConfig( return res, nil } - //if apiKey.ClientSecret != "" { + // if apiKey.ClientSecret != "" { secretKey := fmt.Sprintf("%v/%v", polNamespace, apiKey.ClientSecret) glog.Infof("secretKey: %v", secretKey) secretRef := secretRefs[secretKey] @@ -1366,12 +1366,12 @@ func (p *policiesCfg) addAPIKeyConfig( }) } - sourceName := "$apikey_auth_token_" + context - mapName := fmt.Sprintf("$apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) + sourceName := "$apikey_auth_token" + mapName := fmt.Sprintf("apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) shaToClient := &version2.Map{ Source: sourceName, - Variable: mapName, + Variable: fmt.Sprintf("$%s", mapName), Parameters: params, } @@ -1384,7 +1384,6 @@ func (p *policiesCfg) addAPIKeyConfig( MapName: mapName, } return res, shaToClient - } func (p *policiesCfg) addWAFConfig( diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index c3951b16b6..b073f0e271 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6285,7 +6285,7 @@ func TestGeneratePolicies(t *testing.T) { for _, test := range tests { result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) - //TODO test maps + // TODO test maps println(maps) // temporary print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { @@ -6380,7 +6380,7 @@ func TestGeneratePolicies_GeneratesWAFPolicyOnValidApBundle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) res, maps := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) - //TODO test maps + // TODO test maps println(maps) // temporary print to use the variable res.BundleValidator = nil if !cmp.Equal(tc.want, res) { @@ -7727,7 +7727,7 @@ func TestGeneratePoliciesFails(t *testing.T) { } result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) - //TODO test maps + // TODO test maps println(maps) // Temporty print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 3d0ed28169..53e69b46f0 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -147,7 +147,7 @@ func IsSupportedSecretType(secretType api_v1.SecretType) bool { secretType == SecretTypeOIDC || secretType == SecretTypeHtpasswd || secretType == SecretTypeAPIKey - //secretType == api_v1.SecretTypeOpaque + // secretType == api_v1.SecretTypeOpaque } // ValidateSecret validates the secret. If it is valid, the function returns nil. diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index d14ffa4423..8d845d42d2 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -283,7 +283,7 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { } func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { - //if api + // if api if apiKey.ClientSecret == "" { return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")} } @@ -292,7 +292,6 @@ func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { allErrs = append(allErrs, validateSecretName(apiKey.ClientSecret, fieldPath.Child("clientSecret"))...) return allErrs - } func validateWAF(waf *v1.WAF, fieldPath *field.Path) field.ErrorList { From 083df0615181d375bfeef83fe826afc051a0ff15 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 31 May 2024 15:56:51 +0100 Subject: [PATCH 15/36] make query params work --- internal/configs/version2/nginx-plus.virtualserver.tmpl | 6 +++++- internal/configs/version2/nginx.virtualserver.tmpl | 5 ++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 162c82df1b..c217c08c49 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -259,7 +259,6 @@ server { # no match: {{ .RejectCodeNoMatch }} js_var $apikey_auth_local_map "{{ .MapName}}"; - js_var $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; js_var $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; {{- range $header := .Header}} @@ -494,6 +493,11 @@ server { # client: {{ $client.ClientID }} # secret: {{ $client.EncryptedKey }} {{- end }} + {{- else }} + {{- with $s.APIKey }} + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + {{- end }} + {{- end }} {{- with $l.WAF }} diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index e65014e9f3..a1b527f5fb 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -161,7 +161,6 @@ server { # no match: {{ .RejectCodeNoMatch }} js_var $apikey_auth_local_map "{{ .MapName}}"; - js_var $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; js_var $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; @@ -305,6 +304,10 @@ server { # client: {{ $client.ClientID }} # secret: {{ $client.EncryptedKey }} {{- end }} + {{- else }} + {{- with $s.APIKey }} + set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; + {{- end }} {{- end }} {{ $proxyOrGRPC := "proxy" }}{{ if $l.GRPCPass }}{{ $proxyOrGRPC = "grpc" }}{{ end }} From 512e27b4c5e268568ca988320f2ea33a1bad1617 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 31 May 2024 16:43:06 +0100 Subject: [PATCH 16/36] clean up template files --- .../version2/nginx-plus.virtualserver.tmpl | 29 ------------------ .../configs/version2/nginx.virtualserver.tmpl | 30 ------------------- 2 files changed, 59 deletions(-) diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index c217c08c49..7cd2da4a20 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -254,23 +254,9 @@ server { {{- end }} {{- with $s.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} - js_var $apikey_auth_local_map "{{ .MapName}}"; js_var $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; - {{- range $header := .Header}} - # header: {{ $header }} - {{- end -}} - {{- range $query := .Query }} - # query: {{ $query }} - {{- end -}} - {{- range $client := .Clients }} - # client: {{ $client.ClientID }} - # secret: {{ $client.EncryptedKey }} - {{- end }} {{- end }} {{- with $s.WAF }} @@ -474,25 +460,10 @@ server { {{- with $l.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} - set $apikey_auth_local_map "{{ .MapName }}"; set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; set $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; - - {{- range $header := .Header}} - # header: {{ $header }} - {{- end -}} - {{- range $query := .Query }} - # query: {{ $query }} - {{- end -}} - {{- range $client := .Clients }} - # client: {{ $client.ClientID }} - # secret: {{ $client.EncryptedKey }} - {{- end }} {{- else }} {{- with $s.APIKey }} set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index a1b527f5fb..7e22b35d68 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -156,25 +156,9 @@ server { {{- end }} {{- with $s.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} - js_var $apikey_auth_local_map "{{ .MapName}}"; js_var $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; - - - {{- range $header := .Header}} - # header: {{ $header }} - {{- end -}} - {{- range $query := .Query }} - # query: {{ $query }} - {{- end -}} - {{- range $client := .Clients }} - # client: {{ $client.ClientID }} - # secret: {{ $client.EncryptedKey }} - {{- end }} {{- end }} {{- with $s.EgressMTLS }} @@ -285,25 +269,11 @@ server { {{- end }} {{- with $l.APIKey}} - # API KEY POLICY - # not supplied: {{ .RejectCodeNotSupplied }} - # no match: {{ .RejectCodeNoMatch }} - set $apikey_auth_local_map "{{ .MapName }}"; set $header_query_value {{ makeHeaderQueryValue $l.APIKey | printf }}; set $apikey_auth_token $apikey_auth_hash; auth_request /_validate_apikey_njs; - {{- range $header := .Header}} - # header: {{ $header }} - {{- end -}} - {{- range $query := .Query }} - # query: {{ $query }} - {{- end -}} - {{- range $client := .Clients }} - # client: {{ $client.ClientID }} - # secret: {{ $client.EncryptedKey }} - {{- end }} {{- else }} {{- with $s.APIKey }} set $header_query_value {{ makeHeaderQueryValue $s.APIKey | printf }}; From 573be8039deef6288c06db9a1e22f23f46725dbd Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Thu, 6 Jun 2024 11:35:31 +0100 Subject: [PATCH 17/36] add api key secret validation to reject duplicated keys, remove repeated maps in config, remove reject code, add unit tests Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- .../api-key/api-key-policy-2.yaml | 3 - .../api-key/api-key-policy.yaml | 3 - .../api-key/api-key-secret-1.yaml | 2 +- .../api-key/cafe-virtual-server-2.yaml | 6 + .../api-key/coffee-virtual-server-route.yaml | 5 + internal/configs/configurator.go | 4 +- internal/configs/version1/nginx.tmpl | 1 - internal/configs/version2/http.go | 14 +- .../version2/nginx-plus.virtualserver.tmpl | 2 - internal/configs/virtualserver.go | 145 ++++++++++-------- internal/configs/virtualserver_test.go | 12 +- internal/k8s/secrets/validation.go | 14 +- internal/k8s/secrets/validation_test.go | 65 ++++++++ pkg/apis/configuration/validation/policy.go | 26 +++- 14 files changed, 200 insertions(+), 102 deletions(-) diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml index 7ecf2452fc..2870eb1716 100644 --- a/examples/custom-resources/api-key/api-key-policy-2.yaml +++ b/examples/custom-resources/api-key/api-key-policy-2.yaml @@ -9,7 +9,4 @@ spec: - "X-header-name" query: - "queryName" - rejectCodes: # optional - notSupplied: 408 - noMatch: 410 clientSecret: api-key-client-secret-2 diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml index 2c12b38923..30c45bbd3b 100644 --- a/examples/custom-resources/api-key/api-key-policy.yaml +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -11,7 +11,4 @@ spec: - "some-other-header" query: - "queryName" -# rejectCodes: # optional -# notSupplied: 401 -# noMatch: 403 clientSecret: api-key-client-secret-1 diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml index 318529da6a..bf6954f4b0 100644 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -5,4 +5,4 @@ metadata: type: nginx.org/apikey data: client1: cGFzc3dvcmQ= - client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk + client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk \ No newline at end of file diff --git a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml index db55ebb5fc..7cb56276f1 100644 --- a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml +++ b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml @@ -24,3 +24,9 @@ spec: pass: coffee policies: - name: api-key-policy-2 + - path: /coffee2 + action: + pass: coffee + policies: + - name: api-key-policy + diff --git a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml index d24eca3e1a..5b921011c7 100644 --- a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml +++ b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml @@ -14,3 +14,8 @@ spec: pass: coffee policies: - name: api-key-policy-2 + - path: /coffee2 + action: + pass: coffee + policies: + - name: api-key-policy \ No newline at end of file diff --git a/internal/configs/configurator.go b/internal/configs/configurator.go index 69ce005fbc..9d99c245c2 100644 --- a/internal/configs/configurator.go +++ b/internal/configs/configurator.go @@ -1991,9 +1991,7 @@ func (cnf *Configurator) AddOrUpdateSecret(secret *api_v1.Secret) string { // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. return "" case secrets.SecretTypeAPIKey: - glog.Infof("adding apikey secret: %s", secret.Name) - // OIDC ClientSecret is not required on the filesystem, it is written directly to the config file. - + // APIKey ClientSecret is not required on the filesystem, it is written directly to the config file. return "" default: return cnf.addOrUpdateTLSSecret(secret) diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index fc298fd814..9d9db599d6 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -1,5 +1,4 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version1.MainConfig*/ -}} - worker_processes {{.WorkerProcesses}}; {{- if .WorkerRlimitNofile}} worker_rlimit_nofile {{.WorkerRlimitNofile}};{{end}} diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 18e8338d5b..bd72ae6a9f 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -139,17 +139,9 @@ type OIDC struct { } type APIKey struct { - Header []string - Query []string - RejectCodeNotSupplied int - RejectCodeNoMatch int - Clients []Client - MapName string -} - -type Client struct { - ClientID string - EncryptedKey string + Header []string + Query []string + MapName string } // WAF defines WAF configuration. diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index 7cd2da4a20..bbf5bb76f7 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -1,6 +1,4 @@ {{- /*gotype: github.com/nginxinc/kubernetes-ingress/internal/configs/version2.VirtualServerConfig*/ -}} - - {{ range $u := .Upstreams }} upstream {{ $u.Name }} { zone {{ $u.Name }} {{ if ne $u.UpstreamZoneSize "0" }}{{ $u.UpstreamZoneSize }}{{ else }}512k{{ end }}; diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 6c06c54678..ba79a4b5a7 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -416,18 +416,18 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - policiesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) - - maps = append(maps, mapsFromPolicies...) + policiesCfg := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) if policiesCfg.JWKSAuthEnabled { jwtAuthKey := policiesCfg.JWTAuth.Key policiesCfg.JWTAuthList = make(map[string]*version2.JWTAuth) policiesCfg.JWTAuthList[jwtAuthKey] = policiesCfg.JWTAuth } - // - //if policiesCfg.APIKey { - // apiKey := policiesCfg.APIKey.MapName - //} + + if policiesCfg.APIKeyEnabled { + apiMapName := policiesCfg.APIKey.MapName + policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + policiesCfg.APIKeyClientMap[apiMapName] = policiesCfg.APIKeyClients + } dosCfg := generateDosCfg(dosResources[""]) @@ -571,9 +571,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( vsNamespace: vsEx.VirtualServer.Namespace, vsName: vsEx.VirtualServer.Name, } - routePoliciesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, r.Policies, vsEx.Policies, routeContext, policyOpts) - maps = append(maps, mapsFromPolicies...) - + routePoliciesCfg := vsc.generatePolicies(ownerDetails, r.Policies, vsEx.Policies, routeContext, policyOpts) if policiesCfg.OIDC { routePoliciesCfg.OIDC = policiesCfg.OIDC } @@ -589,6 +587,16 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policiesCfg.JWTAuthList[jwtAuthKey] = routePoliciesCfg.JWTAuth } } + if routePoliciesCfg.APIKeyEnabled { + policiesCfg.APIKeyEnabled = routePoliciesCfg.APIKeyEnabled + apiMapName := routePoliciesCfg.APIKey.MapName + if policiesCfg.APIKeyClientMap == nil { + policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + } + if _, exists := policiesCfg.APIKeyClientMap[apiMapName]; !exists { + policiesCfg.APIKeyClientMap[apiMapName] = routePoliciesCfg.APIKeyClients + } + } limitReqZones = append(limitReqZones, routePoliciesCfg.LimitReqZones...) dosRouteCfg := generateDosCfg(dosResources[r.Path]) @@ -703,8 +711,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policyRefs = r.Policies context = subRouteContext } - routePoliciesCfg, mapsFromPolicies := vsc.generatePolicies(ownerDetails, policyRefs, vsEx.Policies, context, policyOpts) - maps = append(maps, mapsFromPolicies...) + routePoliciesCfg := vsc.generatePolicies(ownerDetails, policyRefs, vsEx.Policies, context, policyOpts) if policiesCfg.OIDC { routePoliciesCfg.OIDC = policiesCfg.OIDC } @@ -720,6 +727,17 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policiesCfg.JWTAuthList[jwtAuthKey] = routePoliciesCfg.JWTAuth } } + if routePoliciesCfg.APIKeyEnabled { + policiesCfg.APIKeyEnabled = routePoliciesCfg.APIKeyEnabled + apiMapName := routePoliciesCfg.APIKey.MapName + if policiesCfg.APIKeyClientMap == nil { + policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + } + if _, exists := policiesCfg.APIKeyClientMap[apiMapName]; !exists { + policiesCfg.APIKeyClientMap[apiMapName] = routePoliciesCfg.APIKeyClients + } + } + limitReqZones = append(limitReqZones, routePoliciesCfg.LimitReqZones...) dosRouteCfg := generateDosCfg(dosResources[r.Path]) @@ -787,6 +805,10 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( } } + for mapName, apiKeyClients := range policiesCfg.APIKeyClientMap { + maps = append(maps, *generateAPIKeyClientMap(mapName, apiKeyClients)) + } + httpSnippets := generateSnippets(vsc.enableSnippets, vsEx.VirtualServer.Spec.HTTPSnippets, []string{}) serverSnippets := generateSnippets( vsc.enableSnippets, @@ -870,8 +892,10 @@ type policiesCfg struct { IngressMTLS *version2.IngressMTLS EgressMTLS *version2.EgressMTLS OIDC bool + APIKeyEnabled bool APIKey *version2.APIKey - APIKeyList map[string]*version2.APIKey + APIKeyClients []APIKeyClient + APIKeyClientMap map[string][]APIKeyClient WAF *version2.WAF ErrorReturn *version2.Return BundleValidator bundleValidator @@ -886,6 +910,11 @@ type internalBundleValidator struct { bundlePath string } +type APIKeyClient struct { + ClientID string + HashedKey string +} + func (i internalBundleValidator) validate(bundle string) (string, error) { bundle = path.Join(i.bundlePath, bundle) _, err := os.Stat(bundle) @@ -1308,19 +1337,17 @@ func (p *policiesCfg) addAPIKeyConfig( polNamespace string, secretRefs map[string]*secrets.SecretReference, context string, -) (*validationResults, *version2.Map) { +) *validationResults { res := newValidationResults() - var clients []version2.Client if p.APIKey != nil { res.addWarningf( "Multiple APIKey policies in the same context is not valid. APIKey policy %s will be ignored", polKey, ) res.isError = true - return res, nil + return res } - // if apiKey.ClientSecret != "" { secretKey := fmt.Sprintf("%v/%v", polNamespace, apiKey.ClientSecret) glog.Infof("secretKey: %v", secretKey) secretRef := secretRefs[secretKey] @@ -1333,57 +1360,65 @@ func (p *policiesCfg) addAPIKeyConfig( if secretType != "" && secretType != secrets.SecretTypeAPIKey { res.addWarningf("API Key policy %s references a secret %s of a wrong type '%s', must be '%s'", polKey, secretKey, secretType, secrets.SecretTypeAPIKey) res.isError = true - return res, nil + return res } else if secretRef.Error != nil { res.addWarningf("API Key %s references an invalid secret %s: %v", polKey, secretKey, secretRef.Error) res.isError = true - return res, nil + return res + } + + p.APIKeyClients = generateAPIKeyClients(secretRef.Secret.Data) + + mapName := fmt.Sprintf("apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) + p.APIKey = &version2.APIKey{ + Header: apiKey.SuppliedIn.Header, + Query: apiKey.SuppliedIn.Query, + MapName: mapName, } + p.APIKeyEnabled = true + return res +} - for clientID, clientSecret := range secretRef.Secret.Data { +func generateAPIKeyClients(secretData map[string][]byte) []APIKeyClient { + var clients []APIKeyClient + for clientID, apiKey := range secretData { h := sha256.New() - h.Write([]byte(clientSecret)) + h.Write([]byte(apiKey)) sha256Hash := hex.EncodeToString(h.Sum(nil)) base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) - glog.Infof("clientSecret %s", clientSecret) + glog.Infof("apiKey %s", apiKey) glog.Infof("sha %s", sha256Hash) glog.Infof("base64Str %s", base64Str) - clients = append(clients, version2.Client{ClientID: clientID, EncryptedKey: sha256Hash}) // + clients = append(clients, APIKeyClient{ClientID: clientID, HashedKey: sha256Hash}) // } + return clients +} - default_parameter := version2.Parameter{ +func generateAPIKeyClientMap(mapName string, apiKeyClients []APIKeyClient) *version2.Map { + glog.Infof("mapName: %v, apiKeyClients: %v", mapName, apiKeyClients) + + defaultParam := version2.Parameter{ Value: "default", Result: "\"\"", } - params := []version2.Parameter{default_parameter} - for _, client := range clients { + params := []version2.Parameter{defaultParam} + for _, client := range apiKeyClients { params = append(params, version2.Parameter{ - Value: fmt.Sprintf("\"%s\"", client.EncryptedKey), + Value: fmt.Sprintf("\"%s\"", client.HashedKey), Result: fmt.Sprintf("\"%s\"", client.ClientID), }) } sourceName := "$apikey_auth_token" - mapName := fmt.Sprintf("apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) - shaToClient := &version2.Map{ + return &version2.Map{ Source: sourceName, Variable: fmt.Sprintf("$%s", mapName), Parameters: params, } - - p.APIKey = &version2.APIKey{ - Header: apiKey.SuppliedIn.Header, - Query: apiKey.SuppliedIn.Query, - RejectCodeNotSupplied: generateIntFromPointer(apiKey.RejectCodes.NotSupplied, 401), - RejectCodeNoMatch: generateIntFromPointer(apiKey.RejectCodes.NoMatch, 403), - Clients: clients, - MapName: mapName, - } - return res, shaToClient } func (p *policiesCfg) addWAFConfig( @@ -1474,9 +1509,8 @@ func (vsc *virtualServerConfigurator) generatePolicies( policies map[string]*conf_v1.Policy, context string, policyOpts policyOptions, -) (policiesCfg, []version2.Map) { +) policiesCfg { config := newPoliciesConfig(vsc.bundleValidator) - maps := []version2.Map{} for _, p := range policyRefs { polNamespace := p.Namespace @@ -1518,44 +1552,27 @@ func (vsc *virtualServerConfigurator) generatePolicies( case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: - res, shaToClientMap := config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs, context) - // TODO: refactor - if res != nil && len(res.warnings) > 0 { - vsc.addWarnings(ownerDetails.owner, res.warnings) - } - - if res != nil && res.isError { - return policiesCfg{ - ErrorReturn: &version2.Return{Code: 500}, - }, maps - } - if res != nil && !res.isError && shaToClientMap != nil { - maps = append(maps, *shaToClientMap) - } - + res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs, context) case pol.Spec.WAF != nil: res = config.addWAFConfig(pol.Spec.WAF, key, polNamespace, policyOpts.apResources) default: res = newValidationResults() } - if res != nil && len(res.warnings) > 0 { - vsc.addWarnings(ownerDetails.owner, res.warnings) - } - - if res != nil && res.isError { + vsc.addWarnings(ownerDetails.owner, res.warnings) + if res.isError { return policiesCfg{ ErrorReturn: &version2.Return{Code: 500}, - }, maps + } } } else { vsc.addWarningf(ownerDetails.owner, "Policy %s is missing or invalid", key) return policiesCfg{ ErrorReturn: &version2.Return{Code: 500}, - }, maps + } } } - return *config, maps + return *config } func generateLimitReq(zoneName string, rateLimitPol *conf_v1.RateLimit) version2.LimitReq { diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index b073f0e271..b74c5b7569 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -6284,9 +6284,9 @@ func TestGeneratePolicies(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) for _, test := range tests { - result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) + result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) // TODO test maps - println(maps) // temporary print to use the variable + // println(maps) // temporary print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) @@ -6379,9 +6379,9 @@ func TestGeneratePolicies_GeneratesWAFPolicyOnValidApBundle(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) - res, maps := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) + res := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) // TODO test maps - println(maps) // temporary print to use the variable + // println(maps) // temporary print to use the variable res.BundleValidator = nil if !cmp.Equal(tc.want, res) { t.Error(cmp.Diff(tc.want, res)) @@ -7726,9 +7726,9 @@ func TestGeneratePoliciesFails(t *testing.T) { vsc.oidcPolCfg = test.oidcPolCfg } - result, maps := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) + result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) // TODO test maps - println(maps) // Temporty print to use the variable + // println(maps) // Temporty print to use the variable result.BundleValidator = nil if diff := cmp.Diff(test.expected, result); diff != "" { t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 53e69b46f0..2392e463b7 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -116,10 +116,15 @@ func ValidateAPIKeySecret(secret *api_v1.Secret) error { if secret.Type != SecretTypeAPIKey { return fmt.Errorf("APIKey secret must be of the type %v", SecretTypeAPIKey) } - //clientSecret, exists := secret.Data[ClientSecretKey] - //if !exists { - // return fmt.Errorf("OIDC secret must have the data field %v", ClientSecretKey) - //} + + uniqueKeys := make(map[string]bool) + for _, key := range secret.Data { + if uniqueKeys[string(key)] { + return fmt.Errorf("API Keys cannot be repeated") + } + uniqueKeys[string(key)] = true + } + return nil } @@ -147,7 +152,6 @@ func IsSupportedSecretType(secretType api_v1.SecretType) bool { secretType == SecretTypeOIDC || secretType == SecretTypeHtpasswd || secretType == SecretTypeAPIKey - // secretType == api_v1.SecretTypeOpaque } // ValidateSecret validates the secret. If it is valid, the function returns nil. diff --git a/internal/k8s/secrets/validation_test.go b/internal/k8s/secrets/validation_test.go index 4539ec4cc6..4297c9788f 100644 --- a/internal/k8s/secrets/validation_test.go +++ b/internal/k8s/secrets/validation_test.go @@ -65,6 +65,71 @@ func TestValidateJWKSecretFails(t *testing.T) { } } +func TestValidateValidateAPIKeySecret(t *testing.T) { + t.Parallel() + secret := &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-secret", + Namespace: "default", + }, + Type: SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("cGFzc3dvcmQ="), + "client2": []byte("N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk"), + }, + } + + err := ValidateAPIKeySecret(secret) + if err != nil { + t.Errorf("ValidateAPIKeySecret() returned error %v", err) + } +} + +func TestValidateValidateAPIKeyFails(t *testing.T) { + t.Parallel() + tests := []struct { + secret *v1.Secret + msg string + }{ + { + secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-secret", + Namespace: "default", + }, + Type: "some-type", + Data: map[string][]byte{ + "client": nil, + }, + }, + msg: "Incorrect type for API Key secret", + }, + { + secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-secret", + Namespace: "default", + }, + Type: SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("cGFzc3dvcmQ="), + "client2": []byte("N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk"), + "client3": []byte("N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk"), + }, + }, + msg: "repeated API Keys for API Key secret", + }, + } + + for _, test := range tests { + err := ValidateAPIKeySecret(test.secret) + t.Logf("ValidateAPIKeySecret() returned error %v", err) + if err == nil { + t.Errorf("ValidateAPIKeySecret() returned no error for the case of %s", test.msg) + } + } +} + func TestValidateHtpasswdSecret(t *testing.T) { t.Parallel() secret := &v1.Secret{ diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 8d845d42d2..5b28c7b375 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -283,11 +283,31 @@ func validateOIDC(oidc *v1.OIDC, fieldPath *field.Path) field.ErrorList { } func validateAPIKey(apiKey *v1.APIKey, fieldPath *field.Path) field.ErrorList { - // if api + allErrs := field.ErrorList{} + if apiKey.SuppliedIn.Query == nil && apiKey.SuppliedIn.Header == nil { + msg := "at least one query or header name must be provided" + allErrs = append(allErrs, field.Required(fieldPath.Child("SuppliedIn"), msg)) + } + + if apiKey.SuppliedIn.Header != nil { + for _, header := range apiKey.SuppliedIn.Header { + for _, msg := range validation.IsHTTPHeaderName(header) { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("suppliedIn.header"), header, msg)) + } + } + } + + if apiKey.SuppliedIn.Query != nil { + for _, query := range apiKey.SuppliedIn.Query { + if err := ValidateEscapedString(query); err != nil { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("suppliedIn.query"), query, err.Error())) + } + } + } + if apiKey.ClientSecret == "" { - return field.ErrorList{field.Required(fieldPath.Child("clientSecret"), "")} + allErrs = append(allErrs, field.Required(fieldPath.Child("clientSecret"), "")) } - allErrs := field.ErrorList{} allErrs = append(allErrs, validateSecretName(apiKey.ClientSecret, fieldPath.Child("clientSecret"))...) From 5a8916b6d8fe1dc2e7ad0e0cad731495be541c4e Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 6 Jun 2024 10:43:39 +0000 Subject: [PATCH 18/36] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- examples/custom-resources/api-key/api-key-secret-1.yaml | 2 +- examples/custom-resources/api-key/cafe-virtual-server-2.yaml | 1 - .../custom-resources/api-key/coffee-virtual-server-route.yaml | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml index bf6954f4b0..318529da6a 100644 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ b/examples/custom-resources/api-key/api-key-secret-1.yaml @@ -5,4 +5,4 @@ metadata: type: nginx.org/apikey data: client1: cGFzc3dvcmQ= - client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk \ No newline at end of file + client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk diff --git a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml index 7cb56276f1..7b26ce35a4 100644 --- a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml +++ b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml @@ -29,4 +29,3 @@ spec: pass: coffee policies: - name: api-key-policy - diff --git a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml index 5b921011c7..ab248ba010 100644 --- a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml +++ b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml @@ -18,4 +18,4 @@ spec: action: pass: coffee policies: - - name: api-key-policy \ No newline at end of file + - name: api-key-policy From 8f877e3b0eab5ebf56db7055db5a38c837f810a4 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Thu, 6 Jun 2024 11:58:00 +0100 Subject: [PATCH 19/36] fix linting, remove unused structs, update crds and 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 | 10 ++--- deploy/crds.yaml | 10 ++--- internal/configs/version2/http.go | 1 + internal/configs/virtualserver.go | 25 ++++++----- internal/k8s/secrets/validation.go | 1 + pkg/apis/configuration/v1/types.go | 17 ++------ .../configuration/v1/zz_generated.deepcopy.go | 43 ------------------- 7 files changed, 24 insertions(+), 83 deletions(-) diff --git a/config/crd/bases/k8s.nginx.org_policies.yaml b/config/crd/bases/k8s.nginx.org_policies.yaml index 8c2b0abb89..fe196acf3d 100644 --- a/config/crd/bases/k8s.nginx.org_policies.yaml +++ b/config/crd/bases/k8s.nginx.org_policies.yaml @@ -68,17 +68,13 @@ spec: type: array type: object apiKey: + description: APIKey defines an API Key policy. properties: clientSecret: type: string - rejectCodes: - properties: - noMatch: - type: integer - notSupplied: - type: integer - type: object suppliedIn: + description: SuppliedIn defines the locations API Key should be + supplied in. properties: header: items: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 2a473b3c78..d93b687b2d 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -270,17 +270,13 @@ spec: type: array type: object apiKey: + description: APIKey defines an API Key policy. properties: clientSecret: type: string - rejectCodes: - properties: - noMatch: - type: integer - notSupplied: - type: integer - type: object suppliedIn: + description: SuppliedIn defines the locations API Key should be + supplied in. properties: header: items: diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index bd72ae6a9f..60bcb82abe 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -138,6 +138,7 @@ type OIDC struct { AccessTokenEnable bool } +// APIKey holds API key configuration. type APIKey struct { Header []string Query []string diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index ba79a4b5a7..2dcf5d28d2 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -425,7 +425,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( if policiesCfg.APIKeyEnabled { apiMapName := policiesCfg.APIKey.MapName - policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + policiesCfg.APIKeyClientMap = make(map[string][]apiKeyClient) policiesCfg.APIKeyClientMap[apiMapName] = policiesCfg.APIKeyClients } @@ -591,7 +591,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policiesCfg.APIKeyEnabled = routePoliciesCfg.APIKeyEnabled apiMapName := routePoliciesCfg.APIKey.MapName if policiesCfg.APIKeyClientMap == nil { - policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + policiesCfg.APIKeyClientMap = make(map[string][]apiKeyClient) } if _, exists := policiesCfg.APIKeyClientMap[apiMapName]; !exists { policiesCfg.APIKeyClientMap[apiMapName] = routePoliciesCfg.APIKeyClients @@ -731,7 +731,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( policiesCfg.APIKeyEnabled = routePoliciesCfg.APIKeyEnabled apiMapName := routePoliciesCfg.APIKey.MapName if policiesCfg.APIKeyClientMap == nil { - policiesCfg.APIKeyClientMap = make(map[string][]APIKeyClient) + policiesCfg.APIKeyClientMap = make(map[string][]apiKeyClient) } if _, exists := policiesCfg.APIKeyClientMap[apiMapName]; !exists { policiesCfg.APIKeyClientMap[apiMapName] = routePoliciesCfg.APIKeyClients @@ -894,8 +894,8 @@ type policiesCfg struct { OIDC bool APIKeyEnabled bool APIKey *version2.APIKey - APIKeyClients []APIKeyClient - APIKeyClientMap map[string][]APIKeyClient + APIKeyClients []apiKeyClient + APIKeyClientMap map[string][]apiKeyClient WAF *version2.WAF ErrorReturn *version2.Return BundleValidator bundleValidator @@ -910,7 +910,7 @@ type internalBundleValidator struct { bundlePath string } -type APIKeyClient struct { +type apiKeyClient struct { ClientID string HashedKey string } @@ -1336,7 +1336,6 @@ func (p *policiesCfg) addAPIKeyConfig( polKey string, polNamespace string, secretRefs map[string]*secrets.SecretReference, - context string, ) *validationResults { res := newValidationResults() if p.APIKey != nil { @@ -1379,24 +1378,24 @@ func (p *policiesCfg) addAPIKeyConfig( return res } -func generateAPIKeyClients(secretData map[string][]byte) []APIKeyClient { - var clients []APIKeyClient +func generateAPIKeyClients(secretData map[string][]byte) []apiKeyClient { + var clients []apiKeyClient for clientID, apiKey := range secretData { h := sha256.New() - h.Write([]byte(apiKey)) + h.Write(apiKey) sha256Hash := hex.EncodeToString(h.Sum(nil)) base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) glog.Infof("apiKey %s", apiKey) glog.Infof("sha %s", sha256Hash) glog.Infof("base64Str %s", base64Str) - clients = append(clients, APIKeyClient{ClientID: clientID, HashedKey: sha256Hash}) // + clients = append(clients, apiKeyClient{ClientID: clientID, HashedKey: sha256Hash}) // } return clients } -func generateAPIKeyClientMap(mapName string, apiKeyClients []APIKeyClient) *version2.Map { +func generateAPIKeyClientMap(mapName string, apiKeyClients []apiKeyClient) *version2.Map { glog.Infof("mapName: %v, apiKeyClients: %v", mapName, apiKeyClients) defaultParam := version2.Parameter{ @@ -1552,7 +1551,7 @@ func (vsc *virtualServerConfigurator) generatePolicies( case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: - res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs, context) + res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) case pol.Spec.WAF != nil: res = config.addWAFConfig(pol.Spec.WAF, key, polNamespace, policyOpts.apResources) default: diff --git a/internal/k8s/secrets/validation.go b/internal/k8s/secrets/validation.go index 2392e463b7..8ea4b436ba 100644 --- a/internal/k8s/secrets/validation.go +++ b/internal/k8s/secrets/validation.go @@ -112,6 +112,7 @@ func ValidateOIDCSecret(secret *api_v1.Secret) error { return nil } +// ValidateAPIKeySecret validates the secret. If it is valid, the function returns nil. func ValidateAPIKeySecret(secret *api_v1.Secret) error { if secret.Type != SecretTypeAPIKey { return fmt.Errorf("APIKey secret must be of the type %v", SecretTypeAPIKey) diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 4733e34960..4451bd5802 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -677,23 +677,14 @@ type SecurityLog struct { LogDest string `json:"logDest"` } +// APIKey defines an API Key policy. type APIKey struct { - SuppliedIn SuppliedIn `json:"suppliedIn"` - RejectCodes RejectCodes `json:"rejectCodes,omitempty"` - ClientSecret string `json:"clientSecret"` + SuppliedIn SuppliedIn `json:"suppliedIn"` + ClientSecret string `json:"clientSecret"` } +// SuppliedIn defines the locations API Key should be supplied in. type SuppliedIn struct { Header []string `json:"header"` Query []string `json:"query"` } - -type RejectCodes struct { - NotSupplied *int `json:"notSupplied"` - NoMatch *int `json:"noMatch"` -} - -type Client struct { - ID string `json:"id"` - Secret string `json:"secret"` -} diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 339c9ee38b..97302bab36 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -13,7 +13,6 @@ import ( func (in *APIKey) DeepCopyInto(out *APIKey) { *out = *in in.SuppliedIn.DeepCopyInto(&out.SuppliedIn) - in.RejectCodes.DeepCopyInto(&out.RejectCodes) return } @@ -196,22 +195,6 @@ func (in *CertManager) DeepCopy() *CertManager { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Client) DeepCopyInto(out *Client) { - *out = *in - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Client. -func (in *Client) DeepCopy() *Client { - if in == nil { - return nil - } - out := new(Client) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Condition) DeepCopyInto(out *Condition) { *out = *in @@ -896,32 +879,6 @@ 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 *RejectCodes) DeepCopyInto(out *RejectCodes) { - *out = *in - if in.NotSupplied != nil { - in, out := &in.NotSupplied, &out.NotSupplied - *out = new(int) - **out = **in - } - if in.NoMatch != nil { - in, out := &in.NoMatch, &out.NoMatch - *out = new(int) - **out = **in - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RejectCodes. -func (in *RejectCodes) DeepCopy() *RejectCodes { - if in == nil { - return nil - } - out := new(RejectCodes) - 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 1d91b7cf6143d9837edc8f590611f8e73622b5c6 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Tue, 11 Jun 2024 09:11:51 +0100 Subject: [PATCH 20/36] add unit tests, unique map names, add validate apikey location block to conf only if api key policy is used 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 | 1 + .../version2/nginx-plus.virtualserver.tmpl | 2 + .../configs/version2/nginx.virtualserver.tmpl | 7 +- internal/configs/virtualserver.go | 18 +- internal/configs/virtualserver_test.go | 450 +++++++++++++++++- pkg/apis/configuration/v1/types.go | 4 +- .../configuration/v1/zz_generated.deepcopy.go | 6 +- 7 files changed, 466 insertions(+), 22 deletions(-) diff --git a/internal/configs/version2/http.go b/internal/configs/version2/http.go index 60bcb82abe..f05a999ca9 100644 --- a/internal/configs/version2/http.go +++ b/internal/configs/version2/http.go @@ -85,6 +85,7 @@ type Server struct { EgressMTLS *EgressMTLS OIDC *OIDC APIKey *APIKey + APIKeyEnabled bool WAF *WAF Dos *Dos PoliciesErrorReturn *Return diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index bbf5bb76f7..e08b8ae744 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -223,10 +223,12 @@ server { {{- end }} # TODO: Do this conditionally + {{- if $s.APIKeyEnabled}} location = /_validate_apikey_njs { internal; js_content apikey_auth.validate; } + {{- end }} {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index 7e22b35d68..ea1eff0d90 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -144,11 +144,12 @@ server { {{- end }} # TODO: Do this conditionally + {{- if $s.APIKeyEnabled}} location = /_validate_apikey_njs { - internal; - js_content apikey_auth.validate; + internal; + js_content apikey_auth.validate; } - + {{- end }} {{- with $s.BasicAuth }} auth_basic {{ printf "%q" .Realm }}; diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 2dcf5d28d2..928b1d6e5d 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -859,6 +859,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( IngressMTLS: policiesCfg.IngressMTLS, EgressMTLS: policiesCfg.EgressMTLS, APIKey: policiesCfg.APIKey, + APIKeyEnabled: policiesCfg.APIKeyEnabled, OIDC: vsc.oidcPolCfg.oidc, WAF: policiesCfg.WAF, Dos: dosCfg, @@ -1335,12 +1336,14 @@ func (p *policiesCfg) addAPIKeyConfig( apiKey *conf_v1.APIKey, polKey string, polNamespace string, + vsNamespace string, + vsName string, secretRefs map[string]*secrets.SecretReference, ) *validationResults { res := newValidationResults() if p.APIKey != nil { res.addWarningf( - "Multiple APIKey policies in the same context is not valid. APIKey policy %s will be ignored", + "Multiple API Key policies in the same context is not valid. API Key policy %s will be ignored", polKey, ) res.isError = true @@ -1368,7 +1371,15 @@ func (p *policiesCfg) addAPIKeyConfig( p.APIKeyClients = generateAPIKeyClients(secretRef.Secret.Data) - mapName := fmt.Sprintf("apikey_auth_client_name_%s", strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1]) + mapName := fmt.Sprintf( + "apikey_auth_client_name_%s_%s_%s", + // vsNamespace, + // vsName, + strings.Replace(vsNamespace, "-", "_", -1), // TODO:refactor + strings.Replace(vsName, "-", "_", -1), + // strings.Split(polKey, "/")[1], + strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1], + ) p.APIKey = &version2.APIKey{ Header: apiKey.SuppliedIn.Header, Query: apiKey.SuppliedIn.Query, @@ -1551,7 +1562,8 @@ func (vsc *virtualServerConfigurator) generatePolicies( case pol.Spec.OIDC != nil: res = config.addOIDCConfig(pol.Spec.OIDC, key, polNamespace, policyOpts.secretRefs, vsc.oidcPolCfg) case pol.Spec.APIKey != nil: - res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, policyOpts.secretRefs) + res = config.addAPIKeyConfig(pol.Spec.APIKey, key, polNamespace, ownerDetails.vsNamespace, + ownerDetails.vsName, policyOpts.secretRefs) case pol.Spec.WAF != nil: res = config.addWAFConfig(pol.Spec.WAF, key, polNamespace, policyOpts.apResources) default: diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index b74c5b7569..dba9882200 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -5653,6 +5653,263 @@ func TestGenerateVirtualServerConfigJWKSPolicy(t *testing.T) { } } +func TestGenerateVirtualServerConfigAPIKeyPolicy(t *testing.T) { + t.Parallel() + + virtualServerEx := VirtualServerEx{ + SecretRefs: map[string]*secrets.SecretReference{ + "default/api-key-secret-spec": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "clientSpec": []byte("password"), + }, + }, + }, + "default/api-key-secret-route": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "clientRoute": []byte("password2"), + }, + }, + }, + }, + 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: "api-key-policy-spec", + }, + }, + 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: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-route", + }, + }, + }, + }, + }, + }, + Policies: map[string]*conf_v1.Policy{ + "default/api-key-policy-spec": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-spec", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"apikey"}, + }, + ClientSecret: "api-key-secret-spec", + }, + }, + }, + "default/api-key-policy-route": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-route", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret-route", + }, + }, + }, + }, + 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: []version2.Map{ + { + Source: "$apikey_auth_token", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_spec", + Parameters: []version2.Parameter{ + { + Value: "default", + Result: `""`, + }, + { + Value: `"5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8"`, + Result: `"clientSpec"`, + }, + }, + }, + { + Source: "$apikey_auth_token", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_route", + Parameters: []version2.Parameter{ + { + Value: "default", + Result: `""`, + }, + { + Value: `"6cf615d5bcaac778352a8f1f3360d23f02f34ec182e259897fd6ce485d7870d4"`, + Result: `"clientRoute"`, + }, + }, + }, + }, + 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{}, + 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: true, + APIKey: &version2.APIKey{ + Header: []string{"X-API-Key"}, + Query: []string{"apikey"}, + MapName: "apikey_auth_client_name_default_cafe_api_key_policy_spec", + }, + 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", + APIKey: &version2.APIKey{ + MapName: "apikey_auth_client_name_default_cafe_api_key_policy_route", + Query: []string{"api-key"}, + }, + }, + }, + }, + } + + baseCfgParams := ConfigParams{ + 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) + + 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{ @@ -5720,6 +5977,22 @@ func TestGeneratePolicies(t *testing.T) { }, }, }, + "default/api-key-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("password"), + }, + }, + }, + "default/api-key-secret-2": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client2": []byte("password2"), + }, + }, + }, }, apResources: &appProtectResourcesForVS{ Policies: map[string]string{ @@ -6246,6 +6519,88 @@ func TestGeneratePolicies(t *testing.T) { }, msg: "oidc reference", }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "api-key-policy", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/api-key-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + }, + expected: policiesCfg{ + APIKey: &version2.APIKey{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + MapName: "apikey_auth_client_name_default_test_api_key_policy", + }, + APIKeyEnabled: true, + APIKeyClientMap: nil, + APIKeyClients: []apiKeyClient{ + { + ClientID: "client1", + HashedKey: "5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8", + }, + }, + }, + msg: "api key reference", + }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "api-key-policy", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/api-key-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + }, + expected: policiesCfg{ + APIKey: &version2.APIKey{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + MapName: "apikey_auth_client_name_default_test_api_key_policy", + }, + APIKeyEnabled: true, + APIKeyClientMap: nil, + APIKeyClients: []apiKeyClient{ + { + ClientID: "client1", + HashedKey: "5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8", + }, + }, + }, + msg: "api key same secrets for different policies", + }, { policyRefs: []conf_v1.PolicyReference{ { @@ -6283,17 +6638,18 @@ func TestGeneratePolicies(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) - for _, test := range tests { - result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, policyOpts) - // TODO test maps - // println(maps) // temporary print to use the variable - result.BundleValidator = nil - if diff := cmp.Diff(test.expected, result); diff != "" { - t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) - } - if len(vsc.warnings) > 0 { - t.Errorf("generatePolicies() returned unexpected warnings %v for the case of %s", vsc.warnings, test.msg) - } + for _, tc := range tests { + t.Run(tc.msg, func(t *testing.T) { + result := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOpts) + result.BundleValidator = nil + + if !cmp.Equal(tc.expected, result) { + t.Errorf(cmp.Diff(tc.expected, result)) + } + if len(vsc.warnings) > 0 { + t.Errorf("generatePolicies() returned unexpected warnings %v for the case of %s", vsc.warnings, tc.msg) + } + }) } } @@ -6380,8 +6736,6 @@ func TestGeneratePolicies_GeneratesWAFPolicyOnValidApBundle(t *testing.T) { t.Run(tc.name, func(t *testing.T) { vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) res := vsc.generatePolicies(ownerDetails, tc.policyRefs, tc.policies, tc.context, policyOptions{apResources: &appProtectResourcesForVS{}}) - // TODO test maps - // println(maps) // temporary print to use the variable res.BundleValidator = nil if !cmp.Equal(tc.want, res) { t.Error(cmp.Diff(tc.want, res)) @@ -7655,6 +8009,76 @@ func TestGeneratePoliciesFails(t *testing.T) { }, msg: "multi oidc", }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "api-key-policy", + Namespace: "default", + }, + { + Name: "api-key-policy-2", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/api-key-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + "default/api-key-policy-2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-2", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + }, + policyOpts: policyOptions{ + secretRefs: map[string]*secrets.SecretReference{ + "default/api-key-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("password"), + }, + }, + }, + }, + }, + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `Multiple API Key policies in the same context is not valid. API Key policy default/api-key-policy-2 will be ignored`, + }, + }, + expectedOidc: &oidcPolicyCfg{}, + msg: "multi api key", + }, + // TODO:wrong secret type + // TODO: secret invalid { policyRefs: []conf_v1.PolicyReference{ { diff --git a/pkg/apis/configuration/v1/types.go b/pkg/apis/configuration/v1/types.go index 4451bd5802..0bc75f8fa9 100644 --- a/pkg/apis/configuration/v1/types.go +++ b/pkg/apis/configuration/v1/types.go @@ -679,8 +679,8 @@ type SecurityLog struct { // APIKey defines an API Key policy. type APIKey struct { - SuppliedIn SuppliedIn `json:"suppliedIn"` - ClientSecret string `json:"clientSecret"` + SuppliedIn *SuppliedIn `json:"suppliedIn"` + ClientSecret string `json:"clientSecret"` } // SuppliedIn defines the locations API Key should be supplied in. diff --git a/pkg/apis/configuration/v1/zz_generated.deepcopy.go b/pkg/apis/configuration/v1/zz_generated.deepcopy.go index 97302bab36..b617f3cb93 100644 --- a/pkg/apis/configuration/v1/zz_generated.deepcopy.go +++ b/pkg/apis/configuration/v1/zz_generated.deepcopy.go @@ -12,7 +12,11 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *APIKey) DeepCopyInto(out *APIKey) { *out = *in - in.SuppliedIn.DeepCopyInto(&out.SuppliedIn) + if in.SuppliedIn != nil { + in, out := &in.SuppliedIn, &out.SuppliedIn + *out = new(SuppliedIn) + (*in).DeepCopyInto(*out) + } return } From 3e53c4bbd48f0a209aaa5decc28566f97f9c3d0d Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 12 Jun 2024 10:50:47 +0100 Subject: [PATCH 21/36] add python tests for vs and vsr --- .../policies/apikey-policy-server.yaml | 15 + .../policies/apikey-policy-valid-2.yaml | 15 + .../policies/apikey-policy-valid.yaml | 14 + .../policies/apikey-policy-vs-route.yaml | 15 + .../secret/apikey-secret-1.yaml | 9 + .../secret/apikey-secret-2.yaml | 9 + .../secret/apikey-secret-route.yaml | 8 + .../secret/apikey-secret-server.yaml | 8 + .../spec/virtual-server-policy-single.yaml | 30 ++ .../spec/vsr/backend1-vsr.yaml | 14 + .../spec/vsr/backend2-vsr.yaml | 16 + .../spec/vsr/virtual-server-with-vsr.yaml | 22 + tests/suite/test_apikey_auth_policies.py | 488 ++++++++++++++++++ tests/suite/utils/resources_utils.py | 40 ++ 14 files changed, 703 insertions(+) create mode 100644 tests/data/apikey-auth-policy/policies/apikey-policy-server.yaml create mode 100644 tests/data/apikey-auth-policy/policies/apikey-policy-valid-2.yaml create mode 100644 tests/data/apikey-auth-policy/policies/apikey-policy-valid.yaml create mode 100644 tests/data/apikey-auth-policy/policies/apikey-policy-vs-route.yaml create mode 100644 tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml create mode 100644 tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml create mode 100644 tests/data/apikey-auth-policy/secret/apikey-secret-route.yaml create mode 100644 tests/data/apikey-auth-policy/secret/apikey-secret-server.yaml create mode 100644 tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml create mode 100644 tests/data/apikey-auth-policy/spec/vsr/backend1-vsr.yaml create mode 100644 tests/data/apikey-auth-policy/spec/vsr/backend2-vsr.yaml create mode 100644 tests/data/apikey-auth-policy/spec/vsr/virtual-server-with-vsr.yaml create mode 100644 tests/suite/test_apikey_auth_policies.py diff --git a/tests/data/apikey-auth-policy/policies/apikey-policy-server.yaml b/tests/data/apikey-auth-policy/policies/apikey-policy-server.yaml new file mode 100644 index 0000000000..db46e29bd4 --- /dev/null +++ b/tests/data/apikey-auth-policy/policies/apikey-policy-server.yaml @@ -0,0 +1,15 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy-server +spec: + apiKey: + suppliedIn: + header: + - "header-server-1" + - "header-server-2" + - "header-server-3" + query: + - "queryServer1" + - "queryServer2" + clientSecret: api-key-client-secret-server diff --git a/tests/data/apikey-auth-policy/policies/apikey-policy-valid-2.yaml b/tests/data/apikey-auth-policy/policies/apikey-policy-valid-2.yaml new file mode 100644 index 0000000000..7c4da9bae0 --- /dev/null +++ b/tests/data/apikey-auth-policy/policies/apikey-policy-valid-2.yaml @@ -0,0 +1,15 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy-2 +spec: + apiKey: + suppliedIn: + header: + - "this-is-another-header" + - "and-other-one" + - "some-other-header" + query: + - "query1" + - "query2" + clientSecret: api-key-client-secret-2 diff --git a/tests/data/apikey-auth-policy/policies/apikey-policy-valid.yaml b/tests/data/apikey-auth-policy/policies/apikey-policy-valid.yaml new file mode 100644 index 0000000000..30c45bbd3b --- /dev/null +++ b/tests/data/apikey-auth-policy/policies/apikey-policy-valid.yaml @@ -0,0 +1,14 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy +spec: + apiKey: + suppliedIn: + header: + - "X-header-name" + - "apikey" + - "some-other-header" + query: + - "queryName" + clientSecret: api-key-client-secret-1 diff --git a/tests/data/apikey-auth-policy/policies/apikey-policy-vs-route.yaml b/tests/data/apikey-auth-policy/policies/apikey-policy-vs-route.yaml new file mode 100644 index 0000000000..99b35026be --- /dev/null +++ b/tests/data/apikey-auth-policy/policies/apikey-policy-vs-route.yaml @@ -0,0 +1,15 @@ +apiVersion: k8s.nginx.org/v1 +kind: Policy +metadata: + name: api-key-policy-vs-route +spec: + apiKey: + suppliedIn: + header: + - "header-route-1" + - "header-route-2" + - "header-route-3" + query: + - "queryRoute1" + - "queryRoute2" + clientSecret: api-key-client-secret-route diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml new file mode 100644 index 0000000000..3048e36bc4 --- /dev/null +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml @@ -0,0 +1,9 @@ + +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-1 +type: nginx.org/apikey +data: + client1: cGFzc3dvcmQ= # password + client2: cGFzc3dvcmQy # password2 diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml new file mode 100644 index 0000000000..a4d6c5860b --- /dev/null +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml @@ -0,0 +1,9 @@ + +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-2 +type: nginx.org/apikey +data: + client3: cGFzc3dvcmQz # password3 + client4: cGFzc3dvcmQ0 # password4 diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-route.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-route.yaml new file mode 100644 index 0000000000..436897147f --- /dev/null +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-route.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-route +type: nginx.org/apikey +data: + client1: cGFzc3dvcmQ3 # password7 + client2: cGFzc3dvcmQ4 # password8 diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-server.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-server.yaml new file mode 100644 index 0000000000..e3097c622c --- /dev/null +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-server.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret-server +type: nginx.org/apikey +data: + client1: cGFzc3dvcmQ1 # password5 + client2: cGFzc3dvcmQ2 # password6 diff --git a/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml b/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml new file mode 100644 index 0000000000..e69e7fc2fa --- /dev/null +++ b/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml @@ -0,0 +1,30 @@ + +apiVersion: k8s.nginx.org/v1 +kind: VirtualServer +metadata: + name: virtual-server +spec: + host: virtual-server.example.com + policies: + - name: api-key-policy + upstreams: + - name: backend2 + service: backend2-svc + port: 80 + - name: backend1 + service: backend1-svc + port: 80 + routes: + - path: /backend1 + action: + pass: backend1 + - path: /backend2 + action: + pass: backend2 + policies: + - name: api-key-policy-2 + - path: /no-auth + action: + pass: backend2 + location-snippets: + auth_request off; diff --git a/tests/data/apikey-auth-policy/spec/vsr/backend1-vsr.yaml b/tests/data/apikey-auth-policy/spec/vsr/backend1-vsr.yaml new file mode 100644 index 0000000000..0c18d8ad93 --- /dev/null +++ b/tests/data/apikey-auth-policy/spec/vsr/backend1-vsr.yaml @@ -0,0 +1,14 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServerRoute +metadata: + name: backend1 +spec: + host: virtual-server.example.com + upstreams: + - name: backend1 + service: backend1-svc + port: 80 + subroutes: + - path: /backend1 + action: + pass: backend1 diff --git a/tests/data/apikey-auth-policy/spec/vsr/backend2-vsr.yaml b/tests/data/apikey-auth-policy/spec/vsr/backend2-vsr.yaml new file mode 100644 index 0000000000..1067a51ebb --- /dev/null +++ b/tests/data/apikey-auth-policy/spec/vsr/backend2-vsr.yaml @@ -0,0 +1,16 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServerRoute +metadata: + name: backend2 +spec: + host: virtual-server.example.com + upstreams: + - name: backend2 + service: backend2-svc + port: 80 + subroutes: + - path: /backend2 + action: + pass: backend2 + policies: + - name: api-key-policy-vs-route diff --git a/tests/data/apikey-auth-policy/spec/vsr/virtual-server-with-vsr.yaml b/tests/data/apikey-auth-policy/spec/vsr/virtual-server-with-vsr.yaml new file mode 100644 index 0000000000..75d1d9f41a --- /dev/null +++ b/tests/data/apikey-auth-policy/spec/vsr/virtual-server-with-vsr.yaml @@ -0,0 +1,22 @@ +apiVersion: k8s.nginx.org/v1 +kind: VirtualServer +metadata: + name: virtual-server +spec: + host: virtual-server.example.com + policies: + - name: api-key-policy-server + upstreams: + - name: backend1 + service: backend1-svc + port: 80 + routes: + - path: /backend1 + route: backend1 + - path: /backend2 + route: backend2 + - path: /no-auth + location-snippets: + auth_request off; + action: + pass: backend1 diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py new file mode 100644 index 0000000000..d29aab8be7 --- /dev/null +++ b/tests/suite/test_apikey_auth_policies.py @@ -0,0 +1,488 @@ +from collections import namedtuple + +import pytest +import requests +from settings import TEST_DATA +from suite.utils.policy_resources_utils import create_policy_from_yaml, delete_policy +from suite.utils.resources_utils import ( + create_secret_from_yaml, + delete_items_from_yaml, + delete_secret, + get_apikey_auth_secrets_from_yaml, + get_apikey_policy_details_from_yaml, + wait_before_test, +) +from suite.utils.vs_vsr_resources_utils import create_v_s_route_from_yaml, delete_and_create_vs_from_yaml + +apikey_auth_pol_valid = f"{TEST_DATA}/apikey-auth-policy/policies/apikey-policy-valid.yaml" +apikey_auth_pol_valid_2 = f"{TEST_DATA}/apikey-auth-policy/policies/apikey-policy-valid-2.yaml" +apikey_auth_pol_server = f"{TEST_DATA}/apikey-auth-policy/policies/apikey-policy-server.yaml" +apikey_auth_pol_route = f"{TEST_DATA}/apikey-auth-policy/policies/apikey-policy-vs-route.yaml" + +apikey_auth_secret_1 = f"{TEST_DATA}/apikey-auth-policy/secret/apikey-secret-1.yaml" +apikey_auth_secret_2 = f"{TEST_DATA}/apikey-auth-policy/secret/apikey-secret-2.yaml" +apikey_auth_secret_server = f"{TEST_DATA}/apikey-auth-policy/secret/apikey-secret-server.yaml" +apikey_auth_secret_route = f"{TEST_DATA}/apikey-auth-policy/secret/apikey-secret-route.yaml" + +apikey_auth_vs_single_src = f"{TEST_DATA}/apikey-auth-policy/spec/virtual-server-policy-single.yaml" +apikey_auth_vs_vsr_src = f"{TEST_DATA}/apikey-auth-policy/spec/vsr/virtual-server-with-vsr.yaml" + +vsr_1_src = f"{TEST_DATA}/apikey-auth-policy/spec/vsr/backend1-vsr.yaml" +vsr_2_src = f"{TEST_DATA}/apikey-auth-policy/spec/vsr/backend2-vsr.yaml" + + +std_vs_src = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" + + +@pytest.mark.policies +@pytest.mark.parametrize( + "crd_ingress_controller, virtual_server_setup", + [ + ( + { + "type": "complete", + "extra_args": [ + f"-enable-custom-resources", + f"-enable-leader-election=false", + f"-enable-snippets", + ], + }, + { + "example": "virtual-server", + "app_type": "simple", + }, + ) + ], + indirect=True, +) +class TestAPIKeyAuthPolicies: + def setup_single_policy( + self, kube_apis, test_namespace: str, secret_src: str, policy_src: str, vs_host: str + ) -> namedtuple: + APIKey_policy_details = namedtuple( + "APIKey_policy_details", ["headers", "queries", "policy_name", "secret_name", "vs_host", "apikeys"] + ) + print(f"Create apikey auth secret") + secret_name = create_secret_from_yaml(kube_apis.v1, test_namespace, secret_src) + apikeys = get_apikey_auth_secrets_from_yaml(secret_src) + details = get_apikey_policy_details_from_yaml(policy_src) + + print(f"Create apikey auth policy") + policy_name = create_policy_from_yaml(kube_apis.custom_objects, policy_src, test_namespace) + wait_before_test() + + headers = details["headers"] + queries = details["queries"] + return APIKey_policy_details( + headers=headers, + queries=queries, + policy_name=policy_name, + secret_name=secret_name, + vs_host=vs_host, + apikeys=apikeys, + ) + + def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_server_setup, test_namespace): + apikey_policy_details = self.setup_single_policy( + kube_apis, test_namespace, apikey_auth_secret_1, apikey_auth_pol_valid, virtual_server_setup.vs_host + ) + + apikey_policy_2_details = self.setup_single_policy( + kube_apis, test_namespace, apikey_auth_secret_2, apikey_auth_pol_valid_2, virtual_server_setup.vs_host + ) + + delete_and_create_vs_from_yaml( + kube_apis.custom_objects, + virtual_server_setup.vs_name, + apikey_auth_vs_single_src, + virtual_server_setup.namespace, + ) + + host = apikey_policy_details.vs_host + + wait_before_test() + + # /no-auth path + no_auth_headers = {"host": host} + no_auth_path = ( + f"http://{virtual_server_setup.public_endpoint.public_ip}" + f":{virtual_server_setup.public_endpoint.port}/no-auth" + ) + no_auth_resp = requests.get(no_auth_path, headers=no_auth_headers) + + # /backend1 path (uses policy on the server level) + # without auth headers + backend1_without_auth_headers = { + "host": host, + } + backend1_without_auth_resp = requests.get( + virtual_server_setup.backend_1_url, headers=backend1_without_auth_headers + ) + # with wrong password in header + backend1_correct_header_with_wrong_password_resps = [] + for header in apikey_policy_details.headers: + backend1_with_auth_headers_but_wrong_password = {"host": host, header: "wrongpassword"} + backend1_wrong_password_resp = requests.get( + virtual_server_setup.backend_1_url, headers=backend1_with_auth_headers_but_wrong_password + ) + backend1_correct_header_with_wrong_password_resps.append(backend1_wrong_password_resp) + # with wrong password in query + backend1_correct_query_with_wrong_password_resps = [] + for query in apikey_policy_details.queries: + host_header = {"host": host} + params = {query: "wrongpassword"} + backend1_wrong_password_resp = requests.get( + virtual_server_setup.backend_1_url, headers=host_header, params=params + ) + backend1_correct_query_with_wrong_password_resps.append(backend1_wrong_password_resp) + # try each header with each correct apikey + backend1_correct_header_with_correct_password_resps = [] + for header in apikey_policy_details.headers: + for key in apikey_policy_details.apikeys: + backend1_with_auth_headers_correct_password = {"host": host, header: key} + backend1_correct_password_resp = requests.get( + virtual_server_setup.backend_1_url, headers=backend1_with_auth_headers_correct_password + ) + backend1_correct_header_with_correct_password_resps.append(backend1_correct_password_resp) + # try each query with each correct apikey + backend1_correct_query_with_correct_password_resps = [] + for query in apikey_policy_details.queries: + for key in apikey_policy_details.apikeys: + params = {query: key} + host_header = {"host": host} + backend1_correct_password_resp = requests.get( + virtual_server_setup.backend_1_url, headers=host_header, params=params + ) + backend1_correct_query_with_correct_password_resps.append(backend1_correct_password_resp) + + # /backend2 path (uses policy on the route level) + # without auth headers + backend2_without_auth_headers = {"host": host} + backend2_without_auth_resp = requests.get( + virtual_server_setup.backend_2_url, headers=backend2_without_auth_headers + ) + # with wrong password in header + backend2_correct_header_with_wrong_password_resps = [] + for header in apikey_policy_2_details.headers: + backend2_with_auth_headers_but_wrong_password = {"host": host, header: "wrongpassword"} + backend2_wrong_password_resp = requests.get( + virtual_server_setup.backend_2_url, headers=backend2_with_auth_headers_but_wrong_password + ) + backend2_correct_header_with_wrong_password_resps.append(backend2_wrong_password_resp) + # with wrong password in query + backend2_correct_query_with_wrong_password_resps = [] + for query in apikey_policy_2_details.queries: + host_header = {"host": host} + params = {query: "wrongpassword"} + backend2_wrong_password_resp = requests.get( + virtual_server_setup.backend_2_url, headers=host_header, params=params + ) + backend2_correct_query_with_wrong_password_resps.append(backend2_wrong_password_resp) + # try each header with each correct apikey + backend2_correct_header_with_correct_password_resps = [] + for header in apikey_policy_2_details.headers: + for key in apikey_policy_2_details.apikeys: + backend2_with_auth_headers_correct_password = {"host": host, header: key} + backend2_correct_password_resp = requests.get( + virtual_server_setup.backend_2_url, headers=backend2_with_auth_headers_correct_password + ) + backend2_correct_header_with_correct_password_resps.append(backend2_correct_password_resp) + # try each query with each correct apikey + backend2_correct_query_with_correct_password_resps = [] + for query in apikey_policy_2_details.queries: + for key in apikey_policy_2_details.apikeys: + params = {query: key} + host_header = {"host": host} + backend2_correct_password_resp = requests.get( + virtual_server_setup.backend_2_url, headers=host_header, params=params + ) + backend2_correct_query_with_correct_password_resps.append(backend2_correct_password_resp) + + delete_policy(kube_apis.custom_objects, apikey_policy_details.policy_name, test_namespace) + delete_secret(kube_apis.v1, apikey_policy_details.secret_name, test_namespace) + + delete_policy(kube_apis.custom_objects, apikey_policy_2_details.policy_name, test_namespace) + delete_secret(kube_apis.v1, apikey_policy_2_details.secret_name, test_namespace) + + delete_and_create_vs_from_yaml( + kube_apis.custom_objects, + virtual_server_setup.vs_name, + std_vs_src, + virtual_server_setup.namespace, + ) + + # /no-auth (snippet to turn off auth_request on this route) + assert no_auth_resp.status_code == 200 + + # /backend1 (policy on server level) + assert backend1_without_auth_resp.status_code == 401 + + # with wrong password in header + assert len(backend1_correct_header_with_wrong_password_resps) > 0 + for response in backend1_correct_header_with_wrong_password_resps: + assert response.status_code == 403 + + # with wrong password in query + assert len(backend1_correct_query_with_wrong_password_resps) > 0 + for response in backend1_correct_query_with_wrong_password_resps: + assert response.status_code == 403 + + # with correct password in header + assert len(backend1_correct_header_with_correct_password_resps) > 0 + for response in backend1_correct_header_with_correct_password_resps: + assert response.status_code == 200 + + # with correct password in query + assert len(backend1_correct_query_with_correct_password_resps) > 0 + for response in backend1_correct_query_with_correct_password_resps: + assert response.status_code == 200 + + # /backend2 (policy on route level) + assert backend2_without_auth_resp.status_code == 401 + + # with wrong password in header + assert len(backend2_correct_header_with_wrong_password_resps) > 0 + for response in backend2_correct_header_with_wrong_password_resps: + assert response.status_code == 403 + + # with wrong password in query + assert len(backend2_correct_query_with_wrong_password_resps) > 0 + for response in backend2_correct_query_with_wrong_password_resps: + assert response.status_code == 403 + + # with correct password in header + assert len(backend2_correct_header_with_correct_password_resps) > 0 + for response in backend2_correct_header_with_correct_password_resps: + assert response.status_code == 200 + + # with correct password in query + assert len(backend2_correct_query_with_correct_password_resps) > 0 + for response in backend2_correct_query_with_correct_password_resps: + assert response.status_code == 200 + + +@pytest.mark.policies +@pytest.mark.parametrize( + "crd_ingress_controller, virtual_server_setup", + [ + ( + { + "type": "complete", + "extra_args": [ + f"-enable-custom-resources", + f"-enable-leader-election=false", + f"-enable-snippets", + ], + }, + { + "example": "virtual-server", + "app_type": "simple", + }, + ) + ], + indirect=True, +) +class TestAPIKeyAuthPoliciesVSR: + def setup_single_policy( + self, kube_apis, test_namespace: str, secret_src: str, policy_src: str, vs_host: str + ) -> namedtuple: + APIKey_policy_details = namedtuple( + "APIKey_policy_details", ["headers", "queries", "policy_name", "secret_name", "vs_host", "apikeys"] + ) + print(f"Create apikey auth secret") + secret_name = create_secret_from_yaml(kube_apis.v1, test_namespace, secret_src) + apikeys = get_apikey_auth_secrets_from_yaml(secret_src) + details = get_apikey_policy_details_from_yaml(policy_src) + + print(f"Create apikey auth policy") + policy_name = create_policy_from_yaml(kube_apis.custom_objects, policy_src, test_namespace) + wait_before_test() + + headers = details["headers"] + queries = details["queries"] + return APIKey_policy_details( + headers=headers, + queries=queries, + policy_name=policy_name, + secret_name=secret_name, + vs_host=vs_host, + apikeys=apikeys, + ) + + def test_apikey_auth_policy_vs_and_vsr( + self, kube_apis, crd_ingress_controller, virtual_server_setup, test_namespace + ): + apikey_policy_details_server = self.setup_single_policy( + kube_apis, test_namespace, apikey_auth_secret_server, apikey_auth_pol_server, virtual_server_setup.vs_host + ) + + apikey_policy_details_route = self.setup_single_policy( + kube_apis, test_namespace, apikey_auth_secret_route, apikey_auth_pol_route, virtual_server_setup.vs_host + ) + + delete_and_create_vs_from_yaml( + kube_apis.custom_objects, + virtual_server_setup.vs_name, + apikey_auth_vs_vsr_src, + virtual_server_setup.namespace, + ) + create_v_s_route_from_yaml(kube_apis.custom_objects, vsr_1_src, virtual_server_setup.namespace) + create_v_s_route_from_yaml(kube_apis.custom_objects, vsr_2_src, virtual_server_setup.namespace) + + host = virtual_server_setup.vs_host + wait_before_test(5) + + # /no-auth path + no_auth_path_server = ( + f"http://{virtual_server_setup.public_endpoint.public_ip}" + f":{virtual_server_setup.public_endpoint.port}/no-auth" + ) + + no_auth_headers = { + "host": host, + } + no_auth_server_resp = requests.get(no_auth_path_server, headers=no_auth_headers) + + # /backend1 (no policy on this vsr route so uses server level policy) + backend1_path = ( + f"http://{virtual_server_setup.public_endpoint.public_ip}" + f":{virtual_server_setup.public_endpoint.port}/backend1" + ) + backend1_without_auth_headers = {"host": host} + backend1_without_auth_resp = requests.get(backend1_path, headers=backend1_without_auth_headers) + + # with wrong password in header + backend1_correct_header_with_wrong_password_resps = [] + for header in apikey_policy_details_server.headers: + backend1_with_auth_headers = {"host": host, header: "wrongpassword"} + backend1_wrong_password = requests.get(backend1_path, headers=backend1_with_auth_headers) + backend1_correct_header_with_wrong_password_resps.append(backend1_wrong_password) + # with wrong password in query + backend1_correct_query_with_wrong_password_resps = [] + for query in apikey_policy_details_server.queries: + host_header = {"host": host} + params = {query: "wrongpassword"} + backend1_wrong_password_resp = requests.get(backend1_path, headers=host_header, params=params) + backend1_correct_query_with_wrong_password_resps.append(backend1_wrong_password_resp) + # try each header with each correct apikey + backend1_correct_header_with_correct_password_resps = [] + for header in apikey_policy_details_server.headers: + for key in apikey_policy_details_server.apikeys: + backend1_with_auth_headers_correct_password = {"host": host, header: key} + backend1_correct_password_resp = requests.get( + backend1_path, headers=backend1_with_auth_headers_correct_password + ) + backend1_correct_header_with_correct_password_resps.append(backend1_correct_password_resp) + # try each query with each correct apikey + backend1_correct_query_with_correct_password_resps = [] + for query in apikey_policy_details_server.queries: + for key in apikey_policy_details_server.apikeys: + params = {query: key} + host_header = {"host": host} + backend1_correct_password_resp = requests.get(backend1_path, headers=host_header, params=params) + backend1_correct_query_with_correct_password_resps.append(backend1_correct_password_resp) + + # /backend2 path (uses policy on the route level) + backend2_path = ( + f"http://{virtual_server_setup.public_endpoint.public_ip}" + f":{virtual_server_setup.public_endpoint.port}/backend2" + ) + # without auth headers + backend2_without_auth_headers = {"host": host} + backend2_without_auth_resp = requests.get(backend2_path, headers=backend2_without_auth_headers) + # with wrong password in header + backend2_correct_header_with_wrong_password_resps = [] + for header in apikey_policy_details_route.headers: + backend2_with_auth_headers_but_wrong_password = {"host": host, header: "wrongpassword"} + backend2_wrong_password_resp = requests.get( + backend2_path, headers=backend2_with_auth_headers_but_wrong_password + ) + backend2_correct_header_with_wrong_password_resps.append(backend2_wrong_password_resp) + # with wrong password in query + backend2_correct_query_with_wrong_password_resps = [] + for query in apikey_policy_details_route.queries: + host_header = {"host": host} + params = {query: "wrongpassword"} + backend2_wrong_password_resp = requests.get(backend2_path, headers=host_header, params=params) + backend2_correct_query_with_wrong_password_resps.append(backend2_wrong_password_resp) + # try each header with each correct apikey + backend2_correct_header_with_correct_password_resps = [] + for header in apikey_policy_details_route.headers: + for key in apikey_policy_details_route.apikeys: + backend2_with_auth_headers_correct_password = {"host": host, header: key} + backend2_correct_password_resp = requests.get( + backend2_path, headers=backend2_with_auth_headers_correct_password + ) + backend2_correct_header_with_correct_password_resps.append(backend2_correct_password_resp) + # try each query with each correct apikey + backend2_correct_query_with_correct_password_resps = [] + for query in apikey_policy_details_route.queries: + for key in apikey_policy_details_route.apikeys: + params = {query: key} + host_header = {"host": host} + backend2_correct_password_resp = requests.get(backend2_path, headers=host_header, params=params) + backend2_correct_query_with_correct_password_resps.append(backend2_correct_password_resp) + + delete_items_from_yaml(kube_apis.custom_objects, vsr_1_src, virtual_server_setup.namespace) + delete_items_from_yaml(kube_apis.custom_objects, vsr_2_src, virtual_server_setup.namespace) + + delete_policy(kube_apis.custom_objects, apikey_policy_details_server.policy_name, test_namespace) + delete_secret(kube_apis.v1, apikey_policy_details_server.secret_name, test_namespace) + + delete_policy(kube_apis.custom_objects, apikey_policy_details_route.policy_name, test_namespace) + delete_secret(kube_apis.v1, apikey_policy_details_route.secret_name, test_namespace) + + delete_and_create_vs_from_yaml( + kube_apis.custom_objects, + virtual_server_setup.vs_name, + std_vs_src, + virtual_server_setup.namespace, + ) + + # /no-auth (snippet to turn off auth_request on this route) + assert no_auth_server_resp.status_code == 200 + + # /backend1 (policy on server level) + assert backend1_without_auth_resp.status_code == 401 + + # with wrong password in header + assert len(backend1_correct_header_with_wrong_password_resps) > 0 + for response in backend1_correct_header_with_wrong_password_resps: + assert response.status_code == 403 + + assert len(backend1_correct_query_with_wrong_password_resps) > 0 + for response in backend1_correct_query_with_wrong_password_resps: + assert response.status_code == 403 + + # with correct password in header + assert len(backend1_correct_header_with_correct_password_resps) > 0 + for response in backend1_correct_header_with_correct_password_resps: + assert response.status_code == 200 + + # with correct password in query + assert len(backend1_correct_query_with_correct_password_resps) > 0 + for response in backend1_correct_query_with_correct_password_resps: + assert response.status_code == 200 + + # /backend2 (policy on route level) + assert backend2_without_auth_resp.status_code == 401 + + # with wrong password in header + assert len(backend2_correct_header_with_wrong_password_resps) > 0 + for response in backend2_correct_header_with_wrong_password_resps: + assert response.status_code == 403 + + # with wrong password in query + assert len(backend2_correct_query_with_wrong_password_resps) > 0 + for response in backend2_correct_query_with_wrong_password_resps: + assert response.status_code == 403 + + # with correct password in header + assert len(backend2_correct_header_with_correct_password_resps) > 0 + for response in backend2_correct_header_with_correct_password_resps: + assert response.status_code == 200 + + # with correct password in query + assert len(backend2_correct_query_with_correct_password_resps) > 0 + for response in backend2_correct_query_with_correct_password_resps: + assert response.status_code == 200 diff --git a/tests/suite/utils/resources_utils.py b/tests/suite/utils/resources_utils.py index 053f3e3e92..7bf82e6e46 100644 --- a/tests/suite/utils/resources_utils.py +++ b/tests/suite/utils/resources_utils.py @@ -1,5 +1,6 @@ """Describe methods to utilize the kubernetes-client.""" +import base64 import json import os import re @@ -1748,3 +1749,42 @@ def get_resource_metrics(kube_apis, plural, namespace="nginx-ingress") -> str: else: return "Invalid plural specified. Please use 'pods' or 'nodes' as the plural" return metrics["items"] + + +def get_apikey_auth_secrets_from_yaml(yaml_manifest) -> list: + """ + Get apikey auth keys from yaml file. + + :param yaml_manifest: an absolute path to file + :return: []apikeys + """ + api_keys = [] + + with open(yaml_manifest) as file: + data = yaml.safe_load(file) + if "data" in data: + for key, encoded_value in data["data"].items(): + decoded_value = base64.b64decode(encoded_value).decode("utf-8") + api_keys.append(decoded_value) + return api_keys + + +def get_apikey_policy_details_from_yaml(yaml_manifest) -> dict: + """ + Extract headers and queries from an API key policy yaml file. + + :param yaml_manifest: an absolute path to file + :return: dictionary with 'headers' and 'queries' + """ + details = {"headers": [], "queries": []} + + with open(yaml_manifest) as file: + data = yaml.safe_load(file) + + if "spec" in data and "apiKey" in data["spec"] and "suppliedIn" in data["spec"]["apiKey"]: + if "header" in data["spec"]["apiKey"]["suppliedIn"]: + details["headers"] = data["spec"]["apiKey"]["suppliedIn"]["header"] + if "query" in data["spec"]["apiKey"]["suppliedIn"]: + details["queries"] = data["spec"]["apiKey"]["suppliedIn"]["query"] + + return details From 028ff04fc359b48c6bf22141f24e1b5eaa6a82e5 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 12 Jun 2024 11:19:58 +0100 Subject: [PATCH 22/36] fix dockerfile merge --- build/Dockerfile | 31 ------------------------------- build/scripts/common.sh | 2 ++ 2 files changed, 2 insertions(+), 31 deletions(-) diff --git a/build/Dockerfile b/build/Dockerfile index a91ca320b2..7fbbdd2590 100644 --- a/build/Dockerfile +++ b/build/Dockerfile @@ -381,37 +381,6 @@ ARG IC_VERSION ARG TARGETPLATFORM ARG NAP_MODULES=none -# copy oidc files on plus build -RUN --mount=type=bind,target=/tmp [ -n "${BUILD_OS##*plus*}" ] && exit 0; mkdir -p /etc/nginx/oidc/ && cp -a /tmp/internal/configs/oidc/* /etc/nginx/oidc/ - -# run only on nap waf build -RUN --mount=type=bind,target=/tmp [ -n "${NAP_MODULES##*waf*}" ] && exit 0; mkdir -p /etc/nginx/waf/nac-policies /etc/nginx/waf/nac-logconfs /etc/nginx/waf/nac-usersigs /var/log/app_protect /opt/app_protect \ - && chown -R 101:0 /etc/app_protect /usr/share/ts /var/log/app_protect/ /opt/app_protect/ \ - && chmod -R g=u /etc/app_protect /usr/share/ts /var/log/app_protect/ /opt/app_protect/ \ - && touch /etc/nginx/waf/nac-usersigs/index.conf \ - && if [ -z "${NGINX_AGENT##true}" ]; then mkdir -p /etc/ssl/nms /opt/nms-nap-compiler \ - && chown -R 101:0 /etc/ssl/nms /opt/nms-nap-compiler \ - && chmod -R g=u /etc/ssl/nms /opt/nms-nap-compiler \ - && NAP_VERSION=$(cat /opt/app_protect/VERSION) && ln -s /opt/app_protect "/opt/nms-nap-compiler/app_protect-${NAP_VERSION}"; fi - -# run only on nap dos build -RUN [ -n "${NAP_MODULES##*dos*}" ] && exit 0; mkdir -p /root/app_protect_dos /etc/nginx/dos/policies /etc/nginx/dos/logconfs /shared/cores /var/log/adm /var/run/adm \ - && chmod 777 /shared/cores /var/log/adm /var/run/adm /etc/app_protect_dos - -RUN --mount=type=bind,target=/tmp mkdir -p /var/lib/nginx /etc/nginx/secrets /etc/nginx/stream-conf.d \ - && setcap 'cap_net_bind_service=+eip' /usr/sbin/nginx 'cap_net_bind_service=+eip' /usr/sbin/nginx-debug \ - && setcap -v 'cap_net_bind_service=+eip' /usr/sbin/nginx 'cap_net_bind_service=+eip' /usr/sbin/nginx-debug \ - && [ -z "${BUILD_OS##*plus*}" ] && PLUS=-plus; cp -a /tmp/internal/configs/version1/nginx$PLUS.ingress.tmpl /tmp/internal/configs/version1/nginx$PLUS.tmpl \ - /tmp/internal/configs/version2/nginx$PLUS.virtualserver.tmpl /tmp/internal/configs/version2/nginx$PLUS.transportserver.tmpl / \ - && if [ -z "${BUILD_OS##*plus*}" ]; then mkdir -p /etc/nginx/state_files/; fi \ - && mkdir -p /etc/nginx/njs/ && cp -a /tmp/internal/configs/njs/* /etc/nginx/njs/ \ - && chown -R 101:0 /etc/nginx /var/cache/nginx /var/lib/nginx /var/log/nginx /*.tmpl \ - && chmod -R g=u /etc/nginx /var/cache/nginx /var/lib/nginx /var/log/nginx /*.tmpl \ - && rm -f /etc/nginx/conf.d/* - -# Patch OS -RUN --mount=type=bind,from=nginx-files,src=patch-os.sh,target=/usr/local/bin/patch-os.sh \ - patch-os.sh RUN --mount=type=bind,target=/tmp \ --mount=type=bind,from=nginx-files,src=common.sh,target=/usr/local/bin/common.sh \ --mount=type=bind,from=nginx-files,src=patch-os.sh,target=/usr/local/bin/patch-os.sh \ diff --git a/build/scripts/common.sh b/build/scripts/common.sh index 87f69c565e..2a1daa74e4 100755 --- a/build/scripts/common.sh +++ b/build/scripts/common.sh @@ -7,9 +7,11 @@ if [ -z "${BUILD_OS##*plus*}" ]; then mkdir -p /etc/nginx/oidc/ cp -a /tmp/internal/configs/oidc/* /etc/nginx/oidc/ mkdir -p /etc/nginx/state_files/ + PLUS=-plus fi +mkdir -p /etc/nginx/njs/ && cp -a /tmp/internal/configs/njs/* /etc/nginx/njs/ mkdir -p /var/lib/nginx /etc/nginx/secrets /etc/nginx/stream-conf.d setcap 'cap_net_bind_service=+eip' /usr/sbin/nginx 'cap_net_bind_service=+eip' /usr/sbin/nginx-debug setcap -v 'cap_net_bind_service=+eip' /usr/sbin/nginx 'cap_net_bind_service=+eip' /usr/sbin/nginx-debug From 93a1e0b17bd9679ca5abb68e8cf8ec99c37f9e2b Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 12 Jun 2024 13:51:23 +0100 Subject: [PATCH 23/36] add wait until pods are ready --- tests/suite/test_apikey_auth_policies.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index d29aab8be7..f386654d57 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -11,6 +11,7 @@ get_apikey_auth_secrets_from_yaml, get_apikey_policy_details_from_yaml, wait_before_test, + wait_until_all_pods_are_ready, ) from suite.utils.vs_vsr_resources_utils import create_v_s_route_from_yaml, delete_and_create_vs_from_yaml @@ -100,6 +101,7 @@ def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_ host = apikey_policy_details.vs_host + wait_until_all_pods_are_ready(kube_apis.v1, test_namespace) wait_before_test() # /no-auth path @@ -330,6 +332,7 @@ def test_apikey_auth_policy_vs_and_vsr( create_v_s_route_from_yaml(kube_apis.custom_objects, vsr_2_src, virtual_server_setup.namespace) host = virtual_server_setup.vs_host + wait_until_all_pods_are_ready(kube_apis.v1, test_namespace) wait_before_test(5) # /no-auth path From 3ac68d484c7e5e231bcb5e1a0c5b7a8644e60bba Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Wed, 12 Jun 2024 15:48:00 +0100 Subject: [PATCH 24/36] update error message Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- internal/k8s/controller_test.go | 2 +- pkg/apis/configuration/validation/policy.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/k8s/controller_test.go b/internal/k8s/controller_test.go index 50725ea714..988493d337 100644 --- a/internal/k8s/controller_test.go +++ b/internal/k8s/controller_test.go @@ -2090,7 +2090,7 @@ func TestGetPolicies(t *testing.T) { expectedPolicies := []*conf_v1.Policy{validPolicy} expectedErrors := []error{ - errors.New("policy default/invalid-policy is invalid: spec: Invalid value: \"\": must specify exactly one of: `accessControl`, `rateLimit`, `ingressMTLS`, `egressMTLS`, `basicAuth`, `jwt`, `oidc`, `waf`"), + errors.New("policy default/invalid-policy is invalid: spec: Invalid value: \"\": must specify exactly one of: `accessControl`, `rateLimit`, `ingressMTLS`, `egressMTLS`, `basicAuth`, `apiKey`, `jwt`, `oidc`, `waf`"), errors.New("policy nginx-ingress/valid-policy doesn't exist"), errors.New("failed to get policy nginx-ingress/some-policy: GetByKey error"), errors.New("referenced policy default/valid-policy-ingress-class has incorrect ingress class: test-class (controller ingress class: )"), diff --git a/pkg/apis/configuration/validation/policy.go b/pkg/apis/configuration/validation/policy.go index 5b28c7b375..5483cd462a 100644 --- a/pkg/apis/configuration/validation/policy.go +++ b/pkg/apis/configuration/validation/policy.go @@ -91,7 +91,7 @@ func validatePolicySpec(spec *v1.PolicySpec, fieldPath *field.Path, isPlus, enab } if fieldCount != 1 { - msg := "must specify exactly one of: `accessControl`, `rateLimit`, `ingressMTLS`, `egressMTLS`, `basicAuth`" + msg := "must specify exactly one of: `accessControl`, `rateLimit`, `ingressMTLS`, `egressMTLS`, `basicAuth`, `apiKey`" if isPlus { msg = fmt.Sprint(msg, ", `jwt`, `oidc`, `waf`") } From 8c1d9d1943c0843ececa763f7587021c3bbf5f81 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 12 Jun 2024 17:08:53 +0100 Subject: [PATCH 25/36] test setting same namespace --- .github/data/matrix-smoke.json | 7 +++++++ tests/suite/test_apikey_auth_policies.py | 25 ++++++++++++++++++++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/.github/data/matrix-smoke.json b/.github/data/matrix-smoke.json index 332475089b..dee21128c5 100644 --- a/.github/data/matrix-smoke.json +++ b/.github/data/matrix-smoke.json @@ -21,6 +21,13 @@ "marker": "vsr", "platforms": "linux/arm, linux/arm64, linux/amd64, linux/ppc64le, linux/s390x" }, + { + "label": "jim", + "image": "alpine", + "type": "oss", + "marker": "jim", + "platforms": "linux/arm, linux/arm64, linux/amd64, linux/ppc64le, linux/s390x" + }, { "label": "policies 1/2", "image": "alpine", diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index f386654d57..9c66006780 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -35,6 +35,7 @@ std_vs_src = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" +@pytest.mark.jim @pytest.mark.policies @pytest.mark.parametrize( "crd_ingress_controller, virtual_server_setup", @@ -85,11 +86,19 @@ def setup_single_policy( def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_server_setup, test_namespace): apikey_policy_details = self.setup_single_policy( - kube_apis, test_namespace, apikey_auth_secret_1, apikey_auth_pol_valid, virtual_server_setup.vs_host + kube_apis, + virtual_server_setup.namespace, + apikey_auth_secret_1, + apikey_auth_pol_valid, + virtual_server_setup.vs_host, ) apikey_policy_2_details = self.setup_single_policy( - kube_apis, test_namespace, apikey_auth_secret_2, apikey_auth_pol_valid_2, virtual_server_setup.vs_host + kube_apis, + virtual_server_setup.namespace, + apikey_auth_secret_2, + apikey_auth_pol_valid_2, + virtual_server_setup.vs_host, ) delete_and_create_vs_from_yaml( @@ -315,11 +324,19 @@ def test_apikey_auth_policy_vs_and_vsr( self, kube_apis, crd_ingress_controller, virtual_server_setup, test_namespace ): apikey_policy_details_server = self.setup_single_policy( - kube_apis, test_namespace, apikey_auth_secret_server, apikey_auth_pol_server, virtual_server_setup.vs_host + kube_apis, + virtual_server_setup.namespace, + apikey_auth_secret_server, + apikey_auth_pol_server, + virtual_server_setup.vs_host, ) apikey_policy_details_route = self.setup_single_policy( - kube_apis, test_namespace, apikey_auth_secret_route, apikey_auth_pol_route, virtual_server_setup.vs_host + kube_apis, + virtual_server_setup.namespace, + apikey_auth_secret_route, + apikey_auth_pol_route, + virtual_server_setup.vs_host, ) delete_and_create_vs_from_yaml( From 9682420533b9c29c7b5febf9cb43f0fa2723488f Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Wed, 12 Jun 2024 17:32:45 +0100 Subject: [PATCH 26/36] custom objects --- tests/suite/test_apikey_auth_policies.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index 9c66006780..fe9e34eb96 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -93,6 +93,11 @@ def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_ virtual_server_setup.vs_host, ) + result = kube_apis.custom_objects.get_namespaced_custom_object( + "k8s.nginx.org", "v1", test_namespace, "policies", "api-key-policy" + ) + print(result) + apikey_policy_2_details = self.setup_single_policy( kube_apis, virtual_server_setup.namespace, From 48f805468f6a618ddd28f5f1495ef26789ab769e Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 13 Jun 2024 09:31:02 +0100 Subject: [PATCH 27/36] add crd print --- .github/actions/smoke-tests/action.yaml | 2 +- tests/suite/test_apikey_auth_policies.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/.github/actions/smoke-tests/action.yaml b/.github/actions/smoke-tests/action.yaml index a85aba1415..f5bafe474c 100644 --- a/.github/actions/smoke-tests/action.yaml +++ b/.github/actions/smoke-tests/action.yaml @@ -73,6 +73,6 @@ runs: --durations=10 \ --show-ic-logs=yes \ --ad-secret=${{ inputs.azure-ad-secret }} \ - -m ${{ inputs.marker != '' && inputs.marker || '""' }} + -m ${{ inputs.marker != '' && inputs.marker || '""' }} -v -s working-directory: ./tests shell: bash diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index fe9e34eb96..0655990f1a 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -2,6 +2,7 @@ import pytest import requests +from kubernetes.client.rest import ApiException from settings import TEST_DATA from suite.utils.policy_resources_utils import create_policy_from_yaml, delete_policy from suite.utils.resources_utils import ( @@ -98,6 +99,16 @@ def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_ ) print(result) + print("Policies CRD") + api_instance = kube_apis.api_extensions_v1 + name = "policies.k8s.nginx.org" + try: + api_response = api_instance.read_custom_resource_definition(name) + print(api_response) + except ApiException as e: + print("Exception when calling ApiextensionsV1Api->read_custom_resource_definition: %s\n" % e) + print("namespaced custom object") + apikey_policy_2_details = self.setup_single_policy( kube_apis, virtual_server_setup.namespace, From 99ba140146c59eaa54d70c17417fac4f24211f8d Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Thu, 13 Jun 2024 13:04:24 +0100 Subject: [PATCH 28/36] add unit tests 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.go | 3 +- internal/configs/virtualserver_test.go | 509 ++++++++++++++++-- internal/k8s/secrets/validation_test.go | 45 ++ .../configuration/validation/policy_test.go | 92 ++++ 4 files changed, 614 insertions(+), 35 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index dd6cb18f8d..c7dccbd589 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -394,7 +394,6 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( dosResources map[string]*appProtectDosResource, ) (version2.VirtualServerConfig, Warnings) { vsc.clearWarnings() - var maps []version2.Map useCustomListeners := false @@ -418,6 +417,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( vsName: vsEx.VirtualServer.Name, } policiesCfg := vsc.generatePolicies(ownerDetails, vsEx.VirtualServer.Spec.Policies, vsEx.Policies, specContext, policyOpts) + if policiesCfg.JWKSAuthEnabled { jwtAuthKey := policiesCfg.JWTAuth.Key policiesCfg.JWTAuthList = make(map[string]*version2.JWTAuth) @@ -517,6 +517,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig( var internalRedirectLocations []version2.InternalRedirectLocation var returnLocations []version2.ReturnLocation var splitClients []version2.SplitClient + var maps []version2.Map var errorPageLocations []version2.ErrorPageLocation var keyValZones []version2.KeyValZone var keyVals []version2.KeyVal diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 44a0274f62..f3731e2774 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -5765,29 +5765,29 @@ func TestGenerateVirtualServerConfigAPIKeyPolicy(t *testing.T) { Maps: []version2.Map{ { Source: "$apikey_auth_token", - Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_spec", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_route", Parameters: []version2.Parameter{ { Value: "default", Result: `""`, }, { - Value: `"5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8"`, - Result: `"clientSpec"`, + Value: `"6cf615d5bcaac778352a8f1f3360d23f02f34ec182e259897fd6ce485d7870d4"`, + Result: `"clientRoute"`, }, }, }, { Source: "$apikey_auth_token", - Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_route", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_spec", Parameters: []version2.Parameter{ { Value: "default", Result: `""`, }, { - Value: `"6cf615d5bcaac778352a8f1f3360d23f02f34ec182e259897fd6ce485d7870d4"`, - Result: `"clientRoute"`, + Value: `"5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8"`, + Result: `"clientSpec"`, }, }, }, @@ -5901,6 +5901,10 @@ func TestGenerateVirtualServerConfigAPIKeyPolicy(t *testing.T) { 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) } @@ -5910,6 +5914,307 @@ func TestGenerateVirtualServerConfigAPIKeyPolicy(t *testing.T) { } } +func TestGenerateVirtualServerConfigAPIKeyClientMaps(t *testing.T) { + t.Parallel() + + virtualServerEx := VirtualServerEx{ + SecretRefs: map[string]*secrets.SecretReference{ + "default/api-key-secret-1": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("password"), + }, + }, + }, + "default/api-key-secret-2": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client2": []byte("password2"), + }, + }, + }, + }, + VirtualServer: &conf_v1.VirtualServer{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "cafe", + Namespace: "default", + }, + Spec: conf_v1.VirtualServerSpec{ + Host: "cafe.example.com", + 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/api-key-policy-1": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-1", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"apikey"}, + }, + ClientSecret: "api-key-secret-1", + }, + }, + }, + "default/api-key-policy-2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-2", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"api-key"}, + }, + ClientSecret: "api-key-secret-2", + }, + }, + }, + }, + Endpoints: map[string][]string{ + "default/tea-svc:80": { + "10.0.0.20:80", + }, + "default/coffee-svc:80": { + "10.0.0.30:80", + }, + }, + } + + expectedAPIKey1 := &version2.APIKey{ + MapName: "apikey_auth_client_name_default_cafe_api_key_policy_1", + Header: []string{"X-API-Key"}, + Query: []string{"apikey"}, + } + + expectedAPIKey2 := &version2.APIKey{ + MapName: "apikey_auth_client_name_default_cafe_api_key_policy_2", + Header: []string{"api-key"}, + } + + expectedMap1 := version2.Map{ + Source: "$apikey_auth_token", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_1", + Parameters: []version2.Parameter{ + { + Value: "default", + Result: `""`, + }, + { + Value: `"5e884898da28047151d0e56f8dc6292773603d0d6aabbdd62a11ef721d1542d8"`, + Result: `"client1"`, + }, + }, + } + + expectedMap2 := version2.Map{ + Source: "$apikey_auth_token", + Variable: "$apikey_auth_client_name_default_cafe_api_key_policy_2", + Parameters: []version2.Parameter{ + { + Value: "default", + Result: `""`, + }, + { + Value: `"6cf615d5bcaac778352a8f1f3360d23f02f34ec182e259897fd6ce485d7870d4"`, + Result: `"client2"`, + }, + }, + } + + baseCfgParams := ConfigParams{ + 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, + ) + + tests := []struct { + specPolicies []conf_v1.PolicyReference + route1Policies []conf_v1.PolicyReference + route2Policies []conf_v1.PolicyReference + expectedSpecAPIKey *version2.APIKey + expectedRoute1APIKey *version2.APIKey + expectedRoute2APIKey *version2.APIKey + expectedMapList []version2.Map + name string + }{ + { + specPolicies: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-1", + }, + }, + route1Policies: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-2", + }, + }, + route2Policies: nil, + expectedSpecAPIKey: expectedAPIKey1, + expectedRoute1APIKey: expectedAPIKey2, + expectedRoute2APIKey: nil, + expectedMapList: []version2.Map{expectedMap1, expectedMap2}, + name: "policy in spec, route 1 and route 2", + }, + { + specPolicies: nil, + route1Policies: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-1", + }, + }, + route2Policies: nil, + expectedSpecAPIKey: nil, + + expectedRoute1APIKey: expectedAPIKey1, + expectedRoute2APIKey: nil, + expectedMapList: []version2.Map{expectedMap1}, + name: "policy in route 1 only", + }, + { + specPolicies: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-2", + }, + }, + route1Policies: nil, + route2Policies: nil, + expectedSpecAPIKey: expectedAPIKey2, + expectedRoute1APIKey: nil, + expectedRoute2APIKey: nil, + expectedMapList: []version2.Map{expectedMap2}, + name: "policy in spec only", + }, + { + specPolicies: nil, + route1Policies: nil, + route2Policies: nil, + expectedRoute1APIKey: nil, + expectedRoute2APIKey: nil, + expectedMapList: nil, + name: "no policies", + }, + } + + invalidTests := []struct { + specPolicies []conf_v1.PolicyReference + teaPolicies []conf_v1.PolicyReference + coffeePolicies []conf_v1.PolicyReference + expectedMapList []version2.Map + expectedWarnings Warnings + name string + }{ + { + specPolicies: []conf_v1.PolicyReference{ + { + Name: "api-key-policy-3", + }, + }, + coffeePolicies: nil, + teaPolicies: nil, + // expectedTeaPolicy: expectedAPIKey2, + // expectedCoffeePolicy: expectedAPIKey1, + expectedMapList: nil, + expectedWarnings: Warnings{ + nil: { + "Policy default/api-key-policy-3 is missing or invalid", + }, + }, + name: "policy does not exist", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + virtualServerEx.VirtualServer.Spec.Policies = tc.specPolicies + virtualServerEx.VirtualServer.Spec.Routes[0].Policies = tc.route1Policies + virtualServerEx.VirtualServer.Spec.Routes[1].Policies = tc.route2Policies + vsConf, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx, nil, nil) + + sort.Slice(vsConf.Maps, func(i, j int) bool { + return vsConf.Maps[i].Variable < vsConf.Maps[j].Variable + }) + + if !cmp.Equal(tc.expectedSpecAPIKey, vsConf.Server.APIKey) { + t.Errorf(cmp.Diff(tc.expectedSpecAPIKey, vsConf.Server.APIKey)) + } + + if !cmp.Equal(tc.expectedRoute1APIKey, vsConf.Server.Locations[0].APIKey) { + t.Errorf(cmp.Diff(tc.expectedRoute1APIKey, vsConf.Server.Locations[0].APIKey)) + } + + if !cmp.Equal(tc.expectedRoute2APIKey, vsConf.Server.Locations[1].APIKey) { + t.Errorf(cmp.Diff(tc.expectedRoute2APIKey, vsConf.Server.Locations[1].APIKey)) + } + + if !cmp.Equal(tc.expectedMapList, vsConf.Maps) { + t.Errorf(cmp.Diff(tc.expectedMapList, vsConf.Maps)) + } + + if len(warnings) != 0 { + t.Errorf("GenerateVirtualServerConfig returned warnings: %v", vsc.warnings) + } + }) + + for _, tc := range invalidTests { + t.Run(tc.name, func(t *testing.T) { + virtualServerEx.VirtualServer.Spec.Policies = tc.specPolicies + virtualServerEx.VirtualServer.Spec.Routes[0].Policies = tc.teaPolicies + virtualServerEx.VirtualServer.Spec.Routes[1].Policies = tc.coffeePolicies + _, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx, nil, nil) + + if len(warnings) == 0 { + t.Errorf("GenerateVirtualServerConfig() does not return the expected error %v", tc.expectedWarnings) + } + }) + } + } +} + func TestGeneratePolicies(t *testing.T) { t.Parallel() ownerDetails := policyOwnerDetails{ @@ -8118,10 +8423,146 @@ func TestGeneratePoliciesFails(t *testing.T) { }, }, expectedOidc: &oidcPolicyCfg{}, - msg: "multi api key", + msg: "api key multi api key policies", + }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "api-key-policy", + Namespace: "default", + }, + { + Name: "api-key-policy-2", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/api-key-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + "default/api-key-policy-2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-2", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + }, + policyOpts: policyOptions{ + secretRefs: map[string]*secrets.SecretReference{ + "default/api-key-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeJWK, + Data: map[string][]byte{ + "client1": []byte("password"), + }, + }, + }, + }, + }, + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `API Key policy default/api-key-policy references a secret default/api-key-secret of a wrong type 'nginx.org/jwk', must be 'nginx.org/apikey'`, + }, + }, + expectedOidc: &oidcPolicyCfg{}, + msg: "api key referencing wrong secret type", + }, + { + policyRefs: []conf_v1.PolicyReference{ + { + Name: "api-key-policy", + Namespace: "default", + }, + { + Name: "api-key-policy-2", + Namespace: "default", + }, + }, + policies: map[string]*conf_v1.Policy{ + "default/api-key-policy": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + "default/api-key-policy-2": { + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-policy-2", + Namespace: "default", + }, + Spec: conf_v1.PolicySpec{ + APIKey: &conf_v1.APIKey{ + SuppliedIn: &conf_v1.SuppliedIn{ + Header: []string{"X-API-Key"}, + Query: []string{"api-key"}, + }, + ClientSecret: "api-key-secret", + }, + }, + }, + }, + policyOpts: policyOptions{ + secretRefs: map[string]*secrets.SecretReference{ + "default/api-key-secret": { + Secret: &api_v1.Secret{ + Type: secrets.SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("password"), + "client2": []byte("password"), + }, + }, + Error: errors.New("secret is invalid"), // TODO: find correct error + }, + }, + }, + expected: policiesCfg{ + ErrorReturn: &version2.Return{ + Code: 500, + }, + }, + expectedWarnings: Warnings{ + nil: { + `API Key default/api-key-policy references an invalid secret default/api-key-secret: secret is invalid`, + }, + }, + expectedOidc: &oidcPolicyCfg{}, + msg: "api key referencing invalid api key secrets", }, - // TODO:wrong secret type - // TODO: secret invalid { policyRefs: []conf_v1.PolicyReference{ { @@ -8187,33 +8628,33 @@ func TestGeneratePoliciesFails(t *testing.T) { } for _, test := range tests { - vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) + t.Run(test.msg, func(t *testing.T) { + vsc := newVirtualServerConfigurator(&ConfigParams{}, false, false, &StaticConfigParams{}, false, &fakeBV) - if test.oidcPolCfg != nil { - vsc.oidcPolCfg = test.oidcPolCfg - } + if test.oidcPolCfg != nil { + vsc.oidcPolCfg = test.oidcPolCfg + } - result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) - // TODO test maps - // println(maps) // Temporty print to use the variable - result.BundleValidator = nil - if diff := cmp.Diff(test.expected, result); diff != "" { - t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) - } - if !reflect.DeepEqual(vsc.warnings, test.expectedWarnings) { - t.Errorf( - "generatePolicies() returned warnings of \n%v but expected \n%v for the case of %s", - vsc.warnings, - test.expectedWarnings, - test.msg, - ) - } - if diff := cmp.Diff(test.expectedOidc.oidc, vsc.oidcPolCfg.oidc); diff != "" { - t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) - } - if diff := cmp.Diff(test.expectedOidc.key, vsc.oidcPolCfg.key); diff != "" { - t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) - } + result := vsc.generatePolicies(ownerDetails, test.policyRefs, test.policies, test.context, test.policyOpts) + result.BundleValidator = nil + if diff := cmp.Diff(test.expected, result); diff != "" { + t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) + } + if !reflect.DeepEqual(vsc.warnings, test.expectedWarnings) { + t.Errorf( + "generatePolicies() returned warnings of \n%v but expected \n%v for the case of %s", + vsc.warnings, + test.expectedWarnings, + test.msg, + ) + } + if diff := cmp.Diff(test.expectedOidc.oidc, vsc.oidcPolCfg.oidc); diff != "" { + t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) + } + if diff := cmp.Diff(test.expectedOidc.key, vsc.oidcPolCfg.key); diff != "" { + t.Errorf("generatePolicies() '%v' mismatch (-want +got):\n%s", test.msg, diff) + } + }) } } diff --git a/internal/k8s/secrets/validation_test.go b/internal/k8s/secrets/validation_test.go index 4297c9788f..186d63f5d9 100644 --- a/internal/k8s/secrets/validation_test.go +++ b/internal/k8s/secrets/validation_test.go @@ -119,6 +119,20 @@ func TestValidateValidateAPIKeyFails(t *testing.T) { }, msg: "repeated API Keys for API Key secret", }, + { + secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key-secret", + Namespace: "default", + }, + Type: SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte(""), + "client2": []byte(""), + }, + }, + msg: "repeated empty API Keys for API Key secret", + }, } for _, test := range tests { @@ -515,6 +529,19 @@ func TestValidateSecret(t *testing.T) { }, msg: "Valid OIDC secret", }, + { + secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key", + Namespace: "default", + }, + Type: SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("cGFzc3dvcmQ="), + }, + }, + msg: "Valid API Key secret", + }, } for _, test := range tests { @@ -574,6 +601,20 @@ func TestValidateSecretFails(t *testing.T) { }, msg: "Missing htpasswd for Htpasswd secret", }, + { + secret: &v1.Secret{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: "api-key", + Namespace: "default", + }, + Type: SecretTypeAPIKey, + Data: map[string][]byte{ + "client1": []byte("cGFzc3dvcmQ="), + "client2": []byte("cGFzc3dvcmQ="), + }, + }, + msg: "duplicated API Keys in API Key secret", + }, } for _, test := range tests { @@ -610,6 +651,10 @@ func TestHasCorrectSecretType(t *testing.T) { secretType: SecretTypeHtpasswd, expected: true, }, + { + secretType: SecretTypeAPIKey, + expected: true, + }, { secretType: "some-type", expected: false, diff --git a/pkg/apis/configuration/validation/policy_test.go b/pkg/apis/configuration/validation/policy_test.go index 36a4736d71..6a66806b54 100644 --- a/pkg/apis/configuration/validation/policy_test.go +++ b/pkg/apis/configuration/validation/policy_test.go @@ -1303,6 +1303,98 @@ func TestValidateOIDC_PassesOnValidOIDC(t *testing.T) { } } +func TestValidateAPIKeyPolicy_PassOnValidInput(t *testing.T) { + t.Parallel() + tests := []struct { + apiKey *v1.APIKey + msg string + }{ + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{ + Header: []string{ + "X-API-Key", + }, + }, + ClientSecret: "secret", + }, + }, + } + + for _, test := range tests { + allErrs := validateAPIKey(test.apiKey, field.NewPath("apiKey")) + if len(allErrs) != 0 { + t.Errorf("validateAPIKey() returned errors %v for valid input for the case of %v", allErrs, test.msg) + } + } +} + +func TestValidateAPIKeyPolicy_FailsOnInvalidInput(t *testing.T) { + t.Parallel() + tests := []struct { + apiKey *v1.APIKey + msg string + }{ + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{ + Query: []string{ + "api_key", + }, + }, + }, + msg: "missing secret", + }, + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{}, + ClientSecret: "secret", + }, + msg: "both header and query are missing", + }, + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{ + Header: []string{ + `api.key"`, + }, + }, + ClientSecret: "secret", + }, + msg: "invalid header", + }, + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{ + Query: []string{ + `api_key\`, + }, + }, + ClientSecret: "secret", + }, + msg: "invalid query", + }, + { + apiKey: &v1.APIKey{ + SuppliedIn: &v1.SuppliedIn{ + Query: []string{ + `api_key`, + }, + }, + ClientSecret: "secret_1", + }, + msg: "invalid secret name", + }, + } + + for _, test := range tests { + allErrs := validateAPIKey(test.apiKey, field.NewPath("apiKey")) + if len(allErrs) == 0 { + t.Errorf("validateAPIKey() returned no errors for invalid input for the case of %v", test.msg) + } + } +} + func TestValidateOIDCScope_ErrorsOnInvalidInput(t *testing.T) { t.Parallel() From 1f5052549b3b408b40998a88b8a6a5d18b87fc57 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 13 Jun 2024 14:24:14 +0100 Subject: [PATCH 29/36] Add example readme for apikey auth policy --- examples/custom-resources/api-key/README.md | 122 ++++++++++++++++++ .../api-key/api-key-policy-2.yaml | 12 -- .../api-key/api-key-policy.yaml | 4 +- .../api-key/api-key-secret-1.yaml | 8 -- .../api-key/api-key-secret-2.yaml | 7 - .../api-key/api-key-secret.yaml | 8 ++ examples/custom-resources/api-key/cafe-2.yaml | 65 ---------- .../api-key/cafe-virtual-server-2.yaml | 31 ----- .../api-key/cafe-virtual-server.yaml | 17 ++- .../api-key/coffee-virtual-server-route.yaml | 21 --- .../api-key/tea-virtual-server-route.yaml | 14 -- 11 files changed, 145 insertions(+), 164 deletions(-) create mode 100644 examples/custom-resources/api-key/README.md delete mode 100644 examples/custom-resources/api-key/api-key-policy-2.yaml delete mode 100644 examples/custom-resources/api-key/api-key-secret-1.yaml delete mode 100644 examples/custom-resources/api-key/api-key-secret-2.yaml create mode 100644 examples/custom-resources/api-key/api-key-secret.yaml delete mode 100644 examples/custom-resources/api-key/cafe-2.yaml delete mode 100644 examples/custom-resources/api-key/cafe-virtual-server-2.yaml delete mode 100644 examples/custom-resources/api-key/coffee-virtual-server-route.yaml delete mode 100644 examples/custom-resources/api-key/tea-virtual-server-route.yaml diff --git a/examples/custom-resources/api-key/README.md b/examples/custom-resources/api-key/README.md new file mode 100644 index 0000000000..c57b9c4d62 --- /dev/null +++ b/examples/custom-resources/api-key/README.md @@ -0,0 +1,122 @@ +# API Key Authentication + +NGINX supports authenticating requests with +[ngx_http_auth_request_module](https://nginx.org/en/docs/http/ngx_http_auth_request_module.html). In this example, we deploy +a web application, configure load balancing for it via a VirtualServer, and apply an API Key Auth policy. + +## Prerequisites + +1. Follow the [installation](https://docs.nginx.com/nginx-ingress-controller/installation/installation-with-manifests/) + instructions to deploy the Ingress Controller. In this example we will be using a snippet to turn the policy off on a specific path so ensure that the `enable-snippets` flag is set. +1. Save the public IP address of the Ingress Controller into a shell variable: + + ```console + IC_IP=XXX.YYY.ZZZ.III + ``` + +1. Save the HTTP port of the Ingress Controller into a shell variable: + + ```console + IC_HTTP_PORT= + ``` + +## Step 1 - Deploy a Web Application + +Create the application deployment and service: + +```console +kubectl apply -f cafe.yaml +``` + +## Step 2 - Deploy the API Key Auth Secret + +Create a secret of type `nginx.org/apikey` with the name `api-key-client-secret` that will be used for authorization on the server level. + +This secret will contain a mapping of client IDs to base64 encoded API Keys. + +```console +kubectl apply -f api-key-secret.yaml +``` + +## Step 3 - Deploy the API Key Auth Policy + +Create a policy with the name `api-key-policy` that references the secret from the previous step in the clientSecret field. +Provide an array of headers and queries in the header and query fields of the suppliedIn field, indicating where the API key can be sent + +```console +kubectl apply -f api-key-policy.yaml +``` + +## Step 4 - Configure Load Balancing + +Create a VirtualServer resource for the web application: + +```console +kubectl apply -f cafe-virtual-server.yaml +``` + +Note that the VirtualServer references the policy `api-key-policy` created in Step 3. + +## Step 5 - Test the Configuration + +If you attempt to access the application without providing a valid API Key in a expected header or query param for that VirtualServer: + +```console +curl --resolve cafe.example.com:$IC_HTTP_PORT:$IC_IP http://cafe.example.com:$IC_HTTP_PORT/ +``` + +```text + +401 Authorization Required + +

401 Authorization Required

+
nginx/1.21.5
+ + +``` + +If you attempt to access the application providing an incorrect API Key in an expected header or query param for that VirtualServer: + +```console +curl --resolve cafe.example.com:$IC_HTTP_PORT:$IC_IP -H "X-header-name: wrongpassword" http://cafe.example.com:$IC_HTTP_PORT/coffee +``` + +```text + +403 Forbidden + +

403 Forbidden

+
nginx/1.27.0
+ + +``` + +If you provide a valid API Key in an a header or query defined in the policy, your request will succeed: + +```console +curl --resolve cafe.example.com:$IC_HTTPS_PORT:$IC_IP -H "X-header-name: password" https://cafe.example.com:$IC_HTTPS_PORT/coffee +``` + +```text +Server address: 10.244.0.6:8080 +Server name: coffee-56b44d4c55-vjwxd +Date: 13/Jun/2024:13:12:17 +0000 +URI: /coffee +Request ID: 4feedb3265a0430a1f58831d016e846d +``` + +If you attempt to access the /tea path, the request will be allowed without an API Key, because the auth_request directive is turned off for that path with a location snippet: + +```console +curl --resolve cafe.example.com:$IC_HTTP_PORT:$IC_IP http://cafe.example.com:$IC_HTTP_PORT/tea +``` + +```text +Server address: 10.244.0.5:8080 +Server name: tea-596697966f-dmq7t +Date: 13/Jun/2024:13:16:46 +0000 +URI: /tea +Request ID: 26e6d7dd0272eca82f31f33bf90698c9 +``` + +Additionally you can set [error pages](https://docs.nginx.com/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/#errorpage) to handle the 401 and 403 responses. diff --git a/examples/custom-resources/api-key/api-key-policy-2.yaml b/examples/custom-resources/api-key/api-key-policy-2.yaml deleted file mode 100644 index 2870eb1716..0000000000 --- a/examples/custom-resources/api-key/api-key-policy-2.yaml +++ /dev/null @@ -1,12 +0,0 @@ -apiVersion: k8s.nginx.org/v1 -kind: Policy -metadata: - name: api-key-policy-2 -spec: - apiKey: - suppliedIn: - header: - - "X-header-name" - query: - - "queryName" - clientSecret: api-key-client-secret-2 diff --git a/examples/custom-resources/api-key/api-key-policy.yaml b/examples/custom-resources/api-key/api-key-policy.yaml index 30c45bbd3b..1cbf203f45 100644 --- a/examples/custom-resources/api-key/api-key-policy.yaml +++ b/examples/custom-resources/api-key/api-key-policy.yaml @@ -7,8 +7,6 @@ spec: suppliedIn: header: - "X-header-name" - - "apikey" - - "some-other-header" query: - "queryName" - clientSecret: api-key-client-secret-1 + clientSecret: api-key-client-secret diff --git a/examples/custom-resources/api-key/api-key-secret-1.yaml b/examples/custom-resources/api-key/api-key-secret-1.yaml deleted file mode 100644 index 318529da6a..0000000000 --- a/examples/custom-resources/api-key/api-key-secret-1.yaml +++ /dev/null @@ -1,8 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - name: api-key-client-secret-1 -type: nginx.org/apikey -data: - client1: cGFzc3dvcmQ= - client2: N2ViNDMwOGItY2Q1Yi00NDEzLWI0NTUtYjMyZmQ4OTg2MmZk diff --git a/examples/custom-resources/api-key/api-key-secret-2.yaml b/examples/custom-resources/api-key/api-key-secret-2.yaml deleted file mode 100644 index bab71a1872..0000000000 --- a/examples/custom-resources/api-key/api-key-secret-2.yaml +++ /dev/null @@ -1,7 +0,0 @@ -apiVersion: v1 -kind: Secret -metadata: - name: api-key-client-secret-2 -type: nginx.org/apikey -data: - client1: cGFzc3dvcmQ= diff --git a/examples/custom-resources/api-key/api-key-secret.yaml b/examples/custom-resources/api-key/api-key-secret.yaml new file mode 100644 index 0000000000..7ae712d917 --- /dev/null +++ b/examples/custom-resources/api-key/api-key-secret.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: Secret +metadata: + name: api-key-client-secret +type: nginx.org/apikey +data: + client1: cGFzc3dvcmQ= # password + client2: YW5vdGhlci1wYXNzd29yZA== # another-password diff --git a/examples/custom-resources/api-key/cafe-2.yaml b/examples/custom-resources/api-key/cafe-2.yaml deleted file mode 100644 index f049e8bf29..0000000000 --- a/examples/custom-resources/api-key/cafe-2.yaml +++ /dev/null @@ -1,65 +0,0 @@ -apiVersion: apps/v1 -kind: Deployment -metadata: - name: coffee -spec: - replicas: 2 - selector: - matchLabels: - app: coffee - template: - metadata: - labels: - app: coffee - spec: - containers: - - name: coffee - image: nginxdemos/nginx-hello:plain-text - ports: - - containerPort: 8080 ---- -apiVersion: v1 -kind: Service -metadata: - name: coffee-svc -spec: - ports: - - port: 80 - targetPort: 8080 - protocol: TCP - name: http - selector: - app: coffee ---- -apiVersion: apps/v1 -kind: Deployment -metadata: - name: tea -spec: - replicas: 1 - selector: - matchLabels: - app: tea - template: - metadata: - labels: - app: tea - spec: - containers: - - name: tea - image: nginxdemos/nginx-hello:plain-text - ports: - - containerPort: 8080 ---- -apiVersion: v1 -kind: Service -metadata: - name: tea-svc -spec: - ports: - - port: 80 - targetPort: 8080 - protocol: TCP - name: http - selector: - app: tea diff --git a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml b/examples/custom-resources/api-key/cafe-virtual-server-2.yaml deleted file mode 100644 index 7b26ce35a4..0000000000 --- a/examples/custom-resources/api-key/cafe-virtual-server-2.yaml +++ /dev/null @@ -1,31 +0,0 @@ -apiVersion: k8s.nginx.org/v1 -kind: VirtualServer -metadata: - name: cafe-2 -spec: - host: cafe-2.example.com - tls: - secret: cafe-secret - policies: - - name: api-key-policy - upstreams: - - name: tea - service: tea-svc - port: 80 - - name: coffee - service: coffee-svc - port: 80 - routes: - - path: /tea - action: - pass: tea - - path: /coffee - action: - pass: coffee - policies: - - name: api-key-policy-2 - - path: /coffee2 - action: - pass: coffee - policies: - - name: api-key-policy diff --git a/examples/custom-resources/api-key/cafe-virtual-server.yaml b/examples/custom-resources/api-key/cafe-virtual-server.yaml index ecc0b61b6f..b523112eb6 100644 --- a/examples/custom-resources/api-key/cafe-virtual-server.yaml +++ b/examples/custom-resources/api-key/cafe-virtual-server.yaml @@ -8,8 +8,19 @@ spec: secret: cafe-secret policies: - name: api-key-policy + upstreams: + - name: coffee + service: coffee-svc + port: 80 + - name: tea + service: tea-svc + port: 80 routes: - - path: /tea - route: default/tea - path: /coffee - route: default/coffee + action: + pass: coffee + - path: /tea + location-snippets: | + auth_request off; + action: + pass: tea diff --git a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml b/examples/custom-resources/api-key/coffee-virtual-server-route.yaml deleted file mode 100644 index ab248ba010..0000000000 --- a/examples/custom-resources/api-key/coffee-virtual-server-route.yaml +++ /dev/null @@ -1,21 +0,0 @@ -apiVersion: k8s.nginx.org/v1 -kind: VirtualServerRoute -metadata: - name: coffee -spec: - host: cafe.example.com - upstreams: - - name: coffee - service: coffee-svc - port: 80 - subroutes: - - path: /coffee - action: - pass: coffee - policies: - - name: api-key-policy-2 - - path: /coffee2 - action: - pass: coffee - policies: - - name: api-key-policy diff --git a/examples/custom-resources/api-key/tea-virtual-server-route.yaml b/examples/custom-resources/api-key/tea-virtual-server-route.yaml deleted file mode 100644 index db445c0280..0000000000 --- a/examples/custom-resources/api-key/tea-virtual-server-route.yaml +++ /dev/null @@ -1,14 +0,0 @@ -apiVersion: k8s.nginx.org/v1 -kind: VirtualServerRoute -metadata: - name: tea -spec: - host: cafe.example.com - upstreams: - - name: tea - service: tea-svc - port: 80 - subroutes: - - path: /tea - action: - pass: tea From e9a0a75e9bd314dc68bf505dfae81dedcbbfdf77 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 13 Jun 2024 16:11:52 +0100 Subject: [PATCH 30/36] clean up --- .github/actions/smoke-tests/action.yaml | 2 +- .github/data/matrix-smoke.json | 7 ------- build/scripts/common.sh | 1 - .../version2/nginx-plus.virtualserver.tmpl | 1 - .../configs/version2/nginx.virtualserver.tmpl | 1 - .../secret/apikey-secret-1.yaml | 1 - .../secret/apikey-secret-2.yaml | 1 - .../spec/virtual-server-policy-single.yaml | 1 - tests/suite/test_apikey_auth_policies.py | 16 ---------------- 9 files changed, 1 insertion(+), 30 deletions(-) diff --git a/.github/actions/smoke-tests/action.yaml b/.github/actions/smoke-tests/action.yaml index f5bafe474c..a85aba1415 100644 --- a/.github/actions/smoke-tests/action.yaml +++ b/.github/actions/smoke-tests/action.yaml @@ -73,6 +73,6 @@ runs: --durations=10 \ --show-ic-logs=yes \ --ad-secret=${{ inputs.azure-ad-secret }} \ - -m ${{ inputs.marker != '' && inputs.marker || '""' }} -v -s + -m ${{ inputs.marker != '' && inputs.marker || '""' }} working-directory: ./tests shell: bash diff --git a/.github/data/matrix-smoke.json b/.github/data/matrix-smoke.json index dee21128c5..332475089b 100644 --- a/.github/data/matrix-smoke.json +++ b/.github/data/matrix-smoke.json @@ -21,13 +21,6 @@ "marker": "vsr", "platforms": "linux/arm, linux/arm64, linux/amd64, linux/ppc64le, linux/s390x" }, - { - "label": "jim", - "image": "alpine", - "type": "oss", - "marker": "jim", - "platforms": "linux/arm, linux/arm64, linux/amd64, linux/ppc64le, linux/s390x" - }, { "label": "policies 1/2", "image": "alpine", diff --git a/build/scripts/common.sh b/build/scripts/common.sh index 2a1daa74e4..0fe5559b72 100755 --- a/build/scripts/common.sh +++ b/build/scripts/common.sh @@ -7,7 +7,6 @@ if [ -z "${BUILD_OS##*plus*}" ]; then mkdir -p /etc/nginx/oidc/ cp -a /tmp/internal/configs/oidc/* /etc/nginx/oidc/ mkdir -p /etc/nginx/state_files/ - PLUS=-plus fi diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl index e08b8ae744..0715915457 100644 --- a/internal/configs/version2/nginx-plus.virtualserver.tmpl +++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl @@ -222,7 +222,6 @@ server { } {{- end }} - # TODO: Do this conditionally {{- if $s.APIKeyEnabled}} location = /_validate_apikey_njs { internal; diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl index ea1eff0d90..3baf6eaec9 100644 --- a/internal/configs/version2/nginx.virtualserver.tmpl +++ b/internal/configs/version2/nginx.virtualserver.tmpl @@ -143,7 +143,6 @@ server { {{ if $rl.Delay }} delay={{ $rl.Delay }}{{ end }}{{ if $rl.NoDelay }} nodelay{{ end }}; {{- end }} - # TODO: Do this conditionally {{- if $s.APIKeyEnabled}} location = /_validate_apikey_njs { internal; diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml index 3048e36bc4..1096e53bbc 100644 --- a/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-1.yaml @@ -1,4 +1,3 @@ - apiVersion: v1 kind: Secret metadata: diff --git a/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml b/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml index a4d6c5860b..80003c22b5 100644 --- a/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml +++ b/tests/data/apikey-auth-policy/secret/apikey-secret-2.yaml @@ -1,4 +1,3 @@ - apiVersion: v1 kind: Secret metadata: diff --git a/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml b/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml index e69e7fc2fa..682caf4179 100644 --- a/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml +++ b/tests/data/apikey-auth-policy/spec/virtual-server-policy-single.yaml @@ -1,4 +1,3 @@ - apiVersion: k8s.nginx.org/v1 kind: VirtualServer metadata: diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index 0655990f1a..9c66006780 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -2,7 +2,6 @@ import pytest import requests -from kubernetes.client.rest import ApiException from settings import TEST_DATA from suite.utils.policy_resources_utils import create_policy_from_yaml, delete_policy from suite.utils.resources_utils import ( @@ -94,21 +93,6 @@ def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_ virtual_server_setup.vs_host, ) - result = kube_apis.custom_objects.get_namespaced_custom_object( - "k8s.nginx.org", "v1", test_namespace, "policies", "api-key-policy" - ) - print(result) - - print("Policies CRD") - api_instance = kube_apis.api_extensions_v1 - name = "policies.k8s.nginx.org" - try: - api_response = api_instance.read_custom_resource_definition(name) - print(api_response) - except ApiException as e: - print("Exception when calling ApiextensionsV1Api->read_custom_resource_definition: %s\n" % e) - print("namespaced custom object") - apikey_policy_2_details = self.setup_single_policy( kube_apis, virtual_server_setup.namespace, From 9806d6b00aa70106baa6499baefd876ec65a58fe Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 13 Jun 2024 16:34:51 +0100 Subject: [PATCH 31/36] further cleanup --- internal/configs/version1/nginx-plus.tmpl | 2 +- internal/configs/version1/nginx.tmpl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/configs/version1/nginx-plus.tmpl b/internal/configs/version1/nginx-plus.tmpl index b417cb0ea6..c62c8b71f1 100644 --- a/internal/configs/version1/nginx-plus.tmpl +++ b/internal/configs/version1/nginx-plus.tmpl @@ -39,7 +39,7 @@ http { map_hash_max_size {{.MapHashMaxSize}}; map_hash_bucket_size {{.MapHashBucketSize}}; - js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + js_import /etc/nginx/njs/apikey_auth.js; js_set $apikey_auth_hash apikey_auth.hash; {{- if .HTTPSnippets}} diff --git a/internal/configs/version1/nginx.tmpl b/internal/configs/version1/nginx.tmpl index 9d9db599d6..f4f46243ee 100644 --- a/internal/configs/version1/nginx.tmpl +++ b/internal/configs/version1/nginx.tmpl @@ -33,7 +33,7 @@ http { map_hash_bucket_size {{.MapHashBucketSize}}; - js_import /etc/nginx/njs/apikey_auth.js; # TODO: only import if its needed + js_import /etc/nginx/njs/apikey_auth.js; js_set $apikey_auth_hash apikey_auth.hash; {{- if .HTTPSnippets}} From 8956744abef36e9e1c1bacdd69682da6bf8a75ac Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Thu, 13 Jun 2024 16:42:09 +0100 Subject: [PATCH 32/36] clean up test --- tests/suite/test_apikey_auth_policies.py | 50 ------------------------ 1 file changed, 50 deletions(-) diff --git a/tests/suite/test_apikey_auth_policies.py b/tests/suite/test_apikey_auth_policies.py index 9c66006780..cd84e56634 100644 --- a/tests/suite/test_apikey_auth_policies.py +++ b/tests/suite/test_apikey_auth_policies.py @@ -35,7 +35,6 @@ std_vs_src = f"{TEST_DATA}/virtual-server/standard/virtual-server.yaml" -@pytest.mark.jim @pytest.mark.policies @pytest.mark.parametrize( "crd_ingress_controller, virtual_server_setup", @@ -271,55 +270,6 @@ def test_apikey_auth_policy_vs(self, kube_apis, crd_ingress_controller, virtual_ for response in backend2_correct_query_with_correct_password_resps: assert response.status_code == 200 - -@pytest.mark.policies -@pytest.mark.parametrize( - "crd_ingress_controller, virtual_server_setup", - [ - ( - { - "type": "complete", - "extra_args": [ - f"-enable-custom-resources", - f"-enable-leader-election=false", - f"-enable-snippets", - ], - }, - { - "example": "virtual-server", - "app_type": "simple", - }, - ) - ], - indirect=True, -) -class TestAPIKeyAuthPoliciesVSR: - def setup_single_policy( - self, kube_apis, test_namespace: str, secret_src: str, policy_src: str, vs_host: str - ) -> namedtuple: - APIKey_policy_details = namedtuple( - "APIKey_policy_details", ["headers", "queries", "policy_name", "secret_name", "vs_host", "apikeys"] - ) - print(f"Create apikey auth secret") - secret_name = create_secret_from_yaml(kube_apis.v1, test_namespace, secret_src) - apikeys = get_apikey_auth_secrets_from_yaml(secret_src) - details = get_apikey_policy_details_from_yaml(policy_src) - - print(f"Create apikey auth policy") - policy_name = create_policy_from_yaml(kube_apis.custom_objects, policy_src, test_namespace) - wait_before_test() - - headers = details["headers"] - queries = details["queries"] - return APIKey_policy_details( - headers=headers, - queries=queries, - policy_name=policy_name, - secret_name=secret_name, - vs_host=vs_host, - apikeys=apikeys, - ) - def test_apikey_auth_policy_vs_and_vsr( self, kube_apis, crd_ingress_controller, virtual_server_setup, test_namespace ): From fff5d6d079d7c7068c1101f915bb7689f062cce5 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Thu, 13 Jun 2024 14:09:15 +0100 Subject: [PATCH 33/36] add unit tests, clean up code Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Signed-off-by: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> --- .../configs/version2/template_helper_test.go | 38 +++++++++++++++++++ internal/configs/virtualserver_test.go | 2 +- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/internal/configs/version2/template_helper_test.go b/internal/configs/version2/template_helper_test.go index 8b0e9b99d9..ed0650ae06 100644 --- a/internal/configs/version2/template_helper_test.go +++ b/internal/configs/version2/template_helper_test.go @@ -4,6 +4,8 @@ import ( "bytes" "testing" "text/template" + + "github.com/google/go-cmp/cmp" ) func TestContainsSubstring(t *testing.T) { @@ -303,6 +305,42 @@ func TestReplaceAll(t *testing.T) { } } +func TestMakeHeaderQueryValue(t *testing.T) { + t.Parallel() + + testCases := []struct { + apiKey APIKey + expected string + }{ + { + apiKey: APIKey{ + Header: []string{"foo", "bar"}, + }, + expected: `"${http_foo}${http_bar}"`, + }, + { + apiKey: APIKey{ + Header: []string{"foo", "bar"}, + Query: []string{"baz", "qux"}, + }, + expected: `"${http_foo}${http_bar}${arg_baz}${arg_qux}"`, + }, + { + apiKey: APIKey{ + Query: []string{"baz", "qux"}, + }, + expected: `"${arg_baz}${arg_qux}"`, + }, + } + + for _, tc := range testCases { + got := makeHeaderQueryValue(tc.apiKey) + if !cmp.Equal(tc.expected, got) { + t.Error(cmp.Diff(tc.expected, got)) + } + } +} + func newHasPrefixTemplate(t *testing.T) *template.Template { t.Helper() tmpl, err := template.New("testTemplate").Funcs(helperFunctions).Parse(`{{hasPrefix .InputString .Prefix}}`) diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 473d4ceba4..72838407a3 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -8546,7 +8546,7 @@ func TestGeneratePoliciesFails(t *testing.T) { "client2": []byte("password"), }, }, - Error: errors.New("secret is invalid"), // TODO: find correct error + Error: errors.New("secret is invalid"), }, }, }, From b983616b3e0fa81164df6d2062e2d58127a54121 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 14 Jun 2024 11:36:27 +0100 Subject: [PATCH 34/36] remove logs, refactor, add tests 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.go | 22 +++++++--------------- internal/configs/virtualserver_test.go | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+), 15 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index f75b568b7f..08203fc653 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -2,7 +2,6 @@ package configs import ( "crypto/sha256" - "encoding/base64" "encoding/hex" "fmt" "net/url" @@ -1354,11 +1353,8 @@ func (p *policiesCfg) addAPIKeyConfig( } secretKey := fmt.Sprintf("%v/%v", polNamespace, apiKey.ClientSecret) - glog.Infof("secretKey: %v", secretKey) secretRef := secretRefs[secretKey] - glog.Infof("secretRefs: %v", secretRefs) var secretType api_v1.SecretType - glog.Infof("secret: %v", secretRef) if secretRef.Secret != nil { secretType = secretRef.Secret.Type } @@ -1376,12 +1372,9 @@ func (p *policiesCfg) addAPIKeyConfig( mapName := fmt.Sprintf( "apikey_auth_client_name_%s_%s_%s", - // vsNamespace, - // vsName, - strings.Replace(vsNamespace, "-", "_", -1), // TODO:refactor - strings.Replace(vsName, "-", "_", -1), - // strings.Split(polKey, "/")[1], - strings.Split(strings.Replace(polKey, "-", "_", -1), "/")[1], + rfc1123ToSnake(vsNamespace), + rfc1123ToSnake(vsName), + strings.Split(rfc1123ToSnake(polKey), "/")[1], ) p.APIKey = &version2.APIKey{ Header: apiKey.SuppliedIn.Header, @@ -1392,6 +1385,10 @@ func (p *policiesCfg) addAPIKeyConfig( return res } +func rfc1123ToSnake(rfc1123String string) string { + return strings.Replace(rfc1123String, "-", "_", -1) +} + func generateAPIKeyClients(secretData map[string][]byte) []apiKeyClient { var clients []apiKeyClient for clientID, apiKey := range secretData { @@ -1399,11 +1396,6 @@ func generateAPIKeyClients(secretData map[string][]byte) []apiKeyClient { h := sha256.New() h.Write(apiKey) sha256Hash := hex.EncodeToString(h.Sum(nil)) - base64Str := base64.URLEncoding.EncodeToString(h.Sum(nil)) - - glog.Infof("apiKey %s", apiKey) - glog.Infof("sha %s", sha256Hash) - glog.Infof("base64Str %s", base64Str) clients = append(clients, apiKeyClient{ClientID: clientID, HashedKey: sha256Hash}) // } return clients diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go index 72838407a3..f03e313e76 100644 --- a/internal/configs/virtualserver_test.go +++ b/internal/configs/virtualserver_test.go @@ -15540,3 +15540,24 @@ func (*fakeBundleValidator) validate(bundle string) (string, error) { } return bundle, nil } + +func TestRFC1123ToSnake(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "valid", + input: "api-policy-1", + expected: "api_policy_1", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if !cmp.Equal(rfc1123ToSnake(tt.input), tt.expected) { + t.Errorf(cmp.Diff(rfc1123ToSnake(tt.input), tt.expected)) + } + }) + } +} From ec753cecf33f921aeb71dc0ba7212e7c899f3ac4 Mon Sep 17 00:00:00 2001 From: Haywood Shannon <5781935+haywoodsh@users.noreply.github.com> Date: Fri, 14 Jun 2024 11:41:49 +0100 Subject: [PATCH 35/36] remove logs 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.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go index 08203fc653..de5fba197f 100644 --- a/internal/configs/virtualserver.go +++ b/internal/configs/virtualserver.go @@ -1402,8 +1402,6 @@ func generateAPIKeyClients(secretData map[string][]byte) []apiKeyClient { } func generateAPIKeyClientMap(mapName string, apiKeyClients []apiKeyClient) *version2.Map { - glog.Infof("mapName: %v, apiKeyClients: %v", mapName, apiKeyClients) - defaultParam := version2.Parameter{ Value: "default", Result: "\"\"", From c6bd588050cfec0469dafd580af9cde4c80eccf9 Mon Sep 17 00:00:00 2001 From: Jim Ryan Date: Fri, 14 Jun 2024 14:38:08 +0100 Subject: [PATCH 36/36] add api key auth to telemetry --- docs/content/overview/product-telemetry.md | 1 + internal/telemetry/cluster.go | 2 ++ internal/telemetry/collector.go | 5 +++++ internal/telemetry/data.avdl | 3 +++ internal/telemetry/exporter.go | 2 ++ internal/telemetry/nicresourcecounts_attributes_generated.go | 1 + 6 files changed, 14 insertions(+) diff --git a/docs/content/overview/product-telemetry.md b/docs/content/overview/product-telemetry.md index d6ed533cd1..d251397707 100644 --- a/docs/content/overview/product-telemetry.md +++ b/docs/content/overview/product-telemetry.md @@ -45,6 +45,7 @@ These are the data points collected and reported by NGINX Ingress Controller: - **IngressAnnotations** List of Ingress annotations managed by NGINX Ingress Controller - **AccessControlPolicies** Number of AccessControl policies. - **RateLimitPolicies** Number of RateLimit policies. +- **APIKeyPolicies** Number of API Key Auth policies. - **JWTAuthPolicies** Number of JWTAuth policies. - **BasicAuthPolicies** Number of BasicAuth policies. - **IngressMTLSPolicies** Number of IngressMTLS policies. diff --git a/internal/telemetry/cluster.go b/internal/telemetry/cluster.go index 10ff570cd3..f44bb45375 100644 --- a/internal/telemetry/cluster.go +++ b/internal/telemetry/cluster.go @@ -193,6 +193,8 @@ func (c *Collector) PolicyCount() map[string]int { policyCounters["OIDC"]++ case spec.WAF != nil: policyCounters["WAF"]++ + case spec.APIKey != nil: + policyCounters["APIKey"]++ } } return policyCounters diff --git a/internal/telemetry/collector.go b/internal/telemetry/collector.go index e2507b69ac..4c480670bf 100644 --- a/internal/telemetry/collector.go +++ b/internal/telemetry/collector.go @@ -142,6 +142,7 @@ func (c *Collector) Collect(ctx context.Context) { IngressClasses: int64(report.IngressClassCount), AccessControlPolicies: int64(report.AccessControlCount), RateLimitPolicies: int64(report.RateLimitCount), + APIKeyPolicies: int64(report.APIKeyAuthCount), JWTAuthPolicies: int64(report.JWTAuthCount), BasicAuthPolicies: int64(report.BasicAuthCount), IngressMTLSPolicies: int64(report.IngressMTLSCount), @@ -191,6 +192,7 @@ type Report struct { AccessControlCount int RateLimitCount int JWTAuthCount int + APIKeyAuthCount int BasicAuthCount int IngressMTLSCount int EgressMTLSCount int @@ -261,6 +263,7 @@ func (c *Collector) BuildReport(ctx context.Context) (Report, error) { var ( accessControlCount int rateLimitCount int + apiKeyCount int jwtAuthCount int basicAuthCount int ingressMTLSCount int @@ -273,6 +276,7 @@ func (c *Collector) BuildReport(ctx context.Context) (Report, error) { policies := c.PolicyCount() accessControlCount = policies["AccessControl"] rateLimitCount = policies["RateLimit"] + apiKeyCount = policies["APIKey"] jwtAuthCount = policies["JWTAuth"] basicAuthCount = policies["BasicAuth"] ingressMTLSCount = policies["IngressMTLS"] @@ -318,6 +322,7 @@ func (c *Collector) BuildReport(ctx context.Context) (Report, error) { IngressClassCount: ingressClassCount, AccessControlCount: accessControlCount, RateLimitCount: rateLimitCount, + APIKeyAuthCount: apiKeyCount, JWTAuthCount: jwtAuthCount, BasicAuthCount: basicAuthCount, IngressMTLSCount: ingressMTLSCount, diff --git a/internal/telemetry/data.avdl b/internal/telemetry/data.avdl index c51ba6e2e2..c374841ead 100644 --- a/internal/telemetry/data.avdl +++ b/internal/telemetry/data.avdl @@ -78,6 +78,9 @@ It is the UID of the `kube-system` Namespace. */ /** RateLimitPolicies is the number of RateLimit policies managed by NGINX Ingress Controller */ long? RateLimitPolicies = null; + /** APIKeyPolicies is the number of APIKey policies managed by NGINX Ingress Controller */ + long? APIKeyPolicies = null; + /** JWTAuthPolicies is the number of JWTAuth policies managed by NGINX Ingress Controller */ long? JWTAuthPolicies = null; diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 942940fdb8..aac38756c5 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -95,6 +95,8 @@ type NICResourceCounts struct { AccessControlPolicies int64 // RateLimitPolicies is the number of RateLimit policies managed by NGINX Ingress Controller RateLimitPolicies int64 + // APIKeyPolicies is the number of APIKey policies managed by NGINX Ingress Controller + APIKeyPolicies int64 // JWTAuthPolicies is the number of JWTAuth policies managed by NGINX Ingress Controller JWTAuthPolicies int64 // BasicAuthPolicies is the number of BasicAuth policies managed by NGINX Ingress Controller diff --git a/internal/telemetry/nicresourcecounts_attributes_generated.go b/internal/telemetry/nicresourcecounts_attributes_generated.go index 21be8dcc3f..f68e171f7c 100644 --- a/internal/telemetry/nicresourcecounts_attributes_generated.go +++ b/internal/telemetry/nicresourcecounts_attributes_generated.go @@ -28,6 +28,7 @@ func (d *NICResourceCounts) Attributes() []attribute.KeyValue { attrs = append(attrs, attribute.Int64("IngressClasses", d.IngressClasses)) attrs = append(attrs, attribute.Int64("AccessControlPolicies", d.AccessControlPolicies)) attrs = append(attrs, attribute.Int64("RateLimitPolicies", d.RateLimitPolicies)) + attrs = append(attrs, attribute.Int64("APIKeyPolicies", d.APIKeyPolicies)) attrs = append(attrs, attribute.Int64("JWTAuthPolicies", d.JWTAuthPolicies)) attrs = append(attrs, attribute.Int64("BasicAuthPolicies", d.BasicAuthPolicies)) attrs = append(attrs, attribute.Int64("IngressMTLSPolicies", d.IngressMTLSPolicies))