Fix OpenSSL::SSL::SSLContext#min_version= failure #215
Conversation
|
Thanks for tackling this!
Perhaps C Ruby is using whatever is set in the openssl config? |
| if (version.isNil()) | ||
| return 0; | ||
| if (version instanceof RubyFixnum) { | ||
| return (int) ((RubyFixnum) version).getLongValue(); |
There was a problem hiding this comment.
let's just to getIntValue() here so we handle overflown values
There was a problem hiding this comment.
Unfortunately there is no getIntValue().
But I double checked and found static int fix2int(RubyFixnum arg)
There was a problem hiding this comment.
right, yeah that one doing the int check before cast.
There was a problem hiding this comment.
Done, thanks
src/test/ruby/ssl/test_socket.rb
Outdated
|
|
||
| #ssl_server = server | ||
| begin | ||
| ssl_client = client(443, host: "google.com", min_version: OpenSSL::SSL::TLS1_VERSION, max_version: OpenSSL::SSL::TLS1_1_VERSION, ciphers: nil) |
There was a problem hiding this comment.
believe we want some unit tests (done above except for the set_minmax_proto_version) for the version parsing/mapping instead of an integration tests here.
also not sure hard-coding what google has on their servers atm.
anyhow it's a good start but we might want to do an integration tests for this elsewhere.
There was a problem hiding this comment.
Yes, totally agree.
This PR will still need to be polished, more tests added.
But first I believe that master branch should be stabilized.
There was a problem hiding this comment.
Ok, I've added integration test in src/test/integration/ssl_test.rb like you suggested.
test_socket.rb still needs improvement.
There was a problem hiding this comment.
should we remove this than, it's very dependend on what google.com has set on its servers, right?
There was a problem hiding this comment.
Yes, let me remove it later since I've added the integration test.
What do you think about the integration test then?
Later I will try to rework test in test_socket.rb or remove completely since it cannot be tested now due to another issue (server method fails with no ciphers match) at least on my env.
There was a problem hiding this comment.
Yes, let me remove it later since I've added the integration test.
sorry but I would prefer not to deal with leftovers as they will bite someone later.
I am fully aware CI testing is broken atm but that is no reason to leave externally dependent tests here.
you can keep it around for yourself in a follow-up branch.
What do you think about the integration test then?
we do not run those atm regularly, so I am fine with having them as you did. thanks.
There was a problem hiding this comment.
Done. I Removed this test.
Instead I've got inspired by test_ssl.rb and implemented parameterized test for this issue.
lib/jopenssl23/openssl/ssl.rb
Outdated
| @@ -214,7 +214,7 @@ def ssl_version=(meth) | |||
| raise ArgumentError, "unknown SSL method `%s'" % meth | |||
| set_minmax_proto_version(version, version) | |||
| @min_proto_version = @max_proto_version = version | |||
| end unless method_defined? :ssl_version= | |||
There was a problem hiding this comment.
let's keep this as is as it seems unrelated to the PR changes?
There was a problem hiding this comment.
I believe I had a reason to remove this. I compared this with CRuby version: https://github.com/ruby/ruby/blob/ruby_2_6/ext/openssl/lib/openssl/ssl.rb#L213
which does not have unless method_defined? :ssl_version=
I'll double check it and revert if it's fine.
There was a problem hiding this comment.
Ok, removed.
Only one smalll difference, unrealistic though, (was successful before, now fails with no protocols available):
ctx.ssl_version = "TLSv1"
ctx.min_version = OpenSSL::SSL::TLS1_VERSION
ctx.max_version = OpenSSL::SSL::TLS1_2_VERSION
There was a problem hiding this comment.
Thanks, will need to 👀 at this since it changed behavior - which I did not expect to happen.
MariuszCwikla
force-pushed
the
issue_6409_ssl_min_version
branch
2 times, most recently
from
645a219 to
1ff15d2
Compare
MariuszCwikla
force-pushed
the
issue_6409_ssl_min_version
branch
from
1ff15d2 to
4ddcdb0
Compare
MariuszCwikla
force-pushed
the
issue_6409_ssl_min_version
branch
2 times, most recently
from
166bd84 to
303f6c8
Compare
MariuszCwikla
force-pushed
the
issue_6409_ssl_min_version
branch
from
303f6c8 to
2d91787
Compare
|
I had to rework this due to issue introduce by my previous commit. I hope now it's fine. It's bit greener now. This test also works fine with CRuby. Try this: |
This is attempt to fix jruby/jruby#6409.
I couldn't test it with unit tests due to other issues. However I tested it with following script
issue_6409.zip
I compared results between ruby and jruby and it seems it's working almost the same, e.g.
I believe these discrepancies could be ignored.
I attempted to implement unit test
test_minmax_ssl_versionhoweverservermethod fails even on master withno cipher matcherror. So firstly master branch should be stabilized due to this and other issues before this PR can be finished.This is still WIP, however comments are welcome.