Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adds per-node ability to disable ssh TCP forwarding #6989

Merged
merged 24 commits into from
Jun 17, 2021
Merged

Adds per-node ability to disable ssh TCP forwarding #6989

merged 24 commits into from
Jun 17, 2021

Conversation

tcsc
Copy link
Contributor

@tcsc tcsc commented May 21, 2021 •
edited
Loading

Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds

  • the allow_tcp_forwarding key to the yaml SSH config block, with a boolean value
  • Plumbing to pipe the resulting config value through to the SSH server
  • A predicate check in the SSH server to [dis]allow port forwarding based on the setting.

This change also

  • adds a common way for integration tests to await the establishment of an SSH session
  • refactors several integration tests to use this new method rather than manually waiting
  • adds some marshaling code to move errors from spawned goroutines back into the main test routine in verifySessionJoin()

See-Also: Issue #6783

@tcsc
Copy link
Contributor Author

tcsc commented May 21, 2021 •
edited
Loading

Gah. I've left a heap of debugging rot in there... deleting
Edit: Done

@tcsc tcsc force-pushed the trent/disable-port-forwarding branch 2 times, most recently from 264969c to 5a20685 Compare May 21, 2021 11:51
r0mant
r0mant reviewed May 26, 2021
View reviewed changes
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
integration/integration_test.go Outdated Show resolved Hide resolved
lib/config/fileconf.go Outdated Show resolved Hide resolved
lib/config/fileconf.go Outdated Show resolved Hide resolved
lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
@russjones
Copy link
Contributor

@tcsc I'll review this later today.

@tcsc tcsc requested a review from r0mant May 28, 2021 03:11
@tcsc tcsc force-pushed the trent/disable-port-forwarding branch from 3788204 to 2afce36 Compare May 28, 2021 03:12
nklaassen
nklaassen approved these changes Jun 1, 2021
View reviewed changes
integration/port_forwarding_test.go Outdated Show resolved Hide resolved
r0mant
r0mant approved these changes Jun 2, 2021
View reviewed changes
integration/integration_test.go Outdated Show resolved Hide resolved
integration/port_forwarding_test.go Outdated Show resolved Hide resolved
@russjones russjones added the ux label Jun 2, 2021
tcsc added 9 commits June 9, 2021 12:12
@tcsc
Adds per-node ability to disable ssh tcp forwarding
58afbb7 
Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds
 * the `allow_tcp_forwarding` key to the yaml SSH config block, with values
   compatable with the equivalent setting for OpenSSH `sshd`, i.e.
   "yes", "no", "all" and "local"
 * Plumbing to pipe the resulting config value through to the SSH server
 * A predicate check in the SSH server to [dis]allow port forwarding
   based on the setting.

See-Also: Issue #6783
@tcsc
Remove debugging crud
7ef782a
@tcsc
Adds integration test
4d2324a
@tcsc
stash
9cd76fe
@tcsc
Apply review feedback
458d3cb
@tcsc
Fix Audit test
4a3a0fa
@tcsc
Review feedback
a3d56d6
@tcsc
Rebase cleanup
c979106
@tcsc
Doc update
9d3cf29
@tcsc tcsc force-pushed the trent/disable-port-forwarding branch from 164cf7b to 9d3cf29 Compare June 9, 2021 04:55
@tcsc tcsc requested a review from inertial-frame as a code owner June 9, 2021 04:55
inertial-frame
inertial-frame approved these changes Jun 9, 2021
View reviewed changes
Copy link
Contributor

@inertial-frame inertial-frame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs sign-off. Thanks for the update!

@russjones
Copy link
Contributor

russjones commented Jun 10, 2021 •
edited
Loading

@tcsc What do all and local mean here?

all seems to mean yes, but I don't think we use that terminology anywhere else right?

To be honest, I don't understand what local means. I read man sshd_config and still don't get it.

@tcsc
Copy link
Contributor Author

tcsc commented Jun 10, 2021

@tcsc What do all and local mean here? all seems to mean yes, but I don't think we use that terminology anywhere else right?

For our purposes all, local and yes all mean the same thing.

The local option is included for compatibility with the OpenSSH sshd config, where it will enable local (i.e. client to internet), but not remote (i.e. Internet to client) forwarding. As far as I can tell, we don't offer remote forwarding, so for Teleport it becomes a simple yes.

tcsc
tcsc commented Jun 13, 2021
View reviewed changes
Copy link
Contributor Author

@tcsc tcsc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collapsed all options back down to a boolean

lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
klizhentas
klizhentas requested changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@klizhentas klizhentas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team, sorry for being late to review.

A quick question before we proceed.

Here is the existing port forwarding option in RBAC:

    # port_forwarding controls whether TCP port forwarding is allowed
    port_forwarding: true

The name allow_tcp_forwarding is slightly different, but has the same value? If that's the case, let's make the names identical - port_forwarding: true.

