Add a static property to AmqpTcpEndpoint to specify the default SSL protocol versions and use it on ConnectionFactory.SetUri #389

paulomorgado · 2018-02-04T21:40:00Z

Proposed Changes

When creating a connection from an Uri there's no way to specify the accepted TLS versions.

Having a property on AmqpTcpEndpoint that is used by ConnectionFactory.SetUri would allow users of the library to control the default SSL protocol versions used.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

Bugfix (non-breaking change which fixes issue #NNNN)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation (correction or otherwise)
Cosmetics (whitespace, appearance)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

I have read the CONTRIBUTING.md document
I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
All tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)
Any dependent changes have been merged and published in related repositories

Further Comments

If this is a relatively large or complex change, kick off the discussion by
explaining why you chose the solution you did and what alternatives you
considered, etc.

michaelklishin · 2018-02-05T09:00:49Z

projects/client/RabbitMQ.Client/src/client/api/AmqpTcpEndpoint.cs

+        /// <summary>
+        /// The default Amqp SSL protocols.
+        /// </summary>
+        public static SslProtocols DefaultAmqpSslProtocols { get; set; } = SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12;


Does it make sense for this field to be an AmqpTcpEndpoint field and not, say, ConnectionFactory?

I agree it would make sense to keep global config in the connection factory.

That was my first thought. But than I saw a DefaultAmqpSslPort in AmqpTcpEndpoint...

For me, it makes a lot more sense to honor the SSL options already part of ConnectionFactory when using an URI than having AmqpTcpEndpoint run free with a mind if its own.

Whst do you think?

I'd move both this field and DefaultAmqpSslPort to connection factory and bump the version to 5.2.0. @kjnilsson WDYT?

Moving DefaultAmqpSslPort to a different class is a breaking change, so a new major is needed if you do that.

Sounds like we have a consensus then? Note that the port property move ideally should be in a separate PR.

Property or constant filed?

And the DefaultAmqpSslProtocols would still be a static property, right? And with the good defaults, right?

So, what's the verdict?

@paulomorgado in this PR, let's move the field to ConnectionFactory. In a separate one for master, move the port one with a backwards compatible reference. Both @kjnilsson and I are leaning towards using properties for no particular reason, merely as a matter of taste.

Thank you!

It's not a matter of taste. It's semantic.

I've split SSL protocols settings into static and instance properties and named them to convey the meaning that they will only be used with URIs.

As for the default port, that one can be overridden in the URI and, isn't 5671 the default as 80 is for HTTTP and 443 for HTTPS? If it is, it's pretty fine where it is. System.Uri has default ports per syntax.

michaelklishin · 2018-02-05T09:01:29Z

projects/client/RabbitMQ.Client/src/client/api/AmqpTcpEndpoint.cs

@@ -138,6 +140,11 @@ public AmqpTcpEndpoint CloneWithHostname(string hostname)
            return new AmqpTcpEndpoint(hostname, _port, Ssl);
        }

+        /// <summary>
+        /// The default Amqp SSL protocols.


How about "TLS protocol versions that will be used by default"?

michaelklishin

The idea is fine and I see no regressions but I'd like to see the field moved to ConnectionFactory.

bording · 2018-02-11T20:46:51Z

projects/client/RabbitMQ.Client/src/client/api/ConnectionFactory.cs

+        /// <summary>
+        /// The default AMQP URI SSL protocols.
+        /// </summary>
+        public static SslProtocols DefaultAmqpUriSslProtocols { get; set; } =


Why does this have a setter? If that's the default, shouldn't it be get-only?

In case the user want's to change the default globally.

For this particular property it sounds OK to me to allow for overriding.

AmqpUriSslProtocols would actually be the one overriding the default.

Then I agree with @bording: the setter can be dropped. We don't need two ways to override the default.

The intention was to have a wider default that would apply to all connection factories if not changed and a setting for each connection factory.

For instance, I want all connections to be Tls but I want the connections created from a particular connection factory to be Tls12.

Otherwise, it doesn't make much chance to have the static property at all.

Fair enough, the setter is something we can accept.

bording · 2018-02-11T20:54:29Z

Given that one of the new properties is DefaultAmqpUriSslProtocols, that should be used regardless of how I set up the properties of the connection factory. There doesn't appear to be anything that sets this value on the Ssl property outside of calling SetUri.

kjnilsson · 2018-02-12T10:26:47Z

Should DefaultAmqpUriSslProtocols be consulted here: https://github.com/paulomorgado/rabbitmq-dotnet-client/blob/ca989563b55abaf4835dfe9c4c575bc19b74f5f6/projects/client/RabbitMQ.Client/src/client/api/ConnectionFactory.cs#L253

paulomorgado · 2018-02-12T13:42:49Z

Should DefaultAmqpUriSslProtocols be consulted here: https://github.com/paulomorgado/rabbitmq-dotnet-client/blob/ca989563b55abaf4835dfe9c4c575bc19b74f5f6/projects/client/RabbitMQ.Client/src/client/api/ConnectionFactory.cs#L253

I don't think so. @kjnilsson. That's why I named the properties with AmqpUri.

If the user is not using an Uri, then she/he must define every parameter: Nevertheless, DefaultAmqpUriSslProtocols can be used by user.

