NIFI-9174: Adding AWS SecretsManager ParamValueProvider for Stateless

[gresockj]

Description of PR

Adds a ParameterValueProvider using AWS SecretsManager

To test:

aws secretsmanager create-secret --name "Context" --secret-string '{ "Param": "value" }'

Create a flow with a Parameter Context named "Context" and a Parameter named "Param". The flow could demonstrate the value by logging the parameter via LogAttribute. Save the flow definition and launch stateless using the following configuration:

nifi.stateless.parameter.provider.AWSSecretsManager.name=AWS Secrets Manager Provider
nifi.stateless.parameter.provider.AWSSecretsManager.type=org.apache.nifi.stateless.parameter.AwsSecretsManagerParameterValueProvider
nifi.stateless.parameter.provider.AWSSecretsManager.properties.aws-credentials-file=./conf/bootstrap-aws.conf

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

• Is there a JIRA ticket associated with this PR? Is it referenced
  in the commit message?

• Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

• Has your PR been rebased against the latest commit within the target branch (typically main)?

• Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

• Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
• Have you written or updated unit tests to verify your changes?
• Have you verified that the full build is successful on JDK 8?
• Have you verified that the full build is successful on JDK 11?
• If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
• If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
• If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
• If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

• Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

[exceptionfactory]

Thanks for working on this feature @gresockj, this will be a very useful integration option. At a high level, is the a reason for requiring secrets to be stored as JSON objects? It seems like requiring the JSON object wrapping around value introduces potential complexity when populating secrets from other tools. Why not just use secretString directly as the parameter value?

On closer inspection, I see that the HashiCorp Vault Parameter Value Provider does require the wrapping object, but perhaps that is something to revisit as well.

[gresockj]

Thanks for working on this feature @gresockj, this will be a very useful integration option. At a high level, is the a reason for requiring secrets to be stored as JSON objects? It seems like requiring the JSON object wrapping around value introduces potential complexity when populating secrets from other tools. Why not just use secretString directly as the parameter value?

On closer inspection, I see that the HashiCorp Vault Parameter Value Provider does require the wrapping object, but perhaps that is something to revisit as well.

Great question -- regarding AWS SecretsManager, it looks like I missed in AWS's documentation the possibility of setting a plain value instead of the "key/value pair" format that translates to JSON. I agree that the simple value is the best approach, and will make that change.

