Add support for multiple keys per secret

[passuied]

Description

AWS Secrets manager secrets store currently doesn't support multiple keys per secret. This would be a very useful feature. We can make it backwards compatible by adding a new optional metadata value MultipleKeysPerSecret

Issue reference

#3707

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

• [x] Code compiles correctly
• [x] Created/updated tests
• [x] Extended the documentation / Created issue in the https://github.com/dapr/docs/ repo: dapr/docs#4592

[joebowbeer]

This looks like a reasonable approach.

The same approach can be used for AWS SSM ParameterStore?

Can this be applied to all secret store implementations that don't natively implement multiple values?

How performant is formatString on non-json values?

By the way, aws-sdk-go reaches end-of-support in a few months. #2155

[passuied]

@joebowbeer Yes we can totally apply this to Ssm parameter store but wanted to keep the PR small. I'm sure we can apply this elsewhere too.

Happy to contribute further to migrate AWS SDK. We heavily use the dynamodb state store and use other components integrating with AWS.

[passuied]

How performant is formatString on non-json values?

I don't have benchmarks but it should fail pretty fast if the first character is not a { or a ".

Also I would expect that consumers would only use this option if they know their secrets have multiple k/v. The structure of the secrets should be known ahead.

[joebowbeer]

On second look, it looks like this only supports a single level of string-valued keys. At least, I don't see tests for anything else.

Note however that arbitrarily nested JSON values are allowed, including values that are JSON objects and arrays. How can these values be extracted?

Since reviewing this, I've looked at the AWS provider for Secrets Store CSI Driver, which also supports extracting secrets from json-encoded values:

https://github.com/aws/secrets-store-csi-driver-provider-aws

In their solution, the user specifies which paths to extract using a JMES path:

jmesPath: This optional field specifies the specific key-value pairs to extract from a JSON-formatted secret. You can use this field to mount key-value pairs from a properly formatted secret value as individual secrets.

I don't think Dapr needs to be concerned with name aliasing and mapping, but it occurs to me that it should be able to handle arbitrarily nested JSON. I also think it should have a strategy for creating the keys for the nested values. For example, by combining the parts using a default separator such as '/' and allowing the user to specify a different separator such as '.'.

Another place to look for ideas is the AWS provider for External Secrets Operator:

https://external-secrets.io/latest/provider/aws-secrets-manager/#json-secret-values

[passuied]

Given that values can be arbitrarily nested JSON, the tests and docs should be extended to cover how nested values are extracted, including objects and arrays.

@joebowbeer the DAPR secretstores interface only supports 1 level and any sub level will be left as a string. I can definitely add more tests to reflect but I don't think we can do anything else to support nested JSON use cases.

[passuied]

@joebowbeer turns out your call was very important. given the unmarshalling to map[string]string, the setting would NOT work in case of nested values. I now have improved the handling and of course added some tests. Ready for re-review.

[passuied]

Is there a test for top-level array values? They are used with rotating secrets.

Wouldn't you use versionId and versionStage for that? I could add a test case for this if you know an array is a valid usecase... My expectation is that it would default to a single property with stringified json...

[joebowbeer]

Here's some test input from the docs:

https://docs.aws.amazon.com/secretsmanager/latest/userguide/reference_secret_json_structure.html#reference_secret_json_structure_AD

{
  "awsSeamlessDomainDirectoryId": "d-12345abc6e",
  "awsSeamlessDomainUsername": "<username>",
  "awsSeamlessDomainPassword": "<password>",
  "directoryServiceSecretVersion": 1,
  "schemaVersion": "1.0",
  "keytabArns": [
    "<ARN of child keytab secret 1>",
    "<ARN of child keytab secret 2>",
    "<ARN of child keytab secret 3>",
  ],
  "lastModifiedDateTime": "2021-07-19 17:06:58"
}

[passuied]

Here's some test input from the docs:

