From db1bf930981383ab99126a7d1abc8856a15189a0 Mon Sep 17 00:00:00 2001 From: Marcus Martins Date: Wed, 27 Apr 2016 20:54:36 -0700 Subject: [PATCH] Add leeway to JWT nbf and exp checking Adds a constant leeway (60 seconds) to the nbf and exp claim check to account for clock skew between the registry servers and the authentication server that generated the JWT. The leeway of 60 seconds is a bit arbitrary but based on the RFC recommendation and hub.docker.com logs/metrics where we don't see drifts of more than a second on our servers running ntpd. I didn't attempt to make the leeway configurable as it would add extra complexity to the PR and I am not sure how Distribution prefer to handle runtime flags like that. Also, I am simplifying the exp and nbf check for readability as the previous `NOT (A AND B)` with cmp operators was not very friendly. Ref: https://tools.ietf.org/html/rfc7519#section-4.1.5 Signed-off-by: Marcus Martins --- registry/auth/token/token.go | 17 ++++-- registry/auth/token/token_test.go | 86 ++++++++++++++++++++++++++++--- 2 files changed, 92 insertions(+), 11 deletions(-) diff --git a/registry/auth/token/token.go b/registry/auth/token/token.go index 2598f362a..bd6b8c6fd 100644 --- a/registry/auth/token/token.go +++ b/registry/auth/token/token.go @@ -20,6 +20,9 @@ const ( // TokenSeparator is the value which separates the header, claims, and // signature in the compact serialization of a JSON Web Token. TokenSeparator = "." + // Leeway is the Duration that will be added to NBF and EXP claim + // checks to account for clock skew as per https://tools.ietf.org/html/rfc7519#section-4.1.5 + Leeway = 60 * time.Second ) // Errors used by token parsing and verification. @@ -143,9 +146,17 @@ func (t *Token) Verify(verifyOpts VerifyOptions) error { } // Verify that the token is currently usable and not expired. - currentUnixTime := time.Now().Unix() - if !(t.Claims.NotBefore <= currentUnixTime && currentUnixTime <= t.Claims.Expiration) { - log.Errorf("token not to be used before %d or after %d - currently %d", t.Claims.NotBefore, t.Claims.Expiration, currentUnixTime) + currentTime := time.Now() + + ExpWithLeeway := time.Unix(t.Claims.Expiration, 0).Add(Leeway) + if currentTime.After(ExpWithLeeway) { + log.Errorf("token not to be used after %s - currently %s", ExpWithLeeway, currentTime) + return ErrInvalidToken + } + + NotBeforeWithLeeway := time.Unix(t.Claims.NotBefore, 0).Add(-Leeway) + if currentTime.Before(NotBeforeWithLeeway) { + log.Errorf("token not to be used before %s - currently %s", NotBeforeWithLeeway, currentTime) return ErrInvalidToken } diff --git a/registry/auth/token/token_test.go b/registry/auth/token/token_test.go index 827dbbd75..af862df7f 100644 --- a/registry/auth/token/token_test.go +++ b/registry/auth/token/token_test.go @@ -91,7 +91,7 @@ func makeTrustedKeyMap(rootKeys []libtrust.PrivateKey) map[string]libtrust.Publi return trustedKeys } -func makeTestToken(issuer, audience string, access []*ResourceActions, rootKey libtrust.PrivateKey, depth int) (*Token, error) { +func makeTestToken(issuer, audience string, access []*ResourceActions, rootKey libtrust.PrivateKey, depth int, now time.Time, exp time.Time) (*Token, error) { signingKey, err := makeSigningKeyWithChain(rootKey, depth) if err != nil { return nil, fmt.Errorf("unable to make signing key with chain: %s", err) @@ -109,8 +109,6 @@ func makeTestToken(issuer, audience string, access []*ResourceActions, rootKey l RawJWK: &rawJWK, } - now := time.Now() - randomBytes := make([]byte, 15) if _, err = rand.Read(randomBytes); err != nil { return nil, fmt.Errorf("unable to read random bytes for jwt id: %s", err) @@ -120,7 +118,7 @@ func makeTestToken(issuer, audience string, access []*ResourceActions, rootKey l Issuer: issuer, Subject: "foo", Audience: audience, - Expiration: now.Add(5 * time.Minute).Unix(), + Expiration: exp.Unix(), NotBefore: now.Unix(), IssuedAt: now.Unix(), JWTID: base64.URLEncoding.EncodeToString(randomBytes), @@ -188,7 +186,7 @@ func TestTokenVerify(t *testing.T) { tokens := make([]*Token, 0, numTokens) for i := 0; i < numTokens; i++ { - token, err := makeTestToken(issuer, audience, access, rootKeys[i], i) + token, err := makeTestToken(issuer, audience, access, rootKeys[i], i, time.Now(), time.Now().Add(5*time.Minute)) if err != nil { t.Fatal(err) } @@ -209,6 +207,78 @@ func TestTokenVerify(t *testing.T) { } } +// This tests that we don't fail tokens with nbf within +// the defined leeway in seconds +func TestLeeway(t *testing.T) { + var ( + issuer = "test-issuer" + audience = "test-audience" + access = []*ResourceActions{ + { + Type: "repository", + Name: "foo/bar", + Actions: []string{"pull", "push"}, + }, + } + ) + + rootKeys, err := makeRootKeys(1) + if err != nil { + t.Fatal(err) + } + + trustedKeys := makeTrustedKeyMap(rootKeys) + + verifyOps := VerifyOptions{ + TrustedIssuers: []string{issuer}, + AcceptedAudiences: []string{audience}, + Roots: nil, + TrustedKeys: trustedKeys, + } + + // nbf verification should pass within leeway + futureNow := time.Now().Add(time.Duration(5) * time.Second) + token, err := makeTestToken(issuer, audience, access, rootKeys[0], 0, futureNow, futureNow.Add(5*time.Minute)) + if err != nil { + t.Fatal(err) + } + + if err := token.Verify(verifyOps); err != nil { + t.Fatal(err) + } + + // nbf verification should fail with a skew larger than leeway + futureNow = time.Now().Add(time.Duration(61) * time.Second) + token, err = makeTestToken(issuer, audience, access, rootKeys[0], 0, futureNow, futureNow.Add(5*time.Minute)) + if err != nil { + t.Fatal(err) + } + + if err = token.Verify(verifyOps); err == nil { + t.Fatal("Verification should fail for token with nbf in the future outside leeway") + } + + // exp verification should pass within leeway + token, err = makeTestToken(issuer, audience, access, rootKeys[0], 0, time.Now(), time.Now().Add(-59*time.Second)) + if err != nil { + t.Fatal(err) + } + + if err = token.Verify(verifyOps); err != nil { + t.Fatal(err) + } + + // exp verification should fail with a skew larger than leeway + token, err = makeTestToken(issuer, audience, access, rootKeys[0], 0, time.Now(), time.Now().Add(-60*time.Second)) + if err != nil { + t.Fatal(err) + } + + if err = token.Verify(verifyOps); err == nil { + t.Fatal("Verification should fail for token with exp in the future outside leeway") + } +} + func writeTempRootCerts(rootKeys []libtrust.PrivateKey) (filename string, err error) { rootCerts, err := makeRootCerts(rootKeys) if err != nil { @@ -307,7 +377,7 @@ func TestAccessController(t *testing.T) { Name: testAccess.Name, Actions: []string{testAccess.Action}, }}, - rootKeys[1], 1, // Everything is valid except the key which signed it. + rootKeys[1], 1, time.Now(), time.Now().Add(5*time.Minute), // Everything is valid except the key which signed it. ) if err != nil { t.Fatal(err) @@ -333,7 +403,7 @@ func TestAccessController(t *testing.T) { token, err = makeTestToken( issuer, service, []*ResourceActions{}, // No access specified. - rootKeys[0], 1, + rootKeys[0], 1, time.Now(), time.Now().Add(5*time.Minute), ) if err != nil { t.Fatal(err) @@ -363,7 +433,7 @@ func TestAccessController(t *testing.T) { Name: testAccess.Name, Actions: []string{testAccess.Action}, }}, - rootKeys[0], 1, + rootKeys[0], 1, time.Now(), time.Now().Add(5*time.Minute), ) if err != nil { t.Fatal(err)