Skip to content
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

Fix X-Auth-Request-Redirect value to reflect the request uri #1472

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Oct 4, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 4, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 4, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 43.566% when pulling 952a27f on aledbf:fix-auth-redirect into 8218421 on kubernetes:master.

@aledbf aledbf merged commit 0f351e2 into kubernetes:master Oct 4, 2017
@aledbf aledbf deleted the fix-auth-redirect branch October 4, 2017 17:05
@jameshartig
Copy link

@aledbf I don't see any bug or issue that is this in reference to and I don't see any description in the commit or this PR. What was this fixing? I'm confused with the commit because it moves:

proxy_set_header X-Auth-Request-Redirect $request_uri;

Into the $authPath location but that doesn't make any sense since oauth2_proxy only checks the X-Auth-Request-Redirect header in the /oauth2/sign_in page, which is not where the $authPath proxy_pass points. Additionally, the $authPath location is only for internally checking to see if a user is logged in or not so unless there was some state that was being persisted between the internal auth request and the later call to sign_in, this wouldn't do anything.

If we revert this commit and leave it in the body of the generic $path, then someone could set the signin-url to /oauth2/sign_in (with no scheme or host) and it would work correctly since the X-Auth-Request-Redirect would then be sent to the sign_in page, as oauth2_proxy expects. This would also eliminate the problematic 302 redirect that bitly/oauth2_proxy#427 talks about.

Furthermore, if we could also set the auth-url to just /oauth2/auth (and eliminate the scheme and host requirement https://github.com/kubernetes/ingress-nginx/blob/master/internal/ingress/annotations/authreq/main.go#L130 then we wouldn't need the $authPath code at all in that case and you could just use the existing /oauth2 location block set up by the oauth2_proxy ingress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants