Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7571

guy-har · 2024-03-18T09:50:19Z

This change Adds an authentication service and implements the STS authentication flow
Broke it into commits to make the review a bit easier (the first 5 commits).
The rest of the commits are changes to make the makefile build the generated files in the right time and the right place

github-actions · 2024-03-18T15:58:08Z

♻️ PR Preview d46dbab has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

idanovo · 2024-03-19T12:29:10Z

pkg/authentication/service.go

+			return nil, fmt.Errorf("claim %s has unexpected value %s: %w", claim, claimValue, ErrInvalidSTS)
+		}
+	}
+	subject, found := claims.Get("sub")


"sub" should be const

still relevant 🙂

idanovo · 2024-03-19T12:29:18Z

pkg/authentication/service.go

+	if !found {
+		return nil, fmt.Errorf("missing subject in claims: %w", ErrInvalidSTS)
+	}
+	expiresAt, found := claims.Get("exp")


"exp" should be const

This on too

idanovo · 2024-03-19T12:31:36Z

pkg/authentication/service_test.go

+		name                 string
+		responseStatusCode   int
+		expectedErr          error
+		error                error


Suggested change

error error

loginError error

idanovo · 2024-03-19T12:35:49Z

cmd/lakefs/cmd/run.go

+		// initialize authorization service
 		var authService auth.Service


👍
I think we should rename also other places like the auth package name and authService variable for example
can be done in another PR

github-actions · 2024-03-20T14:58:34Z

E2E Test Results - DynamoDB Local - Local Block Adapter

idanovo

Add few minor comments
Welcome to merge after answering/resolving 🙂

idanovo · 2024-03-21T08:38:31Z

pkg/api/controller.go

@@ -554,6 +556,24 @@ func (c *Controller) Login(w http.ResponseWriter, r *http.Request, body apigen.L
 	writeResponse(w, r, http.StatusOK, response)
 }

+func (c *Controller) STSLogin(w http.ResponseWriter, r *http.Request, body apigen.STSLoginJSONRequestBody) {
+	ctx := r.Context()
+	responseData, err := c.Authentication.ValidateSTS(ctx, body.Code, body.RedirectUri, body.State)


I think we need to check here if external auth is enabled like we do with other calls

if c.isExternalPrincipalNotSupported(ctx) { writeError(w, r, http.StatusNotImplemented, "Not implemented") return }

idanovo · 2024-03-21T08:40:54Z

pkg/authentication/service.go

+			return nil, fmt.Errorf("claim %s has unexpected value %s: %w", claim, claimValue, ErrInvalidSTS)
+		}
+	}
+	subject, found := claims.Get("sub")


still relevant 🙂

idanovo · 2024-03-21T08:41:09Z

pkg/authentication/service.go

+	if !found {
+		return nil, fmt.Errorf("missing subject in claims: %w", ErrInvalidSTS)
+	}
+	expiresAt, found := claims.Get("exp")


This on too

Jonathan-Rosenberg · 2024-03-21T14:02:15Z

api/authentication.yml

+  description: fluffy HTTP API
+  title: fluffy API


You're fluffy, we're lakeFS 💪 !

Jonathan-Rosenberg · 2024-03-21T14:22:16Z

pkg/config/config.go

@@ -177,12 +177,16 @@ type Config struct {
 			SecretKey SecureString `mapstructure:"secret_key" validate:"required"`
 		} `mapstructure:"encrypt"`
 		API struct {
+			// Endpoint is the endpoint to used for authorization operations


Suggested change

// Endpoint is the endpoint to used for authorization operations

// Endpoint for authorization operations

Jonathan-Rosenberg · 2024-03-21T14:23:38Z

pkg/config/config.go

 			Endpoint           string        `mapstructure:"endpoint"`
 			Token              SecureString  `mapstructure:"token"`
 			SupportsInvites    bool          `mapstructure:"supports_invites"`
 			HealthCheckTimeout time.Duration `mapstructure:"health_check_timeout"`
 			SkipHealthCheck    bool          `mapstructure:"skip_health_check"`
 		} `mapstructure:"api"`
