Support SecretValue for DatabaseCluster masterUser.username

[asnaseer-resilient]

When we used Cloudformation directly, we used to set the master username and password for our DB cluster like this:

...,
"MasterUsername":{"Fn::Sub":"{{resolve:secretsmanager:DB_CREDS:SecretString:username}}"},
"MasterUserPassword":{"Fn::Sub":"{{resolve:secretsmanager:DB_CREDS:SecretString:password}}"},
...

With the CDK, it seems like the credentials are defined as:

    const secret = new Secret(this, 'MyDBSecret', {
      secretName: 'DB_PASSWORD',
      generateSecretString: {
        excludePunctuation: true
      }
    });

    const dbCluster = new DatabaseCluster(this, 'MyDbCluster', {
      ...,
      masterUser: {
        username: 'myUserName',
        password: secret.secretValue
      },
      ...
    });

So only the password can be secure.

It would be nice to be able to set both the username and password based on a JSON secret value of the form:

{
   "username": "myUserName",
   "password": "dontTellAnyone!"
}

I am currently using this workaround to achieve what I want:

    const username = SecretValue.secretsManager(DB_SECRET, { jsonField: 'username' }).toString();
    const password = SecretValue.secretsManager(DB_SECRET, { jsonField: 'password' });

    const dbCluster = new DatabaseCluster(this, 'MyDbCluster', {
      ...,
      masterUser: {
        username
        password
      },
      ...
    });

Use Case

For improved security.

Proposed Solution

In the Login data type, define the username field as string | SecretValue. This might make it more obvious to others that they can also secure the username but, at the same time, keep this backwards compatible.

• 👋 I may be able to implement this feature request
• ⚠️ This feature might incur a breaking change

---

This is a 🚀 Feature Request

[skinny85]

Hey @asnaseer-resilient ,

thanks for opening the issue, but I'm not sure what you're asking for here. The code you show for how you're achieving that today is exactly how I would do it. What's your ask here?

Thanks,
Adam

[asnaseer-resilient]

The current code uses the secrets differently for the username vs the password, e.g. in here:

    const username = SecretValue.secretsManager(DB_SECRET, { jsonField: 'username' }).toString();
    const password = SecretValue.secretsManager(DB_SECRET, { jsonField: 'password' });

    const dbCluster = new DatabaseCluster(this, 'MyDbCluster', {
      ...,
      masterUser: {
        username
        password
      },
      ...
    });

notice that the username needs a .toString() (i.e. as a String data type in Typescript) before it is passed into the masterUser structure whereas the password value can be passed in as-is (i.e. as SecretValue data type in Typescript).

It would be nice to have them both consistent - something like this:

    const username = SecretValue.secretsManager(DB_SECRET, { jsonField: 'username' });
    const password = SecretValue.secretsManager(DB_SECRET, { jsonField: 'password' });

    const dbCluster = new DatabaseCluster(this, 'MyDbCluster', {
      ...,
      masterUser: {
        username
        password
      },
      ...
    });

[njlynch]

So here's a quick overview of the options as they exist today:

DatabaseInstanceProps {
  masterUsername: string;
  masterUserPassword?: secretsmanager.SecretValue;
  masterUserPasswordEncryptionKey?: kms.Key;
}

DatabaseInstanceFromSnapshotProps {
  masterUsername: string;  // Can only be specified if generateMasterUserPassword is true
  masterUserPassword?: secretsmanager.SecretValue; // Can only be specified if generateMasterUserPassword is false
  masterUserPasswordEncryptionKey?: kms.Key; // Only applies if generateMasterUserPassword is true
  generateMasterUserPassword?: boolean;
}

DatabaseClusterProps {
  masterUser: Login;
}

Login {
  username: string;
  password?: secretsmanager.SecretValue;
  encryptionKey?: kms.Key;
}

DatabaseClusterFromSnapshotProps {
  // No login-related props
}

Certainly, the inconsistency between the individual properties (on Instances) and the Login interface (for Clusters) is something that could be addressed. As far as this feature request itself, I see four options:

Option 1:

Introduce a new optional property to Login (usernameSecretValue?) and make username optional as well. Introduce logic in the cluster -- and if we port Login over to instances, there as well -- to check that only one value or the other is specified.

Login {
  username?: string;
  usernameSecretValue?: secretsmanager.SecretValue;
  password?: secretsmanager.SecretValue;
  encryptionKey?: kms.Key;
}

This places the burden on the instance & cluster classes to validate everything's ok, and doesn't really seem to add to the aesthetics/usability of the API as a whole (IMO).

Option 2:

Introduce a new optional property to Login to replace all of the above fields with a Secret that has a defined structure:

Login {
  username?: string;
  password?: secretsmanager.SecretValue;
  encryptionKey?: kms.Key;
  secret?: secretsmanager.Secret; // Must have a "username" and "password" field in the secret body.
}

The usage here would be to either specify one (or more) of the existing fields, or just the new secret field, which would be used for the username, password, and key. Once again, the constructs would need to validate the version of the Login they receive has the right mix of optional fields, which is ugly, but this is probably the nicest balance of "backwards compatible" and "enables nicer use for those with existing secrets".

Option 3:

Replace the current Login interface with a new class that would offer a few different options for login.

class Login {
  public static fromSecret(secret: Secret): Login // Assumes secret with "username" and "password" fields.
  public static fromUsername(username: string, password?: SecretValue, key?: Key): Login
  public readonly secret: Secret;
}

DatabaseInstanceProps {
  masterUser?: Login;
}

DatabaseClusterProps {
  masterUser?: Login;
}

This is perhaps the most structured/modeled, but is also a definite breaking change for every CDK RDS customer. It's not clear that the value here is worth the breaking change.

Option 4:

Do nothing; decide that this consistency for applying existing secrets isn't worth the extra (potentially confusing) props or breaking changes.

Open to feedback from the community here; I'll also be conferring internally to see what the team thinks.

[skinny85]

I vote for option 3, and including a nice message in the Changelog explaining how to migrate. I don't think it will be very intrusive 🙂.