diff mbox

Read /etc/environment first to allow changing environment from user profile

Message ID 7t60rrmf9j.fsf@gmail.com
State Accepted
Headers show

Commit Message

Carlos Sánchez de La Lama July 27, 2016, 12:34 p.m. UTC
Hi all,

as reported in bug #22175, lshd does not honor /etc/environment. This
was fixed here:

http://git.savannah.gnu.org/cgit/guix.git/commit/gnu/system.scm?id=2a5f0db4c45679cac6a747a48993fe73982cadca

However, the order in /etc/profile is problematic: some variables are
set up by "$HOME/.guix-profile/etc/profile", but then they get (wrongly)
overriden by the values in /etc/environment. In my case, this happens
with SSL_CERT_DIR, which has the value

/home/csanchez/.guix-profile/etc/ssl/certs:/etc/ssl/certs

then logging in locally, but only

/etc/ssl/certs

when logging in from lshd.

I attach the proposed patch (just a change of order in /etc/profile). As
'cat' and 'cut' ar most surely available at system-level, it should not
be dangerous to use them before setting up the user profile.

BR

Carlos

Comments

Tobias Geerinckx-Rice July 27, 2016, 4:02 p.m. UTC | #1
Carlos,

On 27/07/2016 14:34, Carlos Sánchez de La Lama wrote:
> I attach the proposed patch (just a change of order in /etc/profile). As
> 'cat' and 'cut' are most surely available at system-level, it should not
> be dangerous to use them before setting up the user profile.

That's a valid point, but this should be doable in pure shell. It might
even be faster (untested), and avoids needless forking.

Most importantly, it's fun.

> . /etc/environment
> export `cat /etc/environment | cut -d= -f1`

‘cat file | ...’ can be replaced with ‘... < file’; ‘cut’ isn't needed
since export accepts ‘VAR[=value]’ arguments[1][2]. You've just imported
‘/etc/environment’ on the previous line, and the values haven't been
modified. Hence, the two lines above can be written as:

  while read line; do export "$line"; done < /etc/environment

I don't know if Guix even cares about non-bash shells, but this should™
work in all POSIX®-compliant ones[4]. It's also more readable.

Thoughts? Gotchas?

Kind regards,

T G-R

[1]: http://pubs.opengroup.org/onlinepubs/009696799/utilities/export.html
[2]: even if it didn't, section 2.6.2 of [3] has got you covered
[3]:
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html
[4]: which reminds me to finish packaging ash for Guix
Danny Milosavljevic July 27, 2016, 4:31 p.m. UTC | #2
>   while read line; do export "$line"; done < /etc/environment
> 
> I don't know if Guix even cares about non-bash shells, but this should™
> work in all POSIX®-compliant ones[4]. It's also more readable.
> 
> Thoughts? Gotchas?

Spaces in keys and/or values don't work that way. The read itself already has problems with them. You might try setting IFS="
" but not sure whether that's standards-compliant.

Also, does csh actually support "export"? The one I have in Solaris doesn't. You need to do "setenv" there.
Tobias Geerinckx-Rice July 27, 2016, 5:18 p.m. UTC | #3
Hi Danny,

On 27/07/2016 18:31, Danny Milosavljevic wrote:
>> while read line; do export "$line"; done < /etc/environment
>> 
>> I don't know if Guix even cares about non-bash shells, but this 
>> should™ work in all POSIX®-compliant ones[4]. It's also more 
>> readable.
>> 
>> Thoughts? Gotchas?
> 
> Spaces in keys and/or values don't work that way.
> The read itself already has problems with them.

No. Perhaps you're confusing it with ‘read < /etc/environment’.

Also, there are no spaces in keys, and technically, spaces in values
work just fine: it's the quotes that don't get dropped. D'oh! :-P

Yeah, trying to emulate unquoting (and all manner of escaping that it
entails) would be utter madness. Uglier option [2] it is then:

  . /etc/environment
  while read line; export "${line%%=*}"; done < /etc/environment

It's ugly and racy and less straightforward.
But then so was the original, I guess.

> You might try setting IFS="
> " but not sure whether that's standards-compliant.

Setting IFS to a newline doesn't make sense here.

To be honest, I've a personal dislike for IFS, though it is
POSIX-compliant. Saving IFS, setting IFS to ‘=’, running ‘read key
value’, ignoring ‘value’, and restoring IFS would work.

> Also, does csh actually support "export"? The one I have in Solaris 
> doesn't. You need to do "setenv" there.

