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; }