Allow users to request TLS client-side enforcement #525
Conversation
There was a problem hiding this comment.
Thanks for the feature. This is great. Looks good to me.
It might be nice to add a unit test here, but definitely not a deal breaker. I think we could probably add a socket test (around here): https://github.com/datastax/cpp-driver/blob/master/tests/src/unit/tests/test_socket.cpp#L255-L269. Something like set the version on the server to something too low and check for an error, etc.
|
This might be failing because of formatting. Try running |
Test covers trying to connect to a server that only supports TLS 1.0-, while trying to enforce TLS 1.2+ on client-side. As expected, the connection does not succeed.
|
Great - thanks! Unit test added, and appveyor passed with it. I don't have visibility into why the jenkins run failed. I've run |
|
Most of the test failures look to be the following: That test did pass on one platform but failed on several others. That appears to be the main reason for the build failures. |
|
The other build issue looks to just be a formatting problem: That failed on only one of the platform tests |
|
I've corrected the formatting error you pointed out - how do you suggest I proceed with the other fixes? It's going to be difficult for me to conclusively fix test failures that don't occur on my system and also don't occur somewhere I have access to. |
|
@FalacerSelene I agree it's hard for you to resolve build issues without additional information. The build failed due to what appears to be a formatting error on CentOS 6 and what looks to be the same test failure on most (but not all) of the other test platforms. I'll try to do some additional digging, see if I can provide more context on these failures. |
If you could provide (or link to, if they're just public ones) docker images that replicate the failing environment(s) I could run tests in there to check. |
|
Unfortunately I can't easily share the images used to run the various test suites @FalacerSelene but I believe I can provide some additional context. The errors reported by the Jenkins process are as follows:
Let's ignore the Centos6 issue for now and just focus on the test failure. I initially thought this might be an issue with the handling of pre-TLS1.2 OpenSSL libs, but the fail/pass/fail sequence we see with Trusty/Xenial/Bionic on Ubuntu makes that seem far less likely. Fortunately I can reproduce this on a FedoraCore 28 environment. Based on testing there I can confirm that this isn't some kind of intermittent or timing issue; the test fails reliably on multiple runs. The on_socket_closed function used in the callback is pretty straightforward: So I got to wondering if I could divine anything meaningful from the errors reported by SocketConnector. I re-ran the tests with the following change in place: This offered up the following very interesting result: It appears that the client is detecting at SSL handshake time that the server doesn't support a TLS version it can work with. This results in a return of SocketConnector::SOCKET_ERROR_SSL_HANDSHAKE via SocketConnector::ssl_handshake(). Given the way the test is structured this result makes a great deal of sense, but what I simply do not understand is why this issue appears only on some platforms and not on others. It seems like the simplest fix would be a custom callback fn in test_socket.cpp which checks for both possible error returns. I'd feel a lot better if we understood why different errors were returned on different platforms so that we could determine if any additional steps need to happen in the impl generally... but I'm not 💯 convinced that's required for this PR (that could perhaps be moved to follow-up work). |
|
@absurdfarce , happily, I've been able to use a public image of ubuntu 18 to replicate the test issue, and have made your 'simplest fix' suggested change which has fixed it at least on that system. I'm guessing that it's due to subtle changes in openssl between versions, but I'm nowhere near familiar enough with the openssl codebase to work that out quickly. I can see that the jenkins job is still failing though, even with this change. Could you let me know what the new issue is? |
|
@FalacerSelene The remaining failure is for the build on CentOS 6. That distro is EOL and it's use in our build matrix will likely be removed soon so it's not something I'm worried about. I'm doing some planning for the next release and would definitely like to include this change in that release so I'm good with merging it now. If the CentOS 6 failure becomes an issue I'll fix it later. Thanks again for all your work (and your patience!) on this effort! |
|
@mpenick any chance of a new release with this change included? |
|
Hey @kw217, thanks for the question! We're definitely aware that we haven't had a release of the C/C++ driver in a while and have consequently built up a bit of a backlog. We're working on a number of time-sensitive issues at the moment but my hope is to clean out that backlog with a new release sometime this summer. Thanks again! |
|
Thanks @absurdfarce. We are currently building off main, but would be much happier taking a tagged release. A summer release sounds good. |
This change allows users of the driver to request that the TLS version is at least a specified minimum. This helps to avoid downgrade attacks, such as POODLE.
In practice I'll always be wanting to call this to enforce TLS1.2, but I thought it made sense to make it optional, and to allow users to say which TLS version they care about so that the driver remains fully back-compatible and flexible.