diff --git a/api/v1alpha1/aso_types.go b/api/v1alpha1/aso_types.go index 10723e9aeb4..568f0784e0a 100644 --- a/api/v1alpha1/aso_types.go +++ b/api/v1alpha1/aso_types.go @@ -17,7 +17,7 @@ type ASOStatus struct { ResourceId string `json:"resourceId,omitempty"` PollingURL string `json:"pollingUrl,omitempty"` SpecHash string `json:"specHash,omitempty"` - ContainsUpdate bool `json:"containsUpdate,omitempty"` + ContainsUpdate bool `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version RequestedAt *metav1.Time `json:"requested,omitempty"` CompletedAt *metav1.Time `json:"completed,omitempty"` FailedProvisioning bool `json:"failedProvisioning,omitempty"` diff --git a/api/v1alpha1/keyvault_types.go b/api/v1alpha1/keyvault_types.go index 5d289f241ba..81a997f1535 100644 --- a/api/v1alpha1/keyvault_types.go +++ b/api/v1alpha1/keyvault_types.go @@ -35,9 +35,9 @@ type AccessPolicyEntry struct { // TenantID - The Azure Active Directory tenant ID that should be used for authenticating requests to the key vault. 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. + // TODO: Remove this in a future API version, see: https://github.com/Azure/azure-service-operator/issues/1351 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 is the AAD object id of the entity to provide access to. ObjectID string `json:"objectID,omitempty"` // ApplicationID - Application ID of the client making request on behalf of a principal ApplicationID string `json:"applicationID,omitempty"` diff --git a/api/v1alpha2/aso_types.go b/api/v1alpha2/aso_types.go index 2fee004ebf9..12a2576aef4 100644 --- a/api/v1alpha2/aso_types.go +++ b/api/v1alpha2/aso_types.go @@ -17,7 +17,7 @@ type ASOStatus struct { ResourceId string `json:"resourceId,omitempty"` PollingURL string `json:"pollingUrl,omitempty"` SpecHash string `json:"specHash,omitempty"` - ContainsUpdate bool `json:"containsUpdate,omitempty"` + ContainsUpdate bool `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version RequestedAt *metav1.Time `json:"requested,omitempty"` CompletedAt *metav1.Time `json:"completed,omitempty"` FailedProvisioning bool `json:"failedProvisioning,omitempty"` diff --git a/api/v1beta1/aso_types.go b/api/v1beta1/aso_types.go index 6803c585c70..84e2ae8331f 100644 --- a/api/v1beta1/aso_types.go +++ b/api/v1beta1/aso_types.go @@ -17,7 +17,7 @@ type ASOStatus struct { ResourceId string `json:"resourceId,omitempty"` PollingURL string `json:"pollingUrl,omitempty"` SpecHash string `json:"specHash,omitempty"` - ContainsUpdate bool `json:"containsUpdate,omitempty"` + ContainsUpdate bool `json:"containsUpdate,omitempty"` // TODO: Unused, remove in future version RequestedAt *metav1.Time `json:"requested,omitempty"` CompletedAt *metav1.Time `json:"completed,omitempty"` FailedProvisioning bool `json:"failedProvisioning,omitempty"` diff --git a/config/samples/azure_v1alpha1_keyvault.yaml b/config/samples/azure_v1alpha1_keyvault.yaml index d79117245bf..ae4c721ac41 100644 --- a/config/samples/azure_v1alpha1_keyvault.yaml +++ b/config/samples/azure_v1alpha1_keyvault.yaml @@ -23,13 +23,11 @@ spec: - /subscriptions//resourceGroups/rg1/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/subnet2 accessPolicies: - tenantID: - clientID: - # 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. + 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: - # objectID: + # We strongly recommend that you do not use the clientID field, it will be removed in a future version of the API + permissions: keys: # backup create decrypt delete encrypt get import list purge recover restore sign unwrapKey update verify wrapKey - list diff --git a/controllers/keyvault_controller_test.go b/controllers/keyvault_controller_test.go index a6d1ab5277e..12abefdeaca 100644 --- a/controllers/keyvault_controller_test.go +++ b/controllers/keyvault_controller_test.go @@ -8,9 +8,12 @@ package controllers import ( "context" "net/http" + "strings" "testing" "time" + uuid "github.com/satori/go.uuid" + azurev1alpha1 "github.com/Azure/azure-service-operator/api/v1alpha1" "github.com/Azure/azure-service-operator/pkg/errhelp" "github.com/Azure/azure-service-operator/pkg/helpers" @@ -375,3 +378,51 @@ func TestKeyvaultControllerWithVirtualNetworkRulesAndUpdate(t *testing.T) { return result.Response.StatusCode == http.StatusNotFound }, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from azure") } + +func TestKeyvaultControllerBadAccessPolicy(t *testing.T) { + t.Parallel() + defer PanicRecover(t) + ctx := context.Background() + + keyVaultName := GenerateTestResourceNameWithRandom("kv", 6) + keyVaultLocation := tc.resourceGroupLocation + + // Declare KeyVault object + accessPolicies := []azurev1alpha1.AccessPolicyEntry{ + { + TenantID: config.GlobalCredentials().TenantID(), + ClientID: uuid.Nil.String(), + + Permissions: &azurev1alpha1.Permissions{ + Keys: &[]string{ + "get", + "list", + }, + }, + }} + + keyVaultInstance := &azurev1alpha1.KeyVault{ + ObjectMeta: metav1.ObjectMeta{ + Name: keyVaultName, + Namespace: "default", + }, + Spec: azurev1alpha1.KeyVaultSpec{ + Location: keyVaultLocation, + ResourceGroup: tc.resourceGroupName, + AccessPolicies: &accessPolicies, + }, + } + + assert := assert.New(t) + + err := tc.k8sClient.Create(ctx, keyVaultInstance) + assert.NoError(err) + namespacedName := types.NamespacedName{Name: keyVaultInstance.GetName(), Namespace: keyVaultInstance.GetNamespace()} + + assert.Eventually(func() bool { + _ = tc.k8sClient.Get(ctx, namespacedName, keyVaultInstance) + return strings.Contains(keyVaultInstance.Status.Message, "Authorization_RequestDenied") + }, tc.timeout, tc.retry, "wait for message to be updated indicating auth error") + + EnsureDelete(ctx, t, tc, keyVaultInstance) +} diff --git a/pkg/resourcemanager/keyvaults/keyvault.go b/pkg/resourcemanager/keyvaults/keyvault.go index af351ecbddf..9fd9e06a2e4 100644 --- a/pkg/resourcemanager/keyvaults/keyvault.go +++ b/pkg/resourcemanager/keyvaults/keyvault.go @@ -22,6 +22,7 @@ import ( "github.com/Azure/azure-service-operator/pkg/resourcemanager/iam" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/to" + "github.com/pkg/errors" uuid "github.com/satori/go.uuid" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -218,7 +219,6 @@ func ParseAccessPolicy(ctx context.Context, creds config.Credentials, policy *v1 return keyvault.AccessPolicyEntry{}, err } newEntry.ObjectID = objID - } else if policy.ObjectID != "" { newEntry.ObjectID = &policy.ObjectID } @@ -226,47 +226,20 @@ func ParseAccessPolicy(ctx context.Context, creds config.Credentials, policy *v1 return newEntry, nil } -// InstantiateVault will instantiate VaultsClient -func InstantiateVault(ctx context.Context, creds config.Credentials, vaultName string, containsUpdate bool) (keyvault.VaultsClient, uuid.UUID, error) { - vaultsClient, err := getVaultsClient(creds) - if err != nil { - return keyvault.VaultsClient{}, uuid.UUID{}, err - } - id, err := uuid.FromString(creds.TenantID()) - if err != nil { - return keyvault.VaultsClient{}, uuid.UUID{}, err - } - - // Check if keyvault name is valid - if !containsUpdate { - vaultNameCheck := keyvault.VaultCheckNameAvailabilityParameters{ - Name: to.StringPtr(vaultName), - Type: to.StringPtr("Microsoft.KeyVault/vaults"), - } - result, err := vaultsClient.CheckNameAvailability(ctx, vaultNameCheck) - if err != nil { - return keyvault.VaultsClient{}, uuid.UUID{}, err - } - if result.Reason == keyvault.Reason("Invalid") || result.Reason == keyvault.AccountNameInvalid { - return keyvault.VaultsClient{}, uuid.UUID{}, fmt.Errorf("AccountNameInvalid") - } else if result.Reason == keyvault.AlreadyExists { - return keyvault.VaultsClient{}, uuid.UUID{}, fmt.Errorf("AlreadyExists") - } - } - - return vaultsClient, id, nil -} - // CreateVault creates a new key vault -func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, vaultExists bool) (keyvault.Vault, error) { +func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (keyvault.Vault, error) { vaultName := instance.Name location := instance.Spec.Location groupName := instance.Spec.ResourceGroup enableSoftDelete := instance.Spec.EnableSoftDelete - vaultsClient, id, err := InstantiateVault(ctx, m.Creds, vaultName, instance.Status.ContainsUpdate) + vaultsClient, err := getVaultsClient(m.Creds) if err != nil { - return keyvault.Vault{}, err + return keyvault.Vault{}, errors.Wrapf(err, "couldn't get vaults client") + } + id, err := uuid.FromString(m.Creds.TenantID()) + if err != nil { + return keyvault.Vault{}, errors.Wrapf(err, "couldn't convert tenantID to UUID") } var accessPolicies []keyvault.AccessPolicyEntry @@ -279,8 +252,6 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph } accessPolicies = append(accessPolicies, newEntry) } - } else { - accessPolicies = []keyvault.AccessPolicyEntry{} } var networkAcls keyvault.NetworkRuleSet @@ -299,11 +270,10 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph keyVaultSku.Name = keyvault.Premium } - pols := []keyvault.AccessPolicyEntry{} params := keyvault.VaultCreateOrUpdateParameters{ Properties: &keyvault.VaultProperties{ TenantID: &id, - AccessPolicies: &pols, + AccessPolicies: &accessPolicies, Sku: &keyVaultSku, NetworkAcls: &networkAcls, EnableSoftDelete: &enableSoftDelete, @@ -312,10 +282,6 @@ func (m *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 @@ -325,10 +291,15 @@ func (m *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph } //CreateVaultWithAccessPolicies creates a new key vault and provides access policies to the specified user +// TODO: Nuke all of this because its only for the tests func (m *azureKeyVaultManager) CreateVaultWithAccessPolicies(ctx context.Context, groupName string, vaultName string, location string, clientID string) (keyvault.Vault, error) { - vaultsClient, id, err := InstantiateVault(ctx, m.Creds, vaultName, false) + vaultsClient, err := getVaultsClient(m.Creds) if err != nil { - return keyvault.Vault{}, err + return keyvault.Vault{}, errors.Wrapf(err, "couldn't get vaults client") + } + id, err := uuid.FromString(m.Creds.TenantID()) + if err != nil { + return keyvault.Vault{}, errors.Wrapf(err, "couldn't convert tenantID to UUID") } apList := []keyvault.AccessPolicyEntry{} @@ -409,24 +380,19 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o // convert kube labels to expected tag format labels := helpers.LabelsToTags(instance.GetLabels()) - instance.Status.Provisioning = true - instance.Status.FailedProvisioning = false + instance.Status.SetProvisioning("") exists := false // Check if this KeyVault already exists and its state if it does. keyvault, err := m.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 - instance.Status.Provisioning = false + instance.Status.SetProvisioned(resourcemanager.SuccessMsg) instance.Status.ResourceId = *keyvault.ID return true, nil } instance.Status.SpecHash = hash - instance.Status.ContainsUpdate = true - } keyvault, err = m.CreateVault( @@ -434,7 +400,6 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o instance, instance.Spec.Sku, labels, - exists, ) if err != nil { done, err := HandleCreationError(instance, err) @@ -450,10 +415,7 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o if keyvault.ID != nil { instance.Status.ResourceId = *keyvault.ID } - instance.Status.ContainsUpdate = false - instance.Status.Provisioned = true - instance.Status.Provisioning = false - instance.Status.Message = resourcemanager.SuccessMsg + instance.Status.SetProvisioned(resourcemanager.SuccessMsg) return true, nil } @@ -461,7 +423,6 @@ func (m *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o 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, @@ -480,11 +441,6 @@ func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) { azerr := errhelp.NewAzureError(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 } @@ -495,7 +451,6 @@ func HandleCreationError(instance *v1alpha1.KeyVault, err error) (bool, error) { 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 } diff --git a/pkg/resourcemanager/keyvaults/keyvault_manager.go b/pkg/resourcemanager/keyvaults/keyvault_manager.go index e58a47c794f..b98e82a1e24 100644 --- a/pkg/resourcemanager/keyvaults/keyvault_manager.go +++ b/pkg/resourcemanager/keyvaults/keyvault_manager.go @@ -14,7 +14,7 @@ import ( ) type KeyVaultManager interface { - CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string, exists bool) (keyvault.Vault, error) + CreateVault(ctx context.Context, instance *azurev1alpha1.KeyVault, sku azurev1alpha1.KeyVaultSku, tags map[string]*string) (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) diff --git a/pkg/secrets/keyvault/client.go b/pkg/secrets/keyvault/client.go index 051b80183cf..b2cb77a7b1d 100644 --- a/pkg/secrets/keyvault/client.go +++ b/pkg/secrets/keyvault/client.go @@ -5,37 +5,25 @@ package keyvault import ( "context" + "encoding/json" "fmt" "strings" "time" - "encoding/json" - - mgmtclient "github.com/Azure/azure-sdk-for-go/services/keyvault/mgmt/2018-02-14/keyvault" "github.com/Azure/azure-sdk-for-go/services/keyvault/v7.0/keyvault" keyvaults "github.com/Azure/azure-sdk-for-go/services/keyvault/v7.0/keyvault" + "github.com/Azure/go-autorest/autorest/date" + "github.com/Azure/go-autorest/autorest/to" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "github.com/Azure/azure-service-operator/api/v1alpha1" "github.com/Azure/azure-service-operator/pkg/errhelp" "github.com/Azure/azure-service-operator/pkg/resourcemanager/config" "github.com/Azure/azure-service-operator/pkg/resourcemanager/iam" "github.com/Azure/azure-service-operator/pkg/secrets" - "github.com/Azure/go-autorest/autorest/date" - "github.com/Azure/go-autorest/autorest/to" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ) -func getVaultsClient(creds config.Credentials) (mgmtclient.VaultsClient, error) { - vaultsClient := mgmtclient.NewVaultsClient(creds.SubscriptionID()) - a, err := iam.GetResourceManagementAuthorizer(creds) - if err != nil { - return vaultsClient, err - } - vaultsClient.Authorizer = a - vaultsClient.AddToUserAgent(config.UserAgent()) - return vaultsClient, nil -} - // KeyvaultSecretClient struct has the Key vault BaseClient that Azure uses and the KeyVault name type KeyvaultSecretClient struct { KeyVaultClient keyvaults.BaseClient diff --git a/pkg/secrets/keyvault/client_test.go b/pkg/secrets/keyvault/client_test.go index fc130a58e04..150eb08ca6a 100644 --- a/pkg/secrets/keyvault/client_test.go +++ b/pkg/secrets/keyvault/client_test.go @@ -70,7 +70,7 @@ var _ = Describe("Keyvault Secrets Client", func() { // Create a keyvault _, err = kvManager.CreateVaultWithAccessPolicies(ctx, resourcegroupName, keyVaultName, resourcegroupLocation, userID) - //Expect(err).NotTo(HaveOccurred()) + Expect(err).NotTo(HaveOccurred()) })