openssh bugs - Mar 2013 - [Bug 2081] New: extend the parameters to the AuthorizedKeysCommand

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

            Bug ID: 2081
           Summary: extend the parameters to the  AuthorizedKeysCommand
    Classification: Unclassified
           Product: Portable OpenSSH
           Version: 6.2p1
          Hardware: All
                OS: All
            Status: NEW
          Severity: enhancement
          Priority: P5
         Component: sshd
          Assignee: unassigned-bugs at mindrot.org
          Reporter: calestyo at scientia.net

Hi.

First, great to see bug #1663 fixed :)

AFAIU, right now you only supply one parameter to the command, the
username being authenticated for.


Why not adding further stuff, especially the command?
That would allow one to return a key list (possibly empty) depending on
the command the user wants to execute.
Especially handy to program e.g. kind of a command restrictor, that
matches the command string (with arguments) against white and black
lists of regular expressions.

Not sure if this would work with control channel muxes though, IIRC,
they make the command fixed for the mux, right?


But also other information, like the selected auth method(s) and cipher
algos could be interesting, e.g. a program could perhaps allow only a
few safe commands with methods/algos being less secure.

etc. pp.


Cheers,
Chris.

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

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

Christoph Anton Mitterer <calestyo at scientia.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://bugzilla.mindrot.or
                   |                            |g/show_bug.cgi?id=1663

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

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

Markus Friedl <markus at openbsd.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |markus at openbsd.org

--- Comment #1 from Markus Friedl <markus at openbsd.org> ---
matching the command is not possible, since sessions
happen much later, after authentication.

but it would make sense to pass the users key to the command.

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

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

Daniel Kahn Gillmor <dkg at fifthhorseman.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dkg at fifthhorseman.net

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

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

Gabor K Horvath <gahorvath at npsh.hu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |gahorvath at npsh.hu

--- Comment #2 from Gabor K Horvath <gahorvath at npsh.hu> ---
This would also allow customized logging when authenticating public
keys.

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

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

Florian Zimmermann <flo at chaos-wg.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |flo at chaos-wg.net
            Version|6.2p1                       |6.5p1

--- Comment #3 from Florian Zimmermann <flo at chaos-wg.net> ---
Hey,

I like the idea of providing some more input to the
AuthorizedKeysCommand since it seems extremely useful in the Github
case, i.e. one git user and tons of public keys that one has to scan
through if the AuthorizedKeysCommand blindly dumps all public keys for
the git user.

