Hello,
On Sun, Jul 05, 2020 at 09:43:23AM +0300, [email protected] wrote:
>
> Subject: Re: [PATCH 2/2] SMALL: ssl: Support SAN extension for certificate
> generation
We commonly use the 'MINOR' tag instead of 'SMALL' here.
> The use of Common Name is fading out in favor of the RFC recommended
> way of using SAN extensions. For example, Chrome from version 58
> will only match server name against SAN.
>
> The following patch adds an optional flag to attach SAN extension
> of type DNS to the generated certificate based on the server name.
> ---
> doc/configuration.txt | 8 ++++++
> include/haproxy/listener-t.h | 1 +
> src/cfgparse-ssl.c | 13 +++++++++
> src/ssl_sock.c | 53 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 75 insertions(+)
>
> diff --git a/doc/configuration.txt b/doc/configuration.txt
> index 1d3878bc1..9a7ae43f0 100644
> --- a/doc/configuration.txt
> +++ b/doc/configuration.txt
> @@ -12166,6 +12166,14 @@ ca-sign-use-chain
> Enabling this flag will attach all public certificates encoded in
> `ca-sign-file`
> to the served certificate to the client, enabling trust.
>
> +ca-sign-use-san
> + This setting is only available when support for OpenSSL was built in. It is
> + the CA private key passphrase. This setting is optional and used only when
> + the dynamic generation of certificates is enabled. See
> + 'generate-certificates' for details.
> + Enabling this flag will add SAN extenstion of type DNS with the requested
> server name
> + inside the generated certificate.
> +
As your other patch I think that should be the default here, I don't
think we need an option for this, I'm even suprised it wasn't working
this way in the first place.
> ca-verify-file <cafile>
> This setting designates a PEM file from which to load CA certificates used
> to
> verify client's certificate. It designates CA certificates which must not
> be
> diff --git a/include/haproxy/listener-t.h b/include/haproxy/listener-t.h
> index 38ca2839f..47524ffd9 100644
> --- a/include/haproxy/listener-t.h
> +++ b/include/haproxy/listener-t.h
> @@ -164,6 +164,7 @@ struct bind_conf {
> char *ca_sign_pass; /* CAKey passphrase */
>
> int ca_sign_use_chain; /* Optionally attached the certificate chain
> to the served certificate */
> + int ca_sign_use_san; /* Optionally add SAN entry to the generated
> certificate */
> struct cert_key_and_chain * ca_sign_ckch; /* CA and possible
> certificate chain for ca generation */
> #endif
> struct proxy *frontend; /* the frontend all these listeners belong
> to, or NULL */
> diff --git a/src/cfgparse-ssl.c b/src/cfgparse-ssl.c
> index 270c857f9..62f754522 100644
> --- a/src/cfgparse-ssl.c
> +++ b/src/cfgparse-ssl.c
> @@ -550,6 +550,18 @@ static int bind_parse_ca_sign_use_chain(char **args, int
> cur_arg, struct proxy *
> return 0;
> }
>
> +/* parse the "ca-sign-use-san" bind keyword */
> +static int bind_parse_ca_sign_use_san(char **args, int cur_arg, struct proxy
> *px, struct bind_conf *conf, char **err)
> +{
> +#if (defined SSL_CTRL_SET_TLSEXT_HOSTNAME && !defined
> SSL_NO_GENERATE_CERTIFICATES)
> + conf->ca_sign_use_san = 1;
> +#else
> + memprintf(err, "%sthis version of openssl cannot generate SSL
> certificates.\n",
> + err && *err ? *err : "");
> +#endif
> + return 0;
> +}
> +
> /* parse the "ca-sign-pass" bind keyword */
> static int bind_parse_ca_sign_pass(char **args, int cur_arg, struct proxy
> *px, struct bind_conf *conf, char **err)
> {
> @@ -1721,6 +1733,7 @@ static struct bind_kw_list bind_kws = { "SSL", { }, {
> { "ca-sign-file", bind_parse_ca_sign_file, 1 }, /* set
> CAFile used to generate and sign server certs */
> { "ca-sign-pass", bind_parse_ca_sign_pass, 1 }, /* set
> CAKey passphrase */
> { "ca-sign-use-chain", bind_parse_ca_sign_use_chain, 1 }, /*
> enable attaching ca chain to generated certificate */
> + { "ca-sign-use-san", bind_parse_ca_sign_use_san, 1 }, /*
> enable adding SAN extension to generated certificate */
> { "ciphers", bind_parse_ciphers, 1 }, /* set
> SSL cipher suite */
> #if (HA_OPENSSL_VERSION_NUMBER >= 0x10101000L)
> { "ciphersuites", bind_parse_ciphersuites, 1 }, /* set
> TLS 1.3 cipher suite */
> diff --git a/src/ssl_sock.c b/src/ssl_sock.c
> index 54829eb98..a16635341 100644
> --- a/src/ssl_sock.c
> +++ b/src/ssl_sock.c
> @@ -1745,6 +1745,54 @@ static int ssl_sock_advertise_alpn_protos(SSL *s,
> const unsigned char **out,
> #ifdef SSL_CTRL_SET_TLSEXT_HOSTNAME
> #ifndef SSL_NO_GENERATE_CERTIFICATES
>
> +int ssl_sock_add_san_ext(X509V3_CTX* ctx, X509* cert, const char
> *servername) {
A commentary on top of the function is welcomed here, to explain a
little bit what it does!
> + int failure = 0;
> + X509_EXTENSION *san_ext = NULL;
> + char *san_name = NULL;
> + CONF *conf = NULL;
> + size_t buffer_size = strlen(servername)+sizeof("DNS:"); // Includes
> terminator
> +
> + conf = NCONF_new(NULL);
> + if (!conf) {
> + failure = 1;
> + goto cleanup;
> + }
> +
> + san_name = calloc(1, buffer_size);
> + if (!san_name) {
> + failure = 1;
> + goto cleanup;
> + }
> +
> + if ((buffer_size-1) != snprintf(san_name, buffer_size, "DNS:%s",
> servername)) {
> + failure = 1;
> + goto cleanup;
> + }
> +
You could probably use get_trash_chunk() here instead of doing a calloc,
and use chunk_appendf() instead of the snprintf, in order to avoid an
allocation.
> + // Build an extension based on the DNS entry above
> + san_ext = X509V3_EXT_nconf_nid(conf, ctx, NID_subject_alt_name,
> san_name);
> + if (!san_ext) {
> + failure = 1;
> + goto cleanup;
> + }
> +
> + // Add the extension
> + if (!X509_add_ext(cert, san_ext, -1 /* Add to end*/)) {
Please avoid c++ style commentaries ( // ) in your patches as well as
commentaries inside function parameters.
> + failure = 1;
> + goto cleanup;
> + }
> +
> + // Success
> + failure = 0;
> +
> +cleanup:
> + if (NULL != san_ext) X509_EXTENSION_free(san_ext);
> + if (NULL != san_name) free(san_name);
> + if (NULL != conf) NCONF_free(conf);
> +
> + return failure;
> +}
> +
> /* Create a X509 certificate with the specified servername and serial. This
> * function returns a SSL_CTX object or NULL if an error occurs. */
> static SSL_CTX *
> @@ -1828,6 +1876,11 @@ ssl_sock_do_create_cert(const char *servername, struct
> bind_conf *bind_conf, SSL
> X509_EXTENSION_free(ext);
> }
>
> + /* Add SAN extension */
> + if (bind_conf->ca_sign_use_san && ssl_sock_add_san_ext(&ctx, newcrt,
> servername)) {
> + goto mkcert_error;
> + }
> +
> /* Sign the certificate with the CA private key */
>
> key_type = EVP_PKEY_base_id(capkey);
I'll be away for a few days but I'm totally okay with merging these
once you made the small changes I suggested!
Thanks!
--
William Lallemand
