From 32d3dd2ce777a48412067dc27e75b424dc712316 Mon Sep 17 00:00:00 2001 From: Erik Tate Date: Tue, 10 Dec 2024 11:35:16 -0500 Subject: [PATCH] adding more granular port forwarding access controls that allow for specifying within a role whether remote forwarding, local forwarding, both, or none should be allowed --- api/types/role.go | 3 - lib/auth/grpcserver.go | 79 ++++++++- lib/auth/grpcserver_test.go | 250 ++++++++++++++++++++++++++++ lib/services/access_checker.go | 3 + lib/services/access_checker_test.go | 207 +++++++++++++++++++++++ lib/services/presets.go | 28 +++- lib/services/role.go | 95 +++++++++-- lib/services/role_test.go | 2 - lib/srv/authhandlers.go | 9 +- lib/srv/forward/sshserver.go | 24 ++- lib/srv/forward/sshserver_test.go | 2 + lib/srv/regular/sshserver.go | 10 +- lib/srv/regular/sshserver_test.go | 132 ++++++++++++--- lib/web/resources_test.go | 1 - 14 files changed, 787 insertions(+), 58 deletions(-) diff --git a/api/types/role.go b/api/types/role.go index 4e718a1efcae9..4b3e41baf6baa 100644 --- a/api/types/role.go +++ b/api/types/role.go @@ -1069,9 +1069,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 f18389c44819b..20cda50d900aa 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -81,6 +81,7 @@ import ( apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/api/types/installers" "github.com/gravitational/teleport/api/types/wrappers" + apiutils "github.com/gravitational/teleport/api/utils" "github.com/gravitational/teleport/lib/auth/accessmonitoringrules/accessmonitoringrulesv1" "github.com/gravitational/teleport/lib/auth/authclient" "github.com/gravitational/teleport/lib/auth/autoupdate/autoupdatev1" @@ -1975,11 +1976,58 @@ func (g *GRPCServer) DeleteAllKubernetesServers(ctx context.Context, req *authpb // version for some features of the role returns a shallow copy of the given // role downgraded for compatibility with the older version. func maybeDowngradeRole(ctx context.Context, role *types.RoleV6) (*types.RoleV6, error) { - // Teleport 16 supports all role features that Teleport 15 does, - // so no downgrade is necessary. + clientVersionString, ok := metadata.ClientVersionFromContext(ctx) + if !ok { + // This client is not reporting its version via gRPC metadata. Teleport + // clients have been reporting their version for long enough that older + // clients won't even support v6 roles at all, so this is likely a + // third-party client, and we shouldn't assume that downgrading the role + // will do more good than harm. + return role, nil + } + + clientVersion, err := semver.NewVersion(clientVersionString) + if err != nil { + return nil, trace.BadParameter("unrecognized client version: %s is not a valid semver", clientVersionString) + } + + role = maybeDowngradeRoleSSHPortForwarding(role, clientVersion) return role, nil } +var minSupportedSSHPortForwardingVersions = map[int64]semver.Version{ + 17: {Major: 17, Minor: 1, Patch: 0}, +} + +func maybeDowngradeRoleSSHPortForwarding(role *types.RoleV6, clientVersion *semver.Version) *types.RoleV6 { + sshPortForwarding := role.GetOptions().SSHPortForwarding + if sshPortForwarding == nil || (sshPortForwarding.Remote == nil && sshPortForwarding.Local == nil) { + return role + } + + minSupportedVersion, ok := minSupportedSSHPortForwardingVersions[clientVersion.Major] + if ok { + if supported, err := utils.MinVerWithoutPreRelease(clientVersion.String(), minSupportedVersion.String()); supported || err != nil { + return role + } + } + + role = apiutils.CloneProtoMsg(role) + options := role.GetOptions() + + //nolint:staticcheck // this field is preserved for backwards compatibility + options.PortForwarding = types.NewBoolOption(services.RoleSet{role}.CanPortForward()) + role.SetOptions(options) + reason := fmt.Sprintf(`Client version %q does not support granular SSH port forwarding. Role %q will be downgraded `+ + `to simple port forwarding rules instead. In order to support granular SSH port forwarding, all clients must be `+ + `updated to version %q or higher.`, clientVersion, role.GetName(), minSupportedVersion) + if role.Metadata.Labels == nil { + role.Metadata.Labels = make(map[string]string, 1) + } + role.Metadata.Labels[types.TeleportDowngradedLabel] = reason + return role +} + // GetRole retrieves a role by name. func (g *GRPCServer) GetRole(ctx context.Context, req *authpb.GetRoleRequest) (*types.RoleV6, error) { auth, err := g.authenticate(ctx) @@ -2062,6 +2110,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) } @@ -2089,6 +2146,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) } @@ -2116,6 +2182,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/auth/grpcserver_test.go b/lib/auth/grpcserver_test.go index 2890b255f7114..5ba47e79a0eae 100644 --- a/lib/auth/grpcserver_test.go +++ b/lib/auth/grpcserver_test.go @@ -57,6 +57,7 @@ import ( clusterconfigpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/clusterconfig/v1" mfav1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/mfa/v1" "github.com/gravitational/teleport/api/internalutils/stream" + "github.com/gravitational/teleport/api/metadata" "github.com/gravitational/teleport/api/mfa" "github.com/gravitational/teleport/api/observability/tracing" "github.com/gravitational/teleport/api/types" @@ -4563,6 +4564,255 @@ func TestGRPCServer_GetInstallers(t *testing.T) { } } +func TestRoleVersions(t *testing.T) { + t.Parallel() + srv := newTestTLSServer(t) + + newRole := func(name string, version string, spec types.RoleSpecV6) types.Role { + role, err := types.NewRoleWithVersion(name, version, spec) + meta := role.GetMetadata() + role.SetMetadata(meta) + require.NoError(t, err) + return role + } + + enabledRole := newRole("test_role_enabled", types.V7, types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindRole, services.RW()), + }, + }, + Options: types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{ + Enabled: types.NewBoolOption(true), + }, + Local: &types.SSHLocalPortForwarding{ + Enabled: types.NewBoolOption(true), + }, + }, + }, + }) + + disabledRole := newRole("test_role_disabled", types.V7, types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindRole, services.RW()), + }, + }, + Options: types.RoleOptions{ + SSHPortForwarding: &types.SSHPortForwarding{ + Remote: &types.SSHRemotePortForwarding{ + Enabled: types.NewBoolOption(false), + }, + Local: &types.SSHLocalPortForwarding{ + Enabled: types.NewBoolOption(false), + }, + }, + }, + }) + + undefinedRole := newRole("test_role_implicit", types.V7, types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindRole, services.RW()), + }, + }, + }) + + user, err := CreateUser(context.Background(), srv.Auth(), "user", enabledRole, disabledRole, undefinedRole) + require.NoError(t, err) + + client, err := srv.NewClient(TestUser(user.GetName())) + require.NoError(t, err) + + for _, tc := range []struct { + desc string + clientVersions []string + expectError bool + inputRole types.Role + expectedRole types.Role + expectDowngraded bool + }{ + { + desc: "up to date - enabled", + clientVersions: []string{ + "17.1.0", "17.1.0-dev", "", + }, + inputRole: enabledRole, + expectedRole: enabledRole, + }, + { + desc: "up to date - disabled", + clientVersions: []string{ + "17.1.0", "17.1.0-dev", "", + }, + inputRole: disabledRole, + expectedRole: disabledRole, + }, + { + desc: "up to date - undefined", + clientVersions: []string{ + "17.1.0", "17.1.0-dev", "", + }, + inputRole: undefinedRole, + expectedRole: undefinedRole, + }, + { + desc: "downgrade SSH access control granularity - enabled", + clientVersions: []string{ + "17.0.0", + }, + inputRole: enabledRole, + expectedRole: newRole(enabledRole.GetName(), types.V7, types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindRole, services.RW()), + }, + }, + Options: types.RoleOptions{ + PortForwarding: types.NewBoolOption(true), + SSHPortForwarding: enabledRole.GetOptions().SSHPortForwarding, + }, + }), + expectDowngraded: true, + }, + { + desc: "downgrade SSH access control granularity - disabled", + clientVersions: []string{ + "17.0.0", + }, + inputRole: disabledRole, + expectedRole: newRole(disabledRole.GetName(), types.V7, types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindRole, services.RW()), + }, + }, + Options: types.RoleOptions{ + PortForwarding: types.NewBoolOption(false), + SSHPortForwarding: disabledRole.GetOptions().SSHPortForwarding, + }, + }), + expectDowngraded: true, + }, + { + desc: "downgrade SSH access control granularity - undefined", + clientVersions: []string{ + "17.0.0", + }, + inputRole: undefinedRole, + expectedRole: undefinedRole, + expectDowngraded: false, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + for _, clientVersion := range tc.clientVersions { + t.Run(clientVersion, func(t *testing.T) { + // setup client metadata + ctx := context.Background() + if clientVersion == "" { + ctx = context.WithValue(ctx, metadata.DisableInterceptors{}, struct{}{}) + } else { + ctx = metadata.AddMetadataToContext(ctx, map[string]string{ + metadata.VersionKey: clientVersion, + }) + } + + checkRole := func(gotRole types.Role) { + t.Helper() + if tc.expectError { + return + } + require.Empty(t, cmp.Diff(tc.expectedRole, gotRole, + cmpopts.IgnoreFields(types.Metadata{}, "Revision", "Labels"))) + // The downgraded label value won't match exactly because it + // includes the client version, so just check it's not empty + // and ignore it in the role diff. + if tc.expectDowngraded { + require.NotEmpty(t, gotRole.GetMetadata().Labels[types.TeleportDowngradedLabel]) + } + } + checkErr := func(err error) { + t.Helper() + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + } + + // Test GetRole + gotRole, err := client.GetRole(ctx, tc.inputRole.GetName()) + checkErr(err) + checkRole(gotRole) + + // Test GetRoles + gotRoles, err := client.GetRoles(ctx) + checkErr(err) + if !tc.expectError { + foundTestRole := false + for _, gotRole := range gotRoles { + if gotRole.GetName() != tc.inputRole.GetName() { + continue + } + checkRole(gotRole) + foundTestRole = true + break + } + require.True(t, foundTestRole, "GetRoles result does not include expected role") + } + + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + + // Test WatchEvents + watcher, err := client.NewWatcher(ctx, types.Watch{Name: "roles", Kinds: []types.WatchKind{{Kind: types.KindRole}}}) + require.NoError(t, err) + defer watcher.Close() + + // Swallow the init event + e := <-watcher.Events() + require.Equal(t, types.OpInit, e.Type) + + // Re-upsert the role so that the watcher sees it, do this + // on the auth server directly to avoid the + // TeleportDowngradedLabel check in ServerWithRoles + tc.inputRole, err = srv.Auth().UpsertRole(ctx, tc.inputRole) + require.NoError(t, err) + + gotRole, err = func() (types.Role, error) { + for { + select { + case <-watcher.Done(): + return nil, watcher.Error() + case e := <-watcher.Events(): + if gotRole, ok := e.Resource.(types.Role); ok && gotRole.GetName() == tc.inputRole.GetName() { + return gotRole, nil + } + } + } + }() + checkErr(err) + checkRole(gotRole) + + if !tc.expectError { + // Try to re-upsert the role we got. If it was + // downgraded, it should be rejected due to the + // TeleportDowngradedLabel + _, err = client.UpsertRole(ctx, gotRole) + if tc.expectDowngraded { + require.Error(t, err) + } else { + require.NoError(t, err) + } + } + }) + } + }) + } +} + func TestUpsertApplicationServerOrigin(t *testing.T) { t.Parallel() 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 f17eb24d76d9c..41b3bc8aaad59 100644 --- a/lib/services/access_checker_test.go +++ b/lib/services/access_checker_test.go @@ -567,6 +567,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 e18be85a95bdd..eaf951c518d16 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -133,9 +133,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), }, @@ -251,7 +248,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 { @@ -2848,15 +2845,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 d474585e58cbf..6c86a86feecaf 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 d80f5704acfe4..eed3b37c0a53f 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 978be8c89ea32..ac83a3266970d 100644 --- a/lib/srv/forward/sshserver.go +++ b/lib/srv/forward/sshserver.go @@ -1017,6 +1017,18 @@ func (s *Server) checkTCPIPForwardRequest(r *ssh.Request) error { 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 } @@ -1084,11 +1096,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(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(ch, err.Error()) + return + } } s.log.Debugf("Opening direct-tcpip channel from %v to %v in context %v.", scx.SrcAddr, scx.DstAddr, scx.ID()) diff --git a/lib/srv/forward/sshserver_test.go b/lib/srv/forward/sshserver_test.go index 98e409e63b692..8b4c572f27382 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 a4bbd56a552df..de400ac756989 100644 --- a/lib/srv/regular/sshserver.go +++ b/lib/srv/regular/sshserver.go @@ -1420,16 +1420,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 @@ -1472,7 +1472,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 { writeStderr(channel, err.Error()) return } @@ -2178,7 +2178,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 c2f5990339255..4c62e387e9569 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