Validate host sets when they are created or updated. (#5)

* Update Readme with details around credential rotation needing to be disabled.

* Apply suggestions from code review

Co-authored-by: Jeff Mitchell <jeffrey.mitchell@gmail.com>

README.md

@@ -19,24 +19,22 @@ Current limitations of this plugin:
   compute resource
 - A client secret must be generated against a registered application with any
   necessary permissions
+- For now, credential rotation must be disabled using the `disable_credential_rotation`
+  attribute. There is some additional work that the Boundary team identified
+  that is necessary to make this a seamless experience.
 
 ## Credential Rotation
 
-Although credentials are stored encrypted within Boundary, by default this
-plugin will attempt to rotate credentials when they are supplied through the
-`secrets` object on a create or update call to the host catalog resource. The
-given credentials will be used to create a new credential, and then the given
-credential will be revoked. In this way, after rotation, only Boundary knows the
-client secret in use by this plugin.
+*NOTE*: For now credential rotation must be explicitly disabled by setting
+`disable_credential_rotation` to true on the host catalog.  When we lift this
+restriction Boundary will manage the rotation of credentials for you.
 
-*NOTE*: Azure's credentials are eventually consistent and there may be
-significant delay (up to ten minutes or more) between when a credential is
-created and is available for use, or when a credential is deleted and
-unavailable for use. At the API level, this can be true even if the Azure Portal
-indicates that the credential is (or isn't) there. This delay will manifest as
-authentication failures when performing actions using this plugin. If secrets
-have just been supplied and `disable_credential_rotation` is not specified,
-please be patient and try again later.
+Although credentials are stored encrypted within Boundary, when credential 
+rotation isn't explicitly disabled, this plugin will attempt to rotate
+credentials when they are supplied through the `secrets` object on a create or
+update call to the host catalog resource. The given credentials will be used to
+create a new credential, and then the given credential will be revoked. In this
+way, after rotation, only Boundary knows the client secret in use by this plugin.
 
 ## Attributes and Secrets
 

plugin/auth.go

@@ -21,7 +21,7 @@ type AuthorizationInfo struct {
 	AuthParams     AuthParams
 }
 
-type Attributes struct {
+type CatalogAttributes struct {
 	SubscriptionId            string `mapstructure:"subscription_id"`
 	ClientId                  string `mapstructure:"client_id"`
 	TenantId                  string `mapstructure:"tenant_id"`
@@ -45,18 +45,17 @@ type AuthParams struct {
 }
 
 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")
 	}
 
 	hcFields := hc.GetAttributes().AsMap()
-	var attrs Attributes
+	var attrs CatalogAttributes
 	if err := mapstructure.Decode(hcFields, &attrs); err != nil {
 		return nil, fmt.Errorf("error decoding host catalog attribute fields: %w", err)
 	}
@@ -154,8 +153,6 @@ func (a *AuthorizationInfo) populateObjectId(ctx context.Context, opt ...Option)
 
 // 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")

plugin/plugin.go

@@ -9,10 +9,13 @@ import (
 
 	"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/hashicorp/boundary/sdk/pbs/controller/api/resources/hostsets"
 	"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,6 +29,10 @@ var (
 	_ pb.HostPluginServiceServer = (*AzurePlugin)(nil)
 )
 
+type SetAttributes struct {
+	Filter string `mapstructure:"filter"`
+}
+
 func rotateCredFromCallback(ctx context.Context, catalog *hostcatalogs.HostCatalog) (*pb.HostCatalogPersisted, error) {
 	authzInfo, err := getAuthorizationInfo(catalog)
 	if err != nil {
@@ -51,167 +58,83 @@ func rotateCredFromCallback(ctx context.Context, catalog *hostcatalogs.HostCatal
 	}, 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")
+func (p *AzurePlugin) OnCreateCatalog(_ context.Context, req *pb.OnCreateCatalogRequest) (*pb.OnCreateCatalogResponse, error) {
+	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")
+func (p *AzurePlugin) OnUpdateCatalog(_ context.Context, req *pb.OnUpdateCatalogRequest) (*pb.OnUpdateCatalogResponse, error) {
+	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
+	return &pb.OnUpdateCatalogResponse{
+		Persisted: &pb.HostCatalogPersisted{
+			Secrets: persist,
+		},
+	}, 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)
-		}
+func (p *AzurePlugin) OnCreateSet(_ context.Context, req *pb.OnCreateSetRequest) (*pb.OnCreateSetResponse, error) {
+	if err := validateSet(req.GetSet()); err != nil {
+		return nil, err
 	}
+	return &pb.OnCreateSetResponse{}, nil
+}
 
-	return &pb.OnDeleteCatalogResponse{}, nil
+func (p *AzurePlugin) OnUpdateSet(_ context.Context, req *pb.OnUpdateSetRequest) (*pb.OnUpdateSetResponse, error) {
+	if err := validateSet(req.GetNewSet()); err != nil {
+		return nil, err
+	}
+	return &pb.OnUpdateSetResponse{}, nil
 }
 
 func (p *AzurePlugin) ListHosts(ctx context.Context, req *pb.ListHostsRequest) (*pb.ListHostsResponse, error) {
@@ -221,11 +144,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 +158,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 +294,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
@@ -491,9 +414,108 @@ func splitId(in, expectedService, expectedResource string) (string, string, erro
 	return splitId[3], splitId[7], nil
 }
 
-func sanitizeSecretsFields(in *structpb.Struct) {
-	// Only we are allowed to set this value
-	if fields := in.GetFields(); fields != nil {
-		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
+}
+
+var allowedCatalogFields = map[string]struct{}{
+	constDisableCredentialRotation: {},
+	constSubscriptionId:            {},
+	constTenantId:                  {},
+	constClientId:                  {},
+}
+
+func validateCatalog(c *hostcatalogs.HostCatalog) error {
+	if c == nil {
+		return status.Error(codes.InvalidArgument, "catalog is nil")
+	}
+	var attrs CatalogAttributes
+	attrMap := c.GetAttributes().AsMap()
+	if err := mapstructure.Decode(attrMap, &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."
+	}
+
+	for f := range attrMap {
+		if _, ok := allowedCatalogFields[f]; !ok {
+			badFields[fmt.Sprintf("attributes.%s", f)] = "Unrecognized field."
+		}
+	}
+
+	if len(badFields) > 0 {
+		return invalidArgumentError("Invalid arguments in the new catalog", badFields)
+	}
+	return nil
+}
+
+var allowedSetFields = map[string]struct{}{
+	constFilter: {},
+}
+
+func validateSet(s *hostsets.HostSet) error {
+	if s == nil {
+		return status.Error(codes.InvalidArgument, "set is nil")
+	}
+	var attrs SetAttributes
+	attrMap := s.GetAttributes().AsMap()
+	if err := mapstructure.Decode(attrMap, &attrs); err != nil {
+		return status.Errorf(codes.InvalidArgument, "error decoding set attributes: %s", err)
+	}
+	badFields := make(map[string]string)
+	if _, ok := attrMap[constFilter]; ok && len(attrs.Filter) == 0 {
+		badFields["attributes.filter"] = "This field must be not empty."
+	}
+
+	for f := range attrMap {
+		if _, ok := allowedSetFields[f]; !ok {
+			badFields[fmt.Sprintf("attributes.%s", f)] = "Unrecognized field."
+		}
+	}
+
+	if len(badFields) > 0 {
+		return invalidArgumentError("Invalid arguments in the new set", badFields)
+	}
+	return nil
 }

plugin/plugin_test.go

@@ -15,6 +15,7 @@ import (
 	"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,113 +47,312 @@ func TestListHosts(t *testing.T) {
 	pretty.Println(lhResp.GetHosts())
 }
 
-func TestOnCreateOnDeleteCatalog(t *testing.T) {
-	require := require.New(t)
+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",
+			}),
+		},
+		{
+			name: "bad type",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: "bad type",
+				constClientId:                  "client_id",
+				constTenantId:                  "tenant_id",
+				constSubscriptionId:            "sub_id",
+			}),
+			wantError: true,
+		},
+		{
+			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,
+		},
+		{
+			name: "unrecognized field",
+			catalogAttrs: wrapMap(t, map[string]interface{}{
+				constDisableCredentialRotation: true,
+				constClientId:                  "client_id",
+				constTenantId:                  "tenant_id",
+				constSubscriptionId:            "sub_id",
+				"unrecognized":                 "unrecognized",
+			}),
+			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 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 _, 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)
+			}
+		})
+	}
+}
+
+func TestValidateSet(t *testing.T) {
+	cases := []struct {
+		name      string
+		setAttrs  *structpb.Struct
+		wantError bool
+	}{
+		{
+			name: "success",
+			setAttrs: wrapMap(t, map[string]interface{}{
+				constFilter: "filter value",
+			}),
+		},
+		{
+			name:     "nil",
+			setAttrs: nil,
+		},
+		{
+			name: "bad type",
+			setAttrs: wrapMap(t, map[string]interface{}{
+				constFilter: true,
+			}),
+			wantError: true,
+		},
+		{
+			name: "empty",
+			setAttrs: wrapMap(t, map[string]interface{}{
+				constFilter: "",
+			}),
+			wantError: true,
+		},
+		{
+			name: "unrecognized field",
+			setAttrs: wrapMap(t, map[string]interface{}{
+				constFilter:    "filter value",
+				"unrecognized": "unrecognized",
+			}),
+			wantError: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := validateSet(&hostsets.HostSet{
+				Attributes: tc.setAttrs,
+			})
+			if tc.wantError {
+				assert.Error(t, got)
+			} else {
+				assert.NoError(t, got)
+			}
+		})
+	}
+}
+
+func TestCreateCatalog(t *testing.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,
+	attrs := wrapMap(t, map[string]interface{}{
+		constDisableCredentialRotation: true,
+		constClientId:                  "client_id",
+		constTenantId:                  "tenant_id",
+		constSubscriptionId:            "sub_id",
 	})
-	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,
+		constSecretValue: "secret_value",
+		constSecretId:    "secret_id",
 	})
 
