On Fri, Jul 10, 2020 at 4:15 PM William Lallemand <wlallem...@haproxy.com>
wrote:

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

Reply via email to