Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API RoleSet #6759

Closed
klizhentas opened this issue May 6, 2021 · 9 comments
Closed

API RoleSet #6759

klizhentas opened this issue May 6, 2021 · 9 comments
Labels
feature-request Used for new features in Teleport, improvements to current should be #enhancements

Comments

@klizhentas
Copy link
Contributor

@Joerger @justinas @r0mant a couple of observations and lessons learned based on

https://github.com/gravitational/cloud/pull/616

  • It seems like any developer using AAP will need something like services.AccessChecker, so it should be really part of the api package with minimum dependencies.
  • The same applies to the role updater. API should provide a role watcher abstraction that sets up a watch and maintains the up-to-date cache of roles.

We need a higher level api.RoleSet that uses the client, watches the roles using watcher and keeps cache. We have all this machinery implemented in teleport core, but need to port it to simplify developer's integration efforts.

@klizhentas klizhentas added the feature-request Used for new features in Teleport, improvements to current should be #enhancements label May 6, 2021
@klizhentas klizhentas added this to the 7.0 "Stockholm" milestone May 6, 2021
@alex-kovoy
Copy link
Contributor

alex-kovoy commented May 6, 2021

Guys, because of 2 methods below, we are vendoring > 500 files. This also forces us to enable CGO flag to build stuff.

roleset = services.NewRoleSet(userRoles...)
matched = services.MatchLabels(role.GetAppLabels(types.Allow), myLabels)

@justinas @r0mant I wonder if there is something we can do now? For example, we can we create a package teleport/lib/services/roles and move the role.go stuff in there. You can create aliases to keep the scope change to minimum.

@justinas
Copy link
Contributor

justinas commented May 6, 2021

@justinas @r0mant I wonder if there is something we can do now? For example, we can we create a package teleport/lib/services/roles and move the role.go stuff in there. You can create aliases to keep the scope change to minimum.

As for CGO, for now I stubbed out the only module that requires it. This at least makes it build again https://github.com/gravitational/cloud/pull/616/commits/711ffced08b41e4485770ebd49737305a4f62517
This does not solve the huge dependency graph, of course.

I am not too fresh on workings of Go modules, but gravitational/teleport is a single module, so I'm not quite sure if we can easily pull in only specific parts of it.

@Joerger
Copy link
Contributor

Joerger commented May 7, 2021

I am not too fresh on workings of Go modules, but gravitational/teleport is a single module, so I'm not quite sure if we can easily pull in only specific parts of it.

When you import a go module, I think you indirectly import it's entire dependency graph. That's why we split off teleport/api into it's own module without any teleport dependencies.

It doesn't seem too difficult to detangle the roleset logic from lib/services, though I'm not sure how difficult the role watcher will be to implement. Where is this logic currently implemented, or would the role watcher be a completely new feature of the api?

Edit: it turns out that you only import/vendor packages that are actually used. You do however add the entire modules dependency graph (go.sum).

@awly
Copy link
Contributor

awly commented May 10, 2021

@justinas @klizhentas @alex-kovoy I'm missing some context here: why do you need to import Teleport's server-side authorization logic?

If this is about code reuse, does it make sense to create a new Go module with a reusable authorization engine? Or maybe it's more reasonable to copy-paste the few helper types/functions you use?

If this is about keeping the authz logic in sync with Teleport, can you elaborate your use case?

@alex-kovoy
Copy link
Contributor

@awly we are trying to use Teleport roles for managing access to the Sales Center (SC) https://github.com/gravitational/cloud/issues/594

SC will fetch Teleport roles and then use AccessChecker to see if a user has access to a SC resource.

The idea is to reuse Teleport code to work with the roles: we can revendor this code or make it part of Teleport Client.

@klizhentas Another option is to turn an AccessChecker to an API itself. This way the actual logic for resolving an access remains in 1 place.

@awly
Copy link
Contributor

awly commented May 11, 2021

thanks for the context @alex-kovoy
I recommend making AccessChecker into an API, to keep authz logic in the auth server.
That way you also won't get out of sync with any fixes we make.
Also, it would be a useful debugging tool, like kubectl auth can-i or https://cloud.google.com/iam/docs/testing-permissions.

Authz logic does not belong in the Teleport client module IMO, it's not useful for anyone building clients against the Teleport API.

@awly
Copy link
Contributor

awly commented May 12, 2021

Actually, @rueey seems to be working on something very similar to an AccessChecker API: https://github.com/gravitational/teleport/pull/6818/files

@klizhentas klizhentas removed this from the 7.0 "Stockholm" milestone May 12, 2021
@klizhentas
Copy link
Contributor Author

@awly @Joerger let's hold back on this for a while, there might be an easier way around: #6821

@zmb3
Copy link
Collaborator

zmb3 commented Apr 2, 2024

Closing due to inactivity.

@zmb3 zmb3 closed this as not planned Won't fix, can't repro, duplicate, stale Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Used for new features in Teleport, improvements to current should be #enhancements
Projects
None yet
Development

No branches or pull requests

6 participants