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

Support OpenSSL 3.0 #1596

Merged
merged 5 commits into from Jan 5, 2023
Merged

Support OpenSSL 3.0 #1596

merged 5 commits into from Jan 5, 2023

Conversation

kentonv
Copy link
Member

@kentonv kentonv commented Dec 28, 2022

This is based on #1434, but I went ahead and made KJ treat unexpected EOF as an error -- even with OpenSSL 1.x -- since this is technically important to prevent truncation attacks.

(It turns out that lots of apps than use OpenSSL 1.x are vulnerable to truncation attacks, which is why OpenSSL 3 decided to strengthen the error reporting to make it clearer that this situation should be an error, but this was considered a breaking change. So KJ is not alone in having this bug, but I went ahead and fixed it even when using OpenSSL 1.x.)

NOTE: I'm flying blind here as my local Debian machine is still on OpenSSL 1.x. Gonna do a little CI-driven-development. I'll mark this no longer a draft once I get CI passing...

kentonv and others added 2 commits December 27, 2022 18:50
Co-authored-by: Tom Lee <github@tomlee.co>
Co-authored-by: Tom Lee <github@tomlee.co>
@kentonv kentonv mentioned this pull request Dec 28, 2022
@kentonv kentonv force-pushed the openssl3 branch 3 times, most recently from f5a4469 to 4fdc121 Compare December 28, 2022 02:04
@kentonv
Copy link
Member Author

kentonv commented Dec 28, 2022

Well the g++-12 build proves OpenSSL 3 now works, but I have no idea why the clang-14 build isn't linking libc++ correctly.

I probably need to play with this on an Ubuntu VM / container rather than iterate in CI. Not going to do that tonight.

@h-vetinari
Copy link

Thanks for tackling this! :)

Picked this PR on top of 0.10.3 in conda-forge/capnproto-feedstock#32, but still failing on linux with:

[ TEST ] kj/compat/tls-test.c++:728: TLS client certificate verification
kj/compat/tls.c++:74: failed: OpenSSL error; message = error:16000069:STORE routines::unregistered scheme
error:80000002:system library::No such file or directory
stack: 7fe63300a740 7fe6330108cb 7fe633010b48 7fe632f5a699 7fe632f5a5a4 55f52f37b411 55f52f37b552 55f52f38806e 7fe632ef038e
[ FAIL ] kj/compat/tls-test.c++:728: TLS client certificate verification (152963 μs)

Similarly on osx:

[ TEST ] kj/compat/tls-test.c++:728: TLS client certificate verification
kj/compat/tls.c++:74: failed: OpenSSL error; message = error:16000069:STORE routines::unregistered scheme
error:80000002:system library::No such file or directory
stack: 1081baed9 1081c334e 1081c2ef5 1081c2530 10821f95f 10821f867 107715959 107714001 1077228ee 10831578a
[ FAIL ] kj/compat/tls-test.c++:728: TLS client certificate verification (184390 μs)

Both builds against openssl 1.1.1 pass the tests.

It's completely possible that some of the other intervening changes on master are also necessary (it wouldn't be a problem to wait for a new version), but I just wanted to provide some additional testing. :)

@kentonv
Copy link
Member Author

kentonv commented Dec 28, 2022

@h-vetinari I'm probably not going to be able to debug a problem that doesn't repro in our CI. That said, if you could do a debug build so that the stack trace is properly symbolized, that might help narrow down the issue. Also, "No such file or directory" is suspicious; if you could use strace to find out what file it's trying to open, that might help. It may be that OpenSSL requires some config file that's missing on your setup, although that's kind of strange since the test that is failing really shouldn't require any external files.

kentonv and others added 3 commits January 3, 2023 08:52
An EOF received without a proper shutdown handshake should be treated as an error, otherwise truncation attacks are possible.

The way this is reported changed between OpenSSL 1.x vs. 3.x. Apparently, OpenSSL 1.x programs widely failed to consider this an error -- KJ being an example, up until now -- hence the OpenSSL developers chose to strengthen the error reporting to make people take it more seriously. Here I've decided to make it an error even when using OpenSSL 1.x, as we always should have done anyway.

Co-authored-by: Tom Lee <github@tomlee.co>
(This isn't related to OpenSSL 3.0 but is in this PR because it's needed to update CI to newer Ubuntu...)
We couldn't do this before because it uses OpenSSL 3, but that should work now.

I went ahead and made it use the latest compiler versions supported in this release.

We still use Ubuntu 20.04 to test older compilers, since 22.04 doesn't support compilers that old anymore.
@kentonv kentonv marked this pull request as ready for review January 3, 2023 15:01
@kentonv
Copy link
Member Author

kentonv commented Jan 3, 2023

OK, got CI working. AFAICT this makes KJ work correctly with OpenSSL 3.

cc @thomaslee

# and started issuing deprecating warnings for -fcoroutines-ts. (I'm not sure which version it
# was exactly since our CI jumped from 10 to 14, so I'm somewhat arbitrarily choosing 14 as the
# cutoff.)
export CXXFLAGS="$CXXFLAGS -std=c++20 -stdlib=libc++"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we lose any code coverage by using -std=c++20 instead of gnu++20? Maybe we don't actually use any GNU extensions that clang doesn't provide by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would expect that if we were using any "GNU extensions" that aren't enabled for -std=c++20, we'd see errors, because I'm not aware of us using #ifdefs that somehow distinguish whether GNU extensions are enabled. So since we see no errors, I suspect it doesn't really make a difference?

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

3 participants