NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets #9386

grishick · 2024-10-14T00:13:47Z

Summary

NIFI-12061
Add Secret Name parameter to AwsSecretsManagerParameterProvider to allow the provider to work without ListSecrets permission. Original behavior where the provider lists all secrets and matches them to Secret Name Pattern is preserved by leaving Secret Name parameter undefined. The new behavior will skip listing secrets and ignore Secret Name Pattern only if Secret Name is provided. I made this change in Anetac's Nifi fork months ago and we are using it in production.

Related PR to main branch: #9483

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-12061
Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-12061

Pull Request Formatting

Pull Request based on current revision of the support/nifi-1.x 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 1.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

EndzeitBegins · 2024-10-14T04:56:20Z

Thanks for your contribution @grishick.

Instead of one property overruling the other and SECRET_NAME_PATTERN remaining required even though it may not have any effect, what are your thoughts on introducing a "fetch mode" (or whatever name you see fit)?
With the current rather implicit approach, users might provide a SECRET_NAME_PATTERN of e.g. FOO_* and a SECRET_NAME of BAR simultaneously.
On first look it might not be obvious what the behaviour is; which property overrules which or are the results from both combined?

I'm not familiar with this component, but to me it reads like introducing a different "mode" of fetching secrets.
Personally, I'd think first having to select a "fetch mode" with allowed values "Secret by Name" and "All Secrets by Pattern" (or something along those lines) and only then have the corresponding properties available would be more intuitive.
Additionally, this keeps the door open for other "modes" in the future.

What are your thoughts on this?

Does it make sense to extend the existing documentation of SECRET_NAME_PATTERN to clarify that it's use required the permission to list secrets?

exceptionfactory

Thanks for the contribution @grishick, and thanks for the thoughtful feedback @EndzeitBegins.

I concur with the recommendation from @EndzeitBegins. Having an alternative retrieval strategy that avoids having to list all secret names seems useful, but it is important to make the configuration understandable.

With that in mind, a new property named something like Secret Listing Strategy with values of Pattern and Enumerated would be a good way forward. The default value would be Pattern, and the Secret Name Pattern property would depend on having set that strategy.

For the new property, listing only one Secret Name is rather limiting, so instead Secret Names, supporting one or more comma-separated values could be dependent on the Enumerated strategy.

grishick · 2024-10-14T19:09:53Z

Thanks for the contribution @grishick, and thanks for the thoughtful feedback @EndzeitBegins.

I concur with the recommendation from @EndzeitBegins. Having an alternative retrieval strategy that avoids having to list all secret names seems useful, but it is important to make the configuration understandable.

With that in mind, a new property named something like Secret Listing Strategy with values of Pattern and Enumerated would be a good way forward. The default value would be Pattern, and the Secret Name Pattern property would depend on having set that strategy.

For the new property, listing only one Secret Name is rather limiting, so instead Secret Names, supporting one or more comma-separated values could be dependent on the Enumerated strategy.

I got to agree, this is a better design. Will try to find time to rewrite it.

grishick · 2024-10-14T23:49:56Z

Thanks for the contribution @grishick, and thanks for the thoughtful feedback @EndzeitBegins.
I concur with the recommendation from @EndzeitBegins. Having an alternative retrieval strategy that avoids having to list all secret names seems useful, but it is important to make the configuration understandable.
With that in mind, a new property named something like Secret Listing Strategy with values of Pattern and Enumerated would be a good way forward. The default value would be Pattern, and the Secret Name Pattern property would depend on having set that strategy.
For the new property, listing only one Secret Name is rather limiting, so instead Secret Names, supporting one or more comma-separated values could be dependent on the Enumerated strategy.

I got to agree, this is a better design. Will try to find time to rewrite it.

I think with the addition of Secret Listing Strategy there is no need for additional Secret Names parameter. Instead, the original Secret Name Patter parameter will be used either as a coma separated list or as a regular expression depending on the listing strategy.

exceptionfactory · 2024-10-15T01:18:54Z

Thanks for the contribution @grishick, and thanks for the thoughtful feedback @EndzeitBegins.
I concur with the recommendation from @EndzeitBegins. Having an alternative retrieval strategy that avoids having to list all secret names seems useful, but it is important to make the configuration understandable.
With that in mind, a new property named something like Secret Listing Strategy with values of Pattern and Enumerated would be a good way forward. The default value would be Pattern, and the Secret Name Pattern property would depend on having set that strategy.
For the new property, listing only one Secret Name is rather limiting, so instead Secret Names, supporting one or more comma-separated values could be dependent on the Enumerated strategy.