@tcsc
Copy link
Contributor Author

tcsc commented Jun 16, 2021 •
edited
Loading

The name was chosen to match the corresponding OpenSSH option, but if we are already using port_forwarding for the same idea in another context then we should stick with that. Will change.

The value parser uses apiutils.ParseBool() behind the scenes, so it will accept true, false, yes, no, etc to mirror the other boolean options that can have various ways of spelling true.

I've pulled it back to a simple true/false rather than add the extra YAML unmarshaller.

@tcsc tcsc requested a review from klizhentas June 16, 2021 01:15
russjones
russjones approved these changes Jun 16, 2021
View reviewed changes
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me after the requested changes are made. Get final UX sign-off from @klizhentas.

lib/srv/regular/sshserver.go Outdated Show resolved Hide resolved
lib/config/fileconf_test.go Show resolved Hide resolved
@tcsc tcsc enabled auto-merge (squash) June 17, 2021 01:04
@tcsc tcsc merged commit 52fb813 into master Jun 17, 2021
@tcsc tcsc deleted the trent/disable-port-forwarding branch June 17, 2021 01:17
@russjones russjones linked an issue Jun 17, 2021 that may be closed by this pull request
Add support for AllowTcpForwarding to hosts. #6783
Closed
tcsc added a commit that referenced this pull request Jun 18, 2021
@tcsc
Adds per-node ability to disable ssh TCP forwarding (#6989)
1ab7674 
Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds:
  * the`port_forwarding` key to the yaml SSH config block, with a boolean value
  * Plumbing to pipe the resulting config value through to the SSH server
  * A predicate check in the SSH server to [dis]allow port forwarding based on the setting.

This change also:
    * adds a common way for integration tests to await the establishment of an SSH session
    * refactors several integration tests to use this new method rather than manually waiting
    * adds some marshaling code to move errors from spawned goroutines back into the
      main test routine in verifySessionJoin()

See-Also: Issue #6783
@tcsc tcsc mentioned this pull request Jun 18, 2021
Merged
tcsc added a commit that referenced this pull request Jun 18, 2021
@tcsc
Adds per-node ability to disable ssh TCP forwarding (#6989)
33c6160 
Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds:
  * the`port_forwarding` key to the yaml SSH config block, with a boolean value
  * Plumbing to pipe the resulting config value through to the SSH server
  * A predicate check in the SSH server to [dis]allow port forwarding based on the setting.

This change also:
    * adds a common way for integration tests to await the establishment of an SSH session
    * refactors several integration tests to use this new method rather than manually waiting
    * adds some marshaling code to move errors from spawned goroutines back into the
      main test routine in verifySessionJoin()

See-Also: Issue #6783
@tcsc tcsc mentioned this pull request Jun 18, 2021
Merged
tcsc added a commit that referenced this pull request Jun 18, 2021
@tcsc
Adds per-node ability to disable ssh TCP forwarding (#6989) (#7354)
ee7fb99 
Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds:
  * the`port_forwarding` key to the yaml SSH config block, with a boolean value
  * Plumbing to pipe the resulting config value through to the SSH server
  * A predicate check in the SSH server to [dis]allow port forwarding based on the setting.

This change also:
    * adds a common way for integration tests to await the establishment of an SSH session
    * refactors several integration tests to use this new method rather than manually waiting
    * adds some marshaling code to move errors from spawned goroutines back into the
      main test routine in verifySessionJoin()

See-Also: Issue #6783
tcsc added a commit that referenced this pull request Jun 18, 2021
@tcsc
Adds per-node ability to disable ssh TCP forwarding (#6989) (#7355)
dcca6ac 
Prior to this change, TCP forwarding over SSH could only be disallowed
by user-based rules, rather than by individual target nodes.

This change adds:
  * the`port_forwarding` key to the yaml SSH config block, with a boolean value
  * Plumbing to pipe the resulting config value through to the SSH server
  * A predicate check in the SSH server to [dis]allow port forwarding based on the setting.

This change also:
    * adds a common way for integration tests to await the establishment of an SSH session
    * refactors several integration tests to use this new method rather than manually waiting
    * adds some marshaling code to move errors from spawned goroutines back into the
      main test routine in verifySessionJoin()

See-Also: Issue #6783
@tcsc tcsc mentioned this pull request Jun 21, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@klizhentas klizhentas klizhentas approved these changes

@r0mant r0mant r0mant approved these changes

@russjones russjones russjones approved these changes

@nklaassen nklaassen nklaassen approved these changes

@inertial-frame inertial-frame inertial-frame approved these changes

Assignees

@r0mant r0mant

@nklaassen nklaassen

Labels
ux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for AllowTcpForwarding to hosts.
6 participants
@tcsc @russjones @klizhentas @r0mant @nklaassen @inertial-frame