Re: [RFC] /etc/shells management (fish, mksh, posh, tcsh, zsh)

[Chris Sutcliffe]

On 13 May 2016 at 06:29, Andrew Schulman wrote:
>> On 2016-05-11 14:06, Yaakov Selkowitz wrote:
>> > On 2016-05-11 12:09, Andrew Schulman wrote:
>> >>> Am 10.05.2016 um 20:19 schrieb Andrew Schulman:
>>  Achim, can you please add /bin/fish and /usr/bin/fish to /etc/shells in
>>  base-files?
>> >>>
>> AFAICS this should be a two-step process.
>>
>> 1) base-files' default /etc/shells should contain only the shells in a
>> Base install, namely:
>>
>> /bin/sh
>> /bin/ash
>> /bin/bash
>> /bin/dash
>> /usr/bin/sh
>> /usr/bin/ash
>> /usr/bin/bash
>> /usr/bin/dash
>> /sbin/nologin
>
> Yep.
>
>> 2) Then all non-Base shells, namely:
>>
>> fish Andrew Schulman
>> mksh Chris Sutcliffe
>> posh Jari Aalto
>> tcsh Corinna Vinschen
>> zsh  Peter A. Castro
>>
>> will bump release adding an update_etc_shells call, per the attached
>> patch, with the path of their shell(s).
>
> Agreed.

I'm fine with this approach as well.

Thanks,

Chris

-- 
Chris Sutcliffe

Re: [RFC] /etc/shells management (fish, mksh, posh, tcsh, zsh)

[Andrew Schulman]

> On 2016-05-11 14:06, Yaakov Selkowitz wrote:
> > On 2016-05-11 12:09, Andrew Schulman wrote:
> >>> Am 10.05.2016 um 20:19 schrieb Andrew Schulman:
>  Achim, can you please add /bin/fish and /usr/bin/fish to /etc/shells in
>  base-files?
> >>>
> AFAICS this should be a two-step process.
> 
> 1) base-files' default /etc/shells should contain only the shells in a 
> Base install, namely:
> 
> /bin/sh
> /bin/ash
> /bin/bash
> /bin/dash
> /usr/bin/sh
> /usr/bin/ash
> /usr/bin/bash
> /usr/bin/dash
> /sbin/nologin

Yep.

> 2) Then all non-Base shells, namely:
> 
> fish Andrew Schulman
> mksh Chris Sutcliffe
> posh Jari Aalto
> tcsh Corinna Vinschen
> zsh  Peter A. Castro
> 
> will bump release adding an update_etc_shells call, per the attached 
> patch, with the path of their shell(s).

Agreed.

> Attached.  Any questions or comments before I make this official?

This looks right, except that it edits /etc/shells directly.  So if a person
edits /etc/shells to remove, say, fish, and fish gets updated, as written this
patch will add fish back in. (Why they'd install fish but not want it in
/etc/shells I don't know, but it's possible.)

Better is to add the new shell to /etc/default/etc/shells, then copy that file
over /etc/shells if that file had not been previously edited.
 
> Or, is this just not worth the trouble?  What are the consequences of 
> having shells listed in /etc/shells which aren't on the system?

/etc/shells doesn't seem to be very important in Cygwin.  And it includes one
shell now (pdksh) that doesn't exist in Cygwin, and it's not hurting anything.

But it's not a lot of work to do it right, and I think we should.

Andrew

Re: [RFC] /etc/shells management (fish, mksh, posh, tcsh, zsh)

[Warren Young]

On May 12, 2016, at 3:36 PM, Yaakov Selkowitz  wrote:
> 
> What are the consequences of having shells listed in /etc/shells which aren't 
> on the system?

