Remove rotation logic. Validate input.

plugin/auth.go

@@ -11,7 +11,6 @@ import (
 	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs"
 	hauth "github.com/manicminer/hamilton/auth"
 	"github.com/manicminer/hamilton/environments"
-	"github.com/manicminer/hamilton/msgraph"
 	"github.com/manicminer/hamilton/odata"
 	"github.com/mitchellh/mapstructure"
 )
@@ -21,6 +20,7 @@ type AuthorizationInfo struct {
 	AuthParams     AuthParams
 }
 
+// The allowed attribute fields from the controller to this plugin
 type Attributes struct {
 	SubscriptionId            string `mapstructure:"subscription_id"`
 	ClientId                  string `mapstructure:"client_id"`
@@ -28,6 +28,7 @@ type Attributes struct {
 	DisableCredentialRotation bool   `mapstructure:"disable_credential_rotation"`
 }
 
+// The allowed secret fields passed in from the controller to this plugin
 type SecretData struct {
 	SecretValue          string `mapstructure:"secret_value"`
 	SecretId             string `mapstructure:"secret_id"`
@@ -35,23 +36,23 @@ type SecretData struct {
 }
 
 type AuthParams struct {
-	SubscriptionId       string
-	ClientId             string
-	ClientObjectId       string
-	TenantId             string
+	SubscriptionId string
+	ClientId       string
+	ClientObjectId string
+	TenantId       string
+
 	SecretValue          string
 	SecretId             string
 	CredsLastRotatedTime time.Time
 }
 
 func getAuthorizationInfo(hc *hostcatalogs.HostCatalog) (*AuthorizationInfo, error) {
-	if hc == nil {
+	switch {
+	case hc == nil:
 		return nil, errors.New("host catalog is nil")
-	}
-	if hc.GetAttributes() == nil {
+	case hc.GetAttributes() == nil:
 		return nil, errors.New("host catalog attributes is nil")
-	}
-	if hc.GetSecrets() == nil {
+	case hc.GetSecrets() == nil:
 		return nil, errors.New("host catalog secret data is nil")
 	}
 
@@ -119,7 +120,7 @@ func (a *AuthorizationInfo) autorestAuthorizer() (autorest.Authorizer, error) {
 	return authorizer, nil
 }
 
-func (a *AuthorizationInfo) populateObjectId(ctx context.Context, opt ...Option) error {
+func (a *AuthorizationInfo) populateObjectId(ctx context.Context, _ ...Option) error {
 	if a.AuthParams.ClientObjectId != "" {
 		return nil
 	}
@@ -151,108 +152,3 @@ func (a *AuthorizationInfo) populateObjectId(ctx context.Context, opt ...Option)
 
 	return nil
 }
-
-// rotateCredentials creates a new password, then uses a client with that new
-// password to revoke the old.
-//
-// NOTE: the underlying auth config will be modified to use the new credentials!
-func rotateCredential(ctx context.Context, authzInfo *AuthorizationInfo, opt ...Option) (*msgraph.PasswordCredential, error) {
-	if authzInfo == nil {
-		return nil, errors.New("empty authz info")
-	}
-	if authzInfo.AuthParams.SecretId == "" {
-		return nil, errors.New("missing original secret id")
-	}
-
-	// Ensure the object ID is set
-	if err := authzInfo.populateObjectId(ctx); err != nil {
-		return nil, fmt.Errorf("error fetching object id: %w", err)
-	}
-
-	newCred, err := addCredential(ctx, authzInfo)
-	if err != nil {
-		return nil, fmt.Errorf("error adding password: %w", err)
-	}
-	if newCred == nil {
-		return nil, errors.New("new credential is nil after adding")
-	}
-	if newCred.SecretText == nil {
-		return nil, errors.New("new credential secret text is nil after adding")
-	}
-
-	if err := removeCredential(ctx, authzInfo); err != nil {
-		return nil, fmt.Errorf("error removing previous credential: %w", err)
-	}
-	return newCred, nil
-}
-
-func addCredential(ctx context.Context, authzInfo *AuthorizationInfo, opt ...Option) (*msgraph.PasswordCredential, error) {
-	if authzInfo == nil {
-		return nil, errors.New("empty authz info")
-	}
-
-	// Ensure the object ID is set
-	if err := authzInfo.populateObjectId(ctx); err != nil {
-		return nil, fmt.Errorf("error fetching object id: %w", err)
-	}
-
-	aClient, err := getApplicationsClient(ctx, authzInfo)
-	if err != nil {
-		return nil, fmt.Errorf("error getting application client: %w", err)
-	}
-	if aClient == nil {
-		return nil, errors.New("applications client is nil when adding credential")
-	}
-
-	// Create the new password
-	displayName := fmt.Sprintf("boundary-rotated-%s", time.Now().Format(time.RFC3339))
-	newPass, _, err := aClient.AddPassword(ctx, authzInfo.AuthParams.ClientObjectId, msgraph.PasswordCredential{
-		DisplayName: &displayName,
-	})
-	if err != nil {
-		return nil, fmt.Errorf("error creating new password: %w", err)
-	}
-	if newPass == nil {
-		return nil, errors.New("nil new password returned")
-	}
-	if newPass.KeyId == nil {
-		return nil, errors.New("nil key ID for new password")
-	}
-	if newPass.SecretText == nil {
-		return nil, errors.New("nil secret text for new password")
-	}
-
-	return newPass, nil
-}
-
-func removeCredential(ctx context.Context, authzInfo *AuthorizationInfo, opt ...Option) error {
-	// Ensure the object ID is set
-	if err := authzInfo.populateObjectId(ctx); err != nil {
-		return fmt.Errorf("error fetching object id: %w", err)
-	}
-
-	opts, err := getOpts(opt...)
-	if err != nil {
-		return fmt.Errorf("error parsing ops: %w", err)
-	}
-
-	secretId := authzInfo.AuthParams.SecretId
-	if opts.withSecretId != "" {
-		secretId = opts.withSecretId
-	}
-
-	aClient, err := getApplicationsClient(ctx, authzInfo)
-	if err != nil {
-		return fmt.Errorf("error getting application client: %w", err)
-	}
-	if aClient == nil {
-		return errors.New("applications client is nil when removing credential")
-	}
-
-	_, err = aClient.RemovePassword(ctx, authzInfo.AuthParams.ClientObjectId, secretId)
-	if err != nil {
-		return fmt.Errorf("error removing old password: %w", err)
-	}
-
-	return nil
-}

plugin/plugin.go

@@ -5,14 +5,15 @@ import (
 	"errors"
 	"fmt"
 	"strings"
-	"time"
 
 	"github.com/Azure/azure-sdk-for-go/profiles/latest/network/mgmt/network"
 	"github.com/Azure/azure-sdk-for-go/profiles/latest/resources/mgmt/resources"
+	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs"
 	"github.com/mitchellh/mapstructure"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/structpb"
 
-	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs"
 	pb "github.com/hashicorp/boundary/sdk/pbs/plugin"
 )
 
@@ -26,192 +27,70 @@ var (
 	_ pb.HostPluginServiceServer = (*AzurePlugin)(nil)
 )
 
-func rotateCredFromCallback(ctx context.Context, catalog *hostcatalogs.HostCatalog) (*pb.HostCatalogPersisted, error) {
-	authzInfo, err := getAuthorizationInfo(catalog)
-	if err != nil {
-		return nil, fmt.Errorf("error getting hauth config when creating catalog: %w", err)
-	}
-	newPass, err := rotateCredential(ctx, authzInfo)
-	if err != nil {
-		return nil, fmt.Errorf("error rotating credentials when creating catalog: %w", err)
-	}
-	if newPass == nil {
-		return nil, errors.New("new credential back from rotate is nil")
-	}
-	newSecrets, err := structpb.NewStruct(map[string]interface{}{
-		constSecretId:             *newPass.KeyId,
-		constSecretValue:          *newPass.SecretText,
-		constCredsLastRotatedTime: time.Now().Format(time.RFC3339Nano),
-	})
-	if err != nil {
-		return nil, fmt.Errorf("error formatting new credential data as struct")
-	}
-	return &pb.HostCatalogPersisted{
-		Secrets: newSecrets,
-	}, err
-}
-
 func (p *AzurePlugin) OnCreateCatalog(ctx context.Context, req *pb.OnCreateCatalogRequest) (*pb.OnCreateCatalogResponse, error) {
-	catalog := req.GetCatalog()
-	if catalog == nil {
-		return nil, errors.New("catalog is nil")
+	if err := validateCatalog(req.GetCatalog()); err != nil {
+		return nil, err
 	}
-	secrets := catalog.GetSecrets()
-	if secrets == nil {
-		// Secrets will have to be provided later, on update
-		return &pb.OnCreateCatalogResponse{}, nil
+	if err := validateSecrets(req.GetCatalog().GetSecrets()); err != nil {
+		return nil, err
 	}
 
-	sanitizeSecretsFields(secrets)
-
-	// Check if rotation is skipped. Always prefer attempting rotation unless we
-	// explicitly are told not to
-	var skipRotation bool
-	if catalog.GetAttributes() != nil {
-		var attrs Attributes
-		if err := mapstructure.Decode(catalog.GetAttributes().AsMap(), &attrs); err != nil {
-			return nil, fmt.Errorf("error decoding catalog attributes: %w", err)
-		}
-		skipRotation = attrs.DisableCredentialRotation
+	secrets := req.GetCatalog().GetSecrets().AsMap()
+	persistedSecrets := map[string]interface{}{
+		constSecretValue: secrets[constSecretValue],
 	}
-	if skipRotation {
-		return &pb.OnCreateCatalogResponse{
-			Persisted: &pb.HostCatalogPersisted{
-				Secrets: secrets,
-			},
-		}, nil
+	if _, ok := secrets[constSecretId]; ok {
+		persistedSecrets[constSecretId] = secrets[constSecretId]
 	}
-
-	newPersisted, err := rotateCredFromCallback(ctx, catalog)
+	persist, err := structpb.NewStruct(persistedSecrets)
 	if err != nil {
-		return nil, fmt.Errorf("error formatting new persisted data: %w", err)
+		return nil, status.Errorf(codes.FailedPrecondition, "failed marshaling persisted secrets: %q", err.Error())
 	}
-	ret := &pb.OnCreateCatalogResponse{
-		Persisted: newPersisted,
-	}
-	return ret, nil
+
+	return &pb.OnCreateCatalogResponse{
+		Persisted: &pb.HostCatalogPersisted{
+			Secrets: persist,
+		},
+	}, nil
 }
 
 func (p *AzurePlugin) OnUpdateCatalog(ctx context.Context, req *pb.OnUpdateCatalogRequest) (*pb.OnUpdateCatalogResponse, error) {
-	newCatalog := req.GetNewCatalog()
-	if newCatalog == nil {
-		// Should never happen
-		return nil, errors.New("new catalog is nil")
+	if err := validateCatalog(req.GetNewCatalog()); err != nil {
+		return nil, err
 	}
 	currentCatalog := req.GetCurrentCatalog()
 	if currentCatalog == nil {
 		// Should never happen
-		return nil, errors.New("current catalog is nil")
+		return nil, status.Error(codes.FailedPrecondition, "current catalog is nil")
 	}
-	secrets := newCatalog.GetSecrets()
+	secrets := req.GetNewCatalog().GetSecrets()
 	if secrets == nil {
 		// If new secrets weren't passed in, don't rotate what we have on
 		// update.
-		//
-		// TODO: There may be combinations of attributes where those values
-		// changing (or being added) should trigger rotation.
 		return &pb.OnUpdateCatalogResponse{}, nil
 	}
 	sanitizeSecretsFields(secrets)
+	if err := validateSecrets(secrets); err != nil {
+		return nil, err
+	}
 
-	// If the old credential had been rotated, attempt to delete the old
-	// credentials using the new ones; if it fails, proceed anyways, since in
-	// theory only Boundary knows this value so simply forgetting it is safe
-	// enough (if not clean).
-	var field *structpb.Value
-	var pdFields map[string]*structpb.Value
-	var err error
-	var authzInfo *AuthorizationInfo
-
-	persisted := req.GetPersisted()
-	if persisted == nil {
-		goto after_revocation
+	secretsMap := secrets.AsMap()
+	persistedSecrets := map[string]interface{}{
+		constSecretValue: secretsMap[constSecretValue],
 	}
-	if persisted.GetSecrets() == nil {
-		goto after_revocation
+	if _, ok := secretsMap[constSecretId]; ok {
+		persistedSecrets[constSecretId] = secretsMap[constSecretId]
 	}
-	pdFields = persisted.GetSecrets().GetFields()
-	if pdFields == nil {
-		goto after_revocation
-	}
-	field = pdFields[constCredsLastRotatedTime]
-	if field == nil {
-		goto after_revocation
-	}
-	// If the field exists, it is a timestamp here indicating last rotation time
-	currentCatalog.Secrets = req.GetPersisted().Secrets
-	authzInfo, err = getAuthorizationInfo(currentCatalog)
+	persist, err := structpb.NewStruct(persistedSecrets)
 	if err != nil {
-		// TODO: warn in some way?
-		goto after_revocation
-	}
-	if err := removeCredential(ctx, authzInfo); err != nil {
-		// TODO: warn
-		goto after_revocation
+		return nil, status.Errorf(codes.FailedPrecondition, "failed marshaling persisted secrets: %q", err.Error())
 	}
 
-after_revocation:
-	// Check if rotation is skipped. Always prefer attempting rotation unless we
-	// explicitly are told not to
-	var skipRotation bool
-	if attrs := newCatalog.GetAttributes(); attrs != nil {
-		if fields := attrs.GetFields(); fields != nil {
-			if field := fields[constDisableCredentialRotation]; field != nil {
-				if field.GetBoolValue() {
-					skipRotation = true
-				}
-			}
-		}
-	}
-	if skipRotation {
-		return &pb.OnUpdateCatalogResponse{
-			Persisted: &pb.HostCatalogPersisted{
-				Secrets: secrets,
-			},
-		}, nil
-	}
-	newPersisted, err := rotateCredFromCallback(ctx, req.GetNewCatalog())
-	if err != nil {
-		return nil, fmt.Errorf("error formatting new persisted data: %w", err)
-	}
-	ret := &pb.OnUpdateCatalogResponse{
-		Persisted: newPersisted,
-	}
-	return ret, nil
-}
-
-func (p *AzurePlugin) OnDeleteCatalog(ctx context.Context, req *pb.OnDeleteCatalogRequest) (*pb.OnDeleteCatalogResponse, error) {
-	if req.GetCatalog() == nil {
-		return nil, errors.New("catalog is nil")
-	}
-	if req.GetPersisted() == nil || req.GetPersisted().GetSecrets() == nil {
-		return &pb.OnDeleteCatalogResponse{}, nil
-	}
-	// Figure out if creds were rotated
-	pdFields := req.GetPersisted().GetSecrets().GetFields()
-	if pdFields == nil {
-		// No data
-		return &pb.OnDeleteCatalogResponse{}, nil
-	}
-	field := pdFields[constCredsLastRotatedTime]
-	if field == nil {
-		return &pb.OnDeleteCatalogResponse{}, nil
-	}
-	// The field existing means it is a string containing the last time it was
-	// rotated. Delete the credential using current creds.
-	catalog := req.GetCatalog()
-	catalog.Secrets = req.GetPersisted().GetSecrets()
-	authzInfo, err := getAuthorizationInfo(catalog)
-	if err != nil {
-		return nil, fmt.Errorf("error getting auth config on catalog deletion: %w", err)
-	}
-	if err := removeCredential(ctx, authzInfo); err != nil {
-		if err != nil {
-			return nil, fmt.Errorf("error revoking credential on catalog deletion: %w", err)
-		}
-	}
-
-	return &pb.OnDeleteCatalogResponse{}, nil
+	return &pb.OnUpdateCatalogResponse{
+		Persisted: &pb.HostCatalogPersisted{
+			Secrets: persist,
+		},
+	}, nil
 }
 
 func (p *AzurePlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (*pb.ListHostsResponse, error) {
@@ -221,11 +100,11 @@ func (p *AzurePlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (
 	}
 	catalog := req.GetCatalog()
 	if catalog == nil {
-		return nil, errors.New("catalog is nil")
+		return nil, status.Error(codes.FailedPrecondition, "catalog is nil")
 	}
 	catalogAttrs := catalog.GetAttributes()
 	if catalogAttrs == nil {
-		return nil, errors.New("catalog has no attributes")
+		return nil, status.Error(codes.FailedPrecondition, "catalog has no attributes")
 	}
 
 	// Create an authorizer
@@ -235,7 +114,7 @@ func (p *AzurePlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (
 		return nil, fmt.Errorf("error fetching authorizationInfo: %w", err)
 	}
 	if authzInfo == nil {
-		return nil, errors.New("authorization info is nil")
+		return nil, status.Error(codes.FailedPrecondition, "authorization info is nil")
 	}
 	authorizer, err := authzInfo.autorestAuthorizer()
 	if err != nil {
@@ -371,11 +250,11 @@ func (p *AzurePlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (
 				return nil, fmt.Errorf("instance view statuses returned for vm with id %q is null", *res.ID)
 			}
 			var running bool
-			for _, status := range *iv.Statuses {
-				if status.Code == nil {
+			for _, s := range *iv.Statuses {
+				if s.Code == nil {
 					continue
 				}
-				state := strings.ToLower(*status.Code)
+				state := strings.ToLower(*s.Code)
 				prefix := "powerstate/"
 				if !strings.HasPrefix(state, prefix) {
 					continue
@@ -497,3 +376,65 @@ func sanitizeSecretsFields(in *structpb.Struct) {
 		delete(fields, constCredsLastRotatedTime)
 	}
 }
+
+// Returns an invalid argument error with the problematic fields included
+// in the error message.
+func invalidArgumentError(msg string, f map[string]string) error {
+	var fieldMsgs []string
+	for field, val := range f {
+		fieldMsgs = append(fieldMsgs, fmt.Sprintf("%q: %q", field, val))
+	}
+	if len(fieldMsgs) > 0 {
+		msg = fmt.Sprintf("%s: [%s]", msg, strings.Join(fieldMsgs, ", "))
+	}
+	return status.Error(codes.InvalidArgument, msg)
+}
+
+func validateSecrets(s *structpb.Struct) error {
+	if s == nil {
+		return status.Error(codes.InvalidArgument, "Secrets not provided but are required")
+	}
+	var secrets SecretData
+	if err := mapstructure.Decode(s.AsMap(), &secrets); err != nil {
+		return status.Errorf(codes.InvalidArgument, "error decoding catalog secrets: %s", err)
+	}
+	badFields := make(map[string]string)
+	if len(secrets.SecretValue) == 0 {
+		badFields["secrets.secret_value"] = "This field is required."
+	}
+	if len(secrets.CredsLastRotatedTime) != 0 {
+		badFields["secrets.creds_last_rotated_time"] = "This field is reserved and cannot be set."
+	}
+	if len(badFields) > 0 {
+		return invalidArgumentError("Error in the secrets provided", badFields)
+	}
+	return nil
+}
+
+func validateCatalog(catalog *hostcatalogs.HostCatalog) error {
+	if catalog == nil {
+		return status.Error(codes.InvalidArgument, "catalog is nil")
+	}
+	var attrs Attributes
+	if err := mapstructure.Decode(catalog.GetAttributes().AsMap(), &attrs); err != nil {
+		return status.Errorf(codes.InvalidArgument, "error decoding catalog attributes: %s", err)
+	}
+	badFields := make(map[string]string)
+	if !attrs.DisableCredentialRotation {
+		badFields["attributes.disable_credential_rotation"] = "This field must be set to true."
+	}
+	if len(attrs.SubscriptionId) == 0 {
+		badFields["attributes.subscription_id"] = "This is a required field."
+	}
+	if len(attrs.ClientId) == 0 {
+		badFields["attributes.client_id"] = "This is a required field."
+	}
+	if len(attrs.TenantId) == 0 {
+		badFields["attributes.tenant_id"] = "This is a required field."
+	}
+
+	if len(badFields) > 0 {
+		return invalidArgumentError("Invalid arguments in the new catalog", badFields)
+	}
+	return nil
+}

plugin/plugin_test.go

@@ -5,16 +5,13 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
-	"time"
 
 	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostcatalogs"
 	"github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/hostsets"
 	pb "github.com/hashicorp/boundary/sdk/pbs/plugin"
 	"github.com/hashicorp/go-secure-stdlib/parseutil"
 	"github.com/kr/pretty"
-	"github.com/manicminer/hamilton/auth"
-	"github.com/manicminer/hamilton/environments"
-	"github.com/manicminer/hamilton/msgraph"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/types/known/structpb"
 )
@@ -46,139 +43,123 @@ func TestListHosts(t *testing.T) {
 	pretty.Println(lhResp.GetHosts())
 }
 
-func TestOnCreateOnDeleteCatalog(t *testing.T) {
-	require := require.New(t)
-	ctx := context.Background()
-	p := &AzurePlugin{}
-	hostCatalog, _ := testGetHostStructs(t)
-	initialAuthConfig, initialCred, cleanup := testCreateInitialSecret(t, hostCatalog)
-	defer cleanup()
-
-	// At this point, remove secrets corresponding to those read in by
-	// testGetHostCatalog as they're our permanent ones
-	hostCatalog.Secrets = nil
-
-	// Give some time -- a lot -- for token to become active
-	time.Sleep(10 * time.Minute)
-
-	// Now, we'll use this credential to simulate creating a host catalog, which
-	// will rotate it. First, ensure we don't error out if creds aren't provided
-	// _yet_ (instead, on update)
-	ccResp, err := p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{
-		Catalog: hostCatalog,
-	})
-	require.NoError(err)
-	require.Nil(ccResp)
-
-	// Now run the same thing with rotation disabled, make sure we get the same
-	// values back
-	secrets := wrapMap(t, map[string]interface{}{
-		constSecretValue: *initialCred.SecretText,
-		constSecretId:    *initialCred.KeyId,
-	})
-	hostCatalog.Secrets = secrets
-	hostCatalog.GetAttributes().GetFields()[constDisableCredentialRotation] = structpb.NewBoolValue(true)
-	ccResp, err = p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{
-		Catalog: hostCatalog,
-	})
-	require.NoError(err)
-	require.NotNil(ccResp)
-	require.NotNil(ccResp.GetPersisted())
-	require.NotNil(ccResp.GetPersisted().GetSecrets())
-	newPdFields := ccResp.GetPersisted().GetSecrets().GetFields()
-	require.Equal(*initialCred.KeyId, newPdFields[constSecretId].GetStringValue())
-	require.Equal(*initialCred.SecretText, newPdFields[constSecretValue].GetStringValue())
-
-	// Now we'll try again, actually rotating the creds
-	delete(hostCatalog.GetAttributes().GetFields(), constDisableCredentialRotation)
-	ccResp, err = p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{
-		Catalog: hostCatalog,
-	})
-
-	require.NoError(err)
-	require.NotNil(ccResp)
-	require.NotNil(ccResp.GetPersisted())
-	require.NotNil(ccResp.GetPersisted().GetSecrets())
-	newPdFields = ccResp.GetPersisted().GetSecrets().GetFields()
-	// Use old creds for this removal to give it a high likelihood of success
-	deferAuthConfig := *initialAuthConfig
-	defer func() {
-		_ = removeCredential(
-			ctx,
-			&deferAuthConfig,
-		)
-	}()
-
-	// Best-effort ensure that this new cred is deleted if OnDelete doesn't work right
-	require.NotEqual(*initialCred.KeyId, newPdFields[constSecretId].GetStringValue())
-	require.NotEqual(*initialCred.SecretText, newPdFields[constSecretValue].GetStringValue())
-
-	// Remove these now prior to OnDelete
-	hostCatalog.Secrets = nil
-
-	// Make sure old creds don't work
-	waitForCredsAuthConfig := *initialAuthConfig
-	// FIXME: Actually don't, because they can still be used for up to a day!??! later?!?!
-	// waitForCreds(t, &waitForCredsAuthConfig, false)
-
-	// Make sure the new creds work, then sleep
-	waitForCredsAuthConfig.AuthParams.SecretId = newPdFields[constSecretId].GetStringValue()
-	waitForCredsAuthConfig.AuthParams.SecretValue = newPdFields[constSecretValue].GetStringValue()
-	waitForCredsAuthConfig.HamiltonConfig.ClientSecret = newPdFields[constSecretValue].GetStringValue()
-	waitForCreds(t, &waitForCredsAuthConfig, true)
-	time.Sleep(10 * time.Minute)
-
-	// Now, let's test OnDelete. First, check the case where we never configured
-	// a credential in the first place.
-	cdResp, err := p.OnDeleteCatalog(ctx, &pb.OnDeleteCatalogRequest{
-		Catalog:   hostCatalog,
-		Persisted: &pb.HostCatalogPersisted{},
-	})
-	require.NoError(err)
-	require.Nil(cdResp)
-
-	// Now, use the credentials we got above to revoke this credential we got from OnCreate
-	secrets = wrapMap(t, map[string]interface{}{
-		constSecretValue: waitForCredsAuthConfig.HamiltonConfig.ClientSecret,
-		constSecretId:    waitForCredsAuthConfig.AuthParams.SecretId,
-	})
-	cdResp, err = p.OnDeleteCatalog(ctx, &pb.OnDeleteCatalogRequest{
-		Catalog: hostCatalog,
-		Persisted: &pb.HostCatalogPersisted{
-			Secrets: secrets,
+func TestValidateCatalog(t *testing.T) {
+	cases := []struct {
+		name         string
+		catalogAttrs *structpb.Struct
+		wantError    bool
+	}{
+		{
+			name: "success",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: true,
+				constClientId:                  "client_id",
+				constTenantId:                  "tenant_id",
+				constSubscriptionId:            "sub_id",
+			}),
 		},
-	})
-	require.NoError(err)
-	require.Nil(cdResp)
-	// Make sure the creds no longer work
-	// FIXME: Don't, for the same reason
-	// waitForCreds(t, &waitForCredsAuthConfig, false)
+		{
+			name:         "nil",
+			catalogAttrs: nil,
+			wantError:    true,
+		},
+		{
+			name: "rotation enabled",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: false,
+				constClientId:                  "client_id",
+				constTenantId:                  "tenant_id",
+				constSubscriptionId:            "sub_id",
+			}),
+			wantError: true,
+		},
+		{
+			name: "no client id",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: true,
+				constTenantId:                  "tenant_id",
+				constSubscriptionId:            "sub_id",
+			}),
+			wantError: true,
+		},
+		{
+			name: "no tenant id",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: false,
+				constClientId:                  "client_id",
+				constSubscriptionId:            "sub_id",
+			}),
+			wantError: true,
+		},
+		{
+			name: "no subscription id",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: false,
+				constClientId:                  "client_id",
+				constTenantId:                  "tenant_id",
+			}),
+			wantError: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := validateCatalog(&hostcatalogs.HostCatalog{
+				Attributes: tc.catalogAttrs,
+			})
+			if tc.wantError {
+				assert.Error(t, got)
+			} else {
+				assert.NoError(t, got)
+			}
+		})
+	}
 }
 
-func waitForCreds(t *testing.T, authzInfo *AuthorizationInfo, shouldWork bool) auth.Authorizer {
-	t.Helper()
-	require := require.New(t)
-	ctx := context.Background()
-	auth, err := auth.NewClientSecretAuthorizer(
-		ctx,
-		environments.Global,
-		auth.MsGraph,
-		auth.TokenVersion2,
-		authzInfo.AuthParams.TenantId,
-		authzInfo.AuthParams.ClientId,
-		authzInfo.AuthParams.SecretValue)
-	require.NoError(err)
+func TestValidateSecrets(t *testing.T) {
+	cases := []struct {
+		name           string
+		catalogSecrets *structpb.Struct
+		wantError      bool
+	}{
+		{
+			name: "success",
+			catalogSecrets: wrapMap(t, map[string]interface{}{
+				constSecretValue: "value",
+				constSecretId:    "secret_id",
+			}),
+		},
+		{
+			name:           "nil secret value",
+			catalogSecrets: nil,
+			wantError:      true,
+		},
+		{
+			name: "no secret value",
+			catalogSecrets: wrapMap(t, map[string]interface{}{
+				constSecretId: "secret_id",
+			}),
+			wantError: true,
+		},
+		{
+			name: "secret with last rotated time",
+			catalogSecrets: wrapMap(t, map[string]interface{}{
+				constSecretValue:          "value",
+				constCredsLastRotatedTime: "something",
+			}),
+			wantError: true,
+		},
+	}
 
-	for {
-		_, err := auth.Token()
-		switch {
-		case shouldWork && err == nil:
-			return auth
-		case !shouldWork && err != nil:
-			return nil
-		default:
-			time.Sleep(30 * time.Second)
-		}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := validateSecrets(tc.catalogSecrets)
+			if tc.wantError {
+				assert.Error(t, got)
+			} else {
+				assert.NoError(t, got)
+			}
+		})
 	}
 }
 
@@ -225,44 +206,3 @@ func testGetHostStructs(t *testing.T) (*hostcatalogs.HostCatalog, []*hostsets.Ho
 
 	return hc, hses
 }
-
-// This function creates a new secret so we don't revoke the original one
-// leading into a test. This will serve as our "initial secret" for a test. We
-// also return a few cached items. It is expected that the host catalog contains
-// Secrets.
-func testCreateInitialSecret(t *testing.T, hostCatalog *hostcatalogs.HostCatalog) (*AuthorizationInfo, *msgraph.PasswordCredential, func()) {
-	require := require.New(t)
-	ctx := context.Background()
-
-	var initialCred *msgraph.PasswordCredential
-
-	// Set up the data, get the config
-	authzInfo, err := getAuthorizationInfo(hostCatalog)
-	require.NoError(err)
-
-	// Get the credential
-	initialCred, err = addCredential(ctx, authzInfo)
-	require.NoError(err)
-	require.NotNil(initialCred)
-	require.NotNil(initialCred.KeyId)
-	require.NotNil(initialCred.SecretText)
-
-	origAuthConfig := *authzInfo
-	origAuthConfig.AuthParams.SecretId = *initialCred.KeyId
-	cleanup := func() {
-		if err := removeCredential(
-			ctx,
-			&origAuthConfig,
-		); err != nil {
-			// It may not exist because it's been rotated
-			require.Contains(err.Error(), "No password credential found with keyId", err.Error())
-		}
-	}
-
-	authzInfo.AuthParams.SecretId = *initialCred.KeyId
-	authzInfo.AuthParams.SecretValue = *initialCred.SecretText
-	authzInfo.HamiltonConfig.ClientSecret = *initialCred.SecretText
-	waitForCreds(t, authzInfo, true)
-
-	return authzInfo, initialCred, cleanup
-}