chore: don't allow nil credential store

Currently we allow a nil credential store for legacy functionality to store secrets in Postgres or Vault. Nowadays secrets go to the CredentialStore interface which can be Vault/Postgres/Other. Secrets/sensitive data are now no longer in dbmodel types.
canonical · Jan 9, 2025 · 2ab8501 · 2ab8501
1 parent 4ee7344
commit 2ab8501
Show file tree

Hide file tree

Showing 26 changed files with 178 additions and 281 deletions.
diff --git a/internal/db/cloudcredential.go b/internal/db/cloudcredential.go
@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.
 
 package db
 
@@ -35,7 +35,7 @@ func (d *Database) SetCloudCredential(ctx context.Context, cred *dbmodel.CloudCr
 			{Name: "owner_identity_name"},
 			{Name: "name"},
 		},
-		DoUpdates: clause.AssignmentColumns([]string{"auth_type", "label", "attributes_in_vault", "attributes", "valid"}),
+		DoUpdates: clause.AssignmentColumns([]string{"auth_type", "label", "valid"}),
 	}).Create(&cred).Error; err != nil {
 		return errors.E(op, dbError(err))
 	}

diff --git a/internal/db/cloudcredential_test.go b/internal/db/cloudcredential_test.go
@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.
 
 package db_test
 
