Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(aws_secretsmanager): automaticallyAfter is not disabling the automatic rotation of secrets #27460

Open
mrlikl opened this issue Oct 9, 2023 · 10 comments
Labels
@aws-cdk/aws-secretsmanager Related to AWS Secrets Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p3

Comments

@mrlikl
Copy link
Contributor

mrlikl commented Oct 9, 2023

Describe the bug

As per doc, if the property automaticallyAfter is set to Duration.days(0) then it should disable automatic rotation, however it is removing the property

 RotationRules:
        AutomaticallyAfterDays: 365

from the resource AWS::SecretsManager::RotationSchedule

Expected Behavior

The resource AWS::SecretsManager::RotationSchedule completely removed from the stack template which will make the call API CancelRotateSecret

Current Behavior

It is removing the below property which is not making any change in the secret's rotation.

 RotationRules:
        AutomaticallyAfterDays: 365

Reproduction Steps

secret_test = secretsmanager.Secret(self, "Secret",
                                            secret_object_value={
                                                "username": cdk.SecretValue.unsafe_plain_text("foo"),
                                                "database": cdk.SecretValue.unsafe_plain_text("foo"),
                                            }
                                            )

rotation_fn = _lambda.Function(self, 'RotationFN', runtime=_lambda.Runtime.PYTHON_3_9,
                                       handler='index.handler', code=_lambda.Code.from_inline("hello world"))

secret_test.add_rotation_schedule(
            "SecretRotation", automatically_after=cdk.Duration.days(0), rotate_immediately_on_update=False, rotation_lambda=rotation_fn)

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.97.1

Framework Version

No response

Node.js Version

20.7.0

OS

macos

Language

Python

Language Version

No response

Other information

No response

@mrlikl mrlikl added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 9, 2023
@github-actions github-actions bot added the @aws-cdk/aws-secretsmanager Related to AWS Secrets Manager label Oct 9, 2023
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Oct 9, 2023
@khushail
Copy link
Contributor

Yes @mrlikl , I can confirm that setting duration.days(0) does not disable the rotation schedule and removes the property only. here is a snippet of the deployed stack -

Screenshot 2023-10-09 at 5 15 47 PM

However if you want to disable the rotation schedule, you could proceed with not adding the rotation schedule at all. Marking this bug as P2.

@khushail khushail added p2 effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Oct 10, 2023
@scanlonp
Copy link
Contributor

@mrlikl could you link the docs that state automatic rotation is disabled by setting automaticallyAfter to Duration.days(0)? I found this doc that gives 1 day as a minimum value.

What is the use case for disabling automatic rotation in this way rather than not adding a rotation schedule in the first place?

@scanlonp scanlonp added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 16, 2023
@mrlikl
Copy link
Contributor Author

mrlikl commented Oct 16, 2023

automaticallyAfter? 

Specifies the number of days after the previous rotation before Secrets Manager triggers the next automatic rotation.

A value of zero will disable automatic rotation - Duration.days(0).

Source

While the underlying API states a minimum value of 1, CDK doc had the above-mentioned. Hence, this was raised as a bug.

Removing the rotation schedule is working as expected. Maybe the CDK doc needs to be updated or the AWS::SecretsManager::RotationSchedule resource is removed completely if Duration.days(0)

@scanlonp
Copy link
Contributor

@mrlikl yes, apologies, I was just in the process of changing my comment since I found the previous PR & doc string. I read through this original issue request (here) and the comments on the PR (here).

Correct me if I am wrong, but I believe that the current behavior is working as intended. The 'magic' value on automaticallyAfter was created to remove the RotationRules property on the template, but leave the rotation schedule intact.

Removing the rotation schedule is working as expected.

So this is not affecting the actual rotation of your secrets correct?

I do agree the doc string could be much clearer. This workaround looks to me to be an easy way to give the lambda all the correct permissions and setting up easy manual rotation.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 16, 2023
@go-to-k
Copy link
Contributor

go-to-k commented Oct 17, 2023

@scanlonp

I will submit another PR for modifying the doc.

However, the min value is 1 day according to the api doc, so shouldn't Duration.days(0) reject input by validation?

If so, I will include that process along with the doc modification. If not necessary, or if we should follow the existing behavior, I will only modify the doc.

I was misunderstood. I understood that there is a use case for deleting only RotationRules. I will just modify the document.

@mrlikl
Copy link
Contributor Author

mrlikl commented Oct 17, 2023

@scanlonp, @go-to-k

The 'magic' value on automaticallyAfter was created to remove the RotationRules property on the template, but leave the rotation schedule intact. -> I have tested this and this does not have any impact after the update. The rules, lambda are intact pre and post update.

I believe this is due to behaviour/logic change of AWS::SecretsManager::RotationSchedule handlers. I am having a look into this internally and I will revert by EOD today/tomorrow on this.

I want to confirm whether removing the property AutomaticallyAfterDays used to disable rotation completely earlier.

mergify bot pushed a commit that referenced this issue Oct 18, 2023
…e is 0 is wrong (#27570)

We discussed that the doc when `automaticallyAfter` for `RotationSchedule` is `Duration.days(0)` is wrong. So I modified the doc.

See the issue (#27460) and another PR (#27497 (comment)).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mrlikl
Copy link
Contributor Author

mrlikl commented Oct 23, 2023

Have confirmed internally that the behaviour was the same since beginning. That is removing the property AutomaticallyAfterDays from the resource will not disable the secret rotation.

@mrlikl
Copy link
Contributor Author

mrlikl commented Oct 23, 2023

@go-to-k is the proposal to remove RotationRules if Duration.days(0) ? If so it will be the same as what it is now. As mentioned earlier, the whole resource AWS::SecretsManager::RotationSchedule need to be removed from the template

@go-to-k
Copy link
Contributor

go-to-k commented Oct 23, 2023

@mrlikl

@go-to-k is the proposal to remove RotationRules if Duration.days(0) ? If so it will be the same as what it is now.

Yes, it's the same as what it is now.

At first, I submitted the PR to remove AWS::SecretsManager::RotationSchedule. However, it seems that removing RotationRules instead of RotationSchedule is as expected, see the following thread .

#27497 (comment)

So my PR was closed, and I submitted a new PR for fixing the doc.

#27570

@go-to-k
Copy link
Contributor

go-to-k commented Oct 23, 2023

@mrlikl

It seems that there is a use case for deleting RotationRules only.

@pahud pahud removed the p2 label Jun 11, 2024
@kellertk kellertk added the p3 label Jul 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-secretsmanager Related to AWS Secrets Manager bug This issue is a bug. effort/medium Medium work item – several days of effort p3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants