ssh-copy-id no newline bug

[Ernesto Alfonso]

When editing ~/.ssh/authorized_keys manually, sometimes users forget
to add a newline at the end of the file, causing the next ssh-copy-id
call to append a new key to an existing key, invalidating both keys.

This can be fixed by simply adding a newline before appending the key.

Something like this change to
openssh-source/openssh-6.7p1/contrib/ssh-copy-id might work:

# Assuming that the remote host treats ~/.ssh/authorized_keys as one
might expect
populate_new_ids 0
[ "$DRY_RUN" ] || printf '%s\n' "$NEW_IDS" | ssh "$@" "
umask 077 ;
mkdir -p .ssh && \
echo >> .ssh/authorized_keys && \
cat >> .ssh/authorized_keys || exit 1 ;
if type restorecon >/dev/null 2>&1 ; then restorecon -F .ssh
.ssh/authorized_keys ; fi" \
|| exit 1
ADDED=$(printf '%s\n' "$NEW_IDS" | wc -l)
;;

[Nico Kadel-Garcia]

Post by Ernesto Alfonso
When editing ~/.ssh/authorized_keys manually, sometimes users forget
to add a newline at the end of the file, causing the next ssh-copy-id
call to append a new key to an existing key, invalidating both keys.

Mind you, it's also fixed by using Emacs intead of vi.

[ esc-x runs-for-cover ]

[Philip Hands]

Post by Ernesto Alfonso
When editing ~/.ssh/authorized_keys manually, sometimes users forget
to add a newline at the end of the file, causing the next ssh-copy-id
call to append a new key to an existing key, invalidating both keys.
This can be fixed by simply adding a newline before appending the key.
Something like this change to

This seems like it should do no harm (given that sshd(8) declares that
blank lines are ignored as comments), although I'd instead do it by
adding a \n to the printf, thus:

[ "$DRY_RUN" ] || printf '\n%s\n' "$NEW_IDS" | ssh ...

Is anyone going to be upset by the resulting blank lines being added by
ssh-copy-id when the file was not missing a terminating newline?

Cheers, Phil.

[Alex Bligh]

Post by Philip Hands
Is anyone going to be upset by the resulting blank lines being added by
ssh-copy-id when the file was not missing a terminating newline?

Well it would be at least mildly annoying my previously nice looking file
now has a pile of blank lines in just because someone didn't know
how to use their editor ...

--
Alex Bligh

[Stephen Harris]

Post by Alex Bligh

Post by Philip Hands
Is anyone going to be upset by the resulting blank lines being added by
ssh-copy-id when the file was not missing a terminating newline?

Well it would be at least mildly annoying my previously nice looking file
now has a pile of blank lines in just because someone didn't know
how to use their editor ...

You can be pretty sure it'd end up causing a bug report as well
("ssh-copy-id introduces blank lines into good files").

[Colin Watson]

Post by Stephen Harris

Post by Alex Bligh

Post by Philip Hands
Is anyone going to be upset by the resulting blank lines being added by
ssh-copy-id when the file was not missing a terminating newline?

Well it would be at least mildly annoying my previously nice looking file
now has a pile of blank lines in just because someone didn't know
how to use their editor ...

You can be pretty sure it'd end up causing a bug report as well
("ssh-copy-id introduces blank lines into good files").

How about something like:

if [ "$(sed -n '${s/.*//;p}' ~/.ssh/authorized_keys | wc -l)" = 0 ]; then
echo >> ~/.ssh/authorized_keys
fi

I feel like there must be a neater but still portable way to do this,
and the above would require some careful quoting to work in the context
of ssh-copy-id.

[Ernesto Alfonso]

I agree it would be bad to add empty lines. I came up with this function:

function is_newline_terminated {
test -z "$(tail -c1 "${1}")"
}

to check whether an append would corrupt the file.

Maybe something like:

is_newline_terminated ~/.ssh/authorized_keys || echo >> ~/.ssh/authorized_keys

would work in the appropriate place.

Post by Stephen Harris

Post by Alex Bligh

Post by Philip Hands
Is anyone going to be upset by the resulting blank lines being added by
ssh-copy-id when the file was not missing a terminating newline?

Well it would be at least mildly annoying my previously nice looking file
now has a pile of blank lines in just because someone didn't know
how to use their editor ...

You can be pretty sure it'd end up causing a bug report as well
("ssh-copy-id introduces blank lines into good files").
--
rgds
Stephen

[Michael Stone]

Post by Colin Watson
if [ "$(sed -n '${s/.*//;p}' ~/.ssh/authorized_keys | wc -l)" = 0 ]; then
echo >> ~/.ssh/authorized_keys
fi
I feel like there must be a neater but still portable way to do this,

Maybe

if [ ! -z `tail -c 1 ~/.ssh/authorized_keys` ] ; then

Mike Stone

[Philip Hands]

Post by Michael Stone

Post by Colin Watson
if [ "$(sed -n '${s/.*//;p}' ~/.ssh/authorized_keys | wc -l)" = 0 ]; then
echo >> ~/.ssh/authorized_keys
fi
I feel like there must be a neater but still portable way to do this,

Maybe
if [ ! -z `tail -c 1 ~/.ssh/authorized_keys` ] ; then

Ah, thanks for that, I'd forgotten about the -c option.

I think that adding the following, just before the 'mkdir .ssh', should
do the trick:

