From 7de3242a2789e8f009de82f0ddd49da5553f650b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zolt=C3=A1n=20Papp?= Date: Tue, 19 May 2026 14:46:27 +0200 Subject: [PATCH] encode SessionExpiresAt as 3-state on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the `sessionExpiresAt` field on LoginResponse, SyncResponse and ExtendAuthSessionResponse was 2-state: a valid timestamp meant "new deadline", and nil meant "clear". That conflated two distinct meanings — "no info in this snapshot" vs "expiry is explicitly off / peer is not SSO-tracked" — so a Sync push that legitimately couldn't compute the deadline (settings lookup failed) would silently clear the client's anchor and lose the warning window. Three states now, encoded on the same field number (no .proto schema churn — only comments and the server-side encoder change): - nil pointer (field absent) → "no info"; client preserves anchor - &Timestamp{} (seconds=0, nanos=0) → explicit "disabled / not SSO" sentinel; client clears - valid timestamp → new absolute UTC deadline A new encodeSessionExpiresAt helper centralises the zero/non-zero encoding and is shared by the Sync, Login and ExtendAuthSession builders. The Sync builder still emits nil when settings are missing. Login and ExtendAuthSession always carry an authoritative value. The matching client-side decoder lands on feature/session-extend. --- .../internals/shared/grpc/conversion.go | 30 +++++++++++++++++-- .../internals/shared/grpc/conversion_test.go | 27 +++++++++++++++++ management/internals/shared/grpc/server.go | 17 ++++++----- shared/management/proto/management.proto | 22 +++++++++----- 4 files changed, 79 insertions(+), 17 deletions(-) diff --git a/management/internals/shared/grpc/conversion.go b/management/internals/shared/grpc/conversion.go index fc2484312..b4a0d8b28 100644 --- a/management/internals/shared/grpc/conversion.go +++ b/management/internals/shared/grpc/conversion.go @@ -6,6 +6,7 @@ import ( "net/netip" "net/url" "strings" + "time" log "github.com/sirupsen/logrus" goproto "google.golang.org/protobuf/proto" @@ -186,15 +187,38 @@ func ToSyncResponse(ctx context.Context, config *nbconfig.Config, httpConfig *nb response.NetworkMap.SshAuth = &proto.SSHAuth{AuthorizedUsers: hashedUsers, MachineUsers: machineUsers, UserIDClaim: userIDClaim} } + // settings == nil → field stays nil → "no info in this snapshot", client + // preserves the deadline it already had. settings non-nil → emit either a + // valid deadline or the explicit-zero "disabled" sentinel via + // encodeSessionExpiresAt. if settings != nil { - if deadline := peer.SessionExpiresAt(settings.PeerLoginExpirationEnabled, settings.PeerLoginExpiration); !deadline.IsZero() { - response.SessionExpiresAt = timestamppb.New(deadline) - } + response.SessionExpiresAt = encodeSessionExpiresAt( + peer.SessionExpiresAt(settings.PeerLoginExpirationEnabled, settings.PeerLoginExpiration), + ) } return response } +// encodeSessionExpiresAt encodes a server-side deadline into the 3-state wire +// representation used on LoginResponse, SyncResponse and +// ExtendAuthSessionResponse. See the proto comments on those messages. +// +// - deadline.IsZero() → returns &Timestamp{} (seconds=0, nanos=0): the +// "expiry disabled or peer is not SSO-tracked" sentinel; the client clears +// its anchor. +// - deadline non-zero → returns timestamppb.New(deadline): the new absolute +// UTC deadline. +// +// Returning nil ("no info, preserve client's anchor") is the caller's job — +// only meaningful on Sync builds where settings were not resolved. +func encodeSessionExpiresAt(deadline time.Time) *timestamppb.Timestamp { + if deadline.IsZero() { + return ×tamppb.Timestamp{} + } + return timestamppb.New(deadline) +} + func buildAuthorizedUsersProto(ctx context.Context, authorizedUsers map[string]map[string]struct{}) ([][]byte, map[string]*proto.MachineUserIndexes) { userIDToIndex := make(map[string]uint32) var hashedUsers [][]byte diff --git a/management/internals/shared/grpc/conversion_test.go b/management/internals/shared/grpc/conversion_test.go index 1e75caf95..5efb24319 100644 --- a/management/internals/shared/grpc/conversion_test.go +++ b/management/internals/shared/grpc/conversion_test.go @@ -5,6 +5,7 @@ import ( "net/netip" "reflect" "testing" + "time" "github.com/stretchr/testify/assert" @@ -200,3 +201,29 @@ func TestBuildJWTConfig_Audiences(t *testing.T) { }) } } + +// TestEncodeSessionExpiresAt pins the wire encoding the client's +// applySessionDeadline depends on: +// +// - zero deadline → &Timestamp{} (seconds=0, nanos=0): the explicit +// "expiry disabled or peer is not SSO-tracked" sentinel. +// - non-zero → timestamppb.New(deadline): the absolute UTC deadline. +// +// The third state (nil pointer = "no info in this snapshot") is the caller's +// responsibility on the Sync path when settings could not be resolved; the +// helper itself never returns nil. +func TestEncodeSessionExpiresAt(t *testing.T) { + t.Run("zero deadline encodes as explicit-zero sentinel", func(t *testing.T) { + got := encodeSessionExpiresAt(time.Time{}) + assert.NotNil(t, got, "must not return nil; nil means 'no info', not 'disabled'") + assert.Equal(t, int64(0), got.GetSeconds()) + assert.Equal(t, int32(0), got.GetNanos()) + }) + + t.Run("non-zero deadline round-trips", func(t *testing.T) { + deadline := time.Date(2030, 1, 2, 3, 4, 5, 0, time.UTC) + got := encodeSessionExpiresAt(deadline) + assert.NotNil(t, got) + assert.True(t, got.AsTime().Equal(deadline)) + }) +} diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index 37c6f89e7..85ea8cd44 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -23,7 +23,6 @@ import ( "google.golang.org/grpc/codes" "google.golang.org/grpc/peer" "google.golang.org/grpc/status" - "google.golang.org/protobuf/types/known/timestamppb" "github.com/netbirdio/netbird/shared/management/client/common" @@ -867,9 +866,11 @@ func (s *Server) ExtendAuthSession(ctx context.Context, req *proto.EncryptedMess return nil, mapError(ctx, err) } - resp := &proto.ExtendAuthSessionResponse{} - if !deadline.IsZero() { - resp.SessionExpiresAt = timestamppb.New(deadline) + // Success path normally returns a non-zero deadline. A defensive zero + // would still encode as the explicit "disabled" sentinel rather than nil, + // so the client clears any stale anchor instead of preserving it. + resp := &proto.ExtendAuthSessionResponse{ + SessionExpiresAt: encodeSessionExpiresAt(deadline), } wgKey, err := s.secretsManager.GetWGKey() @@ -909,9 +910,11 @@ func (s *Server) prepareLoginResponse(ctx context.Context, peer *nbpeer.Peer, ne Checks: toProtocolChecks(ctx, postureChecks), } - if deadline := peer.SessionExpiresAt(settings.PeerLoginExpirationEnabled, settings.PeerLoginExpiration); !deadline.IsZero() { - loginResp.SessionExpiresAt = timestamppb.New(deadline) - } + // settings is always non-nil here, so we never emit nil — encoder returns + // either a valid deadline or the explicit-zero "disabled" sentinel. + loginResp.SessionExpiresAt = encodeSessionExpiresAt( + peer.SessionExpiresAt(settings.PeerLoginExpirationEnabled, settings.PeerLoginExpiration), + ) return loginResp, nil } diff --git a/shared/management/proto/management.proto b/shared/management/proto/management.proto index 51c0a4cd0..990a72a63 100644 --- a/shared/management/proto/management.proto +++ b/shared/management/proto/management.proto @@ -142,10 +142,13 @@ message SyncResponse { // Posture checks to be evaluated by client repeated Checks Checks = 6; - // Absolute UTC instant at which the peer's SSO session expires. - // Unset when the peer is not SSO-registered or login expiration is disabled. - // Carried on every Sync snapshot so admin-side changes propagate live without - // a client reconnect. + // 3-state session deadline. Carried on every Sync snapshot so admin-side + // changes propagate live without a client reconnect. + // field unset (nil) → snapshot carries no info; client keeps the + // deadline it already had + // set, seconds=0 nanos=0 → explicit "expiry disabled" or peer is not + // SSO-registered; client clears its anchor + // set, valid timestamp → new absolute UTC deadline google.protobuf.Timestamp sessionExpiresAt = 7; } @@ -259,8 +262,10 @@ message LoginResponse { // Posture checks to be evaluated by client repeated Checks Checks = 3; - // Absolute UTC instant at which the peer's SSO session expires. - // Unset when the peer is not SSO-registered or login expiration is disabled. + // 3-state session deadline; same encoding as SyncResponse.sessionExpiresAt. + // field unset (nil) → no info; client keeps any deadline it had + // set, seconds=0 nanos=0 → explicit "expiry disabled" / non-SSO peer + // set, valid timestamp → new absolute UTC deadline google.protobuf.Timestamp sessionExpiresAt = 4; } @@ -276,7 +281,10 @@ message ExtendAuthSessionRequest { // ExtendAuthSessionResponse contains the refreshed session deadline. message ExtendAuthSessionResponse { - // Absolute UTC instant at which the peer's SSO session now expires. + // 3-state session deadline; same encoding as SyncResponse.sessionExpiresAt. + // In practice ExtendAuthSession only succeeds for SSO peers with expiry + // enabled, so this carries a valid timestamp on the success path. The + // 3-state encoding is documented here for symmetry with Login/Sync. google.protobuf.Timestamp sessionExpiresAt = 1; }