[bug#29035] .bashrc updates

[bug#29035] .bashrc updates

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hello Guix!

These patches changes the default .bashrc so that it always sources
/etc/profile when the shell is non-interactive, and promptly exits after
that.

Marius Bakke (2):
  skel: Test for interactive shell instead of $SSH_CLIENT in .bashrc.
  skel: Return early from .bashrc when the shell is non-interactive.

 gnu/system/shadow.scm | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

-- 
2.14.3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Marius Bakke]

* gnu/system/shadow.scm (default-skeletons): Instead of testing for
$SSH_CLIENT, check whether '$-' includes the letter 'i'.
---
 gnu/system/shadow.scm | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 236807c70..475960b89 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -157,12 +157,11 @@ 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.
+    # (as in \"ssh host command\").  Source /etc/profile so we
+    # get $PATH and other essential variables.
     source /etc/profile
 fi
 
-- 
2.14.3

[bug#29035] [PATCH 2/2] skel: Return early from .bashrc when the shell is non-interactive.

[Marius Bakke]

* gnu/system/shadow.scm (default-skeletons): Add return statement.
---
 gnu/system/shadow.scm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 475960b89..8d8dbc70a 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -163,6 +163,9 @@ then
     # (as in \"ssh host command\").  Source /etc/profile so we
     # get $PATH and other essential variables.
     source /etc/profile
+
+    # Don't do anything else.
+    return
 fi
 
 # Adjust the prompt depending on whether we're in 'guix environment'.
-- 
2.14.3

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

[Ludovic Courtès]

Heya,

Marius Bakke <mbakke@fastmail.com> 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!

Ludo’, who can still learn new things about Bash after all these years.

[bug#29035] [PATCH 2/2] skel: Return early from .bashrc when the shell is non-interactive.

[Ludovic Courtès]

Marius Bakke <mbakke@fastmail.com> skribis:

> * gnu/system/shadow.scm (default-skeletons): Add return statement.
> ---
>  gnu/system/shadow.scm | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
> index 475960b89..8d8dbc70a 100644
> --- a/gnu/system/shadow.scm
> +++ b/gnu/system/shadow.scm
> @@ -163,6 +163,9 @@ then
>      # (as in \"ssh host command\").  Source /etc/profile so we
>      # get $PATH and other essential variables.
>      source /etc/profile
> +
> +    # Don't do anything else.
> +    return
>  fi

LGTM, thanks!

Ludo’.

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

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 885 bytes --]

Ludovic Courtès <ludo@gnu.org> writes:

> Heya,
>
> Marius Bakke <mbakke@fastmail.com> 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.

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: bashrc.patch --]
[-- Type: text/x-patch, Size: 3328 bytes --]

From fc37dd6dfb6beab9cc4e52de7b7c98946125e7cc Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
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")
 
                           ;; configure needs to find krb5-config
                           ,(string-append "--with-kerberos5="
-- 
2.14.3


From cc817dd809c85571d3e54a226ff16a7a3aa5bdae Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Sun, 29 Oct 2017 11:07:49 +0100
Subject: [PATCH 2/3] system: Don't try to source /etc/profile in '.bashrc'
 skeleton.

* gnu/system/shadow.scm (default-skeletons)[bashrc]: Remove test for $SSH_CLIENT.
---
 gnu/system/shadow.scm | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index 236807c70..f6484f85e 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -157,15 +157,6 @@ 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`\" ]
-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
-fi
-
 # Adjust the prompt depending on whether we're in 'guix environment'.
 if [ -n \"$GUIX_ENVIRONMENT\" ]
 then
-- 
2.14.3


From 53761ac682cf0de4161c40aa7ec5372a2070c2b2 Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
Date: Sat, 28 Oct 2017 01:19:01 +0200
Subject: [PATCH 3/3] system: Return early in skeleton '.bashrc' when the shell
 is non-interactive.

* gnu/system/shadow.scm (default-skeletons)[bashrc]: Test if $- includes the
letter 'i', and return if it doesn't.
---
 gnu/system/shadow.scm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gnu/system/shadow.scm b/gnu/system/shadow.scm
index f6484f85e..e1ff3c42d 100644
--- a/gnu/system/shadow.scm
+++ b/gnu/system/shadow.scm
@@ -157,6 +157,12 @@ if [ -f ~/.bashrc ]; then . ~/.bashrc; fi\n"))
 # honor it and otherwise use /bin/sh.
 export SHELL
 
+# If not running interactively, don't do anything else.
+if [[ $- != *i* ]]
+then
+    return
+fi
+
 # Adjust the prompt depending on whether we're in 'guix environment'.
 if [ -n \"$GUIX_ENVIRONMENT\" ]
 then
-- 
2.14.3


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Ludovic Courtès]

Hi Marius!

Marius Bakke <mbakke@fastmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Heya,
>>
>> Marius Bakke <mbakke@fastmail.com> 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?)

> 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.

> From fc37dd6dfb6beab9cc4e52de7b7c98946125e7cc Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> 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.

Should we do something similar for lsh and Dropbear?

> From cc817dd809c85571d3e54a226ff16a7a3aa5bdae Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Sun, 29 Oct 2017 11:07:49 +0100
> Subject: [PATCH 2/3] system: Don't try to source /etc/profile in '.bashrc'
>  skeleton.
>
> * gnu/system/shadow.scm (default-skeletons)[bashrc]: Remove test for $SSH_CLIENT.

OK.

> From 53761ac682cf0de4161c40aa7ec5372a2070c2b2 Mon Sep 17 00:00:00 2001
> From: Marius Bakke <mbakke@fastmail.com>
> Date: Sat, 28 Oct 2017 01:19:01 +0200
> Subject: [PATCH 3/3] system: Return early in skeleton '.bashrc' when the shell
>  is non-interactive.
>
> * gnu/system/shadow.scm (default-skeletons)[bashrc]: Test if $- includes the
> letter 'i', and return if it doesn't.

OK.

Thank you!

Ludo’.

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

[Marius Bakke]

[-- Attachment #1.1: Type: text/plain, Size: 1368 bytes --]

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Marius!
>
> Marius Bakke <mbakke@fastmail.com> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>>
>>> Heya,
>>>
>>> Marius Bakke <mbakke@fastmail.com> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-tests-ssh-Make-sure-we-can-run-commands-from-PATH.patch --]
[-- Type: text/x-patch, Size: 1689 bytes --]

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)))))
 

[-- Attachment #1.3: Type: text/plain, Size: 2274 bytes --]


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 <mbakke@fastmail.com>
>> 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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.4: 0001-system-Test-for-interactive-shell-instead-of-cat-in-.patch --]
[-- Type: text/x-patch, Size: 1428 bytes --]

From 6f4dfbea9cd92a3b03d7e1db89c75a88f4495ba5 Mon Sep 17 00:00:00 2001
From: Marius Bakke <mbakke@fastmail.com>
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


[-- Attachment #1.5: Type: text/plain, Size: 60 bytes --]


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

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

[Ludovic Courtès]

Heya!

Marius Bakke <mbakke@fastmail.com> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>> 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:

Perfect!

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

Given that --with-default-path doesn’t quite work (due to lack of tilde
expansion), I suppose we should go for .bashrc?

> 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)

That’s maybe more than we’d like to do, and also doesn’t help with the
two other SSH implementations.

>> 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 <mbakke@fastmail.com>
> 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.

LGTM.

Thank you!

Ludo’.

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

[Marius Bakke]

[-- Attachment #1: Type: text/plain, Size: 52 bytes --]

Pushed revised series as 83a7a88f932..5d7141cd284e.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]