diff --git a/api/types/role.go b/api/types/role.go index 1529a4c81cc20..f7f387ed6819a 100644 --- a/api/types/role.go +++ b/api/types/role.go @@ -1039,9 +1039,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/services/access_checker_test.go b/lib/services/access_checker_test.go index aebb3c03d4f2e..9362e2c97700f 100644 --- a/lib/services/access_checker_test.go +++ b/lib/services/access_checker_test.go @@ -569,6 +569,29 @@ 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("all-allow") rv.SetOptions(types.RoleOptions{ @@ -580,6 +603,17 @@ func TestSSHPortForwarding(t *testing.T) { rv.SetNodeLabels(types.Allow, anyLabels) }) + deny := 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) + }) + legacyAllow := newRole(func(rv *types.RoleV6) { rv.SetName("legacy-allow") rv.SetOptions(types.RoleOptions{ @@ -588,13 +622,19 @@ func TestSSHPortForwarding(t *testing.T) { rv.SetNodeLabels(types.Allow, anyLabels) }) - allAllow := newRole(func(rv *types.RoleV6) { - rv.SetName("all-allow") + legacyDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("legacy-allow") + rv.SetOptions(types.RoleOptions{ + PortForwarding: types.NewBoolOption(false), + }) + rv.SetNodeLabels(types.Allow, anyLabels) + }) + + remoteAllow := newRole(func(rv *types.RoleV6) { + rv.SetName("remote-deny") 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) @@ -603,7 +643,6 @@ func TestSSHPortForwarding(t *testing.T) { remoteDeny := newRole(func(rv *types.RoleV6) { rv.SetName("remote-deny") rv.SetOptions(types.RoleOptions{ - PortForwarding: types.NewBoolOption(true), SSHPortForwarding: &types.SSHPortForwarding{ Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(false)}, }, @@ -611,24 +650,21 @@ func TestSSHPortForwarding(t *testing.T) { rv.SetNodeLabels(types.Allow, anyLabels) }) - localDeny := newRole(func(rv *types.RoleV6) { + localAllow := newRole(func(rv *types.RoleV6) { rv.SetName("local-deny") rv.SetOptions(types.RoleOptions{ - PortForwarding: types.NewBoolOption(true), SSHPortForwarding: &types.SSHPortForwarding{ - Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(true)}, }, }) rv.SetNodeLabels(types.Allow, anyLabels) }) - deny := newRole(func(rv *types.RoleV6) { - rv.SetName("all-deny") + localDeny := newRole(func(rv *types.RoleV6) { + rv.SetName("local-deny") rv.SetOptions(types.RoleOptions{ - PortForwarding: types.NewBoolOption(true), SSHPortForwarding: &types.SSHPortForwarding{ - Remote: &types.SSHRemotePortForwarding{Enabled: types.NewBoolOption(false)}, - Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, + Local: &types.SSHLocalPortForwarding{Enabled: types.NewBoolOption(false)}, }, }) rv.SetNodeLabels(types.Allow, anyLabels) @@ -645,18 +681,38 @@ func TestSSHPortForwarding(t *testing.T) { expectedMode SSHPortForwardMode }{ { - name: "explicit allow", + 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: "all allowed", - roleSet: NewRoleSet(allAllow), + name: "legacy deny", + roleSet: NewRoleSet(legacyDeny), + expectedMode: SSHPortForwardModeOff, + }, + { + name: "remote allow", + roleSet: NewRoleSet(remoteAllow), expectedMode: SSHPortForwardModeOn, }, { @@ -664,45 +720,47 @@ func TestSSHPortForwarding(t *testing.T) { roleSet: NewRoleSet(remoteDeny), expectedMode: SSHPortForwardModeLocal, }, + { + name: "local allow", + roleSet: NewRoleSet(localAllow), + expectedMode: SSHPortForwardModeOn, + }, { name: "local deny", roleSet: NewRoleSet(localDeny), expectedMode: SSHPortForwardModeRemote, }, - { - name: "explicit deny", - roleSet: NewRoleSet(deny), - expectedMode: SSHPortForwardModeOff, - }, { name: "implicit allow", roleSet: NewRoleSet(implicitAllow), expectedMode: SSHPortForwardModeOn, }, { - name: "conflicting roles with remote deny", + name: "conflicting roles: allow all with remote deny", roleSet: NewRoleSet(allow, remoteDeny), - expectedMode: SSHPortForwardModeOn, + expectedMode: SSHPortForwardModeLocal, }, { - name: "conflicting roles with local deny", + name: "conflicting roles: allow all with local deny", roleSet: NewRoleSet(allow, localDeny), - expectedMode: SSHPortForwardModeOn, + expectedMode: SSHPortForwardModeRemote, }, { - name: "conflicting roles with legacy allow", + // 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, }, { - name: "conflicting roles with explicit deny", - roleSet: NewRoleSet(allow, deny), + // 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: SSHPortForwardModeOn, + expectedMode: SSHPortForwardModeOff, }, } diff --git a/lib/services/role.go b/lib/services/role.go index 36309f15b18e4..3b36f121d1329 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), }, @@ -2855,10 +2852,12 @@ func (m SSHPortForwardMode) String() string { } } -// SSHPortForwardMode returns the SSHPortForwardMode permitted by a RoleSet. +// 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 { - // explicit allows and denies both need to be tracked in order to enforce implicit allows properly - var allowRemote, allowLocal bool + var denyRemote, denyLocal, legacyDeny bool + legacyCanDeny := true for _, role := range set { config := role.GetOptions().SSHPortForwarding @@ -2866,36 +2865,50 @@ func (set RoleSet) SSHPortForwardMode() SSHPortForwardMode { if config == nil { // TODO (eriktate): remove legacy check in v20 //nolint:staticcheck // this field is preserved for existing deployments, but shouldn't be used going forward - if types.BoolDefaultTrue(role.GetOptions().PortForwarding) { - return SSHPortForwardModeOn + legacy := role.GetOptions().PortForwarding + if legacy != nil { + if legacy.Value { + return SSHPortForwardModeOn + } + + legacyDeny = true } continue } - if config.Remote == nil || types.BoolDefaultTrue(config.Remote.Enabled) { - allowRemote = true - } + if config.Remote != nil && config.Remote.Enabled != nil { + if !config.Remote.Enabled.Value { + denyRemote = true - if config.Local == nil || types.BoolDefaultTrue(config.Local.Enabled) { - allowLocal = true + } + + // an explicit legacy deny is only possible if no explicit SSHPortForwarding config has been provided + legacyCanDeny = false } - if allowRemote && allowLocal { - return SSHPortForwardModeOn + 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 } } - // enforcing implicit allow and preferring expicit allow over explicit deny + // enforcing implicit allow and preferring allow over explicit deny switch { - case allowRemote && allowLocal: - return SSHPortForwardModeOn - case allowRemote: - return SSHPortForwardModeRemote - case allowLocal: + case denyRemote && denyLocal: + return SSHPortForwardModeOff + case legacyDeny && legacyCanDeny: + return SSHPortForwardModeOff + case denyRemote: return SSHPortForwardModeLocal + case denyLocal: + return SSHPortForwardModeRemote default: - return SSHPortForwardModeOff + return SSHPortForwardModeOn } }