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
On 2021-01-29 at 00:06 +0100, ?ngel wrote:> (lots of notes about indentation on previos patch) >I attach a revised version of the patch addressing the indentation issues. It has only spacing changes from Werner's original patch. Best regards -------------- next part -------------- A non-text attachment was scrubbed... Name: 0001-Allow-sending-envrionment-variables-to-the-agent+spacing.patch Type: text/x-patch Size: 12950 bytes Desc: not available URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20210129/3da58d39/attachment-0001.bin>
On Fri, 29 Jan 2021 00:06, ?ngel said:> This seems backwards. I would expect command line to have hidher > priority than ssh_config, not ~/.ssh/config to be able to disable anI also wondered about this but it is clearly stated at the top of ssh_config(5)> 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")I agree, lowercase envvars and in particular "none" and "auto" should be rearly be exportable.>> + 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 theIts longer but fir sure more correct.>> 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 -You need to look at the previous condition: if ((*arg == '-' || *arg == '#') && arg[1]) { ERROR-RETURN> 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.You like in SendEnv. I decded against doing this because the number of envvars to send here should be pretty limited and does not need for more complex code.> + if (options->num_agent_env >= INT_MAX) { > > It is a bit strange to compare >= INT_MAX, since num_agent_env is anCopied from SendEnv> 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.I guess I did not read the hacking guide completely. Frankly I didn't expect that any software these days still uses tabs to compress the source. I consider invisible characters a no-go unless POSIX says to use them (Makefile ungliness). Thanks for the patch in your other mail.> 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).(See above.) Any other technical opinions? Salam-Shalom, Werner -- Die Gedanken sind frei. Ausnahmen regelt ein Bundesgesetz. -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 227 bytes Desc: not available URL: <http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20210307/944ad9b1/attachment.asc>