basic auth service #2262

kunmingg · 2019-01-12T00:58:26Z

Backend part.
And Frontend Part is: #2316

Test passed, ready to merge.

fix #2147
fix #2148
fix #2149

This change is

kunmingg · 2019-01-12T00:58:55Z

/assign @kkasravi
/assign @jlewi

kunmingg · 2019-01-12T01:07:05Z

/hold
Will manual test when ready.

kunmingg · 2019-01-23T05:27:34Z

/hold cancel

jlewi · 2019-01-24T02:04:31Z

components/gatekeeper/auth/AuthServer.go

+func (s *authServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	log.Infof("Path check, url: %v, path: %v", r.URL, r.URL.Path)
+	// login page open to everyone; all other path requires auth with Password or cookie
+	if strings.HasPrefix(r.URL.Path, "/" + LoginPagePath) || s.authCookie(r) == true {


Should we be doing an exact match here to path
r.URL.Path, "/" + LoginPagePath

What if by some coincedence we have a path for which this is a prefix but is not an exact match?

Browser need access resources like "/LoginPagePath/static/..." as well.
How about do exact match "/LoginPagePath" or prefix match "/LoginPagePath/"?

Should be safe to assume any thing behind "/LoginPagePath/" is public?

I see. Maybe we could file an issue to follow up on this. My concern is what if some app randomly adds some prefix like

/LoginPagePath/mynotebook

Then we would match it.

Can we be more restrictive e.g.

Exact match /LoginPagePath

Prefix match /LoginPagePath/static

Also maybe we can add in some nonsense e.g. make

/LoginPagePath-a12c

This way its less likely an app would have it as a prefix.

jlewi · 2019-01-24T02:09:02Z

components/gatekeeper/auth/AuthServer.go

+	// login page open to everyone; all other path requires auth with Password or cookie
+	if strings.HasPrefix(r.URL.Path, "/" + LoginPagePath) || s.authCookie(r) == true {
+		// Handle request from login page
+		if r.Header.Get(LoginPageHeader) != "" {


Can you explain what this if block is doing? Is it modifying the request to indicate that it should be allowed?

This part take care of user's re-login
They already have cookie in browser, so "StatusResetContent" bring them to kubeflow central dashboard.

jlewi · 2019-01-24T02:10:28Z

components/gatekeeper/auth/AuthServer.go

+
+// Default auth check service
+func (s *authServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	log.Infof("Path check, url: %v, path: %v", r.URL, r.URL.Path)


Can we reject non https traffic?

We don't want user logging in if they aren't using https because that would send username/password in the clear?

Can we only access https at gclb?

I don't think there's any way to force GCLB to only accept https traffic. You can look at what we do in click to deploy. Basically we'd want to check if the protocol is http and then redirect to https

jlewi · 2019-01-24T02:12:30Z

components/gatekeeper/auth/AuthServer.go

+	if len(namepw) != 2 {
+		return false
+	}
+	err = bcrypt.CompareHashAndPassword([]byte(s.pwhash), []byte(namepw[1]))


So CompareHashAndPassword returns error if they don't match?

jlewi · 2019-01-24T02:13:58Z

components/gatekeeper/auth/AuthServer.go

+// auth with cookie
+func (s *authServer) authCookie(r *http.Request) bool {
+	if cookie, err := r.Cookie(CookieName); err == nil {
+		if val, ok := s.cookies[cookie.Value]; ok {


What prevents someone from spoofing a cookie?

SameSite: http.SameSiteStrictMode
We restrict the cookie so can only be used for same site.
Should we encrypt value as well?

You mean with bcrypt? So we only keep track of the encrypted value?

Yes, server side only keep encrypted password hash.

If its not too difficult I think that would be valuable; for the same reason its valuable with the password.

jlewi · 2019-01-24T02:16:11Z

components/gatekeeper/auth/AuthServer.go

+	defer s.serverMux.Unlock()
+	// Cookie expire after 12 hours
+	log.Info("cookie set: set new cookie value!")
+	s.cookies[cookieVal] = time.Now().Add(12 * time.Hour)


So the cookie is stored in memory, so if the pod is restarted the cookie will no longer be valid. Will we automatically redirect user to log back in?

Yes, all unauth access end up redirect to login.

Might be worth a P2 issue to fix in a follow up.

jlewi · 2019-01-24T02:17:48Z

Thanks this looks great.

jlewi · 2019-01-24T02:34:56Z

Will LetsEncrypt be an issue? Do we need to allow for the ACME challenge?

On GCP one option would be to migrate to Managed Certificates.

jlewi · 2019-01-25T03:15:10Z

kubeflow/common/basic-auth.libsonnet

+
+      },
+      spec: {
+        replicas: 1,


Add a comment here explaining this should always be 1 because the cookie is stored in memory so if we use more than 1 replica we will have problems because not all replicas will have the cookie.

Comments updated

jlewi · 2019-01-25T03:16:05Z

On second thought I don't think LetsEncrypt is an issue because it adds a separate path to the ingress.

I had one more comment about adding a comment about number of replicas; otherwise this is good to go.

jlewi

Reviewed 1 of 5 files at r1, 1 of 5 files at r2, 4 of 8 files at r3, 1 of 2 files at r4, 2 of 2 files at r5, 1 of 1 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @abhi-g, @ellis-bigelow, and @kunmingg)

components/gatekeeper/auth/AuthServer.go, line 122 at r4 (raw file):

Previously, jlewi (Jeremy Lewi) wrote…

If its not too difficult I think that would be valuable; for the same reason its valuable with the password.

Created #2341 for this

jlewi · 2019-01-28T16:19:10Z

/lgtm
/approve

k8s-ci-robot · 2019-01-28T16:19:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

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:

~~OWNERS~~ [jlewi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kunmingg · 2019-01-28T23:43:07Z

/retest

* [WIP] basic auth service * complete cookie setting and auth logic * fmt * add ksonnet lib; fix bugs * corrent redirect; update image * address review feedbacks * update comments

[WIP] basic auth service

070fe63

k8s-ci-robot added the do-not-merge/work-in-progress label Jan 12, 2019

googlebot added the cla: yes label Jan 12, 2019

k8s-ci-robot added the size/L label Jan 12, 2019

k8s-ci-robot requested a review from abhi-g January 12, 2019 00:58

k8s-ci-robot added the approved label Jan 12, 2019

k8s-ci-robot requested a review from ellistarn January 12, 2019 00:58

k8s-ci-robot assigned jlewi and kkasravi Jan 12, 2019

k8s-ci-robot added the do-not-merge/hold label Jan 12, 2019

complete cookie setting and auth logic

955c826

k8s-ci-robot removed the approved label Jan 17, 2019

kunmingg added 2 commits January 17, 2019 11:34

fmt

512203d

add ksonnet lib; fix bugs

c95381a

k8s-ci-robot added size/XL and removed size/L labels Jan 23, 2019

kunmingg changed the title ~~[WIP] basic auth service~~ basic auth service Jan 23, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress label Jan 23, 2019

k8s-ci-robot removed the do-not-merge/hold label Jan 23, 2019

kunmingg mentioned this pull request Jan 23, 2019

basic auth login UI #2316

Merged

corrent redirect; update image

60c8cd3

jlewi reviewed Jan 24, 2019

View reviewed changes

address review feedbacks

144e1c8

jlewi reviewed Jan 25, 2019

View reviewed changes

update comments

64bf27d

jlewi mentioned this pull request Jan 28, 2019

Basic auth: use bcrypt on the cookies. #2341

Closed

jlewi suggested changes Jan 28, 2019

View reviewed changes

jlewi approved these changes Jan 28, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Jan 28, 2019

k8s-ci-robot added the approved label Jan 28, 2019

jlewi mentioned this pull request Jan 28, 2019

[basic auth] Component test for gatekeeper #2342

Closed

k8s-ci-robot merged commit d73e830 into kubeflow:master Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic auth service #2262

basic auth service #2262

kunmingg commented Jan 12, 2019 •

edited

Loading

kunmingg commented Jan 12, 2019

kunmingg commented Jan 12, 2019

kunmingg commented Jan 23, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 24, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 24, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 25, 2019

jlewi Jan 24, 2019

kunmingg Jan 24, 2019

jlewi Jan 25, 2019

jlewi commented Jan 24, 2019

jlewi commented Jan 24, 2019

jlewi Jan 25, 2019

kunmingg Jan 25, 2019

jlewi commented Jan 25, 2019

jlewi left a comment

jlewi commented Jan 28, 2019

k8s-ci-robot commented Jan 28, 2019

kunmingg commented Jan 28, 2019

basic auth service #2262

basic auth service #2262

Conversation

kunmingg commented Jan 12, 2019 • edited Loading

kunmingg commented Jan 12, 2019

kunmingg commented Jan 12, 2019

kunmingg commented Jan 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi commented Jan 24, 2019

jlewi commented Jan 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlewi commented Jan 25, 2019

jlewi left a comment

Choose a reason for hiding this comment

jlewi commented Jan 28, 2019

k8s-ci-robot commented Jan 28, 2019

kunmingg commented Jan 28, 2019

kunmingg commented Jan 12, 2019 •

edited

Loading