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

Remove OpenSSL #17265

Merged
merged 8 commits into from Nov 19, 2019
Merged

Remove OpenSSL #17265

merged 8 commits into from Nov 19, 2019

Conversation

@fanquake
Copy link
Member

fanquake commented Oct 26, 2019

Now that #17165 has been merged, removing our remaining OpenSSL usage is possible.

That remaining usage was a call to RAND_bytes during the ::SLOW path of ProcRand. As well as feeding output from our RNG back into OpenSSL via RAND_add during the ::SLOW and ::SLEEP paths.

Optimistically tagged for 0.20.0. Needs discussion, potentially in an upcoming weekly meeting?

Closes #12530.

@fanquake fanquake added this to the 0.20.0 milestone Oct 26, 2019
@fanquake fanquake requested a review from sipa Oct 26, 2019
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 26, 2019

Concept ACK!

src/random.cpp Outdated Show resolved Hide resolved
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Oct 26, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16834 (Fetch Headers over DNS by TheBlueMatt)
  • #16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
  • #15382 (util: add runCommandParseJSON by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Oct 26, 2019

I think we wanted to include some of the environmental entropy sources (statistics, pid, ...) that OpenSSL uses in our own RNG state first. See #10299. I'll PR something soon.

@promag

This comment has been minimized.

Copy link
Member

promag commented Oct 26, 2019

Concept ACK.

@BlockMechanic

This comment has been minimized.

Copy link
Contributor

BlockMechanic commented Oct 27, 2019

Concept ACK.

I recently ran into openssl issues here #17123, this is awesome !

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Oct 27, 2019

Concept and code review ACK, agree that we should ideally get #17270 in first.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Oct 27, 2019

Concept ACK

Very pleased to see OpenSSL go :)

@fanquake fanquake force-pushed the fanquake:remove_openssl branch from f56a128 to 1ee67af Oct 28, 2019
@fanquake fanquake changed the title [WIP] Remove OpenSSL Remove OpenSSL Oct 28, 2019
@fanquake

This comment has been minimized.

Copy link
Member Author

fanquake commented Oct 28, 2019

Fixed doc nit above and squashed some commits together. This is waiting on #17270.

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Oct 29, 2019

Concept ACK after #17270. This fixes #12530.

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Oct 29, 2019

big Concept ACK

@fanquake fanquake force-pushed the fanquake:remove_openssl branch from 1ee67af to 24ac38d Nov 2, 2019
fanquake added 8 commits Oct 26, 2019
On the ::SLOW or ::SLEEP paths, we would feed our RNG output back into
OpenSSL using RAND_add. This commit removes that functionality.

RAND_add(): https://www.openssl.org/docs/manmaster/man3/RAND_add.html

RAND_add() mixes the num bytes at buf into the internal state of the
random generator. This function will not normally be needed, as
mentioned above. The randomness argument is an estimate of how much
randomness is contained in buf, in bytes, and should be a number
between zero and num.
On the ::SLOW path we would use OpenSSL as an additional source of
random bytes. This commit removes that functionality. Note that this was
always only an additional source, and that we never checked the return
value

RAND_bytes(): https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html

RAND_bytes() puts num cryptographically strong pseudo-random bytes into buf.
@fanquake fanquake force-pushed the fanquake:remove_openssl branch from 24ac38d to e5a0bec Nov 18, 2019
@fanquake fanquake marked this pull request as ready for review Nov 18, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 18, 2019

ACK e5a0bec

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Nov 19, 2019

Gitian builds

File commit 397c6d3
(master)
commit e585117
(master and this pull)
bitcoin-0.19.99-osx-unsigned.dmg bd2b4ba1fdf14cc3... 82e6d97a073a58b0...
bitcoin-0.19.99-osx64.tar.gz 808ee6836d30ef71... 548abcca6668abd8...
bitcoin-0.19.99-win64-debug.zip f2198cd29b50b270... ec8fa1926b836175...
bitcoin-0.19.99-win64-setup-unsigned.exe 4d888664e10e48b3... 4f5824c20e1e76ff...
bitcoin-0.19.99-win64.zip a5b95db2cb16eead... cfe5011add847980...
bitcoin-0.19.99.tar.gz a53e2d332d78c83f... 3161156d91852540...
bitcoin-core-osx-0.20-res.yml 59bb5b378df58a9a... 187795b725ae0939...
bitcoin-core-win-0.20-res.yml 36d0611ac8b849e8... 7d527c4abd073ab3...
linux-build.log 1899b85b14c6efa8... f9d24a4a5d96e59d...
osx-build.log 6ebc79b0eba7f721... f661d147073efb70...
win-build.log 6e158ddc9a88ac61... 15bf1d25b468ba3a...
bitcoin-core-osx-0.20-res.yml.diff ab1d17c25c6dbb47...
bitcoin-core-win-0.20-res.yml.diff 9d8445f792e214f8...
linux-build.log.diff f316a7c935756c1e...
osx-build.log.diff 646c8ab4a694daf2...
win-build.log.diff 20f1212c6b9d9bfb...
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 19, 2019

ACK e5a0bec

laanwj added a commit that referenced this pull request Nov 19, 2019
e5a0bec doc: add OpenSSL removal to release-notes.md (fanquake)
397dbae ci: remove OpenSSL installation (fanquake)
a4eb839 doc: remove OpenSSL from build instructions and licensing info (fanquake)
648b2e3 depends: remove OpenSSL package (fanquake)
8983ee3 build: remove OpenSSL detection and libs (fanquake)
b49b6b0 random: Remove remaining OpenSSL calls and locking infrastructure (fanquake)
4fcfcc2 random: stop retrieving random bytes from OpenSSL (fanquake)
5624ab0 random: stop feeding RNG output back into OpenSSL (fanquake)

