-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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-10831 enable JWT realm authentication with Elasticsearch #9605
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for introducing JWT support and describing the details @ChrisSamo632.
Of particular note, since the new properties depend on the JWT
scheme, they can be marked as required, which removes the need for custom validation and conditional checking.
On a related note, with the introduction of Run As User support here, does that remove one of the potential use cases for custom HTTP request headers in the related pull request?
.displayName("JWT Shared Secret") | ||
.description("JWT realm Shared Secret.") | ||
.dependsOn(AUTHORIZATION_SCHEME, AuthorizationScheme.JWT) | ||
.required(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be changed to required(true)
and the custom validation can be removed. Also recommend moving this directly under the OAuth2 Access Token Provider
property.
.required(false) | |
.required(true) |
<dependency> | ||
<groupId>commons-io</groupId> | ||
<artifactId>commons-io</artifactId> | ||
</dependency> | ||
<dependency> | ||
<groupId>org.apache.commons</groupId> | ||
<artifactId>commons-compress</artifactId> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these dependencies not used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IntelliJ reports that these dependencies are duplicated or not used here.
It looks like these are now inherited through the nifi-elasticsearch-bundle
, which itself extends the nifi-standard-shared-bom
, which provides these standard dependencies.
But this could be a mistake, I'm happy to add them back in if you think this might be an issue (I'm a little rusty on nested POMs) - things seem to be working OK when I run some tests though at least
private static final List<PropertyDescriptor> properties = List.of(HTTP_HOSTS, PATH_PREFIX, AUTHORIZATION_SCHEME, USERNAME, PASSWORD, | ||
API_KEY_ID, API_KEY, JWT_SHARED_SECRET, OAUTH2_ACCESS_TOKEN_PROVIDER, RUN_AS_USER, PROP_SSL_CONTEXT_SERVICE, PROXY_CONFIGURATION_SERVICE, | ||
CONNECT_TIMEOUT, SOCKET_TIMEOUT, CHARSET, SUPPRESS_NULLS, COMPRESSION, SEND_META_HEADER, STRICT_DEPRECATION, NODE_SELECTOR, | ||
SNIFF_CLUSTER_NODES, SNIFFER_INTERVAL, SNIFFER_REQUEST_TIMEOUT, SNIFF_ON_FAILURE, SNIFFER_FAILURE_DELAY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good opportunity to reformat these properties and list one per line for easier readability.
if (authorizationScheme == AuthorizationScheme.JWT) { | ||
if (oAuth2Provider == null) { | ||
results.add(new ValidationResult.Builder().subject(OAUTH2_ACCESS_TOKEN_PROVIDER.getName()).valid(false) | ||
.explanation(String.format("if '%s' is '%s' then '%s' must be set.", | ||
AUTHORIZATION_SCHEME.getDisplayName(), authorizationScheme.getDisplayName(), OAUTH2_ACCESS_TOKEN_PROVIDER.getDisplayName()) | ||
).build() | ||
); | ||
} | ||
if (!jwtSharedSecretSet) { | ||
results.add(new ValidationResult.Builder().subject(JWT_SHARED_SECRET.getName()).valid(false) | ||
.explanation(String.format("if '%s' is '%s' then '%s' must be set.", | ||
AUTHORIZATION_SCHEME.getDisplayName(), authorizationScheme.getDisplayName(), JWT_SHARED_SECRET.getDisplayName()) | ||
).build() | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be be removed given the use of dependent properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took the opportunity to update the BASIC
and API_KEY
auth types too - I think we can get rid of those custom validations now too (with the associated dependsOn
settings for those)
.displayName("OAuth2 Access Token Provider") | ||
.description("The OAuth2 Access Token Provider used to provide JWTs for Bearer Token Authorization with Elasticsearch.") | ||
.dependsOn(AUTHORIZATION_SCHEME, AuthorizationScheme.JWT) | ||
.required(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.required(false) | |
.required(true) |
6dc0dd2
to
78d2823
Compare
78d2823
to
39b16c3
Compare
Summary
NIFI-10831 enable JWT realm authentication with Elasticsearch.
Also allow for setting of
runas_user
in the Elasticsearch Controller Service, which may be of use to impersonate a user within Elasticsearch from NiFi, without having the user's credentials (but where the connecting NiFi user/token is granted permission to impersonate the alternate user).Other changes include:
Testing:
typ
ofat+jwt
, which Elasticsearch cannot currently handle (Supportat+jwt
Types in JWT Realm elastic/elasticsearch#119370)Client
setup to provide Access Tokens)Note that NiFi requires the IdP to be running over HTTPS (for which SmallStep CA was used to generate certificates locally).
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000
NIFI-00000
Pull Request Formatting
main
branchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
mvn clean install -P contrib-check
Licensing
[ ] New dependencies are compatible with the Apache License 2.0 according to the License Policy[ ] New dependencies are documented in applicableLICENSE
andNOTICE
filesDocumentation
[ ] Documentation formatting appears as expected in rendered files