User secrets drain #16454

ycliuhw · 2023-10-18T08:55:47Z

This PR enables drain feature for user secrets.

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
~~Integration tests, with comments saying what you're testing~~
doc.go added or updated in changed packages

QA steps

K8s Controller

juju add-secret token=34ae35facd4  --label label --info "this is a secret"
secret:cknppinmp25c79ror9tg

juju update-secret cknppinmp25c79ror9tg --info info token=token2

juju show-secret cknppinmp25c79ror9tg --revisions
cknppinmp25c79ror9tg:
  revision: 2
  owner: 7d95a240-3e7d-4b6e-8371-419528e45805
  description: info
  label: label
  created: 2023-10-18T08:52:27Z
  updated: 2023-10-18T08:52:43Z
  revisions:
  - revision: 1
    backend: t1-local
    created: 2023-10-18T08:52:27Z
    updated: 2023-10-18T08:52:27Z
  - revision: 2
    backend: t1-local
    created: 2023-10-18T08:52:43Z
    updated: 2023-10-18T08:52:43Z

mkubectl -nt1 get secrets
NAME                     TYPE                                  DATA   AGE
cknppinmp25c79ror9tg-1   Opaque                                1      79s
cknppinmp25c79ror9tg-2   Opaque                                1      63s

juju model-config secret-backend=myvault

mkubectl -nt1 get secrets
NAME         TYPE                                  DATA   AGE

vault kv list -format json t1-${model_uuid: -6}
[
  "cknppinmp25c79ror9tg-1",
  "cknppinmp25c79ror9tg-2"
]

IaaS controller

juju add-secret token=34ae35facd4  --label label1 --info "this is a secret"
secret:ckoa2f80m8g27765fkl0

juju update-secret ckoa2f80m8g27765fkl0 --info info token=token2

juju show-secret ckoa2f80m8g27765fkl0 --revisions
ckoa2f80m8g27765fkl0:
  revision: 2
  owner: 6358238c-30f7-42dc-8655-f7da2a81fd06
  description: info
  label: label1
  created: 2023-10-19T03:23:41Z
  updated: 2023-10-19T03:24:05Z
  revisions:
  - revision: 1
    backend: internal
    created: 2023-10-19T03:23:41Z
    updated: 2023-10-19T03:23:41Z
  - revision: 2
    backend: internal
    created: 2023-10-19T03:24:05Z
    updated: 2023-10-19T03:24:05Z

juju model-config secret-backend
auto

juju:PRIMARY> db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/1",
	"data" : {
		"token" : "MzRhZTM1ZmFjZDQ="
	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : null
}
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/2",
	"data" : {
		"token" : "dG9rZW4y"
	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : null
}

juju model-config secret-backend=myvault

juju:PRIMARY> db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/1",
	"data" : {

	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : {
		"backend-id" : "6530a10ceb49811633e3c311",
		"revision-id" : "ckoa2f80m8g27765fkl0-1"
	}
}
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/2",
	"data" : {

	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : {
		"backend-id" : "6530a10ceb49811633e3c311",
		"revision-id" : "ckoa2f80m8g27765fkl0-2"
	}
}

vault kv list -format json t1-${model_uuid: -6}
[
  "ckoa2f80m8g27765fkl0-1",
  "ckoa2f80m8g27765fkl0-2"
]

juju model-config secret-backend=auto

vault kv list -format json t1-${model_uuid: -6}
{}

db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/1",
	"data" : {
		"token" : "MzRhZTM1ZmFjZDQ="
	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : null
}
{
	"_id" : "6358238c-30f7-42dc-8655-f7da2a81fd06:ckoa2f80m8g27765fkl0/2",
	"data" : {
		"token" : "dG9rZW4y"
	},
	"owner-tag" : "model-6358238c-30f7-42dc-8655-f7da2a81fd06",
	"value-reference" : null
}

charm owned secrets

juju deploy juju-qa-dummy-sink
Deployed "dummy-sink" from charm-hub charm "juju-qa-dummy-sink", revision 7 in channel stable on ubuntu@22.04/stable

juju exec --unit dummy-sink/0 -- secret-add foo=app --label=app
secret://c1af636d-999b-4714-8a5f-154cb4e27dad/ckoci088kp8i0sbicuhg

juju exec --unit dummy-sink/0 -- secret-add --owner unit foo=unit0
secret://c1af636d-999b-4714-8a5f-154cb4e27dad/ckoci288kp8i0sbicui0

juju secrets
ID                    Owner         Rotation  Revision  Last updated
ckoci088kp8i0sbicuhg  dummy-sink    never            1  13 seconds ago
ckoci288kp8i0sbicui0  dummy-sink/0  never            1  5 seconds ago

