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