[ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols

Flavio Leitner fbl at sysclose.org
Thu Nov 25 16:59:52 UTC 2021 

On Thu, Nov 25, 2021 at 09:22:20AM +0100, Frode Nordahl wrote:
> On Wed, Nov 24, 2021 at 9:31 PM Flavio Leitner <fbl at sysclose.org> wrote:
> >
> > On Mon, Nov 15, 2021 at 10:40:47AM +0100, Frode Nordahl wrote:
> > > On Mon, Sep 13, 2021 at 4:23 AM Frode Nordahl
> > > <frode.nordahl at canonical.com> wrote:
> > > >
> > > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner <fbl at sysclose.org> wrote:
> > > > >
> > > > > On Fri, Sep 10, 2021 at 06:20:45PM +0200, Frode Nordahl wrote:
> > > > > > On Thu, Sep 9, 2021 at 9:53 PM Flavio Leitner <fbl at sysclose.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > > Hi Frode,
> > > > > > >
> > > > > > > Thanks for your patch.
> > > > > > > Please see my comments below.
> > > > > >
> > > > > > Flavio, thank you for taking the time to review.
> > > > > >
> > > > > > > On Wed, Aug 25, 2021 at 01:05:13PM +0200, Frode Nordahl wrote:
> > > > > > > > Contrary to what is stated in the documentation, when SSL
> > > > > > > > ciphers or protocols options are omitted the default values
> > > > > > > > will not be set.  The SSL library default settings will be used
> > > > > > > > instead.
> > > > > > > >
> > > > > > > > Fix handling of default ciphers and protocols so that we actually
> > > > > > > > enforce what is listed as defaults.
> > > > > > > >
> > > > > > > > Fixes: e18a1d086133 ("Add support for specifying SSL connection parameters to ovsdb")
> > > > > > > > Signed-off-by: Frode Nordahl <frode.nordahl at canonical.com>
> > > > > > > > ---
> > > > > > > >  lib/stream-ssl.c | 30 ++++++++++++++++++++++--------
> > > > > > > >  1 file changed, 22 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> > > > > > > > index 0ea3f2c08..6b4cf6970 100644
> > > > > > > > --- a/lib/stream-ssl.c
> > > > > > > > +++ b/lib/stream-ssl.c
> > > > > > > > @@ -162,8 +162,10 @@ struct ssl_config_file {
> > > > > > > >  static struct ssl_config_file private_key;
> > > > > > > >  static struct ssl_config_file certificate;
> > > > > > > >  static struct ssl_config_file ca_cert;
> > > > > > > > -static char *ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> > > > > > > > -static char *ssl_ciphers = "HIGH:!aNULL:!MD5";
> > > > > > > > +static char *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> > > > > > > > +static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> > > > > > > > +static char *ssl_protocols = NULL;
> > > > > > > > +static char *ssl_ciphers = NULL;
> > > > > > > >
> > > > > > > >  /* Ordinarily, the SSL client and server verify each other's certificates using
> > > > > > > >   * a CA certificate.  Setting this to false disables this behavior.  (This is a
> > > > > > > > @@ -1225,14 +1227,19 @@ stream_ssl_set_key_and_cert(const char *private_key_file,
> > > > > > > >  void
> > > > > > > >  stream_ssl_set_ciphers(const char *arg)
> > > > > > > >  {
> > > > > > > > -    if (ssl_init() || !arg || !strcmp(ssl_ciphers, arg)) {
> > > > > > >
> > > > > > > The ssl_init() calls at least one time do_ssl_init() which then
> > > > > > > calls SSL_CTX_set_cipher_list(ctx, "HIGH:!aNULL:!MD5").
> > > > > > > Those are the defaults in the man-page and not from the library.
> > > > > > >
> > > > > > > The do_ssl_init() also does:
> > > > > > >    method = CONST_CAST(SSL_METHOD *, SSLv23_method());
> > > > > > >
> > > > > > > That should return SSLv3, TLSv1, TLSv1.1 and TLS1.2.
> > > > > > >
> > > > > > >    ctx = SSL_CTX_new(method);
> > > > > > >    SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
> > > > > > >
> > > > > > > And there it excludes those SSL v2 and v3.
> > > > > > >
> > > > > > > Therefore, the default would be "TLSv1,TLSv1.1,TLSv1.2" which is
> > > > > > > the same in the man-page.
> > > > > > >
> > > > > > > Did I miss something?
> > > > > >
> > > > > > Thank you for pointing out that, I did not realize we manipulated
> > > > > > these options multiple places.
> > > > > >
> > > > > > I do need to rephrase the commit message, but there is still a problem
> > > > > > here. It became apparent when working on the next patch in the series,
> > > > > > where functional tests behave unexpectedly when passing the
> > > > > > ssl-protocols options. The reason being that when the protocol list
> > > > > > matches what is already in the static char *ssl_protocols in
> > > > > > lib/stream-ssl.c stream_ssl_set_protocols returns early without
> > > > > > setting any option.
> > > > >
> > > > > If that matches then it is because the default is set, so it
> > > > > shouldn't make any difference to return early. Do you still
> > > > > have the next patch without the default_ssl_protocols change
> > > > > for me to take a look? That might help me to see the issue.
> > > >
> > > > That would be true if do_ssl_init() and stream_ssl_set_protocols()
> > > > manipulated the options the same way, the reality is that they do not,
> > > > and the effective output from do_ssl_init() differ depending on the
> > > > version of OpenSSL Open vSwitch is built with.
> > > >
> > > > > > So I guess the question then becomes, should we stop doing this
> > > > > > multiple places or do you want me to update the call to
> > > > > > SSL_CTX_set_options in do_ssl_init and not introduce this change?
> > > > >
> > > > > Not sure yet because I didn't see the problem yet.
> > > >
> > > > Let me demonstrate, with OVS built from master against OpenSSL 1.1.1f
> > > > and the following modification to the ovs-sandbox script:
> > > > --- a/tutorial/ovs-sandbox
> > > > +++ b/tutorial/ovs-sandbox
> > > > @@ -270,7 +270,7 @@ trap 'kill `cat "$sandbox"/*.pid`' 0 1 2 3 13 14 15
> > > >  # Create database and start ovsdb-server.
> > > >  touch "$sandbox"/.conf.db.~lock~
> > > >  run ovsdb-tool create conf.db "$schema"
> > > > -ovsdb_server_args=
> > > > +ovsdb_server_args="--private-key=db:Open_vSwitch,SSL,private_key
> > > > --certificate=db:Open_vSwitch,SSL,certificate
> > > > --bootstrap-ca-cert=db:Open_vSwitch,SSL,ca_cert"
> > > >  rungdb $gdb_ovsdb $gdb_ovsdb_ex ovsdb-server --detach --no-chdir
> > > > --pidfile -vconsole:off --log-file -vsyslog:off \
> > > >         --remote=punix:"$sandbox"/db.sock \
> > > >         --remote=db:Open_vSwitch,Open_vSwitch,manager_options \
> > > >
> > > > $ cd sandbox/
> > > > $ ovs-pki init -d pki -l pki/ovs-log.log
> > > > $ ovs-pki req+sign sc switch -d pki -l pki/ovs-log.log
> > > > sc-req.pem    Mon Sep 13 01:33:35 UTC 2021
> > > >     fingerprint 8a603a3a5c9e38d2ed90f0c711e1f8b9ca59abab
> > > > $ ovs-vsctl set-ssl $(pwd)/sc-privkey.pem $(pwd)/sc-cert.pem
> > > > $(pwd)/pki/switchca/cacert.pem
> > > > $ ovs-vsctl set-manager pssl::127.0.0.1
> > > > $ openssl s_client -connect 127.0.0.1:6653 -cert sc- -CAfile
> > > > pki/switchca/cacert.pem 2>/dev/null |grep Protocol
> > > >     Protocol  : TLSv1.3
> > > >     Protocol  : TLSv1.3
> > > > ^C
> > > >
> > > > This behavior is different from what the stream_ssl_set_protocols()
> > > > function expects and what is indeed documented.
> > > >
> > > > I would think we would make the code more robust if we had only one
> > > > function that determined and set what protocol options to use, and at
> > > > the very least we would need to catch up with the current most widely
> > > > used version of OpenSSL which apparently has already given us TLSv1.3
> > > > support, provided the user supplies no options.
> > > >
> > > > Actually with the OpenSSL version provided on my system the default
> > > > becomes to only provide TLSv1.2 and TLSv1.3, that is of course great
> > > > but again not what the code or documentation in OVS stastes/expects.
> > > >
> > > > I guess it would be fair to say given how the past OpenSSL APIs have
> > > > been for selecting which protocols to enable this is bound to happen,
> > > > and it appears they have acknowledged this fact and deprecated the use
> > > > of SSL_CTX_set_options for enabling individual protocol versions [0]
> > > > and are referring to the use of SSL_CTX_set_min_proto_version [1] /
> > > > SSL_CTX_set_max_proto_version [2] instead.
> > > >
> > > > Another thing we could consider is to deprecate the current user
> > > > exposed options to select individual protocols and replace them with
> > > > min-/max-version counterparts?
> > >
> > > Now that OpenSSL 3.0 has been released the actuality of this is
> > > further increased. I would like to follow up with a new series to
> > > address this and I would like your input before continuing.
> > >
> > > What would you think about the following approach:
> > >
> > > - Deprecate the `ssl-protocols` option, schedule removal in OVS 2.18
> > > and replace statements about default to specific protocol versions in
> > > the documentation with a reference to how system SSL library default
> > > configuration influences the effective default.
> >
> > Do you think we really need to remove the option? Because removing
> > options break upgrades. Perhaps documenting the behavior as you said
> > and recommend to use the new options below instead?
> 
> I see what you mean, right so that would also mean keeping parts of
> the existing implementation which underpinnings might disappear at
> some point.  However, this opens up a possible twist to the proposed
> solution.
> 
> We could instead of adding new options change the meaning of the
> existing one.  What if the comma separated list the user provides is
> parsed and interpreted as a min/max specification? We could then make
> calls to `SSL_CTX_set_min_proto_version` /
> `SSL_CTX_set_max_proto_version` internally based on the result of
> parsing the string.

The issue is that today it receives a list of protocols, so one
call specify TLSv1 because he has old devices and TLSv1.2. Then
setting min to TLSv1 and max to TLSv1.2 means that TLSv1.1 is
included while that might not be desired.

I don't see why we need to change the current behavior. All we
need is to document that the default depends on the lib or fix
OVS to actually use that default.

> That would definitively lead to less downstream churn as result of the
> change.  We would of course still have to deprecate the current
> meaning and give a good notice period before we actually make the
> change.

Right, it may not break upgrades, but may require user intervention.

Thanks!
fbl


> > > - Add `ssl-min-protocol` and `ssl-max-protocol` options, which both
> > > have no default meaning use system SSL library default configuration.
> > > If any of these options are set they will take precedence over the now
> > > deprecated `ssl-protocols` option.
> >
> > That sounds good to me. What do you think about OVS logging
> > the supported protocols during initialization? Because the
> > supported protocol list can change on a lib upgrade.
> 
> Yes, logging the effective list of enabled protocols would indeed be useful.
> 
> > > - Add `ssl-ciphersuites` option to allow control of TLSv1.3
> > > characteristics, document that the `ssl-ciphers` option is for TLSv1.2
> > > and below.
> >
> > Makes sense to me.
> 
> Thanks!
> 
> -- 
> Frode Nordahl
> 
> > Thanks,
> > fbl
> >
> > >
> > > --
> > > Frode Nordahl
> > >
> > > > 0: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_options.html
> > > > 1: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_min_proto_version.html
> > > > 2: https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_max_proto_version.html
> > > >
> > > > --
> > > > Frode Nordahl
> > > >
> > > > > Thanks,
> > > > > fbl
> > > > >
> > > > > >
> > > > > > --
> > > > > > Frode Nordahl
> > > > > >
> > > > > > > Thanks
> > > > > > > fbl
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +    const char *input = arg ? arg : default_ssl_ciphers;
> > > > > > > > +
> > > > > > > > +    if (ssl_init() || !input || (ssl_ciphers && !strcmp(ssl_ciphers, input))) {
> > > > > > > >          return;
> > > > > > > >      }
> > > > > > > > -    if (SSL_CTX_set_cipher_list(ctx,arg) == 0) {
> > > > > > > > +    if (SSL_CTX_set_cipher_list(ctx, input) == 0) {
> > > > > > > >          VLOG_ERR("SSL_CTX_set_cipher_list: %s",
> > > > > > > >                   ERR_error_string(ERR_get_error(), NULL));
> > > > > > > >      }
> > > > > > > > -    ssl_ciphers = xstrdup(arg);
> > > > > > > > +    if (ssl_ciphers) {
> > > > > > > > +        free(ssl_ciphers);
> > > > > > > > +    }
> > > > > > > > +    ssl_ciphers = xstrdup(input);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  /* Set SSL protocols based on the string input. Aborts with an error message
> > > > > > > > @@ -1240,7 +1247,11 @@ stream_ssl_set_ciphers(const char *arg)
> > > > > > > >  void
> > > > > > > >  stream_ssl_set_protocols(const char *arg)
> > > > > > > >  {
> > > > > > > > -    if (ssl_init() || !arg || !strcmp(arg, ssl_protocols)){
> > > > > > > > +    const char *input = arg ? arg : default_ssl_protocols;
> > > > > > > > +
> > > > > > > > +    if (ssl_init() || !input
> > > > > > > > +        || (ssl_protocols && !strcmp(input, ssl_protocols)))
> > > > > > > > +    {
> > > > > > > >          return;
> > > > > > > >      }
> > > > > > > >
> > > > > > > > @@ -1253,7 +1264,7 @@ stream_ssl_set_protocols(const char *arg)
> > > > > > > >  #endif
> > > > > > > >      long protocol_flags = SSL_OP_NO_SSL_MASK;
> > > > > > > >
> > > > > > > > -    char *s = xstrdup(arg);
> > > > > > > > +    char *s = xstrdup(input);
> > > > > > > >      char *save_ptr = NULL;
> > > > > > > >      char *word = strtok_r(s, " ,\t", &save_ptr);
> > > > > > > >      if (word == NULL) {
> > > > > > > > @@ -1281,7 +1292,10 @@ stream_ssl_set_protocols(const char *arg)
> > > > > > > >      /* Set the actual options. */
> > > > > > > >      SSL_CTX_set_options(ctx, protocol_flags);
> > > > > > > >
> > > > > > > > -    ssl_protocols = xstrdup(arg);
> > > > > > > > +    if (ssl_protocols) {
> > > > > > > > +      free(ssl_protocols);
> > > > > > > > +    }
> > > > > > > > +    ssl_protocols = xstrdup(input);
> > > > > > > >
> > > > > > > >  exit:
> > > > > > > >      free(s);
> > > > > > > > --
> > > > > > > > 2.32.0
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > dev mailing list
> > > > > > > > dev at openvswitch.org
> > > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > > >
> > > > > > > --
> > > > > > > fbl
> > > > >
> > > > > --
> > > > > fbl
> > > _______________________________________________
> > > dev mailing list
> > > dev at openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> > --
> > fbl
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

-- 
fbl

More information about the dev mailing list