I looked a bit further into the suggestion of including the fingerprint
into the arguments for the AuthorizedKeysCommand
(http://lists.mindrot.org/pipermail/openssh-unix-dev/2013-June/031457.html)
and (hopefully) fixed the memory leak that would have been introduced
by the suggested patch.

This patch passes two additional arguments to the AuthorizedKeysCommand
(the first argument -- the user being authenticated -- remains):

- the type of the key used for authentication.
  This is one of the strings defined in the keytypes array in key.c,
e.g. "ssh-rsa", "ssh-dss" or "ssh-unknown"

- the MD5 fingerprint (hex formatted) of the key used for
authentication.

I tested this on a virtual machine running Debian Wheezy and it seemed
to work pretty well...


I'm not sure whether passing the entire key that is used for
authentication to the AuthorizedKeysCommand is in any way better or
worse than just using the key type and fingerprint
(http://lists.mindrot.org/pipermail/openssh-unix-dev/2013-January/030967.html).
It seemed like a lot more work though ;)

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

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

--- Comment #4 from Florian Zimmermann <flo at chaos-wg.net> ---
Created attachment 2412
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2412&action=edit
Patch adding two more arguments to the AuthorizedKeysCommand

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

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

Scott Duckworth <sduckwo at clemson.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sduckwo at clemson.edu

--- Comment #5 from Scott Duckworth <sduckwo at clemson.edu> ---
Created attachment 2416
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2416&action=edit
Patch adding environment variables to pass key and fingerprint to
AuthorizedKeysCommand

Per the discussion about this on the developer's mailing list
(http://lists.mindrot.org/pipermail/openssh-unix-dev/2014-March/032341.html),
here is an alternate to the already proposed patch that sends the key
and key fingerprint to AuthorizedKeysCommand via environment variables.

This maintains compatibility with existing programs being used in
AuthorizedKeysCommand which require exactly one command line parameter
(the username).  Additionally, this only modifies the child process
which is forked off to exec the AuthorizedKeysCommand, so there is no
risk of introducing a memory leak in sshd.

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

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

Scott Duckworth <sduckwo at clemson.edu> changed:

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

--- Comment #6 from Scott Duckworth <sduckwo at clemson.edu> ---
Created attachment 2417
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2417&action=edit
Patch adding environment variables to pass key and fingerprint to
AuthorizedKeysCommand

Patch updated to check for errors in setenv() calls.

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

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

--- Comment #7 from Markus Friedl <markus at openbsd.org> ---
(In reply to Scott Duckworth from comment #6)
> Created attachment 2417 [details]
> Patch adding environment variables to pass key and fingerprint to
> AuthorizedKeysCommand
> 
> Patch updated to check for errors in setenv() calls.
I don't want to start using the environment here.

I prefer Damien's suggestion of using %-expanding
to setup the arguments that are passed to the
AuthorizedKeysCommand, e.g.

AuthorizedKeysCommand /path/to/command %u %k

for passing the username and the key, for example.

-m

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

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

--- Comment #8 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> ---
Markus, can you explain a bit more about why you don't want to start
using the environment variable here?

Also, i don't remember seeing damien's suggestion -- can you point to
it?  I'd be happy to read that discussion.

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

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

--- Comment #9 from Markus Friedl <markus at openbsd.org> ---
(In reply to Daniel Kahn Gillmor from comment #8)
> Markus, can you explain a bit more about why you don't want to start
> using the environment variable here?
> 
> Also, i don't remember seeing damien's suggestion -- can you point
> to it?  I'd be happy to read that discussion.
environment is added to keep the old command line conventions.

damien's suggestion of adding %-support for the command line
make this 2nd way of passing information unnecessary.

all relevant information should be passed in the same way.
we should avoid mixing argv[] and environ[] unless necessary.
it's too error prone, especially if passing everything on the command
like
is simpler.

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

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

--- Comment #10 from Daniel Kahn Gillmor <dkg at fifthhorseman.net> ---
in discussion on the mailing list, i also pointed out that the argv are
more likely to leak to other processes on the host than the
environment:

  http://marc.info/?l=openssh-unix-dev&m=139553657027791&w=2

If you think we should make everything available in the same space,
maybe we should also make the user name available in the environment?

iirc, the AuthorizedKeysCommand was initially implemented as a single
executable program with no configurable extra arguments,
shell-metacharacters, percent-escaping, or anything else complicated to
try to avoid creating a footgun for administrators who might put
something over-fancy in the config file, since this command will be
triggered by arbitrary remote network access (because it happens before
authentication/authorization).

Keeping the interface as minimally-configurable as possible seems to
try to keep to that same goal.

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

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

Paul Kilgo <pkilgo at clemson.edu> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pkilgo at clemson.edu

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

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

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #11 from Damien Miller <djm at mindrot.org> ---
This patch seems to do a key_write() to a temporary file and then reads
it back to get the key in a text format. This is pretty silly - it
would be better to duplicate or factor out the innards of key_write()
to return a text representation of a public key in string format
directly.

I wouldn't bother doing this just yet - there are some large key.c
changes coming up in the next couple of weeks that will collide with
any duplication or refactoring here.

-- 
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=2081

info at cryptocrack.de changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |info at cryptocrack.de

--- Comment #12 from info at cryptocrack.de ---
Created attachment 2438
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2438&action=edit
Reworked version of the patch using environment variables

I attached a reworked version of the patch that does not require a
temporary file.

-- 
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=2081

--- Comment #13 from Damien Miller <djm at mindrot.org> ---
After discussing it with Markus and thinking about it some more, IMO it
would be better to extend the syntax of AuthorizedKeysCommand to accept
some arguments that are %-expanded. E.g.

AuthorizedKeysCommand /usr/local/bin/auth-keys %u %t %f %k

would yield a command of:

/usr/local/bin/auth-keys [username] [key-type] [fingerprint] [key blob]

If no argument was specified, then the current single argument of the
username (i.e. %u) would be passed.

-- 
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=2081

--- Comment #14 from Lukas Fleischer <info at cryptocrack.de> ---
Sounds good to me as well. Note that there was a (kind of) concensus to
use environment variables on the ML, see e.g. [1], but I am not sure
whether there were any OpenSSH developers involved in that discussion.

I am willing to write a patch to implement %-expanded arguments in
AuthorizedKeysCommand, however, since this patch is likely to be more
complex (and invasive) than the current approach, it would be great to
know whether the area this patch is touching is affected by the "large
key.c changes coming up in the next couple of weeks". It would be a
shame if the same work would have to be done twice due to massive
refactorings.

[1]
http://lists.mindrot.org/pipermail/openssh-unix-dev/2014-March/032370.html

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2226

--- Comment #15 from Damien Miller <djm at mindrot.org> ---
put this on the todo list for openssh-6.7

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2266

--- Comment #16 from Damien Miller <djm at mindrot.org> ---
Retarget incomplete bugs to 6.8 release.

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2226                        |

--- Comment #17 from Damien Miller <djm at mindrot.org> ---
These bugs are no longer targeted at the imminent 6.7 release

-- 
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=2081

Alon Bar-Lev <alon.barlev at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |alon.barlev at gmail.com

-- 
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=2081

--- Comment #18 from Alon Bar-Lev <alon.barlev at gmail.com> ---
Hi,

I will be glad to help if required to make it confirm comment#13.

Please also take into account to align the ForceCommand option to
recieve similar arguments.

Thanks!

-- 
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=2081

Sami Hartikainen <hasa100 at hotmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hasa100 at hotmail.com

--- Comment #19 from Sami Hartikainen <hasa100 at hotmail.com> ---
Created attachment 2477
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2477&action=edit
Patch enabling optional %-expanded arguments to AuthorizedKeysCommand

A patch implementing the %-expanded optional arguments to
AuthorizedKeysCommand as described in comment 13: %u, %t, %f and %k are
expanded to username, key type, fingerprint and (base64-encoded) key,
respectively. If none of the above arguments are specified, username is
provided to the command as before.

-- 
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=2081

--- Comment #20 from Alon Bar-Lev <alon.barlev at gmail.com> ---
(In reply to Sami Hartikainen from comment #19)
> Created attachment 2477 [details]
> Patch enabling optional %-expanded arguments to AuthorizedKeysCommand
> 
> A patch implementing the %-expanded optional arguments to
> AuthorizedKeysCommand as described in comment 13: %u, %t, %f and %k
> are expanded to username, key type, fingerprint and (base64-encoded)
> key, respectively. If none of the above arguments are specified,
> username is provided to the command as before.
Hi!

Can you please also add %h as in other commands?

Not sure why you do not use percent_expand unconditionallty for all
parameters, the program will get no key and can response in any way it
likes.

Please use execv instead of execl and checking for # of arguments.

I am unsure that argv[4] is valid as I can add many options...

 %u %u %k %k xxx yy xxx

You should translate all.

-- 
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=2081

Sami Hartikainen <hasa100 at hotmail.com> changed:

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

--- Comment #21 from Sami Hartikainen <hasa100 at hotmail.com> ---
Created attachment 2478
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2478&action=edit
Reworked patch enabling optional %-expanded arguments

Reworked the patch to use execv() instead on execl(), and to _not_
impose a hard-coded limit for the number of arguments.

Not sure if the original intent was to allow any arguments beyond the
specified four %-expanded ones; the patch currently allows this.

@Alon: key to fingerprint / base64 computations are done only if needed
because they may fail, causing the AuthorizedKeysCommand to be skipped
even when those parameters were never requested.

@Alon: you requested %h expansion as in... home directory? Or perhaps
you meant host?

-- 
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=2081

--- Comment #22 from Alon Bar-Lev <alon.barlev at gmail.com> ---
(In reply to Sami Hartikainen from comment #21)
> Created attachment 2478 [details]
> Reworked patch enabling optional %-expanded arguments
Thanks!
> Reworked the patch to use execv() instead on execl(), and to _not_
> impose a hard-coded limit for the number of arguments.
> 
> Not sure if the original intent was to allow any arguments beyond
> the specified four %-expanded ones; the patch currently allows this.
for example, I have utility that can get parameter to its configuration
file...
> @Alon: key to fingerprint / base64 computations are done only if
> needed because they may fail, causing the AuthorizedKeysCommand to
> be skipped even when those parameters were never requested.
currently there is no such check, right? so this actually reduces the
events in which the command is executed, no?
> @Alon: you requested %h expansion as in... home directory? Or
> perhaps you meant host?
home directory :)
sshd_config sets %h to home directory always.

comments:

1. not sure argv_to_string is required, at other places within codebase
there is no memory building nor static, just set of debug. example:
sshd.c, scp.c,  but if this usable, then introduce this function in
misc and embed it to all places execv is used.

2. still open issue is if we need to skip calling the utility if no
public key, I leave this to openssh developers to decide, I think we
should execute with empty value.

3. I believe that cleaning up should goto out, for exmaple:
---
+            error("AuthorizedKeysCommand %%k parameter expansion
failed");
+            free(key_fp);
+            return 0;
---
is not good as it cleans as it goes.

4. I do think that regardless we allow variable # of parameters we can
have sane limit and avoid dynamic memory management... pick 10, 20,
30... not that important.

5. I suggest the arg loop to be different...
---
char **argv[20];
int i;
command = cp = percent_expand(options.authorized_keys_command,...);
memset(argv, 0, sizeof(argv));
i = 0;
while(i < sizeof(argv)/sizeof(argv[0])-1 && cp != NULL) {
    argv[i++] = strdelim(&cp);
}
if (argv[1] == NULL)
    argv[1] = user_pw->pw_name;
authorized_keys_command_path = argv[0];
---
or dynamic, replace while above with:
---
while(cp != NULL) {
    if (i == argc-1) {
        argc += 10; /* inline += is not used within this code base */
        argv = xrealloc(argv, argc, sizeof(argv[0]));
    }
    argv[i++] = strdelim(&cp);
    argv[i] = NULL;
}
---

6. not sure the sshkey_to_base64 is first requirement to perform that
conversion... maybe something should be shared with ssh-keygen. but one
good discussion is if we want to provide the certificate to this
utility as well, I think it will be wounderful.

Regards,
Alon

-- 
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=2081

Sami Hartikainen <hasa100 at hotmail.com> changed:

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

--- Comment #23 from Sami Hartikainen <hasa100 at hotmail.com> ---
Created attachment 2479
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2479&action=edit
Reworked patch enabling optional %-expanded arguments

Revised based on feedback, e.g. %h expansion added.
> 2. still open issue is if we need to skip calling the
> utility if no public key, I leave this to openssh
> developers to decide, I think we should execute with
> empty value.
I would like to hear comments from other people on this as
well. But consider an AuthorizedKeysCommand of:

    /usr/local/sbin/myauth --user %u --key %k non-option-arg

If %k is missing (due to sshkey_to_base64() failing),
the 'non-option-arg' will be read as the option value for
--key, possibly breaking the 'myauth' utility.
> 4. I do think that regardless we allow variable # of parameters
> we can have sane limit and avoid dynamic memory management...
Disagree on this, different limits on different places are a source
of hard-to-track bugs.
> 6. not sure the sshkey_to_base64 is first requirement to perform
> that conversion... maybe something should be shared with ssh-keygen.
sshkey_write() is almost the same, so perhaps the 'guts' of it could
be refactored to be usable for this.

--
Sami

-- 
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=2081

--- Comment #24 from Alon Bar-Lev <alon.barlev at gmail.com> ---
(In reply to Sami Hartikainen from comment #23)
> Created attachment 2479 [details]
> Reworked patch enabling optional %-expanded arguments
> 
> Revised based on feedback, e.g. %h expansion added.
> 
> > 2. still open issue is if we need to skip calling the
> > utility if no public key, I leave this to openssh
> > developers to decide, I think we should execute with
> > empty value.
> 
> I would like to hear comments from other people on this as
> well. But consider an AuthorizedKeysCommand of:
> 
>     /usr/local/sbin/myauth --user %u --key %k non-option-arg
> 
> If %k is missing (due to sshkey_to_base64() failing),
> the 'non-option-arg' will be read as the option value for
> --key, possibly breaking the 'myauth' utility.
I thought there is other reason for that... :)

If you first split it based on delimiters, then substitute each then
you will be ok.
> > 6. not sure the sshkey_to_base64 is first requirement to perform
> > that conversion... maybe something should be shared with ssh-keygen.
> 
> sshkey_write() is almost the same, so perhaps the 'guts' of it
could
> be refactored to be usable for this.
this is for openssh developers to instruct.

minor comments:

xrealloc(argv, argc, sizeof(char *));

please use the type of argv[0] instead char*.

thanks!!!!

-- 
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=2081

Micah Cowan <micah at addictivecode.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |micah at addictivecode.org

--- Comment #25 from Micah Cowan <micah at addictivecode.org> ---
Heh, apparently I just submitted a similar patch (written at the
request of my employer) without realizing this issue already existed,
with patches:
http://lists.mindrot.org/pipermail/openssh-unix-dev/2014-October/033026.html
... Obviously, we'd find this feature useful as well. :)

I don't know that we care about whether things are passed via CLI
versus environment, though a full key value strikes me as a rather
larger-than-usual argument.

Am I misreading the 2014-06-06 patch, or is there a memory leak between
the call to key_fingerprint and key_write_str (needing to free the
string created by key_fingerprint before overwriting the pointer with a
new allocation from key_write_str)? Not that it really makes a
difference when we're just about to call execl anyway. :)

-- 
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=2081

--- Comment #26 from Lukas Fleischer <info at cryptocrack.de> ---
What's the status of this one? We have at least five different patches
with different implementations of the same functionality posted to the
bug tracker and the mailing list by now. Any further comments or
guidance from any of the OpenSSH developers will be appreciated.

-- 
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=2081

Sami Hartikainen <hasa100 at hotmail.com> changed:

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

--- Comment #27 from Sami Hartikainen <hasa100 at hotmail.com> ---
Created attachment 2522
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2522&action=edit
Reworked patch enabling optional %-expanded arguments

Changes since (obsoleted) patch #2479: obey the new FingerprintHash
option.

-- 
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=2081

--- Comment #28 from Damien Miller <djm at mindrot.org> ---
I'm not sure about splitting arguments in sshd, I think I'd prefer to
just pass the whole AuthorizedKeysCommand to the shell like the current
code (and all other *command options in ssh/sshd).

Beyond the username (which must exist in the system password database)
and key text (which cannot contain shell metacharacters), there are no
attacker-controllable values in the command and so it should be quite
safe to pass to the shell.

-- 
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=2081

--- Comment #29 from Damien Miller <djm at mindrot.org> ---
Actually no, splitting the arguments is much better - all it would take
is a misconfigured NSS backend to allow ';' and it's game over.

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Attachment #2412|0                           |1
        is obsolete|                            |
   Attachment #2417|0                           |1
        is obsolete|                            |
   Attachment #2438|0                           |1
        is obsolete|                            |
   Attachment #2522|0                           |1
        is obsolete|                            |
             Status|NEW                         |ASSIGNED
           Assignee|unassigned-bugs at mindrot.org |djm at mindrot.org

--- Comment #30 from Damien Miller <djm at mindrot.org> ---
Created attachment 2544
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2544&action=edit
revised diff

Here's a revised diff.

Use a more exact argument splitting that copes with a couple of escaped
characters and bails if there are nested quotes.

Refactor sshkey.c a bit - if we are going to have a sshkey_to_base64()
function then we might as well use it in sshkey_write()

Set up a minimal environment for the AuthorizedKeysCommand, instead of
inheriting everything from sshd (which may well have been started by a
user with an unclean environment).

-- 
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=2081

--- Comment #31 from Damien Miller <djm at mindrot.org> ---
Created attachment 2545
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2545&action=edit
adjust regression test

Extend the regression test to exercise expanded tokens, verify that
legacy behaviour continues to work and check that the environment is
sanitised.

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #32 from Damien Miller <djm at mindrot.org> ---
Created attachment 2546
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2546&action=edit
Revised diff

Not sure what I was thinking banning nested quotes - they are fine.
Revised diff to remove this restriction.

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #33 from Damien Miller <djm at mindrot.org> ---
Created attachment 2549
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2549&action=edit
Revised diff

Correct a mistake in a comment.
When fixing up the command used in log messages, use argv directly

-- 
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=2081

--- Comment #34 from Christoph Anton Mitterer <calestyo at scientia.net>
---
Hey.

It's great to see this having come so far :)

The only thing I still miss is, that the command+arguments the user
tries to invoke (which was my original request ;) ) seems to be not
included as one of the parameters that can be given to the authorized
keys command.
Is there any reason against having that?

Of course one might perhaps want further things like which
forwardings/tunnels a user tries to set up and that like.


Cheers,
Chris.

-- 
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=2081

--- Comment #35 from Christoph Anton Mitterer <calestyo at scientia.net>
---
Oh and perhaps one more thing:

I'm a bit concerned about the fingerprint being used.
Wouldn't it be better to require specification of the algo e.g.
something like %f(md5) or %f(sh512)?

God knows how people will actually use these features, some of them
might be security critical,... and having MD5 used in such cases causes
some concerns...

-- 
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=2081

--- Comment #36 from Sami Hartikainen <hasa100 at hotmail.com> ---
(In reply to Christoph Anton Mitterer from comment #34)
> The only thing I still miss is, that the command+arguments the user
> tries to invoke (which was my original request ;) ) seems to be not
> included as one of the parameters that can be given to the
> authorized keys command.
> Is there any reason against having that?
See comment #1.

-- 
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=2081

--- Comment #37 from Sami Hartikainen <hasa100 at hotmail.com> ---
(In reply to Christoph Anton Mitterer from comment #35)
> I'm a bit concerned about the fingerprint being used.
> Wouldn't it be better to require specification of the algo e.g.
> something like %f(md5) or %f(sh512)?
This is defined by the FingerprintHash configuration option.

-- 
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=2081

--- Comment #38 from Lukas Fleischer <info at cryptocrack.de> ---
Thanks, Damien! Unfortunately, I am too busy to review the whole patch
in detail but it looks good at a glance. I replaced the patch we are
currently using with attachment 2549 in a testing environment and
everything seems to work fine (we are only using %t and %k). Thanks
again, hope this gets merged soon!

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #39 from Damien Miller <djm at mindrot.org> ---
Created attachment 2556
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2556&action=edit
Add AuthorizedPrincipalsCommand

This diff refactors the command execution out and uses it to add
AuthorizedPrincipalsCommand (also w/ arguments)

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #40 from Damien Miller <djm at mindrot.org> ---
Created attachment 2557
  --> https://bugzilla.mindrot.org/attachment.cgi?id=2557&action=edit
Regression tests keys/principals command

Revised regression tests; includes AuthorizedPrincipalsCommand

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|2266                        |

--- Comment #41 from Damien Miller <djm at mindrot.org> ---
OpenSSH 6.8 is approaching release and closed for major work. Retarget
these bugs for the next release.

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |2360

--- Comment #42 from Damien Miller <djm at mindrot.org> ---
Retarget to 6.9

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |elbarto at bocal.org

--- Comment #43 from Damien Miller <djm at mindrot.org> ---
*** Bug 2367 has been marked as a duplicate of this bug. ***

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

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

--- Comment #44 from Damien Miller <djm at mindrot.org> ---
Patch applied. This will be in openssh-6.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=2081

--- Comment #45 from Alon Bar-Lev <alon.barlev at gmail.com> ---
question: now that the hash algorithm is a configuration option, won't
it be nice to be able to send it to the command as well so it know what
hash to match?

-- 
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=2081

--- Comment #46 from Damien Miller <djm at mindrot.org> ---
The hash is now prefixed by the algorithm, so there shouldn't be much
need to pass it separately.

E.g. with AuthorizedKeysCommand="/usr/bin/true %f"

debug3: subprocess: AuthorizedKeysCommand command "/usr/bin/true
SHA256:mjix8AbOTsneAJJ4r5C0IPieXl9c2qoiYIWs4J65/+g" running as djm

-- 
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=2081

--- Comment #47 from Alon Bar-Lev <alon.barlev at gmail.com> ---
(In reply to Damien Miller from comment #46)
> The hash is now prefixed by the algorithm, so there shouldn't be
> much need to pass it separately.
> 
> E.g. with AuthorizedKeysCommand="/usr/bin/true %f"
> 
> debug3: subprocess: AuthorizedKeysCommand command "/usr/bin/true
> SHA256:mjix8AbOTsneAJJ4r5C0IPieXl9c2qoiYIWs4J65/+g" running as djm
thanks! that's great!

-- 
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=2081

Damien Miller <djm at mindrot.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #48 from Damien Miller <djm at mindrot.org> ---
Close all resolved bugs after 7.3p1 release

-- 
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.