On Fri, Jul 10, 2020 at 4:15 PM William Lallemand <[email protected]> wrote:
> 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. > Got it. > > > 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! > Thanks for the comments William! > > -- > William Lallemand >
