Remove OpenSSL

fanquake commented at 2:14 pm on October 26, 2019: member

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 added the label Needs release note on Oct 26, 2019

fanquake added the label Needs Conceptual Review on Oct 26, 2019

fanquake added this to the milestone 0.20.0 on Oct 26, 2019

fanquake requested review from sipa on Oct 26, 2019

jnewbery commented at 3:00 pm on October 26, 2019: member

Concept ACK!

in src/random.cpp:15 in f56a128579 outdated

10@@ -11,7 +11,7 @@
11 #include <compat.h> // for Windows API
12 #include <wincrypt.h>
13 #endif
14-#include <logging.h>  // for LogPrint()
15+#include <logging.h>  // for LogPrintf()
16 #include <sync.h>     // for WAIT_LOCK

fanquake commented at 3:11 pm on October 26, 2019:

note: Will update 5cc4f20c7064fee90a78a827a9e30b2934ce4ca7 to change WAIT_LOCK to Mutex.

DrahtBot commented at 3:31 pm on October 26, 2019: member

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 commented at 7:46 pm on October 26, 2019: member

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 commented at 7:48 pm on October 26, 2019: member

Concept ACK.

BlockMechanic commented at 7:22 am on October 27, 2019: contributor

Concept ACK.

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

laanwj commented at 9:55 am on October 27, 2019: member

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

practicalswift commented at 10:02 pm on October 27, 2019: contributor

Concept ACK

Very pleased to see OpenSSL go :)

fanquake force-pushed on Oct 28, 2019

fanquake renamed this:
~~[WIP] Remove OpenSSL~~
Remove OpenSSL
on Oct 28, 2019

fanquake removed the label Needs Conceptual Review on Oct 28, 2019

fanquake commented at 1:10 pm on October 28, 2019: member

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

Sjors commented at 3:47 pm on October 29, 2019: member

Concept ACK after #17270. This fixes #12530.

jamesob commented at 4:03 pm on October 29, 2019: member

big Concept ACK

fanquake force-pushed on Nov 2, 2019

random: stop feeding RNG output back into OpenSSL

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.

5624ab0b4f

random: stop retrieving random bytes from OpenSSL

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.

4fcfcc294e

random: Remove remaining OpenSSL calls and locking infrastructure b49b6b0f70

build: remove OpenSSL detection and libs 8983ee3e6d

depends: remove OpenSSL package 648b2e3c32

doc: remove OpenSSL from build instructions and licensing info a4eb839619

ci: remove OpenSSL installation 397dbae070

doc: add OpenSSL removal to release-notes.md e5a0bece6e

fanquake force-pushed on Nov 18, 2019

fanquake marked this as ready for review on Nov 18, 2019

fanquake removed the label Needs release note on Nov 18, 2019

fanquake added the label Build system on Nov 18, 2019

fanquake added the label Utils/log/libs on Nov 18, 2019

MarcoFalke added the label Needs gitian build on Nov 18, 2019

MarcoFalke commented at 3:45 pm on November 18, 2019: member

ACK e5a0bece6e84402fcb1fe4f25fd24da1d21ec077

DrahtBot commented at 0:32 am on November 19, 2019: member

Gitian builds

File	commit 397c6d32c8f8a20a3605ef0d51d159adc21fd125(master)	commit e585117b8cbf55a5a24e69daa46213eb85e3ca97(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...`

DrahtBot removed the label Needs gitian build on Nov 19, 2019

laanwj commented at 8:25 am on November 19, 2019: member

ACK e5a0bece6e84402fcb1fe4f25fd24da1d21ec077

laanwj referenced this in commit 2065ef66ee on Nov 19, 2019

laanwj merged this on Nov 19, 2019

laanwj closed this on Nov 19, 2019

fanquake deleted the branch on Nov 19, 2019

sidhujag commented at 3:18 pm on November 19, 2019: none

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

sidhujag referenced this in commit 517bf47683 on Nov 19, 2019

fanquake referenced this in commit b4a1da9ef8 on Nov 19, 2019

sipa commented at 4:41 pm on November 19, 2019: member

@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 referenced this in commit 8acd6757e7 on Nov 19, 2019

laanwj commented at 10:11 am on November 20, 2019: member

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

fanquake referenced this in commit b07b2a9887 on Jan 10, 2020

fanquake referenced this in commit 6fe72fef8a on Jan 10, 2020

fanquake referenced this in commit 0ee3961ccd on Jan 11, 2020

cdecker referenced this in commit 46f879d73f on Jan 12, 2020

deadalnix referenced this in commit 8d6b4a37ad on May 25, 2020

deadalnix referenced this in commit 1092598e82 on May 25, 2020

deadalnix referenced this in commit ea9e88b8c6 on May 25, 2020

RdeWilde commented at 4:05 am on July 26, 2020: none

Great 👌🏼

ftrader referenced this in commit 7e5daea9ee on Aug 17, 2020

zkbot referenced this in commit 5bea6d806f on Sep 23, 2020

zkbot referenced this in commit 8777d2884e on Sep 29, 2020

zkbot referenced this in commit b5fa52b701 on Oct 1, 2020

sidhujag referenced this in commit 8d83aefe28 on Nov 10, 2020

sidhujag referenced this in commit c1d569169a on Nov 10, 2020

furszy referenced this in commit 086fc30d75 on May 12, 2021

DrahtBot locked this on Feb 15, 2022

Remove OpenSSL #17265

Conflicts

Gitian builds