implement token signing key rotation #348

mikehelmick · 2020-08-25T00:36:42Z

Fixes #94

Proposed Changes

Allow the token signing key to be rotated by the server operator. This is totally independent from the certificate signing keys

Release Note

Verification server operators can rotate their token signing key. `TOKEN_SIGNING_KEY` and `TOKEN_SIGNING_KEY_ID` are now array based env vars. They must be the same length. The first items in the lists represents the active key/kid and the remaining entries are allowed to validate.

sethvargo · 2020-08-25T00:40:43Z

pkg/config/api_server_config.go

 	VerificationTokenDuration time.Duration `env:"VERIFICATION_TOKEN_DURATION,default=24h"`
 	TokenSigningKey           string        `env:"TOKEN_SIGNING_KEY,required"`
 	TokenSigningKeyID         string        `env:"TOKEN_SIGNING_KEY_ID,default=v1"`
 	TokenIssuer               string        `env:"TOKEN_ISSUER,default=diagnosis-verification-example"`
+	// In order to rotate keys, one sets these values to the 'old' key ID allowing it to still verify


This backs us into a world where there can only ever be two valid keys. While I understand that's our current situation, given the frequency of change in this project, I wonder if it makes sense to have an array instead where the "first" element is the newest key and all other ones are "old"

then you have to have parallel arrays w/ kid and key ref.

The other option would be to bring this up one level where we take a cryptoKey and allow all non-destroyed versions for validation and the primary for signing.
But - I then we have to support that across all our KMS implementations

Needing more than 2 valid keys means that you're rotating twice in a 24 hour period which hopefully isn't ever necessary.

If we want this to be more flexible, I think we can push it into the database and manage it like the per-realm signing keys.

We do need to have a general 'admin' page in the UI still anyway, to allow for realm creation.

Up to you. I just know the moment we do this, we'll need to support a third...

will switch to arrays in the config

mikehelmick · 2020-08-25T17:59:04Z

/hold

mikehelmick · 2020-08-27T05:07:47Z

/unhold

rebased + moved to array based implementation

sethvargo · 2020-08-27T14:07:23Z

pkg/config/api_server_config.go

