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

[Junio C Hamano <gitster@pobox.com>]

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

> +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?

> 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".

> +	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?

> +		$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.