SSHDriver: implement user switching via su #1220
Conversation
There was a problem hiding this comment.
The commit message misses su_prompt.
The configuration docs need to be updated and the use case should be described there as well. Maybe something like: Targets might not allow direct SSH access for certain users (such as root), the SSHDriver can log in as another user and use su.
There was a problem hiding this comment.
The commit message misses
su_prompt.The configuration docs need to be updated and the use case should be described there as well. Maybe something like: Targets might not allow direct SSH access for certain users (such as root), the SSHDriver can log in as another user and use
su.
These points were not addressed, yet.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #1220 +/- ##
========================================
- Coverage 62.9% 62.9% -0.1%
========================================
Files 161 160 -1
Lines 11861 11897 +36
========================================
+ Hits 7470 7485 +15
- Misses 4391 4412 +21
☔ View full report in Codecov by Sentry. |
| comout, comerr = sub.communicate(timeout=timeout) | ||
| stdout += comout | ||
| if not self.stderr_merge: | ||
| stderr += comerr |
There was a problem hiding this comment.
Why isn't this simply..
stdout, stderr = sub.communicate(timeout=timeout)..as it was before?
There was a problem hiding this comment.
Because in case we are manging the password, we wan't to prepend the additional bytes we might have read after password input.
There was a problem hiding this comment.
I'm still having problems understanding this:
Why is the comout variable needed? Couldn't this be stdout directly?
There was a problem hiding this comment.
If we use stdout directly, the previous contents will be overwritten if we read additional bytes after password input.
There was a problem hiding this comment.
How is..
stdout = b""
comout, comerr = sub.communicate(timeout=timeout)
stdout += comout..any different than..
stdout, comerr = sub.communicate(timeout=timeout)..? What am I missing here?
Add two new attributes to the driver which will use su to switch to a user to run a command. The su_password is required for this feature to be used, su_username only needs to be set if another user than root should be switched to. Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de> Co-developed-by: Jan Luebbe <jlu@pengutronix.de>
This avoids potential SSH log messages from ending up in the command output, especially when using stderr_merge. Signed-off-by: Jan Luebbe <jlu@pengutronix.de> Signed-off-by: Rouven Czerwinski <r.czerwinski@pengutronix.de>
Emantor
force-pushed
the
topic/ssh_su
branch
from
a2b1b14
to
6742131
Compare
| comout, comerr = sub.communicate(timeout=timeout) | ||
| stdout += comout | ||
| if not self.stderr_merge: | ||
| stderr += comerr |
There was a problem hiding this comment.
I'm still having problems understanding this:
Why is the comout variable needed? Couldn't this be stdout directly?
| output = self.handle_password(sub.stdout, sub.stdin, marker) | ||
| sub.stdin.close() | ||
| self.logger.debug("su leftover output: %s", output) | ||
| stderr += output |
There was a problem hiding this comment.
Why is this appended to stderr? Why does this have to be error output?
| self.logger.debug("Sending command: %s", complete_cmd) | ||
| if self.stderr_merge: | ||
| stderr_pipe = subprocess.STDOUT | ||
| else: | ||
| stderr_pipe = subprocess.PIPE | ||
| stdin = subprocess.PIPE if self.su_password else None | ||
| stdout, stderr = b"", b"" |
There was a problem hiding this comment.
This won't work with..
if stderr is None:..below.
Description
Implement user switching for the SSHDriver via su.
Checklist