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.

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

Fix auto-update re-exec arguments modified by aliases #50228

Merged
merged 7 commits into from
Jan 17, 2025

Conversation

vapopov
Copy link
Contributor

@vapopov vapopov commented Dec 13, 2024

In this PR fixed issue related tsh {alias} if we define in alias same executable path of tsh, for instance:

~/.tsh/config/config.yaml

aliases:
  "loginvadym": ".build/tsh login --proxy=localhost:8443 --auth=local --user=vadym --insecure"

we don't spawn new process, instead just re-run the tsh main function with modified execution arguments

// if execPath is our path, skip re-execution and run main directly instead.
// this makes for better error messages in case of failures.
if execPath == currentExecPath {
log.Debugf("Self re-exec command: tsh %v.", arguments)
return trace.Wrap(ar.runTshMain(ctx, arguments))
}
cmd := exec.Command(execPath, arguments...)
cmd.Stdin = os.Stdin
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
log.Debugf("Running external command: %v", cmd)
err = ar.runExternalCommand(cmd)
if err == nil {
return nil
}

but in updater we use os.Args arguments, not modified ones by alias runner

if err := syscall.Exec(path, append([]string{path}, os.Args[1:]...), env); err != nil {
return 0, trace.Wrap(err)

this produce issue with updater, when it executed by alias

vpopov@Vadyms-MBP-2 teleport-docker % ./build/tsh loginvadym
Update progress: [▒▒▒▒▒▒▒▒▒▒] (Ctrl-C to cancel update)
ERROR: recursive alias "loginvadym"; correct alias definition and try again

discussion
Related: https://github.com/gravitational/cloud/issues/10053

changelog: Fixed client tools autoupdates executed by aliases (causes recursive alias error)

@vapopov vapopov added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v15 backport/branch/v16 backport/branch/v17 labels Dec 13, 2024
@github-actions github-actions bot added size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Dec 13, 2024
@vapopov vapopov force-pushed the vapopov/fix-autoupdate-re-exec-with-aliases branch from 4c7c3f3 to ba62d79 Compare December 13, 2024 20:18
@vapopov vapopov requested a review from Tener December 18, 2024 19:20
Copy link
Contributor

@Tener Tener left a comment

Choose a reason for hiding this comment

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

LGTM. After reading the issue analysis my first suggestion would be to move away from the global os.Args, so I'm happy to see this is indeed the fix.

Also, very cool feature 👍

Nit: perhaps worth mentioning this bugfix in the changelog? A bit of marketing for both features involved ;-)

@vapopov vapopov removed the no-changelog Indicates that a PR does not require a changelog entry label Dec 19, 2024
@vapopov
Copy link
Contributor Author

vapopov commented Dec 19, 2024

@hugoShaka could you please take a look when you have time

Copy link
Contributor

@hugoShaka hugoShaka left a comment

Choose a reason for hiding this comment

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

Should we have a test covering the re-exec with alias mechanism? I'm worried someone might break it again in the future and won't detect it.

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.
@vapopov vapopov force-pushed the vapopov/fix-autoupdate-re-exec-with-aliases branch from 6ce9b27 to 894aab9 Compare December 25, 2024 18:51
@vapopov vapopov requested a review from hugoShaka December 30, 2024 20:42
@sclevine
Copy link
Member

@hugoShaka reminder to review when you have a chance 🙂

@vapopov vapopov force-pushed the vapopov/fix-autoupdate-re-exec-with-aliases branch 2 times, most recently from 22392b1 to 5156850 Compare January 17, 2025 07:58
@vapopov
Copy link
Contributor Author

vapopov commented Jan 17, 2025

@hugoShaka I had to restore this part since your last review 894aab9#diff-6ddd22bd400fc52c2062de3b1fc775fdaf05594f85c5ffa5a02262af40f5d62dL163-L182

I mislead myself with this check, after verifying rfd again and draft implementation we should keep priority of env variable version set over that we receive from webapi/find endpoint, in such case if I do login with

TELEPORT_TOOLS_VERSION=off tsh login --insecure --proxy localhost:8443 --user teleport

We able to completely disable updates, same as override the version which is defined in cluster. Let say cluster target version is set to 17.1.3 but we want to use 17.1.2 instead

$ TELEPORT_TOOLS_VERSION=17.1.2 tsh login --insecure --proxy localhost:8443 --user teleport
Update progress: [▒▒▒▒▒▒▒▒▒▒] (Ctrl-C to cancel update)
Enter password for Teleport user teleport:
WARNING: You are using insecure connection to Teleport proxy https://localhost:8443
Enter an OTP code from a device:
> Profile URL:        https://localhost:8443
  Logged in as:       teleport
  Cluster:            logrotate.teleport
  Roles:              editor
  Kubernetes:         enabled
  Valid until:        2025-01-17 09:47:45 -0800 PST [valid for 12h0m0s]
  Extensions:         login-ip, permit-agent-forwarding, permit-port-forwarding, permit-pty, private-key-policy

