Skip to content

NIFI-16137 - Support externally-supplied client assertion in JWTBearerOAuth2AccessTokenProvider - #11463

Open
pvillard31 wants to merge 1 commit into
apache:mainfrom
pvillard31:NIFI-16137
Open

NIFI-16137 - Support externally-supplied client assertion in JWTBearerOAuth2AccessTokenProvider#11463
pvillard31 wants to merge 1 commit into
apache:mainfrom
pvillard31:NIFI-16137

Conversation

@pvillard31

Copy link
Copy Markdown
Contributor

Summary

NIFI-16137 - Support externally-supplied client assertion in JWTBearerOAuth2AccessTokenProvider

The idea is to support a scenario like:

External Identity Token Provider
        │ 
        ▼
JWTBearerOAuth2AccessTokenProvider 
        │  Token Endpoint = https://login.microsoftonline.com/<tenant>/oauth2/v2.0/token
        │  Client ID = Entra app's client ID
        │  Scope = api://<client-id>/.default   
        │  External Assertion Provider = ↑ references External Identity Token Provider
        ▼
AWSCredentialsProviderControllerService
        │  Assume Role ARN = the AWS IAM role trusting Entra
        │  Assume Role Session Name
        │  Assume Role STS Region
        │  OAuth2 Access Token Provider = ↑ references the JWTBearerOAuth2AccessTokenProvider
        ▼
ListS3 processor
        AWS Credentials Provider Service = ↑ references AWSCredentialsProviderControllerService

I tested the changes locally and can provide some screenshot if helpful.

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 contains commits signed with a registered key indicating Verified status

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 ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

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

@exceptionfactory exceptionfactory left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @pvillard31, this looks like a straightforward improvement. I noted a few minor implementation details, then this should be ready to go.


@Override
public String getValue() {
return displayName;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be changed to name()

return description;
}

public static Optional<AssertionStrategy> fromValue(final String value) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this method necessary?

Comment on lines +128 to +131
Determines how the RFC 7523 JWT assertion presented to the Token Endpoint is produced: either
built and signed locally using a Private Key Service, or supplied by an external
OAuth2AccessTokenProvider whose token is used directly as the assertion.
""")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The description should be shortened to avoid duplicating the descriptions of each value

JWSAlgorithm.Ed25519.getName())
.defaultValue(JWSAlgorithm.PS256.getName())
.required(true)
.dependsOn(ASSERTION_STRATEGY, AssertionStrategy.SELF_SIGNED.getValue())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It should be possible to remove getValue() from this and other references in dependOn

Comment on lines +399 to +400
final AssertionStrategy strategy = AssertionStrategy.fromValue(validationContext.getProperty(ASSERTION_STRATEGY).getValue())
.orElse(AssertionStrategy.SELF_SIGNED);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should use asAllowableValue()

Comment on lines +630 to +631
final AssertionStrategy strategy = AssertionStrategy.fromValue(context.getProperty(ASSERTION_STRATEGY).getValue())
.orElse(AssertionStrategy.SELF_SIGNED);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should use asAllowableValue() instead of the fromValue helper

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