Add OpenSSL::SSL::Context#security_level #9831
Conversation
src/openssl/ssl/context.cr
Outdated
| {% if compare_versions(LibSSL::OPENSSL_VERSION, "1.1.0") >= 0 %} | ||
| LibSSL.ssl_ctx_get_security_level(@handle) | ||
| {% else %} | ||
| raise "SSL_CTX_get_security_level not supported" |
There was a problem hiding this comment.
WDYT of using a macro raise here?
There was a problem hiding this comment.
I didn't want to silently ignore with a nop. That's for sure.
I thought that we were on the side of always exposing the same API, despite library versions compiled. If we do a macro raise that is effectively a different API, isn't?
In the recent #cipher_suites= method this is the same situation.
There was a problem hiding this comment.
Sorry, didn't catch it over there :(
Well, I'm not sure how useful a consistent API is if in some environments the compiled binary will just always crash. That's the same as not being able to compile at all right?
So by that standard it would need to print a big fat warning instead of raise.
There was a problem hiding this comment.
It won't always crash. This call might happen based on a configuration and it could in fact never be called for a binary.
There was a problem hiding this comment.
I guess I just don't trust people to a) make sure their "configuration" is actually consistent to whatever OpenSSL version they happen to have, or b) remember to catch exceptions from this call for the scenario of having the wrong OpenSSL version. Rather it'll run into a cycle of "weird, works on my system" until somebody notices that it depends on the OpenSSL version.
So in that regards printing a warning seems still better then, so people at least know to update their "configuration", no?
There was a problem hiding this comment.
I'm fine with a warning and nop in case the operation is not supported due to version mismatch.
It's true that forwarding the responsibility to the user for rescuing or checking the library version is not that comfortable.
Adding require "log" I guess.
There was a problem hiding this comment.
So if my code calls cipher_suites= and it doesn't work because the openssl version is too old, there's no error? The return value even suggests that the cipher suites was correctly set. Since currently you can't even query the configuration on a context instance, there's litterally no way to know whether a call to cipher_suites= was successful. I'd either have to look for a warning message in the log or check the openssl version myself.
I'm very much in doubt that this is a good solution.
There was a problem hiding this comment.
I keep saying this should be a runtime error.
Add docs to allow user discover the need for changing the security_level
On some up to date configurations the ciphers different ciphers lists will not accept tls 1.0 and tls 1.2 connections.
This is because on some platforms the default security level is 2 instead of 1.
This PR exposes
OpenSSL::SSL::Context#security_level/OpenSSL::SSL::Context#security_level=so there is no need to change the global system setting.Ref:
cc: @carlhoerberg