[Feature] Integrate External Auth Principals management #7539

Isan-Rivkin · 2024-03-07T15:16:25Z

Change Description

Adding auth management endpoints for external principals management such as AWS IAM roles.

Background

As part of getting token in lakeFS from external principals such as IAM role we need to support for the CRUD operations around those attachments between an external principal and lakeFS user.

Testing Details

Added unit tests.

Breaking Change?

No breaking changes.
It's disabled by default.

Additional Info

More things that needs to be done before this feature is complete:

Docs
The actual login endpoint

github-actions · 2024-03-07T15:17:40Z

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

_{🤖 By surge-preview}

github-actions · 2024-03-07T15:23:04Z

E2E Test Results - DynamoDB Local - Local Block Adapter

idanovo

Add a few suggestions/questions.
Overall looks good.

api/swagger.yml

idanovo · 2024-03-07T15:24:24Z

api/swagger.yml

+    ExternalPrincipal:
+      type: object
+      required:
+        - users


Why users and not only user IDs?
Where do we use the other information?

Why users and not only user IDs?

Notice users is of type array[string], user IDs.

Where do we use the other information?

Other information is essentially config that will be digested by the remote authenticator.
For example, in AWS remote auth - when we create principal, this will contain flag indicating if we want to use the session name or not.
It's optional, to stay agile it's not specific to anything so that we don't need to break the API tomorrow.

idanovo · 2024-03-07T15:26:09Z

api/swagger.yml

@@ -2523,7 +2554,98 @@ paths:
          description: too many requests
        default:
          $ref: "#/components/responses/ServerError"
-
+  /auth/external/principals:


I'm not sure what this API returns.
a list of external principals configured for the current installation?

I used the exact pattern we use for groups, roles, users.
depends on the HTTP verb, one is a list the other will create attachment.

idanovo · 2024-03-07T15:27:33Z

api/swagger.yml

+    post: 
+      tags: 
+        - auth
+        - external
+      operationId: createExternalPrincipal
+      summary: Create principal as external identity connected to lakeFS user


I thought that external auth are configurable, not something the user need to make an API call to add

I'm not sure what you mean, perhaps a miss understanding?
As the summary suggests (maybe not that well?) it will create a specific principal.
i.e arn:123:role:abc -> user_a.

Co-authored-by: Idan Novogroder <43949240+idanovo@users.noreply.github.com>

api/swagger.yml

Isan-Rivkin · 2024-03-07T16:20:56Z

TODO:

list external principals -> user api
in externalidentity struct -> not users, just userId

api/swagger.yml

Isan-Rivkin · 2024-03-10T16:44:31Z

@idanovo please review
i need to add tests and to handle settings optional (see TODO comment in controller) but i want to first sort out the api and interface correctly it's related to both of us.

itaiad200 · 2024-03-11T07:53:12Z

docs/assets/js/swagger.yml

@@ -2523,6 +2589,104 @@ paths:
          description: too many requests
        default:
          $ref: "#/components/responses/ServerError"
+
+  /auth/external/principals:
+    get: 


It's either removing this API or making it useful by returning something more than the id

I think that getting the list of principals' IDs we have might be useful.
We should consider changing ExternalPrincipalList to return an actual list of ExternalPrincipals so we will get the user IDs too.

pkg/permissions/actions.go

idanovo

Looks good!
Minor comments

idanovo · 2024-03-14T09:26:49Z

pkg/api/controller.go