@@ -119,11 +119,6 @@ func (s *dbSuite) TestSetCloudCredentialUpdate(c *qt.C) {
 	cred.Cloud.Regions = nil
 
 	cred.Label = "test label"
-	cred.Attributes = dbmodel.StringMap{
-		"key1": "value1",
-		"key2": "value2",
-	}
-	cred.AttributesInVault = true
 	cred.Valid = sql.NullBool{
 		Bool:  true,
 		Valid: true,
@@ -139,12 +134,7 @@ func (s *dbSuite) TestSetCloudCredentialUpdate(c *qt.C) {
 	err = s.Database.GetCloudCredential(context.Background(), &dbCred)
 	c.Assert(err, qt.Equals, nil)
 	c.Assert(dbCred, jimmtest.DBObjectEquals, cred)
-	c.Assert(dbCred.Attributes, qt.DeepEquals, dbmodel.StringMap{
-		"key1": "value1",
-		"key2": "value2",
-	})
 	c.Assert(dbCred.Label, qt.Equals, "test label")
-	c.Assert(dbCred.AttributesInVault, qt.IsTrue)
 	c.Assert(dbCred.Valid.Valid, qt.IsTrue)
 	c.Assert(dbCred.Valid.Bool, qt.IsTrue)
 }

diff --git a/internal/dbmodel/cloudcredential.go b/internal/dbmodel/cloudcredential.go
@@ -31,13 +31,6 @@ type CloudCredential struct {
 	// Label is an optional label for the credential.
 	Label string
 
-	// AttributesInVault indicates whether the attributes are stored in
-	// the configured vault key-value store, rather than this database.
-	AttributesInVault bool
-
-	// Attributes contains the attributes of the credential.
-	Attributes StringMap
-
 	// Valid stores whether the cloud-credential is known to be valid.
 	Valid sql.NullBool
 

diff --git a/internal/dbmodel/cloudcredential_test.go b/internal/dbmodel/cloudcredential_test.go
@@ -40,10 +40,6 @@ func TestCloudCredential(t *testing.T) {
 		Owner:    *i,
 		AuthType: "empty",
 		Label:    "test label",
-		Attributes: dbmodel.StringMap{
-			"a": "b",
-			"c": "d",
-		},
 	}
 	result := db.Create(&cred)
 	c.Assert(result.Error, qt.IsNil)

diff --git a/internal/dbmodel/controller.go b/internal/dbmodel/controller.go
@@ -32,14 +32,6 @@ type Controller struct {
 	// purposes.
 	UUID string `gorm:"not null"`
 
-	// AdminIdentityName is the identity name that JIMM uses to connect to the
-	// controller.
-	AdminIdentityName string
-
-	// AdminPassword is the password that JIMM uses to connect to the
-	// controller.
-	AdminPassword string
-
 	// CACertificate is the CA certificate required to access this
 	// controller. This is only set if the controller endpoint's
 	// certificate is not signed by a public CA.
@@ -112,7 +104,6 @@ func (c Controller) ToAPIControllerInfo() apiparams.ControllerInfo {
 	var ci apiparams.ControllerInfo
 	ci.Name = c.Name
 	ci.UUID = c.UUID
-	ci.Username = c.AdminIdentityName
 	ci.PublicAddress = c.PublicAddress
 	for _, hps := range c.Addresses {
 		for _, hp := range hps {
@@ -122,7 +113,6 @@ func (c Controller) ToAPIControllerInfo() apiparams.ControllerInfo {
 	ci.CACertificate = c.CACertificate
 	ci.CloudTag = names.NewCloudTag(c.CloudName).String()
 	ci.CloudRegion = c.CloudRegion
-	ci.Username = c.AdminIdentityName
 	ci.AgentVersion = c.AgentVersion
 	switch {
 	case c.UnavailableSince.Valid:

diff --git a/internal/dbmodel/controller_test.go b/internal/dbmodel/controller_test.go
@@ -40,13 +40,11 @@ func TestController(t *testing.T) {
 	c.Assert(result.Error, qt.IsNil)
 
 	ctl := dbmodel.Controller{
-		Name:              "test-controller",
-		UUID:              "00000000-0000-0000-0000-000000000001",
-		AdminIdentityName: "admin",
-		AdminPassword:     "pw",
-		CACertificate:     "ca-cert",
-		PublicAddress:     "controller.example.com:443",
-		CloudName:         "test-cloud",
+		Name:          "test-controller",
+		UUID:          "00000000-0000-0000-0000-000000000001",
+		CACertificate: "ca-cert",
+		PublicAddress: "controller.example.com:443",
+		CloudName:     "test-cloud",
 		Addresses: dbmodel.HostPorts([][]jujuparams.HostPort{{{
 			Address: jujuparams.Address{
 				Value: "1.1.1.1",
@@ -166,7 +164,6 @@ func TestToAPIControllerInfo(t *testing.T) {
 		CloudRegion: cl.Regions[0],
 		Priority:    dbmodel.CloudRegionControllerPriorityDeployed,
 	}}
-	ctl.AdminIdentityName = "admin"
 	ctl.AgentVersion = "1.2.3"
 
 	ci := ctl.ToAPIControllerInfo()

diff --git a/internal/dbmodel/sql/postgres/019_remove_secrets_in_db.up.sql b/internal/dbmodel/sql/postgres/019_remove_secrets_in_db.up.sql
@@ -0,0 +1,6 @@
+-- deletes secrets that were directly stored in the database.
+
+ALTER TABLE cloud_credentials DROP COLUMN IF EXISTS attributes_in_vault;
+ALTER TABLE cloud_credentials DROP COLUMN IF EXISTS attributes;
+ALTER TABLE controllers DROP COLUMN IF EXISTS admin_identity_name;
+ALTER TABLE controllers DROP COLUMN IF EXISTS admin_password;
diff --git a/internal/jimm/cloudcredential.go b/internal/jimm/cloudcredential.go
@@ -1,4 +1,4 @@
-// Copyright 2024 Canonical.
+// Copyright 2025 Canonical.
 
 package jimm
 
@@ -41,7 +41,6 @@ func (j *JIMM) GetCloudCredential(ctx context.Context, user *openfga.User, tag n
 	if err != nil {
 		return nil, errors.E(op, err)
 	}
-	credential.Attributes = nil
 
 	return &credential, nil
 }
@@ -177,7 +176,6 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar
 	}
 
 	credential.AuthType = args.Credential.AuthType
-	credential.Attributes = args.Credential.Attributes
 
 	if !args.SkipCheck {
 		err := j.forEachController(ctx, controllers, func(ctl *dbmodel.Controller, api API) error {
@@ -204,7 +202,7 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar
 		return result, nil
 	}
 
-	if err := j.updateCredential(ctx, &credential); err != nil {
+	if err := j.updateCredential(ctx, &credential, args.Credential.Attributes); err != nil {
 		return result, errors.E(op, err)
 	}
 
@@ -227,32 +225,18 @@ func (j *JIMM) UpdateCloudCredential(ctx context.Context, user *openfga.User, ar
 }
 
 // updateCredential updates the credential stored in JIMM's database.
-func (j *JIMM) updateCredential(ctx context.Context, credential *dbmodel.CloudCredential) error {
+func (j *JIMM) updateCredential(ctx context.Context, credential *dbmodel.CloudCredential, attr map[string]string) error {
 	const op = errors.Op("jimm.updateCredential")
 
-	if j.CredentialStore == nil {
-		zapctx.Debug(ctx, "credential store is nil!")
-		credential.AttributesInVault = false
-		if err := j.Database.SetCloudCredential(ctx, credential); err != nil {
-			return errors.E(op, err)
-		}
-		return nil
-	}
-
-	credential1 := *credential
-	credential1.Attributes = nil
-	credential1.AttributesInVault = true
-	if err := j.Database.SetCloudCredential(ctx, &credential1); err != nil {
+	if err := j.Database.SetCloudCredential(ctx, credential); err != nil {
 		zapctx.Error(ctx, "failed to store credential id", zap.Error(err))
 		return errors.E(op, err)
 	}
-	if err := j.CredentialStore.Put(ctx, credential.ResourceTag(), credential.Attributes); err != nil {
+	if err := j.CredentialStore.Put(ctx, credential.ResourceTag(), attr); err != nil {
 		zapctx.Error(ctx, "failed to store credentials", zap.Error(err))
 		return errors.E(op, err)
 	}
 
-	zapctx.Info(ctx, "credential store location", zap.Bool("vault", credential.AttributesInVault))
-
 	return nil
 }
 
@@ -263,13 +247,10 @@ func (j *JIMM) updateControllerCloudCredential(
 ) ([]jujuparams.UpdateCredentialModelResult, error) {
 	const op = errors.Op("jimm.updateControllerCloudCredential")
 
-	attr := cred.Attributes
-	if attr == nil {
-		var err error
-		attr, err = j.getCloudCredentialAttributes(ctx, cred)
-		if err != nil {
-			return nil, errors.E(op, err)
-		}
+	var err error
+	attr, err := j.getCloudCredentialAttributes(ctx, cred)
+	if err != nil {
+		return nil, errors.E(op, err)
 	}
 
 	models, err := f(ctx, jujuparams.TaggedCredential{
@@ -302,7 +283,6 @@ func (j *JIMM) ForEachUserCloudCredential(ctx context.Context, u *dbmodel.Identi
 	errStop := errors.E("stop")
 	var iterErr error
 	err := j.Database.ForEachCloudCredential(ctx, u.Name, cloud, func(cred *dbmodel.CloudCredential) error {
-		cred.Attributes = nil
 		iterErr = f(cred)
 		if iterErr != nil {
 			return errStop
@@ -365,17 +345,6 @@ func (j *JIMM) GetCloudCredentialAttributes(ctx context.Context, user *openfga.U
 func (j *JIMM) getCloudCredentialAttributes(ctx context.Context, cred *dbmodel.CloudCredential) (map[string]string, error) {
 	const op = errors.Op("jimm.getCloudCredentialAttributes")
 
-	if !cred.AttributesInVault {
-		if err := j.Database.GetCloudCredential(ctx, cred); err != nil {
-			return nil, errors.E(op, err)
-		}
-		return map[string]string(cred.Attributes), nil
-	}
-
-	// Attributes have to be loaded from vault.
-	if j.CredentialStore == nil {
-		return nil, errors.E(op, errors.CodeServerConfiguration, "vault not configured")
-	}
 	attr, err := j.CredentialStore.Get(ctx, cred.ResourceTag())
 	if err != nil {
 		return nil, errors.E(op, err)