On Thu, Nov 25, 2021 at 6:00 PM Flavio Leitner <f...@sysclose.org> wrote:
>
> On Thu, Nov 25, 2021 at 09:22:20AM +0100, Frode Nordahl wrote:
> > On Wed, Nov 24, 2021 at 9:31 PM Flavio Leitner <f...@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.nord...@canonical.com> wrote:
> > > > >
> > > > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner <f...@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 <f...@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.nord...@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.
Indeed, but the ability to cherry pick individual protocols through a
call to `SSL_CTX_set_options` has been deprecated in OpenSSL. But I
guess we don't need to force a change on our users until the
functionality is actually removed upstream. The prospect of adding the
new options becomes less desirable when we have to maintain the old
one too, but OTOH if we have both we expose a transition path for our
users, so that makes it more palatable.
> 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.
Our desire and goal with introducing a change here is twofold, the
first priority is to fix the current appearance of OVS defaulting to
insecure TLS protocols. As we have discussed, thanks to software
distributions choices for defaults for the OpenSSL library we are safe
as the insecure protocols will not be selected unless explicitly
configured. However as the code is presented this is not obvious, and
having non-obvious side effects in code related to security is a
problem.
The second priority is to reduce the recurring maintenance cost for
this part of the OVS code. There will inevitably be new TLS protocols,
and the current ones will inevitably become insecure. The security
teams of the world are working hard to keep the libraries we rely on
up to date and built with sane defaults.
What I would like us to do is to leverage their work and stop OVS from
having opinions about these details. A transition to min/max protocol
options with no OVS imposed default would address both of these (we
might need some build system checks to dtrt on improperly configured
systems).
To conclude we'll move forward with adding the new options and retain
the old option and behavior for now.
> > 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.
Yes, let's keep the old option until upstream removes the functionality.
--
Frode Nordahl
> 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
> > > > > > > > > d...@openvswitch.org
> > > > > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > > > > > >
> > > > > > > > --
> > > > > > > > fbl
> > > > > >
> > > > > > --
> > > > > > fbl
> > > > _______________________________________________
> > > > dev mailing list
> > > > d...@openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> > > --
> > > fbl
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> --
> fbl
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
