creds endpoint #4

tvoran · 2022-05-12T06:35:04Z

Overview

Implementation of the creds endpoint for generating the credentials for the service account.

Design of Change

The creds endpoint has these parameters:

name (string: <required>): The name of the role.
kubernetes_namespace (string: <required>): The name of the Kubernetes namespace in which to generate the service account.
cluster_role_binding (bool: false): If true, generate a ClusterRoleBinding to grant permissions across the whole cluster instead of within a namespace. Requires the Vault role to have kubernetes_role_type set to ClusterRole.
ttl (string: ""): The ttl of the generated Kubernetes service account.

Other notes:

all created objects have a set of standard labels applied:

	"app.kubernetes.io/managed-by": "HashiCorp-Vault",
	"app.kubernetes.io/created-by": "vault-plugin-secrets-kubernetes",

The wal rollback integration test take about 5 minutes to run; it can be skipped by setting SKIP_WAL_TEST in the env:

make setup-integration-test integration-test TESTARGS="-v" SKIP_WAL_TEST=yes

When the creds create fails in the wal tests, something retries the path_creds request a couple time, so three sets of orphaned objects are created. They all are cleaned up properly by the WAL, but I'm not sure what's retrying the path_creds call. Nevermind, this is probably just the built-in retries in the vault client we're using in the tests.

Testing examples

The integration tests can be run as described in the README.

Manual testing:

# setup a k8s cluster preloaded with service accounts and roles and a vault pod with the built k8s secrets plugin
make setup-kind setup-integration-test

# get a shell on the vault pod that has the plugin available and configure it
kubectl exec -it -n test vault-0 -- /bin/sh
vault secrets enable kubernetes-dev
vault write -f kubernetes-dev/config

# configure roles (in the vault container)
vault write kubernetes-dev/roles/existing-role \
  kubernetes_role_name=test-role-list-pods  \
  allowed_kubernetes_namespaces='default' \
  allowed_kubernetes_namespaces=test \
  token_max_ttl=80h \
  token_default_ttl=1h

vault write kubernetes-dev/roles/my-role \
  service_account_name=sample-app \
  allowed_kubernetes_namespaces='test' \
  token_max_ttl=80h \
  token_default_ttl=1h

vault write kubernetes-dev/roles/rules \
  allowed_kubernetes_namespaces=default \
  allowed_kubernetes_namespaces=test \
  token_max_ttl=80h \
  token_default_ttl=1h \
  generated_role_rules="rules:
- apiGroups:
  - ""
  resources:
  - pods
  verbs:
  - list"

# generate service account tokens (in the vault container)
vault write kubernetes-dev/creds/my-role kubernetes_namespace=test

vault write kubernetes-dev/creds/existing-role kubernetes_namespace=test ttl=30m

vault write kubernetes-dev/creds/rules kubernetes_namespace=test ttl=7h

# use the creds (in a local terminal)
export KUBE_HOST=$(kubectl config view --raw --minify --flatten -o jsonpath='{.clusters[].cluster.server}')

export TEST_TOKEN=<service_account_token>
curl -k $KUBE_HOST/api/v1/namespaces/test/pods --header "Authorization: Bearer $TEST_TOKEN"

You can also configure this with Vault running outside kubernetes, something like this:

make setup-kind setup-integration-test

./scripts/local_dev.sh

# then in a new terminal
export KUBE_HOST=$(kubectl config view --raw --minify --flatten -o jsonpath='{.clusters[].cluster.server}')
export KUBE_CA_CERT=$(kubectl get secret -n test $(kubectl get serviceaccount -n test test-token-create -o jsonpath='{.secrets[0].name}') -o jsonpath='{ .data.ca\.crt }' | base64 --decode)
export TOKEN=$(kubectl get secret -n test $(kubectl get serviceaccount -n test test-token-create -o jsonpath='{.secrets[0].name}') -o jsonpath='{ .data.token }' | base64 --decode)

export VAULT_DEV_ROOT_TOKEN_ID="root" VAULT_TOKEN="root" VAULT_ADDR="http://127.0.0.1:8200"
vault write kube-secrets/config kubernetes_host=$KUBE_HOST service_account_jwt=$TOKEN kubernetes_ca_cert=$KUBE_CA_CERT

# then similar configure roles and generate creds as above

Generates k8s service accounts for the three operation modes: existing service account, existing role, and creating all objects from given role rules.

Tests Role and ClusterRole types, RoleBinding and ClusterRoleBinding creds options. Also uses more common testing functions.

For kubernetes_role_name, set the RoleBinding/ClusterRoleBinding as the owner of the ServiceAccount. For generated_role_rules, set the Role/ClusterRole as the owning object. Only the owner objects will have WAL entries written.

Check the annotations, labels, and rules/role references for created k8s objects in the integration tests. Adds a new service account (super-jwt) that has extra abilities to `get` the k8s objects.

return logical.ErrorResponse's when validation checks fail.

Can be skipped by setting SKIP_WAL_TEST in env.

backend.go

Makefile

client.go

path_config.go

path_creds.go

client.go

go.mod

path_config.go

path_creds.go

path_creds_test.go

tomhjp

Looking good - I've still got the integration tests and path_creds.go to look at, sorry for the partial review.

backend.go

client.go

wal.go

path_roles.go

Apply standard labels after user labels, so users can't override them. Set ForwardPerformanceSecondary and ForwardPerformanceStandby for the creds calls. Cap the lease and token ttl at creds time to the token_max_ttl and/or the system/mount max ttl.

More comments and logging. Directly assign the CACert instead of append'ing.

path_creds.go

tomhjp · 2022-05-19T13:31:14Z

path_creds.go

+		"created_role_type":         role.K8sRoleType,
+	})
+
+	resp.Secret.TTL = theTTL


It's possible the the k8s API may give us back a token with a TTL different from what we requested: https://kubernetes.io/docs/reference/kubernetes-api/authentication-resources/token-request-v1/#TokenRequestSpec

e.g. IIRC the minimum token TTL is 10 minutes. And there is also a kube-apiserver flag --service-account-max-token-expiration.

I haven't thought through how we should handle that too much, but we should at least add a warning to the response if there's a disparity.

If the returned TTL is longer, we can still effectively revoke early if there is a service account or role binding to delete, but we're a bit stuck if we're only generating a token.

If the returned TTL is shorter, we should probably pin the Vault lease down to the shorter TTL as well?

Yep, good points. I added the warnings and set the Vault lease if the token TTL is shorter in c3e9ee6.

When requesting a token, apparently we could also tie it to a k8s object via the boundObjectRef. So in the future perhaps we could create a simple Secret or something that is bound to the token, and would allow us to "revoke" the token in the existing service account case.

I love that idea, I just tested it out and it works perfectly:

kubectl create secret generic foo kubectl create token default --bound-object-kind Secret --bound-object-name foo # The `foo` secret is still empty - no data attached. kubectl proxy & curl --silent http://127.0.0.1:8001/apis/authentication.k8s.io/v1/tokenreviews -H "Content-Type: application/json" -X POST -d '{"apiVersion": "authentication.k8s.io/v1", "kind": "TokenReview", "spec": {"token":"<JWT>"}}' | jq # Authenticated kubectl delete secret foo # Repeating the TokenReview request now returns an error

Not in this PR, but we should definitely implement that.

path_creds.go

wal.go

tomhjp · 2022-05-19T14:49:15Z

path_creds.go

+	if !ok {
+		return logical.ErrorResponse("'kubernetes_namespace' is required"), nil
+	}


Would it maybe make sense to allow omitting kubernetes_namespace if there is only one namespace specified? It could simplify the UX in that case, but maybe it also has potential to make things more confusing. In any case, this definitely doesn't need addressing in this PR, as it's easy to relax the rule later.

Yeah! I think that would be sensible default behavior.

Co-authored-by: Tom Proctor <[email protected]>

Do some template validation on role create/update. Test a custom name template in one of the integration tests. Write the WAL entry before the k8s object is created. Add more doc strings about the WAL + owner reference scheme. Add missing WAL giving up log message for rollbackRoleWAL.

…secrets-kubernetes into VAULT-5671/creds

Use a single mutex lock for getClient(). Check the k8s token ttl and return a warning if it's larger than the lease ttl, also warn and set the lease TTL = token TTL if the token's is shorter. Also test the RoleBinding WAL path in wal_rollback_test.go.

calvn

👍 Other than the open suggestions from others, and a follow up to Christopher's comment I didn't find much else to add.

path_creds.go

wal.go

path_creds.go

since vault is building with go 1.17

tomhjp · 2022-05-20T10:02:32Z

wal.go

