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.
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", &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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
