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 AWS Secrets Manager #10663

Closed
wants to merge 5 commits into from

Conversation

cmaluend
Copy link

@cmaluend cmaluend commented Jul 12, 2020

AWS Secrets Manager Extension

Description

The goal of this extension is to make easy then integration with AWS Secrets Manager. The extension provides an annotation to inject the secret into a variable. Also, it is possible to inject the client and use it directly.

Tasks

  • AWS Secrets Manager Client
  • AWS Secrets Manager Async Client
  • AWS Secrets Manager Annotation
  • AWS Secrets Manager integration with DB Config #6896

AWS Secrets Manager Clients

Sync Client

    @Inject
    SecretsManagerClient client;

Async Client

    @Inject
    SecretsManagerAsyncClient asyncClient;

AWS Secrets Manager Annotation

Allows injecting a secret from AWS Secrets Manager directly to a variable. It supports plaintext and binary secrets.

    @AWSSecretsManager("a-secret-id")
    String secretId;

Sorry, something went wrong.

@boring-cyborg boring-cyborg bot added area/core area/dependencies Pull requests that update a dependency file labels Jul 12, 2020
@sberyozkin
Copy link
Member

Hi @cmaluend, thanks, I think it should be tied somehow with the credentials-spi as well, https://github.com/quarkusio/quarkus/tree/master/extensions/credentials.
CC @vsevel @gsmet

@sberyozkin
Copy link
Member

Just wonder if this PR can contribute to resolving #6896 as well ? Would be great if it were possible :-).

@cmaluend
Copy link
Author

Just wonder if this PR can contribute to resolving #6896 as well ? Would be great if it were possible :-).

My goal was to be able to use AWS Secrets Manager in a native image. But, I think this is a good use case. I will take a look at it. This is my first contribution, probably I will have some questions.

@gsmet
Copy link
Member

gsmet commented Jul 16, 2020

@marcinczeczko could you have a look at that one?
@vsevel you might be interested in this one too.

@marcinczeczko
Copy link
Contributor

marcinczeczko commented Jul 16, 2020 via email

@sberyozkin
Copy link
Member

@cmaluend Thanks for the great contribution so far, and also considering to expose this engine as CredentialsProvider, it would be great because it will let us verify the concept of Vault being replaceable by an alternative provider.
See these docs which Vincent did:
https://quarkus.io/guides/credentials-provider

Hopefully it will be quite straightforward given that you already have the working extension, CredentialsProvider impl may only need to get the suggested configuration properties and delegate to AWS secrets manager client...
Cheers

@cmaluend
Copy link
Author

cmaluend commented Jul 17, 2020

@sberyozkin
I took a quick look at the credential provider yesterday and I have been thinking that a better solution would be using the secret from AWS Secrets Manager directly in the application.properties. This will be more generic and other extensions will be able to use it. For example, credentials for Artemis or AMQP can be stored in AWS Secrets Manager.

Example:

quarkus.secretsmanager.aws.secrets.mysecrets-db-pwd=cristian-db-pwd (1)
quarkus.secretsmanager.aws.secrets.mysecrets-artemise=cristian-artemise-pwd (1)
quarkus.secretsmanager.aws.secrets.mysecrets-amqp=cristian-amqp-pwd (1)
quarkus.datasource.password=${mysecrets-db-pwd} (2)
quarkus.qpid-jms.password=${mysecrets-artemise} (3)
amqp-password=${mysecrets-amqp} (4)
  1. mysecrets will have a secret from AWS Secrets Manager. Where cristian-db-pwd is the AWS secret id.
  2. The datasource will use the password stored in the variable mysecrets-db-pwd
  3. The Artemis extension will use the password stored in the variable mysecrets-artemise
  4. The AMQP extension will use the password stored in the variable mysecrets-amqp

@sberyozkin
Copy link
Member

@cmaluend but Agroal gets the passwords from CredentialsProvider. Perhaps CredentialsProvider needs some enhancements to support your more generic idea ?
So for example, Agroal can get the passwords from Vault via CredentialsProvider. How would your proposal fit in ? Would CredentialsProvider have to be dropped ?

@vsevel
Copy link
Contributor

vsevel commented Jul 19, 2020

I took a quick look at the credential provider yesterday and I have been thinking that a better solution would be using

I will take a look at your ideas in the next few days.

@vsevel
Copy link
Contributor

vsevel commented Jul 20, 2020

there are different use cases to consider:

  1. programmatic api, which looks like your SecretsManagerClient, or the VaultKVSecretEngine in the vault extension
  2. config source, which you seem to hint at in your Adding AWS Secrets Manager #10663 (comment), and has been implemented by the VaultConfigSource in the vault extension => see this guide
  3. credentials provider spi implementation, such as the VaultCredentialsProvider, which allows implementing advanced use cases (e.g. vault dynamic db credentials), and provide a standard way for a user to develop its custom secret store

from what I see you are tackling use cases 1 and 2. for the second one the correct approach is to create a config source. but there are a few challenges at the moment such as #4848 (which should be improved with #9991 (comment)). If you do not develop a config source, then an alternative is to make your extension implement the CredentialsProvider interface. and then it will be automatically usable from extensions that can consume credentials from a CredentialsProvider (i.e. the different db connection pools at the moment).

hope this makes it clearer.

@gastaldi gastaldi added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Jul 23, 2020
@cmaluend
Copy link
Author

I will work on the CredetialsProvider this weekend.

@sberyozkin
Copy link
Member

sberyozkin commented Sep 2, 2020

Hi @cmaluend How are you, have you had a chance to look at the CredentialsProvider update to finalize this PR ? May be it can make sense to follow up with another PR to make it simpler ? thanks

@gsmet
Copy link
Member

gsmet commented Oct 12, 2020

Closing this one. Please reopen if you have some updates.

@gsmet gsmet closed this Oct 12, 2020
@gsmet gsmet added the triage/out-of-date This issue/PR is no longer valid or relevant label Oct 12, 2020
@mvrk57
Copy link

mvrk57 commented Feb 12, 2021

@cmaluend any news? Youre extension looks very interesting. I hope the pull request gets merged :-) I would use youre changes! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file triage/needs-rebase This PR needs to be rebased first because it has merge conflicts triage/out-of-date This issue/PR is no longer valid or relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants