Skip to content
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

Open
wants to merge 1 commit into
base: develop-2.9
Choose a base branch
from

Conversation

llelf
Copy link

@llelf llelf commented Mar 14, 2019

@martinsumner
Copy link

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)?

@llelf
Copy link
Author

llelf commented Mar 14, 2019

What do you mean? (develop-2.9 should pass with it.)

@martinsumner
Copy link

Ah. I tried, but it still failed, with the same crash caused.

The log showed that the extra cacert had been passed in though.

@martinsumner
Copy link

I will try clearing things down and doing a fresh install, in case there is some left over from an earlier test.

@martinsumner
Copy link

FYI, here are the ciphers being passed in the ssl_opts to mochiweb:

{ciphers,[<<"À'">>,<<"À#">>,<<192,19>>,<<"À\t">>,<<"À(">>,<<"À$">>,<<192,20>>,<<"À\n">>,<<0,103>>,<<0,51>>,<<0,64>>,<<0,107>>,<<0,56>>,<<0,57>>,<<192,17>>,<<192,7>>,<<192,31>>,<<192,30>>,<<0,50>>,<<"À)">>,<<"À%">>,<<192,14>>,<<192,4>>,<<0,60>>,<<0,47>>,<<"À\"">>,<<"À!">>,<<0,106>>,<<"À*">>,<<"À&">>,<<192,15>>,<<192,5>>,<<0,61>>,<<0,53>>,<<0,5>>]}

@martinsumner
Copy link

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):

Reason:     {badarg,[{mochiweb_socket,'-filter_broken_cipher_suites/1-fun-0-',1,[{file,"src/mochiweb_socket.erl"},{line,41}]},{lists,'-filter/2-lc$^0/1-0-',2,[{file,"lists.erl"},{line,1271}]},{mochiweb_socket,add_unbroken_ciphers_default,1,[{file,"src/mochiweb_socket.erl"},{line,34}]},{mochiweb_socket,listen,4,[{file,"src/mochiweb_socket.erl"},{line,20}]},{mochiweb_socket_server,listen,3,[{file,"src/mochiweb_socket_server.erl"},{line,224}]},{gen_server,init_it,6,[{file,"gen_server.erl"},{line,304}]},{proc_lib,init_p_do_apply,3,[{file,"proc_lib.erl"},{line,239}]}]}

I don't think the ciphers being passed in in the ssl_opts are atoms as expected by the mochiweb filter logic.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

FYI, here are the ciphers being passed in the ssl_opts to mochiweb:
{ciphers,[<<"À'">>,<<"À#">>,<<192,19>>, ⟨…⟩

They are coming from

9 Ɛ⟩ [  catch ssl_cipher:openssl_suite(C) || C <- string:tokens(riak_core_security:get_ciphers(), ":") ].     
[<<"À/">>,<<"À+">>,<<"À0">>,<<"À,">>,
  ⋮

What are you testing on? Mac?

@martinsumner
Copy link

Yes .. but I'm going to test on ubuntu now!

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Ah I know why it passes for me and fails for you

filter_broken_cipher_suites(Ciphers) ->
	case proplists:get_value(ssl_app, ssl:versions()) of
		"5.3" ++ _ ->

On mac it's "8.1.3.1.1", on Ubuntu "5.3.2"

@martinsumner
Copy link

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.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

@martinsumner so what to change? mochiweb or riak_core?

@martinsumner
Copy link

When you said:

On mac it's "8.1.3.1.1", on Ubuntu "5.3.2"

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?

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Yes, doesn't make any sense. My riak_test setup is broken probably.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Alright. Now it fails for me too.

So… what to change, mochiweb or riak_core?
Because it has nothing to do with the yz test actually, just riak start will happily fail too.

@martinsumner
Copy link

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.

@martinsumner
Copy link

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.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Yeah, was just about to ask if ./riak_test -c rtdev -t ./ebin/http_security.beam works for you.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

@martinsumner I think it's just riak_api_ssl:options/0
which does

riak_core_ssl_util:parse_ciphers(riak_core_security:get_ciphers()).

to get good/bad ciphers.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

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"

ssl:listen is happy with either. And with {ecdhe_rsa,aes_128_gcm,null,sha256}. And with #{key_exchange => ecdhe_rsa,…}.

@martinsumner
Copy link

martinsumner commented Mar 14, 2019

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?

@martinsumner
Copy link

Or perhaps we should just parse the ciphers in Riak, and remove the filter from mochiweb - avoid converting to and fro.

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Doing everything in riak_core is probably better, so we don't get different cipher lists for pb and http.

@martinsumner
Copy link

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?

@llelf
Copy link
Author

llelf commented Mar 14, 2019

Sounds good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants