patch for ssh completion

patch for ssh completion

[Jeremy Cantrell]

[-- Attachment #1.1: Type: text/plain, Size: 361 bytes --]

Hello!

I've found a tiny problem with the ssh completion script where any host
entries defined in included configuration files do not appear as choices
when completing. I've attached a patch that fixes it for me. Below is an
example that illustrates the problem:

~/.ssh/config:
Include hosts

~/.ssh/hosts:
Host example
HostName example.local

Thanks,
Jeremy

[-- Attachment #1.2: Type: text/html, Size: 527 bytes --]

[-- Attachment #2: zsh-ssh-host-completion.patch --]
[-- Type: text/x-patch, Size: 1204 bytes --]

--- /usr/share/zsh/functions/Completion/Unix/_ssh	2016-12-22 11:27:00.000000000 -0800
+++ packages/base/.local/share/zsh/functions/_ssh	2017-02-26 22:37:47.513422235 -0800
@@ -681,16 +681,21 @@
   fi
   if [[ -r $config ]]; then
     local key hosts host
-    while IFS=$'=\t ' read -r key hosts; do
-      if [[ "$key" == (#i)host ]]; then
-         for host in ${(z)hosts}; do
-            case $host in
-            (*[*?]*) ;;
-            (*) config_hosts+=("$host") ;;
-            esac
-         done
-      fi
-    done < "$config"
+    local filename configs=($config)
+    grep '^Include\b' "$config" | sed 's/\s\+/ /g' | cut -d' ' -f2 |
+    while read -r filename; do
+        config=$HOME/.ssh/$filename
+        while IFS=$'=\t ' read -r key hosts; do
+        if [[ "$key" == (#i)host ]]; then
+            for host in ${(z)hosts}; do
+                case $host in
+                (*[*?]*) ;;
+                (*) config_hosts+=("$host") ;;
+                esac
+            done
+        fi
+        done < "$config"
+    done
     if (( ${#config_hosts} )); then
       _wanted hosts expl 'remote host name' \
         compadd -M 'm:{a-zA-Z}={A-Za-z} r:|.=* r:|=*' "$@" $config_hosts

Re: patch for ssh completion

[Daniel Shahaf]

Jeremy Cantrell wrote on Tue, Feb 28, 2017 at 11:17:29 -0800:
> Hello!
> 
> I've found a tiny problem with the ssh completion script where any host
> entries defined in included configuration files do not appear as choices
> when completing. I've attached a patch that fixes it for me. Below is an
> example that illustrates the problem:
> 
> ~/.ssh/config:
> Include hosts
> 
> ~/.ssh/hosts:
> Host example
> HostName example.local

>From ssh_config(5):

     Include
             Include the specified configuration file(s).  Multiple pathnames
             may be specified and each pathname may contain glob(3) wildcards
             and, for user configurations, shell-like ‘~’ references to user
             home directories.  Files without absolute paths are assumed to be
             in ~/.ssh if included in a user configuration file or /etc/ssh if
             included from the system configuration file.  Include directive
             may appear inside a Match or Host block to perform conditional
             inclusion.

> Thanks,
> Jeremy

> --- /usr/share/zsh/functions/Completion/Unix/_ssh	2016-12-22 11:27:00.000000000 -0800
> +++ packages/base/.local/share/zsh/functions/_ssh	2017-02-26 22:37:47.513422235 -0800
> @@ -681,16 +681,21 @@
>    fi
>    if [[ -r $config ]]; then
>      local key hosts host
> +    local filename configs=($config)
> +    grep '^Include\b' "$config" | sed 's/\s\+/ /g' | cut -d' ' -f2 |
> +    while read -r filename; do
> +        config=$HOME/.ssh/$filename

This handles your example, but not the other possibilities documented in
the man page, e.g., absolute paths.  I expect using those would cause
errors on stderr.  (I can't test this since my ssh isn't new enough.)

The «\b» in grep is not in POSIX.  It could be changed to match
whitespace explicitly.  I assume we'll also want to use builtin
functionality such as ${(M)…:#Include*} instead of external executables.

Can filenames be quoted?  I.e., does «Include "foo"» include ~/.ssh/foo
or ~/.ssh/\"foo\"?

Can Include directives be nested?  If they can, it'd be nice to handle
that.  (But I don't think that's a blocker to accepting the patch)

>          while IFS=$'=\t ' read -r key hosts; do
>          if [[ "$key" == (#i)host ]]; then
>              for host in ${(z)hosts}; do
>                  case $host in
>                  (*[*?]*) ;;
>                  (*) config_hosts+=("$host") ;;
>                  esac
>              done
>          fi
>          done < "$config"
> +    done

Thanks for the patch.

Cheers,

Daniel

Re: patch for ssh completion

[Daniel Shahaf]

Jeremy — have you seen my review (quoted below)?  Would you be able to
revise the patch to address these points, at least the first two?

Process-wise, patches that apply to latest master are preferred, but
_ssh hasn't been changed since 5.3.1 so that's okay.

Cheers,

Daniel

Daniel Shahaf wrote on Thu, Mar 02, 2017 at 12:12:29 +0000:
> Jeremy Cantrell wrote on Tue, Feb 28, 2017 at 11:17:29 -0800:
> > Hello!
> > 
> > I've found a tiny problem with the ssh completion script where any host
> > entries defined in included configuration files do not appear as choices
> > when completing. I've attached a patch that fixes it for me. Below is an
> > example that illustrates the problem:
> > 
> > ~/.ssh/config:
> > Include hosts
> > 
> > ~/.ssh/hosts:
> > Host example
> > HostName example.local
> 
> From ssh_config(5):
> 
>      Include
>              Include the specified configuration file(s).  Multiple pathnames
>              may be specified and each pathname may contain glob(3) wildcards
>              and, for user configurations, shell-like ‘~’ references to user
>              home directories.  Files without absolute paths are assumed to be
>              in ~/.ssh if included in a user configuration file or /etc/ssh if
>              included from the system configuration file.  Include directive
>              may appear inside a Match or Host block to perform conditional
>              inclusion.
> 
> > Thanks,
> > Jeremy
> 
> > --- /usr/share/zsh/functions/Completion/Unix/_ssh	2016-12-22 11:27:00.000000000 -0800
> > +++ packages/base/.local/share/zsh/functions/_ssh	2017-02-26 22:37:47.513422235 -0800
> > @@ -681,16 +681,21 @@
> >    fi
> >    if [[ -r $config ]]; then
> >      local key hosts host
> > +    local filename configs=($config)
> > +    grep '^Include\b' "$config" | sed 's/\s\+/ /g' | cut -d' ' -f2 |
> > +    while read -r filename; do
> > +        config=$HOME/.ssh/$filename
> 
> This handles your example, but not the other possibilities documented in
> the man page, e.g., absolute paths.  I expect using those would cause
> errors on stderr.  (I can't test this since my ssh isn't new enough.)
> 
> The «\b» in grep is not in POSIX.  It could be changed to match
> whitespace explicitly.  I assume we'll also want to use builtin
> functionality such as ${(M)…:#Include*} instead of external executables.
> 
> Can filenames be quoted?  I.e., does «Include "foo"» include ~/.ssh/foo
> or ~/.ssh/\"foo\"?
> 
> Can Include directives be nested?  If they can, it'd be nice to handle
> that.  (But I don't think that's a blocker to accepting the patch)
> 
> >          while IFS=$'=\t ' read -r key hosts; do
> >          if [[ "$key" == (#i)host ]]; then
> >              for host in ${(z)hosts}; do
> >                  case $host in
> >                  (*[*?]*) ;;
> >                  (*) config_hosts+=("$host") ;;
> >                  esac
> >              done
> >          fi
> >          done < "$config"
> > +    done
> 
> Thanks for the patch.
> 
> Cheers,
> 
> Daniel

Re: patch for ssh completion

[Mark Nipper]

On 5 Mar 2017, d.s@daniel.shahaf.name wrote:
> Jeremy — have you seen my review (quoted below)?  Would you be able to
> revise the patch to address these points, at least the first two?
> 
> Process-wise, patches that apply to latest master are preferred, but
> _ssh hasn't been changed since 5.3.1 so that's okay.
> 
> Cheers,
> 
> Daniel

	I might have a cleaner patch for this which doesn't rely
on external commands.  It also handles the case of the relative
path for a user's .ssh directory, although it doesn't attempt to
deal with relative include directives in /etc/ssh.  It will
continue to read included files recursively, either from the
user's .ssh directory or via absolute paths.  I figured this was
the most common use case, so didn't attempt to deal with /etc/ssh
since it might not be in a standard location depending on how ssh
was compiled.

	I also wasn't sure if certain shell options should be
assumed for these completion functions.  I am assuming MULTIOS in
this particular example for the included file array to be read
correctly.  I tried to follow the existing line reading logic
already present in _ssh_hosts for weeding out the include
directives.

--- Completion/Unix/Command/_ssh.orig	2016-11-25 14:24:09.000000000 -0600
+++ Completion/Unix/Command/_ssh	2017-03-29 17:20:56.508325833 -0500
@@ -1,5 +1,7 @@
 #compdef ssh slogin=ssh scp ssh-add ssh-agent ssh-keygen sftp ssh-copy-id
 
+local -a config_includes
+
 # TODO: sshd, ssh-keyscan, ssh-keysign
 
 _ssh () {
@@ -662,6 +664,27 @@
   _combination -s '[:@]' my-accounts users-hosts users "$@"
 }
 
+_ssh_includes () {
+  local key includes
+  if [[ -r $@ ]]; then
+    config_includes+=("$@")
+  else
+    return 1
+  fi
+
+  while IFS=$'=\t ' read -r key includes; do
+    if [[ "$key" == (#i)include ]]; then
+      if [[ ${includes[1]} == / ]]; then
+        _ssh_includes $includes
+      else
+        _ssh_includes $HOME/.ssh/$includes
+      fi
+    fi
+  done < "$@"
+
+  return 0
+}
+
 _ssh_hosts () {
   local -a config_hosts
   local config
@@ -679,7 +702,7 @@
   else
     config="$HOME/.ssh/config"
   fi
-  if [[ -r $config ]]; then
+  if _ssh_includes $config; then
     local key hosts host
     while IFS=$'=\t ' read -r key hosts; do
       if [[ "$key" == (#i)host ]]; then
@@ -690,7 +713,7 @@
             esac
          done
       fi
-    done < "$config"
+    done < ${config_includes}
     if (( ${#config_hosts} )); then
       _wanted hosts expl 'remote host name' \
         compadd -M 'm:{a-zA-Z}={A-Za-z} r:|.=* r:|=*' "$@" $config_hosts

-- 
Mark Nipper
nipsy@bitgnome.net (XMPP)
-
Spelling mistakes left as an exercise for the reader.

Re: patch for ssh completion

[Daniel Shahaf]

Mark Nipper wrote on Wed, Mar 29, 2017 at 18:11:01 -0500:
> 	I also wasn't sure if certain shell options should be
> assumed for these completion functions.  I am assuming MULTIOS in
> this particular example for the included file array to be read
> correctly.

Completions options are run with the options from ${_comp_options} (in
compinit) in effect; you need an explicit «setopt localoptions multios»
in this case.

(@workers feel free to review/apply the patch; I might if I have time,
but I can't promise)

Re: patch for ssh completion

[Mark Nipper]

On 29 Mar 2017, Daniel Shahaf wrote:
> Mark Nipper wrote on Wed, Mar 29, 2017 at 18:11:01 -0500:
> > 	I also wasn't sure if certain shell options should be
> > assumed for these completion functions.  I am assuming MULTIOS in
> > this particular example for the included file array to be read
> > correctly.
> 
> Completions options are run with the options from ${_comp_options} (in
> compinit) in effect; you need an explicit «setopt localoptions multios»
> in this case.
> 
> (@workers feel free to review/apply the patch; I might if I have time,
> but I can't promise)

	I assume putting that into that one function then would
be the correct way to set that.  Added that to the patch:

--- Completion/Unix/Command/_ssh.orig	2016-11-25 14:24:09.000000000 -0600
+++ Completion/Unix/Command/_ssh	2017-03-30 08:53:03.103396162 -0500
@@ -1,5 +1,7 @@
 #compdef ssh slogin=ssh scp ssh-add ssh-agent ssh-keygen sftp ssh-copy-id
 
+local -a config_includes
+
 # TODO: sshd, ssh-keyscan, ssh-keysign
 
 _ssh () {
@@ -662,7 +664,29 @@
   _combination -s '[:@]' my-accounts users-hosts users "$@"
 }
 
+_ssh_includes () {
+  local key includes
+  if [[ -r $@ ]]; then
+    config_includes+=("$@")
+  else
+    return 1
+  fi
+
+  while IFS=$'=\t ' read -r key includes; do
+    if [[ "$key" == (#i)include ]]; then
+      if [[ ${includes[1]} == / ]]; then
+        _ssh_includes $includes
+      else
+        _ssh_includes $HOME/.ssh/$includes
+      fi
+    fi
+  done < "$@"
+
+  return 0
+}
+
 _ssh_hosts () {
+  setopt localoptions multios
   local -a config_hosts
   local config
   integer ind
@@ -679,7 +703,7 @@
   else
     config="$HOME/.ssh/config"
   fi
-  if [[ -r $config ]]; then
+  if _ssh_includes $config; then
     local key hosts host
     while IFS=$'=\t ' read -r key hosts; do
       if [[ "$key" == (#i)host ]]; then
@@ -690,7 +714,7 @@
             esac
          done
       fi
-    done < "$config"
+    done < ${config_includes}
     if (( ${#config_hosts} )); then
       _wanted hosts expl 'remote host name' \
         compadd -M 'm:{a-zA-Z}={A-Za-z} r:|.=* r:|=*' "$@" $config_hosts

-- 
Mark Nipper
nipsy@bitgnome.net (XMPP)
-
¸.·´¯`·.´¯`·.¸¸.·´¯`·.¸><(((º>