bug: admission webhook denies auth-url with comma

[mikkel3000]

What happened:
Tried to use nginx.ingress.kubernetes.io/auth-url: with oauth2-proxy and several allowed groups: https://oauth.example/oauth2/auth?allowed_groups=gid1,gid2 after oauth2-proxy documentation in an existing ingress.

This gave the error message:
error: ingresses.networking.k8s.io "tp-ingress-nessus-feature" could not be patched: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: annotation nginx.ingress.kubernetes.io/auth-url contains invalid value

I get the same error with \ in front of the comma, like so:
nginx.ingress.kubernetes.io/auth-url: https://oauth.example/oauth2/auth?allowed_groups=gid1\,gid2

It is not allowed in encoded form either: %2C.

It works without a comma.

I believe you will need to update the validating regex for the auth-url annotation:

ingress-nginx/internal/ingress/annotations/authreq/main.go

Line 60 in fd7e02b

Validator: parser.ValidateRegex(parser.URLWithNginxVariableRegex, true),

The current one only allows :?&=

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

Line 47 in fd7e02b

urlEnabledChars = regexp.QuoteMeta(`:?&=`)

What you expected to happen:
The ingress should have been updated.

NGINX Ingress controller version:
NGINX Ingress controller
Release: v1.9.4
Build: 846d251
Repository: https://github.com/kubernetes/ingress-nginx
nginx version: nginx/1.21.6

Kubernetes version (use kubectl version):
Server Version: v1.27.9

How to reproduce this issue:
Create an ingress with the nginx.ingress.kubernetes.io/auth-url: annotation, add a comma in a query parameter.

Install minikube/kind

• Minikube https://minikube.sigs.k8s.io/docs/start/
• Kind https://kind.sigs.k8s.io/docs/user/quick-start/

Install the ingress controller

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/static/provider/baremetal/deploy.yaml

Install an application that will act as default backend (is just an echo app)

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/docs/examples/http-svc.yaml

Create an ingress (please add any additional annotation required)

echo "
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: foo-bar
annotations:
kubernetes.io/ingress.class: nginx
nginx.ingress.kubernetes.io/auth-url: https://oauth.example/oauth2/auth?allowed_groups=g1,g2
spec:
ingressClassName: nginx # omit this if you're on controller version below 1.0.0
rules:
- host: foo.bar
http:
paths:
- path: /
pathType: Prefix
backend:
service:
name: http-svc
port:
number: 80
" | kubectl apply -f -

[longwuyuan]

Does it work without ,

…

On Tue, 6 Aug, 2024, 15:09 Kubernetes Prow Robot, ***@***.***> wrote: 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 <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow <https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:> repository. — Reply to this email directly, view it on GitHub <#11739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWWTPAJCVZLQHM3MGT3ZQCKUNAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2DCOJZHE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[mikkel3000]

Does it work without ,
…
On Tue, 6 Aug, 2024, 15:09 Kubernetes Prow Robot, @.> wrote: 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 https://git.k8s.io/community/contributors/guide/pull-requests.md. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue: repository. — Reply to this email directly, view it on GitHub <#11739 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWWTPAJCVZLQHM3MGT3ZQCKUNAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2DCOJZHE . You are receiving this because you are subscribed to this thread.Message ID: @.>

Yes, but the query parameter of oauth2-proxy requires a comma to specify several groups to give access to.
https://oauth2-proxy.github.io/oauth2-proxy/features/endpoints#auth

[longwuyuan]

Understood. Thanks. Just for checking, can you experiment with escaping and update. Escape with \

…

On Tue, 6 Aug, 2024, 15:16 mikkelnygard, ***@***.***> wrote: Does it work without , … <#m_5833239902484055101_> On Tue, 6 Aug, 2024, 15:09 Kubernetes Prow Robot, *@*.*> wrote: 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 https://git.k8s.io/community/contributors/guide/pull-requests.md <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue: <https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:> repository. — Reply to this email directly, view it on GitHub <#11739 (comment) <#11739 (comment)>>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWWTPAJCVZLQHM3MGT3ZQCKUNAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2DCOJZHE <https://github.com/notifications/unsubscribe-auth/ABGZVWWTPAJCVZLQHM3MGT3ZQCKUNAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2DCOJZHE> . You are receiving this because you are subscribed to this thread.Message ID: @.*> Yes, but the query parameter of oauth2-proxy requires a comma to specify several groups to give access to. https://oauth2-proxy.github.io/oauth2-proxy/features/endpoints#auth — Reply to this email directly, view it on GitHub <#11739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWQ22H6SUTGYFQ2LXILZQCLQZAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2TONRVGE> . You are receiving this because you commented.Message ID: ***@***.***>

[mikkel3000]

I get the same error with \ in front of the comma, like so:
nginx.ingress.kubernetes.io/auth-url: https://oauth.example/oauth2/auth?allowed_groups=g1\,g2

[longwuyuan]

Thank you for the info. Will try to update soon.

…

On Tue, 6 Aug, 2024, 15:22 mikkelnygard, ***@***.***> wrote: I get the same error with \ in front of the comma, like so: nginx.ingress.kubernetes.io/auth-url: https://oauth.example/oauth2/auth?allowed_groups=g1\,g2 — Reply to this email directly, view it on GitHub <#11739 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWUEMHXJGHSUGBJTYP3ZQCMGLAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA3DQOBQGE> . You are receiving this because you commented.Message ID: ***@***.***>

[longwuyuan]

/remove-kind bug
/kind feature
/triage-accepted

validations were enabled with a objective to secure the controller's use cases. lets wait for developers and others to comment about allowing comma character as valid.

[strongjz]

It can be configured using the following query parameters:

    allowed_groups: comma separated list of allowed groups
    allowed_email_domains: comma separated list of allowed email domains
    allowed_emails: comma separated list of allowed emails

Looks like we do need to a comma to the validation

https://oauth2-proxy.github.io/oauth2-proxy/features/endpoints/#auth

[strongjz]

/kind bug
/triage accepted
/priority backlog