docdb.DatabaseSecret: Secret generated with invalid characters

[Voyen]

docdb.DatabaseSecret by default calls aws_secretsmanager.Secret specifying exclusion characters of '"', '@', and '/'.
However since many databases use a 'proto://user:password@host.....' connection URL, a colon should be included in this exclusions list.

I'm currently trying to spin up a mongo-express container in my ECS environment but can't get it to connect because the secret that was generated contains a colon and so the connection URL is invalid.

Reproduction Steps

const secret = new docdb.DatabaseSecret(this, 'ClusterSecret', {
    username: 'root',
    secretName: 'myClusterSecret',
});

What did you expect to happen?

A valid secret is generated that can be used in a connection URL

What actually happened?

Secret generated with a ':' in it which makes connection URLs invalid

Environment

• CDK CLI Version : 1.114.0 (build 7e41b6b)
• Framework Version:
• Node.js Version: v15.4.0
• OS : 5.12.15.arch1-1
• Language (Version): Using Typescript but I assume this affect all

Other

Note that while using this within your own applications is controllable (my Spring Boot application builds the connection string and url-encodes the password), providing this to out of the box images as environment secrets is impossible

---

This is 🐛 Bug Report

[skinny85]

Thanks for opening the issue @Voyen. I see two possible ways out of this:

1. Change the default excludeCharacters here to have more characters, like :.
2. Allow passing your own excludeCharacters to DatabaseSecret, like we do in RDS.

Thoughts?

[Voyen]

@skinny85 I feel like the ideal scenario would be to go with both. The default should be updated as it's technically not a reasonable default to begin with (allowing invalid connection URLs in environments where you cannot encode it), but the option to be able to specify your own excludeCharaters would be a great feature as it both lines up with an existing similar construct (RDS) and allows for more flexibility/control.

[skinny85]

Fair enough @Voyen!

Any chance you could open us a Pull Request implementing this? Here's out "Contributing" guide: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md.

Thanks,
Adam

[jumic]

I just submitted a PR which adds property excludeCharaters.

@skinny85 Before changing the default excludeCharacters, can I have your feedback on this topic again?
From my point of view, changing the excludeCharacters will trigger the generation of a new Secret. If we change the default behavior in DocumentDB, a new password is created. If the password is maintained manually in a third party system, this will be a breaking change because the password will change.

Which option do you prefer?

• Change the default excludeCharacters and don't care about the regeneration of new passwords (I wouldn't recommend this option).
• Change the default excludeCharacters, add CDK feature flag to active this change in new CDK apps on only.
• Don't change the default excludeCharacters.

Thanks.

[skinny85]

Hey @jumic,

this module is stable, so changing the default is not an option.

Whether you want to make the change under a feature flag, or not yet - that's up to you. We can do it in a separate PR, after #17262 gets merged.

[ahammond]

@jumic I'd love to see the default changed (under a feature flag, I guess, but hey, I'm on CDKv2, so... whatever).

[jumic]

Hey @skinny85,

I tried to implement the new default exclude characters under a feature flag. Unfortunately, there is one thing I can't solve at the moment.

The feature flag has to be evaluated in the constructer of DatabaseSecret. FeatureFlags.of(this) can't be used because this can be used after the super() call only. I also tried to use FeatureFlags.of(scope). This is not possible because of type incompatibilities.

Accessing this.node.defaultChild after super() call would work, however I don't think it's a best practice.

const cfnSecret = this.node.defaultChild as CfnSecret;
cfnSecret.generateSecretString = {
  passwordLength: 41,
  secretStringTemplate: JSON.stringify({
    username: props.username,
    masterarn: props.masterSecret?.secretArn,
  }),
  generateStringKey: 'password',
  excludeCharacters: props.excludeCharacters ?? '"@/', // FeatureFlags.of(this)... 
};

Do you have any hints how to solve it? Thanks.

[skinny85]

I think you can safely use FeatureFlag.of(scope) in this case, which should be available before you call super.

[jumic]

FeatureFlags.of(scope) didn't work.

export class DatabaseSecret extends Secret {
  constructor(scope: Construct, id: string, props: DatabaseSecretProps) {
    FeatureFlags.of(scope)
    super(scope, id, {

Error message:

Argument of type 'import("/Users/julian/dev/aws-cdk/node_modules/constructs/lib/construct").Construct' is not assignable to parameter of type 'import("/Users/julian/dev/aws-cdk/packages/@aws-cdk/core/lib/construct-compat").Construct'.
Type 'Construct' is missing the following properties from type 'Construct': node, validate, prepare, synthesizets(2345)

I tried the same on branch v2-main. On this branch, the error doesn't exist. In my opinion, the difference is caused by the import in class FeatureFlags.

master: import { Construct } from '../lib/construct-compat';
v2-main: import { Construct } from 'constructs';

Maybe it's the best to postpone this change until v2 is released.

[skinny85]

Yeah, I see the problem 😕.

If you've got the stomach for it, you can work around it:

export class DatabaseSecret extends Secret {
  constructor(scope: Construct, id: string, props: DatabaseSecretProps) {
    const featureConstruct = scope.tryFindChild('@SomeRandomid') as CoreConstruct;
    if (!featureConstruct) {
      featureConstruct = new CoreConstruct(scope, '@SomeRandomId');
    }

    FeatureFlags.of(featureConstruct) // now this should work

    super(scope, id, {
      // ...

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.