Re: [PATCH v1 1/1] send-mail: add client certificate options

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 20, 2026, 16:35 UTC
Message-ID: <xmqqh5rbz83b.fsf@gitster.g>
In-Reply-To: <20260220081717.555185-2-dxdt@dev.snart.me>

David Timber <dxdt@dev.snart.me> writes:

Show 5 quoted lines

> +sendemail.smtpSSLClientCert::
> +	Path to a client certificate file to present to the SMTP server.
> +
> +sendemail.smtpSSLClientKey::
> +	Path to the client private key file.

Do we want to add "that corresponds to the smtpSSLClientCert" at the end, perhaps?

Show 11 quoted lines

> diff --git a/Documentation/git-send-email.adoc b/Documentation/git-send-email.adoc
> index ebe8853e9f..51177508c1 100644
> --- a/Documentation/git-send-email.adoc
> +++ b/Documentation/git-send-email.adoc
> @@ -290,6 +290,23 @@ must be used for each option.
>  	variable, if set, or the backing SSL library's compiled-in default
>  	otherwise (which should be the best choice on most platforms).
>  
> +--smtp-ssl-client-cert <path>::
> +	Path to a client certificate file to present to the SMTP server. This option
> +	can be used when the server verifies the certificate from the client. The

Shouldn't there be a word "require" somewhere in the above to clarify why a user may want to use this option? A server may optionally verify a certificate only when it is given one, but if it lets us do what we want without such verification, we do not have much incentive to give them a certificate.

> +	format could be in either PKCS12 or PEM. In the latter case, the private key
> +	can be specified using `--smtp-ssl-client-key` option. More more

Is that "can be specified" or "should be specified"? "More more" -> "For more".

Show 11 quoted lines

> +	detail, see
> +	https://metacpan.org/pod/IO::Socket::SSL#SSL_cert_file-|-SSL_cert-|-SSL_key_file-|-SSL_key
> +	Defaults to the value of the `sendemail.smtpSSLClientCert` configuration
> +	variable, if set.
> +
> +--smtp-ssl-client-key <path>::
> +	Optional path to the client private key file. If this is not given and a
> +	PKCS12 certificate file is used, the private key from the PKCS12 certificate
> +	will be used(see `--smtp-ssl-client-cert`). Defaults to the value of the
> +	`sendemail.smtpSSLClientKey` configuration variable, if set.
> +

"will be used(see" -> "will be used (see".

This makes me wonder what the use case is for giving separate key file with a certificate file with its own private key in it. The documentation above clearly describes what happens (i.e., the separate key file makes the key embedded in the certificate file ignored), but I cannot quite think of a reason why anybody would want to do so. The key in the separate file is still something that corresponds to the public part in the certificate, no? The certificate can say "The subject of the certificate may use a private key that corresponds to any of these three public keys", and the certificate file may only have one or two but not all of these three public keys, or something?

> +	if (defined $smtp_ssl_client_cert) {
> +		# The cert could be in PKCS12 format, which can store both cert and key

The comment confused me initially. Yes, the cert could be PKCS12 with key. But the mention of the fact supports what design decision? Not requiring $smtp_ssl_client_key here (unlike the next if block that requires cert when key is used)? If so, perhaps we would want to spell that out?

		# We do not check and die when client_key is not
                # given, as a separate key file is unneeded for
                # PKCS12 certs.

or something?

Show 7 quoted lines

> +		$ret{SSL_cert_file} = $smtp_ssl_client_cert;
> +		$ret{SSL_use_cert} = 1;
>  	}
> +	if (defined $smtp_ssl_client_key) {
> +		if (!defined $smtp_ssl_client_cert) {
> +			# doesn't make sense to use a client key only
> +			die sprintf(__("Only client key \"%s\" specified"), $smtp_ssl_client_key);

Can you wrap this overly long line?

			die sprintf(__("Only client key \"%s\" specified"),
					$smtp_ssl_client_key);

Thanks.