juju:PRIMARY> db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci088kp8i0sbicuhg/1",
	"data" : {
		"foo" : "YXBw"
	},
	"owner-tag" : "application-dummy-sink"
}
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci288kp8i0sbicui0/1",
	"data" : {
		"foo" : "dW5pdDA="
	},
	"owner-tag" : "unit-dummy-sink-0"
}

juju model-config secret-backend=myvault

vault kv list -format json t1-${model_uuid: -6}
[
  "ckoci088kp8i0sbicuhg-1",
  "ckoci288kp8i0sbicui0-1"
]

juju:PRIMARY> db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci088kp8i0sbicuhg/1",
	"data" : {

	},
	"owner-tag" : "application-dummy-sink",
	"value-reference" : {
		"backend-id" : "6530c94e661dc015de699ef5",
		"revision-id" : "ckoci088kp8i0sbicuhg-1"
	}
}
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci288kp8i0sbicui0/1",
	"data" : {

	},
	"owner-tag" : "unit-dummy-sink-0",
	"value-reference" : {
		"backend-id" : "6530c94e661dc015de699ef5",
		"revision-id" : "ckoci288kp8i0sbicui0-1"
	}
}

juju model-config secret-backend=auto

vault kv list -format json t1-${model_uuid: -6}
{}

juju:PRIMARY> db.secretRevisions.find({},{_id:1, data:1, "owner-tag":1, "value-reference":1}).pretty()
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci088kp8i0sbicuhg/1",
	"data" : {
		"foo" : "YXBw"
	},
	"owner-tag" : "application-dummy-sink",
	"value-reference" : null
}
{
	"_id" : "c1af636d-999b-4714-8a5f-154cb4e27dad:ckoci288kp8i0sbicui0/1",
	"data" : {
		"foo" : "dW5pdDA="
	},
	"owner-tag" : "unit-dummy-sink-0",
	"value-reference" : null
}

Documentation changes

No

Links

Jira card: JUJU-4401

…user secrets;

wallyworld

A few tweaks to look at. I think there's scope on the apiserver side to reduce duplicate logic by putting stuff in common? Also the logic around checking owners was done for apps and units and needs a little tweaking for model owners.
Could you also QA draining of app/unit secrets to be sure there's no accidental breakage there.

apiserver/common/secrets/drain.go

apiserver/common/secrets/drain_test.go

wallyworld · 2023-10-19T04:50:28Z

apiserver/common/secrets/secrets.go