[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;

anyone know of portability issues with that?

Cheers, Phil.

[Gert Doering]

Hi,

Post by Philip Hands
I think that adding the following, just before the 'mkdir .ssh', should
[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;
anyone know of portability issues with that?

I'm not sure what the test will do, but the "echo" should be an
"echo >>", no?

gert

[Philip Hands]

Post by Gert Doering
Hi,

Post by Philip Hands
I think that adding the following, just before the 'mkdir .ssh', should
[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;
anyone know of portability issues with that?

I'm not sure what the test will do, but the "echo" should be an
"echo >>", no?

Yes. Doh! ;-)

The test gets the last character of the file, and puts it in quotes
after removing new-lines -- which basically means that test fails if
there's a newline there, or if the file's empty, since that gives you:

[ "" ]

which means that the second half of the && does not need to be
evaluated -- the result being that a newline is only added if the last
character of the file is not already a newline.

The only thing that's wrong with that is if the file is empty it'll end
up with a blank line at the start, but a) why would there be an empty
file there? and b) who cares?

Cheers, Phil.

[Gert Doering]

Hi,

Post by Philip Hands
The test gets the last character of the file, and puts it in quotes
after removing new-lines -- which basically means that test fails if
[ "" ]

Thanks, I learned something new today - from "man test":

string True if string is not the null string.

... never really noticed this line in 20+ years of shell scripting :-)

To add some useful content - I've checked the oldest (and most exotic)
systems I have around, and both SCO Open Server 5.0 and AIX document
the same thing, so it should be truly portable.

gert

[Cristian Ionescu-Idbohrn]

Post by Philip Hands
which means that the second half of the && does not need to be
evaluated -- the result being that a newline is only added if the last
character of the file is not already a newline.

Well, then there's the case with the non-existing file and the other
case with an empty file. What about this:

f=/tmp/ff && >>$f && [ "$(tail -c1 $f)" ] && echo >>$f || :;stat $f

Post by Philip Hands

$f # like `touch', but no fork (makes sure the file

# exists) so `tail' will not fail

tail -c1 $f # on an empty file will return "" (as if there was a
# a <newline> there

;stat $f # is there just to show what's going on


Cheers,

[Tom G. Christensen]

Post by Philip Hands
[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;
anyone know of portability issues with that?

On Solaris 10 and older /usr/bin/tail does not understand the -c1
syntax, it needs -1c instead.
The XPG4 version in /usr/xpg4/bin/tail can understand either syntax.

I did not have Solaris 11 available to check.

-tgc

[Philip Hands]

Post by Tom G. Christensen

Post by Philip Hands
[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;
anyone know of portability issues with that?

On Solaris 10 and older /usr/bin/tail does not understand the -c1
syntax, it needs -1c instead.

OK, it seems that tail from coreutils on Debian can deal with -1c as
well -- odd -- is this actually the portable option despite not being in
the manual?

BTW can Solaris 10's tail handle it with a space after the c, thus:

tail -c 1

Cheers, Phil.

[Tom G. Christensen]

Post by Philip Hands

Post by Tom G. Christensen

Post by Philip Hands
[ "`tail -c1 .ssh/authorized_keys 2>/dev/null`" ] && echo > .ssh/authorized_keys ;
anyone know of portability issues with that?

On Solaris 10 and older /usr/bin/tail does not understand the -c1
syntax, it needs -1c instead.

OK, it seems that tail from coreutils on Debian can deal with -1c as
well -- odd -- is this actually the portable option despite not being in
the manual?
tail -c 1

No. It seems the default tail in /usr/bin is the old one from the BSD
days of SunOS 4 hence it does not know the more modern syntax.

You can see the manpage here:
https://docs.oracle.com/cd/E26502_01/html/E29030/tail-1.html

-tgc

[Michael Stone]

Post by Philip Hands
OK, it seems that tail from coreutils on Debian can deal with -1c as
well -- odd -- is this actually the portable option despite not being in
the manual?
tail -c 1

That's the POSIX syntax.

Friends don't let friends use the non-POSIX solaris utilities.

Mike Stone

[Philip Hands]

Post by Michael Stone

Post by Philip Hands
OK, it seems that tail from coreutils on Debian can deal with -1c as
well -- odd -- is this actually the portable option despite not being in
the manual?
tail -c 1

That's the POSIX syntax.
Friends don't let friends use the non-POSIX solaris utilities.

I guess that's a function of the PATH in use, and we don't have much
control over that unless I were to start adding weird tests in this
command line, so I'm not quite sure what you're trying to say there.

Anyway, having read the GNU coreutils info page, I see it supports the
-1c "obsolete" usage, and recent BSD manpages say they support the same
"historical" usage too, so the best bet would seem to be -1c (unless
there are some modern tail implementations that fail to support this).

On the strength of that, and having considered suggestions in this
thread, I'm settling on adding this after the ``mkdir .ssh &&'' :

f=.ssh/authorized_keys && { [ -z "`tail -1c $f 2>/dev/null`" ] || echo >> $f ; } &&

I've inverted the test, in order that the series of &&'s can continue,
it's using -1c for tail, as that seems likely to be maximally portable,
likewise `` is more portable than $(), I don't see much benefit in
creating the file just so that the tail won't fail, so I've got the
2>/dev/null instead (I'm open to persuasion on that, since if one
creates the file, and doesn't discard the STDERR, then if tail manages
to fail for some other reason, we'd perhaps find out about it), and the
use of a variable ($f) removes duplication later in the line, and keeps
the line length under 255 which could conceivably be a limit in some
awful shell somewhere.

Did I miss anything?

Cheers, Phil.