net, openssl: support setting the minimum supported SSL/TLS version #14556
Conversation
alaviss
force-pushed
the
versioned-ssl
branch
4 times, most recently
from
0190f14 to
427615d
Compare
| const protTLS* = protTLSv1_2 | ||
| ## The recommended minimum TLS version with adequate security and | ||
| ## compatibility with the Internet. |
There was a problem hiding this comment.
I'd want this to be in ssl_config, but then it'd cause cyclic imports...
|
Ping @dom96 @FedericoCeratto Can I have some eyeballs on this please? |
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
LGTM. But I'd like someone else's eyes on this too so I'll leave to them to merge.
| users from the use of weak and insecure ciphers while still provides | ||
| adequate compatiblity with the majority of the Internet. | ||
|
|
||
| - `net.newContext` drops support for setting the _exact_ version of SSL/TLS. |
There was a problem hiding this comment.
Setting only a minimum version is good but perhaps not flexible enough. As it happened in the past, a newer version can have a critical vulnerability that is not present in previous versions. Users might want to either disable that version or explicitly set a list of enabled versions.
(this is why SSL used flags v2 / v3 / v23)
There was a problem hiding this comment.
This is a breaking change, so I want to push it first.
That said, I'm not sure how to add a "max version" parameter, as that would mean we are artificially limiting the maximum version with whatever the stdlib have at the time (assuming a default parameter of type SslProtVersion). It might be better to provide "maximum version" as a separate proc, so it would be opt-in.
There was a problem hiding this comment.
I'd also like to be able to set the maximum version, turning off tls1.3 is the only way i know to force a client to only use the cipher you specify, e.g. for tls-psk.
as for how, it would be enough to include a SSL_CTX_set_max_proto_version proc in openssl, I dont think it needs to be any friendlier than that
| var newCTX = SSL_CTX_new(SSLv23_method()) | ||
| let minVersion: cint = | ||
| case minProtVersion | ||
| of protTLSv1, protSSLv23: |
There was a problem hiding this comment.
protSSLv23 is silently using TLS1 only?
There was a problem hiding this comment.
Yes, for compatibility with the original, TLS1 is set as the lowest supported version. See https://github.com/nim-lang/Nim/pull/14556/files#diff-123d798b39756a95de82d119d2fe6661157ebb20407e2c394b7490e7588d2294R97 for the documentation.
alaviss
force-pushed
the
versioned-ssl
branch
from
77933ce to
ebd4d42
Compare
|
Rebased with |
|
needs rebase again |
This commit brings minimum supported TLS version setting to Nim, which replaces the exact version system used by `net.newContext`. For more information consult the changelog entry associated with this change.
|
Rebased with |
| proc SSL_CTX_set_min_proto_version*(ctx: SslCtx, version: cint): cint = | ||
| ## Set the minimum supported protocol version. | ||
| const MainProc = "SSL_CTX_set_min_proto_version" | ||
| let theProc {.global.} = cast[proc(ctx: SslCtx, version: cint): cint {.cdecl, gcsafe.}](sslSymNullable(MainProc)) |
There was a problem hiding this comment.
These .global procs are bad style, prefer real globals instead. Also: You probably want .raises: [], locks: 0
There was a problem hiding this comment.
Wouldn't that mean I'd have to produce an unique global name for every proc?
|
This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions. |
6 participants
Depends on #14542(EDIT: Merged todevel)Fixes #13884
@dom96 @FedericoCeratto