Fix sshuttle when using sudo's use_pty option. #712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Typically sshuttle exits by having the main sshuttle client process terminated. This closes file descriptors which the firewall process then sees and uses as a cue to cleanup the firewall rules. The firewall process ignored SIGINT/SIGTERM signals and used setsid() to prevent Ctrl+C from sending signals to the firewall process. This patch makes the firewall process accept SIGINT/SIGTERM signals and then in turn sends a SIGINT signal to the main sshuttle client process which then triggers a regular shutdown as described above. This allows a user to manually send a SIGINT/SIGTERM to either sshuttle process and have it exit gracefully. It also is needed if setsid() fails (known to occur if sudo's use_pty option is used) and then the Ctrl+C SIGINT signal goes to the firewall process. The PID of the sshuttle client process is sent to the firewall process. Using os.getppid() in the firewall process doesn't correctly return the sshuttle client PID.
We previously called setsid() to ensure that the SIGINT generated by Ctrl+C went to the main sshuttle process instead of the firewall process. With the previous commit, we gracefully shutdown if either the sshuttle process or firewall process receives a SIGINT. Therefore, the setsid() call is optional. We still try calling setsid() since the preferred shutdown process involves having the signal go to the main sshuttle process. However, setsid() will fail if the firewall process is started with sudo and sudo is configured with the use_pty option.
If we run sudo with the use_pty option, the firewall process is started in a new pseudoterminal. Other processes that are still printing to the terminal (i.e., the main sshuttle client process, messages from the shuttle server) have their output incorreclty displayed. A newline character simply moves the output to the next line without returning the cursor to the beginning of the line. Simply changing all print commands to use \r\n line endings fixes the problem and does not appear to cause any trouble in other configurations.
brianmay
approved these changes
Jan 9, 2022
|
Looks good to me. Is this ready for merging or does it require more testing? |
|
I'm satisfied that it works on my Ubuntu machine either with or without the sudo use_pty option. I also tried it on at least one other machine. Proceed however you wish. Getting it in the tree would probably be the best way to get more folks to try it. |
|
Agreed. |
Open
skuhl
added a commit
to skuhl/sshuttle
that referenced
this pull request
Jan 4, 2024
This fixes sshuttle#909 and is an alternative to the sshuttle#922 pull request. When sudo's use_pty is used with sshuttle, it causes issues with the terminal. Pull request sshuttle#712 contains some fixes for this problem. However, when sshuttle is run with the --daemon option, it left the user's terminal in a non-sane state. The problem appears to be related to a socketpair that the firewall uses for communication. By setting it up slightly differently (see changes to client.py and firewall.py), the terminal state is no longer disrupted. This commit also changes line endings of the printed messages from \r\n to \n. This undoes a change introduced by pull request sshuttle#712 and is no longer needed.
brianmay
pushed a commit
that referenced
this pull request
Jan 5, 2024
This fixes #909 and is an alternative to the #922 pull request. When sudo's use_pty is used with sshuttle, it causes issues with the terminal. Pull request #712 contains some fixes for this problem. However, when sshuttle is run with the --daemon option, it left the user's terminal in a non-sane state. The problem appears to be related to a socketpair that the firewall uses for communication. By setting it up slightly differently (see changes to client.py and firewall.py), the terminal state is no longer disrupted. This commit also changes line endings of the printed messages from \r\n to \n. This undoes a change introduced by pull request #712 and is no longer needed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request contains a series of small fixes necessary to make sshuttle work with sudo's use_pty option. Recently, Debian has turned this option on by default. This hopefully fixes #664 and debian bug 1003154 without breaking anything else. Testing and feedback is welcome. See comments in the individual commit messages for more detail.