fix(docdb): secret rotation ignores excluded characters in password

[markussiebert]

We need to pass whatever excludeCharacters were passed to the generated Secret to the application responsible for the rotation.

Fixes #17347
Fixes #17575

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[gitpod-io]

[skinny85]

Looks good @markussiebert, but I have some questions 🙂.

[skinny85]

Can you explain why do we need this field, if we already have secret?

Also, why is it public? I don't think we read it anywhere outside of this class in this PR?

[markussiebert]

the secret itself was not enough. We need the DatabaseSecret, because only it has the excludeCharacters property.

I tried to. change it to DatabaseSecret and assign it directly, not via attachSecret, but then it would be a breaking change the tests showed:

"SecretId": {
          "Ref": "DatabaseSecretAttachmentE5D1B020"
        },

would change to

"SecretId": {
              "Ref": "DatabaseSecret3B817195"
            },

so i introduced the second variable holding the database secret. But yes - it can be private.

what do you think @skinny85

[markussiebert]

Also, we're missing a new unit test for the non-default case.

Do we? There is no logic handling a custom excludeCharacters string so I thought testing / ensuring that excludeCharacters inside the secret and the secretrotation match would be enough. So I think it won't increase test coverage - Just my thoughts

[skinny85]

Also, we're missing a new unit test for the non-default case.

Pull request has been modified.

[skinny85]

the secret itself was not enough. We need the DatabaseSecret, because only it has the excludeCharacters property.

I tried to. change it to DatabaseSecret and assign it directly, not via attachSecret, but then it would be a breaking change the tests showed:

"SecretId": {
         "Ref": "DatabaseSecretAttachmentE5D1B020"
       },

would change to

"SecretId": {
"Ref": "DatabaseSecret3B817195"
},

so i introduced the second variable holding the database secret. But yes - it can be private.

what do you think @skinny85

What you want to do is create a new, private field, of type DatabaseSecret. Remove the public secret field, replacing it with a getter also called secret that returns the above private field, just the declared type of the getter is Secret, not DatabaseSecret.

Presto, you have only one field.

[markussiebert]

What you want to do is create a new, private field, of type DatabaseSecret. Remove the public secret field, replacing it with a getter also called secret that returns the above private field, just the declared type of the getter is Secret, not DatabaseSecret.

Presto, you have only one field.

@skinny85 Maybe I don't understand, but the rename doesn't happen because of the type mismatch - it will happen, because secret is filled via this call this.databaseSecret.attach(this); or before my changes secret.attach(this); where secret was a variable declared inside the constructor.

So I can imagine creating a getter for secret returning the secretattachment secret (not secret itself - because this would lead to a change in the generated id). So then I would need hold the secret in a private variable (so I can access it via the setter).

Then I tried to rename the DatabaseSecret from 'Secret' to 'SecretAttachment' - but still then the path in the object tree differs and so the generated ID at the end of the CfResourceName...

And attach can't be called several times, so i can't just put a wrapper around the attach mathod - so I have to store it and then i end up with the current state.

@skinny85 can we "meet" somewhere to discuss this? At the moment I see no other breaking option than this.

[skinny85]

@markussiebert Thinking about it some more, can't you just save the excludedCharacters in a private readonly field, instead of the entire DatabaseSecret? That will be much simpler, and will also mean you won't have to make any changes to the DatabaseSecret.

What do you think?

[markussiebert]

I don't like the idea holding redundant information (as i would, if i introduce a second string).

I don't understand the problem with the DatabaseSecret moving from the constructor to the class scope to be accessed later. I would have build it like this from the beginning... but now problem.

So i created a new "try" ... what do you think about this @skinny85

[skinny85]

I don't like the idea holding redundant information (as i would, if i introduce a second string).

I don't understand the problem with the DatabaseSecret moving from the constructor to the class scope to be accessed later. I would have build it like this from the beginning... but now problem.

So i created a new "try" ... what do you think about this @skinny85

I don't think this attempt is correct, because you don't always create a Secret with that ID (there's an if guarding creating that construct). So, when that if is false, your code will crash.

I have to say, I don't understand what you mean by "I don't like the idea holding redundant information". That's exactly what you're doing when saving databaseSecret as a field, no?

Let's do a compromise. Make excludedCharacters in DatabaseSecret internal (call it _excludedCharacters and annotate with @internal), and save that as a field instead of the entire DatabaseSecret.

I really don't think saving both Secret, and DatabaseSecret, is a good idea. It makes the code pretty confusing in my opinion.

[markussiebert]

@skinny85 I can't see a case where this won't work! Apart from that, making the excludedCharaters internal is a good idea.

both, single and multiuser rotation start with the following lines:

if (!this.secret) {
   throw new Error('Cannot add single user rotation for a cluster without secret.');
}

so we know, that there is this.secret set... and looking in the constructor:

if (secret) {
   this.secret = secret.attach(this);
}

It's the only place where this.secret is set and we know this.secret is only set if this secret isn't undefined and ...

// Create the secret manager secret if no password is specified
let secret: DatabaseSecret | undefined;
if (!props.masterUser.password) {
  secret = new DatabaseSecret(this, 'Secret', {
    username: props.masterUser.username,
    encryptionKey: props.masterUser.kmsKey,
    excludeCharacters: props.masterUser.excludeCharacters,
    secretName: props.masterUser.secretName,
  });
}

this is the only case where secret is defined - and it will allways have the ID Secret...

I have to say, I don't understand what you mean by "I don't like the idea holding redundant information". That's exactly what you're doing when saving databaseSecret as a field, no?

with this I mean:

We say that the default value will be "@/ inside the DatabaseSecretProps. And when initializing the DatabaseInstance, we ensure this with:
props.excludeCharacters ?? '"@/'

So, if we say excludedCharaters is another string property inside DatabaseCluster as you suggested, we would have to initialize it exactly the same way (because, we can't read it from the DatabaseSecret, because it's not accessible - no changes to the DatabaseSecret.

So we would store the exact same information in two places and initialize two variables exactly the same way. Thats what i mean redundant information.
My attempt only tries to make the once stored information accessible.

I really don't think saving both Secret, and DatabaseSecret, is a good idea. It makes the code pretty confusing in my opinion.

I agree - so if I won't care about not breaking anything I would like to replace the type of ISecret with DatabasSecert and only attach the Secret, without referencing the secret attachment.
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-secretsmanager-secrettargetattachment.html#aws-resource-secretsmanager-secrettargetattachment-return-values
It's return values content is the same as of the secret itself - but for cdk context paths move and so some resources will be recreated. That won't break anything I hope ;-)

Pull request has been modified.

[skinny85]

Alright, you've convinced me 😉.

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: de14d09
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).