add minimal types for service account TokenRequest API

[mikedanese]

Adds parts of the types in kubernetes/community#1460. ValidityDuration is omitted because we are still discussing how to surface non-expiring tokens but it should be easy to add in a backwards compatibly.

#58790

@kubernetes/sig-auth-api-reviews

[k8s-ci-robot]

@mikedanese: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[smarterclayton]

Describe what "audience" means, including a quick summary and then reference the real spec (JWT or whatever).

[smarterclayton]

Subsequent PR would change TokenReview to accept audience?

[liggitt]

I expected these types to start in a new version of authentication.k8s.io... v2alpha1, etc

[mikedanese]

@smarterclayton I planned to modify TokenRequest in a followup PR.

[mikedanese]

I'm not sure how to cleanly represent in a TokenRequest that a token should never expire since I would like to default this field. Can we either:

• use an annotation?
• set a validity duration of 1000 years?

[smarterclayton]

Hrm, definitely have never had this before. We could use the pointer for defaulting, but I don't think a negative duration is appropriate, and MaxInt duration is always ugly. If we set a duration of zero as never expire and leave it unset, would that cover it?

[mikedanese]

MaxInt is the openssl x509 approach to non-expiring. I dislike the use of sentinel values, but I think it's better then a NeverExpires bool.

[liggitt]

collapsing the unexpiring case to "really really long expiration" does make the API simpler, if a little weird, and still maintains the possibility for a requester to ask for a 1000 year token and be given a 1 month one (reflected in status).

it removes the watershed division between the two types of tokens, which would make it hard to use a different signing key for eternal tokens, which @mikedanese had mentioned in passing

[smarterclayton]

So the docs will say "if you don't want it to expire, set a really really high value, like a billion"?

I suspect people will just pick arbitrary values and set them (no one actually goes and looks up maxint). I guess that's fine, although it's not really an intent. I don't want to do IntOrString though.

In the future, if we want to tie the lifetime of the token to a pod, we wouldn't specify that via this API (it would be on the volume source), but something similar may apply. We could say:

type TokenExpirationType string
// Token never expires
const Forever TokenExpirationType = "Forever"
// User requests a time limit
const TimeBound TokenExpirationType = "TimeBound"
// Server determines how long this token should last
const ServerPreferred TokenExpirationType = "ServerPreferred"

type TokenRequestSpec struct {
  Expiration TokenExpirationType
  ExpirationSeconds *int64
}

The status would then have Expiration as a time. We could let presence of ExpirationSeconds default Expiration to TimeBound (although in other places that has caused problems, not sure here it does).

Side note: I wouldn't expect us to use duration here, we should be using seconds.

[mikedanese]

Using service account ID tokens as API access tokens is an abuse. Conflating these two ties our hands with respect to key rotation. If we want to support API access tokens, we should support it as a separate construct. They can still be tied to a service account but we should not tie them to a key that we want to rotate.

My goal here is to support the previous flow, while weening clients onto short lived ID tokens allowing operators to tighten max validity duration of both ID tokens and signing keys. We shouldn't advertise "if you don't want it to expire, set a really really high value, like a billion" in docs. Kubelet should just do it until old clients that depend on this behavior are no longer supported.

If we include "Never" as a valid expiration type clusters will either:

• Never rotate their ID token signing keys
• Not support "Never" tokens, and the API is not portable

[smarterclayton]

Ok. Then ExpirationSeconds *int64 with an arbitrary high value (our service account tokens today are 1 year).

[smarterclayton]

To jordans comment, I do think this needs to either be in v1alpha1, or v2alpha1 (technically it could be v1alpha1 since this won't cause us to rev v1 authentication)

[liggitt]

this is tricky, since we're wanting a subresource in api/v1/serviceaccounts/token ... whatever surfaces there sticks forever (hence replicationcontrollers/scale being stuck at extensions/v1beta1 Scale forever)

[mikedanese]

What @liggitt said. I think the best approach is to use authentication.k8s.io/v1 and be pretty confident (e.g. by keeping the API minimal) when we move this feature from alpha to beta. We can then begin to move serviceaccount out of the main API group into authentication.k8s.io/v2alpha1 which will give us a chance to correct any problems we encounter.

[smarterclayton]

The old "shrug, we can't solve a hard problem so we'll do the thing we tell people not to do" approach?

I agree the subresource is hard. Maybe we should just call the subresource alphatokenrequest to force it to be clear, and only promote once it's beta (to tokenrequest).

[mikedanese]

Even if we make an /alphatoken we can't support two versions without creating a new version of this API group and leaving v1 sparse. The /token subresource will be alpha feature guarded so having an /alphatoken path during alpha doesn't add much value unless will eventually /token accept a separate type and both are served simultaneously.

[smarterclayton]

Since it's behind a gate we can punt on this.

[smarterclayton]

Just the one comment left then

[mikedanese]

@smarterclayton fixed.

[smarterclayton]

Feels like a last minute nit, but why not have this just be an owner ref? Node access control?

[mikedanese]

It's an interesting idea that hadn't occurred to me. Some problems i see with owners ref are we would like to enforce a single owner. Potentially thats the purpose of the Controller field.

As documented:

OwnerReference contains enough information to let you identify an owning
object. Currently, an owning object must be in the same namespace, so there
is no namespace field.

This seems vague enough that it might fit.

[mikedanese]

Talking to @liggitt, this doesn't feel like a great fit for a few reasons:

• Owners references are intended to aid object garbage collection. The semantics are "if all objects in this list are deleted, then this object should be deleted". The semantics we would want for this API are "If any object in this list is deleted, the token should be invalid".
• We want to limit the number of bound objects, and right now we think we only want to support one. We would need to add specific validation to a generic field to support this.

I would prefer we don't overload owners references here.

[smarterclayton]

ExpirationSeconds, update the godoc to describe it, and change the status to also be ExpirationSeconds. Also, the godoc on status is also wrong.

[mikedanese]

One is a duration and one is a time. It's a bit confusing to have both with the same name.

[smarterclayton]

If it's a time, then it should be a timestamp, and ExpirationTimestamp metav1.Time (akin to DeletionTimestamp). If it's the seconds remaining before expiration, then it should be ExpirationSeconds (like DeletionGracePeriodSeconds, I guess, although that is not calculated). I'm a bit confused why we would make this "seconds remaining before it expires" (which would require it to be virtual).

If it's really a time, I lean towards ExpirationTimestamp in status, in which case this should be ExpirationSeconds in spec (the number of seconds I want this token to last before expiration).

[mikedanese]

I would expect a user to request a duration of time for the token to be valid, (e.g. thirty minutes) and that the status return the time that the token will expire (e.g. unix epoch time 1517854484).

[mikedanese]

@smarterclayton @liggitt fixed.

[smarterclayton]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, smarterclayton

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

Needs approval from an approver in each of these OWNERS Files:

• pkg/apis/OWNERS [smarterclayton]
• staging/src/k8s.io/api/OWNERS [smarterclayton]
• test/OWNERS [mikedanese,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

[smarterclayton]

We probably (before 1.10) need to clearly identity the api guarantees as part of the godoc for the mixed version types

Addressed.

[k8s-github-robot]

Automatic merge from submit-queue (batch tested with PRs 55986, 59375, 59334, 59348, 58027). If you want to cherry-pick this change to another branch, please follow the instructions here.