Repository navigation
feat(docdb): add managed password support - #35711
Conversation
|
|
||||||||||||||
|
|
||||||||||||||
| // Create the secret manager secret if no password is specified | ||
| // Validate manageMasterUserPassword constraints | ||
| if (props.manageMasterUserPassword && props.masterUser.password) { | ||
| throw new ValidationError('You can\'t manage the master user password with AWS Secrets Manager if masterUser.password is specified', this); | ||
| } | ||
|
|
||
| if (props.masterUserSecretKmsKey && !props.manageMasterUserPassword) { | ||
| throw new ValidationError('masterUserSecretKmsKey is valid only if manageMasterUserPassword is true', this); | ||
| } | ||
|
|
||
| if (props.rotateMasterUserPassword && !props.manageMasterUserPassword) { | ||
| throw new ValidationError('rotateMasterUserPassword is valid only if manageMasterUserPassword is true', this); | ||
| } |
There was a problem hiding this comment.
These ValidationError calls are missing the lit tagged template error code that all other ValidationError calls in this file use. This enforces compile-time literal error codes for structured error handling.
For reference, see line 519:
throw new ValidationError(lit\`InstanceTypeOrServerlessConfigurationRequired\`, '...', this);
| * Specifies whether to rotate the secret managed by AWS Secrets Manager for the master user password. | ||
| * | ||
| * This setting is valid only if the master user password is managed by Amazon DocumentDB in AWS Secrets Manager for the cluster. | ||
| * The secret value contains the updated password. | ||
| * | ||
| * When you rotate the master user password, the change must be applied immediately. |
There was a problem hiding this comment.
Does this property only trigger a one-time rotation ?
If yes, could we update this JSDoc to be clearer:
/**
* Specifies whether to trigger an immediate one-time rotation of the master user password
* managed by AWS Secrets Manager.
*
* This does NOT establish automatic scheduled rotation - Amazon DocumentDB automatically
* rotates the managed secret every 7 days by default. Set this to `true` to trigger
* an additional immediate rotation during the current cluster update.
*
* This setting is valid only if `manageMasterUserPassword` is true.
*
* @see https://docs.aws.amazon.com/documentdb/latest/devguide/docdb-secrets-manager.html
* @default false
*/There was a problem hiding this comment.
Yes, it's a one-time immediate rotation. One additional finding: RotateMasterUserPassword exists only on ModifyDBCluster (not on CreateDBCluster), so it has no effect at cluster creation — I confirmed via CloudTrail that creation events are identical with and without the flag (the rotation observed right after creation is part of the managed secret initialization). Updated the JSDoc based on your suggestion with this addition, and adjusted the README example to apply the flag to an existing cluster.
| /** | ||
| * The AWS KMS key to encrypt a secret that is automatically generated and managed in AWS Secrets Manager. | ||
| * | ||
| * This setting is valid only if the master user password is managed by Amazon DocumentDB in AWS Secrets Manager for the DB cluster. |
There was a problem hiding this comment.
nit: Maybe we can make this clearer -
This setting is valid only if the master user password is managed by Amazon DocumentDB in AWS Secrets Manager for the DB cluster (i.e. when manageMasterUserPassword is true).
There was a problem hiding this comment.
Clarified as suggested.
| * | ||
| * This setting is valid only if the master user password is managed by Amazon DocumentDB in AWS Secrets Manager for the DB cluster. | ||
| * If you don't specify this property, then the `aws/secretsmanager` KMS key is used to encrypt the secret. | ||
| * |
There was a problem hiding this comment.
can we add the documentation link -
* @see https://docs.aws.amazon.com/documentdb/latest/devguide/docdb-secrets-manager.html
| * This provides enhanced security and automatic password rotation capabilities. | ||
| * | ||
| * Constraint: You can't manage the master user password with AWS Secrets Manager if `masterUser.password` is specified. | ||
| * |
There was a problem hiding this comment.
can we add the documentation link -
* @see https://docs.aws.amazon.com/documentdb/latest/devguide/docdb-secrets-manager.html
|
|
||
| new IntegTest(app, 'aws-cdk-docdb-cluster-managed-password-integ', { | ||
| testCases: [stack], | ||
| }); |
There was a problem hiding this comment.
Could we also verify that the managed secret was created using an awsApiCall assertion (for e.g., DescribeDBClusters to check MasterUserSecret is populated) ?
| if (secret) { | ||
| this.secret = secret.attach(this); | ||
| } |
There was a problem hiding this comment.
When manageMasterUserPassword is true, the this.secret property remains undefined, which means users cannot use cluster.secret?.grantRead(role) to grant their applications access to the managed credential.
CloudFormation exposes the managed secret ARN via AWS::DocDB::DBCluster attribute MasterUserSecret.SecretArn. Could we expose it so users can use grants on it?
if (secret) {
this.secret = secret.attach(this);
} else if (props.manageMasterUserPassword) {
this.secret = secretsmanager.Secret.fromSecretCompleteArn(
this,
'ManagedSecret',
this.cluster.attrMasterUserSecretSecretArn,
);
}There was a problem hiding this comment.
I investigated, and this turned out not to be implementable this way: unlike RDS, AWS::DocDB::DBCluster does not support Fn::GetAtt for MasterUserSecret.SecretArn (confirmed in both the CFN resource spec and the docs), so attrMasterUserSecretSecretArn does not exist on the L1. I also verified on a real cluster that the secret name is rds!cluster-<random UUID>, so the ARN cannot be constructed from CFN-accessible attributes either.
Instead, I documented a deploy-time lookup using AwsCustomResource in the README (the same approach cloudfront-origins takes for the VPC origin security group, whose id CloudFormation doesn't provide) and verified it works with a real deployment.
| ManageMasterUserPassword: true, | ||
| MasterUsername: 'admin', | ||
| MasterUserPassword: Match.absent(), | ||
| }); |
There was a problem hiding this comment.
When manageMasterUserPassword is true, the construct should skip creating its own secret (DatabaseSecret) - let's add an assertion to verify that AWS::SecretsManager::Secret resource is not created:
Template.fromStack(stack).resourceCountIs('AWS::SecretsManager::Secret', 0);
|
|
||
| ## AWS Secrets Manager Integration | ||
|
|
||
| DocumentDB clusters can integrate with AWS Secrets Manager to automatically manage master user passwords. This provides enhanced security through automatic password generation and rotation capabilities. |
There was a problem hiding this comment.
We can add these details to make it clearer:
Note: By default, CDK creates and manages a Secrets Manager secret for the master password (with
rotation via Lambda functions using addRotationSingleUser()). The manageMasterUserPassword option
delegates password management entirely to the DocumentDB service, which includes built-in automatic
rotation every 7 days without requiring Lambda functions.
There was a problem hiding this comment.
Added. One adjustment: rotation of the default secret is not automatic — it must be configured explicitly with addRotationSingleUser() — so I worded that part accordingly.
There was a problem hiding this comment.
Have you tested this ?
Does the user need to use addRotationSingleUser to enable rotation of the managed secret ? If yes, currently addRotationSingleUser or addRotationMultiUser will not work if manageMasterUserPassword is true because this.secret will be undefined - https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-docdb/lib/cluster.ts#L741
How do we provide users a way to enable rotation ?
There was a problem hiding this comment.
That README sentence describes the default setup where the construct creates the secret. In that case this.secret is set and addRotationSingleUser() works.
For the managed secret, addRotationSingleUser() is not needed. Rotation is enabled from the start and the service rotates the password every 7 days (see How Amazon DocumentDB uses AWS Secrets Manager).
You are right that calling addRotationSingleUser() / addRotationMultiUser() with manageMasterUserPassword: true fails with a misleading message. I added the same dedicated validation error as the RDS L2.
| (secret ? secret.secretValueFromJson('password').unsafeUnwrap() : props.masterUser.password!.unsafeUnwrap()), | ||
| // ManageMasterUserPassword | ||
| manageMasterUserPassword: props.manageMasterUserPassword, | ||
| masterUserSecretKmsKeyId: props.masterUserSecretKmsKey?.keyArn, |
There was a problem hiding this comment.
Could you verify if masterUserSecretKmsKey needs to grant KMS permissions to Secrets Manager service principal for runtime credential retrieval by creating the cluster and trying some operations on it ? If yes, we would need to add something like this (with the appropriate scoped-down permissions) - https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-secretsmanager/lib/secret.ts#L732-L735
There was a problem hiding this comment.
Verified on a real cluster. With a CMK whose key policy is the default (account root only), both secret creation and rotation succeeded without any key policy changes. Service-managed secrets work through KMS grants that the service creates on the key itself, so the grants in secret.ts:732-735 (which are for CDK-owned secrets) aren't needed here. The RDS L2 likewise adds nothing for manageMasterUserPassword.
|
@mazyu36 Any update on this PR ? |
|
@gudipati Sorry, I hadn’t gotten to this yet. I’ll address your review comments soon. |
| * module README for how to access the managed secret. | ||
| * | ||
| * @see https://docs.aws.amazon.com/documentdb/latest/developerguide/docdb-secrets-manager.html | ||
| * @default false |
There was a problem hiding this comment.
What's the reason for not making the default behaviour the service to manage the password?
There was a problem hiding this comment.
Managed passwords are not a strict superset of the existing behavior, so this is opt-in. The construct-created secret has capabilities the managed one does not: you can set the secret name and excluded characters, choose the rotation interval with addRotationSingleUser(), use alternating-user rotation with addRotationMultiUser(), and it works with global databases and cross-Region read replicas (managed passwords are not supported for those configurations).
Switching the default would also change the behavior of existing stacks, so it would need a feature flag.
There was a problem hiding this comment.
Yes totally agree that there's a use case for the non managed one over the managed one, but have a feeling the one who will use the non managed one is more advanced user that will be aware of all of that, so they will have enough knowledge to explicitly choose what they want vs the beginner user who doesn't want to go in all those complications.
Yes it needs a feature flag, since it's anyway if we did it now or in the future we will need a feature flag, we can leave this one out.
| * @see https://docs.aws.amazon.com/documentdb/latest/developerguide/docdb-secrets-manager.html | ||
| * @default false | ||
| */ | ||
| readonly rotateMasterUserPassword?: boolean; |
There was a problem hiding this comment.
Is there any useful use case for this option? If not i prefer to leave it for the future, just to make it easier for customers to understand this interface since a lot of those props depend on each other which make it very mental exhausting to understand what to choose
What i mean by depend on each other rotateMasterUserPassword needs to have manageMasterUserPassword then manageMasterUserPassword needs to not pass password, etc...
There was a problem hiding this comment.
Agreed, I removed it from this PR. The property maps to a modify-only API parameter, so it has no effect at cluster creation, and its only use is forcing a one-time rotation on an existing cluster. We can add it in a separate PR if there is demand for it.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
40d5d14 to
3c98cac
Compare
3c98cac to
1a785d6
Compare
Merge Queue Status
This pull request spent 12 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status
This pull request spent 2 hours 29 minutes 25 seconds in the queue, including 56 minutes 16 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #.
Reason for this change
This change adds support for AWS Secrets Manager managed passwords in DocumentDB clusters.
This feature provides enhanced security by allowing AWS DocumentDB to automatically generate, manage, and rotate master user passwords using AWS Secrets Manager, eliminating the need for manual password management.
Description of changes
manageMasterUserPassword: boolean- Enables AWS Secrets Manager to manage the master user passwordmasterUserSecretKmsKey: kms.IKey- Specifies a custom KMS key to encrypt the managed secretrotateMasterUserPassword: boolean- Triggers immediate password rotation for managed secretsDescribe any new or updated permissions being added
Description of how you validated changes
Add unit tests and an integ test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license