Remote Authenticator Service & LDAP Removal #5285

Isan-Rivkin · 2023-02-19T17:14:05Z

Support remote authentication service, updated configuration, docs and functionality.
[Breaking Change] Remove LDAP functionality including configuration.

…d-deprecate-ldap

Isan-Rivkin · 2023-02-20T17:00:05Z

@nopcoder please jump in as well if you can take a look 🙏
It's mostly done, so feel free to skim through it.
I'll ping when all done.

arielshaqed

Thanks!

Nothing major requested :-)

cmd/lakefs/cmd/run.go

arielshaqed · 2023-02-20T22:02:09Z

cmd/lakefs/cmd/run.go

+		externalAuthenticator, err := newExternalBasicAuthenticator(cfg, authService, logger)
+
+		if err != nil {
+			fmt.Printf("%s: %s\n", "failed to create external authenticator", err)


Please exit lakeFS from the main program. This function could just return an error, which would let us wrap it with additional information about why the LDAP authenticator got called.

Also if we did that then we would not be writing error messages to standard output as we end up doing here (although that is of course easy to fix at this place).

I'm not quite sure what you mean, can you take a look at the change I added? If not that then please elaborate.
Also, assuming I understand you correctly notice I applied the same change to allowForeign, err := cmd.Flags().GetBool(mismatchedReposFlagName) in the same file as well.

arielshaqed · 2023-02-20T22:02:54Z

docs/reference/authentication.md

@@ -27,6 +27,12 @@ Web UI at Administration / Users to create users. Users have an access key
 for logging into the Web UI or authenticating programmatic requests to the API
 Server or the S3 Gateway.

+#### Remote Authenticator Service 
+
+LakeFS server supports external authentication, the feature can be configured by providing an HTTP endpoint to an external authentication service. This integration can be especially useful if you already have an existing authentication system in place, as it allows you to reuse that system instead of maintaining a new one.


@ortz how much guidance do we plan for explaining external authentication?

let's see how many requests we got for this and we'll invest efforts accordingly.

arielshaqed · 2023-02-20T22:04:03Z

pkg/auth/remote_authenticator/authenticator.go

+
+// Expected response from the remote authenticator service
+type AuthenticationResponse struct {
+	// optional, if returned then the user will be used as the official username in lakeFS


Not sure that this is how LDAP works today. Isn't this planned to be the user's DN?

Yes correct, but the field name is generic to different systems, not sure what's the question?

pkg/auth/remote_authenticator/authenticator.go

arielshaqed · 2023-02-20T22:10:31Z

pkg/auth/remote_authenticator/authenticator.go

+	data, err := ra.doRequest(log, req)
+
+	if err != nil {
+		return "", fmt.Errorf("doing http request %s: %w", username, err)


Suggested change

return "", fmt.Errorf("doing http request %s: %w", username, err)

return "", fmt.Errorf("remote authenticate %s: %w", username, err)

I think it's less indicative since it's less explicit on which step failed. there are many authenticators running (chain) with errors chaining when they occur.
I prefer to keep it (specifically the part about http)

pkg/auth/remote_authenticator/authenticator.go

pkg/config/config.go

cmd/lakefs/cmd/run.go

…d-deprecate-ldap

nopcoder · 2023-02-21T22:52:59Z

cmd/lakefs/cmd/run.go

@@ -25,6 +25,7 @@ import (
 	"github.com/treeverse/lakefs/pkg/auth"
 	"github.com/treeverse/lakefs/pkg/auth/crypt"
 	"github.com/treeverse/lakefs/pkg/auth/email"
+	remoteauth "github.com/treeverse/lakefs/pkg/auth/remote_authenticator"


authremote or same package - not sure we need a different package for this one.

Fixed

Regarding another package, even though it's fairly small, the reason I prefer another package is due to encapsulation. In this particular case for a new comer like myself it's really confusing what code in auth suppose to do, authentication, authorization etc.

nopcoder · 2023-02-21T23:07:37Z

cmd/lakefs/cmd/run.go

-				WithField("feature", "LDAP").
-				Warn("Enabling LDAP on lakeFS server, but this functionality is deprecated")
-			middlewareAuthenticator = append(middlewareAuthenticator, newLDAPAuthenticator(cfg.Auth.LDAP, authService))
+		externalAuthenticator, err := newExternalBasicAuthenticator(cfg, authService, logger)


Think we should drop the Basic from the name - it is a remote authenticator. So maybe call it

We have external and remote - I assume this func was made to consolidate LDAP and Remote. It the code looks more complex as the previous code was simple: check the configuration and chain the auth to the middleware.

Suggest: moving the building of the auth middleware chain to a function (not partial). Check the configuration validity as part of Config, so the code here will just allocate the right middleware based on verified configuration.

it's now all in the same place like LDAP used to be.

cmd/lakefs/cmd/run.go

nopcoder · 2023-02-22T09:42:54Z

pkg/auth/remote_authenticator/authenticator.go

+	if err != nil {
+		log.WithError(err).Error("failed creating request")
+		return "", auth.ErrInvalidRequest
+	}


We just long the err in this case and throw the error - I suggest to return the original in this case. Alt. will be to wrap it by invalid request, but I don't see the point in this case.

pkg/auth/remote_authenticator/authenticator.go

nopcoder · 2023-02-22T09:57:50Z

pkg/auth/remote_authenticator/authenticator.go

+	if err != nil {
+		return "", fmt.Errorf("add newly created remote auth user %s to %s: %w", dbUsername, ra.Config.DefaultUserGroup, err)
+	}
+	return newUser.Username, nil


Use dbUsername while we use it all the way until here. Or use newUser.Username for all places we used dbUsername.

nopcoder · 2023-02-22T09:58:13Z

pkg/auth/remote_authenticator/authenticator.go

+}
+
+func (la *RemoteAuthenticator) String() string {
+	return RemoteAuthSource


Do we need to serialize remote authenticator as string?

Yes, it's used by the chain authenticator.

…d-deprecate-ldap

ortz

Thanks @Isan-Rivkin !
Overall, looks good to me... I do have some questions

ortz · 2023-02-23T13:48:15Z

docs/reference/authentication.md

+
+lakeFS server supports external authentication, the feature can be configured by providing an HTTP endpoint to an external authentication service. This integration can be especially useful if you already have an existing authentication system in place, as it allows you to reuse that system instead of maintaining a new one.
+
+TBD 


leftover? or you plan on adding content here?

Not a left over, just not sure how deep to do see

updated all relevant docs.

ortz · 2023-02-23T13:50:05Z

pkg/config/config.go

-		LDAP *LDAP
-		OIDC OIDC
+		RemoteAuthenticator *RemoteAuthenticator `mapstructure:"remote_authenticator"`
+		OIDC                OIDC                 `mapstructure:"oidc"`


is this relevant?

Yes, The OIDC itself config is relevant. (From previous PR's)
here it's only mapstructure:"oidc" following comment previously about alignment incase field name change.

pkg/auth/remoteauthenticator/authenticator.go

ortz · 2023-02-23T13:55:49Z

pkg/auth/remoteauthenticator/authenticator.go

+	_, err = ra.AuthService.CreateCredentials(ctx, newUser.Username)
+	if err != nil {
+		return "", fmt.Errorf("create credentials for remote auth user %s: %w", newUser.Username, err)
+	}


why do we need to create credentials for that user?

Actually it's a good question, right now it's backward compatible with the LDAP implementation.

I couldn't find a reason for it, testing without this function CreateCredentials seems to work.
Do you happen to know why it's here @arielshaqed @nopcoder ?

I don't remember why we did it this way. I think I didn't want to special-case the existing auth path, which needed to find some credentials or something??!?

Anyway if it works then we're well rid of it >:-)

arielshaqed

Thanks!

Minor points, mostly about logging...

pkg/auth/remoteauthenticator/authenticator.go

arielshaqed · 2023-02-26T06:55:56Z

pkg/auth/remoteauthenticator/authenticator.go

+	_, err = ra.AuthService.CreateCredentials(ctx, newUser.Username)
+	if err != nil {
+		return "", fmt.Errorf("create credentials for remote auth user %s: %w", newUser.Username, err)
+	}


I don't remember why we did it this way. I think I didn't want to special-case the existing auth path, which needed to find some credentials or something??!?

Anyway if it works then we're well rid of it >:-)

pkg/config/config.go

…d-deprecate-ldap

nopcoder · 2023-02-26T10:43:33Z

pkg/config/config.go

@@ -163,8 +166,8 @@ type Config struct {
 			Token           string
 			SupportsInvites bool `mapstructure:"supports_invites"`
 		}
-		LDAP *LDAP
-		OIDC OIDC
+		RemoteAuthenticator *RemoteAuthenticator `mapstructure:"remote_authenticator"`


RemoteAuthenticator have Enabled property - suggest to remove the pointer from here. It will make handling the configuration less issue in case it is nil.
We only need to check cfg.RemoteAuthenticator.Enabled
Note - I'll also embed the type here as the complete configuration is easy to understand without jumping over the types.

👍 as discussed

nopcoder · 2023-02-26T10:45:29Z

pkg/config/config.go

-	UsernameAttribute string `mapstructure:"username_attribute"`
-	UserBaseDN        string `mapstructure:"user_base_dn"`
-	UserFilter        string `mapstructure:"user_filter"`
+// RemoteAuthenticator holds remote authentication configuration.


Just note that in the release notes of this release we should guide ppl to update the configuration as lakeFS will not run with known configuration properties - and we are removing LDAP.
So starting lakeFS with the old configuration will break.

pkg/config/config.go

nopcoder · 2023-02-26T10:52:05Z

pkg/auth/remoteauthenticator/authenticator.go

+	client      *http.Client
+}
+
+func NewRemoteAuthenticator(conf *config.RemoteAuthenticator, authService auth.Service, logger logging.Logger) (auth.Authenticator, error) {


Return the concrete type and not the interface

Remove dependency in config as possible - create a structure with the same layout as your configuration and just pass it while calling NewRemoteAuthenticator from your main. Alternative: pass the relevant parameters to configuration struct that you define as part of this package. This is to decouple of use of config package from this one.

Return the concrete type and not the interface, updated but for clarification, WHY?

Done

nopcoder · 2023-02-26T11:22:17Z

pkg/auth/remoteauthenticator/authenticator.go

+}
+
+func (ra *RemoteAuthenticator) String() string {
+	return RemoteAuthSource


Why do we need this one?

I'm pretty sure I answered this and somehow my comment disappeared 😮
Anyway, it's used by chain auth when auth errors occurs.

yes, I missed your answer.

pkg/auth/remoteauthenticator/authenticator.go

nopcoder · 2023-02-26T11:32:58Z

pkg/auth/remoteauthenticator/authenticator.go

+	// if the external authentication service provided an external user identifier, use it as the username
+
+	if res.ExternalUserIdentifier != nil && *res.ExternalUserIdentifier != "" {
+		log = ra.Logger.WithField("external_user_identifier", *res.ExternalUserIdentifier)


Use log and not ra as you like to add the extra information and not re-write

Good catch thanks!

nopcoder · 2023-02-26T11:35:15Z

pkg/auth/remoteauthenticator/authenticator.go

+	// if the external authentication service provided an external user identifier, use it as the username
+
+	if res.ExternalUserIdentifier != nil && *res.ExternalUserIdentifier != "" {
+		log = ra.Logger.WithField("external_user_identifier", *res.ExternalUserIdentifier)
+		dbUsername = *res.ExternalUserIdentifier
+	}


Suggested change

// if the external authentication service provided an external user identifier, use it as the username

if res.ExternalUserIdentifier != nil && *res.ExternalUserIdentifier != "" {

log = ra.Logger.WithField("external_user_identifier", *res.ExternalUserIdentifier)

dbUsername = *res.ExternalUserIdentifier

}

// if the external authentication service provided an external user identifier, use it as the username

externalUserIdentifier := swag.String(res.ExternalUserIdentifier)

if externalUserIdentifier != "" {

log = log.WithField("external_user_identifier", externalUserIdentifier)

dbUsername = externalUserIdentifier

}

👍 added (with swag.StringValue)

nopcoder · 2023-02-26T11:36:23Z

pkg/auth/remoteauthenticator/authenticator.go

+const DefaultRequestTimeout = 7 * time.Second
+const RemoteAuthSource = "remote_authenticator"


use scope const

suggest to use https://github.com/mvdan/gofumpt

not sure I see the benefit of adding gofumpt, but changed scope

Its alternative to gofmt (command line) that will format your code just in strict way

pkg/auth/remoteauthenticator/authenticator.go

…d-deprecate-ldap

nopcoder · 2023-02-26T14:00:35Z

pkg/auth/remoteauthenticator/authenticator.go

+
+func NewRemoteAuthenticator(conf RemoteAuthenticatorConfig, authService auth.Service, logger logging.Logger) (*RemoteAuthenticator, error) {
+	if conf.Endpoint == "" {
+		return nil, fmt.Errorf("base URL is empty: %w", ErrBadConfig)


Endpoint is empty / missing

nopcoder · 2023-02-26T14:03:33Z

pkg/config/defaults.go

@@ -47,6 +47,10 @@ func setDefaults(local bool) {
 	viper.SetDefault("auth.ui_config.login_failed_message", "The credentials don't match.")
 	viper.SetDefault("auth.ui_config.login_cookie_names", "internal_auth_session")

+	viper.SetDefault("auth.remote_authenticator.enabled", false)


No need to set default to the flag's default

nopcoder · 2023-02-26T14:05:25Z

pkg/config/defaults.go

@@ -47,6 +47,10 @@ func setDefaults(local bool) {
 	viper.SetDefault("auth.ui_config.login_failed_message", "The credentials don't match.")
 	viper.SetDefault("auth.ui_config.login_cookie_names", "internal_auth_session")

+	viper.SetDefault("auth.remote_authenticator.enabled", false)
+	viper.SetDefault("auth.remote_authenticator.default_user_group", "Viewers")
+	viper.SetDefault("auth.remote_authenticator.request_timeout", 7*time.Second)


I prefer the to have a default to be 10 sec or 30 sec - something smaller than 'no timeout' but something that will work for many use-cases and in case the user will want to restrict the time for authenticate, it can be done by configuration.

Previously (In LDAP) was 7, changed to 10 👍

nopcoder

LGTM!

ortz

Great job!
I do have some comments, some small fixes and questions

docs/reference/configuration.md

pkg/config/defaults.go

ortz · 2023-02-26T18:31:21Z

pkg/config/defaults.go

@@ -47,6 +47,9 @@ func setDefaults(local bool) {
 	viper.SetDefault("auth.ui_config.login_failed_message", "The credentials don't match.")
 	viper.SetDefault("auth.ui_config.login_cookie_names", "internal_auth_session")

+	viper.SetDefault("auth.remote_authenticator.default_user_group", "Viewers")


note that lakefs' default group is Developers, I think that we should be aligned with the non-remote authenticator configuration.

pkg/config/config.go

initial commit

a3f327e

Isan-Rivkin added include-changelog PR description should be included in next release changelog team/cloud-native Team cloud native labels Feb 19, 2023

Isan-Rivkin self-assigned this Feb 19, 2023

Isan-Rivkin linked an issue Feb 19, 2023 that may be closed by this pull request

Add external authenticator support and deprecate LDAP #5262

Closed

Isan-Rivkin changed the title ~~External Authenticator Service~~ WIP: External Authenticator Service Feb 19, 2023

Isan-Rivkin added 4 commits February 20, 2023 14:34

remote auth

914c81e

pre-dry-run

82dae02

Merge branch 'master' into 5262-add-external-authenticator-support-an…

d75e007

…d-deprecate-ldap

update docs

176f82e

Isan-Rivkin requested review from arielshaqed and ortz February 20, 2023 13:13

working conf

ef9657e

Isan-Rivkin requested a review from nopcoder February 20, 2023 16:59

arielshaqed requested changes Feb 20, 2023

View reviewed changes

Isan-Rivkin added 2 commits February 21, 2023 10:59

Merge branch 'master' into 5262-add-external-authenticator-support-an…

deb4f73

…d-deprecate-ldap

Merge branch 'master' into 5262-add-external-authenticator-support-an…

2815508

…d-deprecate-ldap

nopcoder requested changes Feb 22, 2023

View reviewed changes

Isan-Rivkin added 4 commits February 23, 2023 12:08

Merge branch 'master' into 5262-add-external-authenticator-support-an…

6c285e9

…d-deprecate-ldap

remove LDAP

7526e02

fixed review comments

0ec18b8

remove comment

103ee9d

Isan-Rivkin changed the title ~~WIP: External Authenticator Service~~ Remote Authenticator Service & LDAP Removal Feb 23, 2023

refactor linter

591502e

Isan-Rivkin requested review from arielshaqed and nopcoder February 23, 2023 13:23

ortz suggested changes Feb 23, 2023

View reviewed changes

Isan-Rivkin requested a review from ortz February 23, 2023 14:24

arielshaqed approved these changes Feb 26, 2023

View reviewed changes

Isan-Rivkin added 3 commits February 26, 2023 11:31

Merge branch 'master' into 5262-add-external-authenticator-support-an…

3a2df91

…d-deprecate-ldap

fix review comments

8d73255

docs

6dbb197

nopcoder requested changes Feb 26, 2023

View reviewed changes

Isan-Rivkin added 6 commits February 26, 2023 14:12

Merge branch 'master' into 5262-add-external-authenticator-support-an…

ff845c5

…d-deprecate-ldap

fixed comments for review

114b884

white space

e970a5b

scope const

408232e

return struct

9aa3b9e

fix linter

2ef4fee

Isan-Rivkin requested a review from nopcoder February 26, 2023 13:33

Isan-Rivkin added 2 commits February 26, 2023 15:56

fix reviews

be2080b

change structs

b1221f2

nopcoder requested changes Feb 26, 2023

View reviewed changes

fix review defaults

920db06

Isan-Rivkin requested a review from nopcoder February 26, 2023 14:08

nopcoder approved these changes Feb 26, 2023

View reviewed changes

ortz approved these changes Feb 26, 2023

View reviewed changes

review docs comments

746ee5c

Isan-Rivkin merged commit 529d1a7 into master Feb 27, 2023

Isan-Rivkin deleted the 5262-add-external-authenticator-support-and-deprecate-ldap branch February 27, 2023 13:55

This was referenced Mar 6, 2023

Update/clarify S3 gateway config notes #5390

Merged

[docs] Add a little more detail to RBAC policy logic #5395

Closed

	return "", fmt.Errorf("doing http request %s: %w", username, err)
	return "", fmt.Errorf("remote authenticate %s: %w", username, err)


		lakeFS server supports external authentication, the feature can be configured by providing an HTTP endpoint to an external authentication service. This integration can be especially useful if you already have an existing authentication system in place, as it allows you to reuse that system instead of maintaining a new one.

		TBD

		const DefaultRequestTimeout = 7 * time.Second
		const RemoteAuthSource = "remote_authenticator"

Remote Authenticator Service & LDAP Removal #5285

Remote Authenticator Service & LDAP Removal #5285

Conversation

Isan-Rivkin commented Feb 19, 2023 • edited Loading

Isan-Rivkin commented Feb 20, 2023

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

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

ortz 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

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

Isan-Rivkin Feb 26, 2023 • edited Loading

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

nopcoder left a comment

Choose a reason for hiding this comment

ortz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Feb 19, 2023 •

edited

Loading

Isan-Rivkin Feb 26, 2023 •

edited

Loading