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

Adding roles #1632

Open
2 tasks done
mfrystacky opened this issue Jan 22, 2025 · 13 comments
Open
2 tasks done

Adding roles #1632

mfrystacky opened this issue Jan 22, 2025 · 13 comments
Labels
enhancement New feature or request

Comments

@mfrystacky
Copy link

Current Behavior

Roles currently do not exist in Hyades/Dependency Track. The existing permission model composes of individual permissions aggregated at a global level in the jwt claim and teams as a mode for having a select set of permissions against a set of projects for a set of users.

Proposed Behavior

Hi friends,
We are looking into adding roles into the alpine framework and into Hyades as an expansion and enrichment of the permission system.
I don't think anyone else is working on this or the permission system so we shouldn't be clobbering anyone's work in progress, but let me know if that isn't the case.
We have a new team, and they are exploring how to best approach this without making major revisions to the existing permission model, so we will be updating this as we are moving along and creating PR's of the incremental work moving forward.

Checklist

@mfrystacky mfrystacky added the enhancement New feature or request label Jan 22, 2025
@nscuro
Copy link
Member

nscuro commented Jan 24, 2025

Hey @mfrystacky, I think adding the concept of roles makes sense. I think teams were somewhat meant to implement RBAC-like capabilities, but of course this model doesn't scale that well if you're having loads of teams.

I don't think anyone else is working on this or the permission system so we shouldn't be clobbering anyone's work in progress, but let me know if that isn't the case.

There is stevespringett/Alpine#674 which aims to make it possible to assign permissions to individual API keys. But I doubt your proposed changes will have any overlap there, so you should be good.

We have a new team, and they are exploring how to best approach this without making major revisions to the existing permission model, so we will be updating this as we are moving along and creating PR's of the incremental work moving forward.

Awesome! Let us know if you need anything!

@jhoward-lm
Copy link

@nscuro For additional context, I wanted to lay out the ultimate goal we are trying to accomplish. I can create a separate issue if that would be more appropriate.

  • Use a GitLab instance as OIDC provider (already working)
  • Get the user's group memberships within GitLab and corresponding effective access levels (roles such as owner, maintainer, developer) for each of these groups using the GitLab API
    • This requires a combination of info from the /oauth/token and /oauth/userinfo endpoints that is currently not captured by the OidcAuthenticationService, and some of which is specific to GitLab
  • Add a DT project for each GitLab group (if one doesn't exist already)
  • Add a DT team mapping the OIDC group to user's effective access/role within GitLab, e.g. <gitlab project path>-maintainer (the sort of RBAC functionality we're looking for)

We are kicking around the idea of creating a GitLab integration within Hyades. Here is what we have so far (link), just don't pay too much attention to the current code within org.dependencytrack.integrations.gitlab. That was mostly me learning Java and getting familiar with the QueryManager, etc. Does this use case seem like something that an integration can fulfill?

One issue we're running into is that the privacy of OidcAuthenticationService and its members make it difficult to extend it without copy-pasting a bunch of code. Is exposing some of those private classes/interfaces as public something we could consider, or would it break the design philosophy?

/cc @pkwiatkowski1 @ashearin @Strakeln @lmphil @jmayer-lm @EphraimEM

@nscuro
Copy link
Member

nscuro commented Jan 28, 2025

Get the user's group memberships within GitLab and corresponding effective access levels (roles such as owner, maintainer, developer) for each of these groups using the GitLab API

  • This requires a combination of info from the /oauth/token and /oauth/userinfo endpoints that is currently not captured by the OidcAuthenticationService, and some of which is specific to GitLab

[...]

One issue we're running into is that the privacy of OidcAuthenticationService and its members make it difficult to extend it without copy-pasting a bunch of code. Is exposing some of those private classes/interfaces as public something we could consider, or would it break the design philosophy?

Depending on what exactly you need from OidcAuthenticationService, I would recommend to look into ServiceLoader. You could define a generic "customizer" or "claims extractor" interface in Alpine, and have OidcAuthenticationService discover implementations of it using ServiceLoader. Dependency-Track can then provide implementations of the interface to achieve the desired outcome. This keeps main logic in Dependency-Track.

