Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for retrieving specific secret versions #45

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tratnayake
Copy link

Note To Readers: Hi folks! 👋🏽 This is my first time contributing to open source software && I'm also pretty new to Go 🥺 and I'm sure I'm probably missing some things. Would definitely appreciate feedback and suggestions on what to do next (like..how do I test this with project convetions?) :)


What does this PR do?

  • Provides the ability for users to specify a secret version to retrieve.
  • A path such as: !var accounting/database#username^AWSPREVIOUS will now fetch the username key with the version ID of AWSPREVIOUS

Context

  • AWS has the ability to specify versions / staging labels for secrets.
  • By default, the newest version will take on the label AWSCURRENT
  • When an AWS SecretsManager executes a update-secret operation, it assigns the version AWSCURRENT to the new value, and AWSPREVIOUS to the one before it.
  • When Summon fetches a secret from AWS Secrets Manager, it retrieves the AWSCURRENT by default.
  • This feature comes in handy if users want to specify specific versions (i.e. when doing a credential rotation)
  • This provider already specifies a way to get a specific key from a secret (if stored as a JSON object) by using the # separator.
    • i.e. If we have a secret such as accounting/database that looks like: {"username":"bob","password":"changeme"}, using a path like !var accounting/database#username will return bob
  • Using the same precedent, we will return specific versions using the ^ operator (i.e. !var accounting/database#username^AWSPREVIOUS

What ticket does this PR close?

Checklists

Change log

  • The CHANGELOG has been updated, or
  • This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

  • I have no idea how to test this ¯_(ツ)_/¯
  • Manual tests:
!var accounting/database#username !var accounting/database#password
AWSPREVIOUS username_a password0
AWSCURRENT username_b password1
  1. Retrieve specific key and version
go run main.go accounting/database#username^AWSPREVIOUS                                                                                                                                                                                                        
username_a%    
  1. Retrieve specific key and current version
 go run main.go accounting/database#username^AWSCURRENT                                                                                                                                                                                                       
username_b%
  1. Show that retrieving specific key without specific version returns current version
 go run main.go accounting/database#username                                                                                                                                                                                                                   
username_b%
  1. Retrieve whole secret with specified current version
 go run main.go accounting/database^AWSCURRENT                                                                                                                                                                                                                 
{"username":"username_b","password":"password1"}%
  1. Retrieve whole secret with specific version
go run main.go accounting/database^AWSPREVIOUS                                                                                                                                                                                                               
{"username":"username_a","password":"password0"}%    
  1. Attempt to get a secret with a version that doesn't exist
go run main.go accounting/database^DOESNTEXIST                                                                                                                                                                                                              
ResourceNotFoundException: Secrets Manager can't find the specified secret value for staging label: DOESNTEXISTexit status 1

Documentation

  • Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or

@tratnayake tratnayake requested a review from a team as a code owner October 14, 2020 18:27
@doodlesbykumbi
Copy link
Contributor

doodlesbykumbi commented Oct 15, 2020

Hi @tratnayake. Thank you for making this contribution. It is very much appreciated!

I think this is a valuable change, and am happy to provide feedback. To begin with I've provided a checklist of some house-keeping, I believe these will be necessary to merge:

  • To accept your contribution, we need a "DCO" signature on your commit. You can find out more about those here. Please also note that we provide a reference that describes the process to add your signoff to commit messages. It's available here.
  • This change would need updates to both the README.md and the CHANGELOG.md
  • The version in VERSION and VERSION.go would need to be bumped.
  • Please add information about the change to your commit message e.g. describe what was fixed or enabled in some detail.

I'll provide feedback on the PR in subsequent comments.

@doodlesbykumbi
Copy link
Contributor

doodlesbykumbi commented Oct 15, 2020

  1. Using the same precedent, we will return specific versions using the ^ operator (i.e. !var accounting/database#username^AWSPREVIOUS)

    Given a path path/to/secret#top/level/key/to/value^version/stage, there's ambiguity. The current implementation makes it impossible to access secrets that have a top level key containing ^, instead it will be interpreted as a version stage.

    I think we originally chose # to separate path/to/secret and top/level/key/to/value, because the format of path/to/secret ensures that it never contains#. We'd likely want to do something similar for the version stage.

  2. Secrets manager supports both VersionStage and VersionId, see https://docs.aws.amazon.com/sdk-for-go/api/service/secretsmanager/#GetSecretValueInput. It would be good if the outcome of this PR supported both, I think at present the PR only support for only VersionStage. I'd love to know your thoughts on how you might approach this, it seems to me that a simple string path might struggle to accommodate everything and so we might need to use JSON or something e.g. !var '{"path": "accounting/database", "key": "username", "versionStage": "AWSPREVIOUS"}'

  3. For testing we have considered using https://github.com/localstack/localstack but have yet to implement it. For now we have unit tests to cover the logic, and I believe for a release we run a manual test against live AWS. Unit tests to validate logic should suffice

Copy link
Contributor

@doodlesbykumbi doodlesbykumbi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tratnayake. I left some comments above. This comment is mostly just to mark the PR as having a review.

@izgeri
Copy link
Contributor

izgeri commented Nov 18, 2020

@tratnayake - 👋 are you going to be able to make the requested changes on this PR? If not, we can look into scheduling time to push this forward for you, but it might not be for a month or two. Please let us know if the requested changes above make sense, and if you have any issues please feel free to ask questions here or on Discourse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants