Implement support for AWS SecretsManager by natepage · Pull Request #4 · brefphp/secrets-loader

natepage · 2023-08-08T01:11:46Z

Hi there 👋

secrets-loader is great, easy to setup, and works beautifuly with AWS SSM.

However, most of our projects work with both SSM and SecretsManager,
This PR allows people to use natively use secrets from SecretsManager with Bref.

mnapoli · 2023-08-08T10:04:57Z

It seems the tests are failing because of Undefined array key "AWS_DEFAULT_REGION": you might want to force an AWS_REGION in the test class?

natepage · 2023-08-08T14:42:33Z

@mnapoli this was a legit error, the SecretsManagerClient shouldn't not be called in the failing test. The issue was triggered by of a contengency between tests because we're using putenv() to set the env variables and it carries between test.
I've explicitly removed the env variable from previous test and it's all green now

natepage · 2023-08-08T21:39:58Z

@mnapoli I've pushed a last fix for symfony/polyfill-uuid, it's required by async-aws/secrets-manager. However, checking their releases, 1.13.0 was broken and it was fixed 2 days later in 1.13.1

You can see this in their changelog, it fixes the issues with constants: https://github.com/symfony/polyfill/blob/1.x/CHANGELOG.md#1131

jaulz · 2023-09-25T11:13:25Z

@natepage nice work 😊 Do you already use it? I assume it also needs some adjustments here:
https://github.com/brefphp/bref/blob/master/src/LazySecretsLoader.php#L36 ?

jaulz · 2023-09-25T13:52:25Z

src/Secrets.php

