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

Add skeleton of IDP interface for resolving friendly names #2391

Closed
wants to merge 8 commits into from

Conversation

evankanderson
Copy link
Member

@evankanderson evankanderson commented Feb 21, 2024

See discussion in #2326. The goal here is to enable the following:

  1. Preserve existing relations in OpenFGA and avoid needing a migration.
  2. Support minder project role grant -r viewer -u JAORMX (which should look up the "JAORMX" preferred_username in Keycloak (populated from GitHub), and then store the (stable) Keycloak ID in the OpenFGA grant. We don't want to store JAORMX, because he might change his account name later.
  3. Support listing JAORMX as the username coming back from ListRoleAssignments, so that we can read the output. We can also include the Keycloak userids in the RPC response, but no one will read them. 😁
  4. Add an IdentityProvider for KeyCloak + GitHub that will call getUsers with a filter and/or create a user and set their GitHub federated identity information
    • This will allow us to resolve users to KeyCloak identities when the role grant is made, rather than at a possible later date when GitHub identities have shifted.
  5. In the future, add an actions IdentityProvider for GitHub actions which would be distinct from KeyCloak + GitHub users in 2
    • The syntax for this would be something like minder project role grant -r editor -u actions/repo:stacklok/minder-rules-and-profiles:ref:refs/heads/main. (Yeah, action names are pretty awesome...)

To fit the interface into existing code, we'll need to replace:

  • ParseAndValidate (4 calls, handlers_token.go and 3x handlers_user.go)
  • Calls around authzClient (default_project.go, handlers_authz.go, handlers_user.go) which will need to use the canonical Identity.String() for Write, Check, and ProjectsForUser, and Identity.Human() for AssignmentsToProject.

Copy link
Contributor

@JAORMX JAORMX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This abstraction makes sense to me. Is the idea also to abstract the OIDC token validation into the implementations here?

// Provider is the identity provider that vended this identity. Note that
// UserID and HumanName are only unique within the context of a single
// identity provider.
Provider IdentityProvider
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add email to this? For some IdPs we wouldn't quite have a preferred user name and the email would be more relevant.

Copy link
Member Author

@evankanderson evankanderson Feb 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think email is the preferred username (HumanName) for some providers. For others (e.g. GitHub Actions) users may not have email addresses.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I think that's fine


// IdentityProvider provides an abstract interface for looking up identities
// in a remote identity provider.
type IdentityProvider interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is IdentityProvider assumed to be OIDC-compatible? if so, let's mark that in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we want to require it. I imagined this as being decoupled from specific identity provider implementations, so we'd still need something like OIDC (or x.509 client certs) to assert the user's identity. Once we have their identity, we can then go through this interface to map it to/from database representation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at zitadel.RelyingParty, that interface looks a lot bigger (and more OIDC-specific) than what I was looking for here, but I could be convinced to expand to include that if we find it generally useful; it looks like it covers a lot more OIDC functionality. (The URL method is admittedly a bit OIDC-specific at the moment.)

internal/auth/interface.go Outdated Show resolved Hide resolved
Copy link
Member Author

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got some email comments from Ria that didn't show up here, adding them and then I'm going to polish this a little tonight and finish tomorrow.

internal/auth/interface.go Outdated Show resolved Hide resolved

// IdentityProvider provides an abstract interface for looking up identities
// in a remote identity provider.
type IdentityProvider interface {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at zitadel.RelyingParty, that interface looks a lot bigger (and more OIDC-specific) than what I was looking for here, but I could be convinced to expand to include that if we find it generally useful; it looks like it covers a lot more OIDC functionality. (The URL method is admittedly a bit OIDC-specific at the moment.)

Copy link
Contributor

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

@github-actions github-actions bot added the Stale label Mar 31, 2024
Copy link
Contributor

github-actions bot commented May 3, 2024

This PR needs additional information before we can continue. It is now marked as stale because it has been open for 30 days with no activity. Please provide the necessary details to continue or it will be closed in 30 days.

@github-actions github-actions bot added the Stale label May 3, 2024
@evankanderson
Copy link
Member Author

We did this in #3155

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants