Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Thu, Nov 25, 2021 at 6:00 PM Flavio Leitner 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 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
> > > > wrote:
> > > > >
> > > > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner
> > > > > 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
> > > > > > > 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
> > > > > > > > > ---
> > > > > > > > > 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
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Thu, Nov 25, 2021 at 09:22:20AM +0100, Frode Nordahl wrote:
> On Wed, Nov 24, 2021 at 9:31 PM Flavio Leitner 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
> > > wrote:
> > > >
> > > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner
> > > > 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
> > > > > > 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
> > > > > > > > ---
> > > > > > > > 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
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Wed, Nov 24, 2021 at 9:31 PM Flavio Leitner 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
> > wrote:
> > >
> > > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner 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
> > > > > 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
> > > > > > > ---
> > > > > > > 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
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Mon, Nov 15, 2021 at 10:40:47AM +0100, Frode Nordahl wrote:
> On Mon, Sep 13, 2021 at 4:23 AM Frode Nordahl
> wrote:
> >
> > On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner 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 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
> > > > > > ---
> > > > > > 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
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Mon, Sep 13, 2021 at 4:23 AM Frode Nordahl
wrote:
>
> On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner 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 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
> > > > > ---
> > > > > 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
>
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Sat, Sep 11, 2021 at 10:23 PM Flavio Leitner 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 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
> > > > ---
> > > > 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/
$
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Fri, Sep 10, 2021 at 06:20:45PM +0200, Frode Nordahl wrote:
> On Thu, Sep 9, 2021 at 9:53 PM Flavio Leitner 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
> > > ---
> > > 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.
> 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.
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
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
On Thu, Sep 9, 2021 at 9:53 PM Flavio Leitner 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
> > ---
> > 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.
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?
--
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", _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
> >
Re: [ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
Hi Frode,
Thanks for your patch.
Please see my comments below.
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
> ---
> 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?
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", _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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 1/2] stream-ssl: Fix handling of default ciphers/protocols
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
---
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)) {
+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", _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
