Skip to content

Commit

Permalink
adding more granular port forwarding access controls that allow for (#…
Browse files Browse the repository at this point in the history 
…49417)

specifying within a role whether remote forwarding, local forwarding,
both, or none should be allowed
  • Loading branch information
@eriktate
eriktate authored Dec 10, 2024
1 parent c497ebd commit 5e08f56
Show file tree
Hide file tree
Showing 13 changed files with 487 additions and 56 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 @@ -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()
}
Expand Down
27 changes: 27 additions & 0 deletions lib/auth/grpcserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
3 changes: 3 additions & 0 deletions lib/services/access_checker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
207 changes: 207 additions & 0 deletions lib/services/access_checker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
28 changes: 21 additions & 7 deletions lib/services/presets.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
Expand Down Expand Up @@ -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},
Expand Down
Loading

0 comments on commit 5e08f56

Please sign in to comment.