| Message ID | cb92f4dd384f972274167f599b5b57c38281d9d3.camel@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] lib/ssl: enable TLSv1.3 if supported by SSL | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/intel-ovs-compilation | success | test: success |
Hi, Dan. Thanks for the patch! Looks like you've lost a commit message somewhere. On 5/4/23 17:47, Dan Williams wrote: > diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man > index 6e54f77ef4d5e..896ce79c6378f 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. > -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. > +\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, \fBTLSv1.2\fR, and > +(if supported by OpenSSL) \fBTLSv1.3\fR. 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 and when the SSL implementation supports > +TLSv1.3, the default also includes \fBTLSv1.3\fR. I'd say we should document in the opposite manner. All the modern versions of OpenSSL should support TLS 1.3. So, I'd suggest we say that it is supported unless it's not supported by the SSL implementation. Instead of saying that it's not supported unless SSL implementation supports. > . > .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 62da9febb663a..4f053d17dfccc 100644 > --- a/lib/stream-ssl.c > +++ b/lib/stream-ssl.c > @@ -162,9 +162,15 @@ 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"; > > +#define BASE_SSL_PROTOS "TLSv1,TLSv1.1,TLSv1.2" > +#ifdef SSL_OP_NO_TLSv1_3 > +static char *ssl_protocols = BASE_SSL_PROTOS",TLSv1.3"; > +#else > +static char *ssl_protocols = BASE_SSL_PROTOS; > +#endif > + > /* 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 > * security risk.) */ > @@ -1284,6 +1290,10 @@ stream_ssl_set_protocols(const char *arg) > on_flag = SSL_OP_NO_TLSv1_1; > } else if (!strcasecmp(word, "TLSv1")){ > on_flag = SSL_OP_NO_TLSv1; > +#ifdef SSL_OP_NO_TLSv1_3 > + } else if (!strcasecmp(word, "TLSv1.3")){ I think, we should place a space between ){. Even though existing code doesn't do that, there is no point in repeating bad patterns. > + on_flag = SSL_OP_NO_TLSv1_3; > +#endif I'm not really a big fan of having handling of 1.3 at the end while other versions are in descending order, but I also see why you placed it here. It takes less ifdef-ed code this way. Alternative suggestion: Maybe we can create a local array of structures that will map the name to a flag. And then iterate over it in a loop. What do you think? I don't have a strong opinion. Best regards, Ilya Maximets.
On 5/5/23 17:23, Ilya Maximets wrote: > Hi, Dan. Thanks for the patch! > > Looks like you've lost a commit message somewhere. > > On 5/4/23 17:47, Dan Williams wrote: >> diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man >> index 6e54f77ef4d5e..896ce79c6378f 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. >> -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. >> +\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, \fBTLSv1.2\fR, and >> +(if supported by OpenSSL) \fBTLSv1.3\fR. 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 and when the SSL implementation supports >> +TLSv1.3, the default also includes \fBTLSv1.3\fR. > > I'd say we should document in the opposite manner. All the modern versions > of OpenSSL should support TLS 1.3. So, I'd suggest we say that it is > supported unless it's not supported by the SSL implementation. Instead > of saying that it's not supported unless SSL implementation supports. > >> . >> .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 62da9febb663a..4f053d17dfccc 100644 >> --- a/lib/stream-ssl.c >> +++ b/lib/stream-ssl.c >> @@ -162,9 +162,15 @@ 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"; >> >> +#define BASE_SSL_PROTOS "TLSv1,TLSv1.1,TLSv1.2" >> +#ifdef SSL_OP_NO_TLSv1_3 >> +static char *ssl_protocols = BASE_SSL_PROTOS",TLSv1.3"; >> +#else >> +static char *ssl_protocols = BASE_SSL_PROTOS; >> +#endif >> + >> /* 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 >> * security risk.) */ >> @@ -1284,6 +1290,10 @@ stream_ssl_set_protocols(const char *arg) >> on_flag = SSL_OP_NO_TLSv1_1; >> } else if (!strcasecmp(word, "TLSv1")){ >> on_flag = SSL_OP_NO_TLSv1; >> +#ifdef SSL_OP_NO_TLSv1_3 >> + } else if (!strcasecmp(word, "TLSv1.3")){ > > I think, we should place a space between ){. Even though existing > code doesn't do that, there is no point in repeating bad patterns. > >> + on_flag = SSL_OP_NO_TLSv1_3; >> +#endif > > I'm not really a big fan of having handling of 1.3 at the end while > other versions are in descending order, but I also see why you placed > it here. It takes less ifdef-ed code this way. > > Alternative suggestion: Maybe we can create a local array of > structures that will map the name to a flag. And then iterate > over it in a loop. > > What do you think? I don't have a strong opinion. Hmm. Also, according to OpenSSL documentation, flags to disable specific versions are deprecated since 1.1.0: https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_options.html And users advised to use SSL_CTX_set_min/max_proto_version() API instead (new in 1.1.0): https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_min_proto_version.html It might make sense to change the interface altogether, i.e. find min and max requested version and use the new API. But I'm not sure. > > Best regards, Ilya Maximets.
On Fri, 2023-05-05 at 20:08 +0200, Ilya Maximets wrote: > On 5/5/23 17:23, Ilya Maximets wrote: > > Hi, Dan. Thanks for the patch! > > > > Looks like you've lost a commit message somewhere. > > > > On 5/4/23 17:47, Dan Williams wrote: > > > diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man > > > index 6e54f77ef4d5e..896ce79c6378f 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. > > > -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. > > > +\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, > > > \fBTLSv1.2\fR, and > > > +(if supported by OpenSSL) \fBTLSv1.3\fR. 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 and when the SSL implementation > > > supports > > > +TLSv1.3, the default also includes \fBTLSv1.3\fR. > > > > I'd say we should document in the opposite manner. All the modern > > versions > > of OpenSSL should support TLS 1.3. So, I'd suggest we say that it > > is > > supported unless it's not supported by the SSL implementation. > > Instead > > of saying that it's not supported unless SSL implementation > > supports. > > > > > . > > > .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 62da9febb663a..4f053d17dfccc 100644 > > > --- a/lib/stream-ssl.c > > > +++ b/lib/stream-ssl.c > > > @@ -162,9 +162,15 @@ 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"; > > > > > > +#define BASE_SSL_PROTOS "TLSv1,TLSv1.1,TLSv1.2" > > > +#ifdef SSL_OP_NO_TLSv1_3 > > > +static char *ssl_protocols = BASE_SSL_PROTOS",TLSv1.3"; > > > +#else > > > +static char *ssl_protocols = BASE_SSL_PROTOS; > > > +#endif > > > + > > > /* 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 > > > * security risk.) */ > > > @@ -1284,6 +1290,10 @@ stream_ssl_set_protocols(const char *arg) > > > on_flag = SSL_OP_NO_TLSv1_1; > > > } else if (!strcasecmp(word, "TLSv1")){ > > > on_flag = SSL_OP_NO_TLSv1; > > > +#ifdef SSL_OP_NO_TLSv1_3 > > > + } else if (!strcasecmp(word, "TLSv1.3")){ > > > > I think, we should place a space between ){. Even though existing > > code doesn't do that, there is no point in repeating bad patterns. > > > > > + on_flag = SSL_OP_NO_TLSv1_3; > > > +#endif > > > > I'm not really a big fan of having handling of 1.3 at the end while > > other versions are in descending order, but I also see why you > > placed > > it here. It takes less ifdef-ed code this way. > > > > Alternative suggestion: Maybe we can create a local array of > > structures that will map the name to a flag. And then iterate > > over it in a loop. > > > > What do you think? I don't have a strong opinion. > > Hmm. Also, according to OpenSSL documentation, flags to disable > specific versions are deprecated since 1.1.0: > https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_options.html > > And users advised to use SSL_CTX_set_min/max_proto_version() API > instead (new in 1.1.0): > > https://www.openssl.org/docs/man3.1/man3/SSL_CTX_set_min_proto_version.html > > It might make sense to change the interface altogether, i.e. find > min and max requested version and use the new API. But I'm not sure. I'll look into this, thanks for the suggestion. Dan > > > > > Best regards, Ilya Maximets. >
diff --git a/lib/ssl-connect.man b/lib/ssl-connect.man index 6e54f77ef4d5e..896ce79c6378f 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. -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. +\fIprotocols\fR include \fBTLSv1\fR, \fBTLSv1.1\fR, \fBTLSv1.2\fR, and +(if supported by OpenSSL) \fBTLSv1.3\fR. 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 and when the SSL implementation supports +TLSv1.3, the default also includes \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 62da9febb663a..4f053d17dfccc 100644 --- a/lib/stream-ssl.c +++ b/lib/stream-ssl.c @@ -162,9 +162,15 @@ 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"; +#define BASE_SSL_PROTOS "TLSv1,TLSv1.1,TLSv1.2" +#ifdef SSL_OP_NO_TLSv1_3 +static char *ssl_protocols = BASE_SSL_PROTOS",TLSv1.3"; +#else +static char *ssl_protocols = BASE_SSL_PROTOS; +#endif + /* 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 * security risk.) */ @@ -1284,6 +1290,10 @@ stream_ssl_set_protocols(const char *arg) on_flag = SSL_OP_NO_TLSv1_1; } else if (!strcasecmp(word, "TLSv1")){ on_flag = SSL_OP_NO_TLSv1; +#ifdef SSL_OP_NO_TLSv1_3 + } else if (!strcasecmp(word, "TLSv1.3")){ + on_flag = SSL_OP_NO_TLSv1_3; +#endif } else { VLOG_ERR("%s: SSL protocol not recognized", word); goto exit;