-	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,
-		)
-	}()
+	res, err := p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{Catalog: &hostcatalogs.HostCatalog{
+		Attributes: attrs,
+		Secrets:    secrets,
+	}})
+	require.NoError(t, err)
+	assert.Equal(t, res.GetPersisted().GetSecrets().AsMap(), secrets.AsMap())
 
-	// 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{},
+	secretsWithExtra := wrapMap(t, map[string]interface{}{
+		constSecretValue: "secret_value",
+		constSecretId:    "secret_id",
+		"extraField":     "extra_value",
 	})
-	require.NoError(err)
-	require.Nil(cdResp)
+	res, err = p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{Catalog: &hostcatalogs.HostCatalog{
+		Attributes: attrs,
+		Secrets:    secretsWithExtra,
+	}})
+	require.NoError(t, err)
+	// still only persist the fields we care about.
+	assert.Equal(t, res.GetPersisted().GetSecrets().AsMap(), secrets.AsMap())
 
-	// 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,
+	secretsWithRotationTime := wrapMap(t, map[string]interface{}{
+		constSecretValue:          "secret_value",
+		constSecretId:             "secret_id",
+		constCredsLastRotatedTime: "something",
 	})
-	cdResp, err = p.OnDeleteCatalog(ctx, &pb.OnDeleteCatalogRequest{
-		Catalog: hostCatalog,
-		Persisted: &pb.HostCatalogPersisted{
-			Secrets: secrets,
-		},
+	res, err = p.OnCreateCatalog(ctx, &pb.OnCreateCatalogRequest{Catalog: &hostcatalogs.HostCatalog{
+		Attributes: attrs,
+		Secrets:    secretsWithRotationTime,
+	}})
+	assert.Error(t, err)
+}
+
+func TestUpdateCatalog(t *testing.T) {
+	ctx := context.Background()
+	p := &AzurePlugin{}
+
+	oldCatalog := &hostcatalogs.HostCatalog{
+		Attributes: wrapMap(t, map[string]interface{}{
+			constDisableCredentialRotation: true,
+			constClientId:                  "foo",
+			constTenantId:                  "foo",
+			constSubscriptionId:            "foo",
+		}),
+	}
+
+	attrs := 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)
+
+	res, err := p.OnUpdateCatalog(ctx, &pb.OnUpdateCatalogRequest{CurrentCatalog: oldCatalog,
+		NewCatalog: &hostcatalogs.HostCatalog{
+			Attributes: attrs,
+		}})
+	require.NoError(t, err)
+	assert.Nil(t, res.GetPersisted())
+
+	secrets := wrapMap(t, map[string]interface{}{
+		constSecretValue: "secret_value",
+		constSecretId:    "secret_id",
+	})
+
+	res, err = p.OnUpdateCatalog(ctx, &pb.OnUpdateCatalogRequest{CurrentCatalog: oldCatalog,
+		NewCatalog: &hostcatalogs.HostCatalog{
+			Attributes: attrs,
+			Secrets:    secrets,
+		}})
+	require.NoError(t, err)
+	assert.Equal(t, res.GetPersisted().GetSecrets().AsMap(), secrets.AsMap())
+
+	secretsWithExtra := wrapMap(t, map[string]interface{}{
+		constSecretValue: "secret_value",
+		constSecretId:    "secret_id",
+		"extraField":     "extra_value",
+	})
+	res, err = p.OnUpdateCatalog(ctx, &pb.OnUpdateCatalogRequest{CurrentCatalog: oldCatalog,
+		NewCatalog: &hostcatalogs.HostCatalog{
+			Attributes: attrs,
+			Secrets:    secretsWithExtra,
+		}})
+	require.NoError(t, err)
+	// still only persist the fields we care about.
+	assert.Equal(t, res.GetPersisted().GetSecrets().AsMap(), secrets.AsMap())
+
+	secretsWithRotationTime := wrapMap(t, map[string]interface{}{
+		constSecretValue:          "secret_value",
+		constSecretId:             "secret_id",
+		constCredsLastRotatedTime: "something",
+	})
+	res, err = p.OnUpdateCatalog(ctx, &pb.OnUpdateCatalogRequest{CurrentCatalog: oldCatalog,
+		NewCatalog: &hostcatalogs.HostCatalog{
+			Attributes: attrs,
+			Secrets:    secretsWithRotationTime,
+		}})
+	assert.Error(t, err)
 }
 
 func waitForCreds(t *testing.T, authzInfo *AuthorizationInfo, shouldWork bool) auth.Authorizer {