That file is a security feature, but the typical way Cygwin works — i.e. that 
normal users are allowed to install software, modify /etc/*, and so forth — 
nullifies its value.

But, if you do somehow lock down /etc/shells so that normal users can’t write 
to it, you’re also presumably locking down /bin, so a malicious user couldn’t 
drop in a bogus /bin/fish file and convince other software to run it as a shell.

Too bad there is no /etc/shells.d.  Then non-Base shells could just add 
themselves there.

[RFC] /etc/shells management (fish, mksh, posh, tcsh, zsh)

[Yaakov Selkowitz]

On 2016-05-11 14:06, Yaakov Selkowitz wrote:

On 2016-05-11 12:09, Andrew Schulman wrote:

Am 10.05.2016 um 20:19 schrieb Andrew Schulman:

Achim, can you please add /bin/fish and /usr/bin/fish to /etc/shells in
base-files?


I seem to remember that this was discussed before.  If you could perhaps
look up that discussion and fill me in what the conclusion was last time
around?


Hm, you're right, it was discussed before:

https://www.cygwin.com/ml/cygwin/2014-02/msg00696.html

I don't know if there was consensus, but the last word there from CGF was
that shell packages should run a postinstall step to add themselves to
/etc/shells.


While I'm always ready to reconsider previous decisions, this is how it
appears to be handled in Linux distros.  The implication thereof is that
(once all packages have been adapted) the default /etc/shells should
only contain those shells available by default (namely, sh, bash, and
/sbin/nologin), e.g.:

https://git.fedorahosted.org/cgit/setup.git/tree/shells

(except that /sbin != /usr/sbin on Cygwin.)


AFAICS this should be a two-step process.

1) base-files' default /etc/shells should contain only the shells in a 
Base install, namely:


/bin/sh
/bin/ash
/bin/bash
/bin/dash
/usr/bin/sh
/usr/bin/ash
/usr/bin/bash
/usr/bin/dash
/sbin/nologin

2) Then all non-Base shells, namely:

fish Andrew Schulman
mksh Chris Sutcliffe
posh Jari Aalto
tcsh Corinna Vinschen
zsh  Peter A. Castro

will bump release adding an update_etc_shells call, per the attached 
patch, with the path of their shell(s).



That seems reasonable. There are questions about the right way to do it,
but I'll ask those in a separate thread.


Probably best if we have a cygport function for creating the necessary
postinstall and preremove commands.


Attached.  Any questions or comments before I make this official?

Or, is this just not worth the trouble?  What are the consequences of 
having shells listed in /etc/shells which aren't on the system?


--
Yaakov
>From 68d32dfee3ed2b0b8c1f356ef794846f086d3583 Mon Sep 17 00:00:00 2001
From: Yaakov Selkowitz 
Date: Thu, 12 May 2016 16:14:33 -0500
Subject: [PATCH] Add update_etc_shells for /etc/shells management

See https://cygwin.com/ml/cygwin/2016-05/msg00135.html
---
 lib/src_install.cygpart | 56 -
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/lib/src_install.cygpart b/lib/src_install.cygpart
index 1fe8176..41adcb3 100644
--- a/lib/src_install.cygpart
+++ b/lib/src_install.cygpart
@@ -877,6 +877,60 @@ make_etc_defaults() {
done
 }
 
+#I* Installing/update_etc_shells
+#  SYNOPSIS
+#  update_etc_shells PATH_TO_SHELL [PATH_TO_SHELL] ...
+#  DESCRIPTION
+#  Indicates that the given fully-qualified path(s) are shells which should be
+#  listed in /etc/shells.  update_etc_shells creates a postinstall script to
+#  add this listing if it doesn't already exist, and a preremove script which
+#  removes it when uninstalling.
+#  NOTES
+#  * Only one of the /bin or /usr/bin paths should be specified for any given
+#shell; the other will be added automatically.
+#  * Generic aliases should be listed as well, e.g. tcsh would use
+#update_etc_shells /bin/tcsh /bin/csh
+#  * Shells which are part of the Base install (namely, bash/sh and dash/ash)
+#should not use this function, as they are already included in the default
+#/etc/shells.
+#
+update_etc_shells() {
+   local alt sh
+
+   for sh in ${@}
+   do
+   case ${sh} in
+   /bin/*) alt=/usr${sh} ;;
+   /usr/bin/*) alt=${sh#/usr} ;;
+   *)  alt= ;;
+   esac
+
+   if [ ! -e ${D}${sh} ] && [ ! -e ${D}${alt:-${sh}} ]
+   then
+   error "shell ${sh} does not exist"
+   fi
+
+   dodir /etc/postinstall
+   cat >> ${D}/etc/postinstall/${PN}.sh <<-_EOF
+   if [ ! -f /etc/shells ] || ! grep -q "^${sh}$" 
/etc/shells
+   then
+   echo -e "${sh}${alt:+\n}${alt}" >> /etc/shells
+   fi
+
+   _EOF
+
+   dodir /etc/preremove
+   cat >> ${D}/etc/preremove/${PN}.sh <<-_EOF
+   if [ -f /etc/shells ]
+   then
+   sed -i -e '\|^${sh}$|d' /etc/shells
+   ${alt:+sed -i -e '\|^${alt}$|d' /etc/shells}
+   fi
+
+   _EOF
+   done
+}
+
 __prepinstalldirs() {
rm -fr ${D}/*;
 }
@@ -950,4 +1004,4 @@ readonly -f __doinstall __fix_shebang \
 exeinto doexe newexe insinto doins newins doicon newicon \