enabling annotation validation prevents deployment of previously allowed permanent redirect annotations

[joshsleeper]

What happened:

after adding the --enable-annotation-validation option to our ingress-nginx controller deployment to mitigate recent CVEs including kubernetes/kubernetes#126817 we encountered some deploy-time issues with historically fine deployments.

specifically, helm deployments containing an Ingress resource with the nginx.ingress.kubernetes.io/permanent-redirect annotation containing a capture group failed to deploy with the following (sanitized) error:

Upgrade "SNIPPED" failed: cannot patch "SNIPPED" with kind Ingress: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation nginx.ingress.kubernetes.io/permanent-redirect contains invalid value

What you expected to happen:

I expected the helm deployments to succeed as they historically had prior to setting the --enable-annotation-validation option

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

nginx-ingress-controller-7fc847c578-6jkbj:/etc/nginx$ /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.9.4
  Build:         846d251814a09d8a5d8d28e2e604bfc7749bcb49
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.21.6

-------------------------------------------------------------------------------

Kubernetes version (use kubectl version):

$ kubectl version
Client Version: v1.28.3
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.25.10-gke.2700
WARNING: version difference between client (1.28) and server (1.25) exceeds the supported minor version skew of +/-1

Environment:

• Cloud provider or hardware configuration:

  GKE on Google Cloud, but that shouldn't be relevant to the issue

• OS (e.g. from /etc/os-release):

  N/A

• Kernel (e.g. uname -a):

  N/A

• Install tools:

  • Please mention how/where was the cluster created like kubeadm/kops/minikube/kind etc.

  N/A

• Basic cluster related info:

  • kubectl version
  • kubectl get nodes -o wide

  N/A

• How was the ingress-nginx-controller installed:

  • If helm was used then please show output of helm ls -A | grep -i ingress
  • If helm was used then please show output of helm -n <ingresscontrollernamepspace> get values <helmreleasename>
  • If helm was not used, then copy/paste the complete precise command used to install the controller, along with the flags and options used
  • if you have more than one instance of the ingress-nginx-controller installed in the same cluster, please provide details for all the instances

  helm 3.12+, but pretty much positive this isn't an issue with how we installed/configured anything.

• Current State of the controller:

  • kubectl describe ingressclasses
  • kubectl -n <ingresscontrollernamespace> get all -A -o wide
  • kubectl -n <ingresscontrollernamespace> describe po <ingresscontrollerpodname>
  • kubectl -n <ingresscontrollernamespace> describe svc <ingresscontrollerservicename>

  N/A

• Current state of ingress object, if applicable:

  • kubectl -n <appnnamespace> get all,ing -o wide
  • kubectl -n <appnamespace> describe ing <ingressname>
  • If applicable, then, your complete and exact curl/grpcurl command (redacted if required) and the reponse to the curl/grpcurl command with the -v flag

  N/A

• Others:

  • Any other related information like ;
    • copy/paste of the snippet (if applicable)
    • kubectl describe ... of any custom configmap(s) created and in use
    • Any other related information that may help

  N/A

How to reproduce this issue:

given any ingress-nginx deployment with the --enable-annotation-validation option provided to the controller container.

given the official ingress-nginx helm chart this can be done by setting controller.enableAnnotationValidations: true in the values.

pared-down example of an affected Ingress resource:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/permanent-redirect: https://new-example.com/$2
    nginx.ingress.kubernetes.io/use-regex: "true"
  name: SNIPPED
  namespace: SNIPPED
spec:
  ingressClassName: nginx
  rules:
  - host: old-example.com
    http:
      paths:
      - backend:
          service:
            name: old-example
            port:
              number: 8080
        path: /demo($|/)(.*)
        pathType: ImplementationSpecific

attempting to kubectl apply a resource like above should trigger a validation error and reject the resource.

Anything else we need to know:

• issue that motivated the change: CVE-2023-5044: Code injection via nginx.ingress.kubernetes.io/permanent-redirect annotation kubernetes#126817
• MR that implemented the affected feature: Implement annotation validation #9673
• relevant sections of code to the issue at hand
  • where the validation is happening:

    ingress-nginx/internal/ingress/annotations/redirect/redirect.go

    Line 64 in da51393

    Validator: parser.ValidateRegex(parser.URLIsValidRegex, false),

  • the validation pattern currently used:

    ingress-nginx/internal/ingress/annotations/parser/validators.go

    Line 62 in da51393

    URLIsValidRegex = regexp.MustCompile("^[" + alphaNumericChars + urlEnabledChars + "]*$")

this could just be a documentation issue if the maintainer's opinion is that supporting capture groups in the redirect annotation isn't something they wish to do, in which case just documenting that is probably the desired outcome.

