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