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

NIFI-13016 Add groups mapping from OIDC token claim for Registry #9566

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

hazmat345
Copy link

@hazmat345 hazmat345 commented Dec 4, 2024

Summary

NIFI-13016

This PR adds support for respecting groups returned by an external OIDC token to nifi-registry.

History

The support was added to nifi proper in NIFI-7823. I've tried to make the implementation in nifi-registry as similar as possible. However, a major overhaul to OIDC was done as part of NIFI-4890, and those changes were not ported to nifi-registry. So I've tried to keep the changes as minimal as I can, which means that there will continue to be differences in the OIDC implementation between nifi and nifi-registry.

Questions / Thoughts

  • I'm using Spring SimpleGrantedAuthority because that's what the current implementation of nifi uses, but as far as I can tell there's no real reason to use it - I'm just packing the group name into it and pulling it out later. It'd probably be cleaner to just pass around the group names. EDIT: I made this change - now passing the group names as strings instead.
  • I haven't added any unit tests yet - as this is my first PR I'd like to get confirmation that the implementation direction makes sense before I add tests.

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
  • JDK 21

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this improvement @hazmat345.

As you noted, NiFi itself has significantly refactored the OIDC implementation using Spring Security, making it more difficult to implement additional functionality for Registry. The changes proposed seem reasonably scoped to consider on their own for now.

I noted a few things in detailed comments. The primary issue is duplicative parsing of the encoded JWT. That reflects some of the shortcomings of the current implementation for NiFi Registry. If you are able to evaluate and propose a refactored approach that avoids multiple rounds of token parsing to retrieve the necessary user and group claims, that should provide a way forward.

@hazmat345
Copy link
Author

hazmat345 commented Dec 11, 2024

@exceptionfactory thanks for the review! I believe I've addressed all of the feedback and this is ready for another round of review.

@hazmat345
Copy link
Author

@exceptionfactory would you mind giving this another look? Thanks!

@hazmat345 hazmat345 changed the title NIFI-13016 Added groups mapping from OIDC token claim for Registry NIFI-13016 Add groups mapping from OIDC token claim for Registry Jan 2, 2025
@exceptionfactory
Copy link
Contributor

Thanks @hazmat345, I plan to follow up on this soon.

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on this @hazmat345, it looks close to completion. I noted a few more minor adjustments related to null handling, and then this should be ready to go.

One additional change, the new property should be added to the OpenID Connect section of the Admin Guide for Registry.

hazmat345 and others added 6 commits January 17, 2025 16:58
…n/java/org/apache/nifi/registry/web/security/authentication/oidc/StandardOidcIdentityProvider.java

Co-authored-by: David Handermann <[email protected]>
…n/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java

Co-authored-by: David Handermann <[email protected]>
@hazmat345
Copy link
Author

@exceptionfactory thanks for the review! I think I've addressed everything but please let me know if there's anything else.

I realized that the property wasn't being added to nifi-registry.properties - I went ahead an added it but noticed there are a couple more properties that are listed in the admin guide but aren't in nifi-registry.properties file. Maybe a follow-up PR?

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @hazmat345, the property additions and documentation update look good.

Additional other properties could be addressed in a separate pull request.

I noticed one remaining place where the groups collection could be null when generating the application token.

@@ -155,6 +176,7 @@ public String generateSignedToken(String identity, String preferredUsername, Str
.audience().add(audience).and()
.claim(USERNAME_CLAIM, preferredUsername)
.claim(KEY_ID_CLAIM, key.getId())
.claim(GROUPS_CLAIM, groups)
Copy link
Contributor

Choose a reason for hiding this comment

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

This groups claim value can be null in some cases, such as authentication with LDAP. Instead of adding a groups claim with a null value, either an empty list should be set, or the claim should be omitted. As this is the JWT that NiFi Registry generates, it seems better to set an empty list when the input groups parameter is null.

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.

2 participants