Re: [ovs-dev] [PATCH v2 2/2] stream-ssl: Update default protocols, enable TLSv1.3
On Fri, Sep 10, 2021 at 06:23:18PM +0200, Frode Nordahl wrote:
> On Thu, Sep 9, 2021 at 10:05 PM Flavio Leitner wrote:
> >
> >
> > Hi Frode,
>
> Hello Flavio, thank you for taking the time to review.
>
> > On Wed, Aug 25, 2021 at 01:05:14PM +0200, Frode Nordahl wrote:
> > > RFC 8996 [0] deprecates the use of TLSv1 and TLSv1.1 for security
> > > reasons. Update our default list of protcols to be in compliance.
> > >
> > > Also add TLSv1.3 to the default list of supported protocols.
> > >
> > > 0: https://datatracker.ietf.org/doc/html/rfc8996
> > > Signed-off-by: Frode Nordahl
> >
> > This patch does two things:
> > Deprecate TLSv1 and TLSv1.2
> > Add support for TLSv1.3
> >
> > Can we split them into separate logical patches?
>
> Yes, that makes sense.
>
> > Also, shouldn't we first warn the users about deprecating
> > TLSv1 and TLSv1.1 for a release period, and then remove from
> > the default list in the next release? I mean, this is an user
> > visible change, so users might need some time to adapt.
> >
> > What do you think?
>
> That would indeed be the appropriate thing to do. I guess I felt a bit
> of a rush since we are a bit late on this deprecation, but better do
> it properly regardless!
Cool, thanks!
fbl
>
> Thanks!
>
> --
> Frode Nordahl
>
> > fbl
> >
> > > ---
> > > NEWS | 7 +++
> > > lib/ssl-connect.man | 6 --
> > > lib/stream-ssl.c | 35 +--
> > > m4/openvswitch.m4 | 16
> > > tests/ovsdb-server.at | 6 ++
> > > 5 files changed, 58 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 1f2adf718..502e6693a 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -8,6 +8,13 @@ Post-v2.16.0
> > > by default. 'other_config:dpdk-socket-limit' can be set equal to
> > > the 'other_config:dpdk-socket-mem' to preserve the legacy memory
> > > limiting behavior.
> > > + - Update default configuration for enabled TLS protocols:
> > > + * RFC 8996 deprecates the use of TLSv1 and TLSv1.1 for security
> > > reasons.
> > > + * Add TLSv1.3 to the default list of enabled protocols when the
> > > built with
> > > + OpenSSL v1.1.1 and newer.
> > > + * The new default is as such: TLSv1.2,TLSv1.3
> > > + * As a consequence we no longer support building Open vSwitch with
> > > OpenSSL
> > > + versions without TLSv1.2 support.
> > >
> > >
> > > v2.16.0 - 16 Aug 2021
> > > diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man
> > > index 6e54f77ef..0dd5a29be 100644
> > > --- a/lib/ssl-connect.man
> > > +++ b/lib/ssl-connect.man
> > > @@ -1,10 +1,12 @@
> > > .IP "\fB\-\-ssl\-protocols=\fIprotocols\fR"
> > > Specifies, in a comma- or space-delimited list, the SSL protocols
> > > \fB\*(PN\fR will enable for SSL connections. Supported
> > > -\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, and \fBTLSv1.2\fR.
> > > +\fIprotocols\fR include \fBTLSv1.2\fR and \fBTLSv1.3\fR depending on
> > > +which version of OpenSSL Open vSwitch is built with.
> > > Regardless of order, the highest protocol supported by both sides will
> > > be chosen when making the connection. The default when this option is
> > > -omitted is \fBTLSv1,TLSv1.1,TLSv1.2\fR.
> > > +omitted is \fBTLSv1.2,TLSv1.3\fR if built with a version of OpenSSL that
> > > +supports \fBTLSv1.3\fR.
> > > .
> > > .IP "\fB\-\-ssl\-ciphers=\fIciphers\fR"
> > > Specifies, in OpenSSL cipher string format, the ciphers \fB\*(PN\fR will
> > > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> > > index 6b4cf6970..954067787 100644
> > > --- a/lib/stream-ssl.c
> > > +++ b/lib/stream-ssl.c
> > > @@ -162,7 +162,13 @@ 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 *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> > > +/* RFC 8996 deprecates the use of TLSv1 and TLSv1.1, users may still
> > > specify
> > > + * them in their configuration, but our defaults are in compliance. */
> > > +#if OPENSSL_VERSION_NUMBER < 0x10101000L
> > > +static char *default_ssl_protocols = "TLSv1.2";
> > > +#else
> > > +static char *default_ssl_protocols = "TLSv1.2,TLSv1.3";
> > > +#endif /* OPENSSL_VERSION_NUMBER < 0x10101000L */
> > > static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> > > static char *ssl_protocols = NULL;
> > > static char *ssl_ciphers = NULL;
> > > @@ -1255,9 +1261,18 @@ stream_ssl_set_protocols(const char *arg)
> > > return;
> > > }
> > >
> > > +/* TODO: Using SSL_CTX_set_options to enable individual protocol
> > > versions
> > > + * is deprecated as of OpenSSL v1.1.0. Once we can drop support for
> > > builds
> > > + * with OpenSSL pre v1.1.0 we should use
> > > SSL_CTX_set_min_proto_version and
> > > + * SSL_CTX_set_max_proto_version instead. */
> > > +
> > > /* Start with all the flags off
Re: [ovs-dev] [PATCH v2 2/2] stream-ssl: Update default protocols, enable TLSv1.3
On Thu, Sep 9, 2021 at 10:05 PM Flavio Leitner wrote:
>
>
> Hi Frode,
Hello Flavio, thank you for taking the time to review.
> On Wed, Aug 25, 2021 at 01:05:14PM +0200, Frode Nordahl wrote:
> > RFC 8996 [0] deprecates the use of TLSv1 and TLSv1.1 for security
> > reasons. Update our default list of protcols to be in compliance.
> >
> > Also add TLSv1.3 to the default list of supported protocols.
> >
> > 0: https://datatracker.ietf.org/doc/html/rfc8996
> > Signed-off-by: Frode Nordahl
>
> This patch does two things:
> Deprecate TLSv1 and TLSv1.2
> Add support for TLSv1.3
>
> Can we split them into separate logical patches?
Yes, that makes sense.
> Also, shouldn't we first warn the users about deprecating
> TLSv1 and TLSv1.1 for a release period, and then remove from
> the default list in the next release? I mean, this is an user
> visible change, so users might need some time to adapt.
>
> What do you think?
That would indeed be the appropriate thing to do. I guess I felt a bit
of a rush since we are a bit late on this deprecation, but better do
it properly regardless!
Thanks!
--
Frode Nordahl
> fbl
>
> > ---
> > NEWS | 7 +++
> > lib/ssl-connect.man | 6 --
> > lib/stream-ssl.c | 35 +--
> > m4/openvswitch.m4 | 16
> > tests/ovsdb-server.at | 6 ++
> > 5 files changed, 58 insertions(+), 12 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 1f2adf718..502e6693a 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -8,6 +8,13 @@ Post-v2.16.0
> > by default. 'other_config:dpdk-socket-limit' can be set equal to
> > the 'other_config:dpdk-socket-mem' to preserve the legacy memory
> > limiting behavior.
> > + - Update default configuration for enabled TLS protocols:
> > + * RFC 8996 deprecates the use of TLSv1 and TLSv1.1 for security
> > reasons.
> > + * Add TLSv1.3 to the default list of enabled protocols when the built
> > with
> > + OpenSSL v1.1.1 and newer.
> > + * The new default is as such: TLSv1.2,TLSv1.3
> > + * As a consequence we no longer support building Open vSwitch with
> > OpenSSL
> > + versions without TLSv1.2 support.
> >
> >
> > v2.16.0 - 16 Aug 2021
> > diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man
> > index 6e54f77ef..0dd5a29be 100644
> > --- a/lib/ssl-connect.man
> > +++ b/lib/ssl-connect.man
> > @@ -1,10 +1,12 @@
> > .IP "\fB\-\-ssl\-protocols=\fIprotocols\fR"
> > Specifies, in a comma- or space-delimited list, the SSL protocols
> > \fB\*(PN\fR will enable for SSL connections. Supported
> > -\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, and \fBTLSv1.2\fR.
> > +\fIprotocols\fR include \fBTLSv1.2\fR and \fBTLSv1.3\fR depending on
> > +which version of OpenSSL Open vSwitch is built with.
> > Regardless of order, the highest protocol supported by both sides will
> > be chosen when making the connection. The default when this option is
> > -omitted is \fBTLSv1,TLSv1.1,TLSv1.2\fR.
> > +omitted is \fBTLSv1.2,TLSv1.3\fR if built with a version of OpenSSL that
> > +supports \fBTLSv1.3\fR.
> > .
> > .IP "\fB\-\-ssl\-ciphers=\fIciphers\fR"
> > Specifies, in OpenSSL cipher string format, the ciphers \fB\*(PN\fR will
> > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> > index 6b4cf6970..954067787 100644
> > --- a/lib/stream-ssl.c
> > +++ b/lib/stream-ssl.c
> > @@ -162,7 +162,13 @@ 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 *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> > +/* RFC 8996 deprecates the use of TLSv1 and TLSv1.1, users may still
> > specify
> > + * them in their configuration, but our defaults are in compliance. */
> > +#if OPENSSL_VERSION_NUMBER < 0x10101000L
> > +static char *default_ssl_protocols = "TLSv1.2";
> > +#else
> > +static char *default_ssl_protocols = "TLSv1.2,TLSv1.3";
> > +#endif /* OPENSSL_VERSION_NUMBER < 0x10101000L */
> > static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> > static char *ssl_protocols = NULL;
> > static char *ssl_ciphers = NULL;
> > @@ -1255,9 +1261,18 @@ stream_ssl_set_protocols(const char *arg)
> > return;
> > }
> >
> > +/* TODO: Using SSL_CTX_set_options to enable individual protocol
> > versions
> > + * is deprecated as of OpenSSL v1.1.0. Once we can drop support for
> > builds
> > + * with OpenSSL pre v1.1.0 we should use SSL_CTX_set_min_proto_version
> > and
> > + * SSL_CTX_set_max_proto_version instead. */
> > +
> > /* Start with all the flags off and turn them on as requested. */
> > #ifndef SSL_OP_NO_SSL_MASK
> > -/* For old OpenSSL without this macro, this is the correct value. */
> > +/* For old OpenSSL without this macro, this is the correct value.
> > + *
> > + * NOTE: We deliberately did not extend this compatibility macro to
> > + * include
Re: [ovs-dev] [PATCH v2 2/2] stream-ssl: Update default protocols, enable TLSv1.3
Hi Frode,
On Wed, Aug 25, 2021 at 01:05:14PM +0200, Frode Nordahl wrote:
> RFC 8996 [0] deprecates the use of TLSv1 and TLSv1.1 for security
> reasons. Update our default list of protcols to be in compliance.
>
> Also add TLSv1.3 to the default list of supported protocols.
>
> 0: https://datatracker.ietf.org/doc/html/rfc8996
> Signed-off-by: Frode Nordahl
This patch does two things:
Deprecate TLSv1 and TLSv1.2
Add support for TLSv1.3
Can we split them into separate logical patches?
Also, shouldn't we first warn the users about deprecating
TLSv1 and TLSv1.1 for a release period, and then remove from
the default list in the next release? I mean, this is an user
visible change, so users might need some time to adapt.
What do you think?
fbl
> ---
> NEWS | 7 +++
> lib/ssl-connect.man | 6 --
> lib/stream-ssl.c | 35 +--
> m4/openvswitch.m4 | 16
> tests/ovsdb-server.at | 6 ++
> 5 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 1f2adf718..502e6693a 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -8,6 +8,13 @@ Post-v2.16.0
> by default. 'other_config:dpdk-socket-limit' can be set equal to
> the 'other_config:dpdk-socket-mem' to preserve the legacy memory
> limiting behavior.
> + - Update default configuration for enabled TLS protocols:
> + * RFC 8996 deprecates the use of TLSv1 and TLSv1.1 for security reasons.
> + * Add TLSv1.3 to the default list of enabled protocols when the built
> with
> + OpenSSL v1.1.1 and newer.
> + * The new default is as such: TLSv1.2,TLSv1.3
> + * As a consequence we no longer support building Open vSwitch with
> OpenSSL
> + versions without TLSv1.2 support.
>
>
> v2.16.0 - 16 Aug 2021
> diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man
> index 6e54f77ef..0dd5a29be 100644
> --- a/lib/ssl-connect.man
> +++ b/lib/ssl-connect.man
> @@ -1,10 +1,12 @@
> .IP "\fB\-\-ssl\-protocols=\fIprotocols\fR"
> Specifies, in a comma- or space-delimited list, the SSL protocols
> \fB\*(PN\fR will enable for SSL connections. Supported
> -\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, and \fBTLSv1.2\fR.
> +\fIprotocols\fR include \fBTLSv1.2\fR and \fBTLSv1.3\fR depending on
> +which version of OpenSSL Open vSwitch is built with.
> Regardless of order, the highest protocol supported by both sides will
> be chosen when making the connection. The default when this option is
> -omitted is \fBTLSv1,TLSv1.1,TLSv1.2\fR.
> +omitted is \fBTLSv1.2,TLSv1.3\fR if built with a version of OpenSSL that
> +supports \fBTLSv1.3\fR.
> .
> .IP "\fB\-\-ssl\-ciphers=\fIciphers\fR"
> Specifies, in OpenSSL cipher string format, the ciphers \fB\*(PN\fR will
> diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> index 6b4cf6970..954067787 100644
> --- a/lib/stream-ssl.c
> +++ b/lib/stream-ssl.c
> @@ -162,7 +162,13 @@ 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 *default_ssl_protocols = "TLSv1,TLSv1.1,TLSv1.2";
> +/* RFC 8996 deprecates the use of TLSv1 and TLSv1.1, users may still specify
> + * them in their configuration, but our defaults are in compliance. */
> +#if OPENSSL_VERSION_NUMBER < 0x10101000L
> +static char *default_ssl_protocols = "TLSv1.2";
> +#else
> +static char *default_ssl_protocols = "TLSv1.2,TLSv1.3";
> +#endif /* OPENSSL_VERSION_NUMBER < 0x10101000L */
> static char *default_ssl_ciphers = "HIGH:!aNULL:!MD5";
> static char *ssl_protocols = NULL;
> static char *ssl_ciphers = NULL;
> @@ -1255,9 +1261,18 @@ stream_ssl_set_protocols(const char *arg)
> return;
> }
>
> +/* TODO: Using SSL_CTX_set_options to enable individual protocol versions
> + * is deprecated as of OpenSSL v1.1.0. Once we can drop support for
> builds
> + * with OpenSSL pre v1.1.0 we should use SSL_CTX_set_min_proto_version
> and
> + * SSL_CTX_set_max_proto_version instead. */
> +
> /* Start with all the flags off and turn them on as requested. */
> #ifndef SSL_OP_NO_SSL_MASK
> -/* For old OpenSSL without this macro, this is the correct value. */
> +/* For old OpenSSL without this macro, this is the correct value.
> + *
> + * NOTE: We deliberately did not extend this compatibility macro to
> + * include SSL_OP_NO_TLSv1_3 because if you do have a version of OpenSSL
> + * with TLSv1.3 support this macro would be defined by OpenSSL. */
> #define SSL_OP_NO_SSL_MASK (SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | \
> SSL_OP_NO_TLSv1 | SSL_OP_NO_TLSv1_1 | \
> SSL_OP_NO_TLSv1_2)
> @@ -1272,12 +1287,20 @@ stream_ssl_set_protocols(const char *arg)
> goto exit;
> }
> while (word != NULL) {
> -long on_flag;
> -if (!strcasecmp(word, "TLSv1.2")){
> +long
