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 <marcus@docker.com>

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()
+	currentTime := time.Now()
-	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)
+	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
 	}
 

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)