diff --git a/pkg/azure/client/storage.go b/pkg/azure/client/storage.go index 4415bad82..9332c5780 100644 --- a/pkg/azure/client/storage.go +++ b/pkg/azure/client/storage.go @@ -65,12 +65,8 @@ func NewBlobStorageClient(_ context.Context, storageAccountName, storageAccountK return &BlobStorageClient{blobclient}, err } -// NewBlobStorageClientFromSecretRef creates a client for an Azure Blob storage by reading auth information from secret reference. -func NewBlobStorageClientFromSecretRef(ctx context.Context, client client.Client, secretRef *corev1.SecretReference) (*BlobStorageClient, error) { - secret, err := extensionscontroller.GetSecretByReference(ctx, client, secretRef) - if err != nil { - return nil, err - } +// NewBlobStorageClientFromSecretRef creates a client for an Azure Blob storage by reading auth information from a secret. +func NewBlobStorageClientFromSecret(ctx context.Context, client client.Client, secret *corev1.Secret) (*BlobStorageClient, error) { storageAccountName, ok := secret.Data[azure.StorageAccount] if !ok { return nil, fmt.Errorf("secret %s/%s doesn't have a storage account", secret.Namespace, secret.Name) @@ -89,6 +85,15 @@ func NewBlobStorageClientFromSecretRef(ctx context.Context, client client.Client return NewBlobStorageClient(ctx, string(storageAccountName), string(storageAccountKey), storageDomain) } +// NewBlobStorageClientFromSecretRef creates a client for an Azure Blob storage by reading auth information from secret reference. +func NewBlobStorageClientFromSecretRef(ctx context.Context, client client.Client, secretRef *corev1.SecretReference) (*BlobStorageClient, error) { + secret, err := extensionscontroller.GetSecretByReference(ctx, client, secretRef) + if err != nil { + return nil, err + } + return NewBlobStorageClientFromSecret(ctx, client, secret) +} + // DeleteObjectsWithPrefix deletes the blob objects with the specific from . // If it does not exist, no error is returned. func (c *BlobStorageClient) DeleteObjectsWithPrefix(ctx context.Context, container, prefix string) error { diff --git a/pkg/azure/client/storageaccount.go b/pkg/azure/client/storageaccount.go index 4d07e0dce..845c91e63 100644 --- a/pkg/azure/client/storageaccount.go +++ b/pkg/azure/client/storageaccount.go @@ -54,19 +54,69 @@ func (c *StorageAccountClient) CreateStorageAccount(ctx context.Context, resourc // ListStorageAccountKey lists the first key of a storage account. func (c *StorageAccountClient) ListStorageAccountKey(ctx context.Context, resourceGroupName, storageAccountName string) (string, error) { + keys, err := c.listStorageAccountKeys(ctx, resourceGroupName, storageAccountName) + if err != nil { + return "", err + } + return *keys[0].Value, nil +} + +// ListStorageAccountKeys lists the keys of a storage account. +func (c *StorageAccountClient) listStorageAccountKeys(ctx context.Context, resourceGroupName, storageAccountName string) ([]*armstorage.AccountKey, error) { 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 { - return "", fmt.Errorf("no key found in storage account %s", storageAccountName) + return nil, fmt.Errorf("no key found in storage account %s", storageAccountName) + } + + return response.Keys, nil +} + +// RotateKey performs a partial key rotation for the given storage account, using the property of Azure storage accounts always having exactly two keys. +// The existing storage keys are checked against the given one, and the one that does _not_ match it will be regenerated and its new value returned. +// This ensures the given key still being valid (until the next rotation) which ensures that there is time for the updated value to propagate. +func (c *StorageAccountClient) RotateKey(ctx context.Context, resourceGroupName, storageAccountName, storageAccountKey string) (string, error) { + keys, err := c.listStorageAccountKeys(ctx, resourceGroupName, storageAccountName) + if err != nil { + return "", err + } + + var keyToRotate *string + switch { + case *keys[0].Value == storageAccountKey: + keyToRotate = keys[1].KeyName + case *keys[1].Value == storageAccountKey: + keyToRotate = keys[0].KeyName + default: + return "", fmt.Errorf("unable to find given key in storage account keys") } - firstKey := response.Keys[0] - return *firstKey.Value, nil + resp, err := c.client.RegenerateKey( + ctx, + resourceGroupName, + storageAccountName, + armstorage.AccountRegenerateKeyParameters{KeyName: keyToRotate}, + nil, + ) + + if err != nil { + return "", err + } + + // This might be a bit paranoid + for _, k := range resp.Keys { + if k.KeyName == keyToRotate { + return *k.Value, nil + } + } + + return "", fmt.Errorf("error rotating storage account key '%v'", keyToRotate) + } diff --git a/pkg/azure/client/types.go b/pkg/azure/client/types.go index d9e8c176e..6f9dad7e7 100644 --- a/pkg/azure/client/types.go +++ b/pkg/azure/client/types.go @@ -140,6 +140,7 @@ type VirtualNetwork interface { type StorageAccount interface { CreateStorageAccount(context.Context, string, string, string) error ListStorageAccountKey(context.Context, string, string) (string, error) + RotateKey(context.Context, string, string, string) (string, error) } // DNSZone represents an Azure DNS zone k8sClient. diff --git a/pkg/controller/backupbucket/actuator.go b/pkg/controller/backupbucket/actuator.go index 4673b3512..b669da417 100644 --- a/pkg/controller/backupbucket/actuator.go +++ b/pkg/controller/backupbucket/actuator.go @@ -8,6 +8,7 @@ import ( "context" "fmt" + extensionscontroller "github.com/gardener/gardener/extensions/pkg/controller" "github.com/gardener/gardener/extensions/pkg/controller/backupbucket" "github.com/gardener/gardener/extensions/pkg/util" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" @@ -17,12 +18,13 @@ import ( "github.com/gardener/gardener-extension-provider-azure/pkg/apis/azure" "github.com/gardener/gardener-extension-provider-azure/pkg/apis/azure/helper" + azuretypes "github.com/gardener/gardener-extension-provider-azure/pkg/azure" azureclient "github.com/gardener/gardener-extension-provider-azure/pkg/azure/client" ) var ( // DefaultBlobStorageClient is the default function to get a backupbucket client. Can be overridden for tests. - DefaultBlobStorageClient = azureclient.NewBlobStorageClientFromSecretRef + DefaultBlobStorageClient = azureclient.NewBlobStorageClientFromSecret ) type actuator struct { @@ -58,6 +60,16 @@ func (a *actuator) Reconcile(ctx context.Context, _ logr.Logger, backupBucket *e return err } + bucketCloudConfiguration, err := azureclient.CloudConfiguration(backupConfig.CloudConfiguration, &backupBucket.Spec.Region) + if err != nil { + return err + } + + storageDomain, err := azureclient.BlobStorageDomainFromCloudConfiguration(bucketCloudConfiguration) + if err != nil { + return fmt.Errorf("failed to determine blob storage service domain: %w", err) + } + // If the generated secret in the backupbucket status not exists that means // no backupbucket exists and it need to be created. if backupBucket.Status.GeneratedSecretRef == nil { @@ -65,26 +77,45 @@ func (a *actuator) Reconcile(ctx context.Context, _ logr.Logger, backupBucket *e if err != nil { return util.DetermineError(err, helper.KnownCodes) } - - bucketCloudConfiguration, err := azureclient.CloudConfiguration(backupConfig.CloudConfiguration, &backupBucket.Spec.Region) - if err != nil { - return err - } - - storageDomain, err := azureclient.BlobStorageDomainFromCloudConfiguration(bucketCloudConfiguration) - if err != nil { - return fmt.Errorf("failed to determine blob storage service domain: %w", err) - } // Create the generated backupbucket secret. if err := a.createBackupBucketGeneratedSecret(ctx, backupBucket, storageAccountName, storageAccountKey, storageDomain); err != nil { return util.DetermineError(err, helper.KnownCodes) } } - blobStorageClient, err := DefaultBlobStorageClient(ctx, a.client, backupBucket.Status.GeneratedSecretRef) + backupSecret, err := extensionscontroller.GetSecretByReference(ctx, a.client, backupBucket.Status.GeneratedSecretRef) + if err != nil { + return err + } + + blobStorageClient, err := DefaultBlobStorageClient(ctx, a.client, backupSecret) if err != nil { return util.DetermineError(err, helper.KnownCodes) } + + doRotation, err := shouldBeRotated(*backupSecret) + if err == nil { + return err + } + + if doRotation { + newKey, err := rotateStorageAccountCredentials(ctx, factory, backupBucket, string(backupSecret.Data[azuretypes.StorageKey])) + if err != nil { + return err + } + oldSecretRef := backupBucket.Status.GeneratedSecretRef + // Generate new secret + if err := a.createBackupBucketGeneratedSecret(ctx, backupBucket, getStorageAccountName(backupBucket), newKey, storageDomain); err != nil { + return util.DetermineError(err, helper.KnownCodes) + } + // Clean up old secret + err = a.deleteBackupBucketGeneratedSecretByRef(ctx, oldSecretRef) + if err != nil { + // TODO: Is this worth erroring out? Warning might suffice, there is only one leftover soon-to-be-outdated secret + return err + } + } + return util.DetermineError(blobStorageClient.CreateContainerIfNotExists(ctx, backupBucket.Name), helper.KnownCodes) } @@ -126,7 +157,7 @@ func (a *actuator) delete(ctx context.Context, _ logr.Logger, backupBucket *exte if secret != nil { // Get a storage account client to delete the backup container in the storage account. - storageClient, err := DefaultBlobStorageClient(ctx, a.client, backupBucket.Status.GeneratedSecretRef) + storageClient, err := DefaultBlobStorageClient(ctx, a.client, secret) if err != nil { return err } diff --git a/pkg/controller/backupbucket/backupbucket.go b/pkg/controller/backupbucket/backupbucket.go index fbc0ee5b6..b95c2d8c4 100644 --- a/pkg/controller/backupbucket/backupbucket.go +++ b/pkg/controller/backupbucket/backupbucket.go @@ -16,11 +16,12 @@ import ( azureclient "github.com/gardener/gardener-extension-provider-azure/pkg/azure/client" ) +func getStorageAccountName(backupBucket *extensionsv1alpha1.BackupBucket) string { + return fmt.Sprintf("bkp%s", utils.ComputeSHA256Hex([]byte(backupBucket.Name))[:15]) +} + func ensureBackupBucket(ctx context.Context, factory azureclient.Factory, backupBucket *extensionsv1alpha1.BackupBucket) (string, string, error) { - var ( - backupBucketNameSha = utils.ComputeSHA256Hex([]byte(backupBucket.Name)) - storageAccountName = fmt.Sprintf("bkp%s", backupBucketNameSha[:15]) - ) + storageAccountName := getStorageAccountName(backupBucket) // Get resource group client to ensure resource group to host backup storage account exists. groupClient, err := factory.Group() @@ -42,7 +43,7 @@ func ensureBackupBucket(ctx context.Context, factory azureclient.Factory, backup return "", "", err } - // Get the key of the storage account. + // Get a key of the storage account. We simply use the first one for new storage accounts (later rotations may change the "current" one). storageAccountKey, err := storageAccountClient.ListStorageAccountKey(ctx, backupBucket.Name, storageAccountName) if err != nil { return "", "", err @@ -50,3 +51,21 @@ func ensureBackupBucket(ctx context.Context, factory azureclient.Factory, backup return storageAccountName, storageAccountKey, nil } + +func rotateStorageAccountCredentials( + ctx context.Context, + factory azureclient.Factory, + backupBucket *extensionsv1alpha1.BackupBucket, + storageAccountKey string, +) (string, error) { + var ( + resourceGroupName = backupBucket.Name + backupBucketNameSha = utils.ComputeSHA256Hex([]byte(backupBucket.Name)) + storageAccountName = fmt.Sprintf("bkp%s", backupBucketNameSha[:15]) + ) + storageAccountClient, err := factory.StorageAccount() + if err != nil { + return "", err + } + return storageAccountClient.RotateKey(ctx, resourceGroupName, storageAccountName, storageAccountKey) +} diff --git a/pkg/controller/backupbucket/secret.go b/pkg/controller/backupbucket/secret.go index c81e82daa..0a6a92e60 100644 --- a/pkg/controller/backupbucket/secret.go +++ b/pkg/controller/backupbucket/secret.go @@ -7,8 +7,10 @@ package backupbucket import ( "context" "fmt" + "time" extensionsv1alpha1 "github.com/gardener/gardener/pkg/apis/extensions/v1alpha1" + "github.com/gardener/gardener/pkg/utils" kutil "github.com/gardener/gardener/pkg/utils/kubernetes" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -19,9 +21,10 @@ import ( ) func (a *actuator) createBackupBucketGeneratedSecret(ctx context.Context, backupBucket *extensionsv1alpha1.BackupBucket, storageAccountName, storageKey, storageDomain string) error { + timestampHash := utils.ComputeSHA256Hex([]byte(time.Now().Format(time.RFC3339))) var generatedSecret = &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: fmt.Sprintf("generated-bucket-%s", backupBucket.Name), + Name: fmt.Sprintf("generated-bucket-%s-%s", backupBucket.Name, timestampHash), Namespace: "garden", }, } @@ -50,7 +53,15 @@ func (a *actuator) deleteBackupBucketGeneratedSecret(ctx context.Context, backup if backupBucket.Status.GeneratedSecretRef == nil { return nil } - return kutil.DeleteSecretByReference(ctx, a.client, backupBucket.Status.GeneratedSecretRef) + return a.deleteBackupBucketGeneratedSecretByRef(ctx, backupBucket.Status.GeneratedSecretRef) +} + +// deleteBackupBucketGeneratedSecretByRef deletes generated the secret pointed to by the provided secretRef. +func (a *actuator) deleteBackupBucketGeneratedSecretByRef(ctx context.Context, secretRef *corev1.SecretReference) error { + if secretRef == nil { + return fmt.Errorf("passed secret ref must not be nil") + } + return kutil.DeleteSecretByReference(ctx, a.client, secretRef) } // getBackupBucketGeneratedSecret get generated secret referred by core BackupBucket resource in garden. @@ -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 +}