+		AuthenticationAPI struct {
+			Endpoint string `mapstructure:"endpoint"`


JUST AN IDEA:

Suggested change

Endpoint string `mapstructure:"endpoint"`

// Endpoint for authentication operations

Endpoint string `mapstructure:"endpoint"`

to make the difference clearer.

Jonathan-Rosenberg · 2024-03-21T14:42:29Z

pkg/config/config.go

@@ -524,6 +528,9 @@ func (c *Config) IsAuthUISimplified() bool {
 	return c.Auth.UIConfig.RBAC == AuthRBACSimplified
 }

+func (c *Config) IsAuthenticationTypeAPI() bool {
+	return c.Auth.AuthenticationAPI.Endpoint != ""
+}
 func (c *Config) IsAuthTypeAPI() bool {


should this be changed to IsAuthorizationTypeAPI?

Didn't touch the Auth as part of this PR, given the fact that it may include some breaking changes such as configuration names

Jonathan-Rosenberg

LGTM
Some minor comments, but the only blocking one is the authentication.yml one.
Thanks!!

Jonathan-Rosenberg

Sababa

guy-har force-pushed the feature/token-by-sts branch 5 times, most recently from a239dc4 to 20dc236 Compare March 18, 2024 15:48

guy-har added 5 commits March 18, 2024 17:53

Add authentication service

c204030

Add authentication service to controller

b5e2234

Change GenerateJWTLogin time parameters

9594d35

Add STS login endpoint

6362624

New generated clients and docs

ba2fb1a

guy-har force-pushed the feature/token-by-sts branch from 20dc236 to ba2fb1a Compare March 18, 2024 15:57

guy-har added 3 commits March 18, 2024 18:03

checks validator

d486ec3

Verfiy user exists and some name changing

b3680bf

Add generations and validations to Makefile

a361192

idanovo reviewed Mar 19, 2024

View reviewed changes

Add authentication swagger generation to gen-api

925d62f

guy-har force-pushed the feature/token-by-sts branch from af53e70 to 925d62f Compare March 20, 2024 09:23

guy-har linked an issue Mar 20, 2024 that may be closed by this pull request

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7582

Closed

guy-har added the include-changelog PR description should be included in next release changelog label Mar 20, 2024

guy-har changed the title ~~Add STS login flow~~ Integrate Support for Short-Lived Tokens (STS) in Remote Authentication Mar 20, 2024

guy-har marked this pull request as ready for review March 20, 2024 09:46

Use correct authentication service in local_load_test

3aa0ca4

guy-har force-pushed the feature/token-by-sts branch 3 times, most recently from 55808d5 to d95bbcb Compare March 20, 2024 13:23

Move mock to package mock

7d9b136

guy-har force-pushed the feature/token-by-sts branch 2 times, most recently from 4ead615 to b008dca Compare March 20, 2024 14:13

guy-har force-pushed the feature/token-by-sts branch 2 times, most recently from 4a1ecf5 to e4d2fb9 Compare March 20, 2024 14:49

add authentication generated code to generated tar

9c32b68

guy-har force-pushed the feature/token-by-sts branch from e4d2fb9 to 9c32b68 Compare March 20, 2024 15:06

Test if the generated esti workflow passes

902e15b

guy-har force-pushed the feature/token-by-sts branch from d54b233 to 902e15b Compare March 20, 2024 15:39

guy-har requested review from idanovo and Jonathan-Rosenberg March 21, 2024 07:32

idanovo approved these changes Mar 21, 2024

View reviewed changes

Restore cache check

ad9dd51

Jonathan-Rosenberg reviewed Mar 21, 2024

View reviewed changes

Jonathan-Rosenberg requested changes Mar 21, 2024

View reviewed changes

guy-har requested a review from Jonathan-Rosenberg March 24, 2024 08:03

Code review changes

d46dbab

guy-har force-pushed the feature/token-by-sts branch from 328614f to d46dbab Compare March 24, 2024 08:07

Jonathan-Rosenberg approved these changes Mar 24, 2024

View reviewed changes

guy-har merged commit 1babac4 into master Mar 24, 2024
37 checks passed

guy-har deleted the feature/token-by-sts branch March 24, 2024 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7571

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7571

guy-har commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading

idanovo Mar 19, 2024

idanovo Mar 21, 2024

Jonathan-Rosenberg Mar 21, 2024

idanovo Mar 19, 2024

idanovo Mar 21, 2024

idanovo Mar 19, 2024

idanovo Mar 19, 2024

github-actions bot commented Mar 20, 2024

idanovo left a comment

idanovo Mar 21, 2024

idanovo Mar 21, 2024

idanovo Mar 21, 2024

Jonathan-Rosenberg Mar 21, 2024

Jonathan-Rosenberg Mar 21, 2024

Jonathan-Rosenberg Mar 21, 2024

Jonathan-Rosenberg Mar 21, 2024

guy-har Mar 24, 2024

Jonathan-Rosenberg left a comment

Jonathan-Rosenberg left a comment

		// initialize authorization service
		var authService auth.Service

	// Endpoint is the endpoint to used for authorization operations
	// Endpoint for authorization operations

	Endpoint string `mapstructure:"endpoint"`
	// Endpoint for authentication operations
	Endpoint string `mapstructure:"endpoint"`

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7571

Integrate Support for Short-Lived Tokens (STS) in Remote Authentication #7571

Conversation

guy-har commented Mar 18, 2024 • edited Loading

github-actions bot commented Mar 18, 2024 • 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

github-actions bot commented Mar 20, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

idanovo 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

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

Jonathan-Rosenberg left a comment

Choose a reason for hiding this comment

guy-har commented Mar 18, 2024 •

edited

Loading

github-actions bot commented Mar 18, 2024 •

edited

Loading