Skip to content

tsh: Implement puttyconfig command to add saved PuTTY sessions to Windows registry#19316

Merged
webvictim merged 19 commits intomasterfrom
gus/tsh-putty
Jul 5, 2023
Merged

tsh: Implement puttyconfig command to add saved PuTTY sessions to Windows registry#19316
webvictim merged 19 commits intomasterfrom
gus/tsh-putty

Conversation

@webvictim
Copy link
Copy Markdown
Contributor

@webvictim webvictim commented Dec 13, 2022

This is an implementation of support for a tsh puttyconfig [user@]host command which will automatically add saved PuTTY sessions to the local Windows registry to make it easy to connect to a named Teleport host.

The added session runs tsh proxy ssh locally to get an authenticated tunnel to the proxy which means PuTTY can neatlly handle auto-relogin and TLS routing etc. The session is configured to authenticate using the ephemeral .ppk file which has been automatically generated and stored in the user's .tsh directory when running tsh login on Windows hosts since v10.0.1.

Every host also has its public host key configured in the registry based on the proxy's hostname, using wildcards when an FQDN is provided or individual hostnames otherwise.

I've tested this on three different clusters (one self-hosted using separate ports, one self-hosted using TLS routing, one Teleport cloud) with a combination of various different users and hostnames and found it to work well in all cases. tsh proxy ssh is doing most of the heavy lifting - this is PR mostly just registry wrangling.

Benefits:

  • Writes to HKEY_CURRENT_USER, so no admin access should be needed
  • Easily scriptable (for HOST in host1 host2 host; do tsh puttyconfig user@$HOST; done)

Caveats:

  • Leaf clusters aren't currently supported Leaf clusters are now supported.
    • I don't think they're prohibitively difficult to add, but this PR has taken far too long to write already
  • Re-running the tsh puttyconfig command will overwrite any changes to any of the ~10 values the command touches
    • any other custom changes like window/font size etc should be preserved, however
  • I originally wanted to implement this as tsh config --putty [user@]host, but unfortunately our fork of Kingpin is too old to allow default arguments/commands, this seems like the next best way
  • There are no tests at the moment, as I'm really not sure where to start... There's no tests for the Windows registry stuff, but the PuTTY hostname/CA logic is tested.

Login and setup:
Screenshot 2023-06-08 at 16 03 58

Session list:
Screenshot 2023-06-08 at 16 04 09

Connected session:
Screenshot 2023-06-08 at 16 04 21

Active session in Teleport:
Screenshot 2023-06-08 at 16 04 48

Registry values added:
Screenshot 2023-06-08 at 16 05 59

Host key settings:
Screenshot 2023-06-08 at 16 06 15

All comments and feedback welcome. I'm curious about what/how I should test and any better ways to handle the registry abstractions.

@webvictim webvictim added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Dec 13, 2022
@webvictim webvictim self-assigned this Dec 13, 2022
@webvictim webvictim requested a review from zmb3 December 13, 2022 04:43
Comment thread tool/tsh/registry_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/registry_windows.go
@webvictim webvictim requested a review from zmb3 December 19, 2022 21:44
@webvictim webvictim requested a review from timothyb89 February 1, 2023 18:08
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
@webvictim webvictim closed this Mar 20, 2023
@webvictim webvictim deleted the gus/tsh-putty branch March 20, 2023 16:06
@webvictim webvictim restored the gus/tsh-putty branch March 20, 2023 17:38
@webvictim webvictim reopened this Mar 20, 2023
@webvictim webvictim requested a review from timothyb89 March 28, 2023 15:09
@webvictim webvictim marked this pull request as ready for review April 3, 2023 19:17
@github-actions github-actions Bot requested a review from jimbishopp April 3, 2023 19:18
@webvictim
Copy link
Copy Markdown
Contributor Author

@zmb3 @timothyb89 This should be ready for review now. Please feel free to add other reviewers or suggest improvements :)

Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
@webvictim
Copy link
Copy Markdown
Contributor Author

Thanks for the initial review @zmb3.

I would like to try and find a way to test the registry methods without requiring the code to run on a Windows box or actually writing to the registry. My best guess is that I could move all the related code to a registry package under utils, then try and find a way to make the registry-calling methods take a parameter that's not a registry.Key, but instead some kind of func or struct that I can mock up and replace with a fake registry.

Any suggestions gratefully received.

Comment thread lib/utils/hostname.go Outdated
@webvictim webvictim requested a review from zmb3 April 4, 2023 16:55
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

A few stylistic comments, nothing major.

Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/hostname.go Outdated
Comment thread lib/utils/registry/registry_windows.go Outdated
Comment thread lib/utils/registry/registry_windows.go Outdated
Comment thread tool/tsh/putty_config_windows.go Outdated
Comment thread tool/tsh/registry_windows.go Outdated
@webvictim webvictim requested a review from zmb3 May 5, 2023 19:12
@webvictim webvictim removed the request for review from jimbishopp June 8, 2023 13:52
@webvictim
Copy link
Copy Markdown
Contributor Author

webvictim commented Jun 28, 2023

Note: this currently contains @rosstimothy's commit 59c5570 which fixes the broken Windows builds. I'll rebase to remove that when #28357 is merged.

Depends on #28357

Edit: Now rebased

return outputHostList
}

var hostnameRegexp = regexp.MustCompile("^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]).)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])$")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was just curious, but the answer was quite educating: https://chat.openai.com/share/0898d082-ba3a-489e-a71c-83e84cbd3657

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was actually really interesting; I'd never thought of using ChatGPT to explain or validate regex. It also gave me ideas for a few more test cases.

Unfortunately...

panic: regexp: Compile(`^(?:(?:(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?)\.){1,126}(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?))$`): error parsing regexp: invalid repeat count: `{1,126}`

Maybe being naive is a good thing?!

Comment thread lib/puttyhosts/puttyhosts.go Outdated
Comment thread lib/puttyhosts/puttyhosts.go Outdated
Comment thread lib/puttyhosts/puttyhosts_test.go Outdated
webvictim and others added 2 commits July 5, 2023 14:34
Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
@webvictim webvictim enabled auto-merge July 5, 2023 17:49
@webvictim webvictim added this pull request to the merge queue Jul 5, 2023
Merged via the queue into master with commit 8abbea6 Jul 5, 2023
@webvictim webvictim deleted the gus/tsh-putty branch July 5, 2023 18:11
ravicious pushed a commit that referenced this pull request Jul 11, 2023
…dows registry (#19316)

* tsh: Implement puttyconfig command to add saved PuTTY sessions to Windows registry

* Addressed comments from code review

* Add support for leaf clusters

* Refactoring from code review

Also moved registry/hostname functions into external packages

* Address more feedback from code review

* Rebase following tsh/common changes

* Fix up putty_config_windows

* Reorder command

* Remove surplus comment

* Use a separate list instead of overloading the 'extra' key

* Address Tim's code review comments

* Address some of Zac's comments

* Refactor formatLocalCommandString to use text/template

* Refactor non-Windows logic into puttyhosts

* Fix subcommand name

* Fix test structure

* Add some more hostnames test cases

* Apply suggestions from code review

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>

* Fix up

---------

Co-authored-by: Zac Bergquist <zac.bergquist@goteleport.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. windows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants