Fix build with OpenSSL 3.0 #25

oleg-jukovec · 2022-04-14T16:01:10Z

FIPS_mode_set() does not exist in OpenSSL 3.0 [1]
X509_check_* functions declarated in openssl/x509v3.h instead of openssl/x509.h [2]
X509_chack_* functions have const char arg inserad of const unsigned char [2]
the patch does not change behavior under OpenSSL version != 3
the patch just fixes build under OpenSSL 3.0 and doesn't update deprecated code
or behavior

BigLep · 2022-05-27T16:57:09Z

2022-05-27: general context: this repo is use speedup RSA operations in go-libp2p-core, hidden behind a build flag.

@oleg-jukovec : a few general comments:

We're supporting v1.x until that is no longer an option
What usecase is this blocking for you?
In order for this to be considered more, we'd need to have tests pass.

oleg-jukovec · 2022-05-27T19:47:36Z

2022-05-27: general context: this repo is use speedup RSA operations in go-libp2p-core, hidden behind a build flag.

@oleg-jukovec : a few general comments:
1. We're supporting v1.x until that is no longer an option

2. What usecase is this blocking for you?

3. In order for this to be considered more, we'd need to have tests pass.

The pull request does not affect OpenSSL v1.x
Ubuntu 22.04 LTS has OpenSSL 3.0. Ubuntu is a very popular Linux distribution.
I think that tests on Windows are not broken by this pull request. It looks more like a test environment problem (same problems in sync: update CI config files #24):

# pkg-config --cflags  --libssl libcrypto
Can't find C:\Strawberry\perl\bin\pkg-config.bat on PATH, '.' not in PATH.
pkg-config: exit status 29

BigLep · 2022-06-03T16:53:44Z

2022-06-03 maintainer conversation: given Ubuntu 22.04 LTS has OpenSSL 3.0 is shipping without OpenSSL 1.x, we agree this should handled.

We need a GitHub actions workflow that exercises the OpenSSL 3.x path. @oleg-jukovec : can you please add a seperate GitHub actions workflow file for testing against this? Per https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources it looks like there is a Ubuntu 22.04 runner we can use.

galargh · 2022-06-06T13:55:45Z

3. I think that tests on Windows are not broken by this pull request. It looks more like a test environment problem (same problems in sync: update CI config files #24):

That was true - we needed to install mingw toolchain which is no longer installed by default on windows-2022 gh actions runners - #30. Might be worth mentioning that, in particular,mingw-w64-x86_64-openssl-1.1.1.o-3 and mingw-w64-i686-openssl-1.1.1.o-3 packages are installed as part of the toolchain.

BigLep · 2022-06-17T17:20:45Z

@oleg-jukovec : can you handle this (basically making the code change with custom github runner)?

From a core maintainer regard, this isn't a high priority. We'd appreciate any help to move this forward. Thanks!

oleg-jukovec · 2022-06-17T18:54:43Z

@oleg-jukovec : can you handle this (basically making the code change with custom github runner)?

From a core maintainer regard, this isn't a high priority. We'd appreciate any help to move this forward. Thanks!

Hello, yes, I'm going to do it. I have no time right now, but maybe next week.

- FIPS_mode_set() does not exist in OpenSSL 3.0 [1] - X509_check_* functions declarated in openssl/x509v3.h instead of openssl/x509.h [2] - X509_chack_* functions have const char arg inserad of const unsigned char [2] - skip MD4 tests if it is unsupported by OpenSSL - the patch does not change behavior under OpenSSL version != 3 - the patch just fixes build under OpenSSL 3.0 and doesn't update deprecated code or behavior 1. https://wiki.openssl.org/index.php/OpenSSL_3.0#Upgrading_from_the_OpenSSL_2.0_FIPS_Object_Module 2. https://www.openssl.org/docs/man3.0/man3/X509_check_host.html

oleg-jukovec · 2022-06-29T13:21:25Z

Thank you for your patience. I've added the Ubuntu 22.04 runner to the CI.

galargh · 2022-06-29T14:32:06Z

.github/workflows/go-test.yml

@@ -9,11 +9,11 @@ jobs:
    strategy:
      fail-fast: false
      matrix:
-        os: [ "ubuntu", "windows", "macos" ]
+        os: [ "ubuntu-22.04", "ubuntu-latest", "windows-latest", "macos-latest" ]


Any changes to this file will get overwritten the next time https://github.com/protocol/.github is pushed to.

Off the top of my head, the easiest way around it would be to add a new workflow in this repo only which runs go test on ubuntu-22.04.

I also created an issue for considering making the os list configurable in Unified CI - protocol/.github#349

Oh, thank you, I understand the problem. Then I think it's better to wait protocol/.github#349 .

I think the best way forward would be to just add a new workflow file go-test-ubuntu-22.04.yml, which would essentially be a copy of go-test.yml.

I think it's done.

It is necessary to handle OpenSSL errors very carefully. Otherwise, errors may appear in unexpected places. For example, we didn't catch an error from EVP_DigestInit_ex() and it appears sometimes in conn.go: func (c *Conn) getErrorHandler(rv C.int, errno error) func() error { errcode := C.SSL_get_error(c.ssl, rv) // <- here

galargh

Approving the CI changes. Let's go with a copy of the test workflow until we have a better story for cases like this one.

aschmahmann · 2022-07-29T15:17:50Z

@marten-seemann is this something we're going to merge? I've heard from @Jorropo that boringcrypto coming in Go 1.19 might be better off for us and not require CGO.

marten-seemann · 2022-07-29T15:21:23Z

Not familiar with boringcrypto in Go, do you have a link for me?

Not depending on OpenSSL seems to be a good idea, especially considering the upcoming fork and the expected split of the community.

Jorropo · 2022-07-29T15:35:56Z

Not familiar with boringcrypto in Go, do you have a link for me?

When you build go you can do:

GOEXPERIMENT=boringcrypto ./make.bash

It will use a boringssl (google's openssl fork) wrapper to replace most of go's crypto librairy. (and AFAIT it has runtime tricks to do cgo without the 500threads and the big overhead)
It's really fast (depends on the workload but you can see 2x improvements over the go implementation easily), it's seems like the same thing we are doing right now but maintained by google and more integrated into go than the thing we have now.

You can read about it here https://github.com/golang/go/blob/cdcb4b6ef37c1ce14637323dd00b5daad7e645c4/README.boringcrypto.md and in the go compiler VCS.

My main question is maybe that interfer with the unsafe hack you do for QUIC and your forked crypto lib. If it doesn't I think we should drop go-openssl package and point people to google's maintained openssl wrapper.

marten-seemann · 2022-08-03T08:30:31Z

My main question is maybe that interfer with the unsafe hack you do for QUIC and your forked crypto lib.

It won't. I removed boring support from the fork: quic-go/qtls-go1-19@2a06850

If it doesn't I think we should drop go-openssl package and point people to google's maintained openssl wrapper.

That would be a great outcome! One less libp2p package :)
We should run a benchmark to convince ourselves that this provides comparable performance. @Jorropo, do you want to work on this?

Jorropo · 2022-08-03T08:46:21Z

@marten-seemann

It won't. I removed boring support from the fork: quic-go/qtls-go1-19@2a06850

Just to be sure, quic-go doesn't support go-openssl either ?

@Jorropo, do you want to work on this?

Ok I'll, any cipher you want see tried ?
I'll try tls1.3 negociation and sending a few bytes, ed25519 key generation, signature and verification, AES CBC, AES GCM and noise (but afaik noise is implemented a third party thing).

marten-seemann · 2022-08-03T08:52:52Z

Just to be sure, quic-go doesn't support go-openssl either ?

It doesn't, and never has.

Ok I'll, any cipher you want see tried ?

Good question. I thought the starting point would be the benchmarks this package comes with. Maybe also add RSA operations to your list, since annoyingly, RSA is still widely used in the ecosystem.
If you want to do an end-to-end TLS test, you might need to jump through some hoops to convince crypto/tls to use the ciphersuite you want, since they're trying to be smart about ciphersuite selection (or just use TLS 1.2 and set it in tls.Config.CipherSuites).

BigLep · 2022-08-05T16:08:09Z

@Jorropo : will you be able to do the benchmarking? Let me know if that is feeling like too much and we can potentially see if @oleg-jukovec can take this.

Jorropo · 2022-08-05T17:35:46Z

@BigLep it's the work of 2 hours, I'll do once the bitswap split is over.

This is not a priority issue.

Stebalien

Code LGTM and CI passes, there's no reason not to just merge this.

MarcoPolo

Boringcrypto isn’t going to be a replacement for this repo. Until it is, I think we should merge this to support OpenSSL 3.0.

More discussion here: libp2p/go-libp2p#1686 (comment)

MarcoPolo · 2022-08-18T02:58:08Z

@marten-seemann let me know if there’s anything else I’m missing, but otherwise we should just merge this.

oleg-jukovec force-pushed the openssl-3-build-fix branch from 5b2dc27 to a4115ef Compare April 19, 2022 14:36

Stebalien mentioned this pull request May 5, 2022

fix: remove fips support #28

Closed

aschmahmann mentioned this pull request May 5, 2022

openssl: support openssl v3 ipfs/kubo#8882

Closed

3 tasks

BigLep added the need/author-input Needs input from the original author label May 27, 2022

BigLep added the P3 Low: Not priority right now label Jun 17, 2022

MarcoPolo self-requested a review June 17, 2022 17:17

oleg-jukovec force-pushed the openssl-3-build-fix branch 2 times, most recently from a67734d to a410882 Compare June 29, 2022 12:46

oleg-jukovec force-pushed the openssl-3-build-fix branch from a410882 to 982e9fb Compare June 29, 2022 13:06

galargh reviewed Jun 29, 2022

View reviewed changes

oleg-jukovec force-pushed the openssl-3-build-fix branch 4 times, most recently from 33caa98 to e5e193b Compare June 29, 2022 16:59

oleg-jukovec added 2 commits June 29, 2022 20:01

Add Ubuntu 22.04 runner to GitHub Actions go test workflow

e5e193b

oleg-jukovec force-pushed the openssl-3-build-fix branch from 805504c to b87b660 Compare June 30, 2022 21:04

marten-seemann self-requested a review July 1, 2022 16:07

Jorropo mentioned this pull request Jul 6, 2022

CVE-2022-2274 ipfs/kubo#9097

Closed

3 tasks

galargh approved these changes Jul 11, 2022

View reviewed changes

Stebalien approved these changes Aug 18, 2022

View reviewed changes

MarcoPolo approved these changes Aug 18, 2022

View reviewed changes

marten-seemann approved these changes Aug 18, 2022

View reviewed changes

marten-seemann merged commit 46d44e1 into libp2p:master Aug 18, 2022

Jorropo mentioned this pull request Aug 18, 2022

chore: release v0.0.7 #33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build with OpenSSL 3.0 #25

Fix build with OpenSSL 3.0 #25

oleg-jukovec commented Apr 14, 2022

BigLep commented May 27, 2022

oleg-jukovec commented May 27, 2022 •

edited

Loading

BigLep commented Jun 3, 2022

galargh commented Jun 6, 2022

BigLep commented Jun 17, 2022

oleg-jukovec commented Jun 17, 2022 •

edited

Loading

oleg-jukovec commented Jun 29, 2022

galargh Jun 29, 2022

galargh Jun 29, 2022

oleg-jukovec Jun 29, 2022

marten-seemann Jun 29, 2022

oleg-jukovec Jun 30, 2022

galargh left a comment

aschmahmann commented Jul 29, 2022

marten-seemann commented Jul 29, 2022

Jorropo commented Jul 29, 2022 •

edited

Loading

marten-seemann commented Aug 3, 2022

Jorropo commented Aug 3, 2022 •

edited

Loading

marten-seemann commented Aug 3, 2022 •

edited

Loading

BigLep commented Aug 5, 2022

Jorropo commented Aug 5, 2022 •

edited

Loading

Stebalien left a comment

MarcoPolo left a comment

MarcoPolo commented Aug 18, 2022

Fix build with OpenSSL 3.0 #25

Fix build with OpenSSL 3.0 #25

Conversation

oleg-jukovec commented Apr 14, 2022

BigLep commented May 27, 2022

oleg-jukovec commented May 27, 2022 • edited Loading

BigLep commented Jun 3, 2022

galargh commented Jun 6, 2022

BigLep commented Jun 17, 2022

oleg-jukovec commented Jun 17, 2022 • edited Loading

oleg-jukovec commented Jun 29, 2022

galargh Jun 29, 2022

Choose a reason for hiding this comment

galargh Jun 29, 2022

Choose a reason for hiding this comment

oleg-jukovec Jun 29, 2022

Choose a reason for hiding this comment

marten-seemann Jun 29, 2022

Choose a reason for hiding this comment

oleg-jukovec Jun 30, 2022

Choose a reason for hiding this comment

galargh left a comment

Choose a reason for hiding this comment

aschmahmann commented Jul 29, 2022

marten-seemann commented Jul 29, 2022

Jorropo commented Jul 29, 2022 • edited Loading

marten-seemann commented Aug 3, 2022

Jorropo commented Aug 3, 2022 • edited Loading

marten-seemann commented Aug 3, 2022 • edited Loading

BigLep commented Aug 5, 2022

Jorropo commented Aug 5, 2022 • edited Loading

Stebalien left a comment

Choose a reason for hiding this comment

MarcoPolo left a comment

Choose a reason for hiding this comment

MarcoPolo commented Aug 18, 2022

oleg-jukovec commented May 27, 2022 •

edited

Loading

oleg-jukovec commented Jun 17, 2022 •

edited

Loading

Jorropo commented Jul 29, 2022 •

edited

Loading

Jorropo commented Aug 3, 2022 •

edited

Loading

marten-seemann commented Aug 3, 2022 •

edited

Loading

Jorropo commented Aug 5, 2022 •

edited

Loading