@@ -238,6 +238,8 @@ func backendConfigInfo(
 		// Granted secrets can be consumed in application level for all units.
 		readFilter.ConsumerTags = append(readFilter.ConsumerTags, authApp)
 	case names.ApplicationTag:
+	case names.ModelTag:


Comment at the top of the switch needs updating

// Find secrets owned by the application that should be readable for non leader units.

I think the comment is still accurate because the code here is finding extra filter for leader units.

wallyworld · 2023-10-19T04:53:13Z

apiserver/common/secrets/secrets.go

 	filter func(*coresecrets.SecretMetadata, *coresecrets.SecretRevisionMetadata) bool,
 ) (params.ListSecretResults, error) {
 	var result params.ListSecretResults
 	listFilter := state.SecretsFilter{
 		// TODO: there is a bug that operator agents can't get any unit owned secrets!
-		// Because the authTag here is the application tag, but not unit tag.
-		OwnerTags: []names.Tag{authTag},
+		// Because the ownerTag here is the application tag, but not unit tag.


It could be the model tag for user secrets. These comments and the logic need tweaking. eg if owner tag is a model tag, no need to do the isLeaderUnit check below etc

IsLeaderUnit handles this correctly. It returns false if the tag is not a unit tag.
But I can do one more check here.

wallyworld · 2023-10-19T05:00:54Z

apiserver/facades/controller/usersecretsdrain/drain.go

+}
+
+// GetSecretBackendConfigs gets the config needed to create a client to secret backends.
+func (s *SecretsDrainAPI) GetSecretBackendConfigs(arg params.SecretBackendArgs) (params.SecretBackendConfigResults, error) {


Should these methods be in apiserver/common/secrets ... the code looks duplicated from the secretsmanager facade?

Nah, they are very different. This one is a much simpler version of the facade for agents.
Because the agent facade supports calling by owners and consumers(event CMR), getting secrets by labels, also --peek, --refresh, etc.

wallyworld · 2023-10-19T05:03:11Z

apiserver/facades/controller/usersecretsdrain/register.go

+	if !context.Auth().AuthController() {
+		return nil, apiservererrors.ErrPerm
+	}
+	leadershipChecker, err := context.LeadershipChecker()


Do we need leadership checks for user secrets?

No need for the user secrets stuff, but the shared code in the common package needs them.

cmd/jujud/agent/engine_test.go

cmd/jujud/agent/model/manifolds.go

ycliuhw · 2023-10-19T06:27:55Z

/merge

manadart · 2023-10-19T09:02:09Z

apiserver/facades/agent/secretsdrain/mocks/facade_mock.go

+// Source: github.com/juju/juju/apiserver/facade (interfaces: Context,Authorizer)
+
+// Package mocks is a generated GoMock package.
+package mocks


We don't really need mocks sub-packages. They were only ever introduced to get around circular dependencies due in part to having separate {package} and {package}_test packages. Just stick it in package_mock_test.go

manadart · 2023-10-19T09:03:26Z

apiserver/facades/controller/usersecretsdrain/drain.go

+
+// SecretsDrainAPI is the implementation for the SecretsDrain facade.
+type SecretsDrainAPI struct {
+	*commonsecrets.SecretsDrainAPI


We don't want to embed APIs any more. It makes versioning of embedding APIs cumbersome. Make it a member and call through to it.

#16470 Merge branch `3.3` into `main`: - #16426 - #16435 - #16445 - #16452 - #16451 - #16453 - #16455 - #16456 - #16457 - #16460 - #16461 - #16454 - #16394 - #16469 ``` # Conflicts: # apiserver/common/secrets/access.go # apiserver/common/secrets/access_test.go # apiserver/common/secrets/drain.go # apiserver/common/secrets/drain_test.go # apiserver/common/secrets/mocks/commonsecrets_mock.go # apiserver/common/secrets/watcher.go # apiserver/common/secrets/watcher_test.go # apiserver/facades/agent/provisioner/imagemetadata_test.go # apiserver/facades/agent/secretsdrain/mocks/modelstate.go # apiserver/facades/agent/secretsdrain/mocks/secretsprovider.go # apiserver/facades/agent/secretsdrain/package_test.go # apiserver/facades/agent/secretsdrain/register.go # apiserver/facades/agent/secretsdrain/state.go # cmd/jujud/agent/caasoperator/manifolds.go # environs/bootstrap/bootstrap.go # environs/simplestreams/datasource.go # environs/simplestreams/datasource_test.go # go.sum # worker/dbaccessor/package_test.go # worker/firewaller/firewaller_test.go # worker/modelcache/worker_test.go # worker/secretsdrainworker/manifold.go # worker/secretsdrainworker/manifold_test.go # worker/secretsdrainworker/package_test.go # worker/uniter/charm/bundles_test.go # worker/uniter/uniter_test.go # worker/uniter/util_test.go ```

ycliuhw added 11 commits October 13, 2023 18:43

Move secretsdrain to common package;

5c53bb1

Add usersecretsdrain facade;

7873429

Refactor secretdrainworker to make it reusable;

d4514a1

Add user-secret-drain-worker for draining user secrets;

0f74d32

Rename secretdrainworker to secretsdrainworker;

efcd10a

Grant secret access for user secrets;

4bbaafd

Fix backendConfigInfo and CanManage in apiserver/common/secrets/ for …

32a5aa9

…user secrets;

Add UserSecretsDrain facade for controller;

3492df4

Add UserSecretsDrain facade client;

2c7fd59

Fix secretsdrainworker facade setup;

3b48bab

Fix tests in worker/secretsdrainworker;

0754c4c

hpidcock added the 3.3 label Oct 18, 2023

ycliuhw added 2 commits October 19, 2023 14:32

Fix tests at cmd/jujud/agent;

55b9b8a

Cleanup apiserver/facades/controller/usersecretsdrain;

9c3618e

wallyworld approved these changes Oct 19, 2023

View reviewed changes

ycliuhw added 2 commits October 19, 2023 17:04

Move secretBackendModelConfigWatcher to separate file;

0198d2b

Rename user-secret-drain-worker to user-secrets-drain-worker;

8f28661

jujubot merged commit a4cc935 into juju:3.3 Oct 19, 2023
17 of 20 checks passed

manadart reviewed Oct 19, 2023

View reviewed changes

ycliuhw mentioned this pull request Oct 19, 2023

Merge 3.3 #16470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

User secrets drain #16454

User secrets drain #16454

ycliuhw commented Oct 18, 2023 •

edited

wallyworld left a comment

wallyworld Oct 19, 2023

ycliuhw Oct 19, 2023 •

edited

wallyworld Oct 19, 2023

ycliuhw Oct 19, 2023

wallyworld Oct 19, 2023

ycliuhw Oct 19, 2023

wallyworld Oct 19, 2023

ycliuhw Oct 19, 2023

ycliuhw commented Oct 19, 2023

manadart Oct 19, 2023

manadart Oct 19, 2023

User secrets drain #16454

User secrets drain #16454

Conversation

ycliuhw commented Oct 18, 2023 • edited

Checklist

QA steps

Documentation changes

Links

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycliuhw Oct 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycliuhw commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycliuhw commented Oct 18, 2023 •

edited

ycliuhw Oct 19, 2023 •

edited