https://docs.aws.amazon.com/secretsmanager/latest/userguide/reference_secret_json_structure.html#reference_secret_json_structure_AD

{
  "awsSeamlessDomainDirectoryId": "d-12345abc6e",
  "awsSeamlessDomainUsername": "<username>",
  "awsSeamlessDomainPassword": "<password>",
  "directoryServiceSecretVersion": 1,
  "schemaVersion": "1.0",
  "keytabArns": [
    "<ARN of child keytab secret 1>",
    "<ARN of child keytab secret 2>",
    "<ARN of child keytab secret 3>",
  ],
  "lastModifiedDateTime": "2021-07-19 17:06:58"
}

So really the secret is still an object and not a collection. So the nested test case would cover it...

[passuied]

Make sure you request a review from one of the owners. I'm just trying to help.

As for me, I would like to see one of the AWS samples in the test data so that it's more clear what happens in a realistic sample with: numeric value (e.g., port numbers), nested array (see AD sample), and nested object (for completeness).

There are a lot samples on this page and I think the AD sample has the most variety.

https://docs.aws.amazon.com/secretsmanager/latest/userguide/reference_secret_json_structure.html

This is a suggestion, not a "requested change". I can't figure out how to dismiss or replace my previous review.

Thanks I appreciate the feedback. I've reached out to @cicoyle in parallel. Seems like there are challenges to cover the component-contrib PRs atm

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[passuied]

@cicoyle any next steps on this PR? I can update the branch but without reviews from maintainers it seems moot

[cicoyle]

Hey @passuied - mind fixing linter?

[cicoyle]

Thanks for the implementation PR - Can you open a PR for this to the dapr/docs repo adding the new optional field to this page?

[passuied]

@joebowbeer you seem to have some good AWS experience. Could you review again with the AWS SDK v2 implementation? Especially the x509 auth support... I cannot find a ton of doc online whether this approach is sound or not... Based on this doc, it seems like v2 may handle refresh and a lot of the complexity introduced in x509.go file but I'm not sure this is the case...

[joebowbeer]

@passuied I think the AWS v2 auth package needs to be thought through before using AWS v2 auth in secretsmanager.

Consider adding a separate package for AWS v2 auth.

https://github.com/dapr/components-contrib/blob/main/common/authentication/aws2 ?

https://github.com/dapr/components-contrib/blob/main/common/authentication/aws/client2.go ?

This decision should be made before accruing more v2 logic in the existing client.

Currently the AWS v2 support in client is isolated to MSK.

[passuied]

Thanks @joebowbeer. Any thoughts on the x509 support in place? I had assumed that was what was involved in making auth work in EKS under a service account role but it doesn't seem to be connected... @yaron2 any guidance on needed support for aws sdk v2? Do we still need it? Should I be creating a separate x509 implementation for v2?

[joebowbeer]

@passuied I suggest reverting v2 changes.

When migrating to v2, all aws refs should be migrated. But at the moment, I see v2 code calling some aws functions.

It's better to add your feature first, and then migrate to v2, which has a much bigger blast radius.

I don't know why the x509 changes were made. I can't see how they are related to your feature or v2 migration.

[passuied]

@joebowbeer I took your advice and will be working on this separately. I still would like to be able to migrate these to v2 one component at a time to avoid a huge PR but I'd rather get this feature merged sooner. cc @yaron2 @mikeee

[mikeee]

https://github.com/dapr/components-contrib/pull/3856 V2 package PR - just investigating whether my changes to the snssqs component have caused the failures or the package which I doubt

[passuied]

just investigating whether my changes to the snssqs component have caused the failures or the package which I doubt

@mikeee not sure what you mean by failures here... Thanks for sharing the PR. I'll definitely refrain from working on v2 upgrade since you seem to be way ahead....

[passuied]

@mikeee if you can review and approve this PR I can create a subsequent one that will upgrade secrets manager to v2

[passuied]

@yaron2 @JoshVanL any chance to get this PR reviewed/approved?