CSS-4936 Postgres as secret backend #1011

kian99 · 2023-07-26T09:01:25Z

Description

Added Postgres as a secret backend so that it satisfies the CredentialStore interface. JIMM will only use Postgres as a secret backend if Vault is not available and an explicit env var is set.

Also refactored slightly our migration code as that didn't work when a new migration was added.

Engineering checklist

Check only items that apply

Documentation updated
Covered by unit tests
Covered by integration tests

Test instructions

Tested by spinning up the Docker compose and curling the well-known endpoint and verifying the data in Postgres.

kian99 · 2023-07-26T09:02:02Z

charms/jimm-k8s/tests/unit/test_charm.py

@@ -189,6 +196,34 @@ def test_bakery_configuration(self):
            },
        )

+    def test_audit_log_retention_config(self):


Added this test as I noticed nothing was testing it.

kian99 · 2023-07-26T09:02:19Z

charms/jimm-k8s/tests/unit/test_charm.py

-    "audit-log-retention-period-in-days": "10",
+}
+
+EXPECTED_ENV = {


Drive-by fix for less duplication in the k8s charm tests.

kian99 · 2023-07-26T09:07:23Z

internal/db/db.go

@@ -70,7 +70,7 @@ func (d *Database) Migrate(ctx context.Context, force bool) error {
 	}

 	for {
-		v := dbmodel.Version{Component: dbmodel.Component}
+		v := dbmodel.Version{Component: dbmodel.Component, Major: 1, Minor: 0}


So to quickly discuss the changes to the migrations, when I added a migration it didn't take effect because the code didn't quite work right. Essentially the problem boiled down to the fact that our first migration was 0_0 and the default value for the struct/DB was also 0 so we had no way of knowing if a migration had taken place after the initial one was done. So the simplest solution is to start our migrations from 1. That is why you will notice our migrations now start at 1_1. This change is best left to feature-rebac since we haven't deployed it (for our staging deploy I can manually tweak things in the DB) and the schema is different to main anyway.

Sounds good to me, but I'm not sure how we were able to migrate up to this point? Perhaps discuss this in standup later / explain it as I don't think I fully understand.

The first migration worked, but the second wouldn't. At the end of the first migration there was a line UPDATE versions SET major=1, minor=0 WHERE component='jimmdb'; now after that migration takes place, the code will query the versions table and see that MajorDesired==MajorSet and MinorRequired>MinorSet so it would try to run the next migration but it doesn't know what the next migration is, and that's why I had to add the Minor+=1 line. And the last thing is that when I added that line but the first migration starts at 0, we have a problem.

internal/db/secrets.go

ale8k · 2023-07-26T09:46:16Z

internal/db/secrets.go

+// newSecret creates a secret object with the time set to the current time
+// and the type and tag fields set from the tag object
+func newSecret(secretType string, secretTag string, data []byte) dbmodel.Secret {
+	return dbmodel.Secret{Time: time.Now(), Type: secretType, Tag: secretTag, Data: data}


This probably needs validation, I notice we check if tag and type exist and duplicate it a lot, couldn't this just return an error and we do it in the constructor for the secret?

What kind of validation? And not sure what you mean by

we check if tag and type exist and duplicate it a lot

if secret.Tag == "" || secret.Type == "" { return errors.E(op, "missing secret tag and type", errors.CodeBadRequest) }

Thing like this ^

internal/db/secrets.go

internal/dbmodel/version.go

ale8k · 2023-07-26T09:53:59Z

service.go

@@ -299,6 +299,11 @@ func NewService(ctx context.Context, p Params) (*Service, error) {
 	}
 	if vs != nil {
 		s.jimm.CredentialStore = vs
+	} else {
+		// Only enable Postgres storage for secrets if explictly enabled.
+		if _, ok := os.LookupEnv("INSECURE_SECRET_STORAGE"); ok {


What if this isn't ok, the service will just fail to put secrets right?

Yes, which is the existing behaviour.

Let's discuss this later too, we should exit here I feel

ale8k

Left a fair few comments sorry, will update once resolved

babakks

Nice job. Just had a few comments.

cmd/jimmsrv/main.go

docker-compose.yaml

internal/db/secrets.go

internal/db/secrets_test.go

service.go

internal/db/secrets.go

- Added logging - Fixed broken test - Fixed cleanup JWKS function

ale8k

lgtm now

kian99 added 4 commits July 25, 2023 18:23

Added Postgres as secret backend

332a8bf

Fix migrations and cleaned up secret implementation

35dbbba

Added tests

f386ea9

merge feature-rebac

4ff622d

kian99 changed the title ~~CSS - 4936 Postgres as secret backend~~ CSS-4936 Postgres as secret backend Jul 26, 2023

kian99 added 4 commits July 26, 2023 11:03

Removed env files

b2ba450

Revert compose changes to use Vault

0efd132

Added godocs

f86dcc5

Update 1_2.sql

3f5c7f8

kian99 commented Jul 26, 2023

View reviewed changes

kian99 requested review from alesstimec, ale8k, babakks and mina1460 July 26, 2023 09:11