search

kubernetes / ingress-nginx

Ingress-NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.28k stars 8.21k forks source link

bug: admission webhook denies auth-url with comma #11739

Closed mikkelnygard closed 3 weeks ago

mikkelnygard commented 1 month ago

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: https://github.com/kubernetes/ingress-nginx/blob/fd7e02b97617d7869f583ff0182a893d5ac61d7f/internal/ingress/annotations/authreq/main.go#L60

The current one only allows :?&= https://github.com/kubernetes/ingress-nginx/blob/fd7e02b97617d7869f583ff0182a893d5ac61d7f/internal/ingress/annotations/parser/validators.go#L47

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

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): Server Version: v1.27.9

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

Install minikube/kind

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:

longwuyuan commented 1 month ago

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 https://github.com/kubernetes/ingress-nginx/issues/11739#issuecomment-2270841999, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWWTPAJCVZLQHM3MGT3ZQCKUNAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2DCOJZHE . You are receiving this because you are subscribed to this thread.Message ID: @.***>

mikkelnygard commented 1 month ago

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 commented 1 month ago

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 , … <#m5833239902484055101> 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) https://github.com/kubernetes/ingress-nginx/issues/11739#issuecomment-2270841999>, 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 https://github.com/kubernetes/ingress-nginx/issues/11739#issuecomment-2270857651, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWQ22H6SUTGYFQ2LXILZQCLQZAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA2TONRVGE . You are receiving this because you commented.Message ID: @.***>

mikkelnygard commented 1 month ago

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 commented 1 month ago

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 https://github.com/kubernetes/ingress-nginx/issues/11739#issuecomment-2270868801, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWUEMHXJGHSUGBJTYP3ZQCMGLAVCNFSM6AAAAABMB6ERGGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZQHA3DQOBQGE . You are receiving this because you commented.Message ID: @.***>

longwuyuan commented 1 month ago

/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 commented 3 weeks ago 
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 commented 3 weeks ago

/kind bug /triage accepted /priority backlog