fix(shared_poll): enforce previous HMAC key cutoff against server time (#1209)

hmac_previous_secret_key_valid_until was compared against the iat carried
inside the track signature only. That value is chosen by whoever mints the
signature, so anyone retaining the rotated-out key could keep producing
accepted signatures indefinitely by backdating iat - the cutoff bounded
nothing in the scenario it exists for.

Check the cutoff against server time before consulting the previous
verifier, the same way jwtverify does for the previous JWT HMAC key. The
iat check is kept as an additional constraint inside the grace period.

Author: FZambia

internal/client/handler.go

@@ -1137,9 +1137,22 @@ func (h *Handler) OnTrack(c Client, e centrifuge.TrackEvent) (centrifuge.TrackRe
 		}
 		verified := verifier.verify(e.Channel, b.Signature, keys, c.UserID())
 		if !verified && prevVerifier != nil {
-			if sharedPollCfg.HMACPreviousSecretKeyValidUntil > 0 {
+			if validUntil := sharedPollCfg.HMACPreviousSecretKeyValidUntil; validUntil > 0 {
+				// The rotation cutoff must be enforced against server time. The iat
+				// inside a signature is chosen by whoever minted it, so checking iat
+				// alone would let anyone holding the rotated-out key keep minting
+				// accepted signatures indefinitely by backdating iat – exactly the
+				// scenario the cutoff exists to contain. Once the cutoff passed the
+				// previous key is not consulted at all, same as JWT verifier does.
+				if now > validUntil {
+					log.Info().Str("channel", e.Channel).Str("client", c.ID()).Str("user", c.UserID()).Msg("previous shared poll secret key expired")
+					return centrifuge.TrackReply{}, centrifuge.ErrorPermissionDenied
+				}
+				// Inside the grace period still require the signature to claim an iat
+				// at or before the cutoff, so an old-key backend which keeps issuing
+				// new signatures does not silently extend the rotation window.
 				iat, _ := parseSignatureTimestamps(b.Signature)
-				if iat > sharedPollCfg.HMACPreviousSecretKeyValidUntil {
+				if iat > validUntil {
 					log.Info().Str("channel", e.Channel).Str("client", c.ID()).Str("user", c.UserID()).Msg("track signature issued after previous secret key expiry")
 					return centrifuge.TrackReply{}, centrifuge.ErrorPermissionDenied
 				}

internal/client/handler_test.go

@@ -1513,6 +1513,134 @@ func TestSharedPollRefreshProxyDispatch_FallbackToDefault(t *testing.T) {
 	require.Equal(t, 1, defaultCalls)
 }
 
+// TestOnTrackPreviousSecretKeyRotation checks that shared_poll
+// hmac_previous_secret_key_valid_until bounds the previous key usage in real
+// server time. The iat inside a signature is chosen by whoever minted it, so a
+// holder of the rotated-out key must not be able to keep minting accepted
+// signatures after the cutoff by backdating iat.
+func TestOnTrackPreviousSecretKeyRotation(t *testing.T) {
+	const (
+		currentKey = "current-secret-key"
+		prevKey    = "previous-secret-key"
+		channel    = "shared:poll"
+		userID     = "42"
+	)
+	keys := []string{"key1", "key2"}
+
+	newHandlerWithClient := func(t *testing.T, validUntil int64) (*Handler, *centrifuge.Client, func()) {
+		t.Helper()
+		node := tools.NodeWithMemoryEngineNoHandlers()
+
+		cfg := config.DefaultConfig()
+		cfg.SharedPoll.HMACSecretKey = currentKey
+		cfg.SharedPoll.HMACPreviousSecretKey = prevKey
+		cfg.SharedPoll.HMACPreviousSecretKeyValidUntil = validUntil
+		cfg.Channel.Proxy.SharedPollRefresh.Endpoint = "http://localhost:9999"
+		cfg.Channel.Namespaces = []configtypes.ChannelNamespace{
+			{
+				Name:           "shared",
+				ChannelOptions: configtypes.ChannelOptions{SubscriptionType: "shared_poll"},
+			},
+		}
+		cfgContainer, err := config.NewContainer(cfg)
+		require.NoError(t, err)
+		h := NewHandler(node, cfgContainer, hmacJWTVerifier(t, cfgContainer), nil, &ProxyMap{})
+
+		node.OnConnecting(func(ctx context.Context, event centrifuge.ConnectEvent) (centrifuge.ConnectReply, error) {
+			return centrifuge.ConnectReply{Credentials: &centrifuge.Credentials{UserID: userID}}, nil
+		})
+
+		transport := tools.NewTestTransport()
+		client, closeFn, err := centrifuge.NewClient(context.Background(), node, transport)
+		require.NoError(t, err)
+
+		encoder := protocol.NewJSONCommandEncoder()
+		data, err := encoder.Encode(&protocol.Command{Id: 1, Connect: &protocol.ConnectRequest{}})
+		require.NoError(t, err)
+		require.True(t, centrifuge.HandleReadFrame(client, bytes.NewReader(data), 1<<20))
+
+		return h, client, func() {
+			_ = closeFn()
+			_ = node.Shutdown(context.Background())
+		}
+	}
+
+	track := func(h *Handler, client *centrifuge.Client, sig string) (centrifuge.TrackReply, error) {
+		return h.OnTrack(client, centrifuge.TrackEvent{
+			Channel: channel,
+			Batches: []centrifuge.TrackBatch{{
+				Items:     []centrifuge.TrackItem{{Key: keys[0]}, {Key: keys[1]}},
+				Signature: sig,
+			}},
+		})
+	}
+
+	now := time.Now().Unix()
+
+	t.Run("previous key accepted inside grace period", func(t *testing.T) {
+		validUntil := now + 3600
+		h, client, cleanup := newHandlerWithClient(t, validUntil)
+		defer cleanup()
+
+		sig := makeTestSignature(prevKey, channel, keys, userID, now-60, now+300)
+		reply, err := track(h, client, sig)
+		require.NoError(t, err)
+		require.Len(t, reply.Batches, 1)
+		require.Equal(t, now+300, reply.Batches[0].ExpireAt)
+	})
+
+	t.Run("previous key rejected for iat after cutoff", func(t *testing.T) {
+		validUntil := now + 3600
+		h, client, cleanup := newHandlerWithClient(t, validUntil)
+		defer cleanup()
+
+		sig := makeTestSignature(prevKey, channel, keys, userID, validUntil+1, now+300)
+		_, err := track(h, client, sig)
+		require.Equal(t, centrifuge.ErrorPermissionDenied, err)
+	})
+
+	t.Run("previous key rejected after cutoff passed", func(t *testing.T) {
+		validUntil := now - 3600
+		h, client, cleanup := newHandlerWithClient(t, validUntil)
+		defer cleanup()
+
+		// Honest signature – iat tells the truth about when it was minted.
+		sig := makeTestSignature(prevKey, channel, keys, userID, now, now+300)
+		_, err := track(h, client, sig)
+		require.Equal(t, centrifuge.ErrorPermissionDenied, err)
+
+		// Backdated signature – minted now but claiming an iat before the cutoff.
+		// Must be rejected too, otherwise the cutoff protects nothing against
+		// someone who retained the rotated-out key.
+		sig = makeTestSignature(prevKey, channel, keys, userID, validUntil-10, now+300)
+		_, err = track(h, client, sig)
+		require.Equal(t, centrifuge.ErrorPermissionDenied, err)
+	})
+
+	t.Run("current key still accepted after cutoff passed", func(t *testing.T) {
+		validUntil := now - 3600
+		h, client, cleanup := newHandlerWithClient(t, validUntil)
+		defer cleanup()
+
+		sig := makeTestSignature(currentKey, channel, keys, userID, now, now+300)
+		reply, err := track(h, client, sig)
+		require.NoError(t, err)
+		require.Len(t, reply.Batches, 1)
+		require.Equal(t, now+300, reply.Batches[0].ExpireAt)
+	})
+
+	t.Run("previous key accepted when no cutoff configured", func(t *testing.T) {
+		h, client, cleanup := newHandlerWithClient(t, 0)
+		defer cleanup()
+
+		sig := makeTestSignature(prevKey, channel, keys, userID, now, now+300)
+		reply, err := track(h, client, sig)
+		require.NoError(t, err)
+		require.Len(t, reply.Batches, 1)
+		require.Equal(t, now+300, reply.Batches[0].ExpireAt)
+	})
+}
+
 func TestSingleConnection(t *testing.T) {
 	node := tools.NodeWithMemoryEngineNoHandlers()
 	defer func() { _ = node.Shutdown(context.Background()) }()