Re: [PATCH] imap-send.c: set CURLOPT_USE_SSL to CURLUSESSL_TRY

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 6, 2015, 20:19 UTC
Message-ID: <xmqqiogjoe67.fsf@gitster.dls.corp.google.com>
In-Reply-To: <31f38398d53bcfb1d659cd248db522e@74d39fa044aa309eaea14b9f57fe79c>

"Kyle J. McKay" <mackyle@gmail.com> writes:

Show 16 quoted lines

> According to the cURL documentation for the CURLOPT_USE_SSL option,
> it is only used with plain text protocols that get upgraded to SSL
> using the STARTTLS command.
>
> The server.use_ssl variable is only set when we are using a protocol
> that is already SSL/TLS (i.e. imaps), so setting CURLOPT_USE_SSL
> when the server.use_ssl variable is set has no effect whatsoever.
>
> Instead, set CURLOPT_USE_SSL to CURLUSESSL_TRY when the server.use_ssl
> variable is NOT set so that cURL will attempt to upgrade the plain
> text connection to SSL/TLS using STARTTLS in that case.
>
> This much more closely matches the behavior of the non-cURL code path.
>
> Signed-off-by: Kyle J. McKay <mackyle@gmail.com>
> ---

It sounds sensible from the description of the approach but I'd prefer independent opinion to sanity-check me. Will queue while waiting for others to speak up.

Thanks.

Show 24 quoted lines

>
> *** PATCH IS AGAINST NEXT ***
>
> In particular, this patch requires br/imap-send-via-libcurl
>
>
>  imap-send.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 4dfe4c25..5251b750 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -1421,8 +1421,8 @@ static CURL *setup_curl(struct imap_server_conf *srvc)
>  		strbuf_release(&auth);
>  	}
>  
> -	if (server.use_ssl)
> -		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_ALL);
> +	if (!server.use_ssl)
> +		curl_easy_setopt(curl, CURLOPT_USE_SSL, (long)CURLUSESSL_TRY);
>  
>  	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYPEER, server.ssl_verify);
>  	curl_easy_setopt(curl, CURLOPT_SSL_VERIFYHOST, server.ssl_verify);