paulomorgado · 2018-03-14T11:08:17Z

I've been away for a few days. Any update on this?

michaelklishin · 2018-03-15T00:50:15Z

@kjnilsson I'd like to drop one setter and modify a comment. Happy to do that on my own. Let me know if you have any objections.

… protocol versions and use it on ConnectionFactory.SetUri

…ry as DefaultAmqpUriSslProtocols Added per-instance AmqpUriSslProtocols to ConnectionFactory.

paulomorgado · 2018-03-15T10:16:48Z

The build is ending in error with

Couldn't find 'project.json' in '.\projects\client\Unit'

But the PR only touches one .cs file. Why is this failing?

paulomorgado · 2018-03-16T09:03:40Z

Maybe we shouldn't provide a default out of the box.

Transport Layer Security (TLS) best practices with the .NET Framework

michaelklishin · 2018-03-16T09:47:13Z

Not providing a default is usually a very bad idea for libraries. It's not like if you don't provide a default, your users will make an informed decisions or will read the docs. No, some will hack it until it stops complaining and move on.

michaelklishin · 2018-03-16T09:49:21Z

The guide says

To ensure .NET Framework applications remain secure, the TLS version should not be hardcoded

we don't hardcode it and won't be hardcording it with this PR. And also

We recommend that you don't use Default; setting SslProtocols.Default
forces the use of SSL 3.0 /TLS 1.0 and prevent TLS 1.2

I don't think we use SslProtocols.Default and RabbitMQ won't accept SSLv3 connections anyway.

So I'm not sure what the argument (and suggested alternative) for "not having a library default" is. Please elaborate?

paulomorgado · 2018-03-18T23:56:53Z

We are creating a default for URIs. And in the PR I've set the default for Tls | Tls11 | Tls12. The default should be None, I think.

michaelklishin · 2018-03-19T00:10:59Z

Why should it be None? What would happen if the user never overrides it?

paulomorgado · 2018-03-19T01:17:03Z

It's my understanding of the recommendation. Am I wrong?

What should the default be, than?

michaelklishin · 2018-03-19T11:57:51Z

Merging per verbal approval from @kjnilsson. The defaults seem OK for the time being, we may revisit the decision to include TLSv1.0 into the default list within a year.

paulomorgado · 2018-03-19T21:00:46Z

Merging per verbal approval from @kjnilsson. The defaults seem OK for the time being, we may revisit the decision to include TLSv1.0 into the default list within a year.

What do you mean, @michaelklishin? The default is SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12.

michaelklishin · 2018-03-20T06:02:49Z

@paulomorgado TLSv1.0 is no longer considered to be suitable for some environments but dropping it would be a breaking change.

paulomorgado · 2018-03-20T07:57:17Z

When will this be available on NuGet?

michaelklishin · 2018-03-20T08:08:08Z

@paulomorgado we make no promises about GA releases. A new 5.1.x milestone can be released shortly.

paulomorgado mentioned this pull request Feb 4, 2018

Consider allowing the definition of the default TLS version #388

Closed

michaelklishin reviewed Feb 5, 2018

View reviewed changes

michaelklishin requested changes Feb 5, 2018

View reviewed changes

bording reviewed Feb 11, 2018

View reviewed changes

paulomorgado added 2 commits March 15, 2018 09:22

added a static property to AmqpTcpEndpoint to specify the default SSL…

011d097

… protocol versions and use it on ConnectionFactory.SetUri

Moved DefaultAmqpSslProtocols from AmqpTcpEndpoint to ConnectionFacto…

bf2a799

…ry as DefaultAmqpUriSslProtocols Added per-instance AmqpUriSslProtocols to ConnectionFactory.

michaelklishin approved these changes Mar 19, 2018

View reviewed changes

michaelklishin merged commit 18419cb into rabbitmq:master Mar 19, 2018

rabbitmq locked as resolved and limited conversation to collaborators Mar 20, 2018

Add a static property to AmqpTcpEndpoint to specify the default SSL protocol versions and use it on ConnectionFactory.SetUri #389

Add a static property to AmqpTcpEndpoint to specify the default SSL protocol versions and use it on ConnectionFactory.SetUri #389

Conversation

paulomorgado commented Feb 4, 2018

Proposed Changes

Types of Changes

Checklist

Further Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelklishin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomorgado Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bording commented Feb 11, 2018

kjnilsson commented Feb 12, 2018

paulomorgado commented Feb 12, 2018 • edited Loading

paulomorgado commented Mar 14, 2018

michaelklishin commented Mar 15, 2018

paulomorgado commented Mar 15, 2018

paulomorgado commented Mar 16, 2018

michaelklishin commented Mar 16, 2018

michaelklishin commented Mar 16, 2018

paulomorgado commented Mar 18, 2018

michaelklishin commented Mar 19, 2018

paulomorgado commented Mar 19, 2018

michaelklishin commented Mar 19, 2018

paulomorgado commented Mar 19, 2018

michaelklishin commented Mar 20, 2018

paulomorgado commented Mar 20, 2018

michaelklishin commented Mar 20, 2018

paulomorgado Mar 15, 2018 •

edited

Loading

paulomorgado commented Feb 12, 2018 •

edited

Loading