Skip to content
New issue

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

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

Already on GitHub? # to your account

remove symlink usage for tsh profile #4347

Merged
merged 1 commit into from
Sep 29, 2020
Merged

Conversation

fspmarshall
Copy link
Contributor

@fspmarshall fspmarshall commented Sep 18, 2020

Changes how tsh tracks the currently active profile. Previously, the currently active profile was tracked via a symlink at ~/.tsh/profile which linked directly to the file. Now, tsh stores the name of the currently active profile in a regular text file at ~/.tsh/current-profile.

Fixes #4095

(bug was reproduced and fix was verified on Windows Server 2019)

@fspmarshall fspmarshall force-pushed the fspmarshall/no-symlink-tsh branch from 990b480 to 1c87f3d Compare September 18, 2020 21:54
@fspmarshall fspmarshall marked this pull request as ready for review September 18, 2020 21:54
@benarent
Copy link
Contributor

@fspmarshall if you can easily make a binary I can try it on my system.

@benarent
Copy link
Contributor

benarent commented Sep 19, 2020

I tested the binary this morning. I noticed that It wasn't working as I had installed my binary to Windows and my user was picking this file.

PS C:\Users\benar\.tsh> (get-command tsh).Path
C:\Windows\system32\tsh.exe

I ran across #3012 again, and this would be good to go out with this release for locked down windows environments.

Example of working as intended.

PS C:\Users\benar\.tsh> ls


    Directory: C:\Users\benar\.tsh


Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d-----         9/19/2020   9:31 AM                keys
-a----         9/19/2020   9:40 AM             34 current-profile
-a----         9/19/2020   9:40 AM            804 known_hosts
-a----         6/24/2020  12:21 PM            138 teleport-ent-4-3.practice.io.yaml
-a----         9/19/2020   9:40 AM            175 teleport-pagerduty.asteroid.earth.yaml

@benarent benarent added the c-ju Internal Customer Reference label Sep 21, 2020
@benarent
Copy link
Contributor

Just to clarify, this did work on my machine. I was picked an earlier version to start.

lib/client/profile.go Outdated Show resolved Hide resolved
@fspmarshall fspmarshall force-pushed the fspmarshall/no-symlink-tsh branch from e94d5f5 to 155ca6a Compare September 22, 2020 17:24
lib/client/profile.go Outdated Show resolved Hide resolved
lib/client/profile.go Outdated Show resolved Hide resolved
@fspmarshall fspmarshall force-pushed the fspmarshall/no-symlink-tsh branch from 155ca6a to d444876 Compare September 22, 2020 22:09
@russjones
Copy link
Contributor

russjones commented Sep 23, 2020

What do you think about the following: we continue to write the symlink as well as the new file that has the profile in it. We create a new package specific file to read in the current profile, for example:

symlink_windows.go
symlink_unix.go

Then macOS/Linux can continue to use the symlink and Windows can use the new format. This will reduce the blast radius for regressions to only Windows because the macOS/Linux logic will not change nor any other tooling customers may have developed around the symlink.

Once we have some breathing room, we can add test coverage and migrate people over to the new approach across all platforms with a flag that allows them to revert logic to the old symlink like --use-symlink.

@awly @fspmarshall The main thing that worries me here is regressions in macOS/Linux tsh clients. People upgrade tsh frequently through things like Homebrew which means this change can have a large negative impact. I know creating platform specific files will increase code duplication, but that's a trade off I feel is worth taking for increased stability.

What do you guys think?

@awly
Copy link
Contributor

awly commented Sep 23, 2020

@russjones I think having different profile structure on different platforms will cause problems for Windows users down the line.
We will make changes with the assumption of symlinks and test them on Linux/Mac, but forget about the special Windows behavior.

Any regressions are likely to happen in the migration code (since we'll exercise the new profile structure regularly). Worst case, the user can delete ~/.tsh to fix a botched migration.

My vote is for keeping this PR as is.

@fspmarshall
Copy link
Contributor Author

I agree with @awly. Platform-dependent systems seem likely to create more problems than they solve.

@webvictim
Copy link
Contributor

webvictim commented Sep 23, 2020

+1 from me for having consistent behaviour across all platforms too. Our Windows experience is already subpar - I don't think it's a good idea for us to deviate any further from the critical path with mitigations.

I agree that the worst-case scenario of deleting the ~/.tsh directory is acceptable. Maybe we could add some detail to the error output to advise users to do that if they're experiencing problems?

This doesn't cover the case where people might have built automation around the symlink, but I guess we can make sure we add something explicit in the release notes about how tsh won't be using a symlink any more so that people are aware. Any changes should be relatively minor.

@benarent benarent added this to the 4.4 "Rome" milestone Sep 24, 2020
@fspmarshall fspmarshall merged commit b2faac1 into master Sep 29, 2020
@fspmarshall fspmarshall deleted the fspmarshall/no-symlink-tsh branch September 29, 2020 17:15
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
c-ju Internal Customer Reference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non-admin windows users error with tsh login
5 participants