Merge pull request #1158 from frodopwns/access-policy-postpone

decouple access policy creation from key vault creation

api/v1alpha1/keyvault_types.go

@@ -9,7 +9,7 @@ import (
 
 // KeyVaultSpec defines the desired state of KeyVault
 type KeyVaultSpec struct {
-	Location         string               `json:"location"`
+	Location string `json:"location"`
 	// +kubebuilder:validation:Pattern=^[-\w\._\(\)]+$
 	// +kubebuilder:validation:MinLength:1
 	// +kubebuilder:validation:Required
@@ -36,6 +36,9 @@ type AccessPolicyEntry struct {
 	TenantID string `json:"tenantID,omitempty"`
 	// ClientID - The client ID of a user, service principal or security group in the Azure Active Directory tenant for the vault. The client ID must be unique for the list of access policies.
 	ClientID string `json:"clientID,omitempty"`
+	// ObjectID is the value to use if the access policy is for a user other than the user creating the Key Vault when the creating user does not have access to the Application API which is used to translate ClientID to Object ID
+	// To get around this, use az-cli or the Azure portal to source the ObjectID from your Service Principal
+	ObjectID string `json:"objectID,omitempty"`
 	// ApplicationID -  Application ID of the client making request on behalf of a principal
 	ApplicationID string `json:"applicationID,omitempty"`
 	// Permissions - Permissions the identity has for keys, secrets, and certificates.

config/samples/azure_v1alpha1_keyvault.yaml

@@ -24,7 +24,12 @@ spec:
   accessPolicies:
     - tenantID: <tenantID>
       clientID: <clientID>
-      applicationID: <appID>
+      # applicationID and objectID are optional replacements for clientID
+      # Use clientID when the access you are providing is for the same Service Principal who created the Key Vault. An access policy actually needs the ObjectID but with proper permissions we can translate clientID to ObjectID.
+      # Use applicationID when providing access to a managed identity
+      # Use objectID when the service principal who needs access is not the same as the one the operator used to create the Key Vault. The permissions a service principal needs to translate clientID to objectID for other SPs is astronomical.
+      # applicationID: <appID>
+      # objectID: <application registration's object id>
       permissions:
         keys: # backup create decrypt delete encrypt get import list purge recover restore sign unwrapKey update verify wrapKey
           - list
@@ -37,4 +42,4 @@ spec:
           - get
         storage: # backup delete deleteas get getas list listsas purge recover regeneratekey restore set setas update
           - list
-          - get
+          - get

pkg/errhelp/errhelp.go

@@ -9,14 +9,34 @@ import (
 	"strings"
 )
 
+// ErrIdsRegex is used to find and remove uuids from errors
+var ErrIdsRegex *regexp.Regexp
+
+// ErrTimesRegex allows timestamp seconds to be removed from error strings
+var ErrTimesRegex *regexp.Regexp
+
 // StripErrorIDs takes an error and returns its string representation after filtering some common ID patterns
 func StripErrorIDs(err error) string {
 	patterns := []string{
 		"RequestID=",
 		"CorrelationId:\\s",
 		"Tracking ID: ",
+		"requestId",
+	}
+
+	if ErrIdsRegex == nil {
+		ErrIdsRegex = regexp.MustCompile(fmt.Sprintf(`(%s)\S+`, strings.Join(patterns, "|")))
+	}
+
+	return ErrIdsRegex.ReplaceAllString(err.Error(), "")
+
+}
+
+// StripErrorTimes removes the hours:minutes:seconds from a date to prevent updates to Status.Message from changing unnecessarily
+func StripErrorTimes(err string) string {
+	if ErrTimesRegex == nil {
+		ErrTimesRegex = regexp.MustCompile(`(T\d\d:\d\d:\d\d)\"`)
 	}
-	reg := regexp.MustCompile(fmt.Sprintf(`(%s)\S+`, strings.Join(patterns, "|")))
-	return reg.ReplaceAllString(err.Error(), "")
+	return ErrTimesRegex.ReplaceAllString(err, "")
 
 }

pkg/resourcemanager/keyvaults/keyvault.go

@@ -6,6 +6,7 @@ package keyvaults
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"strings"
 	"time"
 
@@ -48,20 +49,20 @@ func getVaultsClient() (keyvault.VaultsClient, error) {
 	return vaultsClient, nil
 }
 
-func getObjectID(ctx context.Context, tenantID string, clientID string) *string {
+func getObjectID(ctx context.Context, tenantID string, clientID string) (*string, error) {
 	appclient := auth.NewApplicationsClient(tenantID)
 	a, err := iam.GetGraphAuthorizer()
 	if err != nil {
-		return nil
+		return nil, err
 	}
 	appclient.Authorizer = a
 	appclient.AddToUserAgent(config.UserAgent())
 
 	result, err := appclient.GetServicePrincipalsIDByAppID(ctx, clientID)
 	if err != nil {
-		return nil
+		return nil, err
 	}
-	return result.Value
+	return result.Value, nil
 }
 
 // ParseNetworkPolicy - helper function to parse network policies from Kubernetes spec
@@ -210,9 +211,14 @@ func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context)
 	}
 
 	if policy.ClientID != "" {
-		if objID := getObjectID(ctx, policy.TenantID, policy.ClientID); objID != nil {
-			newEntry.ObjectID = objID
+		objID, err := getObjectID(ctx, policy.TenantID, policy.ClientID)
+		if err != nil {
+			return keyvault.AccessPolicyEntry{}, err
 		}
+		newEntry.ObjectID = objID
+
+	} else if policy.ObjectID != "" {
+		newEntry.ObjectID = &policy.ObjectID
 	}
 
 	return newEntry, nil
@@ -250,7 +256,7 @@ func InstantiateVault(ctx context.Context, vaultName string, containsUpdate bool
 }
 
 // CreateVault creates a new key vault
-func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (keyvault.Vault, error) {
+func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, vaultExists bool) (keyvault.Vault, error) {
 	vaultName := instance.Name
 	location := instance.Spec.Location
 	groupName := instance.Spec.ResourceGroup
@@ -290,10 +296,11 @@ func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 		keyVaultSku.Name = keyvault.Premium
 	}
 
+	pols := []keyvault.AccessPolicyEntry{}
 	params := keyvault.VaultCreateOrUpdateParameters{
 		Properties: &keyvault.VaultProperties{
 			TenantID:         &id,
-			AccessPolicies:   &accessPolicies,
+			AccessPolicies:   &pols,
 			Sku:              &keyVaultSku,
 			NetworkAcls:      &networkAcls,
 			EnableSoftDelete: &enableSoftDelete,
@@ -302,7 +309,14 @@ func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
 		Tags:     tags,
 	}
 
+	if vaultExists {
+		params.Properties.AccessPolicies = &accessPolicies
+	}
+
 	future, err := vaultsClient.CreateOrUpdate(ctx, groupName, vaultName, params)
+	if err != nil {
+		return keyvault.Vault{}, err
+	}
 
 	return future.Result(vaultsClient)
 }
@@ -330,7 +344,11 @@ func (k *azureKeyVaultManager) CreateVaultWithAccessPolicies(ctx context.Context
 		},
 	}
 	if clientID != "" {
-		if objID := getObjectID(ctx, config.TenantID(), clientID); objID != nil {
+		objID, err := getObjectID(ctx, config.TenantID(), clientID)
+		if err != nil {
+			return keyvault.Vault{}, err
+		}
+		if objID != nil {
 			ap.ObjectID = objID
 			apList = append(apList, ap)
 		}
@@ -382,22 +400,19 @@ func (k *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o
 		return true, err
 	}
 
-	// hash the spec and set if new
+	// hash the spec
 	hash := helpers.Hash256(instance.Spec)
-	if instance.Status.SpecHash == "" {
-		instance.Status.SpecHash = hash
-	}
 
 	// convert kube labels to expected tag format
 	labels := helpers.LabelsToTags(instance.GetLabels())
 
 	instance.Status.Provisioning = true
 	instance.Status.FailedProvisioning = false
-
+	exists := false
 	// Check if this KeyVault already exists and its state if it does.
-
 	keyvault, err := k.GetVault(ctx, instance.Spec.ResourceGroup, instance.Name)
 	if err == nil {
+		exists = true
 		if instance.Status.SpecHash == hash {
 			instance.Status.Message = resourcemanager.SuccessMsg
 			instance.Status.Provisioned = true
@@ -408,74 +423,93 @@ func (k *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o
 
 		instance.Status.SpecHash = hash
 		instance.Status.ContainsUpdate = true
+
 	}
 
 	keyvault, err = k.CreateVault(
 		ctx,
 		instance,
 		instance.Spec.Sku,
 		labels,
+		exists,
 	)
-
 	if err != nil {
-		// let the user know what happened
-		instance.Status.Message = err.Error()
-		instance.Status.Provisioning = false
-		// errors we expect might happen that we are ok with waiting for
-		catch := []string{
-			errhelp.ResourceGroupNotFoundErrorCode,
-			errhelp.ParentNotFoundErrorCode,
-			errhelp.NotFoundErrorCode,
-			errhelp.AsyncOpIncompleteError,
-		}
-
-		catchUnrecoverableErrors := []string{
-			errhelp.AccountNameInvalid,
-			errhelp.AlreadyExists,
-			errhelp.InvalidAccessPolicy,
-			errhelp.BadRequest,
-			errhelp.LocationNotAvailableForResourceType,
-		}
-
-		azerr := errhelp.NewAzureErrorAzureError(err)
-		if helpers.ContainsString(catch, azerr.Type) {
-			// most of these error technically mean the resource is actually not provisioning
-			switch azerr.Type {
-			case errhelp.AsyncOpIncompleteError:
-				instance.Status.Provisioning = true
-			}
-			// reconciliation is not done but error is acceptable
+		done, err := HandleCreationError(instance, err)
+		if done && exists {
+			instance.Status.Message = "key vault created but access policies failed: " + instance.Status.Message
 			return false, nil
 		}
-		if helpers.ContainsString(catchUnrecoverableErrors, azerr.Type) {
-			// Unrecoverable error, so stop reconcilation
-			switch azerr.Type {
-			case errhelp.AlreadyExists:
-				timeNow := metav1.NewTime(time.Now())
-				if timeNow.Sub(instance.Status.RequestedAt.Time) < (30 * time.Second) {
-					instance.Status.Provisioning = true
-					return false, nil
-				}
 
-			}
-			instance.Status.Message = "Reconcilation hit unrecoverable error " + err.Error()
-			return true, nil
-		}
-		// reconciliation not done and we don't know what happened
-		return false, err
+		return done, err
+	}
 
+	instance.Status.State = keyvault.Status
+	if keyvault.ID != nil {
+		instance.Status.ResourceId = *keyvault.ID
 	}
 	instance.Status.ContainsUpdate = false
-	instance.Status.State = keyvault.Status
-
 	instance.Status.Provisioned = true
 	instance.Status.Provisioning = false
 	instance.Status.Message = resourcemanager.SuccessMsg
-	instance.Status.ResourceId = *keyvault.ID
 
 	return true, nil
 }
 
+func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) {
+	// let the user know what happened
+	instance.Status.Message = errhelp.StripErrorTimes(errhelp.StripErrorIDs(err))
+	instance.Status.Provisioning = false
+	// errors we expect might happen that we are ok with waiting for
+	catch := []string{
+		errhelp.ResourceGroupNotFoundErrorCode,
+		errhelp.ParentNotFoundErrorCode,
+		errhelp.NotFoundErrorCode,
+		errhelp.AsyncOpIncompleteError,
+	}
+
+	catchUnrecoverableErrors := []string{
+		errhelp.AccountNameInvalid,
+		errhelp.AlreadyExists,
+		errhelp.InvalidAccessPolicy,
+		errhelp.BadRequest,
+		errhelp.LocationNotAvailableForResourceType,
+	}
+
+	azerr := errhelp.NewAzureErrorAzureError(err)
+	if helpers.ContainsString(catch, azerr.Type) {
+		// most of these error technically mean the resource is actually not provisioning
+		switch azerr.Type {
+		case errhelp.AsyncOpIncompleteError:
+			instance.Status.Provisioning = true
+		}
+		// reconciliation is not done but error is acceptable
+		return false, nil
+	}
+
+	if helpers.ContainsString(catchUnrecoverableErrors, azerr.Type) {
+		// Unrecoverable error, so stop reconcilation
+		switch azerr.Type {
+		case errhelp.AlreadyExists:
+			timeNow := metav1.NewTime(time.Now())
+			if timeNow.Sub(instance.Status.RequestedAt.Time) < (30 * time.Second) {
+				instance.Status.Provisioning = true
+				return false, nil
+			}
+
+		}
+		instance.Status.Message = "Reconcilation hit unrecoverable error " + err.Error()
+		return true, nil
+	}
+
+	if azerr.Code == http.StatusForbidden {
+		// permission errors when applying access policies are generally worth waiting on
+		return false, nil
+	}
+
+	// reconciliation not done and we don't know what happened
+	return false, err
+}
+
 func (k *azureKeyVaultManager) Delete(ctx context.Context, obj runtime.Object, opts ...resourcemanager.ConfigOption) (bool, error) {
 	instance, err := k.convert(obj)
 	if err != nil {

pkg/resourcemanager/keyvaults/keyvault_manager.go

@@ -16,7 +16,7 @@ import (
 var AzureKeyVaultManager KeyVaultManager = &azureKeyVaultManager{}
 
 type KeyVaultManager interface {
-	CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (keyvault.Vault, error)
+	CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, exists bool) (keyvault.Vault, error)
 
 	// CreateVault and grant access to the specific user ID
 	CreateVaultWithAccessPolicies(ctx context.Context, groupName string, vaultName string, location string, userID string) (keyvault.Vault, error)

pkg/resourcemanager/keyvaults/keyvault_test.go

@@ -71,6 +71,7 @@ var _ = Describe("KeyVault Resource Manager test", func() {
 					&kv,
 					sku,
 					tags,
+					false,
 				)
 				if err != nil {
 					fmt.Println(err.Error())