Skip to content

Commit

Permalink
Merge pull request #676 from cnadolny/access-policies-fix
Browse files Browse the repository at this point in the history 
Add null pointer check for access policies permissions
  • Loading branch information
@cnadolny
cnadolny authored Feb 29, 2020
2 parents dfef861 + 264faef commit 343e911
Show file tree
Hide file tree
Showing 6 changed files with 247 additions and 89 deletions.
5 changes: 5 additions & 0 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ test: generate fmt vet manifests
test-existing-controllers: generate fmt vet manifests
TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 go test -tags all -parallel 3 -v ./controllers/... -timeout 45m


unit-tests:
go test ./pkg/resourcemanager/keyvaults/unittest/


# Run tests with existing cluster
test-existing-managers: generate fmt vet manifests
TEST_USE_EXISTING_CLUSTER=true TEST_CONTROLLER_WITH_MOCKS=false REQUEUE_AFTER=20 \
Expand Down
126 changes: 81 additions & 45 deletions controllers/keyvault_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ func TestKeyvaultControllerHappyPath(t *testing.T) {
defer PanicRecover()
ctx := context.Background()
assert := assert.New(t)
var err error

keyVaultName := "t-kv-dev-" + helpers.RandomString(10)
const poll = time.Second * 10
Expand All @@ -48,25 +47,13 @@ func TestKeyvaultControllerHappyPath(t *testing.T) {
// Create the Keyvault object and expect the Reconcile to be created
EnsureInstance(ctx, t, tc, keyVaultInstance)

// Prep query for get
keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"}

// verify key vault exists in Azure
assert.Eventually(func() bool {
result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name)
return result.Response.StatusCode == http.StatusOK
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in azure")

// delete key vault
err = tc.k8sClient.Delete(ctx, keyVaultInstance)
assert.Equal(nil, err, "delete keyvault in k8s")

// verify key vault is gone from kubernetes

assert.Eventually(func() bool {
err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance)
return apierrors.IsNotFound(err)
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s")
EnsureDelete(ctx, t, tc, keyVaultInstance)

assert.Eventually(func() bool {
result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name)
Expand All @@ -84,17 +71,29 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) {
keyVaultName := "t-kv-dev-" + helpers.RandomString(10)
const poll = time.Second * 10
keyVaultLocation := tc.resourceGroupLocation
allPermissions := []string{"get", "list", "set", "delete", "recover", "backup", "restore"}
accessPolicies := []azurev1alpha1.AccessPolicyEntry{
{
TenantID: config.TenantID(),
ObjectID: config.ClientID(),

Permissions: &azurev1alpha1.Permissions{
Keys: &allPermissions,
Secrets: &allPermissions,
Certificates: &allPermissions,
Storage: &allPermissions,
Keys: &[]string{
"get",
"list",
},
Secrets: &[]string{
"get",
"list",
"set",
},
Certificates: &[]string{
"get",
"list",
},
Storage: &[]string{
"get",
"list",
},
},
}}

Expand All @@ -111,24 +110,7 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) {
},
}

// Create the Keyvault object and expect the Reconcile to be created
err := tc.k8sClient.Create(ctx, keyVaultInstance)
assert.Equal(nil, err, "create keyvault in k8s")

// Prep query for get
keyVaultNamespacedName := types.NamespacedName{Name: keyVaultName, Namespace: "default"}

assert.Eventually(func() bool {
_ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance)
return HasFinalizer(keyVaultInstance, finalizerName)
}, tc.timeout, tc.retry, "wait for keyvault to have finalizer")

// Wait until key vault is provisioned

assert.Eventually(func() bool {
_ = tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance)
return strings.Contains(keyVaultInstance.Status.Message, successMsg)
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in k8s")
EnsureInstance(ctx, t, tc, keyVaultInstance)

// verify key vault exists in Azure
assert.Eventually(func() bool {
Expand All @@ -145,21 +127,75 @@ func TestKeyvaultControllerWithAccessPolicies(t *testing.T) {
"test1": []byte("test2"),
"test2": []byte("test3"),
}
err = keyvaultSecretClient.Upsert(ctx, key, datanew)
err := keyvaultSecretClient.Upsert(ctx, key, datanew)
assert.Equal(nil, err, "expect secret to be inserted into keyvault")

_, err = keyvaultSecretClient.Get(ctx, key)
assert.Equal(nil, err, "checking if secret is present in keyvault")

// delete key vault
err = tc.k8sClient.Delete(ctx, keyVaultInstance)
assert.Equal(nil, err, "delete keyvault in k8s")
EnsureDelete(ctx, t, tc, keyVaultInstance)

// verify key vault is gone from kubernetes
assert.Eventually(func() bool {
err := tc.k8sClient.Get(ctx, keyVaultNamespacedName, keyVaultInstance)
return apierrors.IsNotFound(err)
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from k8s")
result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name)
return result.Response.StatusCode == http.StatusNotFound
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be gone from azure")
}

func TestKeyvaultControllerWithLimitedAccessPolicies(t *testing.T) {
t.Parallel()
defer PanicRecover()
ctx := context.Background()
assert := assert.New(t)
keyVaultName := "t-kv-dev-" + helpers.RandomString(10)
const poll = time.Second * 10
keyVaultLocation := tc.resourceGroupLocation
limitedPermissions := []string{"backup"}
accessPolicies := []azurev1alpha1.AccessPolicyEntry{
{
TenantID: config.TenantID(),
ObjectID: config.ClientID(),
Permissions: &azurev1alpha1.Permissions{
Keys: &limitedPermissions,
Secrets: &limitedPermissions,
},
}}

// Declare KeyVault object
keyVaultInstance := &azurev1alpha1.KeyVault{
ObjectMeta: metav1.ObjectMeta{
Name: keyVaultName,
Namespace: "default",
},
Spec: azurev1alpha1.KeyVaultSpec{
Location: keyVaultLocation,
ResourceGroup: tc.resourceGroupName,
AccessPolicies: &accessPolicies,
},
}

EnsureInstance(ctx, t, tc, keyVaultInstance)

// verify key vault exists in Azure
assert.Eventually(func() bool {
result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name)
return result.Response.StatusCode == http.StatusOK
}, tc.timeout, tc.retry, "wait for keyVaultInstance to be ready in azure")
//Add code to set secret and get secret from this keyvault using secretclient

keyvaultSecretClient := kvsecrets.New(keyVaultName)
secretName := "test-key"
key := types.NamespacedName{Name: secretName, Namespace: "default"}
datanew := map[string][]byte{
"test1": []byte("test2"),
"test2": []byte("test3"),
}
err := keyvaultSecretClient.Upsert(ctx, key, datanew)
assert.NotEqual(nil, err, "expect secret to not be inserted into keyvault")

_, err = keyvaultSecretClient.Get(ctx, key)
assert.NotEqual(nil, err, "should not be able to get secrets")

EnsureDelete(ctx, t, tc, keyVaultInstance)

assert.Eventually(func() bool {
result, _ := tc.keyVaultManager.GetVault(ctx, tc.resourceGroupName, keyVaultInstance.Name)
Expand Down
7 changes: 2 additions & 5 deletions controllers/keyvaultkey_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,13 @@ func TestKeyvaultKeyControllerHappyPath(t *testing.T) {

keyVaultLocation := tc.resourceGroupLocation

allPermissions := []string{"get", "list", "set", "delete", "recover", "backup", "restore", "create"}
keyPermissions := []string{"get", "list", "update", "delete", "recover", "backup", "restore", "create", "import"}
accessPolicies := []azurev1alpha1.AccessPolicyEntry{
{
TenantID: config.TenantID(),
ObjectID: config.ClientID(),
Permissions: &azurev1alpha1.Permissions{
Keys: &allPermissions,
Secrets: &allPermissions,
Certificates: &allPermissions,
Storage: &allPermissions,
Keys: &keyPermissions,
},
}}

Expand Down
5 changes: 5 additions & 0 deletions pkg/errhelp/errors.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package errhelp
import (
"encoding/json"
"fmt"
"strings"

"github.com/Azure/go-autorest/autorest"
"github.com/Azure/go-autorest/autorest/azure"
Expand Down Expand Up @@ -32,6 +33,7 @@ const (
InvalidRequestFormat = "InvalidRequestFormat"
KeyNotFound = "KeyNotFound"
InvalidParameters = "InvalidParameters"
InvalidAccessPolicy = "InvalidAccessPolicy"
Forbidden = "Forbidden"
NoSuchHost = "no such host"
CannotParseError = "CannotParseError"
Expand Down Expand Up @@ -96,6 +98,9 @@ func NewAzureError(err error) error {
} else if err.Error() == AccountNameInvalid {
kind = AccountNameInvalid
reason = AccountNameInvalid
} else if strings.Contains(err.Error(), InvalidAccessPolicy) {
kind = InvalidAccessPolicy
reason = InvalidAccessPolicy
}
ae.Reason = reason
ae.Type = kind
Expand Down
106 changes: 67 additions & 39 deletions pkg/resourcemanager/keyvaults/keyvault.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,64 +117,91 @@ func parseNetworkPolicy(instance *v1alpha1.KeyVault) keyvault.NetworkRuleSet {
return networkAcls
}

func parseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) (keyvault.AccessPolicyEntry, error) {
func ParseAccessPolicy(policy *v1alpha1.AccessPolicyEntry, ctx context.Context) (keyvault.AccessPolicyEntry, error) {
tenantID, err := uuid.FromString(policy.TenantID)
if err != nil {
return keyvault.AccessPolicyEntry{}, err
}

var keyPermissions []keyvault.KeyPermissions
validKeyPermissions := keyvault.PossibleKeyPermissionsValues()
for _, key := range *policy.Permissions.Keys {
for _, validKey := range validKeyPermissions {
if keyvault.KeyPermissions(key) == validKey {
keyPermissions = append(keyPermissions, validKey)
break
newEntry := keyvault.AccessPolicyEntry{
TenantID: &tenantID,
Permissions: &keyvault.Permissions{},
}

if policy.Permissions.Keys != nil {
var keyPermissions []keyvault.KeyPermissions
permissions := keyvault.PossibleKeyPermissionsValues()
validKeyPermissions := []string{}
for _, item := range permissions {
validKeyPermissions = append(validKeyPermissions, string(item))
}

for _, key := range *policy.Permissions.Keys {
if helpers.ContainsString(validKeyPermissions, key) {
keyPermissions = append(keyPermissions, keyvault.KeyPermissions(key))
} else {
return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Key Permission")
}
}

newEntry.Permissions.Keys = &keyPermissions
}

var secretPermissions []keyvault.SecretPermissions
validSecretPermissions := keyvault.PossibleSecretPermissionsValues()
for _, key := range *policy.Permissions.Secrets {
for _, validSecret := range validSecretPermissions {
if keyvault.SecretPermissions(key) == validSecret {
secretPermissions = append(secretPermissions, validSecret)
break
if policy.Permissions.Secrets != nil {
var secretPermissions []keyvault.SecretPermissions
permissions := keyvault.PossibleSecretPermissionsValues()
validSecretPermissions := []string{}
for _, item := range permissions {
validSecretPermissions = append(validSecretPermissions, string(item))
}

for _, key := range *policy.Permissions.Secrets {
if helpers.ContainsString(validSecretPermissions, key) {
secretPermissions = append(secretPermissions, keyvault.SecretPermissions(key))
} else {
return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Secret Permission")
}
}

newEntry.Permissions.Secrets = &secretPermissions
}

var certificatePermissions []keyvault.CertificatePermissions
validCertificatePermissions := keyvault.PossibleCertificatePermissionsValues()
for _, key := range *policy.Permissions.Certificates {
for _, validCert := range validCertificatePermissions {
if keyvault.CertificatePermissions(key) == validCert {
certificatePermissions = append(certificatePermissions, validCert)
break
if policy.Permissions.Certificates != nil {
var certificatePermissions []keyvault.CertificatePermissions
permissions := keyvault.PossibleCertificatePermissionsValues()
validCertificatePermissions := []string{}
for _, item := range permissions {
validCertificatePermissions = append(validCertificatePermissions, string(item))
}

for _, key := range *policy.Permissions.Certificates {
if helpers.ContainsString(validCertificatePermissions, key) {
certificatePermissions = append(certificatePermissions, keyvault.CertificatePermissions(key))
} else {
return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Certificate Permission")
}
}

newEntry.Permissions.Certificates = &certificatePermissions
}

var storagePermissions []keyvault.StoragePermissions
validStoragePermissions := keyvault.PossibleStoragePermissionsValues()
for _, key := range *policy.Permissions.Storage {
for _, validStorage := range validStoragePermissions {
if keyvault.StoragePermissions(key) == validStorage {
storagePermissions = append(storagePermissions, validStorage)
break
if policy.Permissions.Storage != nil {
var storagePermissions []keyvault.StoragePermissions
permissions := keyvault.PossibleStoragePermissionsValues()
validStoragePermissions := []string{}
for _, item := range permissions {
validStoragePermissions = append(validStoragePermissions, string(item))
}

for _, key := range *policy.Permissions.Storage {
if helpers.ContainsString(validStoragePermissions, key) {
storagePermissions = append(storagePermissions, keyvault.StoragePermissions(key))
} else {
return keyvault.AccessPolicyEntry{}, fmt.Errorf("InvalidAccessPolicy: Invalid Storage Permission")
}
}
}

newEntry := keyvault.AccessPolicyEntry{
TenantID: &tenantID,
Permissions: &keyvault.Permissions{
Keys: &keyPermissions,
Secrets: &secretPermissions,
Certificates: &certificatePermissions,
Storage: &storagePermissions,
},
newEntry.Permissions.Storage = &storagePermissions
}

if policy.ApplicationID != "" {
Expand Down Expand Up @@ -240,7 +267,7 @@ func (k *azureKeyVaultManager) CreateVault(ctx context.Context, instance *v1alph
var accessPolicies []keyvault.AccessPolicyEntry
if instance.Spec.AccessPolicies != nil {
for _, policy := range *instance.Spec.AccessPolicies {
newEntry, err := parseAccessPolicy(&policy, ctx)
newEntry, err := ParseAccessPolicy(&policy, ctx)
if err != nil {
return keyvault.Vault{}, err
}
Expand Down Expand Up @@ -392,6 +419,7 @@ func (k *azureKeyVaultManager) Ensure(ctx context.Context, obj runtime.Object, o
catchUnrecoverableErrors := []string{
errhelp.AccountNameInvalid,
errhelp.AlreadyExists,
errhelp.InvalidAccessPolicy,
}

azerr := errhelp.NewAzureErrorAzureError(err)
Expand Down
Loading

0 comments on commit 343e911

Please sign in to comment.