auth-signin annotation fails validation with encoded url characters

[jasday]

What happened:

auth-signin annotation validation does not allow url-encoded values. This causes an error when the annotation is being loaded:

W0128 14:50:28.714993       7 validators.go:237] validation error on ingress foo/bar: annotation auth-signin contains invalid value https://example.com/oauth2/start?rd=https%3A%2F%2F$host$request_uri
W0128 14:50:28.715105       7 main.go:332] auth-signin value is invalid: annotation nginx.ingress.kubernetes.io/auth-signin contains invalid value

What you expected to happen:

This should pass validation and be accepted.

NGINX Ingress controller version:

-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.12.0
  Build:         ba73b2c24d355f1cdcf4b31ef7c5574059f12118
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.25.5

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

Kubernetes version (use kubectl version):

Environment:

• Cloud provider or hardware configuration: AWS EKS Auto Mode

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

• Install tools:

  • Created with Terraform, using the helm chart

• Basic cluster related info:

  • Server Version: v1.30.8-eks-2d5f260

• Others:
  I believe https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L47 should be changed to:
  urlEnabledChars = regexp.QuoteMeta(',:?&=%')

How to reproduce this issue:

Create an ingress with the nginx.ingress.kubernetes.io/auth-signin: annotation, add an encoded character within the value (e.g https:// -> https%3A%2F%2F).

Anything else we need to know:
This was working in v1.11.3 and stopped working when upgrading to v1.12.0

Happy to make a pull request with the changes required, but wanted this to go up first in case there was a specific reason for disallowing the character.

[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-sigs/prow repository.

[longwuyuan]

Is it safe to allow escape characters ?

[jasday]

Assuming you mean url-encoded characters, I'd say it safe, yes, in fact almost required.

In the instance above, I'm passing a redirect through to an oauth2-proxy, and this redirect will need to be escaped so it's not evaluated as part of the auth-signin value.

If adding a % to the validation is allowing too much, we could adjust the validator for this annotation to decode the url encoded values before validating, but then pass the encoded values to the actual annotation? This could ensure the validator is strict on allowed characters whilst also allowing for these redirects?

[longwuyuan]

There is a history of CVEs related that is behind limiting characters. I understand it would help a lot but having to choose between a feature and security comes into picture here.

[Gacko]

It starts failing because we enabled annotation validation by default in v1.12.0. Apart from that I currently cannot help a lot nor take a deeper look into the issue.

[longwuyuan]

@Gacko , cause is the use of "%something" (encoded) chars which the code link pointed earlier shows as not allowed. I think that allowing that would cause a CVE potentially, on the lines that allowing those chars can potentially drop to shell or malware execution.

[jasday]

I understand where you're coming from - would the decode-then-validate approach I mentioned earlier not reduce some of this risk? As it'll be checking the decoded value to inhibit malicious characters?

Another way I'm looking at it is as url-encoded characters may be needed for some projects to redirect correctly, and as these are now going to stop working, the easiest solution for users here is just to manually re-disable annotation validation - which is going to make all these projects inherently less secure?

This is, admittedly, then a risk the user is accepting by doing this themselves, and obviously security by default should be the way to go for the project - but I can see it forcing a subset of users to either having to lower their protection or having to spend time altering their projects to account for the inability to do url encoded characters.

Maybe worth a thought at some extended URL annotation validation as other annotations could be affected.

[longwuyuan]

@jasday 100% agree with your comments. You are absolutely right on all 3 counts.

So beyond this is the project's capability to allocate developer to it and unfortunately we don't have that option. All resources are dedicated to the making the project secure out of the box and acceptably stable, give tne circumstances. That is besides working on the Gatway-API implementation.

If a PR comes in with the appropriate info in description and is admittedly bundled with tests that are a improvement to the e2e-tests, for the auth feature, at least a review would in the pipeline. My worry is the depth needed to allow characters and encoded URLs without re-introducing the CVE is talked about. No idea how this will be possible.

[jasday]

That's understandable - I'll see if i can find some time to do a deeper investigation and get a PR up if it's feasible.

For now, (for anyone else running into the same issue) I'll be disabling annotation validation.

Happy to change this issue from bug to a feature request for updated URL validation