diff --git a/management/cmd/proxy/proxy.go b/management/cmd/proxy/proxy.go index 73f83b3d6..1186c8d61 100644 --- a/management/cmd/proxy/proxy.go +++ b/management/cmd/proxy/proxy.go @@ -10,6 +10,7 @@ import ( "io" "strings" "text/tabwriter" + "unicode" "github.com/spf13/cobra" @@ -68,8 +69,8 @@ func runDisconnectAll(ctx context.Context, s store.Store, out io.Writer, in io.R toDisconnect := 0 w := tabwriter.NewWriter(out, 0, 0, 2, ' ', 0) - _, _ = fmt.Fprintln(w, "ID\tCLUSTER\tIP\tACCOUNT\tSTATUS\tLAST SEEN") - _, _ = fmt.Fprintln(w, "--\t-------\t--\t-------\t------\t---------") + _, _ = fmt.Fprintln(w, "ID\tCLUSTER\tIP\tVERSION\tACCOUNT\tSTATUS\tLAST SEEN") + _, _ = fmt.Fprintln(w, "--\t-------\t--\t-------\t-------\t------\t---------") for _, p := range proxies { if p.Status != rpproxy.StatusDisconnected { @@ -80,11 +81,16 @@ func runDisconnectAll(ctx context.Context, s store.Store, out io.Writer, in io.R if p.AccountID != nil { account = *p.AccountID } + version := "-" + if p.Version != "" { + version = sanitizeReportedValue(p.Version) + } - _, _ = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\n", - p.ID, + _, _ = fmt.Fprintf(w, "%s\t%s\t%s\t%s\t%s\t%s\t%s\n", + sanitizeReportedValue(p.ID), p.ClusterAddress, p.IPAddress, + version, account, p.Status, p.LastSeen.Format("2006-01-02 15:04:05"), @@ -139,3 +145,16 @@ func confirmDisconnectAll(out io.Writer, in io.Reader) (bool, error) { return strings.EqualFold(strings.TrimSpace(scanner.Text()), disconnectAllConfirmation), nil } + +// sanitizeReportedValue replaces non-printable characters in a value the proxy +// reports about itself. Both the id and the version arrive unvalidated over +// gRPC, so a tab would forge a column, a carriage return or ANSI escape would +// redraw the operator's terminal, and U+202E would reverse the rest of the line. +func sanitizeReportedValue(s string) string { + return strings.Map(func(r rune) rune { + if unicode.IsPrint(r) { + return r + } + return '\uFFFD' + }, s) +} diff --git a/management/cmd/proxy/proxy_test.go b/management/cmd/proxy/proxy_test.go index ff0dc8119..6e3cd0c01 100644 --- a/management/cmd/proxy/proxy_test.go +++ b/management/cmd/proxy/proxy_test.go @@ -35,6 +35,7 @@ func seedProxies(t *testing.T, ctx context.Context, s store.Store) { SessionID: "session-1", ClusterAddress: "cluster-a.example.com", IPAddress: "10.0.0.1", + Version: "0.60.0", LastSeen: time.Now(), Status: rpproxy.StatusConnected, }, @@ -89,6 +90,7 @@ func TestRunDisconnectAllWithConfirmation(t *testing.T) { require.Contains(t, output, "proxy-2") require.Contains(t, output, "proxy-3") require.Contains(t, output, "cluster-a.example.com") + require.Contains(t, output, "0.60.0") require.Contains(t, output, "account-1") require.Contains(t, output, "Type \"disconnect all proxies\" to continue") require.Contains(t, output, "Force-marked 2 of 3 reverse proxy instance(s) as disconnected.") @@ -178,3 +180,40 @@ func TestRunDisconnectAllEmpty(t *testing.T) { require.NoError(t, runDisconnectAll(ctx, s, &out, strings.NewReader(""), false, false)) require.Contains(t, out.String(), "No reverse proxy instances found.") } + +func TestRunDisconnectAllEscapesProxyReportedFields(t *testing.T) { + ctx := context.Background() + s := newTestStore(t) + + // A proxy reports its own id and version on connect, so both reach this + // listing unvalidated. Carriage returns, tabs and ANSI escapes would let + // a malicious proxy redraw the table or forge a row on the operator's + // terminal; U+202E would reverse the rendering of the rest of the line. + require.NoError(t, s.SaveProxy(ctx, &rpproxy.Proxy{ + ID: "proxy-\r\x1b[2Kevil", + SessionID: "session-1", + ClusterAddress: "cluster-a.example.com", + IPAddress: "10.0.0.1", + Version: "0.60.0\tfake\rcolumn\u202e", + LastSeen: time.Now(), + Status: rpproxy.StatusConnected, + })) + + var out bytes.Buffer + require.NoError(t, runDisconnectAll(ctx, s, &out, strings.NewReader(disconnectAllConfirmation+"\n"), true, false)) + + output := out.String() + for _, forbidden := range []string{"\r", "\x1b", "\u202e"} { + require.NotContains(t, output, forbidden, "listing must not carry proxy-reported control characters") + } + // The table has one data row; a smuggled tab would add a phantom column. + var dataRow string + for _, line := range strings.Split(output, "\n") { + if strings.Contains(line, "evil") { + dataRow = line + } + } + require.NotEmpty(t, dataRow, "listing should still show the proxy row") + require.NotContains(t, dataRow, "\t", "tabwriter output should not carry a smuggled column separator") + require.Contains(t, dataRow, "0.60.0", "the printable part of the version should survive") +} diff --git a/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go b/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go index 5c973c40e..fed402498 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager_realstore_test.go @@ -99,7 +99,7 @@ func setupDomainTest(t *testing.T) *domainTestEnv { proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter("")) require.NoError(t, err) - _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", testCluster, "127.0.0.1", nil, nil) + _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", testCluster, "127.0.0.1", "", nil, nil) require.NoError(t, err) resolver := &stubResolver{cnames: make(map[string]string)} diff --git a/management/internals/modules/reverseproxy/proxy/manager.go b/management/internals/modules/reverseproxy/proxy/manager.go index 26214c11b..a591b86ca 100644 --- a/management/internals/modules/reverseproxy/proxy/manager.go +++ b/management/internals/modules/reverseproxy/proxy/manager.go @@ -11,7 +11,7 @@ import ( // Manager defines the interface for proxy operations type Manager interface { - Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress string, accountID *string, capabilities *Capabilities) (*Proxy, error) + Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress, version string, accountID *string, capabilities *Capabilities) (*Proxy, error) Disconnect(ctx context.Context, proxyID, sessionID string) error Heartbeat(ctx context.Context, p *Proxy) error GetActiveClusterAddresses(ctx context.Context) ([]string, error) diff --git a/management/internals/modules/reverseproxy/proxy/manager/manager.go b/management/internals/modules/reverseproxy/proxy/manager/manager.go index 943766004..edfa32aa9 100644 --- a/management/internals/modules/reverseproxy/proxy/manager/manager.go +++ b/management/internals/modules/reverseproxy/proxy/manager/manager.go @@ -50,7 +50,7 @@ func NewManager(store store, meter metric.Meter) (*Manager, error) { // Connect registers a new proxy connection in the database. // capabilities may be nil for old proxies that do not report them. -func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress string, accountID *string, capabilities *proxy.Capabilities) (*proxy.Proxy, error) { +func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress, version string, accountID *string, capabilities *proxy.Capabilities) (*proxy.Proxy, error) { now := time.Now() var caps proxy.Capabilities if capabilities != nil { @@ -61,6 +61,7 @@ func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddres SessionID: sessionID, ClusterAddress: clusterAddress, IPAddress: ipAddress, + Version: truncateVersion(version), AccountID: accountID, LastSeen: now, ConnectedAt: &now, @@ -78,6 +79,7 @@ func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddres "sessionID": sessionID, "clusterAddress": clusterAddress, "ipAddress": ipAddress, + "version": p.Version, }).Info("proxy connected") return p, nil @@ -184,3 +186,13 @@ func (m *Manager) DeleteAccountCluster(ctx context.Context, clusterAddress, acco } return nil } + +// truncateVersion cuts a proxy-reported version to the column width so an +// oversized value cannot fail the save and block the connect. +func truncateVersion(version string) string { + runes := []rune(version) + if len(runes) <= proxy.MaxVersionLength { + return version + } + return string(runes[:proxy.MaxVersionLength]) +} diff --git a/management/internals/modules/reverseproxy/proxy/manager/manager_test.go b/management/internals/modules/reverseproxy/proxy/manager/manager_test.go index 5c44470a3..d5a3ce777 100644 --- a/management/internals/modules/reverseproxy/proxy/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/proxy/manager/manager_test.go @@ -4,8 +4,10 @@ import ( "context" "errors" "fmt" + "strings" "testing" "time" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -124,7 +126,7 @@ func TestConnect_WithAccountID(t *testing.T) { } mgr := newTestManager(s) - _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "cluster.example.com", "10.0.0.1", &accountID, nil) + _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "cluster.example.com", "10.0.0.1", "0.60.0", &accountID, nil) require.NoError(t, err) require.NotNil(t, savedProxy) @@ -132,6 +134,7 @@ func TestConnect_WithAccountID(t *testing.T) { assert.Equal(t, "session-1", savedProxy.SessionID) assert.Equal(t, "cluster.example.com", savedProxy.ClusterAddress) assert.Equal(t, "10.0.0.1", savedProxy.IPAddress) + assert.Equal(t, "0.60.0", savedProxy.Version, "reported proxy version should be stored") assert.Equal(t, &accountID, savedProxy.AccountID) assert.Equal(t, proxy.StatusConnected, savedProxy.Status) assert.NotNil(t, savedProxy.ConnectedAt) @@ -147,7 +150,7 @@ func TestConnect_WithoutAccountID(t *testing.T) { } mgr := newTestManager(s) - _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "eu.proxy.netbird.io", "10.0.0.1", nil, nil) + _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "eu.proxy.netbird.io", "10.0.0.1", "", nil, nil) require.NoError(t, err) require.NotNil(t, savedProxy) @@ -155,6 +158,29 @@ func TestConnect_WithoutAccountID(t *testing.T) { assert.Equal(t, proxy.StatusConnected, savedProxy.Status) } +func TestConnect_TruncatesOversizedVersion(t *testing.T) { + var savedProxy *proxy.Proxy + s := &mockStore{ + saveProxyFunc: func(_ context.Context, p *proxy.Proxy) error { + savedProxy = p + return nil + }, + } + + // Multi-byte runes make sure the cut counts characters, as varchar does, + // and never splits a rune into invalid UTF-8. + version := strings.Repeat("ΓΌ", proxy.MaxVersionLength+10) + + mgr := newTestManager(s) + _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "cluster.example.com", "10.0.0.1", version, nil, nil) + require.NoError(t, err) + + require.NotNil(t, savedProxy) + assert.Equal(t, proxy.MaxVersionLength, utf8.RuneCountInString(savedProxy.Version), "stored version should be cut to the column width") + assert.True(t, utf8.ValidString(savedProxy.Version), "stored version should remain valid UTF-8") + assert.True(t, strings.HasPrefix(version, savedProxy.Version), "stored version should be a prefix of the reported one") +} + func TestConnect_StoreError(t *testing.T) { s := &mockStore{ saveProxyFunc: func(_ context.Context, _ *proxy.Proxy) error { @@ -163,7 +189,7 @@ func TestConnect_StoreError(t *testing.T) { } mgr := newTestManager(s) - _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "cluster.example.com", "10.0.0.1", nil, nil) + _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "cluster.example.com", "10.0.0.1", "", nil, nil) assert.Error(t, err) } diff --git a/management/internals/modules/reverseproxy/proxy/manager_mock.go b/management/internals/modules/reverseproxy/proxy/manager_mock.go index 36d6f53fc..ec6df8a4a 100644 --- a/management/internals/modules/reverseproxy/proxy/manager_mock.go +++ b/management/internals/modules/reverseproxy/proxy/manager_mock.go @@ -113,18 +113,18 @@ func (mr *MockManagerMockRecorder) ClusterSupportsPrivate(ctx, clusterAddr any) } // Connect mocks base method. -func (m *MockManager) Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress string, accountID *string, capabilities *Capabilities) (*Proxy, error) { +func (m *MockManager) Connect(ctx context.Context, proxyID, sessionID, clusterAddress, ipAddress, version string, accountID *string, capabilities *Capabilities) (*Proxy, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "Connect", ctx, proxyID, sessionID, clusterAddress, ipAddress, accountID, capabilities) + ret := m.ctrl.Call(m, "Connect", ctx, proxyID, sessionID, clusterAddress, ipAddress, version, accountID, capabilities) ret0, _ := ret[0].(*Proxy) ret1, _ := ret[1].(error) return ret0, ret1 } // Connect indicates an expected call of Connect. -func (mr *MockManagerMockRecorder) Connect(ctx, proxyID, sessionID, clusterAddress, ipAddress, accountID, capabilities any) *gomock.Call { +func (mr *MockManagerMockRecorder) Connect(ctx, proxyID, sessionID, clusterAddress, ipAddress, version, accountID, capabilities any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Connect", reflect.TypeOf((*MockManager)(nil).Connect), ctx, proxyID, sessionID, clusterAddress, ipAddress, accountID, capabilities) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Connect", reflect.TypeOf((*MockManager)(nil).Connect), ctx, proxyID, sessionID, clusterAddress, ipAddress, version, accountID, capabilities) } // CountAccountProxies mocks base method. diff --git a/management/internals/modules/reverseproxy/proxy/proxy.go b/management/internals/modules/reverseproxy/proxy/proxy.go index 4404b0d24..fdecc5bfd 100644 --- a/management/internals/modules/reverseproxy/proxy/proxy.go +++ b/management/internals/modules/reverseproxy/proxy/proxy.go @@ -9,6 +9,9 @@ const ( StatusDisconnected = "disconnected" ) +// MaxVersionLength is the width of the Version column, in characters. +const MaxVersionLength = 255 + // Capabilities describes what a proxy can handle, as reported via gRPC. // Nil fields mean the proxy never reported this capability. type Capabilities struct { @@ -31,6 +34,7 @@ type Proxy struct { SessionID string `gorm:"type:varchar(36)"` ClusterAddress string `gorm:"type:varchar(255);not null;index:idx_proxy_cluster_status"` IPAddress string `gorm:"type:varchar(45)"` + Version string `gorm:"type:varchar(255)"` AccountID *string `gorm:"type:varchar(255);index:idx_proxy_account_id"` LastSeen time.Time `gorm:"not null;index:idx_proxy_last_seen"` ConnectedAt *time.Time diff --git a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go index ccb955cd8..6f641b73b 100644 --- a/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go +++ b/management/internals/modules/reverseproxy/service/manager/domain_validation_test.go @@ -30,7 +30,7 @@ func withRealDomainManager(t *testing.T, mgr *Manager, testStore store.Store) { proxyMgr, err := proxymanager.NewManager(testStore, noop.NewMeterProvider().Meter("")) require.NoError(t, err) - _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", validationTestCluster, "127.0.0.1", nil, nil) + _, err = proxyMgr.Connect(ctx, "proxy-1", "session-1", validationTestCluster, "127.0.0.1", "", nil, nil) require.NoError(t, err) accountMgr := &mock_server.MockAccountManager{ diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index b1527f0ab..28df7ed6f 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -414,6 +414,7 @@ func (s *ProxyServiceServer) SetProxyController(proxyController proxy.Controller type proxyConnectParams struct { proxyID string address string + version string capabilities *proto.ProxyCapabilities } @@ -424,6 +425,7 @@ func (s *ProxyServiceServer) GetMappingUpdate(req *proto.GetMappingUpdateRequest return err } params.capabilities = req.GetCapabilities() + params.version = req.GetVersion() conn, proxyRecord, err := s.registerProxyConnection(stream.Context(), params, &proxyConnection{ stream: stream, @@ -457,6 +459,7 @@ func (s *ProxyServiceServer) SyncMappings(stream proto.ProxyService_SyncMappings return err } params.capabilities = init.GetCapabilities() + params.version = init.GetVersion() conn, proxyRecord, err := s.registerProxyConnection(stream.Context(), params, &proxyConnection{ syncStream: stream, @@ -568,7 +571,7 @@ func (s *ProxyServiceServer) registerProxyConnection(ctx context.Context, params } } - proxyRecord, err := s.proxyManager.Connect(ctx, params.proxyID, sessionID, params.address, peerInfo, accountID, caps) + proxyRecord, err := s.proxyManager.Connect(ctx, params.proxyID, sessionID, params.address, peerInfo, params.version, accountID, caps) if err != nil { cancel() if accountID != nil { diff --git a/management/internals/shared/grpc/proxy_connect_version_test.go b/management/internals/shared/grpc/proxy_connect_version_test.go new file mode 100644 index 000000000..e2fc49a38 --- /dev/null +++ b/management/internals/shared/grpc/proxy_connect_version_test.go @@ -0,0 +1,93 @@ +package grpc + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" + rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" + "github.com/netbirdio/netbird/shared/management/proto" +) + +const ( + versionTestProxyID = "proxy-a" + versionTestCluster = "cluster.example.com" + versionTestVersion = "0.60.0" +) + +// hangupStream cancels its context on the first Send, emulating a proxy that +// disconnects right after receiving the initial snapshot. The legacy stream +// carries no proxy-to-management messages, so this is the only way for +// GetMappingUpdate to return. +type hangupStream struct { + recordingStream + ctx context.Context + cancel context.CancelFunc +} + +func (s *hangupStream) Send(m *proto.GetMappingUpdateResponse) error { + s.cancel() + return s.recordingStream.Send(m) +} + +func (s *hangupStream) Context() context.Context { return s.ctx } + +// newVersionTestServer wires a server whose proxy manager only accepts a +// Connect carrying versionTestVersion, so a dropped or mangled version fails +// the test as an unexpected call. +func newVersionTestServer(t *testing.T) *ProxyServiceServer { + t.Helper() + ctrl := gomock.NewController(t) + + svcMgr := rpservice.NewMockManager(ctrl) + svcMgr.EXPECT().GetGlobalServices(gomock.Any()).Return(nil, nil) + + proxyMgr := proxy.NewMockManager(ctrl) + proxyMgr.EXPECT(). + Connect(gomock.Any(), versionTestProxyID, gomock.Any(), versionTestCluster, gomock.Any(), versionTestVersion, gomock.Any(), gomock.Any()). + Return(&proxy.Proxy{ID: versionTestProxyID, Version: versionTestVersion}, nil) + proxyMgr.EXPECT().Disconnect(gomock.Any(), versionTestProxyID, gomock.Any()).Return(nil) + + s := newSnapshotTestServer(t, 10) + s.serviceManager = svcMgr + s.proxyManager = proxyMgr + return s +} + +func TestSyncMappings_ForwardsProxyVersion(t *testing.T) { + s := newVersionTestServer(t) + + // The init carries the version, the ack acknowledges the empty snapshot, + // and the exhausted fake stream then ends the RPC. + stream := &syncRecordingStream{ + recvMsgs: []*proto.SyncMappingsRequest{ + {Msg: &proto.SyncMappingsRequest_Init{Init: &proto.SyncMappingsInit{ + ProxyId: versionTestProxyID, + Address: versionTestCluster, + Version: versionTestVersion, + }}}, + ackMsg(), + }, + } + + err := s.SyncMappings(stream) + require.ErrorContains(t, err, "no more recv messages") +} + +func TestGetMappingUpdate_ForwardsProxyVersion(t *testing.T) { + s := newVersionTestServer(t) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + stream := &hangupStream{ctx: ctx, cancel: cancel} + + err := s.GetMappingUpdate(&proto.GetMappingUpdateRequest{ + ProxyId: versionTestProxyID, + Address: versionTestCluster, + Version: versionTestVersion, + }, stream) + require.ErrorIs(t, err, context.Canceled) +} diff --git a/management/internals/shared/grpc/validate_session_test.go b/management/internals/shared/grpc/validate_session_test.go index 4e70e61e4..b300e8c1d 100644 --- a/management/internals/shared/grpc/validate_session_test.go +++ b/management/internals/shared/grpc/validate_session_test.go @@ -570,7 +570,7 @@ func (m *testValidateSessionServiceManager) DeleteAccountCluster(_ context.Conte type testValidateSessionProxyManager struct{} -func (m *testValidateSessionProxyManager) Connect(_ context.Context, _, _, _, _ string, _ *string, _ *proxy.Capabilities) (*proxy.Proxy, error) { +func (m *testValidateSessionProxyManager) Connect(_ context.Context, _, _, _, _, _ string, _ *string, _ *proxy.Capabilities) (*proxy.Proxy, error) { return nil, nil } diff --git a/proxy/management_integration_test.go b/proxy/management_integration_test.go index df016e790..0e148f858 100644 --- a/proxy/management_integration_test.go +++ b/proxy/management_integration_test.go @@ -204,7 +204,7 @@ func (m *testAccessLogManager) GetAllAccessLogs(_ context.Context, _, _ string, // testProxyManager is a mock implementation of proxy.Manager for testing. type testProxyManager struct{} -func (m *testProxyManager) Connect(_ context.Context, proxyID, sessionID, _, _ string, _ *string, _ *nbproxy.Capabilities) (*nbproxy.Proxy, error) { +func (m *testProxyManager) Connect(_ context.Context, proxyID, sessionID, _, _, _ string, _ *string, _ *nbproxy.Capabilities) (*nbproxy.Proxy, error) { return &nbproxy.Proxy{ID: proxyID, SessionID: sessionID, Status: nbproxy.StatusConnected}, nil }