I got to agree, this is a better design. Will try to find time to rewrite it.

I think with the addition of Secret Listing Strategy there is no need for additional Secret Names parameter. Instead, the original Secret Name Patter parameter will be used either as a coma separated list or as a regular expression depending on the listing strategy.

That could probably work, although it may involve changing the name and description of the property. With the dependsOn() builder method, the properties are shown or hidden based on the selected value, so in this case, it may be cleaner to have different properties to avoid any confusion.

grishick · 2024-10-15T01:47:02Z

Thanks for the contribution @grishick, and thanks for the thoughtful feedback @EndzeitBegins.
I concur with the recommendation from @EndzeitBegins. Having an alternative retrieval strategy that avoids having to list all secret names seems useful, but it is important to make the configuration understandable.
With that in mind, a new property named something like Secret Listing Strategy with values of Pattern and Enumerated would be a good way forward. The default value would be Pattern, and the Secret Name Pattern property would depend on having set that strategy.
For the new property, listing only one Secret Name is rather limiting, so instead Secret Names, supporting one or more comma-separated values could be dependent on the Enumerated strategy.

I got to agree, this is a better design. Will try to find time to rewrite it.

I think with the addition of Secret Listing Strategy there is no need for additional Secret Names parameter. Instead, the original Secret Name Patter parameter will be used either as a coma separated list or as a regular expression depending on the listing strategy.

That could probably work, although it may involve changing the name and description of the property. With the dependsOn() builder method, the properties are shown or hidden based on the selected value, so in this case, it may be cleaner to have different properties to avoid any confusion.

If I add Secret Names in addition to Secret Listing Strategy - would Secret Names and Secret Pattern have required set to true or to false?

exceptionfactory · 2024-10-15T02:00:01Z

If I add Secret Names in addition to Secret Listing Strategy - would Secret Names and Secret Pattern have required set to true or to false?

They can both be set to required(true), but the framework ensures that only the one associated with the related strategy value will be shown and required.

exceptionfactory

Thanks for the initial updates @grishick. Based on the new strategy property, adding a new Secret Names property looks like it would be cleaner and easier to understand than overloading the existing Secret Name Pattern property.

As with other pull requests, please also submit the pull request to the main branch first, and then it can be considered for backporting in a follow-on PR.

exceptionfactory · 2024-10-16T23:36:31Z

...roviders/src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java

+            .description("Strategy to use when listing secrets. 'Pattern' strategy treats Secret Name Pattern as a regular expression and fetches  " +
+                    "all secrets whose names match the pattern. 'Pattern' strategy requires ListSecrets and GetSecretValue permissions. "+
+                    "'Enumerated' strategy treats Secret Name Pattern as a coma-separated list and fetches all secrets whose names are in the list. " +
+                    "'Enumerated' strategy requires only GetSecretValue permission.")


Instead of describing the option in the property description, each value should be an instance of AllowableValue and include a description property.

I agree with David to move the description of the options to each option itself respectively.

An alternative approach to using AllowableValue is the declaration of an enum that implements the DescribedValue interface. An example can be found here.
Due to improved type safety at the use site, this approach allows for exhaustive switch expressions, as can be seen here and here for example.

The advantage I see in this approach is that the compiler enforces to handle all options on all use sites, especially as new options are added.

Regardless of the approach you take, moving the descriptions to the options is sensible.

exceptionfactory · 2024-10-16T23:38:05Z

...roviders/src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java

+                    "Any secrets whose names do not match this pattern will not be fetched. Using this parameter requires the ListSecrets permission." +
+                    "This parameter is ignored if the Secret Name parameter is set.")


To avoid confusion, I think it is clearer to keep this property description unchanged, and add a new property with the a corresponding description.

I added dependsOn to SECRET_NAME_PATTERN and added a new property called SECRET_NAMES also with dependsOn option. The new property SECRET_NAMES is expected to be a coma-separated list of AWS secret names. I've also added testFetchTwoSecrets unit test to verify the behavior when SECRET_NAMES is a list.

grishick · 2024-10-18T16:12:25Z

OK. I'll try to make the changes asap. It's a small change, but integrating it back to Anetac's fork is the tricky part.

EndzeitBegins

Thank you for your continued work on this @grishick.