+                if ($e->getCode() === 400) {
+                    // Extra descriptive error message for the most common error
+                    throw new RuntimeException(
+                        "Bref was not able to resolve secrets contained in environment variables from SecretsManager because of a permissions issue with the SecretsManager API. Did you add IAM permissions in serverless.yml to allow Lambda to access SecretsManager? (docs: https://bref.sh/docs/environment/variables.html#at-deployment-time).\nFull exception message: {$e->getMessage()}",


It would be cool to try to get all the secret values first and afterwards throw the error (including indicating which secrets have failed).

In case of a permission issue it's more likely that permissions are not good for all secrets. If not, that's not a common case, I'm fine keeping it simple honestly (easier to maintain).

mnapoli · 2023-09-29T09:32:42Z

I don't use SecretsManager and I have one hesitation before merging this: isn't it more common to store multiple variables in 1 SecretManager entry?

That's a pattern I've seen multiple times, and I think it's caused by the fact that SecretManager deals well with JSON values (so multiple values in 1 secret) and that secrets are expensive (by the unit). It also means 1 HTTP call (faster and cheaper) to retrieve all values.

For example, that's how secrets are stored for AWS services: https://docs.aws.amazon.com/secretsmanager/latest/userguide/reference_secret_json_structure.html

jaulz · 2023-09-29T11:19:08Z

I don't use SecretsManager and I have one hesitation before merging this: isn't it more common to store multiple variables in 1 SecretManager entry?

Are you referring to this PR or #7? This PR indeed assumes that all secrets are in JSON format but actually you can also just store simple strings and it would fail then.

This is how this PR works:

Secrets:

DB_CONNECTION: { "host": "example.org", "username": "test", "password": "test" }

serverless.yml:

provider:
  environment:
    DB_CONNECTION: bref-secretsmanager:DB_CONNECTION

Expands to these environment variables:

DB_CONNECTION={ "host": "example.org", "username": "test", "password": "test" }
host=example.org
username=test
password=test

However, I think it should simply expand to the actual value (see #7) and the rest is up to the application to parse it properly:

DB_CONNECTION: { "host": "example.org", "username": "test", "password": "test" }

It's unopinionated and would work exactly the same like the bref-ssm: prefix.

natepage · 2023-10-02T05:45:43Z

Hey there, sorry I missed the party 😄

@mnapoli agree with you here, all projects I worked on using AWS SecretsManager are storing multiple variables in a single secret.
However, @jaulz got a point, the current implementation wouldn't support secrets that aren't valid JSON values.

What about introducing some kind of processor, similar to what Symfony does with their env variables?
We could support both:

bref-secretsmanager:MY_SECRET => Simply set env var with whatever value from AWS
bref-secretsmanager:json:MY_SECRET => JSON decode value from AWS and set individual env vars

I'm happy to push changes to support this if sounds good to you!

natepage · 2023-10-02T06:19:58Z

The changes to add a json processor were not too hard so I went ahead and did it so you can have a look a decide, I added a test to illustrate how it would work

natepage · 2024-01-19T00:26:33Z

Hey there, Happy New Year!

A gentle follow up on this one as we would to be able to use this in our projects 😄

f-lombardo · 2024-01-26T10:37:07Z

composer.json

        "async-aws/ssm": "^1.3"
    },
    "require-dev": {
+        "async-aws/secrets-manager": "^1.0",


Is it OK to put this in the require-dev instead of in the require section?

@f-lombardo my thinking was people don't need this dependency unless they are working with secrets manager, and because ssm is the default implementation of this library it's most likely to be the only use in a majority of cases. But I do not have strong opinions about it 😄

IMHO since async-aws/ssm is in the require section, the same should hold for async-aws/secrets-manager

f-lombardo · 2024-03-10T18:56:32Z

Hey there, Happy New Year!

A gentle follow up on this one as we would to be able to use this in our projects 😄

Hello @natepage, since I also had to use some new features in a few projects, I created a small fork of this one: https://github.com/f-lombardo/secrets-loader

If you want, you could submit a PR to my repo too. I'll take care of mantaining my project aligned with the original one

f-lombardo · 2024-04-07T09:44:43Z

Hey there, sorry I missed the party 😄

@mnapoli agree with you here, all projects I worked on using AWS SecretsManager are storing multiple variables in a single secret. However, @jaulz got a point, the current implementation wouldn't support secrets that aren't valid JSON values.

What about introducing some kind of processor, similar to what Symfony does with their env variables? We could support both:
* `bref-secretsmanager:MY_SECRET` => Simply set env var with whatever value from AWS

* `bref-secretsmanager:json:MY_SECRET` => JSON decode value from AWS and set individual env vars
I'm happy to push changes to support this if sounds good to you!

I've implemented something similar here:
https://github.com/f-lombardo/secrets-loader/blob/master/README.md

natepage · 2024-07-17T22:59:31Z

Hi @mnapoli 👋

Hope you're well, I would like to follow up on this as we're very interested in the functionality 😄

DIbraDev · 2025-07-13T10:01:36Z

Hello,

Hi @mnapoli 👋

Hope you're well, I would like to follow up on this as we're very interested in the functionality 😄

Indeed, using Secrets Manager is important for cases like pulling RDS password at runtime.

@mnapoli would you consider this PR please ?

Implement support for AWS SecretsManager

9eda652

natepage mentioned this pull request Aug 8, 2023

Support secrets from AWS Secrets Manager brefphp/bref#1514

Open

Fix test and cs

269868d

Fix minimum version for symfony/polyfill-uuid

13e6914

jaulz mentioned this pull request Sep 25, 2023

feat: uncouple secrets loader brefphp/bref#1652

Closed

jaulz reviewed Sep 25, 2023

View reviewed changes

jaulz mentioned this pull request Sep 25, 2023

feat: implement support for AWS Secrets Manager (alternative) #7

Open

Implement json processor for bref-secretsmanager

6b8ceba

f-lombardo reviewed Jan 26, 2024

View reviewed changes

Uh oh!

Conversation

natepage commented Aug 8, 2023

Uh oh!

mnapoli commented Aug 8, 2023

Uh oh!

natepage commented Aug 8, 2023

Uh oh!

natepage commented Aug 8, 2023

Uh oh!

jaulz commented Sep 25, 2023

Uh oh!

jaulz Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

mnapoli Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

mnapoli commented Sep 29, 2023

Uh oh!

jaulz commented Sep 29, 2023

Uh oh!

natepage commented Oct 2, 2023

Uh oh!

natepage commented Oct 2, 2023

Uh oh!

natepage commented Jan 19, 2024

Uh oh!

f-lombardo Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

natepage Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

f-lombardo Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

f-lombardo commented Mar 10, 2024

Uh oh!

f-lombardo commented Apr 7, 2024

Uh oh!

natepage commented Jul 17, 2024

Uh oh!

DIbraDev commented Jul 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants