feat: Add AWS Secrets Manager Store #267

danmactough · 2020-06-13T02:49:45Z

Implements #84 (currently closed as stale)

It sounds from #84 like there's still some interest in adding support for AWS Secrets Manager as a store, but previous efforts stalled. I started working on this implementation as a proof-of-concept for work -- at a previous job, we used Chamber and liked it a lot, but at my current job, we use Secrets Manager for secrets.

I'm not entirely sure about the design of this implementation, so I'm very open to feedback. Specifically:

All secrets for a "service" are stored in a single Secrets Manager secret. This reduces the number of AWS API calls for the most common use cases -- chamber exec and chamber env can be fulfilled with a single API call. This should eliminate the biggest pain point I've experienced with using Chamber -- getting rate-limited during a deployment. However, there's a greater risk of hitting the 4 KiB size limit per Secrets Manager secret.
I'm using STS to identify the user to keep the metadata on par with that available with the SSM store. Can we safely assume that STS is available in all use cases (i.e., whenever writing a secret)? Also, I'm using the user's ARN, which includes their "AssumedRole", but I'm not sure if that's equivalent to the metadata available with the SSM store.
I haven't implemented ListServices. I wasn't sure what to do about the fact that we have lots of Secrets Manager secrets already, none of which are "services" that I would want to list here. Should we suggest that the service start with a prefix, such as service/, and limit the ListServices results to secrets starting with that prefix?

I feel like I should also note that I don't write Go day-to-day, so please don't hesitate to point out things that are not conventional, poor implementation, etc. 😊

nickatsegment · 2020-06-18T17:44:30Z

All secrets for a "service" are stored in a single Secrets Manager secret. This reduces the number of AWS API calls for the most common use cases -- chamber exec and chamber env can be fulfilled with a single API call. This should eliminate the biggest pain point I've experienced with using Chamber -- getting rate-limited during a deployment. However, there's a greater risk of hitting the 4 KiB size limit per Secrets Manager secret.

This is sort of similar to the S3 backend's index file. It's just meant as a speed hack. BTW, the S3 backend was implemented for exactly this purpose -- to avoid rate limits.

I think this is a good approach, but it limits their compatibility with non-chamber tools, which has been complained about previously WRT case. Our stance has always been that we don't shoot for non-chamber compatibility, so it's fine with me.

I'm using STS to identify the user to keep the metadata on par with that available with the SSM store. Can we safely assume that STS is available in all use cases (i.e., whenever writing a secret)? Also, I'm using the user's ARN, which includes their "AssumedRole", but I'm not sure if that's equivalent to the metadata available with the SSM store.

Seems fine to me. I can't imagine a where someone would block GetCallerIdentity but allow writing to Secrets Manager.

I haven't implemented ListServices. I wasn't sure what to do about the fact that we have lots of Secrets Manager secrets already, none of which are "services" that I would want to list here. Should we suggest that the service start with a prefix, such as service/, and limit the ListServices results to secrets starting with that prefix?

I'm fine leaving it stubbed out for now. Personally I find limited utility in ListServices. A prefix might be nice, but it's your call. I'd say it should be chamber/. We're committing to a sort of storage format version here, and much like the v1/v2 paths/no paths migration, it can be bumpy switching, so pick a good choice :)

Anyway, this all looks pretty good to me. Thanks!

nickatsegment

Yep, you wrote tests and they seem to pass!

nickatsegment · 2020-06-18T17:46:51Z

I'm going to just get a second opinion from one of the others here, and then we're good to merge.

eculver · 2020-06-18T19:18:44Z

store/secretsmanagerstore.go

+		if len(value) == 0 {
+			return err
+		}
+		if err != ErrSecretNotFound {


when err == ErrSecretNotFound, shouldn't mustCreate be set to true?

Ah, no. This is a very tricky bit of code.

ErrSecretNotFound is the Chamber internal error, but mustCreate means that "we must create the underlying AWS Secrets Manager secret." Because this implementation doesn't have a 1:1 mapping of Chamber secret to AWS Secrets Manager secret, ErrSecretNotFound doesn't mean we need to create the underlying Secrets Manager secret. In fact, we will not encounter ErrSecretNotFound in this code path if the underlying AWS Secrets Manager secret needs to be created. If the AWS Secrets Manager secret does not exist, when we try to get it, we'll get the AWS error secretsmanager.ErrCodeResourceNotFoundException instead. That is the only case in which the underlying AWS Secrets Manager secret needs to be created.

Does this make sense? I'll add a code comment to explain this here.

Got it, thanks for the explanation.

Added a comment via 44c774e

eculver · 2020-06-18T19:42:10Z

store/secretsmanagerstore.go

+			SecretString:  aws.String(string(contents)),
+			VersionStages: []*string{aws.String("AWSCURRENT"), aws.String("CHAMBER" + string(version))},
+		}
+		_, err = s.svc.PutSecretValue(putSecretValueInput)


