Adding granular SSH port forwarding controls

[eriktate]

This adds the implementation for the new SSHPortForwarding configuration added to the role proto here. It's meant to maintain existing default behaviors while allowing for more fine grained control over what types of SSH port forwarding a role should grant access to. The commits are broken up somewhat intentionally to make reviewing user.

Expected cases can be seen more specifically in the test coverage itself, but at a high level the assumptions are:

• Default role behavior should still be implicit allow with explicit deny.
• If an ssh_port_forwarding configuration is provided, the port_forwarding bool on the same role is effectively ignored.
• An explicit deny from any role in a RoleSet takes precedence over everything else. This is different than current default behavior and there's a question about this below.
• An explicit deny from a port_forwarding field that does not have a sibling ssh_port_forwarding configuration denies both local and remote forwarding.
• Explicit allows are accepted but are effectively no-ops.

I'll be opening a follow up PR for maintaining backwards compatibility with older teleport agents that don't know about the new ssh_port_forwarding config.

Questions

• Current default behavior is that any occurrence of port_forwarding: true results in port forwarding being allowed. Do we want to maintain that going forward? It seems the opposite of what I would expect. So much so that I didn't even notice it at first.
  • Talked to Tim and we agreed we should stick as close to the existing behavior as possible to avoid complicating things for existing installations.
• If we keep the default behavior above, should we adjust the default value of port_forwarding? In order to get granular controls to work, you would have to explicitly set port_forwarding: false.
  • This actually wasn't true either way because we can just ignore the value of port_forwarding if there's an explicit ssh_port_forwarding field configured.
• Should the port forward mode be included in cert extensions?

changelog: Added more granular access controls for SSH port forwarding. Access to remote or local port forwarding can now be controlled individually using the new ssh_port_forwarding role option. TODO: include link to future docs

[nklaassen]

suggestion:

• only use the legacy behavior if no role in the entire roleset uses the new options
• if using the new behavior, a single explicit legacy deny is a total deny
• a single explicit local or remote deny in the new option should result in that local/remote type being denied

something like this:

var (
	legacyDeny bool
	legacyAllow
	denyLocal
	denyRemote
	usingNewOptions
)
for _, role := range set {
	legacyDeny |= !types.BoolDefaultTrue(role.GetOptions().PortForwarding)
	legacyAllow |= types.BoolDefaultTrue(role.GetOptions().PortForwarding)	
	config := role.GetOptions().SSHPortForwarding
	if config == nil {
		continue
	}
	usingNewOptions = true
	if config.Remote != nil {
		denyRemote |= !types.BoolDefaultTrue(config.Remote.Enabled)
	}
	if config.Local != nil {
		denyLocal |= !types.BoolDefaultTrue(config.Local.Enabled)
	}
}
if !usingNewOptions {
	// Preserve legacy behavior, a single allow or unset option allows portforwarding
	if legacyAllow {
		return SSHPortForwardModeOn
	}
	return SSHPortForwardModeOff
}
switch {
case legacyDeny || denyLocal && denyRemote:
	return SSHPortForwardModeOff
case denyLocal:
	return SSHPortForwardModeRemote
case denyRemote:
	return SSHPortForwardModeLocal
default:
	return SSHPortForwardModeOn
}

[eriktate]

• only use the legacy behavior if no role in the entire roleset uses the new options
• if using the new behavior, a single explicit legacy deny is a total deny
• a single explicit local or remote deny in the new option should result in that local/remote type being denied

That's actually what I started with and I agree that would make more sense if this were greenfield. Especially since I think that's more in line with how we handle similar cases. That's the opposite of how port_forwarding works, though and it seems like a potential land mine if adding ssh_port_forwarding to a single role effectively invalidates other roles that appear like they should allow forwarding.

I'm not necessarily against prioritizing explicit denies but the backwards compatibility concerns and potential breakage are things we'll want to weigh against

[nklaassen]

That's the opposite of how port_forwarding works, though

Good, the way it currently works is terrible, I see this as an opportunity for us to rectify our past mistakes

it seems like a potential land mine if adding ssh_port_forwarding to a single role effectively invalidates other roles that appear like they should allow forwarding

