ssh-copy-id fails when run with colon (:)

This is an update to the patch i posted in Debians BTS - it diffs "the correct way", rather then improving functionality at all. If it works for other people, I'll try and do up a proper using-patch-system debdiff.

I don't use a patch system for openssh, and would prefer you not to introduce one.

As I said in the Debian BTS, perhaps ssh itself should do this, rather than ssh-copy-id? You apparently ignored this comment. That said, that seems to be fiddly and maybe isn't worth it.

Your patch introduces 'set -ex'. -e doesn't make any difference in this code, and -x is noisy.

Your patch doesn't follow the indentation convention in that file. You should always follow the prevailing convention.

It would be better just to strip colons at the end, rather than throughout (which could produce very confusing results!). For that, just replacing $1 with ${1%:} would be entirely sufficient; you don't need to introduce another shell variable. (And why export HOSTNAME? Nothing outside this script needs it.)

In future, consider writing this instead of introducing an unnecessary call to the '[' program:

-if [ `echo "$1" |grep -c \: -` ]; then
+if echo "$1" | grep -q :; then

In the end, I've applied the attached patch to my Debian CVS repository. Thanks for nudging this along.

On Thu, October 9, 2008 21:25, Colin Watson wrote:
> As I said in the Debian BTS, perhaps ssh itself should do this, rather
> than ssh-copy-id? You apparently ignored this comment.

I have no C, which i assume is what ssh proper is written in.

That said, that
> seems to be fiddly and maybe isn't worth it.
>
> Your patch introduces 'set -ex'. -e doesn't make any difference in this
> code, and -x is noisy.

Sorry, I should have cleaned up after testing.

> Your patch doesn't follow the indentation convention in that file. You
> should always follow the prevailing convention.
>
> It would be better just to strip colons at the end, rather than
> throughout (which could produce very confusing results!). For that, just
> replacing $1 with ${1%:} would be entirely sufficient; you don't need to
> introduce another shell variable. (And why export HOSTNAME? Nothing
> outside this script needs it.)

>
> In future, consider writing this instead of introducing an unnecessary
> call to the '[' program:
>
> -if [ `echo "$1" |grep -c \: -` ]; then
> +if echo "$1" | grep -q :; then

I didnt know this was posible, so thanks for letting me know. I'll try and
familiarise myself with it.

>
> In the end, I've applied the attached patch to my Debian CVS repository.
> Thanks for nudging this along.

Thanks for fixing it up.
kk

This bug was fixed in the package openssh - 1:5.1p1-4ubuntu1

---------------
openssh (1:5.1p1-4ubuntu1) jaunty; urgency=low

  * Resynchronise with Debian. Remaining changes:
    - Add support for registering ConsoleKit sessions on login.
    - Drop openssh-blacklist and openssh-blacklist-extra to Suggests; they
      take up a lot of CD space, and I suspect that rolling them out in
      security updates has covered most affected systems now.
    - Add ufw integration.

openssh (1:5.1p1-4) unstable; urgency=low

  * ssh-copy-id: Strip trailing colons from hostname (closes: #226172,
    LP: #249706; thanks to Karl Goetz for nudging this along; forwarded
    upstream as https://bugzilla.mindrot.org/show_bug.cgi?id=1530).
  * Backport from upstream CVS (Markus Friedl):
    - Only send eow and no-more-sessions requests to openssh 5 and newer;
      fixes interop problems with broken ssh v2 implementations (closes:
      #495917).
  * Fix double-free when failing to parse a forwarding specification given
    using ~C (closes: #505330; forwarded upstream as
    https://bugzilla.mindrot.org/show_bug.cgi?id=1539).

 -- Colin Watson <email address hidden> Sun, 23 Nov 2008 14:55:17 +0000