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