On Mon, Sep 13, 2021 at 4:23 AM Frode Nordahl <[email protected]> wrote: > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > > --- > > > > > 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 > > > > > [email protected] > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > > > -- > > > > fbl > > > > -- > > fbl _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