I'm curious if we should prevent updates to secrets that have RotationEnabled=true to prevent conflicts with secrets being managed by external Lambdas? Also, with regards to versions and stages, what's the rationale of introducing a CHAMBER+version stage? I'm wondering if it would make sense to just leave the Chamber label out to prevent complications in the future should Chamber ever want to utilize this for a rotation strategy of its own.

Also, from the docs on PutSecretValue:

If another version of this secret already exists, then this operation does not automatically move any staging labels other than those that you explicitly specify in the VersionStages parameter

I'm wondering if we should just omit VersionStages and let AWS set these automatically rather than introduce potential conflicts or errors?

I'm curious if we should prevent updates to secrets that have RotationEnabled=true to prevent conflicts with secrets being managed by external Lambdas?

@eculver I'm not sure any design that supports Chamber functionality could reasonably interop with the AWS-provided Lambdas for rotating secrets. To use Secrets Manager as a Chamber store, we need to manually manage the secret's metadata together with the secret's name and value. None of the AWS-provided Lambdas for rotating secrets are going to be aware of that metadata management (and honestly, just the structure of the secret value is probably a barrier to using those AWS-provided Lambdas for rotation). Instead, if a user wishes to use the Secrets Manager store and enable automatic rotation, they'll need to use Chamber in their rotation Lambda -- meaning we wouldn't be able to prevent updates to secrets that have RotationEnabled=true or those rotation Lambdas won't work. 🌀

what's the rationale of introducing a CHAMBER+version stage

This allows us to keep more than 2 versions of a secret. Without an additional staging label, all but the AWSCURRENT and AWSPREVIOUS versions of the secret are considered "deprecated" by Secrets Manager and subject to deletion. In my experience, these get deleted pretty darn quickly.

I'm not sure any design that supports Chamber functionality could reasonably interop with the AWS-provided Lambdas for rotating secrets.

This is exactly my point. Chamber can technically still operate on the secrets that are managed by Lambdas which would cause problems. I'm thinking that a simple check for if RotationEnabled=true to ensure that the lack of interoperability doesn't cause problems.

This is my only concern at this point. Everything else is looking great.

This is exactly my point. Chamber can technically still operate on the secrets that are managed by Lambdas which would cause problems. I'm thinking that a simple check for if RotationEnabled=true to ensure that the lack of interoperability doesn't cause problems.

@eculver I think I understand, but let me make sure. You're saying that if RotationEnabled=true we can read the secrets, but we may not write the secrets, correct? (And this would only be applicable if the secret already exists, of course.) This approach makes sense to me now that the data structure is read-interoperable.

One tricky bit is that we don't know value of RotationEnabled at this point in the code. 🤔

I think I understand, but let me make sure. You're saying that if RotationEnabled=true we can read the secrets, but we may not write the secrets, correct? (And this would only be applicable if the secret already exists, of course.) This approach makes sense to me now that the data structure is read-interoperable.

@eculver I went ahead and implemented this via 583cf39

danmactough · 2020-06-19T14:04:42Z

A prefix might be nice, but it's your call. I'd say it should be chamber/

@nickatsegment Agree that chamber/ would be a better prefix. I'm not inclined to flesh out ListServices at this time if you're ok with shipping without it.

nickatsegment · 2020-06-23T16:28:55Z

@danmactough WFM.

Just waiting on @eculver to confirm your rationale above.

danmactough · 2020-07-07T15:18:25Z

👋 @nickatsegment @eculver Any news on this?

eculver · 2020-07-08T16:40:34Z

Any news on this?

Thanks for following up. I just want to make a decision on the RotationEnabled=true detection as mentioned in the comment above. That's all we have before we get this sucker merged.

danmactough · 2020-07-19T17:05:09Z

Hey @eculver @nickatsegment FYI, I've been thinking about @eculver's comments, and I have a reworked implementation almost ready that I think will be more interoperable with rotation and other key features of Secrets Manager.

danmactough · 2020-07-20T03:57:23Z

Ok, here's that reworked implementation. Basically, we've revamped how we store values and metadata.

