Add limit to the TokenRequest expiration time by WanLinghao · Pull Request #63653 · kubernetes/kubernetes

WanLinghao · 2018-05-10T09:59:50Z

What this PR does / why we need it:
A new API TokenRequest has been implemented.It improves current serviceaccount model from many ways.
This patch adds limit to TokenRequest expiration time.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #63575

Special notes for your reviewer:

Release note:

NONE

WanLinghao · 2018-05-11T09:35:54Z

/test pull-kubernetes-integration

WanLinghao · 2018-05-11T10:12:27Z

@mikedanese PTAL

mikedanese · 2018-05-11T18:22:28Z

pkg/kubeapiserver/options/authentication.go

I'm not sure if this is useful to configure. I would suggest leaving the command line option out until people ask for it.

WanLinghao · 2018-05-12T06:46:07Z

@mikedanese WDYT this time

WanLinghao · 2018-05-16T03:27:01Z

@mikedanese friendly ping

mikedanese · 2018-05-16T17:32:43Z

cmd/kube-apiserver/app/options/options_test.go

This doesn't need to be plumbed through if the value is static.

mikedanese · 2018-05-16T17:33:08Z

pkg/registry/core/serviceaccount/storage/token.go

static validation should occur in pkg/apis/authentication/validation

mikedanese · 2018-05-16T17:49:03Z

pkg/kubeapiserver/options/authentication.go

can you call this service-account-token-max-expiration and give it a type of pflag.Duration:

https://godoc.org/github.com/spf13/pflag#FlagSet.Duration

mikedanese · 2018-05-16T17:50:14Z

I'd suggest only implementing maximum in this PR. How can we also enforce this maximum on the service account token volume projection?

liggitt · 2018-05-16T21:02:12Z

if you request over the max, do you get rejected or truncated to the max? if you get rejected, that means any pod spec that specifies a max is non-portable.

WanLinghao · 2018-05-17T08:27:58Z

From my point of view, we can simply do like other projected-volume did.
For example ,when you create a pod with following yaml file, at the same time, the secret "user" and "pass" are non-existent. The pod will stuck at ContainerCreating stage. The warning output would be like

Events:
  Type     Reason       Age               From                Message
  ----     ------       ----              ----                -------
  Normal   Scheduled    1m                default-scheduler   Successfully assigned office/test-projected-volume to 127.0.0.1
  Warning  FailedMount  44s (x8 over 1m)  kubelet, 127.0.0.1  MountVolume.SetUp failed for volume "all-in-one" : [secrets "user" not found, secrets "pass" not found]

apiVersion: v1
kind: Pod 
metadata:
  name: test-projected-volume
spec:
  containers:
  - name: alpine
    image: radial/busyboxplus:curl
    command: [ "/bin/ash", "-c", "--" ]
    args: [ "while true; do sleep 30; done;" ]
    volumeMounts:
    - name: all-in-one
      mountPath: "/projected-volume"
  volumes:
  - name: all-in-one
    projected:
      sources:
      - secret:
          name: user
      - secret:
          name: pass

In other words, we don't need special process here, the api-server would refuse the TokenRequest and the pod would be stuck at ContainerCreating stage with "expiration time out of range" warning.

I'd suggest only implementing maximum in this PR. How can we also enforce this maximum on the service account token volume projection?

WanLinghao · 2018-05-17T09:00:54Z

@liggitt I think we can add support of percent expiration-time.
For example, a cluster starts with max-expiration time of 10 days could accept "50%" max expiration-time, which equals 5 days.
WDYT

if you request over the max, do you get rejected or truncated to the max? if you get rejected, that means any pod spec that specifies a max is non-portable.

liggitt · 2018-05-17T13:30:51Z

@liggitt I think we can add support of percent expiration-time.
For example, a cluster starts with max-expiration time of 10 days could accept "50%" max expiration-time, which equals 5 days.
WDYT

what are the reasons someone would request a specific validity period? requesting a percentage of an unknown period doesn't make sense to me.

mikedanese · 2018-05-17T20:41:50Z

I suggest we do none configurable hard validation as in #63999 and do a configurable soft validation. i.e. we should add a flag --service-account-token-max-expiration that configures the registry to truncate larger requested durations to an upper bound. I think this will be a useful flag when people start rotating these keys on a schedule. @WanLinghao @liggitt does that seem reasonable?

WanLinghao · 2018-05-18T05:20:10Z

@mikedanese I think for lower bound, none configurable hard validation is reasonable.
But for upper bound, configurable soft validation alone is enough. Since if we set the hard validation too low, it would restrict users. If we set it too high, it makes no difference from permanent one.

Maybe we can do like this, user can't request a token with expiration time lower than 10minutes, its hard validation and non-configurable. For upper bound, if it is configured, any larger request would be truncated into max value. If it is not configured, there's no upper bound on expiration time.

WanLinghao · 2018-06-11T10:15:16Z

@liggitt @mikedanese test added, PTAL

liggitt · 2018-06-11T14:29:45Z

cmd/kube-apiserver/app/server.go

if non-zero, should verify this is:

>= the default value (1 hour)

<= the max value (2^32 seconds)

lgtm otherwise

WanLinghao · 2018-06-12T06:18:00Z

/test pull-kubernetes-e2e-gce

WanLinghao · 2018-06-12T14:31:51Z

@liggitt done PTAL

liggitt · 2018-06-12T14:56:25Z

cmd/kube-apiserver/app/server.go

liggitt · 2018-06-12T14:58:15Z

cmd/kube-apiserver/app/server.go

lowBound := time.Hour would be simpler

liggitt · 2018-06-12T14:58:38Z

cmd/kube-apiserver/app/server.go

upBound := time.Duration(1<<32) * time.Second would be simpler

WanLinghao · 2018-06-13T03:54:08Z

@liggitt Done PTAL

liggitt · 2018-06-13T14:51:51Z

pkg/registry/core/serviceaccount/storage/token.go

name this maxExpirationSeconds so it's clear what the unit is

liggitt · 2018-06-13T14:52:17Z

cmd/kube-apiserver/app/server.go

..., must be between ...

liggitt · 2018-06-13T14:53:10Z

/approve
a couple nits, then go ahead and squash
@mikedanese has lgtm

…ins a TokenRequest's expiration time to avoid extreme value which could harm the cluster.

WanLinghao · 2018-06-14T03:46:01Z

@mikedanese PTAL

WanLinghao · 2018-06-19T09:19:32Z

@mikedanese friendly ping

mikedanese · 2018-06-26T23:06:15Z

/lgtm

k8s-ci-robot · 2018-06-26T23:06:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mikedanese, WanLinghao

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:

~~cmd/kube-apiserver/OWNERS~~ [liggitt,mikedanese]
~~pkg/kubeapiserver/OWNERS~~ [liggitt]
~~pkg/master/OWNERS~~ [mikedanese]
~~pkg/registry/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt,mikedanese]

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

WanLinghao · 2018-06-27T01:01:18Z

/retest

WanLinghao · 2018-06-27T02:15:19Z

/retest

WanLinghao · 2018-06-27T03:50:42Z

/retest

k8s-github-robot · 2018-06-27T06:06:37Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-27T07:31:08Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from deads2k, hongchaodeng, janetkuo and liggitt May 10, 2018 09:59

mikedanese self-assigned this May 10, 2018

WanLinghao force-pushed the token_expiry_limit branch from f69720e to ff1e95b Compare May 11, 2018 09:05

WanLinghao changed the title ~~[WIP]Add limit to the TokenRequest expiration time~~ Add limit to the TokenRequest expiration time May 11, 2018

k8s-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 May 11, 2018

mikedanese reviewed May 11, 2018

View reviewed changes

WanLinghao force-pushed the token_expiry_limit branch from ff1e95b to 57cfbaa Compare May 12, 2018 05:55

mikedanese reviewed May 16, 2018

View reviewed changes

mikedanese mentioned this pull request May 17, 2018

svcacct: validate min and max expiration seconds on TokenRequest #63999

Merged

WanLinghao force-pushed the token_expiry_limit branch from 57cfbaa to 94b4a87 Compare May 18, 2018 06:32

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 11, 2018

WanLinghao force-pushed the token_expiry_limit branch from c7f9618 to eb05913 Compare June 11, 2018 09:25

liggitt reviewed Jun 11, 2018

View reviewed changes

WanLinghao force-pushed the token_expiry_limit branch 2 times, most recently from 12c943e to 30b99bd Compare June 12, 2018 04:05

liggitt reviewed Jun 12, 2018

View reviewed changes

cmd/kube-apiserver/app/server.go Outdated

Copy link
Copy Markdown

Member

liggitt Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: hour

liggitt reviewed Jun 12, 2018

View reviewed changes

cmd/kube-apiserver/app/server.go Outdated

Copy link
Copy Markdown

Member

liggitt Jun 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowBound := time.Hour would be simpler

liggitt reviewed Jun 12, 2018

View reviewed changes

liggitt reviewed Jun 13, 2018

View reviewed changes

cmd/kube-apiserver/app/server.go Outdated

Copy link
Copy Markdown

Member

liggitt Jun 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..., must be between ...

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2018

This patch adds limit to the TokenRequest expiration time. It constra…

f16470c

…ins a TokenRequest's expiration time to avoid extreme value which could harm the cluster.

WanLinghao force-pushed the token_expiry_limit branch from 7b8b02c to f16470c Compare June 14, 2018 01:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2018

k8s-github-robot merged commit 2da4932 into kubernetes:master Jun 27, 2018

WanLinghao mentioned this pull request Jul 13, 2018

Add descripitons of --service-account-max-token-expiration option kubernetes/website#9500

Merged

Conversation

WanLinghao commented May 10, 2018

Uh oh!

WanLinghao commented May 11, 2018

Uh oh!

WanLinghao commented May 11, 2018

Uh oh!

mikedanese May 11, 2018 • 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!

WanLinghao commented May 12, 2018

Uh oh!

WanLinghao commented May 16, 2018

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!

mikedanese commented May 16, 2018

Uh oh!

liggitt commented May 16, 2018

Uh oh!

WanLinghao commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WanLinghao commented May 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liggitt commented May 17, 2018

Uh oh!

mikedanese commented May 17, 2018

Uh oh!

WanLinghao commented May 18, 2018

Uh oh!

WanLinghao commented Jun 11, 2018

Uh oh!

liggitt Jun 11, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

WanLinghao commented Jun 12, 2018

Uh oh!

WanLinghao commented Jun 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Jun 12, 2018 • 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!

WanLinghao commented Jun 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jun 13, 2018

Uh oh!

WanLinghao commented Jun 14, 2018

Uh oh!

WanLinghao commented Jun 19, 2018

Uh oh!

mikedanese commented Jun 26, 2018

Uh oh!

k8s-ci-robot commented Jun 26, 2018

Uh oh!

WanLinghao commented Jun 27, 2018

Uh oh!

WanLinghao commented Jun 27, 2018

Uh oh!

WanLinghao commented Jun 27, 2018

Uh oh!

k8s-github-robot commented Jun 27, 2018

mikedanese May 11, 2018 •

edited

Loading

WanLinghao commented May 17, 2018 •

edited

Loading

WanLinghao commented May 17, 2018 •

edited

Loading

liggitt Jun 11, 2018 •

edited

Loading

liggitt Jun 12, 2018 •

edited

Loading