diff --git a/api/types/role.go b/api/types/role.go index 6551921bc9aeb..98263f1973d4f 100644 --- a/api/types/role.go +++ b/api/types/role.go @@ -1065,9 +1065,6 @@ func (r *RoleV6) CheckAndSetDefaults() error { if r.Spec.Options.MaxSessionTTL.Value() == 0 { r.Spec.Options.MaxSessionTTL = NewDuration(defaults.MaxCertDuration) } - if r.Spec.Options.PortForwarding == nil { - r.Spec.Options.PortForwarding = NewBoolOption(true) - } if len(r.Spec.Options.BPF) == 0 { r.Spec.Options.BPF = defaults.EnhancedEvents() } diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index e46e06a157133..5bf20e1f15b56 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -2064,6 +2064,15 @@ func (g *GRPCServer) CreateRole(ctx context.Context, req *authpb.CreateRoleReque return nil, trace.Wrap(err) } + // This check *must* happen at the RPC layer rather than somewhere like ValidateRole or CheckAndSetDefaults. We want to prevent role + // creation and updates from defining both port_forwarding and ssh_port_forwarding for the same role. However, when making effective + // roles available to nodes it should be possible for both fields to be assigned in order to maintain backwards compatibility with older + // agents (similar to a role downgrade). + //nolint:staticcheck // this field is preserved for backwards compatibility, but shouldn't be used going forward + if req.Role.GetOptions().SSHPortForwarding != nil && req.Role.GetOptions().PortForwarding != nil { + return nil, trace.BadParameter("options define both 'port_forwarding' and 'ssh_port_forwarding', only one can be set") + } + if err = services.ValidateRole(req.Role); err != nil { return nil, trace.Wrap(err) } @@ -2091,6 +2100,15 @@ func (g *GRPCServer) UpdateRole(ctx context.Context, req *authpb.UpdateRoleReque return nil, trace.Wrap(err) } + // This check *must* happen at the RPC layer rather than somewhere like ValidateRole or CheckAndSetDefaults. We want to prevent role + // creation and updates from defining both port_forwarding and ssh_port_forwarding for the same role. However, when making effective + // roles available to nodes it should be possible for both fields to be assigned in order to maintain backwards compatibility with older + // agents (similar to a role downgrade). + //nolint:staticcheck // this field is preserved for backwards compatibility, but shouldn't be used going forward + if req.Role.GetOptions().SSHPortForwarding != nil && req.Role.GetOptions().PortForwarding != nil { + return nil, trace.BadParameter("options define both 'port_forwarding' and 'ssh_port_forwarding', only one can be set") + } + if err = services.ValidateRole(req.Role); err != nil { return nil, trace.Wrap(err) } @@ -2118,6 +2136,15 @@ func (g *GRPCServer) UpsertRoleV2(ctx context.Context, req *authpb.UpsertRoleReq return nil, trace.Wrap(err) } + // This check *must* happen at the RPC layer rather than somewhere like ValidateRole or CheckAndSetDefaults. We want to prevent role + // creation and updates from defining both port_forwarding and ssh_port_forwarding for the same role. However, when making effective + // roles available to nodes it should be possible for both fields to be assigned in order to maintain backwards compatibility with older + // agents (similar to a role downgrade). + //nolint:staticcheck // this field is preserved for backwards compatibility, but shouldn't be used going forward + if req.Role.GetOptions().SSHPortForwarding != nil && req.Role.GetOptions().PortForwarding != nil { + return nil, trace.BadParameter("options define both 'port_forwarding' and 'ssh_port_forwarding', only one can be set") + } + if err = services.ValidateRole(req.Role); err != nil { return nil, trace.Wrap(err) } diff --git a/lib/services/access_checker.go b/lib/services/access_checker.go index 6174f56d7f7ba..2c7317b42d98d 100644 --- a/lib/services/access_checker.go +++ b/lib/services/access_checker.go @@ -111,6 +111,9 @@ type AccessChecker interface { // CanPortForward returns true if this RoleSet can forward ports. CanPortForward() bool + // SSHPortForwardMode returns the SSHPortForwardMode that the RoleSet allows. + SSHPortForwardMode() SSHPortForwardMode + // DesktopClipboard returns true if the role set has enabled shared // clipboard for desktop sessions. Clipboard sharing is disabled if // one or more of the roles in the set has disabled it. diff --git a/lib/services/access_checker_test.go b/lib/services/access_checker_test.go index 37d0247bb8442..86b49d07d5cb0 100644 --- a/lib/services/access_checker_test.go +++ b/lib/services/access_checker_test.go @@ -565,6 +565,213 @@ func TestAccessCheckerHostUsersShell(t *testing.T) { require.Equal(t, expectedShell, hui.Shell) } +func TestSSHPortForwarding(t *testing.T) { + anyLabels := types.Labels{"*": {"*"}} + localCluster := "cluster" + + allAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("all-allow") + rv.SetOptions(types.RoleOptions{ + PortForwarding: types.NewBoolOption(true), + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(true)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(true)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + allDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("all-deny") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(false)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + allow := newRole(func(rv *types.RoleV6) { + rv.SetName("allow") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(true)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(true)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + deny := newRole(func(rv *types.RoleV6) { + rv.SetName("deny") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(false)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + legacyAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("legacy-allow") + rv.SetOptions(types.RoleOptions{ + PortForwarding: types.NewBoolOption(true), + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + legacyDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("legacy-deny") + rv.SetOptions(types.RoleOptions{ + PortForwarding: types.NewBoolOption(false), + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + remoteAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("remote-allow") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(true)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + remoteDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("remote-deny") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(false)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + localAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("local-allow") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(true)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + localDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("local-deny") + rv.SetOptions(types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, + }, + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + implicitAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("implicit-allow") + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + testCases := []struct { + name string + roleSet RoleSet + expectedMode SSHPortForwardMode + }{ + { + name: "allow all", + roleSet: NewRoleSet(allAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "deny all", + roleSet: NewRoleSet(allDeny), + expectedMode: SSHPortForwardModeOff, + }, + { + name: "allow remote and local", + roleSet: NewRoleSet(allow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "deny remote and local", + roleSet: NewRoleSet(deny), + expectedMode: SSHPortForwardModeOff, + }, + { + name: "legacy allow", + roleSet: NewRoleSet(legacyAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "legacy deny", + roleSet: NewRoleSet(legacyDeny), + expectedMode: SSHPortForwardModeOff, + }, + { + name: "remote allow", + roleSet: NewRoleSet(remoteAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "remote deny", + roleSet: NewRoleSet(remoteDeny), + expectedMode: SSHPortForwardModeLocal, + }, + { + name: "local allow", + roleSet: NewRoleSet(localAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "local deny", + roleSet: NewRoleSet(localDeny), + expectedMode: SSHPortForwardModeRemote, + }, + { + name: "implicit allow", + roleSet: NewRoleSet(implicitAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "conflicting roles: allow all with remote deny", + roleSet: NewRoleSet(allow, remoteDeny), + expectedMode: SSHPortForwardModeLocal, + }, + { + name: "conflicting roles: allow all with local deny", + roleSet: NewRoleSet(allow, localDeny), + expectedMode: SSHPortForwardModeRemote, + }, + { + // legacy behavior prefers explicit allow, so make sure we respect that if one is given + name: "conflicting roles: deny all with legacy allow", + roleSet: NewRoleSet(deny, legacyAllow), + expectedMode: SSHPortForwardModeOn, + }, + { + // legacy behavior prioritizes explicit allow, so make sure we respect that if another role would allow access + name: "conflicting roles: allow all with legacy deny", + roleSet: NewRoleSet(allow, legacyDeny), + expectedMode: SSHPortForwardModeOn, + }, + { + name: "conflicting roles implicit allow explicit deny", + roleSet: NewRoleSet(implicitAllow, deny), + expectedMode: SSHPortForwardModeOff, + }, + } + + for _, c := range testCases { + t.Run(c.name, func(t *testing.T) { + accessChecker := NewAccessCheckerWithRoleSet(&AccessInfo{}, localCluster, c.roleSet) + require.Equal(t, c.expectedMode, accessChecker.SSHPortForwardMode()) + }) + } +} + type serverStub struct { types.Server } diff --git a/lib/services/presets.go b/lib/services/presets.go index d82ba05a4f4b2..887545d164cf6 100644 --- a/lib/services/presets.go +++ b/lib/services/presets.go @@ -117,9 +117,16 @@ func NewPresetEditorRole() types.Role { Options: types.RoleOptions{ CertificateFormat: constants.CertificateFormatStandard, MaxSessionTTL: types.NewDuration(apidefaults.MaxCertDuration), - PortForwarding: types.NewBoolOption(true), - ForwardAgent: types.NewBool(true), - BPF: apidefaults.EnhancedEvents(), + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{ + Enabled: types.NewBoolOption(true), + }, + Local: &types.SSHLocalPortForwarding{ + Enabled: types.NewBoolOption(true), + }, + }, + ForwardAgent: types.NewBool(true), + BPF: apidefaults.EnhancedEvents(), RecordSession: &types.RecordSession{ Desktop: types.NewBoolOption(false), }, @@ -210,10 +217,17 @@ func NewPresetAccessRole() types.Role { Options: types.RoleOptions{ CertificateFormat: constants.CertificateFormatStandard, MaxSessionTTL: types.NewDuration(apidefaults.MaxCertDuration), - PortForwarding: types.NewBoolOption(true), - ForwardAgent: types.NewBool(true), - BPF: apidefaults.EnhancedEvents(), - RecordSession: &types.RecordSession{Desktop: types.NewBoolOption(true)}, + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{ + Enabled: types.NewBoolOption(true), + }, + Local: &types.SSHLocalPortForwarding{ + Enabled: types.NewBoolOption(true), + }, + }, + ForwardAgent: types.NewBool(true), + BPF: apidefaults.EnhancedEvents(), + RecordSession: &types.RecordSession{Desktop: types.NewBoolOption(true)}, }, Allow: types.RoleConditions{ Namespaces: []string{apidefaults.Namespace}, diff --git a/lib/services/role.go b/lib/services/role.go index 485225c3ebb2c..37418d27c41a0 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -134,9 +134,6 @@ func NewImplicitRole() types.Role { Spec: types.RoleSpecV6{ Options: types.RoleOptions{ MaxSessionTTL: types.MaxDuration(), - // Explicitly disable options that default to true, otherwise the option - // will always be enabled, as this implicit role is part of every role set. - PortForwarding: types.NewBoolOption(false), RecordSession: &types.RecordSession{ Desktop: types.NewBoolOption(false), }, @@ -252,7 +249,7 @@ func withWarningReporter(f func(error)) validateRoleOption { } } -// ValidateRole parses validates the role, and sets default values. +// ValidateRole parses, validates, and sets default values on a role. func ValidateRole(r types.Role, opts ...validateRoleOption) error { options := defaultValidateRoleOptions() for _, opt := range opts { @@ -2849,15 +2846,93 @@ func (set RoleSet) CanForwardAgents() bool { return false } -// CanPortForward returns true if a role in the RoleSet allows port forwarding. -func (set RoleSet) CanPortForward() bool { +// SSHPortForwardMode enumerates the possible SSH port forwarding modes available at a given time. +type SSHPortForwardMode int + +const ( + // SSHPortForwardModeOn is the default mode, both remote and local port forwarding is allowed + SSHPortForwardModeOn SSHPortForwardMode = iota + // SSHPortForwardModeOff disallows any port forwarding. + SSHPortForwardModeOff + // SSHPortForwardModeRemote allows remote port forwarding. + SSHPortForwardModeRemote + // SSHPortForwardModeLocal allows local port forwarding. + SSHPortForwardModeLocal +) + +// String implements the Stringer interface for SSHPortForwardMode +func (m SSHPortForwardMode) String() string { + switch m { + case SSHPortForwardModeOff: + return "off" + case SSHPortForwardModeLocal: + return "local" + case SSHPortForwardModeRemote: + return "remote" + default: + return "on" + } +} + +// SSHPortForwardMode returns the SSHPortForwardMode permitted by a RoleSet. Port forwarding is implicitly allowed, but explicit denies take +// precedence of explicit allows when using SSHPortForwarding. The legacy PortForwarding field prefers explicit allows for backwards +// compatibility reasons, but is only evaluated in the absence of an SSHPortForwarding config on the same role. +func (set RoleSet) SSHPortForwardMode() SSHPortForwardMode { + var denyRemote, denyLocal, legacyDeny bool + legacyCanDeny := true + for _, role := range set { - //nolint:staticcheck // this field is preserved for existing deployments, but shouldn't be used going forward - if types.BoolDefaultTrue(role.GetOptions().PortForwarding) { - return true + config := role.GetOptions().SSHPortForwarding + // only consider legacy allows when config isn't provided on the same role + if config == nil { + //nolint:staticcheck // this field is preserved for backwards compatibility, but shouldn't be used going forward + if legacy := role.GetOptions().PortForwarding; legacy != nil { + if legacy.Value { + return SSHPortForwardModeOn + } + legacyDeny = true + } + + continue + } + + if config.Remote != nil && config.Remote.Enabled != nil { + if !config.Remote.Enabled.Value { + denyRemote = true + } + + // an explicit legacy deny is only possible if no explicit SSHPortForwarding config has been provided + legacyCanDeny = false + } + + if config.Local != nil && config.Local.Enabled != nil { + if !config.Local.Enabled.Value { + denyLocal = true + } + + // an explicit legacy deny is only possible if no explicit SSHPortForwarding config has been provided + legacyCanDeny = false } } - return false + + // enforcing implicit allow and preferring allow over explicit deny + switch { + case denyRemote && denyLocal: + return SSHPortForwardModeOff + case legacyDeny && legacyCanDeny: + return SSHPortForwardModeOff + case denyRemote: + return SSHPortForwardModeLocal + case denyLocal: + return SSHPortForwardModeRemote + default: + return SSHPortForwardModeOn + } +} + +// CanPortForward returns true if the RoleSet allows both local and remote port forwarding. +func (set RoleSet) CanPortForward() bool { + return set.SSHPortForwardMode() == SSHPortForwardModeOn } // RecordDesktopSession returns true if the role set has enabled desktop diff --git a/lib/services/role_test.go b/lib/services/role_test.go index b76e9c9956f77..7c85f6a07e0a8 100644 --- a/lib/services/role_test.go +++ b/lib/services/role_test.go @@ -267,7 +267,6 @@ func TestRoleParse(t *testing.T) { Options: types.RoleOptions{ CertificateFormat: constants.CertificateFormatStandard, MaxSessionTTL: types.NewDuration(apidefaults.MaxCertDuration), - PortForwarding: types.NewBoolOption(true), RecordSession: &types.RecordSession{ Default: constants.SessionRecordingModeBestEffort, Desktop: types.NewBoolOption(true), @@ -321,7 +320,6 @@ func TestRoleParse(t *testing.T) { Options: types.RoleOptions{ CertificateFormat: constants.CertificateFormatStandard, MaxSessionTTL: types.NewDuration(apidefaults.MaxCertDuration), - PortForwarding: types.NewBoolOption(true), RecordSession: &types.RecordSession{ Default: constants.SessionRecordingModeBestEffort, Desktop: types.NewBoolOption(true), diff --git a/lib/srv/authhandlers.go b/lib/srv/authhandlers.go index 0947c6a975709..03de6e26c779a 100644 --- a/lib/srv/authhandlers.go +++ b/lib/srv/authhandlers.go @@ -262,8 +262,13 @@ func (h *AuthHandlers) CheckFileCopying(ctx *ServerContext) error { } // CheckPortForward checks if port forwarding is allowed for the users RoleSet. -func (h *AuthHandlers) CheckPortForward(addr string, ctx *ServerContext) error { - if ok := ctx.Identity.AccessChecker.CanPortForward(); !ok { +func (h *AuthHandlers) CheckPortForward(addr string, ctx *ServerContext, requestedMode services.SSHPortForwardMode) error { + allowedMode := ctx.Identity.AccessChecker.SSHPortForwardMode() + if allowedMode == services.SSHPortForwardModeOn { + return nil + } + + if allowedMode == services.SSHPortForwardModeOff || allowedMode != requestedMode { systemErrorMessage := fmt.Sprintf("port forwarding not allowed by role set: %v", ctx.Identity.AccessChecker.RoleNames()) userErrorMessage := "port forwarding not allowed" diff --git a/lib/srv/forward/sshserver.go b/lib/srv/forward/sshserver.go index 53b432e0fea5e..0cf519fdde889 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -1016,6 +1016,18 @@ func (s *Server) checkTCPIPForwardRequest(ctx context.Context, r *ssh.Request) e return err } + // RBAC checks are only necessary when connecting to an agentless node + if s.targetServer != nil && s.targetServer.IsOpenSSHNode() { + scx, err := srv.NewServerContext(s.Context(), s.connectionContext, s, s.identityContext) + if err != nil { + return err + } + + if err := s.authHandlers.CheckPortForward(scx.DstAddr, scx, services.SSHPortForwardModeRemote); err != nil { + return trace.Wrap(err) + } + } + return nil } @@ -1083,11 +1095,13 @@ func (s *Server) handleDirectTCPIPRequest(ctx context.Context, ch ssh.Channel, r ch = scx.TrackActivity(ch) - // Check if the role allows port forwarding for this user. - err = s.authHandlers.CheckPortForward(scx.DstAddr, scx) - if err != nil { - s.stderrWrite(ctx, ch, err.Error()) - return + // RBAC checks are only necessary when connecting to an agentless node + if s.targetServer != nil && s.targetServer.IsOpenSSHNode() { + err = s.authHandlers.CheckPortForward(scx.DstAddr, scx, services.SSHPortForwardModeLocal) + if err != nil { + s.stderrWrite(ctx, ch, err.Error()) + return + } } s.logger.DebugContext(ctx, "Opening direct-tcpip channel", "source_addr", scx.SrcAddr, "dest_addr", scx.DstAddr, "session_id", scx.ID()) diff --git a/lib/srv/forward/sshserver_test.go b/lib/srv/forward/sshserver_test.go index 36eec86b3314d..3dd1b250de40e 100644 --- a/lib/srv/forward/sshserver_test.go +++ b/lib/srv/forward/sshserver_test.go @@ -34,6 +34,7 @@ import ( "github.com/gravitational/teleport/api/utils/keys" apisshutils "github.com/gravitational/teleport/api/utils/sshutils" "github.com/gravitational/teleport/lib/fixtures" + "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/srv" "github.com/gravitational/teleport/lib/sshutils" "github.com/gravitational/teleport/lib/utils" @@ -190,6 +191,7 @@ func TestDirectTCPIP(t *testing.T) { cases := []struct { name string login string + accessChecker services.AccessChecker expectAccepted bool expectRejected bool }{ diff --git a/lib/srv/regular/sshserver.go b/lib/srv/regular/sshserver.go index d80c143e96118..3440c40e761ed 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -1413,16 +1413,16 @@ func (s *Server) HandleNewChan(ctx context.Context, ccx *sshutils.ConnectionCont // canPortForward determines if port forwarding is allowed for the current // user/role/node combo. Returns nil if port forwarding is allowed, non-nil // if denied. -func (s *Server) canPortForward(scx *srv.ServerContext) error { +func (s *Server) canPortForward(scx *srv.ServerContext, mode services.SSHPortForwardMode) error { // Is the node configured to allow port forwarding? if !s.allowTCPForwarding { return trace.AccessDenied("node does not allow port forwarding") } // Check if the role allows port forwarding for this user. - err := s.authHandlers.CheckPortForward(scx.DstAddr, scx) + err := s.authHandlers.CheckPortForward(scx.DstAddr, scx, mode) if err != nil { - return err + return trace.Wrap(err) } return nil @@ -1465,7 +1465,7 @@ func (s *Server) handleDirectTCPIPRequest(ctx context.Context, ccx *sshutils.Con // Bail out now if TCP port forwarding is not allowed for this node/user/role // combo - if err = s.canPortForward(scx); err != nil { + if err = s.canPortForward(scx, services.SSHPortForwardModeLocal); err != nil { s.writeStderr(ctx, channel, err.Error()) return } @@ -2171,7 +2171,7 @@ func (s *Server) createForwardingContext(ctx context.Context, ccx *sshutils.Conn scx.SessionRecordingConfig.SetMode(types.RecordOff) scx.SetAllowFileCopying(s.allowFileCopying) - if err := s.canPortForward(scx); err != nil { + if err := s.canPortForward(scx, services.SSHPortForwardModeRemote); err != nil { scx.Close() return nil, nil, trace.Wrap(err) } diff --git a/lib/srv/regular/sshserver_test.go b/lib/srv/regular/sshserver_test.go index 5e0b9a18bfec9..e0cbd76424c91 100644 --- a/lib/srv/regular/sshserver_test.go +++ b/lib/srv/regular/sshserver_test.go @@ -769,15 +769,43 @@ func TestLockInForce(t *testing.T) { require.NoError(t, err) } +func setPortForwarding(t *testing.T, ctx context.Context, f *sshTestFixture, legacy, remote, local *types.BoolOption) { + roleName := services.RoleNameForUser(f.user) + role, err := f.testSrv.Auth().GetRole(ctx, roleName) + require.NoError(t, err) + roleOptions := role.GetOptions() + roleOptions.PermitX11Forwarding = types.NewBool(true) + roleOptions.ForwardAgent = types.NewBool(true) + //nolint:staticcheck // this field is preserved for existing deployments, but shouldn't be used going forward + roleOptions.PortForwarding = legacy + + if remote != nil || local != nil { + roleOptions.SSHPortForwarding = &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{ + Enabled: remote, + }, + Local: &types.SSHLocalPortForwarding{ + Enabled: local, + }, + } + } + + role.SetOptions(roleOptions) + _, err = f.testSrv.Auth().UpsertRole(ctx, role) + require.NoError(t, err) +} + // TestDirectTCPIP ensures that the server can create a "direct-tcpip" // channel to the target address. The "direct-tcpip" channel is what port // forwarding is built upon. func TestDirectTCPIP(t *testing.T) { + ctx := context.Background() t.Parallel() f := newFixtureWithoutDiskBasedLogging(t) // Startup a test server that will reply with "hello, world\n" ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintln(w, "hello, world") })) defer ts.Close() @@ -786,26 +814,60 @@ func TestDirectTCPIP(t *testing.T) { u, err := url.Parse(ts.URL) require.NoError(t, err) - // Build a http.Client that will dial through the server to establish the - // connection. That's why a custom dialer is used and the dialer uses - // s.clt.Dial (which performs the "direct-tcpip" request). - httpClient := http.Client{ - Transport: &http.Transport{ - Dial: func(network string, addr string) (net.Conn, error) { - return f.ssh.clt.DialContext(context.Background(), "tcp", u.Host) + t.Run("Local forwarding is successful", func(t *testing.T) { + // Build a http.Client that will dial through the server to establish the + // connection. That's why a custom dialer is used and the dialer uses + // s.clt.Dial (which performs the "direct-tcpip" request). + httpClient := http.Client{ + Transport: &http.Transport{ + Dial: func(network string, addr string) (net.Conn, error) { + return f.ssh.clt.DialContext(context.Background(), "tcp", u.Host) + }, }, - }, - } + } - // Perform a HTTP GET to the test HTTP server through a "direct-tcpip" request. - resp, err := httpClient.Get(ts.URL) - require.NoError(t, err) - defer resp.Body.Close() + // Perform a HTTP GET to the test HTTP server through a "direct-tcpip" request. + resp, err := httpClient.Get(ts.URL) + require.NoError(t, err) + defer resp.Body.Close() - // Make sure the response is what was expected. - body, err := io.ReadAll(resp.Body) - require.NoError(t, err) - require.Equal(t, []byte("hello, world\n"), body) + // Make sure the response is what was expected. + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, []byte("hello, world\n"), body) + }) + + t.Run("Local forwarding fails when access is denied", func(t *testing.T) { + httpClient := http.Client{ + Transport: &http.Transport{ + Dial: func(network string, addr string) (net.Conn, error) { + return f.ssh.clt.DialContext(context.Background(), "tcp", u.Host) + }, + }, + } + + setPortForwarding(t, ctx, f, nil, nil, types.NewBoolOption(false)) + // Perform a HTTP GET to the test HTTP server through a "direct-tcpip" request. + //nolint:bodyclose // We expect an error here, no need to close. + _, err := httpClient.Get(ts.URL) + require.Error(t, err) + }) + + t.Run("Local forwarding fails when access is denied by legacy config", func(t *testing.T) { + httpClient := http.Client{ + Transport: &http.Transport{ + Dial: func(network string, addr string) (net.Conn, error) { + return f.ssh.clt.DialContext(context.Background(), "tcp", u.Host) + }, + }, + } + + setPortForwarding(t, ctx, f, types.NewBoolOption(false), nil, nil) + // Perform a HTTP GET to the test HTTP server through a "direct-tcpip" request. + //nolint:bodyclose // We expect an error here, no need to close. + _, err := httpClient.Get(ts.URL) + require.Error(t, err) + }) t.Run("SessionJoinPrincipal cannot use direct-tcpip", func(t *testing.T) { // Ensure that ssh client using SessionJoinPrincipal as Login, cannot @@ -832,8 +894,12 @@ func TestTCPIPForward(t *testing.T) { hostname, err := os.Hostname() require.NoError(t, err) tests := []struct { - name string - listenAddr string + name string + listenAddr string + legacyAllow *types.BoolOption + remoteAllow *types.BoolOption + localAllow *types.BoolOption + expectErr bool }{ { name: "localhost", @@ -847,14 +913,37 @@ func TestTCPIPForward(t *testing.T) { name: "hostname", listenAddr: hostname + ":0", }, + { + name: "remote deny", + listenAddr: "localhost:0", + remoteAllow: types.NewBoolOption(false), + expectErr: true, + }, + { + name: "legacy deny", + listenAddr: "localhost:0", + legacyAllow: types.NewBoolOption(false), + expectErr: true, + }, + { + name: "local deny", + listenAddr: "localhost:0", + localAllow: types.NewBoolOption(false), + expectErr: false, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { f := newFixtureWithoutDiskBasedLogging(t) - + setPortForwarding(t, context.Background(), f, tc.legacyAllow, tc.remoteAllow, tc.localAllow) // Request a listener from the server. listener, err := f.ssh.clt.Listen("tcp", tc.listenAddr) - require.NoError(t, err) + if tc.expectErr { + require.Error(t, err) + return + } else { + require.NoError(t, err) + } // Start up a test server that uses the port forwarded listener. ts := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -871,6 +960,7 @@ func TestTCPIPForward(t *testing.T) { require.NoError(t, err) resp, err := ts.Client().Do(req) require.NoError(t, err) + t.Cleanup(func() { require.NoError(t, resp.Body.Close()) }) diff --git a/lib/web/resources_test.go b/lib/web/resources_test.go index 06a72e099f5c4..3a593cc0df1d7 100644 --- a/lib/web/resources_test.go +++ b/lib/web/resources_test.go @@ -240,7 +240,6 @@ spec: enabled: true max_session_ttl: 30h0m0s pin_source_ip: false - port_forwarding: true record_session: default: best_effort desktop: true