NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl #6619

nandorsoma · 2022-11-04T14:21:45Z

Summary

NIFI-10760

Tracking

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

Issue Tracking

Apache NiFi Jira issue created

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 8
- JDK 11
- JDK 17

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

Thanks for the improvement @nandorsoma!

The general approach with new properties looks good, I noted several recommendations regarding naming and allowable values.

...ient-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java

...ient-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java

...ient-service-api/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientService.java

...ient-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java

ChrisSamo632 · 2022-11-05T16:46:12Z

...arch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java

+
+public enum AuthorizationScheme {
+    BASIC("Basic"),
+    API_KEY("API key");


Should there be an entry for connections using 2-way TLS (pki Realm in Elasticsearch)?

And a NONE option for unsecured Elasticsearch instances (not that we'd encourage that, but imagine there are plenty of people doing that, particularly in development environments)

It would make sense, but we can't add it because we need to provide backward compatibility. What would be the default value in that case? I guess NONE should be. It would cause confusion if someone upgrades to a newer version and Basic Authentication is set.

I'd probably stick with BASIC being the default as that would mean Username/Password remain available by default when adding the Processor to the canvas, which matches the current behaviour and therefore is backwards compatible.

If the user selects NONE then I guess we'd be hiding the existing Username/Password and new API Key property inputs (via dependsOn).

If the user selected PKI then it would be great if the SSLContext Controller become mandatory, but I don't think that's currently possible, so in effect this would have the same effect as NONE on the available processor properties (we don't want to hide SSL Context if something other than PKI is selected here, as https/TLS might very well still be needed even if it's not used for Auth in Elasticsearch).

One option might be to use some customerValidation for this though - if the user has selected PKI, they must also specify an SSL Context in their processor configuration. Similarly, if NONE is selected, then we could validate that none of the Username/Password/API Key fields are provided?

With the introduction of the new enum, supporting TLS as an Authorization Scheme sounds like something that could be handled in a subsequent issue.

...arch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java

ChrisSamo632 · 2022-11-05T17:01:13Z

...arch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java

+public enum AuthorizationScheme {
+    BASIC("Basic"),
+    API_KEY("API key");
+


Should jwt and kerberos Elasticsearch realms also be supported?

Happy for this to be raised as new follow-on jira ticket(s) for the future if too much to add now - might need some further consideration as these connection types would likely need to use regularly changing tokens rather than more static credentials set on the controller, so wee might need a way of getting that from the Processor or some other sort of token provider controller if/when they exist in future

It would make sense, but as you noted probably in a separate pr.

Are you happy to raise a new Jira ticket for those once this approach has been approved, as the new ticket could point to how this has been implemented?

I agree that a separate issue sounds good, as it would provide more opportunity to outline the implementation approach.

…rviceImpl

nandorsoma · 2022-11-06T22:42:40Z

need to force push because of the conflict

nandorsoma · 2022-11-07T02:22:46Z

Thank you for the reviews @exceptionfactory and @ChrisSamo632!
I tried to address your comments, please check my followup commits and comments.

exceptionfactory

Thanks for making the adjustments @nandorsoma, the current version looks good!

Do you have any additional feedback @ChrisSamo632? It sounds like there are some possibilities for follow-on improvements, but the current version looks ready.

ChrisSamo632 · 2022-11-07T20:49:39Z

Thanks for making the adjustments @nandorsoma, the current version looks good!

Do you have any additional feedback @ChrisSamo632? It sounds like there are some possibilities for follow-on improvements, but the current version looks ready.

If the remaining suggestions are captured as follow-on tickets (so we don't forget them), I've no objection to these change being Approved & merged

nandorsoma · 2022-11-08T11:33:20Z

Hey @exceptionfactory, @ChrisSamo632!
Thank you for the reviews.

Follow up Jiras (I didn't add NONE as a separate one, but mentioned it in the PKI one):
NIFI-10775 - Improve support for DescribedValue in PropertyDescriptor.Builder
NIFI-10776 - Add support for PKI Authentication in ElasticSearchClientServiceImpl
NIFI-10777 - Add support for JWT Authentication in ElasticSeachClientServiceImpl
NIFI-10778 - Add support for Kerberos Authentication in ElasticSearchClientServiceImpl

exceptionfactory · 2022-11-08T22:01:29Z

Thanks for the confirmation @ChrisSamo632!

Thanks for the work on this @nandorsoma, and for writing up the follow-on issues!

+1 merging

This closes apache#6619 Signed-off-by: David Handermann <exceptionfactory@apache.org>

wannadream

@nandorsoma could you take a look at my comment and help us?

wannadream · 2023-07-20T16:34:29Z

...ient-service/src/main/java/org/apache/nifi/elasticsearch/ElasticSearchClientServiceImpl.java

+        final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
+        final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));


Hi @nandorsoma , I ran into issues with Elastic Search API auth. Standard HTTP client works well for me, but it did not work with this client implementation. I looked into the code then found some questionable lines. What is API key id doing here? It is a required field, but should it be part of the token? And why does it encode the string once more as we already pass in base64 token string. Could you elaborate?

The id and key would be the values returned by Elasticsearch when you create an api key - NiFi requires both of these to construct the Authorization (ApiKey) header

From your description, it sounds like you are applying the encoded field from the Elasticsearch response (which is the Base64 encoded id & key) - NiFi doesn't currently use that in its approach - if you require that, you could raise a Jira to request it be added (submit a PR of your able) to make that an alternative to providing these existing fields

Thanks, Chris. Feeling enlightened.

exceptionfactory requested changes Nov 4, 2022

View reviewed changes

ChrisSamo632 reviewed Nov 5, 2022

View reviewed changes

...arch-client-service-api/src/main/java/org/apache/nifi/elasticsearch/AuthorizationScheme.java Outdated Show resolved Hide resolved

ChrisSamo632 reviewed Nov 5, 2022

View reviewed changes

nandorsoma added 2 commits November 6, 2022 20:36

NIFI-10760 Add Api key authentication option to ElasticSearchClientSe…

10baa72

…rviceImpl

NIFI-10760 Address review comments

32bd6f7

nandorsoma force-pushed the NIFI-10760 branch from 9b691dd to 32bd6f7 Compare November 6, 2022 22:43

NIFI-10760 Add tests

35a0170

exceptionfactory approved these changes Nov 7, 2022

View reviewed changes

exceptionfactory closed this in d374c1f Nov 8, 2022

ChrisSamo632 mentioned this pull request Nov 28, 2022

NIFI-10882 prevent setting of both BASIC and API_KEY Authorization Scheme properties in ElasticSearchClientService #6722

Closed

9 tasks

lizhizhou pushed a commit to lizhizhou/nifi that referenced this pull request Jan 2, 2023

NIFI-10760 Add API Key authentication to ElasticSearchClientServiceImpl

6d439f8

This closes apache#6619 Signed-off-by: David Handermann <exceptionfactory@apache.org>

wannadream reviewed Jul 20, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl #6619

NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl #6619

nandorsoma commented Nov 4, 2022

exceptionfactory left a comment

ChrisSamo632 Nov 5, 2022

ChrisSamo632 Nov 5, 2022

nandorsoma Nov 7, 2022

ChrisSamo632 Nov 7, 2022 •

edited

exceptionfactory Nov 7, 2022

ChrisSamo632 Nov 5, 2022

nandorsoma Nov 7, 2022

ChrisSamo632 Nov 7, 2022

exceptionfactory Nov 7, 2022

nandorsoma commented Nov 6, 2022

nandorsoma commented Nov 7, 2022

exceptionfactory left a comment

ChrisSamo632 commented Nov 7, 2022

nandorsoma commented Nov 8, 2022

exceptionfactory commented Nov 8, 2022

wannadream left a comment

wannadream Jul 20, 2023 •

edited

ChrisSamo632 Jul 20, 2023

wannadream Jul 20, 2023

		final String apiKeyCredentials = String.format("%s:%s", apiKeyId, apiKey);
		final String apiKeyAuth = Base64.getEncoder().encodeToString((apiKeyCredentials).getBytes(StandardCharsets.UTF_8));

NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl #6619

NIFI-10760 Add Api key authentication option to ElasticSearchClientServiceImpl #6619

Conversation

nandorsoma commented Nov 4, 2022

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

exceptionfactory left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisSamo632 Nov 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandorsoma commented Nov 6, 2022

nandorsoma commented Nov 7, 2022

exceptionfactory left a comment

Choose a reason for hiding this comment

ChrisSamo632 commented Nov 7, 2022

nandorsoma commented Nov 8, 2022

exceptionfactory commented Nov 8, 2022

wannadream left a comment

Choose a reason for hiding this comment

wannadream Jul 20, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisSamo632 Nov 7, 2022 •

edited

wannadream Jul 20, 2023 •

edited