-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CORS template check inside location for externalAuth.SignURL #8814
Add CORS template check inside location for externalAuth.SignURL #8814
Conversation
@harry1064: This issue is currently awaiting triage. If Ingress contributors determines this is a relevant issue, they will accept it by applying the The 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. |
Welcome @harry1064! |
Hi @harry1064. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Hi @harry1064 , Personally, I would like to see the info I list below before commenting. Any chance you can add this info ;
|
Hi @longwuyuan kubectl get all,ing -A -o wide
kubectl describe ing -A
kubectl describe svc
kubectl exec -it ingress-nginx-controller-5689d5bcd6-sq957 -n ingress-nginx -- cat nginx.conf
curl -v http://localhost/protected-service/protected -H "Host: app.test" -H "origin: https://allowed.origin.com"
|
Remove the message posted about deployment yaml. It is just clutter and does not help focus on the problem/solution, in my opinion. Thanks for the other post where the info requested was posted. But that is half clarity. See my thoughts below and correct me or educate me if I am wrong. This is complicated stuff so it will be good if we make all information crystal clear in these conversations. (1) Your services of --LoadBalancer are in pending state so while I see some of your URL are internal to cluster, I would not trust any of the data presented as it does not simulate a real use case. By real use case I mean if there was a service --type ClusterIP, then it is reasonable that those URLs with internal-to-cluster FQDN/HostName are working. (2) Where is this (3) Remove the rewrite annotation If you can make changes and repost or edit the current post, it will help. That is where the logs become relevant. And although I did not mention it, if you can show that this was done with |
I edited my previous message so please check. Also suggesting again that you post logs of contrllerpod in addition to |
Do you mean pending state of External IP? As I am running the cluster locally, I don' think we require it.
as we are passing
This does not matter as the according to my understanding the problem statement is whatever the exernalAuth.SignURL is configured, when we are redirecting the user it should return the CORS origin configured in the ingress resource, as other wise browser will block if the headers are missing.
This case is not even triggered in the use case we are testing here. In our case
it will go to auth-service, auth-service will return 401 and then based on our nginx config, 401 will get internally redirected to location block which will return the user 302 to redirect him to URL configured in exernalAuth.SignURL. With CORS template now added in this |
kubectl cluster-info
kubectl logs ingress-nginx-controller-5689d5bcd6-sq957 -n ingress-nginx
git diff
As |
@harry1064 this fleshing out of the details, reduces the work for approvers, so thank you very much. |
@longwuyuan Yes sure. I can write the e2-test. I am familiarizing myself with the way the e2e test are structured as I am new to project. |
@harry1064 that is awesome. Please also join the ingress-nginx-dev channel on slack in case you need to talk. This is good stuff so thank you again. |
c5abeb9
to
6b97a1c
Compare
@longwuyuan Added one e2e test case. Have a look and let me know your thoughts. |
I saw the test just now. Thank again for this. /ok-to-test @tao12345666333 @rikatz @strongjz, it seems like there has been a bug for years, that if auth & cors are used together in one ingress, then cors headers are missing. When you get a chance, please check & label this PR . I am not sure how such a bug can exist for years as auth+cors should be common enterprise use case. |
@longwuyuan Thank you too for guiding through the process. |
/assign I will add it to my list, and check it ASAP. Thanks |
@tao12345666333 , Also I did not check if this opens a corner-case vulnerability. |
Any progress on this? |
Any blocker we can help to resolve here? |
@tao12345666333 @rikatz @strongjz Is there any progress on this PR? We also have this problem and would be happy if this can be released. |
/lgtm Thanks |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harry1064, rikatz The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…ernetes#8814) * Add CORS template check inside location for externalAuth.SignURL * Add testcase for CORS header for auth-signin redirect with CORS enabled.
What this PR does / why we need it:
This PR add the CORS template while handling redirect location for externalAuth.SignURL.
Similar to
add_header Set-Cookie $auth_cookie;
need to again set inside thelocation
block handling redirect for externalAuth.SignURL, we have added the CORS template again.Types of changes
Which issue/s this PR fixes
fixes #8786
How Has This Been Tested?
I've tested locally by setting up cluster using
Kind
and following instruction from the Issue reporter. After making this change, provided CORS related annotation are provide and as wellnginx.ingress.kubernetes.io/auth-signin
, response header contains the CORS related headers.Checklist: