Enable Bound Service Account Tokens #150

marun · 2019-12-11T19:34:40Z

deads2k · 2019-12-12T17:10:31Z

enhancements/kube-apiserver/bound-sa-tokens.md

+
+### Goals
+
+- A pod can request a bound service account token via a projected volume.


numbered list please

deads2k · 2019-12-12T17:11:17Z

enhancements/kube-apiserver/bound-sa-tokens.md

+
+### Non-Goals
+
+- Integrate or ship the aws pod identity webhook


@derekwaynecarr @cuppett do we get to punt or not?

According to the epic this is a non-goal for this team and this release.

I was under the assumption that one of the exit criteria would be to make sure the iam setup works.. I don't think we have to ship it, but we have to verify it works with sane steps.

This is called out in the testing section.

enhancements/kube-apiserver/bound-sa-tokens.md

cuppett · 2019-12-12T17:39:47Z

enhancements/kube-apiserver/bound-sa-tokens.md

+    in kube > 1.12. They are only configured by the apiserver if the following options are
+    provided:
+    - service-account-signing-key-file (set to private key managed by operator)
+    - service-account-issuer (defaults to https://kubernetes.default.svc/)


Should we default to something more specific/unique per cluster (thinking a derivative of the cluster ID)

Note that the default specified here is the same as the upstream default. What would be the use case for providing something unique?

enhancements/kube-apiserver/bound-sa-tokens.md

sttts · 2020-01-08T13:17:16Z

enhancements/kube-apiserver/bound-sa-tokens.md

+
+## Summary
+
+Enable the optional use of bound service account tokens via volume projection and the


links to the APIs would be nice.

Done. Note that the best doc on both of these is arguably the jpweber.io link in see-also

enhancements/kube-apiserver/bound-sa-tokens.md

sttts · 2020-01-08T13:26:35Z

enhancements/kube-apiserver/bound-sa-tokens.md

+    bound tokens.
+  - The keypair should be written to a secret in the `openshift-kube-apiserver` namespace
+  - The public key should be added to (rather than replaced in) a configmap in the
+    `openshift-config-managed` namespace.


it surprises me that there is no standard location defined by upstream.

it surprises me that there is no standard location defined by upstream.

our operator configuration uses this standard location. Recall that the kube-apiserver doesn't read its config from in-cluster sources.

Meant the public key. I guess the kube-apiserver is the only one verifying that, so it is the only one that has to know its location.

sttts · 2020-01-08T13:28:16Z

enhancements/kube-apiserver/bound-sa-tokens.md

+      `KubeAPIServerConfig.ServiceAccountPublicKeyFiles` for provision to the apiserver.
+      - In the event that the keypair secret is deleted, a new keypair will be generated
+        but tokens signed by the previous private key will continue to validate since the
+        corresponding public key will still be used to validate bound tokens.


so the openshift-config-managed configmap has multiple public key possibly? How are they named?

Updated to reflect: sequentially, in the same manner as sa token public keys are maintained.

enhancements/kube-apiserver/bound-sa-tokens.md

sttts · 2020-01-08T13:34:17Z

enhancements/kube-apiserver/bound-sa-tokens.md

+
+It probably makes sense to manually verify integration with the AWS pod identity webhook,
+since this is the primary motivation for this enhancement. It may make sense to delegate
+this testing to those that will be deploying the webhook for customers.


Maybe deletion of the secret would make sense to test.

Updated to include testing of both secret and configmap removal.

deads2k · 2020-01-09T13:56:53Z

/approve

@sttts has lgtm

sttts · 2020-01-14T10:39:29Z

enhancements/kube-apiserver/bound-sa-tokens.md

+      - Operator should set this to the path of the private key it manages
    - service-account-issuer
-      - Operator should set this to https://kubernetes.default.svc/
+      - Operator should default this to `bound-sa-token.auth.openshift.io` and it should be


if this is changed, which consequences does it have (especially for existing tokens) ?

Existing tokens signed by the previous issuer will fail to validate. If long-lived tokens have been issued to pods, pod restart will be the only fix.

sttts · 2020-01-14T10:40:46Z

enhancements/kube-apiserver/bound-sa-tokens.md

 - Validating that a bound token can be requested via the TokenRequest API
 - Validating that a pod can request a bound token via volume projection

+The key management proposed by this enhancement will also need to tested:


nit: be tested

sttts · 2020-01-14T10:41:42Z

enhancements/kube-apiserver/bound-sa-tokens.md

+- Would it make sense to disable bound tokens if --service-account-issuer is not set? Or
+  should the feature be enabled by default with iss defaulted if not provided on initial
+  deployment, and it could be changed from the default subsequently if required?
+  - How does the kubelet respond to a change in issuer?


Is this still open?

Which of the two will be implemented now?

--service-account-issuer will be defaulted to allow the feature to come enabled out of the box.

marun · 2020-01-21T07:14:06Z

@sttts Updated, PTAL

sttts · 2020-01-21T16:16:09Z

enhancements/kube-apiserver/bound-sa-tokens.md

+      - Operator should set this to the path of the private key it manages
+    - service-account-issuer
+      - Operator should default this to `auth.openshift.io` and it should be possible to
+        override it. When it is overriden, tokens from the previous issuer will no longer


nit: overridden

sttts · 2020-01-21T16:17:40Z

enhancements/kube-apiserver/bound-sa-tokens.md

+  - Enabling bound tokens will satisfy goals #1 and #2.
+
+- The apiserver operator should manage a keypair used to sign and verify bound tokens
+  - An rsa keypair will be used. The trust model for digital signatures (via jwt) is


sttts · 2020-01-21T16:21:23Z

enhancements/kube-apiserver/bound-sa-tokens.md

+
+### Implementation Details/Notes/Constraints [optional]
+
+Bound tokens are not currently required until bootstrap is complete, so it should be


cc @bergerhoffer, this needs to go into docs as a warning

Thanks @sttts I'll make a note of this.

sttts · 2020-01-21T16:32:01Z

/lgtm

openshift-ci-robot · 2020-01-21T16:32:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, marun, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k,sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 11, 2019

marun changed the title ~~Enable Bound Service Account Tokens~~ WIP Enable Bound Service Account Tokens Dec 11, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 11, 2019

openshift-ci-robot requested review from abhinavdahiya and dustymabe December 11, 2019 19:35

deads2k reviewed Dec 12, 2019

View reviewed changes

enhancements/kube-apiserver/bound-sa-tokens.md Show resolved Hide resolved

deads2k reviewed Dec 12, 2019

View reviewed changes

enhancements/kube-apiserver/bound-sa-tokens.md Show resolved Hide resolved

deads2k reviewed Dec 12, 2019

View reviewed changes

enhancements/kube-apiserver/bound-sa-tokens.md Show resolved Hide resolved

cuppett reviewed Dec 12, 2019

View reviewed changes

marun changed the title ~~WIP Enable Bound Service Account Tokens~~ Enable Bound Service Account Tokens Jan 4, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2020

sttts reviewed Jan 8, 2020

View reviewed changes

enhancements/kube-apiserver/bound-sa-tokens.md Show resolved Hide resolved

sttts reviewed Jan 8, 2020

View reviewed changes

enhancements/kube-apiserver/bound-sa-tokens.md Show resolved Hide resolved

sttts reviewed Jan 8, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2020

sttts reviewed Jan 14, 2020

View reviewed changes

This was referenced Jan 16, 2020

Enable bound SA tokens openshift/cluster-kube-apiserver-operator#718

Merged

Enable overriding service account issuer for bound tokens openshift/api#569

Merged

WIP Enable configuration of bound service account tokens openshift/origin#24416

Closed

sttts reviewed Jan 21, 2020

View reviewed changes

Enable Bound Service Account Tokens

46b5c34

openshift-ci-robot assigned sttts Jan 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2020

openshift-merge-robot merged commit 4be1fa4 into openshift:master Jan 21, 2020

marun mentioned this pull request Jan 23, 2020

Enable changing bound service account token issuer from default openshift/cluster-kube-apiserver-operator#727

Merged

bergerhoffer mentioned this pull request Apr 13, 2020

OSDOCS-876: Adding docs for bound service account tokens openshift/openshift-docs#21118

Merged


		### Goals

		- A pod can request a bound service account token via a projected volume.


		### Non-Goals

		- Integrate or ship the aws pod identity webhook


		## Summary

		Enable the optional use of bound service account tokens via volume projection and the


		### Implementation Details/Notes/Constraints [optional]

		Bound tokens are not currently required until bootstrap is complete, so it should be

Enable Bound Service Account Tokens #150

Enable Bound Service Account Tokens #150

Uh oh!

Conversation

marun commented Dec 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Jan 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marun commented Jan 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Jan 21, 2020

Uh oh!

openshift-ci-robot commented Jan 21, 2020

Uh oh!

marun Jan 9, 2020 •

edited

Loading