Hello, On Sun, Jul 05, 2020 at 09:43:23AM +0300, gers...@gmail.com 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