-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
yz_security test: forgotten CA cert for riak_core config #752
base: develop-2.9
Are you sure you want to change the base?
Conversation
Thanks. Just double-checking, but this isn't something you expect to change the outcome of the test (i.e. develop-2.9 will still fail, and Riak 2.2.6 will still pass)? |
What do you mean? (develop-2.9 should pass with it.) |
Ah. I tried, but it still failed, with the same crash caused. The log showed that the extra cacert had been passed in though. |
I will try clearing things down and doing a fresh install, in case there is some left over from an earlier test. |
FYI, here are the ciphers being passed in the ssl_opts to mochiweb:
|
I still get this as the problem. The riak node crashes when the test attempts to start the TLS listener in Riak (with mochiweb crashing):
I don't think the ciphers being passed in in the ssl_opts are atoms as expected by the mochiweb filter logic. |
They are coming from
What are you testing on? Mac? |
Yes .. but I'm going to test on ubuntu now! |
Ah I know why it passes for me and fails for you
On mac it's "8.1.3.1.1", on Ubuntu "5.3.2" |
Ah. It is failing for me on ubuntu as well, but again perhaps that is a version of ubuntu thing. I have within erl the same values for both mac osx and ubuntu e.g. ssl:versions() returns {ssl_app,"5.3.1"} and the same cipher suites. I will riak attach and see what I get out of riak_core_security:get_ciphers/1. Bear with me. |
@martinsumner so what to change? mochiweb or riak_core? |
When you said:
Is that the right way round? Does it pass for you when the ssl version is 8.1.3, and hence the filtering of the ciphers doesn't happen? |
Yes, doesn't make any sense. My riak_test setup is broken probably. |
Alright. Now it fails for me too. So… what to change, mochiweb or riak_core? |
I don't know where the ciphers (in the wrong format) are coming from. I had assumed that ciphers were being read from one of the pem files, and hence why the test prompted the failure. But that was just a guess. Based on my reading of the code in riak_core_security, I can't explain why the ciphers being passed through are these short binaries. There are some ssl tests in riak_test that are being run, and are passing. So I'm going to look at those now. |
Ah. In the test suite we don't run in the mainstream tests 'riak_test/http_security', and this fails as well for the same reason. This is using the same cert files and key files. I'm going to play around and see if I can trace how riak is fetching/passing ciphers. |
Yeah, was just about to ask if |
@martinsumner I think it's just
to get good/bad ciphers. |
So you can go between those those ciphers “forms”: 21 Ɛ⟩ ssl_cipher:openssl_suite("ECDHE-RSA-AES128-GCM-SHA256").
<<"À/">>
22 Ɛ⟩ ssl_cipher:openssl_suite_name(<<"À/">>).
"ECDHE-RSA-AES128-GCM-SHA256"
|
Ah yes, I can see the conversion here https://github.com/erlang/otp/blob/OTP_R16B03/lib/ssl/src/ssl_cipher.erl#L750-L857 https://github.com/erlang/otp/blob/OTP_R16B03/lib/ssl/src/ssl_cipher.hrl so the mochiweb filter cares what form they're passed in as, and riak is converting from the text form to the binary short codes - which is the form the filter doesn't like. I suspect we should change mochiweb to convert before it filters depending on the form it receives them? |
Or perhaps we should just parse the ciphers in Riak, and remove the filter from mochiweb - avoid converting to and fro. |
Doing everything in riak_core is probably better, so we don't get different cipher lists for pb and http. |
So I intend to remove the filter from mochiweb, and then filter out the default ciphers from this list: https://github.com/basho/riak_core/blob/develop-2.9/src/riak_core_security.erl#L38-L61 So if you want to pass in a broken cipher you can, but by default riak_core won't use any of the ECDH* ciphers. Does this seem reasonable? |
Sounds good |
/cc @martinsumner