Skip to content

Commit

Permalink
removing default port_forwarding value from default and implicit role…
Browse files Browse the repository at this point in the history 
… definitions and adjusting SSHPortForwarding to prefer denies without breaking existing expectations that legacy port_forwarding prefers explicit allows
  • Loading branch information
@eriktate
eriktate committed Dec 2, 2024
1 parent 819ea87 commit 6c7483d
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 54 deletions.
3 changes: 0 additions & 3 deletions api/types/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
116 changes: 87 additions & 29 deletions lib/services/access_checker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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)
Expand All @@ -603,32 +643,28 @@ 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)},
},
})
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)
Expand All @@ -645,64 +681,86 @@ 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,
},
{
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: "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,
},
}

Expand Down
57 changes: 35 additions & 22 deletions lib/services/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
Expand Down Expand Up @@ -2855,47 +2852,63 @@ 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
// only consider legacy allows when config isn't provided on the same role
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
}
}

Expand Down

0 comments on commit 6c7483d

Please sign in to comment.