Feature/support OIDC auth #3452

johnnyaug · 2022-06-02T20:30:16Z

Support authentication with OIDC - step 1

Allows the user to configure an external OIDC provider. Once done, the login page will include a link to sign-in using the provider. When a user logs in with the provider, a corresponding lakeFS user is created.

Permissions (authorization) for the created user are still managed internally in lakeFS.

This reverts commit 35e0c42.

arielshaqed

Thanks! Looks really nice, not requesting anything major.

cmd/lakefs/cmd/run.go

docs/reference/oidc.md

pkg/api/auth_middleware.go

pkg/api/oidc_login_handler.go

arielshaqed · 2022-06-12T09:57:54Z

pkg/api/oidc_login_handler.go

+		if r.TLS != nil {
+			scheme = "https"
+		}
+		u := url.URL{
+			Scheme: scheme,
+			Host:   r.Host,
+			Path:   BaseURL + "/oidc/callback",
+		}


Can't we use a relative URL? This way seems to reimplement a particular relative URL.

You mean dropping the host from the redirect URL?
OAuth requires providing an absolute URL for this parameter.

:-(

If we had the request URL u we could still u.Resolve(BaseURL + "/oidc/callback"), but I can understand that this might not be readily available. (If you can get it onto r, then of course that would be better!)

pkg/ddl/000038_oidc_user.down.sql

webui/src/pages/auth/login.jsx

arielshaqed

Thanks!

arielshaqed · 2022-06-13T06:19:48Z

pkg/api/oidc_login_handler.go

+	IDTokenClaimsSessionKey = "id_token_claims"
+	StateSessionKey         = "state"
+
+	stateByteSize = 32


Is there a recommendation for this? The best I could find was this on login.gov:

state
A unique value at least 22 characters in length used for maintaining state between the request and the callback. This value will be returned to the client on a successful authorization.

Admittedly this is quite a weak recommendation.

22 characters from a set of (say) 64 different characters is 6*22=132 bites, or 32.5 bytes. But I would be happier to specify this in terms of characters, and use 22 or some recommended value. nanoid is your friend for doing this sort of thing.

arielshaqed · 2022-06-13T06:24:29Z

pkg/api/oidc_login_handler.go

+		if r.TLS != nil {
+			scheme = "https"
+		}
+		u := url.URL{
+			Scheme: scheme,
+			Host:   r.Host,
+			Path:   BaseURL + "/oidc/callback",
+		}


:-(

If we had the request URL u we could still u.Resolve(BaseURL + "/oidc/callback"), but I can understand that this might not be readily available. (If you can get it onto r, then of course that would be better!)

arielshaqed · 2022-06-13T06:31:00Z

pkg/ddl/000038_oidc_user.up.sql

+BEGIN;
+
+ALTER TABLE auth_users
+    ADD COLUMN IF NOT EXISTS external_id VARCHAR(255) UNIQUE NULL;


I don't think we use the NULL (non-)constraint in our DDL. I had to look up what it means, so I slightly prefer that you remove it from here.

cmd/lakefs/cmd/run.go

ortz · 2022-06-15T08:28:04Z

cmd/lakefs/cmd/run.go

+				logger.WithError(err).Fatal("Failed to initialize OIDC provider")
+			}
+			cfg.GetBlockstoreDefaultNamespacePrefix()
+			oauthConfig = &oauth2.Config{


shouldn't we add the RedirectURL also?

Since the redirect URL also contains the host, I construct it ad-hoc in every login.

ortz · 2022-06-15T08:48:02Z

pkg/api/controller.go

+	session, err := c.sessionStore.Get(r, OIDCAuthSessionName)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, err)
+		return
+	}
+	session.Values = nil
+	err = session.Save(r, w)
+	if err != nil {
+		writeError(w, http.StatusInternalServerError, err)
+		return
+	}


what happens if OIDC is not configured?
shouldn't this return an error?

