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

Initial implementation of IDP interface and Keycloak implementation #3155

Merged
merged 18 commits into from
Apr 26, 2024

Conversation

evankanderson
Copy link
Member

@evankanderson evankanderson commented Apr 24, 2024

Summary

Defines and implements an IDP interface and a Keycloak implementation of said interface which allows us to specify and look up users by their GitHub login aliases while recording their KeyCloak user IDs in the actual database. Manually tested with the following after logging in both evankanderson and stacklokdemo:

minder project role grant -r admin -s stacklokdemo

The data written to OpenFGA uses the Keycloak user IDs as user:<GUID>, but the API reports the github names.

As part of this change, I've introduced the use of OpenFeature with the GO Feature Flag library in module configuration (i.e. reading from a file). To activate, you'll need to add the following flags-config.yaml to your minder directory:

idp_resolver:
  variations:
    UUIDonly: false
    NiceNames: true
  defaultRule:
    variation: NiceNames

(There are many more fine-grained options, but this turns on the flag. The file is mounted into the Docker container, and the server will notice updates within a minute or two, so you can toggle the feature on and off live.)

Fixes #3151

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

I've added unit tests with decent coverage for the various new code. I've also put the feature behind a feature flag, because there's more work to be done to clean up and unify this interface.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@evankanderson
Copy link
Member Author

Note that the vast majority of this is checking in the keycloak OpenAPI YAML (11k lines) and the generated client code (5k lines). I chose to download and generate our own client because most of the generated libraries out there seemed to be old, and the published Keycloak OpenAPI docs seemed to be relatively new.

@evankanderson evankanderson changed the title Initial implementation of IDP interface and KeyCloak implementation Initial implementation of IDP interface and Keycloak implementation Apr 24, 2024
@coveralls
Copy link

coveralls commented Apr 24, 2024

Coverage Status

coverage: 50.528% (+0.4%) from 50.164%
when pulling 69f94dd on evankanderson:3151-merge-idp
into 7c6d8f4 on stacklok:main.

@evankanderson evankanderson force-pushed the 3151-merge-idp branch 2 times, most recently from 82fd831 to d1c6b95 Compare April 24, 2024 05:42
@@ -157,6 +157,7 @@ func TestEntityContextProjectInterceptor(t *testing.T) {
}

for _, tc := range testCases {
tc := tc
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as below - curious if you ran into a problem that required this to be reinserted.

@@ -103,6 +104,15 @@ var serveCmd = &cobra.Command{
return fmt.Errorf("unable to prepare authz client for run: %w", err)
}

kc, err := keycloak.NewKeyCloak("", cfg.Identity.Server)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the significance of the name argument to NewKeyCloak, and why is it left blank here?

Copy link
Member Author

Choose a reason for hiding this comment

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

From the definition of IdentityProvider:

	// String returns the name of the identity provider.  This should be a short
	// one-word string suitable for presentation.  As a special case, a _single_
	// provider may use the empty string as its name to act as a default / fallback
	// provider.

We're using KeyCloak as the default provider. At some point in the future, I'd like to be able to add various machine identities as providers which can be granted access to the system without needing to have accounts in our primary IDP. (For example, an AWS role, a GCP service account, or a GitHub Action -- these might be stored as user:gha/repo:octo-org/octo-repo:environment:prod as the user key in OpenFGA, for example.)

I'm not ready to commit to implementing one of these machine identity providers yet, but I wanted to put the foundations in place while I was doing this refactoring.

Currently, we only have the one default identity provider, which is Keycloak. This PR doesn't switch over all our Keycloak identity usage yet, only puts a backwards-compatible shim over the Keycloak + OpenFGA combination.

@dmjb
Copy link
Contributor

dmjb commented Apr 24, 2024

Changes LGTM aside from the small questions.

@evankanderson evankanderson marked this pull request as ready for review April 25, 2024 06:58
@evankanderson evankanderson requested a review from a team as a code owner April 25, 2024 06:58
@evankanderson
Copy link
Member Author

This is ready for review. Sorry it got a bit large, but hopefully it's not too painful to review. Writing the tests already helped me catch a couple bugs that weren't in the common path, so yay tests!

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.

I'm guessing we'll need a migration from our current role assignments to assignments using the new subjects, right?

internal/flags/flags.go Outdated Show resolved Hide resolved
@evankanderson
Copy link
Member Author

I'm guessing we'll need a migration from our current role assignments to assignments using the new subjects, right?

No, that's what the "" IDP is about -- by registering the current Keycloak as the default IDP, the user:<Keycloak ID> assignments in OpenFGA don't need to change. In the future, we can add something like aws/<account>:<role> or what-not and it will be stored in OpenFGA as user:aws/<underlying id>, but the default IDP is stored without a prefix so that we don't need to migrate.

JAORMX
JAORMX previously approved these changes Apr 25, 2024
@evankanderson
Copy link
Member Author

Okay, @dmjb, I implemented a fake for openfeature.IClient since I noticed that they did actually ship an interface as well as the concrete Client struct. This lets us using t.Parallel and is overall cleaner, at a cost of about 180 LOC.

JAORMX
JAORMX previously approved these changes Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce an interface for looking up IDP identities
4 participants