$ tsh --insecure version
Teleport v17.1.2 git:v17.1.2-0-g5274176 go1.23.4
Proxy version: 17.0.0-dev
Proxy: localhost:8443

So, the version is picked from the environment variable in this case. After logging out and re-logging in, the client tools should update to the version defined in the cluster configuration, exmaple:

$ tsh login --insecure --proxy localhost:8443 --user teleport
This is wrapper script
Update progress: [▒▒▒▒▒▒▒▒▒▒] (Ctrl-C to cancel update)
Enter password for Teleport user teleport:
WARNING: You are using insecure connection to Teleport proxy https://localhost:8443
Enter an OTP code from a device:
> Profile URL:        https://localhost:8443
  Logged in as:       teleport
  Cluster:            logrotate.teleport
  Roles:              editor
  Kubernetes:         enabled
  Valid until:        2025-01-17 09:51:37 -0800 PST [valid for 12h0m0s]
  Extensions:         login-ip, permit-agent-forwarding, permit-port-forwarding, permit-pty, private-key-policy

$ tsh --insecure version
Teleport v17.1.3 git:v17.1.3-0-g9726471 go1.23.4
Proxy version: 17.0.0-dev
Proxy: localhost:8443

Another part is related to double re-execution, as we have LocalCheck, which is responsible for checking the TELEPORT_TOOLS_VERSION environment variable, and RemoteCheck, which makes a remote call to the proxy webapi/find endpoint to determine the update version.

LocalCheck is integrated into tsh and tctl before any other initialization, including actual command parsing. If the version environment variable is set and either no version was previously installed or the specified version differs from the installed one, the update and re-execution are triggered, bypassing any further logic.

RemoteCheck executed only for login command or re-login action.

For instance we have /usr/local/bin/tsh initially installed from deb/rpm or tarball, version 1.0.0, previously it was already updated by client tools autoupdate and we have binary ~/.tsh/bin/tsh with version 2.0.0, in this setup we want to re-login to the same cluster where target version was updated to 3.0.0.

When we run a command usr/local/bin/tsh login --proxy example.proxy.com, updater logic verifies that env wasn't set, after that it checks the version by tools directory ~/.tsh/bin/tsh, if we have installed tool there, we re-execute it with same args and envs, ~/.tsh/bin/tsh login --proxy example.proxy.com (first re-execution). In login command we hit RemoteCheck where version is set to 3.0.0 which is different with current one 2.0.0, we run update and re-execution (second re-execution). ~/.tsh/bin/tsh replace itself in the same path with newer version.

Previously we set TELEPORT_TOOLS_VERSION=off to first re-execution, which leads to ignoring RemoteCheck thats why it misleads me with this change 894aab9#diff-6ddd22bd400fc52c2062de3b1fc775fdaf05594f85c5ffa5a02262af40f5d62dL163-L182 , instead I've added check of execution path to allow second re-execution: https://github.com/gravitational/teleport/pull/50228/files#diff-6ddd22bd400fc52c2062de3b1fc775fdaf05594f85c5ffa5a02262af40f5d62dR341-R348

@vapopov vapopov added this pull request to the merge queue Jan 17, 2025
@vapopov vapopov removed this pull request from the merge queue due to a manual request Jan 17, 2025
In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
@vapopov vapopov force-pushed the vapopov/fix-autoupdate-re-exec-with-aliases branch from 5156850 to bb59630 Compare January 17, 2025 09:35
@vapopov vapopov enabled auto-merge January 17, 2025 09:36
@vapopov vapopov added this pull request to the merge queue Jan 17, 2025
Merged via the queue into master with commit fc01a5a Jan 17, 2025
41 checks passed
@vapopov vapopov deleted the vapopov/fix-autoupdate-re-exec-with-aliases branch January 17, 2025 10:14
@public-teleport-github-review-bot

@vapopov See the table below for backport results.

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

vapopov added a commit that referenced this pull request Jan 17, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
vapopov added a commit that referenced this pull request Jan 17, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
vapopov added a commit that referenced this pull request Jan 17, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
vapopov added a commit that referenced this pull request Jan 17, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
github-merge-queue bot pushed a commit that referenced this pull request Jan 18, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
github-merge-queue bot pushed a commit that referenced this pull request Jan 18, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
mvbrock pushed a commit that referenced this pull request Jan 18, 2025
* Fix auto-update re-exec arguments modified by aliases

* Make arguments to be required to set

* Restore progress bar show before request

* Improve integration tests to execute with `tsh` and `tctl`

Added a full-cycle integration test to verify client tools
auto-updates within a test cluster by modifying AutoUpdateConfig
and AutoUpdateVersion resources. The test executes the login
command using alias configurations to ensure no recursive
re-execution occurs.

The updater binary used in integration tests has been replaced
with the `Run` logic of tctl and tsh.

* Set generated test password by env variable instead of constant value

* Restore priority of env check over remote check

In case of double re-execution case we should stop second one to prevent loop re-execution
Drop localDir set during compilation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v15 backport/branch/v16 backport/branch/v17 size/sm tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants