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

refine fix for deadlock by using non-blocking f_recv #125

Merged
merged 4 commits into from
Mar 6, 2018

Conversation

samoconnor
Copy link
Contributor

No description provided.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@@ -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))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If mbedtls_ssl_handshake wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think eof is commonly the more general thing to wait for, but are basically equivalent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

palm--->face!!

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If mbedtls_ssl_read wants more data, wait for at least one byte to be available in the LibuvStream receive buffer before trying again.

@samoconnor samoconnor mentioned this pull request Mar 4, 2018
@vtjnash
Copy link
Member

vtjnash commented Mar 4, 2018

This is looking really good! I hope it works :)

@samoconnor
Copy link
Contributor Author

samoconnor commented Mar 4, 2018

I hope it works too :)
But I'm left wondering why we are using MbedTLS?
It feels like it is intended for the embedded world. All the stuff I see on the forums is people working on tiny ARM systems.
Given that Julia's audience is (currently) number crunching bofins with i7 laptops and compute clusters, why are we not using the platform OpenSSL, Security.SecureTransport, or whatever the Windows equivalent is? or s2n?

I'm not arguing that we should change, I'd just like to understand why it is the way it is?

@quinnj
Copy link
Member

quinnj commented Mar 4, 2018

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 SSL.jl package that hooked into whatever platform-provided SSL was available, but that also sounds tricky to make work in a unified interface + get enough platform coverage so that anyone could use it. MbedTLS definitely has the advantage that it can pretty much be bundled/used on any platform.

@samoconnor
Copy link
Contributor Author

OpenSSL doesn't have the greatest licensing situation

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.

tricky to make work

Is it not proving to be tricky to make MbedTLS work?

@codecov-io
Copy link

codecov-io commented Mar 4, 2018

Codecov Report

Merging #125 into master will increase coverage by 0.93%.
The diff coverage is 82.6%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/ssl.jl 69.35% <82.6%> (+3.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af7ab92...961b185. Read the comment docs.

@samoconnor
Copy link
Contributor Author

@malmaud, I see that this was your baby way back at the start of the commit log.
I don't intend to be critical of the effort that has been put it, or of the decisions that were made along the way. But, I find myself in a position where:

  • There is no longer an active owner of the code.
  • I'm spending time fixing the problems I find.
  • I have found memory corruption issues, which is worrying in a cyrpto context.
  • My interactions with the upstream vendor are not filling me with confidence https://esp32.com/viewtopic.php?t=1101#p4945.

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.

@malmaud
Copy link
Contributor

malmaud commented Mar 5, 2018

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.
Of course as you've discovered, the wrapper seems to have other (non-security-related) issues, so I think it's great that you're looking into them.

@nalimilan
Copy link
Member

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.

@samoconnor
Copy link
Contributor Author

samoconnor commented Mar 5, 2018

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

@samoconnor
Copy link
Contributor Author

@nalimilan you beat me to it re OpenSSL apache license :)
I think on Linux that would be moot anyway because of the GPL's ok-to-link-with-built-in-os-libs clause.

@quinnj
Copy link
Member

quinnj commented Mar 5, 2018

I think having a SSL.jl package that wrapped OS-native functionality would be great; it sounds non-trivial to me to get all the platforms to conform to a single api, but I'm also very unfamiliar w/ what all those APIs look like.

I've also been keeping an eye on https://bearssl.org/, which seems nice from a small footprint/good license standpoint.

@quinnj
Copy link
Member

quinnj commented Mar 5, 2018

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

@nalimilan
Copy link
Member

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

@tkelman
Copy link
Contributor

tkelman commented Mar 5, 2018

#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.

@malmaud
Copy link
Contributor

malmaud commented Mar 5, 2018

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.

@samoconnor
Copy link
Contributor Author

I'm happy to merge and tag

I'm happy for you to merge, but I'd hold of on the tag until after you've done your "dev-load testing".

@samoconnor
Copy link
Contributor Author

I still don't think there's any reason to doubt the actual security aspects of MbedTLS

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 SOUP reviews?". As a dev I wan't to be able to say "don't worry, all the comms goes through the same crypto we use for everything else, it's already been certified".

@samoconnor
Copy link
Contributor Author

Re: ... a package that integrates with an OS's native TLS toolkit ...

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:
https://github.com/samoconnor/TLSClient.jl/blob/master/README.md

Comments are welcome.
@quinnj, @malmaud, @vtjnash, @nalimilan, @tkelman, @yuyichao, @Keno

@tkelman
Copy link
Contributor

tkelman commented Mar 6, 2018

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.

@samoconnor
Copy link
Contributor Author

Good points Tony, let's continue over here: JuliaWeb/TLSClient.jl#1 (comment)

@quinnj quinnj merged commit e6b366c into JuliaLang:master Mar 6, 2018
@samoconnor
Copy link
Contributor Author

@quinnj are we ready to tag this yet?

samoconnor added a commit to JuliaWeb/HTTP.jl that referenced this pull request Mar 13, 2018
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

7 participants