+// This represents the keys that are allowed to be used to verify tokens,
+// the TokenSigningKey/TokenSigningKeyID.
+func (c *APIServerConfig) AllowedTokenPublicKeys() map[string]string {
+	{


Curious why the curlies?

visual scoping of the read lock section

sethvargo · 2020-08-27T14:07:51Z

pkg/config/api_server_config.go

+func (c *APIServerConfig) AllowedTokenPublicKeys() map[string]string {
+	{
+		c.mu.RLock()
+		if len(c.allowedTokenPublicKeys) != 0 {


Nit: I feel like > 0 conveys better

sethvargo · 2020-08-27T14:08:51Z

pkg/config/api_server_config.go

+		return c.allowedTokenPublicKeys
+	}
+
+	c.allowedTokenPublicKeys = make(map[string]string)


Suggested change

c.allowedTokenPublicKeys = make(map[string]string)

c.allowedTokenPublicKeys = make(map[string]string, len(c.TokenSigning.TokenSigningKeyID))

sethvargo · 2020-08-27T14:09:37Z

pkg/config/api_server_config.go

@@ -79,6 +111,10 @@ func (c *APIServerConfig) Validate() error {
 		}
 	}

+	if err := c.TokenSigning.Validate(); err != nil {
+		return err


Suggested change

return err

return fmt.Errorf("failed to validate signing tokens: %w", err)

sethvargo · 2020-08-27T14:09:45Z

pkg/config/token_signing_config.go

-	TokenSigningKeyID string `env:"TOKEN_SIGNING_KEY_ID, default=v1"`
-	TokenIssuer       string `env:"TOKEN_ISSUER, default=diagnosis-verification-example"`
+	TokenSigningKey   []string `env:"TOKEN_SIGNING_KEY, required"`
+	TokenSigningKeyID []string `env:"TOKEN_SIGNING_KEY_ID, default=v1"`


Keys? and IDS?

sethvargo · 2020-08-27T14:10:55Z

pkg/config/token_signing_config.go

+}
+
+func (t *TokenSigningConfig) ActiveKey() string {
+	return t.TokenSigningKey[0]


This will panic if TokenSigningKey is empty

sethvargo · 2020-08-27T14:10:58Z

pkg/config/token_signing_config.go

+}
+
+func (t *TokenSigningConfig) ActiveKeyID() string {
+	return t.TokenSigningKeyID[0]


sethvargo · 2020-08-27T14:12:10Z

pkg/config/token_signing_config.go

+
+func (t *TokenSigningConfig) Validate() error {
+	if len(t.TokenSigningKey) != len(t.TokenSigningKeyID) {
+		return fmt.Errorf("TOKEN_SIGNING_KEY and TOKEN_SIGNING_KEY_ID must be lists of the same length")


keys, ids := len(t.TokenSigningKey), len(t.TokenSigningKeyID); keys != ids { return fmt.Errorf("TOKEN_SIGNING_KEY (%d) and TOKEN_SIGNING_KEY_ID (%d) must be the same length", keys, ids) }

sethvargo · 2020-08-27T14:13:01Z

pkg/controller/certapi/certapi.go

-func (c *Controller) validateToken(ctx context.Context, verToken string, publicKey crypto.PublicKey) (string, *database.Subject, error) {
+// A map of valid 'kid' values is supported.
+// If the token is valid the token id (`tid') and subject (`sub`) claims are returned.
+func (c *Controller) validateToken(ctx context.Context, verToken string, publicKeys map[string]crypto.PublicKey) (string, *database.Subject, error) {


This is probably fine for now, but this looks awfully like the publicKeyCache

it's populated from the public key cache - see certificate.go, line 63

sethvargo · 2020-08-27T14:13:23Z

pkg/controller/certapi/certificate.go

-			stats.Record(ctx, c.metrics.CertificateErrors.M(1))
-			c.h.RenderJSON(w, http.StatusInternalServerError, api.InternalError())
-			return
+		allowedPublicKeys := make(map[string]crypto.PublicKey)


Suggested change

allowedPublicKeys := make(map[string]crypto.PublicKey)

allowedPublicKeys := make(map[string]crypto.PublicKey, len(c.config.AllowedTokenPublicKeys()))

jeremyfaller

I'm fine w/ this if Seth is.

jeremyfaller · 2020-08-27T15:41:36Z

pkg/config/api_server_config.go

+func (c *APIServerConfig) AllowedTokenPublicKeys() map[string]string {
+	{
+		c.mu.RLock()
+		if len(c.allowedTokenPublicKeys) != 0 {


The RLock is not necessarily released. If, for example, c=nil, you panic, and don't release the lock. More robust is:

func() {
c.mu.RLock()
defer c.mu.RUnlock()
if .....
}()

google-oss-robot · 2020-08-27T15:53:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyfaller, mikehelmick

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:

~~OWNERS~~ [jeremyfaller,mikehelmick]

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

jeremyfaller · 2020-08-27T16:13:32Z

Shoot, I might have inadvertently caused an early merge. My mistake. I will clean up if Mike's too busy. (Sorry, for the noob mistake.)

google-oss-robot requested review from jeremyfaller and whaught August 25, 2020 00:36

google-cla bot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Aug 25, 2020

google-oss-robot added approved labels Aug 25, 2020

sethvargo reviewed Aug 25, 2020

View reviewed changes

google-oss-robot added the do-not-merge/hold label Aug 25, 2020

implement token signing key rotation

f754629

mikehelmick force-pushed the issue94 branch from 54cd000 to f754629 Compare August 27, 2020 05:06

google-oss-robot removed the do-not-merge/hold label Aug 27, 2020

sethvargo reviewed Aug 27, 2020

View reviewed changes

jeremyfaller approved these changes Aug 27, 2020

View reviewed changes

google-oss-robot assigned jeremyfaller Aug 27, 2020

google-oss-robot added the lgtm label Aug 27, 2020

google-oss-robot merged commit a3954b3 into google:main Aug 27, 2020

mikehelmick deleted the issue94 branch August 27, 2020 16:18

google locked and limited conversation to collaborators Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement token signing key rotation #348

implement token signing key rotation #348

mikehelmick commented Aug 25, 2020 •

edited

Loading

sethvargo Aug 25, 2020

mikehelmick Aug 25, 2020

sethvargo Aug 25, 2020

mikehelmick Aug 25, 2020

mikehelmick commented Aug 25, 2020

mikehelmick commented Aug 27, 2020

sethvargo Aug 27, 2020

mikehelmick Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

sethvargo Aug 27, 2020

mikehelmick Aug 27, 2020

sethvargo Aug 27, 2020

jeremyfaller left a comment

jeremyfaller Aug 27, 2020

google-oss-robot commented Aug 27, 2020

jeremyfaller commented Aug 27, 2020

	c.allowedTokenPublicKeys = make(map[string]string)
	c.allowedTokenPublicKeys = make(map[string]string, len(c.TokenSigning.TokenSigningKeyID))

	return err
	return fmt.Errorf("failed to validate signing tokens: %w", err)

	allowedPublicKeys := make(map[string]crypto.PublicKey)
	allowedPublicKeys := make(map[string]crypto.PublicKey, len(c.config.AllowedTokenPublicKeys()))

implement token signing key rotation #348

implement token signing key rotation #348

Conversation

mikehelmick commented Aug 25, 2020 • edited Loading

Proposed Changes

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

mikehelmick commented Aug 25, 2020

mikehelmick commented Aug 27, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyfaller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-oss-robot commented Aug 27, 2020

jeremyfaller commented Aug 27, 2020

mikehelmick commented Aug 25, 2020 •

edited

Loading