Backport: #42 to release/vault-1.4.x

backend.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/locksutil"
@@ -54,6 +55,12 @@ func backend() *azureSecretBackend {
 		},
 		BackendType: logical.TypeLogical,
 		Invalidate:  b.invalidate,
+
+		WALRollback: b.walRollback,
+
+		// Role assignment can take up to a few minutes, so ensure we don't try
+		// to roll back during creation.
+		WALRollbackMinAge: 10 * time.Minute,
 	}
 
 	b.getProvider = newAzureProvider

backend_test.go

@@ -73,6 +73,42 @@ type mockProvider struct {
 	failNextCreateApplication bool
 }
 
+// errMockProvider simulates a normal provider which fails to associate a role,
+// returning an error
+type errMockProvider struct {
+	*mockProvider
+}
+
+// CreateRoleAssignment for the errMockProvider intentionally fails
+func (e *errMockProvider) CreateRoleAssignment(ctx context.Context, scope string, roleAssignmentName string, parameters authorization.RoleAssignmentCreateParameters) (authorization.RoleAssignment, error) {
+	return authorization.RoleAssignment{}, errors.New("PrincipalNotFound")
+}
+
+// GetApplication for the errMockProvider only returns an application if that
+// 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) {
+	for s := range e.applications {
+		if s == applicationObjectID {
+			return graphrbac.Application{
+				AppID: to.StringPtr(s),
+			}, nil
+		}
+	}
+	return graphrbac.Application{}, errors.New("not found")
+}
+
+func newErrMockProvider() AzureProvider {
+	return &errMockProvider{
+		mockProvider: &mockProvider{
+			subscriptionID: generateUUID(),
+			applications:   make(map[string]bool),
+			passwords:      make(map[string]bool),
+		},
+	}
+}
+
 func newMockProvider() AzureProvider {
 	return &mockProvider{
 		subscriptionID: generateUUID(),

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/Azure/go-autorest/autorest/date v0.2.0
 	github.com/Azure/go-autorest/autorest/to v0.3.0
 	github.com/Azure/go-autorest/autorest/validation v0.2.0 // indirect
+	github.com/frankban/quicktest v1.10.1 // indirect
 	github.com/go-test/deep v1.0.2-0.20181118220953-042da051cf31
 	github.com/hashicorp/errwrap v1.0.0
 	github.com/hashicorp/go-hclog v0.12.0
@@ -17,6 +18,15 @@ require (
 	github.com/hashicorp/vault/api v1.0.5-0.20200317185738-82f498082f02
 	github.com/hashicorp/vault/sdk v0.1.14-0.20200317185738-82f498082f02
 	github.com/hashicorp/yamux v0.0.0-20181012175058-2f1d1f20f75d // indirect
-	golang.org/x/text v0.3.1-0.20181227161524-e6919f6577db // indirect
-	google.golang.org/genproto v0.0.0-20190404172233-64821d5d2107 // indirect
+	github.com/kr/text v0.2.0 // indirect
+	github.com/mattn/go-colorable v0.1.6 // indirect
+	github.com/mitchellh/mapstructure v1.3.2
+	github.com/pierrec/lz4 v2.5.2+incompatible // indirect
+	golang.org/x/crypto v0.0.0-20200604202706-70a84ac30bf9 // indirect
+	golang.org/x/net v0.0.0-20200602114024-627f9648deb9 // indirect
+	golang.org/x/sys v0.0.0-20200602225109-6fdc65e7d980 // indirect
+	golang.org/x/text v0.3.2 // indirect
+	golang.org/x/time v0.0.0-20200416051211-89c76fbcd5d1 // indirect
+	google.golang.org/protobuf v1.24.0 // indirect
+	gopkg.in/square/go-jose.v2 v2.5.1 // indirect
 )

path_service_principal.go

@@ -77,7 +77,7 @@ func (b *azureSecretBackend) pathSPRead(ctx context.Context, req *logical.Reques
 	if role.ApplicationObjectID != "" {
 		resp, err = b.createStaticSPSecret(ctx, client, roleName, role)
 	} else {
-		resp, err = b.createSPSecret(ctx, client, roleName, role)
+		resp, err = b.createSPSecret(ctx, req.Storage, client, roleName, role)
 	}
 
 	if err != nil {
@@ -91,36 +91,49 @@ func (b *azureSecretBackend) pathSPRead(ctx context.Context, req *logical.Reques
 }
 
 // createSPSecret generates a new App/Service Principal.
-func (b *azureSecretBackend) createSPSecret(ctx context.Context, c *client, roleName string, role *roleEntry) (*logical.Response, error) {
+func (b *azureSecretBackend) createSPSecret(ctx context.Context, s logical.Storage, c *client, roleName string, role *roleEntry) (*logical.Response, error) {
 	// Create the App, which is the top level object to be tracked in the secret
-	// and deleted upon revocation. If any subsequent step fails, the App is deleted.
+	// and deleted upon revocation. If any subsequent step fails, the App will be
+	// deleted as part of WAL rollback.
 	app, err := c.createApp(ctx)
 	if err != nil {
 		return nil, err
 	}
 	appID := to.String(app.AppID)
 	appObjID := to.String(app.ObjectID)
 
+	// Write a WAL entry in case the SP create process doesn't complete
+	walID, err := framework.PutWAL(ctx, s, walAppKey, &walApp{
+		AppID:      appID,
+		AppObjID:   appObjID,
+		Expiration: time.Now().Add(maxWALAge),
+	})
+	if err != nil {
+		return nil, errwrap.Wrapf("error writing WAL: {{err}}", err)
+	}
+
 	// Create a service principal associated with the new App
 	sp, password, err := c.createSP(ctx, app, spExpiration)
 	if err != nil {
-		c.deleteApp(ctx, appObjID)
 		return nil, err
 	}
 
 	// Assign Azure roles to the new SP
 	raIDs, err := c.assignRoles(ctx, sp, role.AzureRoles)
 	if err != nil {
-		c.deleteApp(ctx, appObjID)
 		return nil, err
 	}
 
 	// Assign Azure group memberships to the new SP
 	if err := c.addGroupMemberships(ctx, sp, role.AzureGroups); err != nil {
-		c.deleteApp(ctx, appObjID)
 		return nil, err
 	}
 
+	// SP is fully created so delete the WAL
+	if err := framework.DeleteWAL(ctx, s, walID); err != nil {
+		return nil, errwrap.Wrapf("error deleting WAL: {{err}}", err)
+	}
+
 	data := map[string]interface{}{
 		"client_id":     appID,
 		"client_secret": password,

path_service_principal_test.go

@@ -11,8 +11,10 @@ import (
 	"github.com/Azure/go-autorest/autorest/to"
 	log "github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-uuid"
+	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/logging"
 	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/mitchellh/mapstructure"
 )
 
 var (
@@ -49,6 +51,99 @@ var (
 	}
 )
 
+// TestSP_WAL_Cleanup tests that any Service Principal that gets created, but
+// fails to have roles associated with it, gets cleaned up by the periodic WAL
+// function.
+func TestSP_WAL_Cleanup(t *testing.T) {
+	b, s := getTestBackend(t, true)
+
+	// overwrite the normal test backend provider with the errMockProvider
+	errMockProvider := newErrMockProvider()
+	b.getProvider = func(s *clientSettings) (AzureProvider, error) {
+		return errMockProvider, nil
+	}
+
+	// verify basic cred issuance
+	t.Run("Role assign fail", func(t *testing.T) {
+		name := generateUUID()
+		testRoleCreate(t, b, s, name, testRole)
+
+		// create a short timeout to short-circuit the retry process and trigger the
+		// deadline error
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+
+		resp, err := b.HandleRequest(ctx, &logical.Request{
+			Operation: logical.ReadOperation,
+			Path:      "creds/" + name,
+			Storage:   s,
+		})
+
+		if err == nil || !strings.Contains(err.Error(), "context deadline exceeded") {
+			t.Fatalf("expected deadline error, but got '%s'", err.Error())
+		}
+		if resp.IsError() {
+			t.Fatalf("expected no response error, actual:%#v", resp.Error())
+		}
+
+		assertEmptyWAL(t, b, errMockProvider, s)
+	})
+}
+
+func assertEmptyWAL(t *testing.T, b *azureSecretBackend, emp AzureProvider, s logical.Storage) {
+	t.Helper()
+
+	wal, err := framework.ListWAL(context.Background(), s)
+	if err != nil {
+		t.Fatalf("error listing wal: %s", err)
+	}
+	req := &logical.Request{
+		Storage: s,
+	}
+
+	// loop of WAL entries and trigger the rollback method for each, simulating
+	// Vault's rollback mechanism
+	for _, v := range wal {
+		ctx := context.Background()
+		entry, err := framework.GetWAL(ctx, s, v)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Decode the WAL data
+		var app walApp
+		d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
+			DecodeHook: mapstructure.StringToTimeHookFunc(time.RFC3339),
+			Result:     &app,
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+		err = d.Decode(entry.Data)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		_, err = emp.GetApplication(context.Background(), app.AppObjID)
+		if err != nil {
+			t.Fatalf("expected to find application (%s), but wasn't found", app.AppObjID)
+		}
+
+		err = b.walRollback(ctx, req, entry.Kind, entry.Data)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if err := framework.DeleteWAL(ctx, s, v); err != nil {
+			t.Fatal(err)
+		}
+
+		_, err = emp.GetApplication(context.Background(), app.AppObjID)
+		if err == nil {
+			t.Fatalf("expected error getting application")
+		}
+	}
+}
+
 func TestSPRead(t *testing.T) {
 	b, s := getTestBackend(t, true)
 

scripts/local_dev.sh

@@ -46,7 +46,7 @@ function cleanup {
 trap cleanup EXIT
 
 echo "    Authing"
-vault auth root &>/dev/null
+vault login root &>/dev/null
 
 echo "--> Building"
 go build -o "$SCRATCH/plugins/$PLUGIN_NAME" "./cmd/$PLUGIN_NAME" 

wal.go

@@ -0,0 +1,62 @@
+package azuresecrets
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/mitchellh/mapstructure"
+)
+
+const walAppKey = "appCreate"
+
+// Eventually expire the WAL if for some reason the rollback operation consistently fails
+var maxWALAge = 24 * time.Hour
+
+type walApp struct {
+	AppID      string
+	AppObjID   string
+	Expiration time.Time
+}
+
+func (b *azureSecretBackend) walRollback(ctx context.Context, req *logical.Request, kind string, data interface{}) error {
+	if kind != walAppKey {
+		return fmt.Errorf("unknown rollback type %q", kind)
+	}
+	// Decode the WAL data
+	var entry walApp
+	d, err := mapstructure.NewDecoder(&mapstructure.DecoderConfig{
+		DecodeHook: mapstructure.StringToTimeHookFunc(time.RFC3339),
+		Result:     &entry,
+	})
+	if err != nil {
+		return err
+	}
+	err = d.Decode(data)
+	if err != nil {
+		return err
+	}
+
+	client, err := b.getClient(ctx, req.Storage)
+	if err != nil {
+		return err
+	}
+
+	b.Logger().Debug("rolling back SP", "appID", entry.AppID, "appObjID", entry.AppObjID)
+
+	// Attempt to delete the App. deleteApp doesn't return an error if the app isn't
+	// found, so no special handling is needed for that case. If we don't succeed within
+	// maxWALAge (e.g. client creds have changed and the delete will never succeed),
+	// unconditionally remove the WAL.
+	if err := client.deleteApp(ctx, entry.AppObjID); err != nil {
+		b.Logger().Warn("rollback error deleting App", "err", err)
+
+		if time.Now().After(entry.Expiration) {
+			return nil
+		}
+		return err
+	}
+
+	return nil
+}