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

feat: add secrets adapter for aws secrets manager #1141

Merged
merged 5 commits into from
Nov 21, 2024

Conversation

justindell
Copy link
Contributor

@justindell justindell commented Oct 21, 2024

This add AWS Secrets Manager as a secrets adapter. It assumes the AWS CLI is installed and has access to secrets manager, so no login step is required. Instead, the --account option is used to select a profile name, which is typically default.

Site documentation PR: documentation for aws secrets manager

@@ -0,0 +1,25 @@
class Kamal::Secrets::Adapters::AwsSecretsmanager < Kamal::Secrets::Adapters::Base
Copy link
Collaborator

@djmb djmb Nov 4, 2024

Choose a reason for hiding this comment

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

The official name is AWS Secrets Manager, so let's call this AwsSecretsManager (and rename the file to aws_secrets_manager.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's funny, I went back and forth on the name but ultimately decided on one word because the AWS CLI calls it "secretsmanager" over "secrets-manager". I agree though, I like the two word version better and will get that changed.

class Kamal::Secrets::Adapters::AwsSecretsmanager < Kamal::Secrets::Adapters::Base
private
def login(_account)
nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no sensible default way to implement this is there? Would be nice if we could prompt for MFA from here if that was enabled, but we'd need to know all about the IAM setup for that to work I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's just too many different ways to authenticate with AWS. I believe the preferred method is SSO using the aws sso login command, but the user could also be authenticated with access keys, environment variables, automatically (if they're on an EC2 instance with an IAM Role), or even an external process.

One option would be to check if the user is authenticated with aws sts get-caller-identity, and if that command returns an error, attempt to run aws sso login. However, this could be confusing if they normally authenticate a different way, or even worse have aws cli v1 installed which doesn't include sso login.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes let's leave it for now. We could I guess add a separate SsoAwsSecretsManager or something like that later.

@@ -0,0 +1,87 @@
require "test_helper"

class AwsSecretsmanagerAdapterTest < SecretAdapterTestCase
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we rename this too?

end

def get_from_secrets_manager(secrets, account:)
`aws secretsmanager batch-get-secret-value --secret-id-list #{secrets.map(&:shellescape).join(" ")} --profile #{account}`.tap do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's shell escape the account here as well 👍

@djmb
Copy link
Collaborator

djmb commented Nov 4, 2024

@justindell - thanks for the PR!

Just a few minor suggestions, then we can get this merged.

@djmb
Copy link
Collaborator

djmb commented Nov 4, 2024

Sorry one more thing - could we also implement the check_dependencies! method that was recently added to the base adapter. It should give a nice error message if the cli is not installed.

@justindell
Copy link
Contributor Author

Should be all set! Let me know if you see anything else

@djmb djmb merged commit 57e48a3 into basecamp:main Nov 21, 2024
8 checks passed
@chrisdebruin
Copy link
Contributor

@justindell Thanks for this addition just what I needed. However I found an issue when trying this out. I have some passwords containing ( and this goes wrong. For example:

results  = { test: 'password(' }
secrets = JSON.dump(results).shellescape =>  "\\{\\\"test\\\":\\\"password\\(\\\"\\}" 
JSON.parse(secrets) => unexpected token at '\{\"test\":\"password\(\"\}' (JSON::ParserError)

as you can see it is escaping the ( in the password string but it's not unescaped when reading it and trying to parse the json

@justindell
Copy link
Contributor Author

@justindell Thanks for this addition just what I needed. However I found an issue when trying this out. I have some passwords containing ( and this goes wrong. For example:

results  = { test: 'password(' }
secrets = JSON.dump(results).shellescape =>  "\\{\\\"test\\\":\\\"password\\(\\\"\\}" 
JSON.parse(secrets) => unexpected token at '\{\"test\":\"password\(\"\}' (JSON::ParserError)

as you can see it is escaping the ( in the password string but it's not unescaped when reading it and trying to parse the json

Could it be related to this? #1009 What happens if you try the kamal secrets print command?

@chrisdebruin
Copy link
Contributor

chrisdebruin commented Dec 9, 2024

@justindell

here is output of kamal secrets print

./bin/kamal secrets print -d staging
SECRETS=\{\"staging/TEST\":\"password\(\",\"staging/TEST2\":\"bar\"\}
TEST=$(bundle exec kamal secrets extract TEST \{\"staging/TEST\":\"password\(\",\"staging/TEST2\":\"bar\"\})

when running bundle exec kamal secrets extract $SECRETS I get this error ERROR (JSON::ParserError):`

was able to fix it with a new method

 def extract(name, secrets)
    parsed_secrets = parse_secrets(secrets)
    ....
 end 

  def parse_secrets(secrets)
      secrets = secrets.shellsplit.first if secrets.start_with?("\\{")

      JSON.parse(secrets)
    end

@justindell
Copy link
Contributor Author

This looks like an issue outside of the scope of the aws secrets manager. I just created passwords with both 1password and AWS Secrets Manager with a password value of "test(", and it looks like it isn't properly being extracted (although "test" works fine on both). Would you like to create an issue?

@chrisdebruin
Copy link
Contributor

I'll create a PR to fix this issue

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

Successfully merging this pull request may close these issues.

3 participants