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

Add support for "signin url" #410

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented Mar 9, 2017

replaces #190

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

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 45.963% when pulling 681af2d on aledbf:colemickens-signin-url into 1b1e960 on kubernetes:master.

@rikatz
Copy link
Contributor

rikatz commented Mar 9, 2017

@aledbf Just a question, is this valid only for external authentications? We could make this usable also for other authentications (like basic https://github.com/kubernetes/ingress/blob/master/controllers/nginx/configuration.md#authentication), and redirect a user even when using this kind, couldn't we? :)

We could also make this available in nginx.tmpl for certificate authentications, but for different error codes (http://nginx.org/en/docs/http/ngx_http_ssl_module.html#errors).

I haven't tested this yet, but it should work fine. The only difference is that when we use the CertAuth, the error code is '496'.

Will test this now and let you know.

@aledbf
Copy link
Member Author

aledbf commented Mar 9, 2017

is this valid only for external authentications?

Yes

We could make this usable also for other authentications (like basic)

In the basic auth we already have a secret with all the passwords. If the user enters an invalida user/pass we just need to return 401.

and redirect a user even when using this kind, couldn't we? :)

We can do that but to where? (keep in mind my comment ^^)

@rikatz
Copy link
Contributor

rikatz commented Mar 9, 2017

@aledbf Have tested succesfully here this 'redirect' when a CertAuth doesn't work goes well (no cert presented or invalid cert presented).

So what's my suggestion: We decouple this from the External Authentication and create a new kind of annotation (maybe in the auth/main.go) that allows user to redirect to another site / page when it gots an unauthorized error.

And in nginx.tmpl we should add this as a return for 401 error code (when the annotation exists) and as a return for 495 and 496 error code inside this if: https://github.com/kubernetes/ingress/blob/master/controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl#L233

Does this make sense? :)

@rikatz
Copy link
Contributor

rikatz commented Mar 9, 2017

@aledbf Have seen your comment now.

My idea is to make this something more global, like not being only a redirect for signin, but a redirect for an unauthorized request.

This implementation is fine for me. If this is the case, I can open a PR latelly to implement this as a separate annotation also for CertAuth :)

@aledbf
Copy link
Member Author

aledbf commented Mar 9, 2017

@rikatz lets merge this PR and add the redirect in the other annotations. I am not sure it makes sense to use the sign-in annotation in the basic or cert auth (in both cases we have the data source against we validate the credentials) but maybe it's me :)

@aledbf aledbf merged commit a5f8af7 into kubernetes:master Mar 9, 2017
@colemickens
Copy link
Contributor

You're still planning to have a release tomorrow? I'm anxious to get the nginx-ingress chart updated...

@aledbf
Copy link
Member Author

aledbf commented Mar 9, 2017

@colemickens yes, just waiting to merge #325

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. nginx
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants