serviceaccounts: Add JWT KeyIDs to tokens #78502

ahmedtd · 2019-05-29T21:50:07Z

This commit fills out the JWT "kid" (KeyID) field on most serviceaccount tokens we create. The KeyID value we use is derived from the public key of keypair that backs the cluster's OIDC issuer.

OIDC verifiers use the KeyID to smoothly cope with key rotations:

During a rotation, the verifier will have multiple keys cached from the issuer, any of which could have signed the token being verified. KeyIDs let the verifier pick the appropriate key without having to try each one.
Seeing a new KeyID is a trigger for the verifier to invalidate its cached keys and fetch the new set of valid keys from the identity provider.

The value we use for the KeyID is derived from the identity provider's public key by serializing it in DER format, taking the SHA256 hash, and then urlsafe base64-encoding it. This gives a value that is strongly bound to the key, but can't be reversed to obtain the public key, which keeps people from being tempted to derive the key from the key ID and using that for verification.

Tokens based on jose OpaqueSigners are omitted for now --- I don't see any way to actually run the API server that results in an OpaqueSigner being used.

/kind feature
/sig auth

Does this PR introduce a user-facing change?:

Service account tokens now include the JWT Key ID field in their header.

k8s-ci-robot · 2019-05-29T21:50:09Z

Welcome @ahmedtd!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-05-29T21:50:15Z

Hi @ahmedtd. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

ahmedtd · 2019-05-29T21:56:25Z

/assign @liggitt

enj · 2019-05-29T23:48:22Z

IMO a change like this requires more thought and possibly a KEP.

Also is there any real performance difference with trying all combinations when we have the token cache?

ahmedtd · 2019-05-30T20:56:30Z

I'm happy to put together a KEP if you think it's necessary.

To add some context, this is part of the work we're (mikedanese, awly, alexcope), doing within Google to let Google IAM trust tokens issued by the cluster's OIDC issuer (GKE Workload Identity [1]).

The performance difference here is not for any component within the Kubernetes cluster, but for the OIDC relying party that is verifying the tokens (Google IAM in our case, but any other OIDC relying party should see the same benefit).

To verify a token, the relying party needs to fetch the identity provider's JWK public keys from it's well-known endpoint. There could be several keys advertised here (though there aren't in Kubernetes' case). To verify the token, each of the advertised keys needs to be tried, until the one that signed the token originally is found. Key IDs let the relying party skip these multiple expensive operations and jump directly to the correct key.

Additionally, the relying party caches the Identity Provider's public keys for a period of time, to avoid slamming the identity provider if with traffic proportional to the traffic the relying party is experiencing. Seeing a new Key ID triggers the relying party to dump its cache and re-fetch keys. (This behavior might be specific to Google IAM, but it seems like most relying parties would need to make a similar choice). Without this trigger, the cache TTL needs to be low enough to not cause a significant outage if the identity provider rotates its keys.

[1] https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity

enj · 2019-05-31T12:47:35Z

I'm happy to put together a KEP if you think it's necessary.

Let us start with a discussion on the sig-auth biweekly meeting. This seems highly related to kubernetes/enhancements#704 and kubernetes/community#2314

To add some context, this is part of the work we're (mikedanese, awly, alexcope), doing within Google to let Google IAM trust tokens issued by the cluster's OIDC issuer (GKE Workload Identity [1]).

The performance difference here is not for any component within the Kubernetes cluster, but for the OIDC relying party that is verifying the tokens (Google IAM in our case, but any other OIDC relying party should see the same benefit).

To verify a token, the relying party needs to fetch the identity provider's JWK public keys from it's well-known endpoint. There could be several keys advertised here (though there aren't in Kubernetes' case). To verify the token, each of the advertised keys needs to be tried, until the one that signed the token originally is found. Key IDs let the relying party skip these multiple expensive operations and jump directly to the correct key.

Seems like you could simply re-use the token cache logic from kube to avoid this cost. I think you would want a short lived cache anyway.

Additionally, the relying party caches the Identity Provider's public keys for a period of time, to avoid slamming the identity provider if with traffic proportional to the traffic the relying party is experiencing. Seeing a new Key ID triggers the relying party to dump its cache and re-fetch keys. (This behavior might be specific to Google IAM, but it seems like most relying parties would need to make a similar choice). Without this trigger, the cache TTL needs to be low enough to not cause a significant outage if the identity provider rotates its keys.

You could just re-fetch the keys when none match the token right (with some smarts around how often you are willing to do that)?

[1] https://cloud.google.com/kubernetes-engine/docs/how-to/workload-identity

My general hesitation here is that we are expanding the API surface that we expose external components (and I am not sure we actually need to do that).

ahmedtd · 2019-05-31T20:36:27Z

Let us start with a discussion on the sig-auth biweekly meeting.

Sounds good!

kubernetes/community#2314

You're right, I think this behavior needs to be part of this proposal. (The JWKS url is required to specify key IDs if it advertises multiple keys).

re-use the token cache logic from kube to avoid this cost. I think you would want a short lived cache anyway.

Can you point me to the code you're talking about? I've done a little bit of digging, but I can't figure out what you're referring to.

You could just re-fetch the keys when none match the token right (with some smarts around how often you are willing to do that)?

Yes, the key ID cache-dumping behavior originally seemed to be unnecessary to me, but it is pointed to in the OpenID Connect spec. I'm not totally sure of the rationale.

lbragstad · 2019-06-06T20:59:06Z

pkg/serviceaccount/jwt.go

+	// KeyID helps OIDC verifiers cope with key rotations.  Making the
+	// derivation non-reversible makes it impossible for someone to
+	// accidentally obtain the real key from the key ID and use it for token
+	// validation.


Even though the KeyID is generated using a one-way hash of the public key, is it useful for bad actors to collect JWTs they can assert were signed with the same private key?

It seems like that's already possible, with some effort — the cluster advertises its public keys, and it's easy to test that a given JWT was signed by a given key.

Oh, interesting. Thanks for the additional information.

mikedanese · 2019-06-12T16:34:16Z

pkg/serviceaccount/jwt.go

+	// accidentally obtain the real key from the key ID and use it for token
+	// validation.
+
+	publicKeyDERBytes, err := x509.MarshalPKIXPublicKey(&keyPair.PublicKey)


extract this to a function and share it between RSA and ECDSA

mikedanese · 2019-06-12T16:34:38Z

pkg/serviceaccount/jwt.go

+	signer, err := jose.NewSigner(
+		jose.SigningKey{
+			Algorithm: alg,
+			Key:       opaqueSigner,


Opaque signers should also forward key ids:

KeyID: opaqueSigner.Public().KeyID

Key rotation is going to be tricky though.

Actually, I spoke too soon --- SigningKey doesn't have a KeyID field. Is it safe to change the Key field from opaqueSigner to a JSONWebKey that wraps opaqueSigner?

signer, err := jose.NewSigner( jose.SigningKey{ Algorithm: alg, Key: &jose.JSONWebKey{ Algorithm: alg, Key: opaqueSigner, KeyID: opaqueSigner.Public().KeyID, Use: "sig", }, }, nil )

mikedanese · 2019-06-12T16:35:32Z

pkg/serviceaccount/jwt.go

+	)
+
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to create signer")


please, not yet :) I'll learn what Wrapf does when it's in the stdlib (maybe go1.13).

Done. It looks like what's coming in 1.13 is not Wrapf, but instead a new format code for Errorf ("%w")

mikedanese · 2019-06-12T16:38:08Z

Also is there any real performance difference with trying all combinations when we have the token cache?

This is primarily useful for relying parties of the service account issuer that cannot use TokenReview. A kid change signals that the verifying client needs to refetch keys.

ahmedtd · 2019-06-21T23:32:23Z

@enj We discussed this at the last sig-auth meeting. The only concerns that were raised were

ensure that opaque signers passed through their underlying key IDs
mention the key IDs in the OIDC Discovery Proposal

mikedanese · 2019-06-29T02:44:14Z

Would it be easier to just at an extra header?

kubernetes/vendor/gopkg.in/square/go-jose.v2/signing.go

Lines 53 to 56 in 2837c32

    
           // Optional map of additional keys to be inserted into the protected header 
        
           // of a JWS object. Some specifications which make use of JWS like to insert 
        
           // additional values here. All values must be JSON-serializable. 
        
           ExtraHeaders map[HeaderKey]interface{}

ahmedtd · 2019-06-30T06:12:34Z

Would it be easier to just at an extra header?

I can't tell what you're referring to here. Can you give me some more context?

mikedanese · 2019-08-27T13:54:18Z

pkg/serviceaccount/jwt.go