Pull request description:

  Now that #17165 has been merged, removing our remaining OpenSSL usage is possible.

  That remaining usage was a call to [`RAND_bytes`](https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html) during the ::SLOW path of [ProcRand](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L616). As well as feeding output from our RNG back into OpenSSL via [`RAND_add`](https://www.openssl.org/docs/manmaster/man3/RAND_add.html) during the ::SLOW and ::SLEEP paths.

  Optimistically tagged for `0.20.0`. Needs discussion, potentially in an upcoming weekly meeting?

  Closes #12530.

ACKs for top commit:
  MarcoFalke:
    ACK e5a0bec
  laanwj:
    ACK e5a0bec

Tree-SHA512: 02fce08ec91d20e0da51e9314eec53dcf8699cded02f0a005417d627520c20b826332cb42bdae132af283d4903aa3088a9f613f3aea915d655a51532a4d4796c
@laanwj laanwj merged commit e5a0bec into bitcoin:master Nov 19, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake deleted the fanquake:remove_openssl branch Nov 19, 2019
@sidhujag

This comment has been minimized.

Copy link

sidhujag commented Nov 19, 2019

concept ACK, is libsecp256k1 going to remove OpenSSL as well? It uses $CRYPTO_CFLAGS

sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 19, 2019
e5a0bec doc: add OpenSSL removal to release-notes.md (fanquake)
397dbae ci: remove OpenSSL installation (fanquake)
a4eb839 doc: remove OpenSSL from build instructions and licensing info (fanquake)
648b2e3 depends: remove OpenSSL package (fanquake)
8983ee3 build: remove OpenSSL detection and libs (fanquake)
b49b6b0 random: Remove remaining OpenSSL calls and locking infrastructure (fanquake)
4fcfcc2 random: stop retrieving random bytes from OpenSSL (fanquake)
5624ab0 random: stop feeding RNG output back into OpenSSL (fanquake)

Pull request description:

  Now that bitcoin#17165 has been merged, removing our remaining OpenSSL usage is possible.

  That remaining usage was a call to [`RAND_bytes`](https://www.openssl.org/docs/manmaster/man3/RAND_bytes.html) during the ::SLOW path of [ProcRand](https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp#L616). As well as feeding output from our RNG back into OpenSSL via [`RAND_add`](https://www.openssl.org/docs/manmaster/man3/RAND_add.html) during the ::SLOW and ::SLEEP paths.

  Optimistically tagged for `0.20.0`. Needs discussion, potentially in an upcoming weekly meeting?

  Closes bitcoin#12530.

ACKs for top commit:
  MarcoFalke:
    ACK e5a0bec
  laanwj:
    ACK e5a0bec

Tree-SHA512: 02fce08ec91d20e0da51e9314eec53dcf8699cded02f0a005417d627520c20b826332cb42bdae132af283d4903aa3088a9f613f3aea915d655a51532a4d4796c
fanquake added a commit that referenced this pull request Nov 19, 2019
ea3c7e5 test: Remove libssl-dev packages from CI scripts (Wladimir J. van der Laan)
7ea5526 test: remove lsan suppression for libcrypto (Wladimir J. van der Laan)
2d70665 build: remove libcrypto as internal dependency in libbitcoinconsensus.pc (Wladimir J. van der Laan)
278751e doc: Remove ssl as a required dependency from build-unix (Wladimir J. van der Laan)

Pull request description:

  Some doc and build cleanups following #17265.

  I intentionally left the libssl-dev install in `gitian-win-signer.yml`, as it's necessary for the ossl signer.

ACKs for top commit:
  MarcoFalke:
    ACK ea3c7e5 🗯
  jamesob:
    ACK ea3c7e5
  practicalswift:
    ACK ea3c7e5 - nice!
  fanquake:
    ACK ea3c7e5 - thanks.

Tree-SHA512: 67ea35bdd6d6e512d69e6734713534c88cae033a2ed695677ea15c3e3d5ff570374e342775c88e60877fa43a19047853e7b2a433e2c9a4349a5c423726a7457e
@sipa

This comment has been minimized.

Copy link
Member

sipa commented Nov 19, 2019

@sidhujag libsecp256k1 only uses OpenSSL as an optional dependency in tests. This may change in the future, but the same reasons really don't apply.

sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 19, 2019
…d build

ea3c7e5 test: Remove libssl-dev packages from CI scripts (Wladimir J. van der Laan)
7ea5526 test: remove lsan suppression for libcrypto (Wladimir J. van der Laan)
2d70665 build: remove libcrypto as internal dependency in libbitcoinconsensus.pc (Wladimir J. van der Laan)
278751e doc: Remove ssl as a required dependency from build-unix (Wladimir J. van der Laan)

Pull request description:

  Some doc and build cleanups following bitcoin#17265.

  I intentionally left the libssl-dev install in `gitian-win-signer.yml`, as it's necessary for the ossl signer.

ACKs for top commit:
  MarcoFalke:
    ACK ea3c7e5 🗯
  jamesob:
    ACK bitcoin@ea3c7e5
  practicalswift:
    ACK ea3c7e5 - nice!
  fanquake:
    ACK ea3c7e5 - thanks.

Tree-SHA512: 67ea35bdd6d6e512d69e6734713534c88cae033a2ed695677ea15c3e3d5ff570374e342775c88e60877fa43a19047853e7b2a433e2c9a4349a5c423726a7457e
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Nov 20, 2019

it doesn't affect any of the bitcoin core binaries, so it's off topic here. Please take your question upstream.
(That said, cross-checking against other libs is generally a good idea for cryptographic libraries.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.