Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 2, 2023, 20:56 UTC
Message-ID: <xmqq5ycju6q1.fsf@gitster.g>
In-Reply-To: <patch-v2-6.6-686febb8cdc-20230202T093706Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 22 quoted lines

> Before [1] we'd force the "imap.host" to be set, even if the
> "imap.tunnel" was set, and then proceed to not use the "host" for
> establishing a connection, as we'd use the tunneling command.
>
> However, we'd still use the "imap.host" if it was set as the "host"
> field given to the credential helper, and in messages that were shared
> with the non-tunnel mode, until a preceding commit made these OpenSSL
> codepaths tunnel-only.
>
> Let's always give "host=tunnel" to the credential helper when in the
> "imap.tunnel" mode, and rephrase the relevant messages to indicate
> that we're tunneling. This changes the existing behavior, but that
> behavior was emergent and didn't make much sense. If we were using
> "imap.tunnel" the value in "imap.host" might be entirely unrelated to
> the host we're tunneling to. Let's not pretend to know more than we do
> in that case.
>
> 1. 34b5cd1fe9f (Don't force imap.host to be set when imap.tunnel is
>    set, 2008-04-22)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---

I agree with the above flow of thought in principle, but I wonder if "tunnel" is distinct enough to allow credential helpers to tell that they are dealing with a "tunnel", and not a host whose name happens to be "tunnel". Would it help to use a token that can never be a valid hostname instead, I wonder?

Show 6 quoted lines

> @@ -1004,7 +1004,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>  				if (!CAP(AUTH_CRAM_MD5)) {
>  					fprintf(stderr, "You specified "
>  						"CRAM-MD5 as authentication method, "
> -						"but %s doesn't support it.\n", srvc->host);
> +						"but tunnel doesn't support it.\n");

Do we need some article before "tunnel"?

Show 6 quoted lines

>  			if (CAP(NOLOGIN)) {
> -				fprintf(stderr, "Skipping account %s@%s, server forbids LOGIN\n",
> -					srvc->user, srvc->host);
> +				fprintf(stderr, "Skipping account %s, server forbids LOGIN\n",
> +					srvc->user);
>  				goto bail;

OK. We are talking to whatever "tunnel" is that was spawned to talk somewhere we do not have a way to know, so trying to say <this user> at <that host> is futile. Makes sense.

Show 10 quoted lines

> -	if (!server.host) {
> -		if (!server.tunnel) {
> -			fprintf(stderr, "no imap host specified\n");
> -			return 1;
> -		}
> -		server.host = "tunnel";
> +	if (!server.host && !server.tunnel) {
> +		fprintf(stderr, "no imap host specified\n");
> +		return 1;
>  	}

OK, this is a natural consequence that we no longer abuse server.host in the tunneling case. Makes sense.

Thanks.  Will queue.