Sending envvars via ssh agent protocol

[Ángel]

On 2021-01-28 at 09:24 +0100, Werner Koch via Gnupg-devel wrote:
> The rationale for the "-" thingy is to allow a config file to
> override what for example the command line has already set.  The "#"
> can be used to disable a globally set option from the commandline or
> ~/.ssh/config.

This seems backwards. I would expect command line to have hidher
priority than ssh_config, not ~/.ssh/config to be able to disable an
explicit command line option.
However, this may be useful for ~/.ssh/configto override
/etc/ssh/ssh_config (or as a command line parameter -oAcceptEnv=-)

Also, I would suggest using none instead of -, as that's what other ssh
options use. (This might cause issues if you wanted to pass an
environment variable named "none", but the same problem already exists
for "auto")


On a quick review of the patch:


> @@ -313,11 +313,22 @@ static struct {
>         { "proxyjump", oProxyJump },
>         { "securitykeyprovider", oSecurityKeyProvider },
>         { "knownhostscommand", oKnownHostsCommand },
> +        { "agentenv", oAgentEnv },

There is an extra space identation here.


> +                               error("%s line %d: Invalid environment name.",
Maybe nitpicking, but on this error (appears twice) I would say
"Invalid name of environment variable". The environment would be the
whole block of variables and values, not just one variable.


> if (*arg == '-') {

> if (*arg == '#') {

You are comparing against the first character of the argument.
Per your description I would expect that you compared that the whole
was that, not just that it began with # or -


And particularly, I can easily see how one might want to prefix an
environment variable with a minus to *substract* it from the set of
accepted vars.


+                        if (options->num_agent_env >= INT_MAX) {

It is a bit strange to compare >= INT_MAX, since num_agent_env is an
int, but after reviewing, you only need the == comparison, so probably
ok.

There are more indentation sheningans around this block.


> +++ b/readconf.h
> @@ -126,6 +126,10 @@ typedef struct {
>  	char   **send_env;
>  	int     num_setenv;
>  	char   **setenv;
> +        int     no_more_agent_env;
> +  	int     num_agent_env;
> +	char   **agent_env;
> +

Bad indentation. send_env, num_setenv and setenv are indented with a
tab, no_more_agent_env with 8 spaces, num_agent_env with 3 spaces and
agent_env with a tab.


The fixme comments of ssh-add.c and ssh-keygen.c also use 8 spaces
instead of a tab (but these should probably end up implemented).

> +++ b/sshconnect.c
> @@ -1718,6 +1718,12 @@ maybe_add_key_to_agent(const char *authfile,
> struct sshkey *private,
>  	}
>  	if (sshkey_is_sk(private))
>  		skprovider = options.sk_provider;
> +        if ((r = ssh_send_agent_env (auth_sock, options.agent_env,
> +                                     options.num_agent_env)) != 0) {
> +                debug("agent does not support AgentEnv: (%d)", r);
> +                close(auth_sock);
> +                return;
> +        }

Indentation with 8 spaces, not with tabs

> +++ b/sshconnect2.c
> 
> +/* Helper for pubkey_prepare.  */
> +static int
> +authentication_socket(int *fdp)

More indentation errors (there is a mixture in the function itself)


Best regards