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

fixed incompatibility with newer libssl #221

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

BrannonKing
Copy link
Member

I was going to wait for the upstream merge, but I don't want to have to explain this problem in my introductory video.

@BrannonKing
Copy link
Member Author

@lbrynaut , @kaykurokawa to finish our conversation from yesterday, the upstream bitcoin was changed to use a completely different library in 0.13: bitcoin/bitcoin@9049cde

@BrannonKing
Copy link
Member Author

The change to openssl from 1.0.2 to 1.1 is quite long:
https://www.openssl.org/news/changelog.html#x12
There are latest versions of 1.0.2 and 1.1.x -- it looks like many of the same security fixes go into both:
https://www.openssl.org/news/vulnerabilities.html
I can't make any argument that 1.1.latest is more/less vulnerable than 1.0.latest.

@BrannonKing
Copy link
Member Author

Our current hard-coded dependency is openssl-1.0.1p. This is far behind the current version, and it was patched by all the major OSs ages ago. It's a strong argument against using the well-known version.
https://www.openssl.org/news/vulnerabilities.html

@lbrynaut
Copy link
Contributor

It sounds clearly like the way to go is to also update to some other library, and not to continue using OpenSSL, which has many known issues and has never been thoroughly reviewed. I've actually never been involved with a project that actively wanted to work with OpenSSL, rather than making best efforts to move away from it. Enabling users to continue on using it adds more harm than good, IMO.

@lbrynaut
Copy link
Contributor

I was going to wait for the upstream merge, but I don't want to have to explain this problem in my introductory video.

If reproducible_build.sh isn't working, that's a much higher priority issue to resolve than this kind of hack for openssl. It should be the "go to" for installation, since getting the dependencies right are difficult, and may affect how the code operates with system version differences. I'm not saying our script is the best script for this, but there's a very good reason bitcoin related code strives for reproducible builds with fixed dependencies.

@BrannonKing
Copy link
Member Author

BrannonKing commented Oct 18, 2018

[reproducable_build.sh] should be the "go to" for installation,

I assume that you mean compilation, not installation. (Our curated release builds are not in question here; we must have static builds for official releases.) And I fully disagree with this stated ideal. The "go to" should be the common build tools that developers across the planet use. In C++ land that means autotools or CMake. We don't need an additional learning curve for compiling our software.

since getting the dependencies right are difficult...

It may be difficult on Windows or OSX; I've never tried it on one of those. But on Linux, this is a common and well-handled scenario. The bitcoin build instructions are very clear on how to get and install dependencies using common paradigms. There is no "deps, build, and test" script in the bitcoin codebase that I can see. Gitian seems to be the modern equivalent of their reproducible deployment, but that's not what developers are iterating with. Current instructions: https://github.com/bitcoin/bitcoin/blob/master/doc/build-unix.md

and may affect how the code operates with system version differences.

We have three primary dependencies:

  1. BerkleyDB -- used only by the built-in wallet, something our customers are generally not using.
  2. OpenSSL -- release with our old version of this is a significant mistake, and if it's good enough for the user's system it's probably good enough for us, since all modern OSs keep this up to date. We rely on consensus for security anyway, not what's on a minority of users' machines. This goes away with the upstream merge. (This whole PR is a temporary solution.)
  3. Boost -- it's used heavily in the 0.12 code, but these are core boost pieces, not the more esoteric parts of Boost. Very little on these pieces has changed in recent years. In addition, it's a stated goal of bitcoin to eliminate Boost dependencies. Upstream has already made significant progress on this with more PRs in the wings. Whatever is available on any modern machine will be fine.

bitcoin-related code strives for reproducible builds with fixed dependencies.

I could find no proof of this in the bitcoin docs. However, I do think this is a common need. And the "go to" solution for this is Docker, which completely eliminates the dependency build time. I've been debating on where to host the Dockerfile for compiling lbrycrd; this repo or the lbry-docker repo? The Dockerfile is trivial. I have one that I use already.

It sounds clearly like the way to go is to also update to some other library...

Yes, that's in progress, no? But I need something this week, something for the interim.

@bvbfan
Copy link
Collaborator

bvbfan commented Oct 18, 2018

Our current hard-coded dependency is openssl-1.0.1p. This is far behind the current version, and it was patched by all the major OSs ages ago.

I'm on 1.0.2p so we can increase it. Versions 1.1 and 1.0 goes in parallel, i don't see drama here. OpenSSL is far away from insecure, so moving on other one is not required.

@BrannonKing, changes are sane you can go for them.
@lbrynaut, when you merge new bitcoin, on demand, depend on OpenSSL can be removed.

@lbrynaut
Copy link
Contributor

[reproducable_build.sh] should be the "go to" for installation,

I assume that you mean compilation, not installation. (Our curated release builds are not in question here; we must have static builds for official releases.) And I fully disagree with this stated ideal. The "go to" should be the common build tools that developers across the planet use. In C++ land that means autotools or CMake. We don't need an additional learning curve for compiling our software.

It sounds like you agree that the provided releases builds are the way to go, but you don't think there's a difference between running supported software linked with correct dependencies and compiling (unsupported) software linked with incorrect dependencies. Bugs can happen either way, but there's a chance also that they may be mismatched, no?

It sounds clearly like the way to go is to also update to some other library...

Yes, that's in progress, no? But I need something this week, something for the interim.

If you need a quick fix this week, it sounds like you know what you want to support.

I think that reproducible_build.sh should work (not just on travis), and once installed, it's trivial to modify and build the code subsequently with supported dependencies. It'll never improve if we don't know there are issues. Maybe that's something others can help with too. It is also specifically designed to not have the kind of issue that this kind of 'solution' attempts to address.

@bvbfan This isn't drama -- I just don't agree that it's necessary or a good idea. There's no reason we all have to agree on something like this. If the goal is to get more people building unsupported builds, we just have differing goals.

@BrannonKing BrannonKing merged commit 24da3ac into master Oct 19, 2018
@BrannonKing BrannonKing deleted the support_new_openssl_try2 branch October 25, 2018 17:43
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.

4 participants