+
+func (c *Controller) CreateUserExternalPrincipal(w http.ResponseWriter, r *http.Request, body apigen.CreateUserExternalPrincipalJSONRequestBody, userID, principalID string) {
+	ctx := r.Context()
+	if c.Config.IsAuthUISimplified() || !c.Auth.IsExternalPrincipalsEnabled(ctx) {


Can you create a function for this? It's a reused code.

pkg/auth/service_test.go

idanovo · 2024-03-14T09:38:12Z

pkg/auth/service.go

+func (a *APIAuthService) IsExternalPrincipalsEnabled(ctx context.Context) bool {
+	return a.externalPrincipalseEnabled
+}
+func (a *APIAuthService) CreateUserExternalPrincipal(ctx context.Context, userID, principalID string) error {


I'm not sure if we need to check here user's existence.
WDYT?

I wouldn't want to check this here.

If you look at other requests such as AttachPolicyToUser and DetachPolicyFromUser when using APIAuthService (API service) we let that API decide on the correct authorization.
Especially around external principal implementation i'd rather let the auth service dictate the business logic since the implementation details belong to there.

idanovo

LGTM

itaiad200

Thanks for the PR, it's great to see the progress 💪
Mostly nit, a few blocking comments

itaiad200 · 2024-03-14T13:23:21Z

api/swagger.yml

+        user_id:
+          type: string
+          description: |
+            lakeFS user ID to associate with an external principal.


Email? something else?

I have no idea what's the question?

I think we have many id types for a user in Treeverse, right? I may be wrong..

itaiad200 · 2024-03-14T13:23:37Z

api/swagger.yml

+      properties:
+        id:
+          type: string
+          description: A unique identifier for the external principal


Can you add a string example?

itaiad200 · 2024-03-14T13:25:00Z

api/swagger.yml

-
-

I actually like the blank lines, it's very hard for me to udnerstand where an object ends without them

itaiad200 · 2024-03-14T13:26:09Z

api/swagger.yml

+      additionalProperties:
+        type: string
+      description: Additional settings to be consumed by the remote authenticator


I don't understand what this is. Is it Opaque? Can you add an example?

This is opaque on purpose to avoid breaking the API.
An example is forcing session_name or only matching based on role_name in IAM.
Another example would be specifying max ttl.

itaiad200 · 2024-03-14T13:26:47Z

api/swagger.yml

+    ExternalPrincipalCreation:
+      type: object
+      properties:
+        settings:
+          type: object
+          items:
+            $ref: "#/components/schemas/ExternalPrincipalSettings"


Creation has just a list of settings which are strings?

That's currently optional, ExternalPrincipalSettings is an object of key/value with string type.

pkg/auth/service.go

itaiad200

Thanks for the prompt fix! Single non blocking comment

itaiad200 · 2024-03-14T16:37:58Z

pkg/api/controller.go

+	c.LogAction(ctx, "create_user_external_principal", r, "", "", "")
+
+	err := c.Auth.CreateUserExternalPrincipal(ctx, userID, principalID)
+	if c.handleAPIError(ctx, w, r, err) {


Oh I see what happened here.
You check first thing for isExternalPrincipalNotSupported and then return not implemented. I thought we reach the AuthService that returns an error auth.ErrNotImplemented but I was wrong. When handleAPIError sees an error it doesn't recognize it will return Internal Server Error which is not what we want but also not the case here.

I believe it's slightly better to have the AuthService handle the possible ExternalPrincipals enabled/disabled status in terms of separation of concerns, i.e. it's not the controller responsibility, you have errors as communication protocol between the 2 components. BUT - it is also not a bug, and definitely not a blocking bug.

draft: principal api endpointsz

3aafcec

Isan-Rivkin requested a review from idanovo March 7, 2024 15:16

Isan-Rivkin self-assigned this Mar 7, 2024

Isan-Rivkin marked this pull request as draft March 7, 2024 15:16

idanovo requested changes Mar 7, 2024

View reviewed changes

Isan-Rivkin and others added 2 commits March 7, 2024 17:43

minor fix

92b9d7b

Update api/swagger.yml

44e3cd2

Co-authored-by: Idan Novogroder <43949240+idanovo@users.noreply.github.com>

itaiad200 reviewed Mar 7, 2024

View reviewed changes

api/swagger.yml Outdated Show resolved Hide resolved

idanovo added the exclude-changelog PR description should not be included in next release changelog label Mar 7, 2024

Isan-Rivkin added 3 commits March 7, 2024 17:48

update comments

40df335

update experimental

02953d0

update user not users

aade81b

update review

9cc77c8

Isan-Rivkin requested a review from idanovo March 10, 2024 09:57

idanovo reviewed Mar 10, 2024

View reviewed changes

api/swagger.yml Outdated Show resolved Hide resolved

idanovo reviewed Mar 10, 2024

View reviewed changes

api/swagger.yml Outdated Show resolved Hide resolved

update spsaces

9b2b076

Isan-Rivkin requested a review from idanovo March 10, 2024 10:21

Isan-Rivkin added 4 commits March 10, 2024 13:46

add initial rbac

d336de4

update perms

200caf3

update

b57a5ff

integration authenticator

5a3bb8b

itaiad200 reviewed Mar 11, 2024

View reviewed changes

Isan-Rivkin added 3 commits March 11, 2024 11:12

update base actions

2108e6d

update api public endpoints

a347036

update external api controller

4458308

Isan-Rivkin added 4 commits March 11, 2024 11:55

update

8bd869a

tests

c2103b8

remove test

8ecf161

fix unitest

2b690e2

idanovo requested changes Mar 14, 2024

View reviewed changes

Isan-Rivkin added 2 commits March 14, 2024 13:36

Merge branch 'master' into external-auth-principals-api

d98439b

update check for support

3ffb207

Isan-Rivkin requested review from idanovo and itaiad200 March 14, 2024 11:57

idanovo approved these changes Mar 14, 2024

View reviewed changes

Isan-Rivkin marked this pull request as ready for review March 14, 2024 12:15

itaiad200 requested changes Mar 14, 2024

View reviewed changes

update review comments

7600075

Isan-Rivkin requested a review from itaiad200 March 14, 2024 15:52

itaiad200 approved these changes Mar 14, 2024

View reviewed changes

Isan-Rivkin added include-changelog PR description should be included in next release changelog and removed exclude-changelog PR description should not be included in next release changelog labels Mar 17, 2024

Isan-Rivkin merged commit 34b172d into master Mar 17, 2024
41 checks passed

Isan-Rivkin deleted the external-auth-principals-api branch March 17, 2024 08:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Integrate External Auth Principals management #7539

[Feature] Integrate External Auth Principals management #7539

Isan-Rivkin commented Mar 7, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024

idanovo left a comment

idanovo Mar 7, 2024

Isan-Rivkin Mar 7, 2024

idanovo Mar 7, 2024

Isan-Rivkin Mar 7, 2024

idanovo Mar 7, 2024

Isan-Rivkin Mar 7, 2024

Isan-Rivkin commented Mar 7, 2024

Isan-Rivkin commented Mar 10, 2024

itaiad200 Mar 11, 2024

idanovo Mar 11, 2024

idanovo left a comment

idanovo Mar 14, 2024

idanovo Mar 14, 2024

Isan-Rivkin Mar 14, 2024 •

edited

Loading

idanovo left a comment

itaiad200 left a comment

itaiad200 Mar 14, 2024

Isan-Rivkin Mar 14, 2024

itaiad200 Mar 14, 2024

itaiad200 Mar 14, 2024

itaiad200 Mar 14, 2024

itaiad200 Mar 14, 2024

Isan-Rivkin Mar 14, 2024

itaiad200 Mar 14, 2024

Isan-Rivkin Mar 14, 2024

itaiad200 left a comment

itaiad200 Mar 14, 2024

[Feature] Integrate External Auth Principals management #7539

[Feature] Integrate External Auth Principals management #7539

Conversation

Isan-Rivkin commented Mar 7, 2024 • edited Loading

Change Description

Background

Testing Details

Breaking Change?

Additional Info

github-actions bot commented Mar 7, 2024 • edited Loading

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

Isan-Rivkin commented Mar 7, 2024

Isan-Rivkin commented Mar 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Isan-Rivkin Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

idanovo left a comment

Choose a reason for hiding this comment

itaiad200 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

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Mar 7, 2024 •

edited

Loading

github-actions bot commented Mar 7, 2024 •

edited

Loading

Isan-Rivkin Mar 14, 2024 •

edited

Loading