Implement annotation validation (#9673)

* Add validation to all annotations

* Add annotation validation for fcgi

* Fix reviews and fcgi e2e

* Add flag to disable cross namespace validation

* Add risk, flag for validation, tests

* Add missing formating

* Enable validation by default on tests

* Test validation flag

* remove ajp from list

* Finalize validation changes

* Add validations to CI

* Update helm docs

* Fix code review

* Use a better name for annotation risk

.github/workflows/ci.yaml

@@ -319,6 +319,55 @@ jobs:
  name: e2e-test-reports-${{ matrix.k8s }}
  path: 'test/junitreports/report*.xml'
 
+ kubernetes-validations:
+ name: Kubernetes with Validations
+ runs-on: ubuntu-latest
+ needs:
+ - changes
+ - build
+ if: |
+ (needs.changes.outputs.go == 'true') || ${{ inputs.run_e2e }}
+
+ strategy:
+ matrix:
+ k8s: [v1.27.1]
+
+ steps:
+ - name: Checkout
+ uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+
+ - name: cache
+ uses: actions/download-artifact@9bc31d5ccc31df68ecc42ccf4149144866c47d8a # v3.0.2
+ with:
+ name: docker.tar.gz
+
+ - name: Create Kubernetes ${{ matrix.k8s }} cluster
+ id: kind
+ run: |
+ kind create cluster --image=kindest/node:${{ matrix.k8s }} --config test/e2e/kind.yaml
+
+ - name: Load images from cache
+ run: |
+ echo "loading docker images..."
+ pigz -dc docker.tar.gz | docker load
+
+ - name: Run e2e tests
+ env:
+ KIND_CLUSTER_NAME: kind
+ SKIP_CLUSTER_CREATION: true
+ SKIP_IMAGE_CREATION: true
+ ENABLE_VALIDATIONS: true
+ run: |
+ kind get kubeconfig > $HOME/.kube/kind-config-kind
+ make kind-e2e-test
+
+ - name: Upload e2e junit-reports
+ uses: actions/upload-artifact@0b7f8abb1508181956e8e162db84b466c27e18ce # v3.1.2
+ if: success() || failure()
+ with:
+ name: e2e-test-reports-${{ matrix.k8s }}
+ path: 'test/junitreports/report*.xml'
+
 
  kubernetes-chroot:
  name: Kubernetes chroot

charts/ingress-nginx/README.md

@@ -294,6 +294,7 @@ As of version `1.26.0` of this chart, by simply not providing any clusterIP valu
 | controller.dnsConfig | object | `{}` | Optionally customize the pod dnsConfig. |
 | controller.dnsPolicy | string | `"ClusterFirst"` | Optionally change this to ClusterFirstWithHostNet in case you have 'hostNetwork: true'. By default, while using host network, name resolution uses the host's DNS. If you wish nginx-controller to keep resolving names inside the k8s network, use ClusterFirstWithHostNet. |
 | controller.electionID | string | `""` | Election ID to use for status update, by default it uses the controller name combined with a suffix of 'leader' |
+| controller.enableAnnotationValidations | bool | `false` | |
 | controller.enableMimalloc | bool | `true` | Enable mimalloc as a drop-in replacement for malloc. # ref: https://github.com/microsoft/mimalloc # |
 | controller.enableTopologyAwareRouting | bool | `false` | This configuration enables Topology Aware Routing feature, used together with service annotation service.kubernetes.io/topology-aware-hints="auto" Defaults to false |
 | controller.existingPsp | string | `""` | Use an existing PSP instead of creating one |

charts/ingress-nginx/templates/_params.tpl

@@ -1,5 +1,8 @@
 {{- define "ingress-nginx.params" -}}
 - /nginx-ingress-controller
+{{- if .Values.controller.enableAnnotationValidations }}
+- --enable-annotation-validation=true
+{{- end }}
 {{- if .Values.defaultBackend.enabled }}
 - --default-backend-service=$(POD_NAMESPACE)/{{ include "ingress-nginx.defaultBackend.fullname" . }}
 {{- end }}

charts/ingress-nginx/values.yaml

@@ -15,6 +15,7 @@ commonLabels: {}
 
 controller:
  name: controller
+ enableAnnotationValidations: false
  image:
  ## Keep false as default for now!
  chroot: false

docs/user-guide/cli-arguments.md

@@ -15,6 +15,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment
 | `--default-backend-service` | Service used to serve HTTP requests not matching any known server name (catch-all). Takes the form "namespace/name". The controller configures NGINX to forward requests to the first port of this Service. |
 | `--default-server-port` | Port to use for exposing the default server (catch-all). (default 8181) |
 | `--default-ssl-certificate` | Secret containing a SSL certificate to be used by the default HTTPS server (catch-all). Takes the form "namespace/name". |
+| `--enable-annotation-validation` | If true, will enable the annotation validation feature. This value will be defaulted to true on a future release. |
 | `--disable-catch-all` | Disable support for catch-all Ingresses. (default false) |
 | `--disable-full-test` | Disable full test of all merged ingresses at the admission stage and tests the template of the ingress being created or updated (full test of all ingresses is enabled by default). |
 | `--disable-svc-external-name` | Disable support for Services of type ExternalName. (default false) |

docs/user-guide/nginx-configuration/configmap.md

@@ -29,7 +29,9 @@ The following table shows a configuration option's name, type, and the default v
 |:---|:---|:------|:----|
 |[add-headers](#add-headers)|string|""||
 |[allow-backend-server-header](#allow-backend-server-header)|bool|"false"||
+|[allow-cross-namespace-resources](#allow-cross-namespace-resources)|bool|"true"||
 |[allow-snippet-annotations](#allow-snippet-annotations)|bool|true||
+|[annotations-risk-level](#annotations-risk-level)|string|Critical||
 |[annotation-value-word-blocklist](#annotation-value-word-blocklist)|string array|""||
 |[hide-headers](#hide-headers)|string array|empty||
 |[access-log-params](#access-log-params)|string|""||
@@ -239,13 +241,37 @@ Sets custom headers from named configmap before sending traffic to the client. S
 
 Enables the return of the header Server from the backend instead of the generic nginx string. _**default:**_ is disabled
 
+## allow-cross-namespace-resources
+
+Enables users to consume cross namespace resource on annotations, when was previously enabled . _**default:**_ true
+
+**Annotations that may be impacted with this change**:
+* `auth-secret`
+* `auth-proxy-set-header`
+* `auth-tls-secret`
+* `fastcgi-params-configmap`
+* `proxy-ssl-secret`
+
+
+**This option will be defaulted to false in the next major release**
+
 ## allow-snippet-annotations
 
 Enables Ingress to parse and add *-snippet annotations/directives created by the user. _**default:**_ `true`
 
 Warning: We recommend enabling this option only if you TRUST users with permission to create Ingress objects, as this
 may allow a user to add restricted configurations to the final nginx.conf file
 
+**This option will be defaulted to false in the next major release**
+
+## annotations-risk-level
+
+Represents the risk accepted on an annotation. If the risk is, for instance `Medium`, annotations with risk High and Critical will not be accepted.
+
+Accepted values are `Critical`, `High`, `Medium` and `Low`.
+
+Defaults to `Critical` but will be changed to `High` on the next minor release
+
 ## annotation-value-word-blocklist
 
 Contains a comma-separated value of chars/words that are well known of being used to abuse Ingress configuration

internal/ingress/annotations/alias/main.go

@@ -27,19 +27,44 @@ import (
  "k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
+const (
+ serverAliasAnnotation = "server-alias"
+)
+
+var aliasAnnotation = parser.Annotation{
+ Group: "alias",
+ Annotations: parser.AnnotationFields{
+ serverAliasAnnotation: {
+ Validator: parser.ValidateArrayOfServerName,
+ Scope: parser.AnnotationScopeIngress,
+ Risk: parser.AnnotationRiskHigh, // High as this allows regex chars
+ Documentation: `this annotation can be used to define additional server 
+ aliases for this Ingress`,
+ },
+ },
+}
+
 type alias struct {
- r resolver.Resolver
+ r resolver.Resolver
+ annotationConfig parser.Annotation
 }
 
 // NewParser creates a new Alias annotation parser
 func NewParser(r resolver.Resolver) parser.IngressAnnotation {
- return alias{r}
+ return alias{
+ r: r,
+ annotationConfig: aliasAnnotation,
+ }
+}
+
+func (a alias) GetDocumentation() parser.AnnotationFields {
+ return a.annotationConfig.Annotations
 }
 
 // Parse parses the annotations contained in the ingress rule
 // used to add an alias to the provided hosts
 func (a alias) Parse(ing *networking.Ingress) (interface{}, error) {
- val, err := parser.GetStringAnnotation("server-alias", ing)
+ val, err := parser.GetStringAnnotation(serverAliasAnnotation, ing, a.annotationConfig.Annotations)
  if err != nil {
  return []string{}, err
  }
@@ -61,3 +86,8 @@ func (a alias) Parse(ing *networking.Ingress) (interface{}, error) {
 
  return l, nil
 }
+
+func (a alias) Validate(anns map[string]string) error {
+ maxrisk := parser.StringRiskToRisk(a.r.GetSecurityConfiguration().AnnotationsRiskLevel)
+ return parser.CheckAnnotationRisk(anns, maxrisk, aliasAnnotation.Annotations)
+}

internal/ingress/annotations/alias/main_test.go

@@ -27,7 +27,7 @@ import (
  "k8s.io/ingress-nginx/internal/ingress/resolver"
 )
 
-var annotation = parser.GetAnnotationWithPrefix("server-alias")
+var annotation = parser.GetAnnotationWithPrefix(serverAliasAnnotation)
 
 func TestParse(t *testing.T) {
  ap := NewParser(&resolver.Mock{})
@@ -36,16 +36,20 @@ func TestParse(t *testing.T) {
  }
 
  testCases := []struct {
- annotations map[string]string
- expected []string
+ annotations map[string]string
+ expected []string
+ skipValidation bool
+ wantErr bool
  }{
- {map[string]string{annotation: "a.com, b.com, , c.com"}, []string{"a.com", "b.com", "c.com"}},
- {map[string]string{annotation: "www.example.com"}, []string{"www.example.com"}},
- {map[string]string{annotation: "*.example.com,www.example.*"}, []string{"*.example.com", "www.example.*"}},
- {map[string]string{annotation: `~^www\d+\.example\.com$`}, []string{`~^www\d+\.example\.com$`}},
- {map[string]string{annotation: ""}, []string{}},
- {map[string]string{}, []string{}},
- {nil, []string{}},
+ {map[string]string{annotation: "a.com, b.com, , c.com"}, []string{"a.com", "b.com", "c.com"}, false, false},
+ {map[string]string{annotation: "www.example.com"}, []string{"www.example.com"}, false, false},
+ {map[string]string{annotation: "*.example.com,www.example.*"}, []string{"*.example.com", "www.example.*"}, false, false},
+ {map[string]string{annotation: `~^www\d+\.example\.com$`}, []string{`~^www\d+\.example\.com$`}, false, false},
+ {map[string]string{annotation: `www.xpto;lala`}, []string{}, false, true},
+ {map[string]string{annotation: `www.xpto;lala`}, []string{"www.xpto;lala"}, true, false}, // When we skip validation no error should happen
+ {map[string]string{annotation: ""}, []string{}, false, true},
+ {map[string]string{}, []string{}, false, true},
+ {nil, []string{}, false, true},
  }
 
  ing := &networking.Ingress{
@@ -58,7 +62,16 @@ func TestParse(t *testing.T) {
 
  for _, testCase := range testCases {
  ing.SetAnnotations(testCase.annotations)
- result, _ := ap.Parse(ing)
+ if testCase.skipValidation {
+ parser.EnableAnnotationValidation = false
+ }
+ defer func() {
+ parser.EnableAnnotationValidation = true
+ }()
+ result, err := ap.Parse(ing)
+ if (err != nil) != testCase.wantErr {
+ t.Errorf("ParseAliasAnnotation() annotation: %s, error = %v, wantErr %v", testCase.annotations, err, testCase.wantErr)
+ }
  if !reflect.DeepEqual(result, testCase.expected) {
  t.Errorf("expected %v but returned %v, annotations: %s", testCase.expected, result, testCase.annotations)
  }

internal/ingress/annotations/annotations.go

@@ -44,8 +44,8 @@ import (
  "k8s.io/ingress-nginx/internal/ingress/annotations/fastcgi"
  "k8s.io/ingress-nginx/internal/ingress/annotations/globalratelimit"
  "k8s.io/ingress-nginx/internal/ingress/annotations/http2pushpreload"
+ "k8s.io/ingress-nginx/internal/ingress/annotations/ipallowlist"
  "k8s.io/ingress-nginx/internal/ingress/annotations/ipdenylist"
- "k8s.io/ingress-nginx/internal/ingress/annotations/ipwhitelist"
  "k8s.io/ingress-nginx/internal/ingress/annotations/loadbalancing"
  "k8s.io/ingress-nginx/internal/ingress/annotations/log"
  "k8s.io/ingress-nginx/internal/ingress/annotations/mirror"
@@ -109,14 +109,14 @@ type Ingress struct {
  UpstreamHashBy upstreamhashby.Config
  LoadBalancing string
  UpstreamVhost string
- Whitelist ipwhitelist.SourceRange
  Denylist ipdenylist.SourceRange
  XForwardedPrefix string
  SSLCipher sslcipher.Config
  Logs log.Config
  ModSecurity modsecurity.Config
  Mirror mirror.Config
  StreamSnippet string
+ Allowlist ipallowlist.SourceRange
 }
 
 // Extractor defines the annotation parsers to be used in the extraction of annotations
@@ -159,7 +159,7 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor {
  "UpstreamHashBy": upstreamhashby.NewParser(cfg),
  "LoadBalancing": loadbalancing.NewParser(cfg),
  "UpstreamVhost": upstreamvhost.NewParser(cfg),
- "Whitelist": ipwhitelist.NewParser(cfg),
+ "Allowlist": ipallowlist.NewParser(cfg),
  "Denylist": ipdenylist.NewParser(cfg),
  "XForwardedPrefix": xforwardedprefix.NewParser(cfg),
  "SSLCipher": sslcipher.NewParser(cfg),
@@ -173,16 +173,23 @@ func NewAnnotationExtractor(cfg resolver.Resolver) Extractor {
 }
 
 // Extract extracts the annotations from an Ingress
-func (e Extractor) Extract(ing *networking.Ingress) *Ingress {
+func (e Extractor) Extract(ing *networking.Ingress) (*Ingress, error) {
  pia := &Ingress{
  ObjectMeta: ing.ObjectMeta,
  }
 
  data := make(map[string]interface{})
  for name, annotationParser := range e.annotations {
+ if err := annotationParser.Validate(ing.GetAnnotations()); err != nil {
+ return nil, errors.NewRiskyAnnotations(name)
+ }
  val, err := annotationParser.Parse(ing)
  klog.V(5).InfoS("Parsing Ingress annotation", "name", name, "ingress", klog.KObj(ing), "value", val)
  if err != nil {
+ if errors.IsValidationError(err) {
+ klog.ErrorS(err, "ingress contains invalid annotation value")
+ return nil, err
+ }
  if errors.IsMissingAnnotations(err) {
  continue
  }
@@ -220,5 +227,5 @@ func (e Extractor) Extract(ing *networking.Ingress) *Ingress {
  klog.ErrorS(err, "unexpected error merging extracted annotations")
  }
 
- return pia
+ return pia, nil
 }

internal/ingress/annotations/annotations_test.go

@@ -134,8 +134,11 @@ func TestSSLPassthrough(t *testing.T) {
 
  for _, foo := range fooAnns {
  ing.SetAnnotations(foo.annotations)
- r := ec.Extract(ing).SSLPassthrough
- if r != foo.er {
+ r, err := ec.Extract(ing)
+ if err != nil {
+ t.Errorf("Errors should be null: %v", err)
+ }
+ if r.SSLPassthrough != foo.er {
  t.Errorf("Returned %v but expected %v", r, foo.er)
  }
  }
@@ -158,8 +161,11 @@ func TestUpstreamHashBy(t *testing.T) {
 
  for _, foo := range fooAnns {
  ing.SetAnnotations(foo.annotations)
- r := ec.Extract(ing).UpstreamHashBy.UpstreamHashBy
- if r != foo.er {
+ r, err := ec.Extract(ing)
+ if err != nil {
+ t.Errorf("error should be null: %v", err)
+ }
+ if r.UpstreamHashBy.UpstreamHashBy != foo.er {
  t.Errorf("Returned %v but expected %v", r, foo.er)
  }
  }
@@ -185,7 +191,11 @@ func TestAffinitySession(t *testing.T) {
 
  for _, foo := range fooAnns {
  ing.SetAnnotations(foo.annotations)
- r := ec.Extract(ing).SessionAffinity
+ rann, err := ec.Extract(ing)
+ if err != nil {
+ t.Errorf("error should be null: %v", err)
+ }
+ r := rann.SessionAffinity
  t.Logf("Testing pass %v %v", foo.affinitytype, foo.cookiename)
 
  if r.Type != foo.affinitytype {
@@ -228,7 +238,11 @@ func TestCors(t *testing.T) {
 
  for _, foo := range fooAnns {
  ing.SetAnnotations(foo.annotations)
- r := ec.Extract(ing).CorsConfig
+ rann, err := ec.Extract(ing)
+ if err != nil {
+ t.Errorf("error should be null: %v", err)
+ }
+ r := rann.CorsConfig
  t.Logf("Testing pass %v %v %v %v %v", foo.corsenabled, foo.methods, foo.headers, foo.origin, foo.credentials)
 
  if r.CorsEnabled != foo.corsenabled {
@@ -277,7 +291,11 @@ func TestCustomHTTPErrors(t *testing.T) {
 
  for _, foo := range fooAnns {
  ing.SetAnnotations(foo.annotations)
- r := ec.Extract(ing).CustomHTTPErrors
+ rann, err := ec.Extract(ing)
+ if err != nil {
+ t.Errorf("error should be null: %v", err)
+ }
+ r := rann.CustomHTTPErrors
 
  // Check that expected codes were created
  for i := range foo.er {