session: don't spawn cockpit-bridge with the shell #14375
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
martinpitt
previously requested changes
Jul 20, 2020
There was a problem hiding this comment.
Thanks! Are the session and kerberos changes really related? It almost seems like the last two commit (which could be squashed -- code changes and test) could be its own PR, plus PR #14372 as a prerequisite?
allisonkarlitskaya
force-pushed
the
new-tcsh
branch
from
76a2f8d
to
4c9baec
Compare
Replace our session() and fork_session() functions with a pure "spawn" utility function which handles all of the things we need it to (fd remapping, changing uid/gid, etc). This mostly removes all of the unsafe things we were doing after fork() but before exec(). It also allows a cleanup of a global variable. The biggest remaining TODO is to replace fdwalk with something based only on "safe" functions.
Spawning cockpit-bridge under the shell has caused quite a lot of annoying problems over time, mostly to do with file descriptors: - the user's shell rc scripts can write messages to stdout, which would be interpreted as cockpit frames - some shells (csh) don't support the redirection syntax we've been trying to use to solve the above problem - some shells (tcsh) seem to close all fds >= 3 - some shells (tlog-rec-session) open a new pseudoterminal, running the session command with stdin/out/err attached to it, and writing the results to stdout, regardless of which fds they were written on The above problems make it pretty difficult to come up with a mechanism to get a clean fd through to cockpit-bridge which we can reliably speak cockpit protocol on. Let's give up on trying. Instead, we use our fancy new spawn helper to spawn the user's shell with a simple command: 'exit 71'. We use this randomly-chosen number to check if the given shell is a valid login shell or not. If the shell exits with the expected status, we allow the login to continue, spawning cockpit-bridge directly. Otherwise, we give a permissions error. Closes cockpit-project#14375
It should now be possible to log in with tcsh set as the user's shell.
allisonkarlitskaya
force-pushed
the
new-tcsh
branch
from
4c9baec
to
e87f31a
Compare
allisonkarlitskaya
dismissed
martinpitt’s stale review
Most comments related to krb changes (now already merged under separate PR). Reviewer is somewhere far away riding his bike. :)
mvollmer
reviewed
Aug 10, 2020
| * from ~/.profile and friends to interfere with the protocol; route shell's | ||
| * stdout to its stderr, so that we can still see it in the logs */ | ||
| const int remap_fds[] = { -1, 2, -1, 1, login_messages_fd }; | ||
| const int remap_fds[] = { -1, -1, -1, login_messages_fd }; |
There was a problem hiding this comment.
This needs a comment re what it does, I'd say.
There was a problem hiding this comment.
Or maybe not, fdremap is actually much simpler than I thought.
mvollmer
approved these changes
Aug 10, 2020
| m.execute("sed -r -i.bak '/^admin:/ s_:[^:]+$_:/bin/tcsh_' /etc/passwd") | ||
| b.login_and_go() | ||
| b.logout() | ||
| m.execute("mv /etc/passwd.bak /etc/passwd") |
There was a problem hiding this comment.
Ah, this is tricky. This is a @nondestructive test, so it needs to restore /etc/passwd also on failure, but it actually does since all non-destructive tests explicitly restore /etc/passwd, as one of a number of special cases. So this is exactly correct, and I guess I am just not enough used to non-destructive tests...
allisonkarlitskaya
added a commit
that referenced
this pull request
Aug 11, 2020
Spawning cockpit-bridge under the shell has caused quite a lot of annoying problems over time, mostly to do with file descriptors: - the user's shell rc scripts can write messages to stdout, which would be interpreted as cockpit frames - some shells (csh) don't support the redirection syntax we've been trying to use to solve the above problem - some shells (tcsh) seem to close all fds >= 3 - some shells (tlog-rec-session) open a new pseudoterminal, running the session command with stdin/out/err attached to it, and writing the results to stdout, regardless of which fds they were written on The above problems make it pretty difficult to come up with a mechanism to get a clean fd through to cockpit-bridge which we can reliably speak cockpit protocol on. Let's give up on trying. Instead, we use our fancy new spawn helper to spawn the user's shell with a simple command: 'exit 71'. We use this randomly-chosen number to check if the given shell is a valid login shell or not. If the shell exits with the expected status, we allow the login to continue, spawning cockpit-bridge directly. Otherwise, we give a permissions error. Closes #14375
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.
Spawning cockpit-bridge under the shell has caused quite a lot of
annoying problems over time, mostly to do with file descriptors:
the user's shell rc scripts can write messages to stdout, which would
be interpreted as cockpit frames
some shells (csh) don't support the redirection syntax we've been
trying to use to solve the above problem
some shells (tcsh) seem to close all fds >= 3
some shells (tlog-rec-session) open a new pseudoterminal, running the
session command with stdin/out/err attached to it, and writing the
results to stdout, regardless of which fds they were written on
The above problems make it pretty difficult to come up with a mechanism
to get a clean fd through to cockpit-bridge which we can reliably speak
cockpit protocol on. Let's give up on trying.
Instead, we use our fancy new spawn helper to spawn the user's shell
with a simple command: 'exit 71'. We use this randomly-chosen number to
check if the given shell is a valid login shell or not. If the shell
exits with the expected status, we allow the login to continue, spawning
cockpit-bridge directly. Otherwise, we give a permissions error.