[ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
Frode Nordahl
frode.nordahl at canonical.com
Mon Nov 15 09:40:47 UTC 2021
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.
- 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.
- Add `ssl-ciphersuites` option to allow control of TLSv1.3
characteristics, document that the `ssl-ciphers` option is for TLSv1.2
and below.
--
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
More information about the dev
mailing list