Previously, the structure stored value and metadata for each secret nested under the secret's name:

Secrets Manager was not able to convert this structure to key/value pairs:

Also, this structure would have required any Rotation Lambda to understand our bespoke structure to update the value of a secret.

The new structure is keeps all Chamber metadata under a single property while secrets are structured as key/value pairs.

Secrets Manager can understand this structure.

I'm still tinkering with this to see if I can get Chamber to read a non-Chamber secret from Secrets Manager, which I think would be desirable. That would mean that a user could leverage Chamber solely for the utility of chamber env and chamber exec while using Secrets Manager's rotation feature without needing to know about Chamber's implementation details.

danmactough · 2020-07-21T03:39:40Z

I'm still tinkering with this to see if I can get Chamber to read a non-Chamber secret from Secrets Manager, which I think would be desirable. That would mean that a user could leverage Chamber solely for the utility of chamber env and chamber exec while using Secrets Manager's rotation feature without needing to know about Chamber's implementation details.

Ok @eculver @nickatsegment, I think this is ready for another review. 😅

Now, in addition to the change in structure, I've made the following additional changes to increase interoperability with non-Chamber secrets in Secrets Manager:

slightly tweaked the behavior of the History command so that missing metadata is silently ignored
reworked how we unmarshal Secrets Manager JSON to handle non-string values, converting them where possible to strings

With these changes, you can create a new Secrets Manager secret using one of the "wizards" in the AWS console (such as "Credentials for RDS database") -- which will create a secret having a "port" property with a value that is a number instead of a string -- and Chamber can still read that secret. This means you can use chamber exec or chamber env with that secret even though one or more values are not strings, and even though the secret contains none of the metadata that Chamber needs for some of its other functionality (such as history and reading a specific version). You can also use chamber write to update these non-Chamber secrets.

The interoperability isn't 100% perfect, but it's pretty good! Nothing is "broken", at least by some definition of broken. 🙃

danmactough · 2020-07-31T17:19:57Z

👋 @eculver @nickatsegment Please let me know if there's anything I can do to help land this -- my team is excited about adopting chamber with Secrets Manager integration. 🤗

eculver

Few more questions.

eculver · 2020-08-03T16:32:09Z

store/secretsmanagerstore.go

+			s = strconv.FormatFloat(value.Float(), 'f', -1, 64)
+		case reflect.Bool:
+			s = strconv.FormatBool(value.Bool())
+		default:


does this also need to support int types?

Nope! No need to support int types: https://golang.org/pkg/encoding/json/#Unmarshal

eculver · 2020-08-03T16:34:15Z

store/secretsmanagerstore.go