This is the global logout, not specific to OIDC.
We could perform this part only if OIDC is configured - but maybe it's better to clear the session even if OIDC is disabled.

nopcoder · 2022-06-15T22:19:42Z

cmd/lakefs/cmd/run.go

@@ -14,6 +14,7 @@ import (
 	"syscall"
 	"time"

+	"github.com/coreos/go-oidc"


This is v2 - I think you want to use github.com/coreos/go-oidc/v3/oidc

nopcoder · 2022-06-15T22:44:40Z

go.mod

@@ -81,6 +81,14 @@ require (
 	golang.org/x/time v0.0.0-20220224211638-0e9765cccd65
 )

+require (
+	github.com/coreos/go-oidc v2.2.1+incompatible // indirect


looks like you need to run go mod tidy

nopcoder

Some minor issues. Two suggestions. Very glad we have all the rest. Very cool!!

nopcoder · 2022-06-17T17:12:22Z

cmd/lakefs/cmd/run.go

+		oidcConfig := cfg.GetAuthOIDCConfiguration()
+		var oauthConfig *oauth2.Config
+		var oidcProvider *oidc.Provider
+		if oidcConfig != nil && oidcConfig.Enabled {


Suggest not to have the oidc config as a pointer in the first place - we have Enabled property, so having this scope of configuration disabled by default will do the work.

nopcoder · 2022-06-17T17:17:14Z

pkg/api/auth_middleware.go

+			case "oidc_auth":
+				user, err = userFromOIDC(ctx, logger, authService, session, oidcConfig)


Suggest to merge the cookie_auth and the oidc_auth. Handling authentication using session storage (aka cookie) can use one mechanism.
In this case we are adding a package that handle the cookie storage - it can be also used to handle the current server side cookie we used without OIDC. It just need to include this information as part of the session cookie.
Users will have to login again - but I think it will pay off in the long run.

Done: also added (temporary) migration from the old cookie, so that users are not logged out.

pkg/api/auth_middleware.go

nopcoder · 2022-06-18T21:30:35Z

pkg/api/controller.go

 ) *Controller {
+	gob.Register(oidc.Claims{})


can we use json encoding and not gob? I assume this is for session information serialization.

Synced f2f: not really an option here since internally gorilla sessions have a map[interface{}]interface{} which is not support by the JSON encoder.

nopcoder · 2022-06-18T21:43:53Z

pkg/api/oidc_login_handler.go

+		scheme := "http"
+		if r.TLS != nil {
+			scheme = "https"
+		}


we do not perform ssl termination - I think you should use a configured base url to build the callback and it should include the scheme and host.

nopcoder · 2022-06-18T21:45:51Z

pkg/api/oidc_login_handler.go

+
+		session, err := sessionStore.Get(r, OIDCAuthSessionName)
+		if err != nil {
+			logger.Errorf("failed to get oidc session: %w", err)


Suggested change

logger.Errorf("failed to get oidc session: %w", err)

logger.WithError(err).Error("failed to get oidc session")

nopcoder · 2022-06-18T21:46:10Z

pkg/api/oidc_login_handler.go

+		})
+		state, err := nanoid.New(stateLength)
+		if err != nil {
+			logger.Errorf("failed to generate state for oidc: %w", err)


Suggested change

logger.Errorf("failed to generate state for oidc: %w", err)

logger.WithError(err).Error("failed to generate state for oidc")

nopcoder · 2022-06-18T22:11:10Z

pkg/api/controller.go

+	err := doOIDCLogout(w, r, c.sessionStore)
+	if err != nil {
+		c.Logger.WithError(err).Error("failed to perform OIDC logout")
+	}


Think we need to change one of two places. We can ignore the error here as loging a session in a system that doesn't have a session will fail. Or ignore it as we already just log it.

Fixed: now ignoring the error from sessionStore.Get when it makes sense.

nopcoder · 2022-06-18T22:13:02Z

pkg/auth/oidc/authenticator.go

+	rawIDToken, ok := token.Extra("id_token").(string)
+	if !ok {
+		return nil, err
+	}


There is no err here

nopcoder · 2022-06-18T22:14:16Z

pkg/ddl/000038_oidc_user.up.sql

+ALTER TABLE auth_users
+    ADD COLUMN IF NOT EXISTS external_id VARCHAR(255) UNIQUE;


Any reason to use varchar and not text?

Missing drop column in the ...down.sql.

Changed to TEXT

According to @arielshaqed's suggestion, living the down migration empty, and made the up-migration idempotent.

nopcoder

Added some comments, the one I was worried about is counting on save state follow up by get. All the rest looks great, thanks!

nopcoder · 2022-06-21T08:00:01Z

cmd/lakefs/cmd/run.go

+			oauthConfig = &oauth2.Config{
+				ClientID:     oidcConfig.ClientID,
+				ClientSecret: oidcConfig.ClientSecret,
+				RedirectURL:  oidcConfig.CallbackBaseURL + api.BaseURL + "/oidc/callback",


Need to trim leading "/" from callback base url so we will not end with "//"

nopcoder · 2022-06-21T08:23:39Z

pkg/api/auth_middleware.go

+// Deprecated
+// TODO(johnnyaug) remove this a week after released
+func migrateFromLegacyCookie(r *http.Request, w http.ResponseWriter, logger logging.Logger, sessionStore sessions.Store) {


Need to use code doc - start with the method name

nopcoder · 2022-06-21T08:26:45Z

pkg/api/auth_middleware.go

+				if token == "" {
+					migrateFromLegacyCookie(r, w, logger, sessionStore)
+				}
+				internalAuthSession, err = sessionStore.Get(r, InternalAuthSessionName)


We need to clear the cookie as you did - but reading the session after saving it - I don't think it will work.
When you "save" a cookie - you write a header. When you "read" a cookie, it is from the request header that we already parsed.
I maybe wrong as the code can implement some kind of workaround - just wanted to verify with you how it was implemented.

nopcoder · 2022-06-21T08:43:56Z

pkg/api/controller.go

 func (c *Controller) Login(w http.ResponseWriter, r *http.Request, body LoginJSONRequestBody) {
+	err := clearSession(w, r, c.sessionStore, OIDCAuthSessionName)


need to clear the old one - not the new one while login

synced f2f: clearing the OIDC session here, and setting the "internal login" one.

nopcoder · 2022-06-21T08:44:25Z

pkg/api/controller.go

+	ctx := r.Context()
+	session, _ := c.sessionStore.Get(r, OIDCAuthSessionName)
+	if r.URL.Query().Get("state") != session.Values[StateSessionKey] {
+		writeError(w, http.StatusBadRequest, "Invalid state parameter.")


Suggested change

writeError(w, http.StatusBadRequest, "Invalid state parameter.")

writeError(w, http.StatusBadRequest, "Invalid state parameter")

nopcoder · 2022-06-21T08:47:18Z

pkg/api/oidc_login_handler.go

+			writeError(w, http.StatusInternalServerError, "Failed to redirect to login page")
+			return
+		}
+
+		session, _ := sessionStore.Get(r, OIDCAuthSessionName)
+		session.Values[StateSessionKey] = state
+		if err := session.Save(r, w); err != nil {
+			logger.WithError(err).Error("failed to save oidc session")
+			writeError(w, http.StatusInternalServerError, "Failed to redirect to login page")


If possible, change something small in the writeError in one of the two, so we can identify the issue without going into the logs. No need to expose more information if not needed, just a way to distinct between the two.

johnnyaug added 14 commits May 17, 2022 19:58

wip

36d2f7e

Merge branch 'master' into feature/support_oidc_auth

cb7ef87

wip

1917428

wip

2773c98

remove logout on provider side

bc0e929

some docs

8701140

some docs

9962f06

Merge branch 'master' into feature/support_oidc_auth

0eaae0c

wipˆ

e85f159

fix marshalling bug in db service

35e0c42

Revert "fix marshalling bug in db service"

4196132

This reverts commit 35e0c42.

Merge branch 'master' into feature/support_oidc_auth

8273b0e

Merge branch 'master' into feature/support_oidc_auth

b531983

wip

8cb2bcd

johnnyaug self-assigned this Jun 9, 2022

johnnyaug added the include-changelog PR description should be included in next release changelog label Jun 9, 2022

johnnyaug added 3 commits June 12, 2022 11:34

wip

2577260

Merge branch 'master' into feature/support_oidc_auth

3cd7c8d

docs

4e169b2

johnnyaug requested review from nopcoder, arielshaqed and guy-har and removed request for nopcoder and arielshaqed June 12, 2022 09:10

johnnyaug marked this pull request as ready for review June 12, 2022 09:10

fix docs

55370e1

arielshaqed requested changes Jun 12, 2022

View reviewed changes

fix tests and linters

e012862

johnnyaug added 4 commits June 12, 2022 13:22

cr fixes

29211b7

typo

a0153d7

cr fix

d402c95

cr fix + more docs

a5e8518

johnnyaug requested a review from arielshaqed June 12, 2022 16:02

arielshaqed approved these changes Jun 13, 2022

View reviewed changes

cr fixes

cb40ff2

ortz reviewed Jun 15, 2022

View reviewed changes

nopcoder reviewed Jun 15, 2022

View reviewed changes

johnnyaug added 4 commits June 16, 2022 11:05

cr fixes

d0c7df8

Merge branch 'master' into feature/support_oidc_auth

d288f0f

add external id to proto

840fed1

add friendly name in places

253a994

nopcoder approved these changes Jun 18, 2022

View reviewed changes

johnnyaug added 5 commits June 19, 2022 11:05

Merge branch 'master' into feature/support_oidc_auth

5f4941a

CR fixes!

26d6753

fix lint

863d356

Merge branch 'master' into feature/support_oidc_auth

47c3e08

cr fixes

c5190eb

johnnyaug requested a review from nopcoder June 20, 2022 11:54

johnnyaug added 2 commits June 21, 2022 10:52

Merge branch 'master' into feature/support_oidc_auth

282a5f2

fix lint

38e43a6

nopcoder approved these changes Jun 21, 2022

View reviewed changes

johnnyaug added 2 commits June 21, 2022 12:49

cr fixes

f4a5e57

rename cookie in swagger

d1953d8

johnnyaug merged commit b55e3a4 into master Jun 21, 2022

johnnyaug deleted the feature/support_oidc_auth branch June 21, 2022 11:16

		case "oidc_auth":
		user, err = userFromOIDC(ctx, logger, authService, session, oidcConfig)

	logger.Errorf("failed to get oidc session: %w", err)
	logger.WithError(err).Error("failed to get oidc session")

	logger.Errorf("failed to generate state for oidc: %w", err)
	logger.WithError(err).Error("failed to generate state for oidc")

		ALTER TABLE auth_users
		ADD COLUMN IF NOT EXISTS external_id VARCHAR(255) UNIQUE;

		func (c Controller) Login(w http.ResponseWriter, r http.Request, body LoginJSONRequestBody) {
		err := clearSession(w, r, c.sessionStore, OIDCAuthSessionName)

	writeError(w, http.StatusBadRequest, "Invalid state parameter.")
	writeError(w, http.StatusBadRequest, "Invalid state parameter")

Feature/support OIDC auth #3452

Feature/support OIDC auth #3452

Conversation

johnnyaug commented Jun 2, 2022 • edited Loading

Support authentication with OIDC - step 1

arielshaqed left a 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

arielshaqed left a 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

nopcoder left a 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

johnnyaug Jun 19, 2022 • edited Loading

Choose a reason for hiding this comment

nopcoder left a 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

johnnyaug commented Jun 2, 2022 •

edited

Loading

johnnyaug Jun 19, 2022 •

edited

Loading