[bug#29035] [PATCH 1/2] skel: Test for interactive shell instead of $SSH_CLIENT in .bashrc.

[Marius Bakke]

Ludovic Courtès <address@hidden> writes:

> Hi Marius!
>
> Marius Bakke <address@hidden> skribis:
>
>> Ludovic Courtès <address@hidden> writes:
>>
>>> Heya,
>>>
>>> Marius Bakke <address@hidden> skribis:
>>>
>>>> * gnu/system/shadow.scm (default-skeletons): Instead of testing for
>>>> $SSH_CLIENT, check whether '$-' includes the letter 'i'.
>>>
>>> That’s an improvement indeed, LGTM!
>>
>> I realized this will source /etc/profile twice when bash is invoked as
>> 'bash -l -c foo', which isn't great.  It also assumes /etc/profile
>> exists at all, which might not hold true e.g. in a container.
>
> OK.  (Though GuixSD containers do have /etc/profile, don’t they?)

I only checked `guix environment -C` (no further arguments!).

>> The main motivation for this commit is to make things like
>> 'git-receive-pack', 'rsync' etc work out-of-the-box when installed in
>> a user profile.  The test for `cat` was ineffective on OpenSSH since it
>> has a default PATH set to "/run/current-system/profile/bin".
>>
>> I've tested adding ~/.guix-profile/bin to the compiled-in default
>> OpenSSH PATH instead, and it works.  WDYT of this series?
>
> OK.
>
> I think it would make sense to add a test to (gnu tests ssh) for this,
> because it’s one of these things that annoy everyone.

Good idea.  I came up with this:

diff --git a/gnu/tests/ssh.scm b/gnu/tests/ssh.scm
index 41be36035..6d367dc75 100644
--- a/gnu/tests/ssh.scm
+++ b/gnu/tests/ssh.scm
@@ -169,6 +170,33 @@ root with an empty password."
                  (call-with-remote-input-file sftp-session witness
                                               read)))))
 
+          ;; Connect to the guest over SSH.  Make sure we can run commands
+          ;; from the system profile.
+          (test-equal "run executables from system profile"
+            #t
+            (call-with-connected-session/auth
+             (lambda (session)
+               (let ((channel (make-channel session)))
+                 (channel-open-session channel)
+                 (channel-request-exec
+                  channel
+                  (string-append
+                   "mkdir -p /root/.guix-profile/bin && "
+                   "touch /root/.guix-profile/bin/witness && "
+                   "chmod 755 /root/.guix-profile/bin/witness"))
+                 (zero? (channel-get-exit-status channel))))))
+
+          ;; Connect to the guest over SSH.  Make sure we can run commands
+          ;; that only exist in the user profile.
+          (test-equal "run executable from user profile"
+            #t
+            (call-with-connected-session/auth
+             (lambda (session)
+               (let ((channel (make-channel session)))
+                 (channel-open-session channel)
+                 (channel-request-exec channel "witness")
+                 (zero? (channel-get-exit-status channel))))))
+
           (test-end)
           (exit (= (test-runner-fail-count (test-runner-current)) 0)))))
 

It works as expected with both approaches (changing .bashrc, and
changing "--with-default-path").  WDYT?

>> From fc37dd6dfb6beab9cc4e52de7b7c98946125e7cc Mon Sep 17 00:00:00 2001
>> From: Marius Bakke <address@hidden>
>> Date: Sun, 29 Oct 2017 10:31:25 +0100
>> Subject: [PATCH 1/3] gnu: openssh: Add user profiles to the default PATH.
>>
>> * gnu/packages/ssh.scm (openssh)[arguments]<#:configure-flags>: Add
>> '~/guix-profile/bin' to '--with-default-path' arguments.
>> ---
>>  gnu/packages/ssh.scm | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/gnu/packages/ssh.scm b/gnu/packages/ssh.scm
>> index 8317f29cd..2aeeeae1e 100644
>> --- a/gnu/packages/ssh.scm
>> +++ b/gnu/packages/ssh.scm
>> @@ -149,7 +149,9 @@ a server that supports the SSH-2 protocol.")
>>        #:configure-flags  `("--sysconfdir=/etc/ssh"
>>  
>>                             ;; Default value of 'PATH' used by sshd.
>> -                          
>> "--with-default-path=/run/current-system/profile/bin"
>> +                           ,(string-append "--with-default-path="
>> +                                           
>> "/run/current-system/profile/bin:"
>> +                                           "~/.guix-profile/bin")
>
> If sshd performs tilde expansion, that’s fine with me.

Unfortunately, I think the tilde is expanded by the shell, and this made
me look up how POSIX handles tilde in PATH.  It appears bash, when
invoked with '--posix', does *not* perform tilde expansion if it appears
as the first character in a PATH element:

<https://www.gnu.org/software/bash/manual/html_node/Bash-POSIX-Mode.html>
(note 16)

So while it works with "bash" as the login shell, unexpected results may
occur with others.  It doesn't seem impossible to patch OpenSSH to
perform this expansion, though:

<https://github.com/openssh/openssh-portable/blob/b7548b12a6b2b4abf4d057192c353147e0abba08/session.c#L998>
(_PATH_STDPATH is the --with-default-path)

> Should we do something similar for lsh and Dropbear?

Probably.  Since we have a system test, it's easy to experiment with.
For now I think this .bashrc workaround might be the easiest approach,
which makes the above test pass for both OpenSSH and Dropbear:

From 6f4dfbea9cd92a3b03d7e1db89c75a88f4495ba5 Mon Sep 17 00:00:00 2001
From: Marius Bakke <address@hidden>
Date: Sun, 29 Oct 2017 21:02:19 +0100
Subject: [PATCH] system: Test for interactive shell instead of `cat` in
 skeleton '.bashrc'.

* gnu/system/shadow.scm (default-skeletons)[bashrc]: Wrap $SSH_CLIENT test in
a conditional testing for interactive shell.
---
 gnu/system/shadow.scm | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 236807c70..58613e620 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -157,13 +157,12 @@ if [ -f ~/.bashrc ]; then . ~/.bashrc; fi\n"))
 # honor it and otherwise use /bin/sh.
 export SHELL
 
-if [ -n \"$SSH_CLIENT\" -a -z \"`type -P cat`\" ]
+if [[ $- != *i* ]]
 then
-    # We are being invoked from a non-interactive SSH session
-    # (as in \"ssh host command\") but 'cat' cannot be found
-    # in $PATH.  Source /etc/profile so we get $PATH and other
-    # essential variables.
-    source /etc/profile
+    # We are being invoked from a non-interactive shell.  If this
+    # is an SSH session (as in \"ssh host command\"), source
+    # /etc/profile so we get PATH and other essential variables.
+    [[ -n \"$SSH_CLIENT\" ]] && source /etc/profile
 fi
 
 # Adjust the prompt depending on whether we're in 'guix environment'.
-- 
2.14.3

Thanks for the fast replies, and sorry for the round-trip!

signature.asc
Description: PGP signature