ssh-copy-id bugfix

[Radek Podgorny]

hello everyone!

i'd like to sincerely ask you to include a fix for ssh-copy-id bug
i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life of
many. it's been field-tested by redhat devs and users so i see no
problem in incorporating it.

http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix-ss
h-copy-id-on-non-sh-shell.patch

thanks a lot!
R.

[Ruediger Meier]

Hi,

Post by Radek Podgorny
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id bug
i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life of
many. it's been field-tested by redhat devs and users so i see no
problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix
-ss h-copy-id-on-non-sh-shell.patch

- umask 077 ;
+ exec sh -c 'umask 077; mkdir -p .ssh && cat >> .ssh/authorized_keys || exit 1; if type restorecon >/dev/null 2>&1; then restorecon -F .ssh .ssh/authorized_keys; fi'" \
- mkdir -p .ssh && cat >> .ssh/authorized_keys || exit 1 ;
- if type restorecon >/dev/null 2>&1 ; then restorecon -F .ssh .ssh/authorized_keys ; fi" \

Does "exec sh -c ..." really make sense in general? People who are
using non-posix login shells where not even "2>&1" or "&&" works are
probably good candidates who would also link /bin/sh to point to a
non-posix shell.

Personally I think it's hard enough to write POSIX compatible shell
scripts and I wouldn't start to add such hacks for fish and tcsh.
Next week somebody may complain that his "shell" does not
support "exec ...".


cu,
Rudi

[Radek Podgorny]

hi,

Post by Ruediger Meier
Hi,

Post by Radek Podgorny
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id
bug i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life
of many. it's been field-tested by redhat devs and users so i see
no problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix

- -ss h-copy-id-on-non-sh-shell.patch

Post by Ruediger Meier

Post by Radek Podgorny

- umask 077 ; + exec sh -c 'umask 077; mkdir -p .ssh && cat >>
.ssh/authorized_keys || exit 1; if type restorecon >/dev/null
2>&1; then restorecon -F .ssh .ssh/authorized_keys; fi'" \ -
mkdir -p .ssh && cat >> .ssh/authorized_keys || exit 1 ; - if
type restorecon >/dev/null 2>&1 ; then restorecon -F .ssh
.ssh/authorized_keys ; fi" \

Does "exec sh -c ..." really make sense in general? People who are
using non-posix login shells where not even "2>&1" or "&&" works
are probably good candidates who would also link /bin/sh to point
to a non-posix shell.
Personally I think it's hard enough to write POSIX compatible
shell scripts and I wouldn't start to add such hacks for fish and
tcsh. Next week somebody may complain that his "shell" does not
support "exec ...".

i wouldn't be afraid of that. i think it's a common practice (no hard
numbers for that, thou) that you leave the sh link pointed to posix
shell at all times - there's too many things in the wild depending on
that.

anyway, i wouldn't call it a hack. you need a posix shell on the
remote side and this so far the best method to state it. of course,
someone may have a relly odd shell with no exec support or have the sh
link pointing elsewhere but for such poor guy, the ssh-copy-id is not
working today, anyway, so no real "breakage" happens. on the other
hand, there's many people who would benefit from this patch and as
it's backwards compatible, nothing gets broken for anyone.

if - and that may never happen - in the future someone complains about
his shell not being supported, let's find a better way. but until then
i think this is a safe thing to do.

thanks,
R.

Post by Ruediger Meier
cu, Rudi

[Nico Kadel-Garcia]

Post by Ruediger Meier
Hi,

Post by Radek Podgorny
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id bug
i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life of
many. it's been field-tested by redhat devs and users so i see no
problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix
-ss h-copy-id-on-non-sh-shell.patch

Personally I think it's hard enough to write POSIX compatible shell
scripts and I wouldn't start to add such hacks for fish and tcsh.
Next week somebody may complain that his "shell" does not
support "exec ...".

Making things work for more people, when it doesn't introduce a
security risk or break other tools, seems very reasonable. And there
are people out there who who do use both fish and tcsh.

What seems to be missing in the patch is a comment line, above the
stanza, explaining why the code uses "exec". It's great to be clever
and solve a problem, but it boosts your pay and makes people who
follow in your role hate you a lot less if they can understand why you
chose a particular solution.

[Michael Stone]

Post by Ruediger Meier
Does "exec sh -c ..." really make sense in general? People who are
using non-posix login shells where not even "2>&1" or "&&" works are
probably good candidates who would also link /bin/sh to point to a
non-posix shell.

Non-posix login shells (csh et al) are not exactly uncommon. /bin/sh
pointing to something non-posix is uncommon to the point of being unable
to work on common systems.

Mike Stone

[Nico Kadel-Garcia]

Post by Michael Stone

Post by Ruediger Meier
Does "exec sh -c ..." really make sense in general? People who are
using non-posix login shells where not even "2>&1" or "&&" works are
probably good candidates who would also link /bin/sh to point to a
non-posix shell.

Non-posix login shells (csh et al) are not exactly uncommon. /bin/sh
pointing to something non-posix is uncommon to the point of being unable to
work on common systems.

Has anyone tried this one with Ubuntu, which uses a symlink from
/bin/sh to /bin/dash?

[Jakub Jelen]

Post by Nico Kadel-Garcia

Post by Ruediger Meier
Personally I think it's hard enough to write POSIX compatible shell
scripts and I wouldn't start to add such hacks for fish and tcsh.
Next week somebody may complain that his "shell" does not
support "exec ...".

Making things work for more people, when it doesn't introduce a
security risk or break other tools, seems very reasonable. And there
are people out there who who do use both fish and tcsh.
What seems to be missing in the patch is a comment line, above the
stanza, explaining why the code uses "exec". It's great to be clever
and solve a problem, but it boosts your pay and makes people who
follow in your role hate you a lot less if they can understand why you
chose a particular solution.

Currently, it is the only solution we have and works for conventional
shells as well as for fish and tcsh. Maybe there are other solutions,
better or worse. I am no expert on different shells and their
compatibility, so I just shared what we actually use across our systems
for some time and which works for us, so other interested can benefit
from it. I am not forcing anyone to use it.

I completely agree that there should be some wider reasoning behind
every change. But here we have just "experience" with larger subset of
shells used these days.

Regards,

[Philip Hands]

Post by Nico Kadel-Garcia

Post by Ruediger Meier
Hi,

Post by Radek Podgorny
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id bug
i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life of
many. it's been field-tested by redhat devs and users so i see no
problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix
-ss h-copy-id-on-non-sh-shell.patch

Personally I think it's hard enough to write POSIX compatible shell
scripts and I wouldn't start to add such hacks for fish and tcsh.
Next week somebody may complain that his "shell" does not
support "exec ...".

Making things work for more people, when it doesn't introduce a
security risk or break other tools, seems very reasonable. And there
are people out there who who do use both fish and tcsh.
What seems to be missing in the patch is a comment line, above the
stanza, explaining why the code uses "exec".

My reading of the presence of "exec" there was:

We're assuming that the current shell may not be to our liking, so
there seems to be little point keeping it in memory solely so it can
at worst somehow get in the way of a clean exit.

Does that really need a comment? I'm not sure I can make a succinct
explanation of what's going on for anyone that doesn't already know what
exec does. Feel free to make suggestions though.

Cheers, Phil.

[Philip Hands]

Hi Radek,

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id bug
i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life of
many. it's been field-tested by redhat devs and users so i see no
problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix-ss
h-copy-id-on-non-sh-shell.patch

Seems fair enough to me.

I've been meaning to do some maintenance on ssh-copy-id for quite a
while, so thanks for the nudge -- that's what parenthood does for
available spare time ;-)

BTW I've just pushed this fix to my git repo:

http://git.hands.com/ssh-copy-id

hopefully that can be in the next OpenSSH release -- I'll endeavour to
deal with some of the rest of the bug backlog in the next day or two.

Cheers, Phil.

[Radek Podgorny]

thanks for your effort, phil!

R.

Post by Philip Hands
Hi Radek,

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
hello everyone!
i'd like to sincerely ask you to include a fix for ssh-copy-id
bug i'll be linking below. it's a trivial fix which resolves
https://bugzilla.mindrot.org/show_bug.cgi?id=2206 and eases life
of many. it's been field-tested by redhat devs and users so i see
no problem in incorporating it.
http://pkgs.fedoraproject.org/cgit/openssh.git/tree/openssh-6.8p1-fix

- -ss
h-copy-id-on-non-sh-shell.patch

Post by Philip Hands
Seems fair enough to me.
I've been meaning to do some maintenance on ssh-copy-id for quite
a while, so thanks for the nudge -- that's what parenthood does
for available spare time ;-)
http://git.hands.com/ssh-copy-id
hopefully that can be in the next OpenSSH release -- I'll endeavour
to deal with some of the rest of the bug backlog in the next day or
two.
Cheers, Phil.

[Nico Kadel-Garcia]

Post by Philip Hands

Post by Nico Kadel-Garcia
What seems to be missing in the patch is a comment line, above the
stanza, explaining why the code uses "exec".

We're assuming that the current shell may not be to our liking, so
there seems to be little point keeping it in memory solely so it can
at worst somehow get in the way of a clean exit.
Does that really need a comment? I'm not sure I can make a succinct
explanation of what's going on for anyone that doesn't already know what
exec does. Feel free to make suggestions though.

That is _precisely_ why it needs a comment. It's a selection of a
particular technology for a particular reason that someone may not
understand as important without having to dig back to a thread or bug
report like this. For example:

# Use "exec sh -c" to ensure POSIX compliant scripting,
especially for fish and tcsh users
[ "$DRY_RUN" ] || printf '%s\n' "$NEW_IDS" | ssh "$@" "
.....

[Peter Stuge]

Post by Nico Kadel-Garcia

Post by Philip Hands
Does that really need a comment?

That is _precisely_ why it needs a comment. It's a selection of a
particular technology for a particular reason that someone may not
understand as important

Not even if they understand what the command does? (Use another shell.)

I dislike redundant comments, which this seems to be. As code change
and comments don't, those comments end up creating a lot more confusion
than they resolve.

Having clear code is far more important.

It's reasonable to require that someone reading code already knows the
tools being used.

It's not reasonable for code to educate readers on tools being used.


//Peter

[Nico Kadel-Garcia]

Post by Peter Stuge

Post by Nico Kadel-Garcia

Post by Philip Hands
Does that really need a comment?

That is _precisely_ why it needs a comment. It's a selection of a
particular technology for a particular reason that someone may not
understand as important

Not even if they understand what the command does? (Use another shell.)
I dislike redundant comments, which this seems to be. As code change
and comments don't, those comments end up creating a lot more confusion
than they resolve.
Having clear code is far more important.
It's reasonable to require that someone reading code already knows the
tools being used.

I'm familiar with the old "the code is the comments" approach to
documentation. The difficulty with it is exactly what we see here.
It's not clear, even to a reasonably intelligent bash progammer, that
the use of "exec" is to insure compatibility with fish and tcsh users.
I've used both, and in fact published the first public ports of tcsh
for SunOS, and even I'd have think carefully to deduce, from raw
principles, why my scripting in bash is written in a fairly ugly to
ensure compatibility with alternative shells. 5 years from now, I'd
probably have forgotten completely about this thread and be
hard-pressed to remember why I or someone else didn't simply quote
ordinary bash syntax there.

Post by Peter Stuge
It's not reasonable for code to educate readers on tools being used.

I suggest that it's very reasonable to explain particular choices in
code, especially when those choices were made for compatibility
reasons less common configurations. Failure to document such choices
leads to regression errors when someone unweaves the undocumented
compatibility code.

Been there, done that, especially had screwups with underdocumented
and subtly incompatible or inconsistent tools for editing
$HOME/.ssh/authorized_keys editing.

[Stephen Harris]

Post by Nico Kadel-Garcia
It's not clear, even to a reasonably intelligent bash progammer, that
the use of "exec" is to insure compatibility with fish and tcsh users.

The use of exec is not to ensure compatability. Just doing
sh -c "..."
would be enough.

The "exec" is for efficiency. It is not _needed_.

I would skip it, personally. The efficiency gains are neglibible.

[Philip Hands]

Post by Stephen Harris

Post by Nico Kadel-Garcia
It's not clear, even to a reasonably intelligent bash progammer, that
the use of "exec" is to insure compatibility with fish and tcsh users.

The use of exec is not to ensure compatability. Just doing
sh -c "..."
would be enough.
The "exec" is for efficiency. It is not _needed_.
I would skip it, personally. The efficiency gains are neglibible.

I doubt I'd have put the "exec" in, but having thought about it briefly,
I decided to keep it (at least until someone points to a shell that
doesn't have exec, but would otherwise succeed in running sh).

FWIW I think Nico does have a point about having a comment that would
act as a reminder not to break the portability features of that line, so
that's what I'll do.

Cheers, Phil.

[Stephen Harris]

Post by Philip Hands

Post by Stephen Harris

Post by Nico Kadel-Garcia
It's not clear, even to a reasonably intelligent bash progammer, that
the use of "exec" is to insure compatibility with fish and tcsh users.

The use of exec is not to ensure compatability. Just doing
sh -c "..."
would be enough.
The "exec" is for efficiency. It is not _needed_.

I doubt I'd have put the "exec" in, but having thought about it briefly,
I decided to keep it (at least until someone points to a shell that
doesn't have exec, but would otherwise succeed in running sh).

I can come up with an artificial case (a "menu shell" type construct,
or some embedded device with a limited CLI) but I can't think of any
regular shell that'd suffer the issue.

Post by Philip Hands
FWIW I think Nico does have a point about having a comment that would
act as a reminder not to break the portability features of that line, so
that's what I'll do.

A comment is definitely necessary because this thread has shown that
a multi-decade old coding construct (I first came across it in 1991)
isn't properly understood, even by the smart people on this list.

If I was doing it from scratch I might have considered just having
something like
ssh -t "$@" /bin/sh -i
and then putting the umask/mkdir/... commands on stdin before the pubkeys
(that's how I've built solutions in the past) to keep complicated commands
and quoting away from the CLI parser, but in this case the commands are
simple enough that the proposed patch works well enough.