refine fix for deadlock by using non-blocking f_recv #125
Conversation
| @@ -113,24 +111,23 @@ end | |||
|
|
|||
| function f_send(c_ctx, c_msg, sz) | |||
| jl_ctx = unsafe_pointer_to_objref(c_ctx) | |||
| jl_msg = unsafe_wrap(Array, c_msg, sz) | |||
| return Cint(write(jl_ctx.bio, jl_msg)) | |||
| return Cint(unsafe_write(jl_ctx, c_msg, sz)) | |||
There was a problem hiding this comment.
Avoid allocating wrapper Array.
Just dump the bytes directly into the LibuvStream send buffer.
| n = readbytes!(jl_ctx.bio, jl_msg, sz) | ||
| n = nb_available(jl_ctx) | ||
| if n == 0 | ||
| return Cint(MBEDTLS_ERR_SSL_WANT_READ) |
There was a problem hiding this comment.
If there is nothing in the LibuvStream receive buffer, tell MbedTLS that it needs to call f_recv again later.
| end | ||
| n = min(sz, n) | ||
| unsafe_read(jl_ctx, c_msg, n) |
There was a problem hiding this comment.
avoid blocking by not trying to read more than is available in the LibuvStream receive buffer.
src/ssl.jl
Outdated
| if n != MBEDTLS_ERR_SSL_WANT_READ | ||
| mbed_err(n) | ||
| end | ||
| Base.wait_readnb(ctx.bio, 1) |
There was a problem hiding this comment.
If mbedtls_ssl_handshake wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.
There was a problem hiding this comment.
I think eof is commonly the more general thing to wait for, but are basically equivalent
src/ssl.jl
Outdated
| if n != MBEDTLS_ERR_SSL_WANT_READ | ||
| n < 0 && mbed_err(n) | ||
| elseif n == MBEDTLS_ERR_SSL_WANT_READ | ||
| Base.wait_readnb(ctx.bio, 1) |
There was a problem hiding this comment.
If mbedtls_ssl_read wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.
|
This is looking really good! I hope it works :) |
|
I hope it works too :) I'm not arguing that we should change, I'd just like to understand why it is the way it is? |
|
AFAIU, OpenSSL doesn't have the greatest licensing situation. MbedTLS is a small, simple library w/ good licensing and C api, so it's a natural fit. I think it'd be great to have an |
Can you be more specific? As far as I can see (IANAL) it says: do whatever you like but give us credit and don't blame us if its broken.
Is it not proving to be tricky to make MbedTLS work? |
Codecov Report
@@ Coverage Diff @@
## master #125 +/- ##
==========================================
+ Coverage 65.03% 65.97% +0.93%
==========================================
Files 10 10
Lines 429 432 +3
==========================================
+ Hits 279 285 +6
+ Misses 150 147 -3
Continue to review full report at Codecov.
|
|
@malmaud, I see that this was your baby way back at the start of the commit log.
I'm left with a concern about the implied responsibility I'm taking on by making changes here. Users of a ctypto library have a reasonable expectation that it will keep their sensitive information secret. But I don't feel like I have enough experience with crypto to feel confident that the code will meet that expectation without more thorough review. So, I'd really like to know a bit about the history of why MbedTLS was chosen at the time so that I can evaluate the tradeoffs I find in front of me today. |
…l block before mbedtls has a chance to do its thing
|
It was mostly because of the flexible licensing - mbedtls is dual-licensed under Apache and GPL. Mbedtls generally ships with Julia to power the cryptography of the package manager. Mbedtls is used by many big companies - I don't think there's any reason to believe that it is less secure than other major crypto libraries. I'm also skeptical that the Julia wrapper could introduce security issues, since it is not handling any sensitive cryptography itself, but I guess it's theoretically possible. |
|
FWIW, the OpenSSL license is currently incompatible with the GPL, but they are fixing this by relicensing under Apache. Apparently there remains only very few contributors who haven't given their agreement, so this should be ready soon. |
|
Thanks for that @malmaud. So I found this discussion JuliaLang/julia#10763 about how FFTW's GPL license forbids the OpenSSL licence's requirement for giving credit to the authors. Seems like a good reason to remove FFTW, but so be it. Interestingly it looks like OpenSSL will be apache licensed RSN : https://www.openssl.org/blog/blog/2018/03/01/last-license/ I had a quick play with s2n yesterday, it certainly has a simple clean API and comes from a vendor with serious motivation to do security right. One nice feature of s2n is that it handles integration with the underlying BSD sockets for you, so you don't have to worry about getting the callback functions exactly right (subject of this PR). OSX has https://developer.apple.com/documentation/security/secure_transport I assume Windows has some MS-blessed system TLS API, and linux has OpenSSL. It would be great to be able to ship stuff to clients, and when the next TLS vulnerability comes along, just be able to say to them "don't worry, Julia uses the OS TLS library and the OS vendor has already pushed out a fix". I think it would be nice to have a JuliaTLSClient package with OS native backends and a very small API like: tls_connect(host, port) -> TLSHandle
unsafe_read(::TLSHandle, buf, nb) # blocking, calls Base.poll_fd() to run main event loop
unsafe_write(::TLSHandle, buf, nb) # blocking, "
wait(::TLSHandle) # wait for there to be bytesavailable
bytesavailable(::TLSHandle) -> Int |
|
@nalimilan you beat me to it re OpenSSL apache license :) |
|
I think having a I've also been keeping an eye on https://bearssl.org/, which seems nice from a small footprint/good license standpoint. |
|
In any case, we could open an issue on the JuliaWeb/roadmap repo to further discuss future ssl/tls options, but for now, is this PR ready to go? CI looks good (0.7 are known failures due to BinaryProvider, which should be fixed soon). I'm happy to merge and tag (I plan on doing some dev-load testing today, so I can certainly report any issues I see). |
|
It seems that Swift has the same goal of implementing a high-level SSL API using the system libraries https://forums.swift.org/t/pitch-cross-platform-swift-tls-library/5610/5 |
|
#123 (comment) should be resolved before tagging, unless the tag is from a branch that includes this but not the BinDeps change. All the descriptive comments would be much better in the code than in this PR, in terms of being able to find and use them later. |
|
Ya, the conversation about whether MbedTLS is the best choice for a particular purpose seems to make more sense for the repos that use MbedTLS, like Julia's libgit2 fork and HTTP.jl. FWIW, I think a package that integrates with an OS's native TLS toolkit would be great, although I still don't think there's any reason to doubt the actual security aspects of MbedTLS. |
I'm happy for you to merge, but I'd hold of on the tag until after you've done your "dev-load testing". |
Aside from the general degree to which one should doubt the security of everything, I agree. However, it still seems preferable to ask a user to trust the OS vendor that they have already decided to trust, rather than ask them to trust that the Julia guys have no doubts about MbedTLS. If I take my engineer hat off and put my manager hat on, I'm going to be thinking "My dev want's to deploy a module using this Julia thing that the data guys are all raving about. But what dependencies does it pull in? Do any of them have a privacy compliance risk? Are we going to have to get experts in to do |
I don't know if I'll have time to work on this any time soon, but I though it was worth making some notes given that it's been on my mind. I've sketched out some ideas here: Comments are welcome. |
|
I'll just say that for historical context, there have been somewhere between 2-5 distinct attempts at writing OpenSSL bindings for Julia in the past. I'm not going to try to comprehensively list them, but you can look around and find them - to my knowledge none have seen any development at any time within the last year or two, or longer. There was GnuTLS.jl which was actively used for a while (mostly for license compatibility reasons) but it had its set of problems and fell out of favor rapidly as soon as Jon wrote this package and got it up and functional. When you work with OpenSSL, you also need to worry about how its API has changed over time, and the widely varying versions out there in the wild on different ages of Linux distro. The forks like LibreSSL and BoringSSL are also viable options. I don't know whether anyone has tried to write Julia bindings for WinHTTP or SecureTransport. https://blog.regehr.org/archives/1261 is also an interesting anecdote, but from a few years ago. |
|
Good points Tony, let's continue over here: JuliaWeb/TLSClient.jl#1 (comment) |
|
@quinnj are we ready to tag this yet? |
No description provided.