[Fixed] revocation check used current time instead of jwt issue time

Also empty revoked keys once account jwt has no revocations.

Signed-off-by: Matthias Hanel <mh@synadia.com>

go.mod

@@ -2,7 +2,7 @@ module github.com/nats-io/nats-server/v2
 
 require (
 	github.com/minio/highwayhash v1.0.0
-	github.com/nats-io/jwt/v2 v2.0.0-20200930010033-c4fd08d85545
+	github.com/nats-io/jwt/v2 v2.0.0-20201006231922-e00ffcea7738
 	github.com/nats-io/nats.go v1.10.1-0.20200606002146-fc6fed82929a
 	github.com/nats-io/nkeys v0.2.0
 	github.com/nats-io/nuid v1.0.1

go.sum

@@ -14,8 +14,8 @@ github.com/minio/highwayhash v1.0.0/go.mod h1:xQboMTeM9nY9v/LlAOxFctujiv5+Aq2hR5
 github.com/nats-io/jwt v0.3.2/go.mod h1:/euKqTS1ZD+zzjYrY7pseZrTtWQSjujC7xjPc8wL6eU=
 github.com/nats-io/jwt v0.3.3-0.20200519195258-f2bf5ce574c7 h1:RnGotxlghqR5D2KDAu4TyuLqyjuylOsJiAFhXvMvQIc=
 github.com/nats-io/jwt v0.3.3-0.20200519195258-f2bf5ce574c7/go.mod h1:n3cvmLfBfnpV4JJRN7lRYCyZnw48ksGsbThGXEk4w9M=
-github.com/nats-io/jwt/v2 v2.0.0-20200930010033-c4fd08d85545 h1:RhEai4U9Ax2izzNupSdputRLZcJU1fpP1CE2zgwuTRI=
-github.com/nats-io/jwt/v2 v2.0.0-20200930010033-c4fd08d85545/go.mod h1:vs+ZEjP+XKy8szkBmQwCB7RjYdIlMaPsFPs4VdS4bTQ=
+github.com/nats-io/jwt/v2 v2.0.0-20201006231922-e00ffcea7738 h1:MlwwastrhUZSIvSs4M70vT0fOWTCF6WxOu9S4/NtY9U=
+github.com/nats-io/jwt/v2 v2.0.0-20201006231922-e00ffcea7738/go.mod h1:vs+ZEjP+XKy8szkBmQwCB7RjYdIlMaPsFPs4VdS4bTQ=
 github.com/nats-io/nats-server/v2 v2.1.8-0.20200524125952-51ebd92a9093/go.mod h1:rQnBf2Rv4P9adtAs/Ti6LfFmVtFG6HLhl/H7cVshcJU=
 github.com/nats-io/nats-server/v2 v2.1.8-0.20200601203034-f8d6dd992b71/go.mod h1:Nan/1L5Sa1JRW+Thm4HNYcIDcVRFc5zK9OpSZeI2kk4=
 github.com/nats-io/nats.go v1.10.0/go.mod h1:AjGArbfyR50+afOUotNX2Xs5SYHf+CoOa5HH1eEl2HE=

server/accounts.go

@@ -2253,13 +2253,13 @@ func (a *Account) clearExpirationTimer() bool {
 }
 
 // checkUserRevoked will check if a user has been revoked.
-func (a *Account) checkUserRevoked(nkey string) bool {
+func (a *Account) checkUserRevoked(nkey string, issuedAt int64) bool {
 	a.mu.RLock()
 	defer a.mu.RUnlock()
 	if a.usersRevoked == nil {
 		return false
 	}
-	if t, ok := a.usersRevoked[nkey]; !ok || t > time.Now().Unix() {
+	if t, ok := a.usersRevoked[nkey]; !ok || t < issuedAt {
 		return false
 	}
 	return true
@@ -2595,6 +2595,8 @@ func (s *Server) updateAccountClaimsWithRefresh(a *Account, ac *jwt.AccountClaim
 		for pk, t := range ac.Revocations {
 			a.usersRevoked[pk] = t
 		}
+	} else {
+		a.usersRevoked = nil
 	}
 	a.defaultPerms = buildPermissionsFromJwt(&ac.DefaultPermissions)
 	a.incomplete = len(incompleteImports) != 0
@@ -2639,7 +2641,6 @@ func (s *Server) updateAccountClaimsWithRefresh(a *Account, ac *jwt.AccountClaim
 		}
 	}
 
-	now := time.Now().Unix()
 	for i, c := range clients {
 		a.mu.RLock()
 		exceeded := a.mconns != jwt.NoLimit && i >= int(a.mconns)
@@ -2650,17 +2651,15 @@ func (s *Server) updateAccountClaimsWithRefresh(a *Account, ac *jwt.AccountClaim
 		}
 		c.mu.Lock()
 		c.applyAccountLimits()
-		// Check for being revoked here. We use ac one to avoid
-		// the account lock.
-		var nkey string
-		if c.user != nil {
-			nkey = c.user.Nkey
-		}
+		theJWT := c.opts.JWT
 		c.mu.Unlock()
-
-		// Check if we have been revoked.
+		// Check for being revoked here. We use ac one to avoid the account lock.
 		if ac.Revocations != nil {
-			if t, ok := ac.Revocations[nkey]; ok && now >= t {
+			if juc, err := jwt.DecodeUserClaims(theJWT); err != nil {
+				c.Debugf("User JWT not valid: %v", err)
+				c.authViolation()
+				continue
+			} else if ok := ac.IsClaimRevoked(juc); ok {
 				c.sendErrAndDebug("User Authentication Revoked")
 				c.closeConnection(Revocation)
 				continue

server/auth.go

@@ -559,7 +559,7 @@ func (s *Server) processClientOrLeafAuthentication(c *client, opts *Options) boo
 				return false
 			}
 		}
-		if acc.checkUserRevoked(juc.Subject) {
+		if acc.checkUserRevoked(juc.Subject, juc.IssuedAt) {
 			c.Debugf("User authentication revoked")
 			return false
 		}

server/jwt_test.go

@@ -4268,3 +4268,93 @@ func TestJWTJetStreamLimits(t *testing.T) {
 	expect_JSDisabledForAccount(c)
 	c.Close()
 }
+
+func TestJWTUserRevocation(t *testing.T) {
+	createAccountAndUser := func(done chan struct{}, pubKey, jwt1, jwt2, creds1, creds2 *string) {
+		t.Helper()
+		kp, _ := nkeys.CreateAccount()
+		*pubKey, _ = kp.PublicKey()
+		claim := jwt.NewAccountClaims(*pubKey)
+		var err error
+		*jwt1, err = claim.Encode(oKp)
+		require_NoError(t, err)
+
+		ukp, _ := nkeys.CreateUser()
+		seed, _ := ukp.Seed()
+		upub, _ := ukp.PublicKey()
+		uclaim := newJWTTestUserClaims()
+		uclaim.Subject = upub
+
+		ujwt1, err := uclaim.Encode(kp)
+		require_NoError(t, err)
+		*creds1 = genCredsFile(t, ujwt1, seed)
+
+		// create updated claim need to assure that issue time differs
+		claim.Revoke(upub) // revokes all jwt from now on
+		time.Sleep(time.Millisecond * 1100)
+		*jwt2, err = claim.Encode(oKp)
+		require_NoError(t, err)
+
+		ujwt2, err := uclaim.Encode(kp)
+		require_NoError(t, err)
+		*creds2 = genCredsFile(t, ujwt2, seed)
+
+		done <- struct{}{}
+	}
+	// Create Accounts and corresponding revoked and non revoked user creds. Do so concurrently to speed up the test
+	doneChan := make(chan struct{}, 2)
+	defer close(doneChan)
+	var syspub, sysjwt, dummy1, sysCreds, dummyCreds string
+	go createAccountAndUser(doneChan, &syspub, &sysjwt, &dummy1, &sysCreds, &dummyCreds)
+	var apub, ajwt1, ajwt2, aCreds1, aCreds2 string
+	go createAccountAndUser(doneChan, &apub, &ajwt1, &ajwt2, &aCreds1, &aCreds2)
+	for i := 0; i < cap(doneChan); i++ {
+		<-doneChan
+	}
+	defer os.Remove(sysCreds)
+	defer os.Remove(dummyCreds)
+	defer os.Remove(aCreds1)
+	defer os.Remove(aCreds2)
+	dirSrv := createDir(t, "srv")
+	defer os.RemoveAll(dirSrv)
+	conf := createConfFile(t, []byte(fmt.Sprintf(`
+		listen: -1
+		operator: %s
+		system_account: %s
+		resolver: {
+			type: full
+			dir: %s
+		}
+    `, ojwt, syspub, dirSrv)))
+	defer os.Remove(conf)
+	srv, _ := RunServerWithConfig(conf)
+	defer srv.Shutdown()
+	updateJwt(t, srv.ClientURL(), sysCreds, syspub, sysjwt, 1) // update system account jwt
+	updateJwt(t, srv.ClientURL(), sysCreds, apub, ajwt1, 1)    // set account jwt without revocation
+	// use credentials that will be revoked ans assure that the connection will be disconnected
+	nc := natsConnect(t, srv.ClientURL(), nats.UserCredentials(aCreds1),
+		nats.DisconnectErrHandler(func(conn *nats.Conn, err error) {
+			if lErr := conn.LastError(); lErr != nil && strings.Contains(lErr.Error(), "Authentication Revoked") {
+				doneChan <- struct{}{}
+			}
+		}))
+	defer nc.Close()
+	// update account jwt to contain revocation
+	if passCnt := updateJwt(t, srv.ClientURL(), sysCreds, apub, ajwt2, 1); passCnt != 1 {
+		t.Fatalf("Expected jwt update to pass")
+	}
+	// assure that nc got disconnected due to the revocation
+	select {
+	case <-doneChan:
+	case <-time.After(time.Second):
+		t.Fatalf("Expected connection to have failed")
+	}
+	// try again with old credentials. Expected to fail
+	if nc1, err := nats.Connect(srv.ClientURL(), nats.UserCredentials(aCreds1)); err == nil {
+		nc1.Close()
+		t.Fatalf("Expected revoked credentials to fail")
+	}
+	// Assure new creds pass
+	nc2 := natsConnect(t, srv.ClientURL(), nats.UserCredentials(aCreds2))
+	defer nc2.Close()
+}

vendor/github.com/nats-io/jwt/v2/account_claims.go

@@ -235,7 +235,8 @@ func (a *AccountClaims) Revoke(pubKey string) {
 	a.RevokeAt(pubKey, time.Now())
 }
 
-// RevokeAt enters a revocation by public key and timestamp into this export
+// RevokeAt enters a revocation by public key and timestamp into this account
+// This will revoke all jwt issued for pubKey, prior to timestamp
 // If there is already a revocation for this public key that is newer, it is kept.
 func (a *AccountClaims) RevokeAt(pubKey string, timestamp time.Time) {
 	if a.Revocations == nil {
@@ -250,14 +251,18 @@ func (a *AccountClaims) ClearRevocation(pubKey string) {
 	a.Revocations.ClearRevocation(pubKey)
 }
 
-// IsRevokedAt checks if the public key is in the revoked list with a timestamp later than
-// the one passed in. Generally this method is called with time.Now() but other time's can
-// be used for testing.
-func (a *AccountClaims) IsRevokedAt(pubKey string, timestamp time.Time) bool {
-	return a.Revocations.IsRevoked(pubKey, timestamp)
+// isRevoked checks if the public key is in the revoked list with a timestamp later than the one passed in.
+// Generally this method is called with the subject and issue time of the jwt to be tested.
+// DO NOT pass time.Now(), it will not produce a stable/expected response.
+func (a *AccountClaims) isRevoked(pubKey string, claimIssuedAt time.Time) bool {
+	return a.Revocations.IsRevoked(pubKey, claimIssuedAt)
 }
 
-// IsRevoked checks if the public key is in the revoked list with time.Now()
-func (a *AccountClaims) IsRevoked(pubKey string) bool {
-	return a.Revocations.IsRevoked(pubKey, time.Now())
+// IsClaimRevoked checks if the account revoked the claim passed in.
+// Invalid claims (nil, no Subject or IssuedAt) will return true.
+func (a *AccountClaims) IsClaimRevoked(claim *UserClaims) bool {
+	if claim == nil || claim.IssuedAt == 0 || claim.Subject == "" {
+		return true
+	}
+	return a.isRevoked(claim.Subject, time.Unix(claim.IssuedAt, 0))
 }

vendor/github.com/nats-io/jwt/v2/revocation_list.go

@@ -39,9 +39,9 @@ func (r RevocationList) ClearRevocation(pubKey string) {
 }
 
 // IsRevoked checks if the public key is in the revoked list with a timestamp later than
-// the one passed in. Generally this method is called with time.Now() but other time's can
+// the one passed in. Generally this method is called with an issue time but other time's can
 // be used for testing.
 func (r RevocationList) IsRevoked(pubKey string, timestamp time.Time) bool {
 	ts, ok := r[pubKey]
-	return ok && ts > timestamp.Unix()
+	return ok && ts >= timestamp.Unix()
 }

vendor/github.com/nats-io/jwt/v2/validation.go

@@ -105,3 +105,14 @@ func (v *ValidationResults) Errors() []error {
 	}
 	return errs
 }
+
+// Warnings returns only non blocking issues as strings
+func (v *ValidationResults) Warnings() []string {
+	var errs []string
+	for _, v := range v.Issues {
+		if !v.Blocking {
+			errs = append(errs, v.Description)
+		}
+	}
+	return errs
+}

vendor/modules.txt

@@ -1,6 +1,6 @@
 # github.com/minio/highwayhash v1.0.0
 github.com/minio/highwayhash
-# github.com/nats-io/jwt/v2 v2.0.0-20200930010033-c4fd08d85545
+# github.com/nats-io/jwt/v2 v2.0.0-20201006231922-e00ffcea7738
 github.com/nats-io/jwt/v2
 # github.com/nats-io/nats.go v1.10.1-0.20200606002146-fc6fed82929a
 github.com/nats-io/nats.go