Merge pull request #1686 from marcusmartins/nbf-leeway

Add leeway to JWT nbf checking

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)