Alpine has something similar for customization of the MeterRegistry already:

The main chunk of work is coming up with a good interface contract that perhaps allows for more than just GitLab to integrated in a similar fashion. Do you think this could work for your case?

Does this use case seem like something that an integration can fulfill?

An asynchronous task (or workflow in the future) should be able to achieve this.

However, IIUC, you need the users' GitLab access token when performing the synchronization?
In that case, what you could do is:

  1. Implement a customizer as mentioned above to acquire the additional data you need (i.e. role).
  2. In that customizer, kick off a GitLabSyncEvent.
    • Extend GitLabSyncEvent to take the user's GitLab access token.
    • Use DataEncryption to encrypt the token if the event has to be transmitted over the wire.
  3. Perform the synchronization logic in the task that handles GitLabSyncEvents.

For completeness, we also have a bare-bones plugin mechanism you could look into.

Example for pluggable file storage:

The intention is to eventually publish the plugin API to Maven Central, and load plugin JARs from the file system. But this could be expedited if you find it useful in this context. I figure this would be a viable option if your desired solution ends up being too specific to LM.

@jhoward-lm
Copy link

@nscuro Great info, thank you! We're exploring the customizer/claims extractor route now

@jhoward-lm
Copy link

The main chunk of work is coming up with a good interface contract that perhaps allows for more than just GitLab to integrated in a similar fashion. Do you think this could work for your case?

@nscuro Do you have any recommendations for what the type parameter to Consumer<T> should be? My initial thought was ClaimsSet but I'm not sure if that's correct

@FunctionalInterface
public interface OidcAuthenticationServiceCustomizer extends Consumer<ClaimsSet> {

}

or maybe just the AuthenticationService interface?

@FunctionalInterface
public interface OidcAuthenticationServiceCustomizer extends Consumer<AuthenticationService> {

}

Thanks for all the help and suggestions!

@nscuro
Copy link
Member

nscuro commented Jan 29, 2025

@jhoward-lm That really depends on what specifically you want to achieve. You are not limited to Consumer either. To give you a very rough idea:

interface OidcAuthenticationCustomizer {

    // Could be necessary if you have special needs for mapping claims
    // to an OidcProfile (i.e. to extract team membership).
    //
    // https://github.com/stevespringett/Alpine/blob/38cdda1eea6e1bed45ae7e26e305338cd5ca75be/alpine-server/src/main/java/alpine/server/auth/OidcAuthenticationService.java#L129-L136
    OidcProfile createProfile(ClaimsSet claimsSet);
    
    // This decision currently drives whether only the ID token,
    // or additionally the /userinfo endpoint is used to authenticate.
    //
    // The default implementation short-circuits if the profile is
    // considered complete based on the ID token alone.
    // https://github.com/stevespringett/Alpine/blob/master/alpine-server/src/main/java/alpine/server/auth/OidcAuthenticationService.java#L210-L214
    //
    // If you need to ALWAYS call the /userinfo endpoint,
    // you obviously need to modify the decision logic.
    bool isProfileComplete(OidcProfile profile, boolean teamSyncEnabled);

    // If the service used both the ID token and the /userinfo endpoint
    // to gather profile information, you need to decide what data to keep.
    //
    // https://github.com/stevespringett/Alpine/blob/38cdda1eea6e1bed45ae7e26e305338cd5ca75be/alpine-server/src/main/java/alpine/server/auth/OidcAuthenticationService.java#L216-L223
    OidcProfile mergeProfiles(OidcProfile left, OidcProfile right);

    // Invoked by OidcAuthenticationService when the authentication succeeded.
    // Here is where your main logic could be. Since the user is authenticated
    // at this point, it's safe to kick off a reconciliation job for them in the background.
    //
    // You could pass in the ID and access token as well.
    void onAuthenticationSuccess(OidcProfile profile);