+		if len(value) == 0 {
+			return err
+		}
+		if err != ErrSecretNotFound {


Got it, thanks for the explanation.

eculver · 2020-08-03T16:37:16Z

store/secretsmanagerstore.go

+
+	if len(value) == 0 {
+		if mustCreate {
+			return err


err will always be nil here no? I'm not sure I understand what's going on here, could you add a comment explaining the logic here? It seems like if there's a branch for len(value) == 0 it should be checked earlier unless I'm just not following something.

err will always be nil here no?

Ah, yes. Good catch.

Removed via 744c0fc

eculver · 2020-08-03T16:41:17Z

store/secretsmanagerstore.go

+// Delete removes a secret. Note this removes all versions of the secret. (True?)
+func (s *SecretsManagerStore) Delete(id SecretId) error {
+	// delegate to Write
+	return s.Write(id, "")


Is this the semantics we want for delete? There is a DeleteSecret action. I'm curious why that's not being used.

This is another one of those name collisions causing confusion. 😞 This delete method doesn't map to deleting the secret from Secrets Manager because the Secrets Manager secret may contain more than one Chamber secret. They intentionally don't map 1:1 (because one of the design goals on this store implementation is to reduce the number of API calls).

So, instead of deleting the Secrets Manager secret, we want to remove the Chamber secret from the Secrets Manager secret. Make sense?

eculver · 2020-08-03T17:49:10Z

store/secretsmanagerstore_test.go

+		assert.Equal(t, "true", obj["isPhony"])
+		assert.Equal(t, "", obj["empty"])
+		assert.Equal(t, "", obj["nested"])
+		assert.Equal(t, "", obj["array"])


Is this expected? Nested values are ignored?

This is intentional because ultimately we need to distill everything to name/value pairs of environment variables but we don't have a rule for translating nested values. I could definitely see adding that as an enhancement (for example, this nested object could be expanded to NESTED__FOO=bar or something).

danmactough · 2020-08-12T15:25:06Z

I want to highlight this commit I just pushed cbd2cc2 It changes the behavior a bit, but I think for the better.

Currently, all keys get converted to lowercase on write and then converted to UPPERCASE when using the env, exec and export commands. I'm not sure why it was desirable to convert to lowercase instead of UPPERCASE on write, but I can imagine that in the case of SSM, it's more aesthetically pleasing.

When using Secrets Manager as a backend, however, it's very surprising (and annoying) to have the keys converted to lowercase because when using the AWS SDK to fetch the secret where Chamber is not available (such as in a NodeJS Lambda function), it leads to this split brain for the developer -- when I use this secret one way (with chamber exec), the keys are UPPERCASE but when I use this secret any other way, the keys are lowercase. Concretely, I was testing out the workflow of using Chamber to manage my secret and then using it in a NodeJS Lambda and did the following:

Then I realized that if I wanted to be able to refer to secrets on process.env the same way I do when I use Chamber, I'd have to do this:

It seems wrong for my Lambda to have to know about implementation details of Chamber.

I think the change in cbd2cc2 -- in which the secretsmanagerstore converts the keys to UPPERCASE -- resolves this confusion.

Happy to discuss.

stale · 2020-10-11T22:46:00Z

This issue has been automatically marked stale because it has not had any activity in the last 60 days. If no further activity occurs within 7 days, it will be closed. Closed does not mean "never", just that it has no momentum to get accomplished any time soon.
See CONTRIBUTING.md for more info.

danmactough · 2020-10-12T23:07:38Z

😭 @eculver @nickatsegment

nickatsegment · 2020-10-13T23:21:55Z

Sorry @danmactough , so many different irons in different fires around here I've lost track :D

WRT to case, I think regardless of the reasoning for why some parts are uppercase, it would be weird if only one provider converted to uppercase, wouldn't you say? If anything, we should do the case transformation at a higher level?

when I use this secret one way (with chamber exec), the keys are UPPERCASE but when I use this secret any other way, the keys are lowercase.

I wasn't here when it was invented, but I think the rationale is simply that environment variables tend to be UPPER_CASE.

danmactough · 2020-10-14T03:56:59Z

so many different irons in different fires around here I've lost track :D

No worries @nickatsegment. At least one of those fires has been all over the news, right? 🤑 🍾 🎊

On the case issue, I'm happy to revert cbd2cc2.

To restate my thinking a bit, I think that converting all the secret keys to lowercase when using Secrets Manager makes the interoperability that we've been striving for less useful. Imagine that you use Chamber to manage a secret in Secrets Manager and when you use chamber env or chamber exec the environment variable names are always UPPERCASE, but then, expecting Chamber and Secrets Manager to be interoperable, in another context (where using Chamber is not possible or practical) you use Secrets Manager to get the secret and the property names are all lowercase instead of UPPERCASE -- I think that's surprising in a bad way.

But honestly, normalizing the names in ANY way is kind of surprising. Like, why can't I have PascalCase or camelCase environment variable names if that's what I want? So, like you say maybe consistency is better at this point, even if it's not optimal, and perhaps an improvement in v3 😄 would be to find a way to preserve the original input for environment variable names.

Using a type that matches the API rather than specifically an STS client allow us to pass in an STS mock

…amber secrets

danmactough · 2020-10-14T04:08:57Z

rebased and dropped cbd2cc2

nickatsegment · 2020-10-14T17:58:29Z

store/secretsmanagerstore.go

@@ -0,0 +1,562 @@
+package store


Last thing, I promise :)

Would you be willing to be the maintainer of this backend for the time-being? Nothing formal or enforceable obviously. All PRs would still go through Segmenters for approval (we haven't quite figured out how to deal with giving external folks merge access). We'll just defer to you on issues and PRs.

If that sounds good, just add a comment to the top of this file:

Suggested change

package store

// Secrets Manager Store is maintained by Dan McTough https://github.com/danmactough. Thanks Dan!

package store

No problem. I'll just correct the spelling of my name. 🙃

Added via 53be3ab @nickatsegment

No problem. I'll just correct the spelling of my name. 🙃

Whoops! My bad, sorry.

nickatsegment · 2020-10-14T18:01:16Z

OK, looks good to me. Just that question on notating maintainership :) Then we'll merge

nickatsegment · 2020-10-14T19:03:12Z

Re interoperability, basically chamber is not really designed to be interoperable: it's designed to work with secrets it has written, and makes no attempt to make its write format flexible or interoperable.

Making it interoperable is perhaps a nice idea, but obviously would be a big change and require a bunch more knobs and dials that are all opportunities for error. In my mind, it's fundamentally a different tool at that point.

But honestly, normalizing the names in ANY way is kind of surprising. Like, why can't I have PascalCase or camelCase environment variable names if that's what I want?

I believe the intention is to reduce the possibility of error and confusion. It's a tradeoff of flexibility in the name of simplicity and avoiding footguns.

#267 added secret manager support, so I updated the readme to note that. That way people won't need to dig through the issues to figure out what the support status is.

nickatsegment self-requested a review June 18, 2020 17:44

nickatsegment approved these changes Jun 18, 2020

View reviewed changes

eculver self-requested a review June 18, 2020 17:50

eculver reviewed Jun 18, 2020

View reviewed changes

danmactough force-pushed the sm-backend branch from 10b2b6c to 8c41e44 Compare July 20, 2020 03:14

danmactough force-pushed the sm-backend branch from 01128f3 to 74494b2 Compare July 21, 2020 03:22

eculver reviewed Aug 3, 2020

View reviewed changes

danmactough force-pushed the sm-backend branch from 583cf39 to 6d8f119 Compare August 12, 2020 03:28

stale bot added the stale label Oct 11, 2020

stale bot removed the stale label Oct 12, 2020

danmactough added 4 commits October 14, 2020 00:01

Add secretsmanager to vendor

34ae58f

Initial implementation of Secrets Manager backend

2723e82

Use copies inside for loops

8553478

Retain max versions of secret

10342be

danmactough added 11 commits October 14, 2020 00:01

Make store more testable

89624ab

Using a type that matches the API rather than specifically an STS client allow us to pass in an STS mock

Factor out helper to convert JSON to a secretValueObject type

5b2131a

Fix history to deduplicate and sort versions

6974726

Fix deletion not erroring when secret is not present

375d769

Add SecretsManagerStore tests

260e21a

Add SecretsManagerStore benchmark

98c53a7

Rewrite how we store values and metadata

a6abbeb

Change json unmarshalling and history handling to interop with non-Ch…

0333fde

…amber secrets

Remove unnecessary check

f9bd3e4

Add explanatory comments for key deletion

915195e

Fail when asked to write to a secret having rotation enabled

8d52e89

danmactough force-pushed the sm-backend branch from cbd2cc2 to 8d52e89 Compare October 14, 2020 04:01

Check in vendor

43cdc2c

nickatsegment approved these changes Oct 14, 2020

View reviewed changes

nickatsegment reviewed Oct 14, 2020

View reviewed changes

Add maintainership comment

53be3ab

nickatsegment merged commit 0829349 into segmentio:master Oct 14, 2020

calebAtIspot mentioned this pull request Jun 5, 2024

add note about secrets manager #504

Merged

bhavanki pushed a commit that referenced this pull request Jun 17, 2024

add note about secrets manager (#504)

ba7e4e6

#267 added secret manager support, so I updated the readme to note that. That way people won't need to dig through the issues to figure out what the support status is.

	package store
	// Secrets Manager Store is maintained by Dan McTough https://github.com/danmactough. Thanks Dan!
	package store

feat: Add AWS Secrets Manager Store #267

feat: Add AWS Secrets Manager Store #267

Conversation

danmactough commented Jun 13, 2020

nickatsegment commented Jun 18, 2020

nickatsegment left a comment

Choose a reason for hiding this comment

nickatsegment commented Jun 18, 2020

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

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

danmactough commented Jun 19, 2020

nickatsegment commented Jun 23, 2020

danmactough commented Jul 7, 2020

eculver commented Jul 8, 2020

danmactough commented Jul 19, 2020

danmactough commented Jul 20, 2020

danmactough commented Jul 21, 2020

danmactough commented Jul 31, 2020

eculver 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

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

Choose a reason for hiding this comment

danmactough commented Aug 12, 2020 • edited Loading

stale bot commented Oct 11, 2020

danmactough commented Oct 12, 2020

nickatsegment commented Oct 13, 2020 • edited Loading

danmactough commented Oct 14, 2020

danmactough commented Oct 14, 2020

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

nickatsegment commented Oct 14, 2020

nickatsegment commented Oct 14, 2020

danmactough commented Aug 12, 2020 •

edited

Loading

nickatsegment commented Oct 13, 2020 •

edited

Loading