Use the MS Graph API for atomic add/remove password operations

Azure Active Directory Graph API, now deprecated, does not provide support for atomically creating/removing passwords on an application. As a result, there is a race conditions that can occur when creds are being created for roles configured with an existing service principal that is configured on multiple mounts or across multiple Vault clusters. Unfortunately, [`Azure/azure-sdk-for-go`](https://github.com/Azure/azure-sdk-for-go) does not yet offer a MS Graph API client, therefore, this PR utilizes [`Azure/go-autorest`](https://github.com/Azure/go-autorest) to construct a client the same as [`Azure/azure-sdk-for-go`](https://github.com/Azure/azure-sdk-for-go). This changeset preserves using the AAD Graph API by default but provides a mount configuration option for toggling to the new MS Graph API. This is because the two APIs require different API permissions. This allows users to upgrade to the new plugin version and then switch to the new API. Additionally, although using the MS Graph API is a net benefit, it itself has reliability issues when handling multiple requests in parallel. More details can be found in https://github.com/mdgreenfield/microsoft-graph-api-reliability and I am working with Microsoft to try to get some of these reliability issues resolved. Fixes hashicorp#58
mdgreenfield · Aug 30, 2021 · b5fa247 · b5fa247
1 parent 34e122b
commit b5fa247
Show file tree

Hide file tree

Showing 9 changed files with 613 additions and 165 deletions.
diff --git a/backend.go b/backend.go
@@ -14,7 +14,7 @@ import (
 type azureSecretBackend struct {
 	*framework.Backend
 
-	getProvider func(*clientSettings) (AzureProvider, error)
+	getProvider func(*clientSettings, bool, passwords) (AzureProvider, error)
 	client      *client
 	settings    *clientSettings
 	lock        sync.RWMutex
@@ -121,16 +121,16 @@ func (b *azureSecretBackend) getClient(ctx context.Context, s logical.Storage) (
 		b.settings = settings
 	}
 
-	p, err := b.getProvider(b.settings)
-	if err != nil {
-		return nil, err
-	}
-
 	passwords := passwords{
 		policyGenerator: b.System(),
 		policyName:      config.PasswordPolicy,
 	}
 
+	p, err := b.getProvider(b.settings, config.UseMsGraphAPI, passwords)
+	if err != nil {
+		return nil, err
+	}
+
 	c := &client{
 		provider:   p,
 		settings:   b.settings,

diff --git a/backend_test.go b/backend_test.go
@@ -6,13 +6,15 @@ import (
 	"fmt"
 	"regexp"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/profiles/latest/compute/mgmt/compute"
 	"github.com/Azure/azure-sdk-for-go/services/graphrbac/1.6/graphrbac"
 	"github.com/Azure/azure-sdk-for-go/services/preview/authorization/mgmt/2018-01-01-preview/authorization"
 	"github.com/Azure/go-autorest/autorest"
+	"github.com/Azure/go-autorest/autorest/date"
 	"github.com/Azure/go-autorest/autorest/to"
 	log "github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/vault/sdk/helper/logging"
@@ -44,7 +46,7 @@ func getTestBackend(t *testing.T, initConfig bool) (*azureSecretBackend, logical
 
 	b.settings = new(clientSettings)
 	mockProvider := newMockProvider()
-	b.getProvider = func(s *clientSettings) (AzureProvider, error) {
+	b.getProvider = func(s *clientSettings, usMsGraphApi bool, p passwords) (AzureProvider, error) {
 		return mockProvider, nil
 	}
 
@@ -69,8 +71,9 @@ func getTestBackend(t *testing.T, initConfig bool) (*azureSecretBackend, logical
 type mockProvider struct {
 	subscriptionID            string
 	applications              map[string]bool
-	passwords                 map[string]bool
+	passwords                 map[string]passwordCredential
 	failNextCreateApplication bool
+	lock                      sync.Mutex
 }
 
 // errMockProvider simulates a normal provider which fails to associate a role,
@@ -88,23 +91,23 @@ func (e *errMockProvider) CreateRoleAssignment(ctx context.Context, scope string
 // key is found, unlike mockProvider which returns the same application object
 // id each time. Existing tests depend on the mockProvider behavior, which is
 // why errMockProvider has it's own version.
-func (e *errMockProvider) GetApplication(ctx context.Context, applicationObjectID string) (graphrbac.Application, error) {
+func (e *errMockProvider) GetApplication(ctx context.Context, applicationObjectID string) (ApplicationResult, error) {
 	for s := range e.applications {
 		if s == applicationObjectID {
-			return graphrbac.Application{
+			return ApplicationResult{
 				AppID: to.StringPtr(s),
 			}, nil
 		}
 	}
-	return graphrbac.Application{}, errors.New("not found")
+	return ApplicationResult{}, errors.New("not found")
 }
 
 func newErrMockProvider() AzureProvider {
 	return &errMockProvider{
 		mockProvider: &mockProvider{
 			subscriptionID: generateUUID(),
 			applications:   make(map[string]bool),
-			passwords:      make(map[string]bool),
+			passwords:      make(map[string]passwordCredential),
 		},
 	}
 }
@@ -113,7 +116,7 @@ func newMockProvider() AzureProvider {
 	return &mockProvider{
 		subscriptionID: generateUUID(),
 		applications:   make(map[string]bool),
-		passwords:      make(map[string]bool),
+		passwords:      make(map[string]passwordCredential),
 	}
 }
 
@@ -174,22 +177,26 @@ func (m *mockProvider) CreateServicePrincipal(ctx context.Context, parameters gr
 	}, nil
 }
 
-func (m *mockProvider) CreateApplication(ctx context.Context, parameters graphrbac.ApplicationCreateParameters) (graphrbac.Application, error) {
+func (m *mockProvider) CreateApplication(ctx context.Context, displayName string) (ApplicationResult, error) {
 	if m.failNextCreateApplication {
 		m.failNextCreateApplication = false
-		return graphrbac.Application{}, errors.New("Mock: fail to create application")
+		return ApplicationResult{}, errors.New("Mock: fail to create application")
 	}
 	appObjID := generateUUID()
+
+	m.lock.Lock()
+	defer m.lock.Unlock()
+
 	m.applications[appObjID] = true
 
-	return graphrbac.Application{
-		AppID:    to.StringPtr(generateUUID()),
-		ObjectID: &appObjID,
+	return ApplicationResult{
+		AppID: to.StringPtr(generateUUID()),
+		ID:    &appObjID,
 	}, nil
 }
 
-func (m *mockProvider) GetApplication(ctx context.Context, applicationObjectID string) (graphrbac.Application, error) {
-	return graphrbac.Application{
+func (m *mockProvider) GetApplication(ctx context.Context, applicationObjectID string) (ApplicationResult, error) {
+	return ApplicationResult{
 		AppID: to.StringPtr("00000000-0000-0000-0000-000000000000"),
 	}, nil
 }
@@ -199,32 +206,41 @@ func (m *mockProvider) DeleteApplication(ctx context.Context, applicationObjectI
 	return autorest.Response{}, nil
 }
 
-func (m *mockProvider) UpdateApplicationPasswordCredentials(ctx context.Context, applicationObjectID string, parameters graphrbac.PasswordCredentialsUpdateParameters) (result autorest.Response, err error) {
-	m.passwords = make(map[string]bool)
-	for _, v := range *parameters.Value {
-		m.passwords[*v.KeyID] = true
+func (m *mockProvider) AddApplicationPassword(ctx context.Context, applicationObjectID string, displayName string, endDateTime date.Time) (result PasswordCredentialResult, err error) {
+	keyID := generateUUID()
+	cred := passwordCredential{
+		DisplayName: to.StringPtr(displayName),
+		StartDate:   &date.Time{Time: time.Now()},
+		EndDate:     &endDateTime,
+		KeyID:       to.StringPtr(keyID),
+		SecretText:  to.StringPtr(generateUUID()),
 	}
 
-	return autorest.Response{}, nil
+	m.lock.Lock()
+	defer m.lock.Unlock()
+	m.passwords[keyID] = cred
+
+	return PasswordCredentialResult{
+		passwordCredential: cred,
+	}, nil
 }
 
-func (m *mockProvider) ListApplicationPasswordCredentials(ctx context.Context, applicationObjectID string) (result graphrbac.PasswordCredentialListResult, err error) {
-	var creds []graphrbac.PasswordCredential
-	for keyID := range m.passwords {
-		creds = append(creds, graphrbac.PasswordCredential{KeyID: &keyID})
-	}
+func (m *mockProvider) RemoveApplicationPassword(background context.Context, applicationObjectID string, keyID string) (result autorest.Response, err error) {
+	m.lock.Lock()
+	defer m.lock.Unlock()
 
-	return graphrbac.PasswordCredentialListResult{
-		Value: &creds,
-	}, nil
+	delete(m.passwords, keyID)
+
+	return autorest.Response{}, nil
 }
 
 func (m *mockProvider) appExists(s string) bool {
 	return m.applications[s]
 }
 
 func (m *mockProvider) passwordExists(s string) bool {
-	return m.passwords[s]
+	_, ok := m.passwords[s]
+	return ok
 }
 
 func (m *mockProvider) VMGet(ctx context.Context, resourceGroupName string, VMName string, expand compute.InstanceViewTypes) (result compute.VirtualMachine, err error) {

diff --git a/client.go b/client.go
@@ -44,30 +44,23 @@ func (c *client) Valid() bool {
 // createApp creates a new Azure application.
 // An Application is a needed to create service principals used by
 // the caller for authentication.
-func (c *client) createApp(ctx context.Context) (app *graphrbac.Application, err error) {
+func (c *client) createApp(ctx context.Context) (app *ApplicationResult, err error) {
 	name, err := uuid.GenerateUUID()
 	if err != nil {
 		return nil, err
 	}
 
 	name = appNamePrefix + name
 
-	appURL := fmt.Sprintf("https://%s", name)
-
-	result, err := c.provider.CreateApplication(ctx, graphrbac.ApplicationCreateParameters{
-		AvailableToOtherTenants: to.BoolPtr(false),
-		DisplayName:             to.StringPtr(name),
-		Homepage:                to.StringPtr(appURL),
-		IdentifierUris:          to.StringSlicePtr([]string{appURL}),
-	})
+	result, err := c.provider.CreateApplication(ctx, name)
 
 	return &result, err
 }
 
 // createSP creates a new service principal.
 func (c *client) createSP(
 	ctx context.Context,
-	app *graphrbac.Application,
+	app *ApplicationResult,
 	duration time.Duration) (svcPrinc *graphrbac.ServicePrincipal, password string, err error) {
 
 	// Generate a random key (which must be a UUID) and password
@@ -114,85 +107,26 @@ func (c *client) createSP(
 }
 
 // addAppPassword adds a new password to an App's credentials list.
-func (c *client) addAppPassword(ctx context.Context, appObjID string, duration time.Duration) (keyID string, password string, err error) {
-	keyID, err = uuid.GenerateUUID()
-	if err != nil {
-		return "", "", err
-	}
-
-	// Key IDs are not secret, and they're a convenient way for an operator to identify Vault-generated
-	// passwords. These must be UUIDs, so the three leading bytes will be used as an indicator.
-	keyID = "ffffff" + keyID[6:]
-
-	password, err = c.passwords.generate(ctx)
-	if err != nil {
-		return "", "", err
-	}
-
-	now := time.Now().UTC()
-	cred := graphrbac.PasswordCredential{
-		StartDate: &date.Time{Time: now},
-		EndDate:   &date.Time{Time: now.Add(duration)},
-		KeyID:     to.StringPtr(keyID),
-		Value:     to.StringPtr(password),
-	}
-
-	// Load current credentials
-	resp, err := c.provider.ListApplicationPasswordCredentials(ctx, appObjID)
+func (c *client) addAppPassword(ctx context.Context, appObjID string, expiresIn time.Duration) (string, string, error) {
+	exp := date.Time{Time: time.Now().Add(expiresIn)}
+	resp, err := c.provider.AddApplicationPassword(ctx, appObjID, "vault-plugin-secrets-azure", exp)
 	if err != nil {
-		return "", "", errwrap.Wrapf("error fetching credentials: {{err}}", err)
-	}
-	curCreds := *resp.Value
-
-	// Add and save credentials
-	curCreds = append(curCreds, cred)
-
-	if _, err := c.provider.UpdateApplicationPasswordCredentials(ctx, appObjID,
-		graphrbac.PasswordCredentialsUpdateParameters{
-			Value: &curCreds,
-		},
-	); err != nil {
 		if strings.Contains(err.Error(), "size of the object has exceeded its limit") {
 			err = errors.New("maximum number of Application passwords reached")
 		}
 		return "", "", errwrap.Wrapf("error updating credentials: {{err}}", err)
 	}
 
-	return keyID, password, nil
+	return to.String(resp.KeyID), to.String(resp.SecretText), nil
 }
 
 // deleteAppPassword removes a password, if present, from an App's credentials list.
 func (c *client) deleteAppPassword(ctx context.Context, appObjID, keyID string) error {
-	// Load current credentials
-	resp, err := c.provider.ListApplicationPasswordCredentials(ctx, appObjID)
-	if err != nil {
-		return errwrap.Wrapf("error fetching credentials: {{err}}", err)
-	}
-	curCreds := *resp.Value
-
-	// Remove credential
-	found := false
-	for i := range curCreds {
-		if to.String(curCreds[i].KeyID) == keyID {
-			curCreds[i] = curCreds[len(curCreds)-1]
-			curCreds = curCreds[:len(curCreds)-1]
-			found = true
-			break
+	if _, err := c.provider.RemoveApplicationPassword(ctx, appObjID, keyID); err != nil {
+		if strings.Contains(err.Error(), "No password credential found with keyId") {
+			return nil
 		}
-	}
-
-	// KeyID is not present, so nothing to do
-	if !found {
-		return nil
-	}
-
-	// Save new credentials list
-	if _, err := c.provider.UpdateApplicationPasswordCredentials(ctx, appObjID,
-		graphrbac.PasswordCredentialsUpdateParameters{
-			Value: &curCreds,
-		},
-	); err != nil {
-		return errwrap.Wrapf("error updating credentials: {{err}}", err)
+		return errwrap.Wrapf("error removing credentials: {{err}}", err)
 	}
 
 	return nil