openssh bugs - Aug 2017 - [Bug 2755] New: [PATCH] sshd_config: allow directories in AuthorizedKeysFile=

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

            Bug ID: 2755
           Summary: [PATCH] sshd_config: allow directories in
                    AuthorizedKeysFile           Product: Portable OpenSSH
           Version: 7.5p1
          Hardware: All
                OS: Linux
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: ssh
          Assignee: unassigned-bugs at mindrot.org
          Reporter: lucab at debian.org

Created attachment 3028
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3028&action=edit
sshd_config: allow directories in AuthorizedKeysFile
This patch enhances AuthorizedKeysFile= to accept directory paths
in addition to single files.

It provides an include semantics similar to `.d` / `run-parts(8)`
approach, offering a consistent way for different entities to add
public keys to a given account without single-file contention.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Luca BRUNO <lucab at debian.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|ssh                         |sshd
                 CC|                            |lucab at debian.org

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #1 from Luca BRUNO <lucab at debian.org> ---
Gentle ping for a review.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #2 from Luca BRUNO <lucab at debian.org> ---
As I got no answers so far, this is another gentle ping for a review.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |djm at mindrot.org

--- Comment #3 from Damien Miller <djm at mindrot.org> ---
We're late in preparations for the 7.6 release. We'll look at this
after

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #4 from Luca BRUNO <lucab at debian.org> ---
I guess this fell off the radar, but I'm still considering having this
feature implemented and I'll be happy to go through a patch review.

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Jakub Jelen <jjelen at redhat.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jjelen at redhat.com

--- Comment #5 from Jakub Jelen <jjelen at redhat.com> ---
You can always use "AuthorizedKeysCommand", which will point to your
script that will pull the keys from all the files in specific
directory.

I am not sure if this is needed. It would make it all more complex,
though it would be a nice addition.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Benjamin Gilbert <bgilbert at backtick.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bgilbert at backtick.net

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Dusty Mabe <dusty at dustymabe.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dusty at dustymabe.com

--- Comment #6 from Dusty Mabe <dusty at dustymabe.com> ---
Hi Damien,

This is another item we are looking at that we'd like to use for our
new CoreOS efforts. We could use the AuthorizedKeysCommand workaround
as Jakub suggested in the short term, but we do think it would be
better in the long term if AuthorizedKeysFile= could accept
directories.

Would it be possible to evaluate if this is likely or unlikely to be
accepted long term?

Thanks!

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #7 from Dusty Mabe <dusty at dustymabe.com> ---
I forgot to add a link for context:
https://github.com/coreos/fedora-coreos-tracker/issues/139

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Erik Sj?lund <erik.sjolund at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |erik.sjolund at gmail.com

--- Comment #8 from Erik Sj?lund <erik.sjolund at gmail.com> ---
Comment on attachment 3028
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3028
sshd_config: allow directories in AuthorizedKeysFile
Comments about the patch authorized-keys-d.patch

It seems all file operations are done as the priviledged user.
It would be more secure to change identity to the login user before
doing this.

An example of changing identity can be seen in the function
user_key_allowed2() in the file ssh/auth2-pubkey.c at the line:

temporarily_use_uid(pw);

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #9 from Erik Sj?lund <erik.sjolund at gmail.com> ---
Regarding the order of files from readdir():

It seems readdir() has no guaranty about order: 

https://stackoverflow.com/questions/8977441/does-readdir-guarantee-an-order

I think the order could influence how access is given by the server if
multiple keys allow access in different ways.  Because of that it would
make sense to define an order in which the files are read. For
simplicity I would suggest alphabetical order.

Some limits are probably needed regarding the maximum number of
authorized files and the maximum filename length. Maybe those numbers
could be configurable.

The allowed set of characters in the filenames should probably also be
limited.
To make the alphabetical ordering easy to understand one could limit
the allowed characters to be for instance

a-z 

or maybe a bit more generous:

a-z
0-9
_

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Luca BRUNO <lucab at lucabruno.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3028|0                           |1
        is obsolete|                            |

--- Comment #10 from Luca BRUNO <lucab at lucabruno.net> ---
Created attachment 3411
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3411&action=edit
sshd_config: allow directories in AuthorizedKeysFile
Patch v2

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #11 from Luca BRUNO <lucab at lucabruno.net> ---
Thanks for the feedback! I've rebased and updated the patch to a v2
based on the comments above, please take a look.

I'm also keeping it mirrored at
https://github.com/openssh/openssh-portable/pull/70, in case that makes
it easier for review.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Luca BRUNO <lucab at lucabruno.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                URL|                            |https://github.com/openssh/
                   |                            |openssh-portable/pull/70

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

--- Comment #12 from Benjamin Gilbert <bgilbert at backtick.net> ---
ssh-key-dir <https://github.com/coreos/ssh-key-dir> was implemented as
a workaround for the missing ~/.ssh/authorized_keys.d support.  It runs
as an AuthorizedKeysCommand and has been shipping in Fedora CoreOS by
default for a couple years now.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3411|0                           |1
        is obsolete|                            |
             Status|NEW                         |ASSIGNED
                 CC|                            |dtucker at dtucker.net
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org
   Attachment #3846|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
Created attachment 3846
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3846&action=edit
glob(3) wildcards for AuthorizedKeysFile and AuthorizedPrincipalsFile

I think that accepting directories for these options is dangerous as a
mistake such as omitting a path component could result in using
unexpected files.

Instead I propose to allow glob(3) wildcards in these options. This
AFAIK achieves the same goal but is more explicit and exact about what
gets pulled in.

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3847|                            |ok?(dtucker at dtucker.net)
              Flags|                            |

--- Comment #14 from Damien Miller <djm at mindrot.org> ---
Created attachment 3847
  --> https://bugzilla.mindrot.org/attachment.cgi?id=3847&action=edit
Regress test

Regression test

-- 
You are receiving this mail because:
You are watching someone on the CC list of the bug.
You are watching the assignee of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3846|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Darren Tucker <dtucker at dtucker.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #3847|ok?(dtucker at dtucker.net)    |ok+
              Flags|                            |

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |3740

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
This has been committed and will be in OpenSSH 10.0


Referenced Bugs:

https://bugzilla.mindrot.org/show_bug.cgi?id=3740
[Bug 3740] Tracking bug for OpenSSH 10.0
-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.

https://bugzilla.mindrot.org/show_bug.cgi?id=2755

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

-- 
You are receiving this mail because:
You are watching the assignee of the bug.
You are watching someone on the CC list of the bug.