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

Periodically rotate etcd storage account secret #984

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndreasBurger
Copy link
Member

How to categorize this PR?

/area security
/kind enhancement
/platform azure

What this PR does / why we need it:
Rotate the key used to authenticate with the storage account for etcd-backups periodically.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Release note:

Credentials used for ETCD backups will now be periodically rotated.

@gardener-robot gardener-robot added area/security Security related kind/enhancement Enhancement, improvement, extension platform/azure Microsoft Azure platform/infrastructure needs/review Needs review size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) labels Oct 10, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 10, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 10, 2024
@AndreasBurger AndreasBurger force-pushed the rotate-etcd-credentials branch from 519a744 to 33be110 Compare October 16, 2024 15:34
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 16, 2024
@AndreasBurger AndreasBurger force-pushed the rotate-etcd-credentials branch from 33be110 to afe351d Compare October 21, 2024 11:37
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2024
@AndreasBurger AndreasBurger force-pushed the rotate-etcd-credentials branch from afe351d to 5ae1574 Compare October 21, 2024 11:38
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 21, 2024
@AndreasBurger AndreasBurger force-pushed the rotate-etcd-credentials branch from 5ae1574 to 9f42b83 Compare October 22, 2024 14:26
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2024
@AndreasBurger AndreasBurger marked this pull request as ready for review October 22, 2024 14:27
@AndreasBurger AndreasBurger requested review from a team as code owners October 22, 2024 14:27
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 22, 2024
@AndreasBurger
Copy link
Member Author

Some small last-minute changes, nothing functional so good to review. Will run another final test after reviews are done.

response, err := c.client.ListKeys(ctx, resourceGroupName, storageAccountName, &armstorage.AccountsClientListKeysOptions{
// doc: "Specifies type of the key to be listed. Possible value is kerb.. Specifying any value will set the value to kerb."
Expand: ptr.To("kerb"),
})

if err != nil {
return "", err
return nil, err
}

if len(response.Keys) < 1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit: but don't we expect the Keys to have a length of 2?

@@ -64,3 +75,7 @@ func (a *actuator) getBackupBucketGeneratedSecret(ctx context.Context, backupBuc
}
return secret, nil
}

func shouldBeRotated(secret corev1.Secret) (bool, error) {
return secret.CreationTimestamp.Time.Before(time.Now().AddDate(0, 0, -14)), nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this configurable ?

@gardener-robot gardener-robot added the needs/rebase Needs git rebase label Nov 11, 2024
@gardener-robot
Copy link

@AndreasBurger You need rebase this pull request with latest master branch. Please check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security Security related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/rebase Needs git rebase needs/review Needs review platform/azure Microsoft Azure platform/infrastructure size/m Size of pull request is medium (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants