[Bug 2081] extend the parameters to the AuthorizedKeysCommand

[bugzilla-daemon at mindrot.org]

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.