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