+// Eventually expire the WAL if for some reason the rollback operation consistently fails
+var maxWALAge = 24 * time.Hour
+
+func (b *backend) walRollback(ctx context.Context, req *logical.Request, kind string, data interface{}) error {


AFAICT I think it's currently possible for createCreds and walRollback to run simultaneously, which could result in walRollback revoking some credentials that are part way through being provisioned.

That is mitigated somewhat by the WALRollbackMinAge, which defaults to 10 minutes: https://github.com/hashicorp/vault/blob/ab0944d9653f41df7c8091adbbf3b8e516a2f58e/sdk/framework/backend.go#L577-L582
i.e. credentials/objects in K8s need to have been created for at least 10 minutes before they'll start being processed in walRollback. This is also why the integration test takes ~10 minutes to run since there's a rollback test.

Ahh cool, I missed that. Sounds fine then. Although on another note, maybe it would be good to try and bring that integration test time down in a future PR.

Ohh, I didn't realize that reverting to the default value also delays the test by that amount. Is there a way to just reduce the min age for the test?

Yeah we'll have to look into changing that for the integration tests. Perhaps with some compile flags or something.

path_creds.go

tomhjp

🎉 No blockers from me

path_creds.go

tvoran added 11 commits May 9, 2022 16:55

Adding creds endpoint

c6336f5

Generates k8s service accounts for the three operation modes: existing service account, existing role, and creating all objects from given role rules.

wal implementation

6aa7f10

test major operating modes with integration tests

6e6cbf5

Tests Role and ClusterRole types, RoleBinding and ClusterRoleBinding creds options. Also uses more common testing functions.

use owner references

f27f412

For kubernetes_role_name, set the RoleBinding/ClusterRoleBinding as the owner of the ServiceAccount. For generated_role_rules, set the Role/ClusterRole as the owning object. Only the owner objects will have WAL entries written.

bumping go and k8s versions

8362875

fix integration tests and bump k8s & go versions

14ab958

adding standard labels and case Role/ClusterRole

a5ddce4

verify k8s objects created in integration tests

e53945e

Check the annotations, labels, and rules/role references for created k8s objects in the integration tests. Adds a new service account (super-jwt) that has extra abilities to `get` the k8s objects.

use k8s pkg's v0.22.2 to match vault

ef16dc1

cleanup debug and error responses

327eb00

return logical.ErrorResponse's when validation checks fail.

adds WAL rollback integration test

5edea13

Can be skipped by setting SKIP_WAL_TEST in env.

tvoran marked this pull request as ready for review May 17, 2022 01:10

tvoran requested review from tomhjp, swenson and calvn May 17, 2022 06:36

calvn reviewed May 18, 2022

View reviewed changes

swenson reviewed May 18, 2022

View reviewed changes

client.go Outdated Show resolved Hide resolved

go.mod Show resolved Hide resolved

path_config.go Show resolved Hide resolved

path_creds.go Outdated Show resolved Hide resolved

path_creds_test.go Show resolved Hide resolved

feedback cleanup

b6c4270

tomhjp reviewed May 18, 2022

View reviewed changes

tvoran added 5 commits May 18, 2022 14:42

run the wal rollback tests in parallel

27bebca

labels order, forward, and ttl caps

108b38a

Apply standard labels after user labels, so users can't override them. Set ForwardPerformanceSecondary and ForwardPerformanceStandby for the creds calls. Cap the lease and token ttl at creds time to the token_max_ttl and/or the system/mount max ttl.

rename token_ttl to token_default_ttl

875708b

store the normalized roletype, logging, cacert

c1b83ea

More comments and logging. Directly assign the CACert instead of append'ing.

fix tests

21d37fc

tomhjp reviewed May 19, 2022

View reviewed changes

tvoran and others added 4 commits May 19, 2022 11:29

Apply suggestions from code review

918c840

Co-authored-by: Tom Proctor <[email protected]>

Merge branch 'VAULT-5671/creds' of github.com:hashicorp/vault-plugin-…

ba0c0ef

…secrets-kubernetes into VAULT-5671/creds

calvn approved these changes May 19, 2022

View reviewed changes

path_creds.go Outdated Show resolved Hide resolved

wal.go Outdated Show resolved Hide resolved

calvn reviewed May 19, 2022

View reviewed changes

path_creds.go Outdated Show resolved Hide resolved

tvoran added 4 commits May 19, 2022 17:34

lock after checking the client

c135d17

another backend -> engine correction

f1861e7

update rollback comments

f557604

set go 1.17 in go.mod and tidy

626d345

since vault is building with go 1.17

tomhjp reviewed May 20, 2022

View reviewed changes

path_creds.go Outdated Show resolved Hide resolved

path_creds.go Show resolved Hide resolved

path_creds.go Show resolved Hide resolved

tomhjp mentioned this pull request May 20, 2022

Add usage documentation for new Kubernetes Secrets Engine hashicorp/vault#15527

Merged

tomhjp reviewed May 20, 2022

View reviewed changes

path_creds.go Outdated Show resolved Hide resolved

only one walID needed

edaa89e

tomhjp approved these changes May 20, 2022

View reviewed changes

tomhjp reviewed May 20, 2022

View reviewed changes

path_creds.go Outdated Show resolved Hide resolved

tvoran added 3 commits May 20, 2022 08:24

call DeleteWAL() at the end of createCreds()

45c1f25

don't support GET for creds

9f3947e

lock at start of getClient

02aea05

tvoran merged commit f61499d into main May 20, 2022

tvoran deleted the VAULT-5671/creds branch May 20, 2022 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

creds endpoint #4

creds endpoint #4

tvoran commented May 12, 2022 •

edited

Loading

tomhjp left a comment

tomhjp May 19, 2022

tvoran May 19, 2022

tomhjp May 20, 2022 •

edited

Loading

tomhjp May 19, 2022

tvoran May 19, 2022

calvn left a comment

tomhjp May 20, 2022

tvoran May 20, 2022

tomhjp May 20, 2022

calvn May 20, 2022

tvoran May 20, 2022

tomhjp left a comment

creds endpoint #4

creds endpoint #4

Conversation

tvoran commented May 12, 2022 • edited Loading

Overview

Design of Change

Testing examples

tomhjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomhjp May 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvn left a 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

tomhjp left a comment

Choose a reason for hiding this comment

tvoran commented May 12, 2022 •

edited

Loading

tomhjp May 20, 2022 •

edited

Loading