+func signerFromOpaqueSigner(opaqueSigner jose.OpaqueSigner) (jose.Signer, error) {
+	alg := jose.SignatureAlgorithm(opaqueSigner.Public().Algorithm)
+
+	signer, err := jose.NewSigner(


return jose.NewSigner. You decorate the error already above.

I feel like it's important for debuggability to decorate the errors at the point they enter the kubernetes codebase from our dependencies.

That way, someone trying to debug this code doesn't have to try to guess which library call the error they're seeing came out of.

mikedanese · 2019-08-27T13:54:42Z

pkg/serviceaccount/jwt.go

+		KeyID:     keyID,
+		Use:       "sig",
+	}
+
 	signer, err := jose.NewSigner(


return jose.NewSigner

I feel like it's important for debuggability to decorate the errors at the point they enter the kubernetes codebase from our dependencies.

That way, someone trying to debug this code doesn't have to try to guess which library call the error they're seeing came out of.

mikedanese · 2019-08-27T13:58:03Z

pkg/serviceaccount/jwt_test.go

+		t.Fatalf("Error checking for key ID: couldn't parse token: %v", err)
+	}
+
+	if jws.Signatures[0].Header.KeyID == "" {


This check seems unnecessary given the one below.

I feel like it's important for debuggability to decorate the errors at the point they enter the kubernetes codebase from our dependencies.

That way, someone trying to debug this code doesn't have to try to guess which library call the error they're seeing came out of.

mikedanese · 2019-08-27T13:58:12Z

pkg/serviceaccount/jwt_test.go

+	}
+
+	if jws.Signatures[0].Header.KeyID != expectedKeyID {
+		t.Fatalf("Token %q has the wrong KeyID (got %q, want %q)", jwsString, jws.Signatures[0].Header.KeyID, expectedKeyID)


This commit fills out the JWT "kid" (KeyID) field on most serviceaccount tokens we create. The KeyID value we use is derived from the public key of keypair that backs the cluster's OIDC issuer. OIDC verifiers use the KeyID to smoothly cope with key rotations: * During a rotation, the verifier will have multiple keys cached from the issuer, any of which could have signed the token being verified. KeyIDs let the verifier pick the appropriate key without having to try each one. * Seeing a new KeyID is a trigger for the verifier to invalidate its cached keys and fetch the new set of valid keys from the identity provider. The value we use for the KeyID is derived from the identity provider's public key by serializing it in DER format, taking the SHA256 hash, and then urlsafe base64-encoding it. This gives a value that is strongly bound to the key, but can't be reversed to obtain the public key, which keeps people from being tempted to derive the key from the key ID and using that for verification. Tokens based on jose OpaqueSigners are omitted for now --- I don't see any way to actually run the API server that results in an OpaqueSigner being used.

mikedanese · 2019-08-29T05:31:00Z

/lgtm
/approve

k8s-ci-robot · 2019-08-29T05:31:43Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/serviceaccount/OWNERS~~ [mikedanese]

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

enj · 2019-08-29T18:51:44Z

pkg/serviceaccount/jwt.go

+// pick the correct key for verification when the identity party advertises
+// multiple keys.
+//
+// Making the derivation non-reversible makes it impossible for someone to


Technically someone could just build the hash from the public key. But that would be silly to guard against.

heh, ya. good point. it's just an arbitrary value and the choice doesn't have security implications unless you make a really bad decision. This hash is nice because it's not going to change if the key is not rotated and (most likely) going to change when the key is rotated but that's probably the extent of it.

enj · 2019-08-29T18:54:42Z

I only scanned the code, but including the key ID sounds fine to me. 👍

I do think we need to declare that all service account tokens are not opaque since using the key ID implies structure.

mikedanese · 2019-08-29T20:19:36Z

I do think we need to declare that all service account tokens are not opaque since using the key ID implies structure.

Feel free to review the KEP so we can start making changes :). kubernetes/enhancements#1205

I'd like to see GA of TokenRequest in 1.17

mikedanese · 2019-08-29T20:19:45Z

/retest

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 29, 2019

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 29, 2019

k8s-ci-robot requested review from deads2k and enj May 29, 2019 21:50

k8s-ci-robot assigned liggitt May 29, 2019

mikedanese self-assigned this Jun 1, 2019

lbragstad reviewed Jun 6, 2019

View reviewed changes

mikedanese reviewed Jun 12, 2019

View reviewed changes

ahmedtd force-pushed the jwt-keyid branch 3 times, most recently from f483f65 to aa26c1d Compare June 20, 2019 02:07

mikedanese requested a review from micahhausler June 21, 2019 02:28

mikedanese reviewed Aug 27, 2019

View reviewed changes

mikedanese added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Aug 27, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Aug 27, 2019

ahmedtd force-pushed the jwt-keyid branch from aa26c1d to b4e9958 Compare August 28, 2019 21:18

mikedanese added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 29, 2019

mikedanese added this to the v1.16 milestone Aug 29, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019

enj reviewed Aug 29, 2019

View reviewed changes

k8s-ci-robot merged commit f44d8f5 into kubernetes:master Aug 30, 2019

micahhausler mentioned this pull request Sep 11, 2019

Removed key id from jwks JSON hack script aws/amazon-eks-pod-identity-webhook#9

Merged

ahmedtd mentioned this pull request Jul 18, 2023

REQUEST: New membership for ahmedtd kubernetes/org#4336

Closed

9 tasks

serviceaccounts: Add JWT KeyIDs to tokens #78502

serviceaccounts: Add JWT KeyIDs to tokens #78502

Conversation

ahmedtd commented May 29, 2019 • edited by mikedanese Loading

k8s-ci-robot commented May 29, 2019

k8s-ci-robot commented May 29, 2019

ahmedtd commented May 29, 2019

enj commented May 29, 2019

ahmedtd commented May 30, 2019

enj commented May 31, 2019

ahmedtd commented May 31, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Jun 12, 2019

ahmedtd commented Jun 21, 2019 • edited Loading

mikedanese commented Jun 29, 2019

ahmedtd commented Jun 30, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Aug 29, 2019

k8s-ci-robot commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented Aug 29, 2019

mikedanese commented Aug 29, 2019

mikedanese commented Aug 29, 2019

ahmedtd commented May 29, 2019 •

edited by mikedanese

Loading

ahmedtd commented Jun 21, 2019 •

edited

Loading