if that is supposed to be supported, then I believe the desired outcome is to correct the validation pattern used when the annotation validation is enabled.

[k8s-ci-robot]

This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[channaneq]

I am currently running into a similar issue with the enable-annotation-validation flag just not with the same annotation.

I am trying to get around this issue by running nginx in chroot mode, meaning you don't have to use the enable-annotation-validation flag, assuming that you are making use of this flag in response to the recent nginx vulnerabilities CVE-2023-5043 and CVE-2023-5044. This explains that chroot mode mitigates the vulnerability from a secrets exploitation perspective kubernetes/kubernetes#126816

I am testing this out at the moment myself, but if you don't want to bother with it yourself I think a redesign of how that annotation is setup (ie the dollar sign being present) should get around your issue

[bmv126]

@channaneq Can you share details on which annotations caused issue for you

[channaneq]

nginx.ingress.kubernetes.io/auth-signin

annotation auth-signin contains invalid value

[eaterm]

We are also running into the same issue with annotation:nginx.ingress.kubernetes.io/session-cookie-samesite: None

We always see the error: validators.go:221] validation error on ingress <namespace/<ingress>: annotation session-cookie-samesite contains invalid value None

Tested this with all 3 values and all gave the same result.

[siegenthalerroger]

Concerning the permanent-redirect I think I've narrowed down the regression in behaviour down to an issue with the Validator not being given capture groups (unlike the rewrite-target annotation which does get these, hence why it works). I've opened another issue for this here: #10734

Hopefully we hear back

[sdickhoven]

@channaneq wrote:

nginx.ingress.kubernetes.io/auth-signin

annotation auth-signin contains invalid value

...specifically:

nginx.ingress.kubernetes.io/auth-signin: "https://$host/oauth2/start?rd=$escaped_request_uri"

which is what is used in the example here:

https://kubernetes.github.io/ingress-nginx/examples/auth/oauth-external-auth/

i think the current incarnation of annotation validation simply rejects anything with a $ in it. that's not a very useful validation imo.

there are variables that should absolutely be considered "safe" by this validation. e.g.

• $host
• $request_uri
• $service_name
• $service_port
• $namespace
• and many more

[longwuyuan]

Some changes were made and also cherry picked into the v1.10.x and v1.11.x releases. It could be allowing the dollar/$ sign now on some annotations.

But that has to be tested on the recent release of the controller and the detailed data of the test posted here so that readers do not need to do the test themselves just for triaging the issue.

The background to hardeing annotations is the CVEs announced and overall security concerns because crafty strings in annotations are a threat that need to be addressed without any delay asap.

On a long shot someone can also claim that the validation code itself can get comprehensive enough to check some valid use-cases of characters. But that works requires resources that are not available now. And top that with the absolute need to ship a controller that is secure by default. We have actually deprecated features because there are no resources available to support/maintain the entire range of possible use-cases. The focus is on ensuring that the Ingress-API implied behavior is all working as specified in KEP and all other features are subject to availability of resources to maintain & support.

So its better you engage in the Kubernetes slack about this as there are more engineers & maintainers there and there is hardly any resources here. Once the slack discussion comes to a state of deciding on the dollar/$ character in whichever annotation , then this issue can be updated with those discussions and decisions (related data).

Until then I will close the issue as it is adding to the tally of open issues without any action item on any party. After data is posted here to track a action item, the issue can be reopened.

/close

[k8s-ci-robot]

@longwuyuan: Closing this issue.

In response to this:

Some changes were made and also cherry picked into the v1.10.x and v1.11.x releases. It could be allowing the dollar/$ sign now on some annotations.

But that has to be tested on the recent release of the controller and the detailed data of the test posted here so that readers do not need to do the test themselves just for triaging the issue.

The background to hardeing annotations is the CVEs announced and overall security concerns because crafty strings in annotations are a threat that need to be addressed without any delay asap.

On a long shot someone can also claim that the validation code itself can get comprehensive enough to check some valid use-cases of characters. But that works requires resources that are not available now. And top that with the absolute need to ship a controller that is secure by default. We have actually deprecated features because there are no resources available to support/maintain the entire range of possible use-cases. The focus is on ensuring that the Ingress-API implied behavior is all working as specified in KEP and all other features are subject to availability of resources to maintain & support.

So its better you engage in the Kubernetes slack about this as there are more engineers & maintainers there and there is hardly any resources here. Once the slack discussion comes to a state of deciding on the dollar/$ character in whichever annotation , then this issue can be updated with those discussions and decisions (related data).

Until then I will close the issue as it is adding to the tally of open issues without any action item on any party. After data is posted here to track a action item, the issue can be reopened.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.