From 8844dffa8b14e215ba31b36a0e4b78aa81dbd0e4 Mon Sep 17 00:00:00 2001 From: Kian Parvin Date: Wed, 26 Jul 2023 14:42:53 +0200 Subject: [PATCH] PR Tweaks - Added logging - Fixed broken test - Fixed cleanup JWKS function --- docker-compose.yaml | 1 + internal/db/secrets.go | 38 +++++++++++++++++++++-- internal/db/secrets_test.go | 53 +++++++++++++++++++++++++++----- internal/dbmodel/version_test.go | 14 +++------ service.go | 1 + 5 files changed, 88 insertions(+), 19 deletions(-) diff --git a/docker-compose.yaml b/docker-compose.yaml index d2cb73ec4..b0165a152 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -48,6 +48,7 @@ services: VAULT_PATH: "/jimm-kv/" VAULT_SECRET_FILE: "/vault/approle.json" VAULT_AUTH_PATH: "/auth/approle/login" + # Note: By default we should use Vault as that is the primary means of secret storage. # INSECURE_SECRET_STORAGE: "enabled" # JIMM_DASHBOARD_LOCATION: "" JIMM_DNS_NAME: "jimm.localhost" diff --git a/internal/db/secrets.go b/internal/db/secrets.go index 4d9d598c7..35a9c7210 100644 --- a/internal/db/secrets.go +++ b/internal/db/secrets.go @@ -10,7 +10,9 @@ import ( "github.com/canonical/jimm/internal/dbmodel" "github.com/canonical/jimm/internal/errors" "github.com/juju/names/v4" + "github.com/juju/zaputil/zapctx" "github.com/lestrrat-go/jwx/v2/jwk" + "go.uber.org/zap" "gorm.io/gorm/clause" ) @@ -46,7 +48,7 @@ func (d *Database) UpsertSecret(ctx context.Context, secret *dbmodel.Secret) err // GetSecret gets the secret with the specified type and tag. func (d *Database) GetSecret(ctx context.Context, secret *dbmodel.Secret) error { - const op = errors.Op("db.AddSecret") + const op = errors.Op("db.GetSecret") if err := d.ready(); err != nil { return errors.E(op, err) } @@ -90,11 +92,13 @@ func (d *Database) Get(ctx context.Context, tag names.CloudCredentialTag) (map[s secret := dbmodel.NewSecret(tag.Kind(), tag.String(), nil) err := d.GetSecret(ctx, &secret) if err != nil { + zapctx.Error(ctx, "failed to get secret data", zap.Error(err)) return nil, errors.E(op, err) } var attr map[string]string err = json.Unmarshal(secret.Data, &attr) if err != nil { + zapctx.Error(ctx, "failed to unmarshal secret data", zap.Error(err)) return nil, errors.E(op, err) } return attr, nil @@ -108,6 +112,7 @@ func (d *Database) Put(ctx context.Context, tag names.CloudCredentialTag, attr m } dataJson, err := json.Marshal(attr) if err != nil { + zapctx.Error(ctx, "failed to marshal secret data", zap.Error(err)) return errors.E(op, err, "failed to marshal secret data") } secret := dbmodel.NewSecret(tag.Kind(), tag.String(), dataJson) @@ -126,11 +131,13 @@ func (d *Database) GetControllerCredentials(ctx context.Context, controllerName secret := dbmodel.NewSecret(names.ControllerTagKind, controllerName, nil) err := d.GetSecret(ctx, &secret) if err != nil { + zapctx.Error(ctx, "failed to get secret data", zap.Error(err)) return "", "", errors.E(op, err) } var secretData map[string]string err = json.Unmarshal(secret.Data, &secretData) if err != nil { + zapctx.Error(ctx, "failed to unmarshal secret data", zap.Error(err)) return "", "", errors.E(op, err) } username, ok := secretData[usernameKey] @@ -152,6 +159,7 @@ func (d *Database) PutControllerCredentials(ctx context.Context, controllerName secretData[passwordKey] = password dataJson, err := json.Marshal(secretData) if err != nil { + zapctx.Error(ctx, "failed to unmarshal secret data", zap.Error(err)) return errors.E(op, err, "failed to marshal secret data") } secret := dbmodel.NewSecret(names.ControllerTagKind, controllerName, dataJson) @@ -162,7 +170,24 @@ func (d *Database) PutControllerCredentials(ctx context.Context, controllerName func (d *Database) CleanupJWKS(ctx context.Context) error { const op = errors.Op("database.CleanupJWKS") secret := dbmodel.NewSecret(jwksKind, jwksPublicKeyTag, nil) - return d.DeleteSecret(ctx, &secret) + err := d.DeleteSecret(ctx, &secret) + if err != nil { + zapctx.Error(ctx, "failed to cleanup jwks public key", zap.Error(err)) + return errors.E(op, err, "failed to cleanup jwks public key") + } + secret = dbmodel.NewSecret(jwksKind, jwksPrivateKeyTag, nil) + err = d.DeleteSecret(ctx, &secret) + if err != nil { + zapctx.Error(ctx, "failed to cleanup jwks private key", zap.Error(err)) + return errors.E(op, err, "failed to cleanup jwks private key") + } + secret = dbmodel.NewSecret(jwksKind, jwksExpiryTag, nil) + err = d.DeleteSecret(ctx, &secret) + if err != nil { + zapctx.Error(ctx, "failed to cleanup jwks expiry info", zap.Error(err)) + return errors.E(op, err, "failed to cleanup jwks expiry info") + } + return nil } // GetJWKS returns the current key set stored within the DB. @@ -171,10 +196,12 @@ func (d *Database) GetJWKS(ctx context.Context) (jwk.Set, error) { secret := dbmodel.NewSecret(jwksKind, jwksPublicKeyTag, nil) err := d.GetSecret(ctx, &secret) if err != nil { + zapctx.Error(ctx, "failed to get jwks data", zap.Error(err)) return nil, errors.E(op, err) } ks, err := jwk.ParseString(string(secret.Data)) if err != nil { + zapctx.Error(ctx, "failed to parse jwk data", zap.Error(err)) return nil, errors.E(op, err) } return ks, nil @@ -186,11 +213,13 @@ func (d *Database) GetJWKSPrivateKey(ctx context.Context) ([]byte, error) { secret := dbmodel.NewSecret(jwksKind, jwksPrivateKeyTag, nil) err := d.GetSecret(ctx, &secret) if err != nil { + zapctx.Error(ctx, "failed to get jwks jwks private key", zap.Error(err)) return nil, errors.E(op, err) } var pem []byte err = json.Unmarshal(secret.Data, &pem) if err != nil { + zapctx.Error(ctx, "failed to unmarshal pem data data", zap.Error(err)) return nil, errors.E(op, err) } return pem, nil @@ -202,11 +231,13 @@ func (d *Database) GetJWKSExpiry(ctx context.Context) (time.Time, error) { secret := dbmodel.NewSecret(jwksKind, jwksExpiryTag, nil) err := d.GetSecret(ctx, &secret) if err != nil { + zapctx.Error(ctx, "failed to get jwks expiry", zap.Error(err)) return time.Time{}, errors.E(op, err) } var expiryTime time.Time err = json.Unmarshal(secret.Data, &expiryTime) if err != nil { + zapctx.Error(ctx, "failed to unmarshal jwks expiry data", zap.Error(err)) return time.Time{}, errors.E(op, err) } return expiryTime, nil @@ -217,6 +248,7 @@ func (d *Database) PutJWKS(ctx context.Context, jwks jwk.Set) error { const op = errors.Op("database.PutJWKS") jwksJson, err := json.Marshal(jwks) if err != nil { + zapctx.Error(ctx, "failed to marshal jwks", zap.Error(err)) return errors.E(op, err, "failed to marshal jwks data") } secret := dbmodel.NewSecret(jwksKind, jwksPublicKeyTag, jwksJson) @@ -229,6 +261,7 @@ func (d *Database) PutJWKSPrivateKey(ctx context.Context, pem []byte) error { const op = errors.Op("database.PutJWKSPrivateKey") privateKeyJson, err := json.Marshal(pem) if err != nil { + zapctx.Error(ctx, "failed to marshal pem data", zap.Error(err)) return errors.E(op, err, "failed to marshal jwks private key") } secret := dbmodel.NewSecret(jwksKind, jwksPrivateKeyTag, privateKeyJson) @@ -240,6 +273,7 @@ func (d *Database) PutJWKSExpiry(ctx context.Context, expiry time.Time) error { const op = errors.Op("database.PutJWKSExpiry") expiryJson, err := json.Marshal(expiry) if err != nil { + zapctx.Error(ctx, "failed to marshal jwks expiry data", zap.Error(err)) return errors.E(op, err, "failed to marshal jwks data") } secret := dbmodel.NewSecret(jwksKind, jwksExpiryTag, expiryJson) diff --git a/internal/db/secrets_test.go b/internal/db/secrets_test.go index f5157a261..a2cdc34e7 100644 --- a/internal/db/secrets_test.go +++ b/internal/db/secrets_test.go @@ -29,6 +29,13 @@ func (s *dbSuite) TestInsertSecret(c *qt.C) { Data: nil, } c.Assert(s.Database.DB.Create(&u).Error, qt.IsNil) + secret := dbmodel.Secret{} + tx := s.Database.DB.First(&secret) + c.Assert(tx.Error, qt.IsNil) + c.Assert(secret.Time, qt.Equals, testTime) + c.Assert(secret.Type, qt.Equals, "generic") + c.Assert(secret.Tag, qt.Equals, "123") + c.Assert(secret.Data, qt.IsNil) } func (s *dbSuite) TestUpsertSecret(c *qt.C) { @@ -162,6 +169,14 @@ func getJWKS(c *qt.C) jwk.Set { "n": "yeNlzlub94YgerT030codqEztjfU_S6X4DbDA_iVKkjAWtYfPHDzz_sPCT1Axz6isZdf3lHpq_gYX4Sz-cbe4rjmigxUxr-FgKHQy3HeCdK6hNq9ASQvMK9LBOpXDNn7mei6RZWom4wo3CMvvsY1w8tjtfLb-yQwJPltHxShZq5-ihC9irpLI9xEBTgG12q5lGIFPhTl_7inA1PFK97LuSLnTJzW0bj096v_TMDg7pOWm_zHtF53qbVsI0e3v5nmdKXdFf9BjIARRfVrbxVxiZHjU6zL6jY5QJdh1QCmENoejj_ytspMmGW7yMRxzUqgxcAqOBpVm0b-_mW3HoBdjQ", "e": "AQAB", "kid": "32d2b213-d3fe-436c-9d4c-67a673890620" + }, + { + "alg": "RS256", + "kty": "RSA", + "use": "sig", + "n": "jbglzlub94YgerT030codqEztjfU_S6X4DbDA_iVKkjAWtYfPHDzz_sPCT1Axz6isZdf3lHpq_gYX4Sz-cbe4rjmigxUxr-FgKHQy3HeCdK6hNq9ASQvMK9LBOpXDNn7mei6RZWom4wo3CMvvsY1w8tjtfLb-yQwJPltHxShZq5-ihC9irpLI9xEBTgG12q5lGIFPhTl_7inA1PFK97LuSLnTJzW0bj096v_TMDg7pOWm_zHtF53qbVsI0e3v5nmdKXdFf9BjIARRfVrbxVxiZHjU6zL6jY5QJdh1QCmENoejj_ytspMmGW7yMRxzUqgxcAqOBpVm0b-_mW3HoBdjQ", + "e": "FEGB", + "kid": "32d2b213-d3fe-436c-9d4c-67a673890621" } ] } @@ -186,14 +201,16 @@ func (s *dbSuite) TestPutAndGetJWKS(c *qt.C) { gotJwks, err := s.Database.GetJWKS(ctx) c.Assert(err, qt.IsNil) ki := gotJwks.Keys(ctx) - ki.Next(ctx) - key := ki.Pair().Value.(jwk.Key) - - _, err = uuid.Parse(key.KeyID()) - c.Assert(err, qt.IsNil) - - c.Assert(key.KeyUsage(), qt.Equals, "sig") - c.Assert(key.Algorithm(), qt.Equals, jwa.RS256) + gotOneKey := false + for ki.Next(ctx) { + gotOneKey = true + key := ki.Pair().Value.(jwk.Key) + _, err = uuid.Parse(key.KeyID()) + c.Assert(err, qt.IsNil) + c.Assert(key.KeyUsage(), qt.Equals, "sig") + c.Assert(key.Algorithm(), qt.Equals, jwa.RS256) + } + c.Assert(gotOneKey, qt.IsTrue) } func (s *dbSuite) TestPutAndGetJwksExpiry(c *qt.C) { @@ -231,3 +248,23 @@ func (s *dbSuite) TestPutAndGetJwksPrivateKey(c *qt.C) { c.Assert(err, qt.IsNil) c.Assert(gotPem, qt.DeepEquals, pem) } + +func (s *dbSuite) TestCleanupJWKS(c *qt.C) { + err := s.Database.Migrate(context.Background(), true) + c.Assert(err, qt.Equals, nil) + ctx := context.Background() + pem := []byte("123") + c.Assert(s.Database.PutJWKSPrivateKey(ctx, pem), qt.IsNil) + expiryTime := time.Date(2023, 7, 26, 0, 0, 0, 0, time.UTC) + c.Assert(s.Database.PutJWKSExpiry(ctx, expiryTime), qt.IsNil) + jwks := getJWKS(c) + c.Assert(s.Database.PutJWKS(ctx, jwks), qt.IsNil) + // Verify 3 secrets exist + var count int64 + c.Assert(s.Database.DB.Model(&dbmodel.Secret{}).Count(&count).Error, qt.IsNil) + c.Assert(count, qt.Equals, int64(3)) + // Verify all JWKS secrets are removed + c.Assert(s.Database.CleanupJWKS(ctx), qt.IsNil) + c.Assert(s.Database.DB.Model(&dbmodel.Secret{}).Count(&count).Error, qt.IsNil) + c.Assert(count, qt.Equals, int64(0)) +} diff --git a/internal/dbmodel/version_test.go b/internal/dbmodel/version_test.go index 9b3d28815..533ad83b6 100644 --- a/internal/dbmodel/version_test.go +++ b/internal/dbmodel/version_test.go @@ -6,7 +6,6 @@ import ( "testing" qt "github.com/frankban/quicktest" - "gorm.io/gorm" "github.com/canonical/jimm/internal/dbmodel" ) @@ -17,21 +16,18 @@ func TestVersion(t *testing.T) { var v0 dbmodel.Version result := db.First(&v0, "component = ?", dbmodel.Component) - c.Check(result.Error, qt.Equals, gorm.ErrRecordNotFound) + c.Check(result.Error, qt.IsNil) + c.Check(v0.Major, qt.DeepEquals, dbmodel.Major) + c.Check(v0.Minor, qt.Equals, dbmodel.Minor) v1 := dbmodel.Version{ Component: dbmodel.Component, Major: dbmodel.Major, Minor: dbmodel.Minor, } - result = db.Create(&v1) + result = db.First(&v1, "component = ?", dbmodel.Component) c.Assert(result.Error, qt.IsNil) - c.Check(result.RowsAffected, qt.Equals, int64(1)) - - var v2 dbmodel.Version - result = db.First(&v2, "component = ?", dbmodel.Component) - c.Assert(result.Error, qt.IsNil) - c.Check(v2, qt.DeepEquals, v1) + c.Check(v1, qt.DeepEquals, v1) v3 := dbmodel.Version{ Component: dbmodel.Component, diff --git a/service.go b/service.go index 82af0bd9b..0d2b0be1d 100644 --- a/service.go +++ b/service.go @@ -302,6 +302,7 @@ func NewService(ctx context.Context, p Params) (*Service, error) { } else { // Only enable Postgres storage for secrets if explictly enabled. if _, ok := os.LookupEnv("INSECURE_SECRET_STORAGE"); ok { + zapctx.Warn(ctx, "using plaintext postgres for secret storage") s.jimm.CredentialStore = &s.jimm.Database } }