imo it's a far bigger landmine if you can have an existing role with ssh_port_forwarding: {server: false}, and then adding another role that doesn't mention port forwarding at all suddenly allows server port forwarding

[nklaassen]

But yes we should definitely avoid breaking changes, that's why I think using the new field should be an opt-in to the new behavior, nothing will change until people start using this

Looks like you were discussing this with @rosstimothy, I'd be happy to chat about this with both of you, i feel fairly strongly we should take this opportunity to avoid the current state where roles that don't even mention port forwarding can completely override apparent "denies" in other roles

I am open to dropping my second bullet though "if using the new field, a single explicit legacy deny is a total deny", don't really care about this as long as behavior with roles that use the new field or nothing is sane

[eriktate]

@nklaassen what if we biased towards respecting the legacy behavior while preferring explicit denies in the new ssh_port_forwarding config? I just pushed a commit that should represent what I mean exactly, but the most important points are:

• port_forwarding can be left undefined now rather than always defaulting to true.
• If a role has an ssh_port_forwarding configured, the port_forwarding field on the same role is ignored.
• If a role has port_forwarding: true defined with no ssh_port_forwarding config, that's an immediate allow all which is what we would expect to happen today.
• If no roles are using the legacy port_forwarding field, ssh_port_forwarding biases towards explicit denies.

I think this maintains full backwards compatibility with existing behavior without forcing ssh_port_forwarding to follow the same semantics. It also makes it possible for users to opt fully into the new model as long as they remove port_forwarding from all of their existing roles.

[nklaassen]

If a role has an ssh_port_forwarding configured, the port_forwarding field on the same role is ignored.

Can we make it an error to create or update a role with both set?

[eriktate]

• only use the legacy behavior if no role in the entire roleset uses the new options
• if using the new behavior, a single explicit legacy deny is a total deny
• a single explicit local or remote deny in the new option should result in that local/remote type being denied

That's actually what I started with and I agree that would make more sense if this were greenfield. Especially since I think that's more in line with how we handle similar cases. That's the opposite of how port_forwarding works, though and it seems like a potential land mine if adding ssh_port_forwarding to a single role effectively invalidates other roles that appear like they should allow forwarding.

I'm not necessarily against prioritizing explicit denies but the backwards compatibility concerns and potential breakage are things we'll want to weigh against

[eriktate]

From my perspective, there's two primary questions we need to answer:

• How do we bias? Does a single explicit allow grant access? Or does a single explicit deny block access?
• How should ssh_port_forwarding interact with port_forwarding?

For the second bullet, I think the possible options are:

• Ignore port_forwarding when ssh_port_forwarding is defined for the same role.
• Ignore ssh_port_forwarding when port_forwarding is defined on the same role.
• Ignore port_forwarding when any role in the RoleSet has ssh_port_forwarding defined.
• Ignore ssh_port_forwarding when any role in the RoleSet has port_forwarding defined.

I would also suggest we try to avoid changing the semantics of the legacy port_forwarding field as much as possible. Meaning that even if we decided to bias towards deny for ssh_port_forwarding, I don't think we should do the same for port_forwarding. If we can agree on that, then I think the remaining options are:

1. When ssh_port_forwarding is defined, enabling a mode immediately grants access while disabling a mode only denies if no other roles contradict. Otherwise when port_forwarding is defined it behaves exactly as it does today.
2. When ssh_port_forwarding is defined, disabling a mode immediately denies access. Otherwise when port_forwarding is defined, enabling or disabling is only considered if no other roles contradict.
3. When port_forwarding is defined it behaves exactly as it does today. Otherwise if ssh_port_forwarding is defined, enabling a mode immediately grants access while disabling only denies access if no other roles contradict.
4. When port_forwarding is defined, enabling or disabling is only considered if no other roles contradict. Otherwise if ssh_port_forwarding is defined, disabling a mode immediately denies access while enabling only allows access if no other roles contradict.
5. When ssh_port_forwarding is defined for any role in the RoleSet, enabling a mode immediately grants access while disabling a mode only denies if no other roles contradict. Otherwise port_forwarding is evaluated the same as today.
6. When ssh_port_forwarding is defined for any role in the RoleSet, disabling a mode immediately denies access while enabling a mode only allows if no other roles contradict. Otherwise port_forwarding is evaluated the same as today.
7. When port_forwarding is defined for any role in the RoleSet, it behaves exactly as it does today. Otherwise if ssh_port_forwarding is defined, enabling a mode immediately grants access while disabling a mode only denies if no other roles contradict.
8. When port_forwarding is defined for any role in the RoleSet, it behaves exactly as it does today. Otherwise if ssh_port_forwarding is defined, disabling a mode immediately denies access while enabling a mode only allows if no other roles contradict.