There may have been a mistake due to the force-push, but some of the previous review feedback hasn't been addressed in the latest code version.
I've added some remarks and suggestions.

Additionally, as NiFi 2.0 is almost there, pull-requests should be addressing the main branch first before back porting changes to the support/nifi-1.x branch.

...roviders/src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java

EndzeitBegins · 2024-10-28T18:31:09Z

...roviders/src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java

+        // Fetch either by pattern or by enumerated list. See description of SECRET_LISTING_STRATEGY for more details.
+        String listingStrategy = context.getProperty(SECRET_LISTING_STRATEGY).getValue();
+        if (ENUMERATED_STRATEGY.equals(listingStrategy)) {
+            // if secret-name is set, fetch the secrets
+            String secretNames = context.getProperty(SECRET_NAMES).getValue();
+            for (String secretName : secretNames.split(",")) {
+                groups.addAll(fetchSecret(secretsManager, secretName));
            }
-            final String nextToken = listSecretsResult.getNextToken();
-            if (nextToken == null) {
-                break;
+        } else {
+            final Pattern secretNamePattern = Pattern.compile(context.getProperty(SECRET_NAME_PATTERN).getValue());
+            ListSecretsRequest listSecretsRequest = new ListSecretsRequest();
+            ListSecretsResult listSecretsResult = secretsManager.listSecrets(listSecretsRequest);
+            while(!listSecretsResult.getSecretList().isEmpty()) {
+                for (final SecretListEntry entry : listSecretsResult.getSecretList()) {
+                    String secretName = entry.getName();
+                    if (!secretNamePattern.matcher(secretName).matches()) {
+                        getLogger().debug("Secret [{}] does not match the secret name pattern {}", secretName, secretNamePattern);
+                        continue;
+                    }
+                    groups.addAll(fetchSecret(secretsManager, secretName));
+                }
+                final String nextToken = listSecretsResult.getNextToken();
+                if (nextToken == null) {
+                    break;
+                }
+                listSecretsRequest.setNextToken(nextToken);
+                listSecretsResult = secretsManager.listSecrets(listSecretsRequest);


You could untangle determining the set of secrets to fetch from the actual fetching in here, turning one intertwined action into two separate steps.

That is,

collecting a Set<String> of secretNames (based on either the provided enumeration or by fetching all available secret names and then filtering for those matching the regex), and then

using that set to fetch the corresponding secrets.

Good point. Done.

…src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java Co-authored-by: Lucas <[email protected]>

Co-authored-by: Lucas <[email protected]>

greg-anetac · 2024-11-03T22:22:08Z

Thank you for your continued work on this @grishick.

There may have been a mistake due to the force-push, but some of the previous review feedback hasn't been addressed in the latest code version. I've added some remarks and suggestions.

Additionally, as NiFi 2.0 is almost there, pull-requests should be addressing the main branch first before back porting changes to the support/nifi-1.x branch.

Roger that. I'll fix the merge. I was waiting to create a PR for main until discussions on this PR settle on implementation approach.

grishick · 2024-11-04T01:33:08Z

Thank you for your continued work on this @grishick.
There may have been a mistake due to the force-push, but some of the previous review feedback hasn't been addressed in the latest code version. I've added some remarks and suggestions.
Additionally, as NiFi 2.0 is almost there, pull-requests should be addressing the main branch first before back porting changes to the support/nifi-1.x branch.

Roger that. I'll fix the merge. I was waiting to create a PR for main until discussions on this PR settle on implementation approach.

done

grishick · 2024-11-04T18:20:58Z

Here is a PR for main branch with the same change: #9483

EndzeitBegins

Most of David's review comments on #9483 apply here too.

The only things that isn't available for 1.x that comes to my mind is asAllowableValue().
List.of isn't available for Java 8, so the PROPERTIES declaration can stay as is.

exceptionfactory requested changes Oct 14, 2024

View reviewed changes

exceptionfactory requested changes Oct 16, 2024

View reviewed changes

greg-anetac added 3 commits October 27, 2024 19:51

NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets

3c5b55c

NIFI-12061: allow coma-separated list of secrets

5aad384

NIFI-12061: allow coma-separated list of secrets

16a285b

grishick force-pushed the NIFI-12061 branch from aef0b34 to 16a285b Compare October 28, 2024 02:51

NIFI-12061: remove unused import

2ee3c4d

grishick requested a review from exceptionfactory October 28, 2024 06:41

EndzeitBegins requested changes Oct 28, 2024

View reviewed changes

grishick and others added 5 commits October 28, 2024 18:14

Update nifi-nar-bundles/nifi-aws-bundle/nifi-aws-parameter-providers/…

4551454

…src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java Co-authored-by: Lucas <[email protected]>

Update nifi-nar-bundles/nifi-aws-bundle/nifi-aws-parameter-providers/…

a99987e

…src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java Co-authored-by: Lucas <[email protected]>

Update nifi-nar-bundles/nifi-aws-bundle/nifi-aws-parameter-providers/…

495611f

…src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java Co-authored-by: Lucas <[email protected]>

Update nifi-nar-bundles/nifi-aws-bundle/nifi-aws-parameter-providers/…

36a8a06

…src/main/java/org/apache/nifi/parameter/aws/AwsSecretsManagerParameterProvider.java Co-authored-by: Lucas <[email protected]>

Apply suggestions from code review

a87dca5

Co-authored-by: Lucas <[email protected]>

NIFI-12061: use DescribedValue enum for listing strategy

0f925dc

grishick force-pushed the NIFI-12061 branch from a9f7370 to 0f925dc Compare November 4, 2024 00:38

grishick requested a review from EndzeitBegins November 4, 2024 01:32

grishick mentioned this pull request Nov 4, 2024

NIFI-12061: allow using AWS Secrets Manager without ListSecrets permi… #9483

Open

10 tasks

EndzeitBegins requested changes Nov 5, 2024

View reviewed changes

NIFI-12061: remove value member from ListingStrategy

94f5b7b

grishick requested a review from EndzeitBegins November 10, 2024 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets #9386

NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets #9386

grishick commented Oct 14, 2024 •

edited

Loading

EndzeitBegins commented Oct 14, 2024 •

edited

Loading

exceptionfactory left a comment

grishick commented Oct 14, 2024

grishick commented Oct 14, 2024

exceptionfactory commented Oct 15, 2024

grishick commented Oct 15, 2024

exceptionfactory commented Oct 15, 2024

exceptionfactory left a comment

exceptionfactory Oct 16, 2024

EndzeitBegins Oct 17, 2024

exceptionfactory Oct 16, 2024

grishick Oct 28, 2024

grishick commented Oct 18, 2024

EndzeitBegins left a comment

EndzeitBegins Oct 28, 2024 •

edited

Loading

grishick Nov 4, 2024

greg-anetac commented Nov 3, 2024

grishick commented Nov 4, 2024

grishick commented Nov 4, 2024

EndzeitBegins left a comment •

edited

Loading

		"Any secrets whose names do not match this pattern will not be fetched. Using this parameter requires the ListSecrets permission." +
		"This parameter is ignored if the Secret Name parameter is set.")

NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets #9386

Are you sure you want to change the base?

NIFI-12061: add SECRET_NAME parameter to avoid listing all secrets #9386

Conversation

grishick commented Oct 14, 2024 • edited Loading

Summary

Tracking

Issue Tracking

Pull Request Tracking

Pull Request Formatting

Verification

Build

Licensing

Documentation

EndzeitBegins commented Oct 14, 2024 • edited Loading

exceptionfactory left a comment

Choose a reason for hiding this comment

grishick commented Oct 14, 2024

grishick commented Oct 14, 2024

exceptionfactory commented Oct 15, 2024

grishick commented Oct 15, 2024

exceptionfactory commented Oct 15, 2024

exceptionfactory left a comment

Choose a reason for hiding this comment

exceptionfactory Oct 16, 2024

Choose a reason for hiding this comment

EndzeitBegins Oct 17, 2024

Choose a reason for hiding this comment

exceptionfactory Oct 16, 2024

Choose a reason for hiding this comment

grishick Oct 28, 2024

Choose a reason for hiding this comment

grishick commented Oct 18, 2024

EndzeitBegins left a comment

Choose a reason for hiding this comment

EndzeitBegins Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

grishick Nov 4, 2024

Choose a reason for hiding this comment

greg-anetac commented Nov 3, 2024

grishick commented Nov 4, 2024

grishick commented Nov 4, 2024

EndzeitBegins left a comment • edited Loading

Choose a reason for hiding this comment

grishick commented Oct 14, 2024 •

edited

Loading

EndzeitBegins commented Oct 14, 2024 •

edited

Loading

EndzeitBegins Oct 28, 2024 •

edited

Loading

EndzeitBegins left a comment •

edited

Loading