Add AWS remote auth login #7578

idanovo · 2024-03-19T15:35:00Z

Closes #7574

github-actions · 2024-03-19T15:36:03Z

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

_{🤖 By surge-preview}

github-actions · 2024-03-19T15:41:57Z

E2E Test Results - DynamoDB Local - Local Block Adapter

api/swagger.yml

Isan-Rivkin · 2024-03-20T12:55:12Z

api/authorization.yml

@@ -363,6 +366,15 @@ components:
          items:
            $ref: "#/components/schemas/ExternalPrincipal"

+    ExternalLoginInformation:


lakeFS should not be aware of the details of the identity token details fields or params.
it should pass it to the remote authentication services.
so in other words, it should be some generic object key/value in the body that will be passed to the remote authentication service as is.

The remote service on the other hand that does the login will seriallize this object and extract specific fields to create AWS request.

Isan-Rivkin · 2024-03-20T14:27:36Z

pkg/api/controller.go

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

+func (c *Controller) ExternalLogin(w http.ResponseWriter, r *http.Request, body apigen.ExternalLoginJSONRequestBody) {


let's be consistent:

Suggested change

func (c *Controller) ExternalLogin(w http.ResponseWriter, r *http.Request, body apigen.ExternalLoginJSONRequestBody) {

func (c *Controller) ExternalPrincipalLogin(w http.ResponseWriter, r *http.Request, body apigen.ExternalLoginJSONRequestBody) {

Isan-Rivkin · 2024-03-20T14:27:49Z

pkg/api/controller.go

+		return
+	}
+
+	c.LogAction(ctx, "external_login", r, "", "", "")


Suggested change

c.LogAction(ctx, "external_login", r, "", "", "")

c.LogAction(ctx, "external_principal_login", r, "", "", "")

Isan-Rivkin · 2024-03-20T14:29:32Z

pkg/auth/service.go

@@ -85,6 +85,7 @@ type ExternalPrincipalsService interface {
 	DeleteUserExternalPrincipal(ctx context.Context, userID, principalID string) error
 	GetExternalPrincipal(ctx context.Context, principalID string) (*model.ExternalPrincipal, error)
 	ListUserExternalPrincipals(ctx context.Context, userID string, params *model.PaginationParams) ([]*model.ExternalPrincipal, *model.Paginator, error)
+	ExternalLogin(ctx context.Context, externalLoginInfo map[string]interface{}) (string, error)


It needs to be a different authentication service.
The service doing the login is not RBAC / GIAM, it doesn't have reference to KV.
That's the part I wanted you to sync with @guy-har.

guy-har

Overall looks good, I have a few concerns regarding the service
IIUC the authService is currently in charge of authorization (that is, authorization and authentication may run on different endpoints).
Another thing I'm not sure about, is that currently lakeFS sometimes needs to validate claims (validate_id_token_claims in the configuration), are this claims required in this flow or not?

guy-har · 2024-03-21T07:43:45Z

api/authorization.yml

+  /auth/external/login:
+    post:


Shouldn't this be part of the authentication API?

It should.
I'll move it there once you merge your PR

Thanks! Sorry for blocking 😬

guy-har · 2024-03-21T08:10:42Z

pkg/api/controller.go

+	if c.handleAPIError(ctx, w, r, err) {
+		if errors.Is(err, ErrAuthenticatingRequest) {
+			writeResponse(w, r, http.StatusUnauthorized, http.StatusText(http.StatusUnauthorized))
+			return
+		}


handleAPIError already writes the response, so you just need to return

It looks like the token generation happens only in case of error (brackets are in the wrong place I think)

…4-aws-remote-auth-login

pkg/api/controller.go

pkg/authentication/service.go

lakefs.yaml

pkg/api/controller.go

…4-aws-remote-auth-login

docs/reference/configuration.md

Isan-Rivkin · 2024-04-10T12:37:00Z

pkg/authentication/service.go

+	if err != nil {
+		return nil, fmt.Errorf("calling authenticate user: %w", err)
+	}
+	if resp.StatusCode() != http.StatusOK || resp.JSON200 == nil {


If resp.JSON200 nil it's a bug and should panic.

Suggested change

if resp.StatusCode() != http.StatusOK || resp.JSON200 == nil {

if resp.StatusCode() != http.StatusOK {

api/swagger.yml

docs/reference/configuration.md

Isan-Rivkin

LGTM! THANK YOU!

Fixed all the comments

Add AWS remote auth login

ca02c1c

idanovo added 5 commits March 19, 2024 17:53

Add a test

ce5b5e0

Fix

692916a

Fix logic

31f7761

Add relevant tags

60389b0

Fix

47ddb2b

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

idanovo linked an issue Mar 20, 2024 that may be closed by this pull request

IAM Role - AWS Remote Authenticator login #7574

Closed

idanovo added 4 commits March 20, 2024 11:36

Fix

b09b303

Fix

ed4042f

Fix

bde0214

Fix

5920fcc

idanovo marked this pull request as ready for review March 20, 2024 10:13

idanovo requested review from Isan-Rivkin and guy-har March 20, 2024 10:14

Isan-Rivkin requested changes Mar 20, 2024

View reviewed changes

Fix PR comments

134ba63

idanovo requested a review from Isan-Rivkin March 20, 2024 13:43

Isan-Rivkin requested changes Mar 20, 2024

View reviewed changes

Fix

5cb4a8d

guy-har previously requested changes Mar 21, 2024

View reviewed changes

idanovo added 4 commits March 21, 2024 10:34

Fix external login API call

e91fe5d

merged from master

cab7bdd

Revert

462bee5

Fix test

0247f49

idanovo requested review from guy-har and Isan-Rivkin March 25, 2024 10:44

Return principal ID instead of user ID for ExternalPrincipalLogin

fb95a20

idanovo added 5 commits April 2, 2024 19:13

Fix PR comments

8fe9e61

Fix test

bbbe23e

Lint

9fbc1b0

Pull from master

7d1d27e

Fix

68422ae

idanovo requested a review from Isan-Rivkin April 3, 2024 09:54

idanovo added 2 commits April 7, 2024 13:19

Update docs

e8a2349

Merge branch 'master' of https://github.com/treeverse/lakeFS into 757…

bf86e73

…4-aws-remote-auth-login

Isan-Rivkin requested changes Apr 9, 2024

View reviewed changes

pkg/api/controller.go Show resolved Hide resolved

pkg/authentication/service.go Outdated Show resolved Hide resolved

pkg/authentication/service.go Outdated Show resolved Hide resolved

lakefs.yaml Outdated Show resolved Hide resolved

pkg/api/controller.go Show resolved Hide resolved

PR review

77f7bb3

idanovo requested a review from Isan-Rivkin April 9, 2024 15:36

Return external login status code

77b65c1

Isan-Rivkin requested changes Apr 10, 2024

View reviewed changes

pkg/api/controller.go Outdated Show resolved Hide resolved

Error handling

6bdbe62

idanovo requested a review from Isan-Rivkin April 10, 2024 11:22

Merge branch 'master' of https://github.com/treeverse/lakeFS into 757…

92f99af

…4-aws-remote-auth-login

Isan-Rivkin requested changes Apr 10, 2024

View reviewed changes

PR review

05a794a

Isan-Rivkin requested changes Apr 10, 2024

View reviewed changes

api/swagger.yml Show resolved Hide resolved

idanovo requested a review from Isan-Rivkin April 10, 2024 12:49

Docs update

1a2755f

Isan-Rivkin requested changes Apr 10, 2024

View reviewed changes

docs/reference/configuration.md Show resolved Hide resolved

Fix docs

1e5fa4c

idanovo requested a review from Isan-Rivkin April 10, 2024 13:00

Isan-Rivkin approved these changes Apr 10, 2024

View reviewed changes

idanovo enabled auto-merge (squash) April 10, 2024 13:10

idanovo merged commit 1e6647b into master Apr 10, 2024
37 checks passed

idanovo deleted the 7574-aws-remote-auth-login branch April 10, 2024 13:12

emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024

Add AWS remote auth login (treeverse#7578)

21c8be1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AWS remote auth login #7578

Add AWS remote auth login #7578

idanovo commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited

Isan-Rivkin Mar 20, 2024

Isan-Rivkin Mar 20, 2024

Isan-Rivkin Mar 20, 2024

Isan-Rivkin Mar 20, 2024

guy-har left a comment

guy-har Mar 21, 2024

idanovo Mar 21, 2024

guy-har Mar 21, 2024

guy-har Mar 21, 2024

Isan-Rivkin Apr 10, 2024

Isan-Rivkin left a comment

	func (c Controller) ExternalLogin(w http.ResponseWriter, r http.Request, body apigen.ExternalLoginJSONRequestBody) {
	func (c Controller) ExternalPrincipalLogin(w http.ResponseWriter, r http.Request, body apigen.ExternalLoginJSONRequestBody) {

	c.LogAction(ctx, "external_login", r, "", "", "")
	c.LogAction(ctx, "external_principal_login", r, "", "", "")

	if resp.StatusCode() != http.StatusOK \|\| resp.JSON200 == nil {
	if resp.StatusCode() != http.StatusOK {

Add AWS remote auth login #7578

Add AWS remote auth login #7578

Conversation

idanovo commented Mar 19, 2024 • edited

github-actions bot commented Mar 19, 2024 • edited

github-actions bot commented Mar 19, 2024 • edited

E2E Test Results - DynamoDB Local - Local Block Adapter

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

guy-har 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

Isan-Rivkin left a comment

Choose a reason for hiding this comment

idanovo commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited

github-actions bot commented Mar 19, 2024 •

edited