Regarding HashiCorp Vault, I believe this is the only way Secrets are represented in the K/V Secrets Engine (see https://learn.hashicorp.com/tutorials/vault/getting-started-first-secret). So when you put a secret like this:

vault kv put secret/hello foo=world

then the secret is represented as { "foo": "world" } in the API. However, if I find a way to avoid using JSON with Vault, I'll address that as well.

[exceptionfactory]

Thanks for the reply and reference to HashiCorp Vault documentation @gresockj. With that background, the HashiCorp Vault implementation makes sense as it stands.

Making the adjustments to support a simple string for AWS Secrets Manager sounds good, that should also eliminate the dependency on Jackson.

[exceptionfactory]

Thanks for adjusting the secret storage format @gresockj! I noted a few additional recommendations and questions regarding the credentials implementation.

[exceptionfactory]

@gresockj can you rebase this PR to incorporate the recent build improvements to address test failures?

[markap14]

Generally, the way that the parameters work in stateless, the context name should be considered optional. I.e., if the context name is "MyContext" and the parameter name is "MyParameter", we should return a value for "MyContext/MyParameter" if it exists, but if that doesn't exist, we should return the value for "MyParameter"

[gresockj]

Thanks for the additional context! I'll make that update. (no pun intended)

[markap14]

I added a secret in SecretManager named MyContext/MyParameter. I used an "Other type of secret" and I set the key to "MyContext/MyParameter" and the value to "Hello". I expected the parameter to resolve to Hello. Instead, it resolved to {"MyContext/MyParameter":"Hello"}. Am I using this wrong?

[gresockj]

Are you adding the secret as a plain text secret? If you use the AWS key/value interface, it creates the JSON you posted instead of simply "Hello". Basically, if you use the command line example in the PR, it should resolve to "Hello". I will also take a look at this again, in case it is a bug in the code, though.

[markap14]

Ahhh yes. I used the key/value interface. The README gives an example of using the CLI, but we should probably indicate in the README also that if you're going to use the AWS Console (i.e., the UI) you need to create the secret and use a type of "Other type of secrets (e.g. API key)" and then go to the plaintext tab, remove the JSON completely, and then just type the secret value that you'd like to use.

[markap14]

Might be worth mentioning here that whatever credentials are supplied must have permissions both to List Secrets and to retrieve the values of the secrets.

[markap14]

@gresockj sorry for the confusion. The context name will never be ignored. Rather, what I was suggesting is that it should be ignored if there's no secret name that uses it. For example, if the Context Name is MyContext and the parameter name is MyParam, then isParameterDefined should return true if there's a parameter named MyContext/MyParam OR if there's a parameter named MyParam. Similarly, when getParameterValue is called, if there's a secret named MyContext/MyParam, then its value should be returned. Otherwise, return the value for the MyParam secret. Does that make sense?

[gresockj]

Ah, I see -- it's fallback logic, rather than simply handling a null Context.

[markap14]

Yes, exactly. Because 90% of the time a stateless flow will only use a single Parameter Context. So we don't want to require that they use the name of the parameter context each time. But, if they have a secret named "Password" or something like that, we do want to support disambiguating that.

[markap14]

Actually @gresockj after playing with the AWS Secrets Manager a bit more, I'm wondering if we should actually go a slightly different route here. Rather than having the value of a secret be a 'plaintext' value, perhaps it makes more sense to use the key/value pairs that the UI is tailored to?

So then, instead of treating each secret as a separate parameter, we would treat each AWS Secret like a Parameter Context. And each parameter would then map to one of those key/value pairs. Then users can just go into AWS Secrets Manager, create a new Secret, and enter all of their parameters that they care about. Then the provider would be configured with a mapping of ParameterContext Name to Secret Name, with a default Secret Name to be used if there is no mapping.

For example, if my flow has ContextA and ContextB, I could configure the Provider so that ContextA maps to secret SecretA and ContextB maps to SecretB. Or I could configure it so that ContextA maps to SecretA and ContextB maps to SecretA. Or configure it so that ContextA maps to SecretA and not provide a mapping for ContextB, just specifying SecretA as the default secret name.

Thoughts on this approach?

[exceptionfactory]

Thoughts on this approach?

@markap14 Thanks for describing some options for implementation, particularly in relation to the AWS Secrets Manager web user interface. PR #5410 for NIFI-9221 provides similar capabilities for NiFi Sensitive Properties. Using a plain string is easier to handle in code since it avoids the need for JSON parsing, but it also looks like the AWS Secrets Manager UI encourages using a JSON object as the default representation for generic secret values.

Going with the JSON object approach provides the ability to store multiple keys and values in a single Secret as you described, which could be useful. On the other hand, requiring a JSON obejct representation would break use cases where the Secret is a simple string. Without getting too complicated, a potential hybrid approach might be to attempt JSON parsing, and otherwise return the plain string, at least in the case of the NiFi Sensitive Property Provider for NIFI-9221.

Either way, it would be helpful to have a consistent approach, even though these are different use cases.

[gresockj]

Thoughts on this approach?

@markap14 Thanks for describing some options for implementation, particularly in relation to the AWS Secrets Manager web user interface. PR #5410 for NIFI-9221 provides similar capabilities for NiFi Sensitive Properties. Using a plain string is easier to handle in code since it avoids the need for JSON parsing, but it also looks like the AWS Secrets Manager UI encourages using a JSON object as the default representation for generic secret values.

Going with the JSON object approach provides the ability to store multiple keys and values in a single Secret as you described, which could be useful. On the other hand, requiring a JSON obejct representation would break use cases where the Secret is a simple string. Without getting too complicated, a potential hybrid approach might be to attempt JSON parsing, and otherwise return the plain string, at least in the case of the NiFi Sensitive Property Provider for NIFI-9221.

Either way, it would be helpful to have a consistent approach, even though these are different use cases.

An additional benefit of the JSON approach is that it would store fewer secrets (less cost). In the case of the AWS Secrets Manager Sensitive Property Provider, in order to stay consistent we could map the ProtectedPropertyContext.contextName to the Secret name, and ProtectedPropertyContext.propertyName to the key within the secret.

As for allowing Parameter Contexts to be arbitrarily mapped to different Secret names, @markap14, I'm going to suggest we go for simplicity here and simply enforce that a Parameter Context represents exactly one Secret, and so its name would become the Secret name.

[markap14]

@gresockj @exceptionfactory thanks for the feedback. Yes, I agree there could be some benefit to also allowing for a 'plaintext' approach, but I also think we should choose an approach to start and say this is how it works. If a need arises to allow for plaintext, we can look into it then. I do not think we should attempt to parse JSON and if it fails, fallback to 'plaintext', though. Instead, it makes sense to me to either have two separate Provider implementations or to have a configuration option in the Provider that says how to handle the data. A really good thing to consider is for Google Cloud credentials, you generally will be given a JSON document that contains a Base64 encoded certificate. So we have a secret JSON document, which would cause a lot of confusion if we attempted to separate those into separate key/value pairs.

[markap14]

"If not provided, the default AWS credentials will be used." The problem here is that it is always provided, because it has a default value. Probably need to remove the default value.

[markap14]

If the given secret name is not a valid secret name, this throws an AWSSecretsManagerException. In that case, I think we need to catch the Exception and delegate to the default Secret or return null, but we should throw an Exception here.

[markap14]

Thanks for all the work here @gresockj ! And thanks for helping with the reviews @exceptionfactory. I've done a good bit of testing and everything is looking good to me. +1 will merge to main if you're good also @exceptionfactory.

[exceptionfactory]

The implementation looks good, thanks @gresockj! +1

As a follow-on effort, we should evaluate the dependency tree of aws-java-sdk-secretsmanager and look at aligning this version with the version used for the AWS Secrets Sensitive Property Provider. Given the testing and current reviews, it looks good to merge!