csh isn't a (POSIX) sh.

Thanks for the bug spotting,

T G-R

[1]: It seems to be standard, but I didn't bother testing.

> On 27/07/2016 18:02, Tobias Geerinckx-Rice wrote:
> [2]: even if it didn't, section 2.6.2 of [3] has got you covered
> [3]:
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html
Tobias Geerinckx-Rice July 27, 2016, 5:23 p.m. UTC | #4
On 27/07/2016 19:18, Tobias Geerinckx-Rice wrote:
> [1]: It seems to be standard, but I didn't bother testing.

Ignore this forgotten line. I couldn't help myself and did bother.
Unfortunately, it worked.
Ludovic Courtès July 27, 2016, 9:11 p.m. UTC | #5
Hi,

csanchezdll@gmail.com (Carlos Sánchez de La Lama) skribis:

> as reported in bug #22175, lshd does not honor /etc/environment. This
> was fixed here:
>
> http://git.savannah.gnu.org/cgit/guix.git/commit/gnu/system.scm?id=2a5f0db4c45679cac6a747a48993fe73982cadca
>
> However, the order in /etc/profile is problematic: some variables are
> set up by "$HOME/.guix-profile/etc/profile", but then they get (wrongly)
> overriden by the values in /etc/environment. In my case, this happens
> with SSL_CERT_DIR, which has the value
>
> /home/csanchez/.guix-profile/etc/ssl/certs:/etc/ssl/certs
>
> then logging in locally, but only
>
> /etc/ssl/certs
>
> when logging in from lshd.

Indeed, good catch!

> I attach the proposed patch (just a change of order in /etc/profile). As
> 'cat' and 'cut' ar most surely available at system-level, it should not
> be dangerous to use them before setting up the user profile.

Yes.

Tobias: if you want to avoid the ‘cut’ invocation, feel free to send
another patch.  :-)

> From 474e8980ee933e6694cc55ca61607adae86dacf1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Carlos=20S=C3=A1nchez=20de=20La=20Lama?=
>  <csanchezdll@gmail.com>
> Date: Wed, 27 Jul 2016 14:27:00 +0200
> Subject: [PATCH] Read /etc/environment first to allow changing environment
>  from user profile.
>

I tested in ‘guix system vm’ and it seems to work as expected.  Pushed
as cad7e6abafc14de220265e09a0fc4bbd96664599 with a commit log that
follows our conventions.

Thank you!

Ludo’.
diff mbox

Patch

From 474e8980ee933e6694cc55ca61607adae86dacf1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Carlos=20S=C3=A1nchez=20de=20La=20Lama?=
 <csanchezdll@gmail.com>
Date: Wed, 27 Jul 2016 14:27:00 +0200
Subject: [PATCH] Read /etc/environment first to allow changing environment
 from user profile.

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

diff --git a/gnu/system.scm b/gnu/system.scm
index 476720b..3ae4ae7 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -408,6 +408,16 @@  GUIX_PROFILE=/run/current-system/profile \\
 # Prepend setuid programs.
 export PATH=/run/setuid-programs:$PATH
 
+# Since 'lshd' does not use pam_env, /etc/environment must be explicitly
+# loaded when someone logs in via SSH.  See <http://bugs.gnu.org/22175>.
+# We need 'PATH' to be defined here, for 'cat' and 'cut'.
+if [ -f /etc/environment -a -n \"$SSH_CLIENT\" \\
+     -a -z \"$LINUX_MODULE_DIRECTORY\" ]
+then
+  . /etc/environment
+  export `cat /etc/environment | cut -d= -f1`
+fi
+
 if [ -f \"$HOME/.guix-profile/etc/profile\" ]
 then
   # Load the user profile's settings.
@@ -419,16 +429,6 @@  else
   export PATH=\"$HOME/.guix-profile/bin:$PATH\"
 fi
 
-# Since 'lshd' does not use pam_env, /etc/environment must be explicitly
-# loaded when someone logs in via SSH.  See <http://bugs.gnu.org/22175>.
-# We need 'PATH' to be defined here, for 'cat' and 'cut'.
-if [ -f /etc/environment -a -n \"$SSH_CLIENT\" \\
-     -a -z \"$LINUX_MODULE_DIRECTORY\" ]
-then
-  . /etc/environment
-  export `cat /etc/environment | cut -d= -f1`
-fi
-
 # Set the umask, notably for users logging in via 'lsh'.
 # See <http://bugs.gnu.org/22650>.
 umask 022
-- 
2.7.3