    // Just for demonstration, might not be needed.
    void onAuthenticationFailure(OidcProfile profile, Exception cause);

    // For conflict resolution, see below.
    int priority();
    
}

You could replace OidcProfile in the above with the raw ClaimsSet as well. See what works best. If you go with OidcProfile, you may need to extend that class to hold additional, generic values, e.g. via Map<String, Object>.

To discover and load such a customizer, you could do it as follows. Note that there can be multiple implementations, and you'll have to choose only one. Again, this is just a rough sketch of how you could do it:

// in OidcAuthenticationService#authenticate

var customizer = ServiceLoader.load(OidcAuthenticationCustomizer.class).stream()
    .map(ServiceLoader.Provider::get)
    .filter(customizer -> {
        // Here's where you could further narrow down which implementation to use.
        // i.e. if the authentication is performed with a GitLab IdP, only choose
        // customizers meant to be used with GitLab.
    })
    // If you have multiple implementations still, order them by priority,
    // and select the first of them. For example, a default implementation provided
    // by Alpine should have a lower prio than a specialized one provided by DT.
    .sorted(Comparator.comparingInt(OidcAuthenticationCustomizer::priority))
    .findFirst()
    .orElseThrow(IllegalStateException::new);

// ...

if (customizer.isProfileComplete(idTokenProfile, teamSyncEnabled)) {

// ...

Alternatively, add a Config.Key that takes the fully qualified class name of the customizer. You then do:

String customizerClassName = Config.getInstance().getProperty(Config.AlpineKey.FOO);
var customizer = ServiceLoader.load(OidcAuthenticationCustomizer.class).stream()
    .filter(provider.type().getName().equals(customizerClassName))
    .map(ServiceLoader.Provider::get)
    .findFirst()
    .orElseThrow(IllegalStateException::new);

Users can then configure this via env vars, i.e. FOO=org.dependencytrack.auth.GitLabOidcAuthenticationCustomizer.

@jhoward-lm
Copy link

@nscuro This is way above and beyond the level of assistance we were hoping for, thank you so much!

If going with OidcProfile, its current access for the class is package-private and its members are either explicitly private or package-private. What are your thoughts on setting the access of the class to public and its getters/setters to either public or protected? That is in addition to adding the Map<String, Object> you suggested.

Going to explore this avenue now, thanks again!

@nscuro
Copy link
Member

nscuro commented Jan 29, 2025

@jhoward-lm Feel free to modify visibility or other internals as necessary.

@jhoward-lm
Copy link

@nscuro When you get a chance, would you mind taking a look at the changes I have so far? This more or less covers what we've talked about, but I wanted to get it in front of you early in case I misunderstood anything or it looks like there are too many touch points in the code. I tried to keep the formatter in my editor from taking liberties but there might be some whitespace changes in there; let me know if I need to revert.

A couple of highlights:

  • The methods removed from OidcAuthenticationService were moved into DefaultOidcAuthenticationCustomizer
  • OidcAuthenticationService.authenticateInternal became DefaultOidcAuthenticationCustomizer.onAuthenticationSuccess

Thanks!

@jhoward-lm
Copy link

@nscuro Or I could open a draft PR if you prefer

@nscuro
Copy link
Member

nscuro commented Jan 30, 2025

I did not have the chance to look at it yet, I'll try to give you feedback later today.

@nscuro
Copy link
Member

nscuro commented Jan 30, 2025

@jhoward-lm This looks quite promising already. But do you need to customize large chunks of the authentication logic itself?

Otherwise I'd suggest to not move authenticateInternal to the customizer. Same for autoProvision.

Wouldn't it suffice to simply invoke customizer.onAuthenticationSuccess here (and other locations where authentication completed successfully)?

Edit: Writing this made me realize that commenting on a PR would make this easier. If you could raise one for that purpose, that would be great.

@jhoward-lm
Copy link

@nscuro Those are good suggestions, I'll try to rework. Opened a draft PR with these changes. Thanks for all the help!

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

No branches or pull requests

3 participants