All of these assume implicit allow when neither field is defined, but that's technically not possible right now because we enforce that port_forwarding is defined on every role. Both for new roles or updates to roles that attempt to remove it, port_forwarding ends up being explicitly set to true. This makes integrating ssh_port_forwarding tougher, but it also makes things more confusing as a user since there's no way to remove the ambiguity. After thinking about it some more, I just don't see a good reason to preserve that behavior so the suggested options above assume that it would be possible to leave port_forwarding undefined going forward.

Just to include my two cents on each option:

• I think 3, 7, and 8 are the "safest" in terms of breakage because they prioritize the legacy behavior over the new behavior no matter what.
• 1 preserves the original behavior of being "allow" biased but not automatically deferring to the legacy field.
• 5 is the same as 1 except we'll never consider port_forwarding if an ssh_port_forwarding is present.
• 6 is the biggest change where we shift to being deny biased and any occurrence of ssh_port_forwarding stops considering port_forwarding
• 4 honestly seems sort of pointless, I don't think I would recommend this one.
• 2 might also be sort of pointless because it's effectively the same as 6. The only difference might be that we could maybe deny access if all port_forwarding values were explicitly false but that seems complicated for not much benefit.

@nklaassen @rosstimothy Do you agree with these? Anything I missed that you think should be included?

[rosstimothy]

Should we also be updating the preset roles?

https://github.com/gravitational/teleport/blob/master/lib/services/presets.go#L120
https://github.com/gravitational/teleport/blob/master/lib/services/presets.go#L211

[rosstimothy]

Can we extend regular.TestTCPIPForward, regular.TestDirectTCPIP, forward.TestCheckTCPIPForward, and forward.TestDirectTCPIP to ensure the SSH servers are doing the right thing as well?

[nklaassen]

If a role has an ssh_port_forwarding configured, the port_forwarding field on the same role is ignored.

Can we make it an error to create or update a role with both set?

[nklaassen]

let's say that my user has a single role with

ssh_port_forwarding:
  remote: false

CanPortForward should return false here and the PermitPortForwarding extension in the cert should be unset. That's all good. I'm wondering if an older ssh node that doesn't know about the new role fields will respect that the extension is unset and block port forwarding, or if it ignores the cert and it will just allow port forwarding. Have you tested this out, or can you, and let us know here? Ideally I think we should try to prevent port forwarding on older nodes in that case

[eriktate]

As far as I can tell we don't reference the cert extension anywhere other than to set it. I'm doing another round of manual testing though so I'll double check that this works as expected 👍

[nklaassen]

let us know how the testing goes, you're right I don't think we reference it anywhere, but there's a chance that golang.org/x/crypto/ssh will honor it. But I think if you can port-forward to older nodes with my original example role we should try to find a mitigation (like downgrading the role for those nodes)

[eriktate]

I tested with a v16 agent and it allowed me to port forward even with ssh_port_forwarding set to false across the board, which is what we expected to happen. I've got another PR ready to go behind this one that currently downgrades the role for older agents.

I actually think we can get away with always setting port_forwarding appropriately rather than doing an optional downgrade based on version. Newer agents will ignore values for port_forwarding in favor of ssh_port_forwarding which should meanport_forwarding is free to be leveraged as a way to signal correct behavior to older agents.

[public-teleport-github-review-bot]

@eriktate See the table below for backport results.

Branch	Result
branch/v15	Failed
branch/v16	Failed
branch/v17	Failed