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.
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.
This would also allow customized logging when authenticating public keys.
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 ;)
Created attachment 2412 [details] Patch adding two more arguments to the AuthorizedKeysCommand
Created attachment 2416 [details] 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.
Created attachment 2417 [details] Patch adding environment variables to pass key and fingerprint to AuthorizedKeysCommand Patch updated to check for errors in setenv() calls.
(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
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.
(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.
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.
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.
Created attachment 2438 [details] Reworked version of the patch using environment variables I attached a reworked version of the patch that does not require a temporary file.
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.
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
put this on the todo list for openssh-6.7
Retarget incomplete bugs to 6.8 release.
These bugs are no longer targeted at the imminent 6.7 release
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!
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.
(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.
Created attachment 2478 [details] 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?
(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
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. > 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
(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!!!!
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. :)
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.
Created attachment 2522 [details] Reworked patch enabling optional %-expanded arguments Changes since (obsoleted) patch #2479: obey the new FingerprintHash option.
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.
Actually no, splitting the arguments is much better - all it would take is a misconfigured NSS backend to allow ';' and it's game over.
Created attachment 2544 [details] 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).
Created attachment 2545 [details] 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.
Created attachment 2546 [details] Revised diff Not sure what I was thinking banning nested quotes - they are fine. Revised diff to remove this restriction.
Created attachment 2549 [details] Revised diff Correct a mistake in a comment. When fixing up the command used in log messages, use argv directly
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.
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...
(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.
(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.
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 [details] in a testing environment and everything seems to work fine (we are only using %t and %k). Thanks again, hope this gets merged soon!
Created attachment 2556 [details] Add AuthorizedPrincipalsCommand This diff refactors the command execution out and uses it to add AuthorizedPrincipalsCommand (also w/ arguments)
Created attachment 2557 [details] Regression tests keys/principals command Revised regression tests; includes AuthorizedPrincipalsCommand
OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release.
Retarget to 6.9
*** Bug 2367 has been marked as a duplicate of this bug. ***
Patch applied. This will be in openssh-6.9
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?
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
(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!
Close all resolved bugs after 7.3p1 release