QUIC for 3.3.0 #159

wbl · 2024-05-08T19:16:34Z

Review carefully: we've tested the code decently well (thanks @nibanks). The branch wladd/quic-comparison-branch has everything squashed down to ease review. I renamed our feature to boring-quic-api in this branch as the first big change, hopefully everywhere it needed to happen in the build system.

wbl · 2024-05-08T19:27:40Z

Looks like some more work needed for CI to work, will update in a bit.

This adds a compatible API for BoringSSL's QUIC support, based on the current |draft-ietf-quic-tls|. Based on BoringSSL commit 3c034b2cf386b3131f75520705491871a2e0cafe Based on BoringSSL commit c8e0f90f83b9ec38ea833deb86b5a41360b62b6a Based on BoringSSL commit 3cbb0299a28a8bd0136257251a78b91a96c5eec8 Based on BoringSSL commit cc9d935256539af2d3b7f831abf57c0d685ffd81 Based on BoringSSL commit e6eef1ca16a022e476bbaedffef044597cfc8f4b Based on BoringSSL commit 6f733791148cf8a076bf0e95498235aadbe5926d Based on BoringSSL commit 384d0eaf1930af1ebc47eda751f0c78dfcba1c03 Based on BoringSSL commit a0373182eb5cc7b81d49f434596b473c7801c942 Based on BoringSSL commit b1b76aee3cb43ce11889403c5334283d951ebd37

wbl · 2024-05-13T13:10:23Z

Looks like all checks pass modulo the fuzzer, but that seems to have run fine other times?

wbl · 2024-05-20T14:42:55Z

I do not understand what the difference is between the two fuzzer builds. @tmshort any ideas? Going to fiddle with the docs some more, and then a review would be very appreciated.

tmshort · 2024-05-22T14:51:59Z

i wouldn't worry about it. The one tagged (pull_request) is likely only run on an actual pull request, and is more comprehensive than the one tagged (push) which would happen on all pushes to the repo (even if they don't become a PR).

tmshort · 2024-05-22T14:55:13Z

The CIFuzz (pull_request) has almost always failed.

tmshort · 2024-06-03T14:05:53Z

Any updates here @wbl ?

wbl · 2024-06-03T14:09:13Z

Sorry I think balls ended up on floors as Ive forgotten to poke. I've made the comparison branch and gotten this one to what I think is reviewable state but another set of eyes would be helpful. So take a look or reremind me what I need to do first.

richsalz

some nits, some politics. didn't look closely at the bulk of the QIUC changes since they look like they always do.

README.md

richsalz · 2024-06-13T23:12:20Z

README.md

-The OpenSSL project does not distribute the toolkit in binary form.
+The APIs here are used by Microsoft's
+[MsQuic](https://github.com/microsoft/msquic) and Google's
+[Chromium QUIC](https://chromium.googlesource.com/chromium/src/+/master/net/quic/)


add ", among other."

richsalz · 2024-06-13T23:13:07Z

README.md

---------------------------
+This fork can be considered a supported version of
+[OpenSSL PR 8797](https://github.com/openssl/openssl/pull/8797).
+We will endeavor to track OpenSSL releases within a day or so, and there is an


s/within a day or so/within days/ ?

README.md

apps/info.c

richsalz · 2024-06-14T00:43:37Z

ssl/ssl_lib.c

+
+int SSL_clear_quic(SSL *s)
+{
+    SSL_CONNECTION *sc = SSL_CONNECTION_FROM_SSL(s);


blank line after decl?

richsalz · 2024-06-14T00:44:12Z

ssl/ssl_lib.c

+    sc->quic_write_level = ssl_encryption_initial;
+    sc->quic_latest_level_received = ssl_encryption_initial;
+    while (sc->quic_input_data_head != NULL) {
+        QUIC_DATA *qd;


could merge with the assignment on 584

richsalz · 2024-06-14T00:44:47Z

ssl/ssl_lib.c

+    OPENSSL_free(s->ext.peer_quic_transport_params);
+    BUF_MEM_free(s->quic_buf);
+    while (s->quic_input_data_head != NULL) {
+        QUIC_DATA *qd;


could also merge here.

richsalz · 2024-06-14T00:47:56Z

ssl/statem/statem_lib.c

+                                                     s->init_num);
+            if (!ret) {
+                ret = -1;
+                /* QUIC can't sent anything out sice the above failed */


s/sent/send/ s/sice/since/

wbl · 2024-06-26T17:32:03Z

Thanks @richsalz for looking it over. I'll fix up the comments if no one else chimes in with more soon in a separate commit.

nibanks · 2024-07-17T13:40:53Z

Sorry, I haven't followed up here in a while. I just updated microsoft/msquic#4274 to use the latest commit here with the latest version of MsQuic and I am seeing build failures on Ubuntu 24.04. Note, the last time I tried this, we didn't build for 24.04 yet, so this isn't necessarily a recent break, but it is a regression from previous v3 openssl.

https://github.com/microsoft/msquic/actions/runs/9974479617/job/27562154754#step:8:2184

/usr/bin/ld: ../../../../_deps/opensslquic-build/openssl3/lib/libssl.a(libssl-lib-ssl_lib.o): in function `SSL_CTX_free':
ssl_lib.c:(.text+0x93f1): undefined reference to `OSSL_STACK_OF_X509_free'
/usr/bin/ld: ../../../../_deps/opensslquic-build/openssl3/lib/libssl.a(libssl-lib-ssl_lib.o): in function `SSL_dup':
ssl_lib.c:(.text+0x9e44): undefined reference to `OSSL_STACK_OF_X509_free'
/usr/bin/ld: ../../../../_deps/opensslquic-build/openssl3/lib/libssl.a(libssl-lib-ssl_lib.o): in function `ossl_ssl_connection_free':
ssl_lib.c:(.text+0xa262): undefined reference to `OSSL_STACK_OF_X509_free'
/usr/bin/ld: ssl_lib.c:(.text+0xa540): undefined reference to `OSSL_STACK_OF_X509_free'
/usr/bin/ld: ../../../../_deps/opensslquic-build/openssl3/lib/libssl.a(libssl-lib-ssl_lib.o): in function `SSL_CTX_new_ex':
ssl_lib.c:(.text+0xa6fc): undefined reference to `OPENSSL_LH_set_thunks'
/u
...

wbl · 2024-07-19T17:34:24Z

@nibanks I'll look into that failure: if those things got redefined to be macros or functions across versions, maybe its a build picking up wrong things sort of issue, but we should know that for sure and I'll go attack it. What's the easiest way for me to locally reproduce where I can poke around the image/reproduce in CI: open a junk pr to MSQuic?

nibanks · 2024-07-19T17:41:03Z

@nibanks I'll look into that failure: if those things got redefined to be macros or functions across versions, maybe its a build picking up wrong things sort of issue, but we should know that for sure and I'll go attack it. What's the easiest way for me to locally reproduce where I can poke around the image/reproduce in CI: open a junk pr to MSQuic?

Do you want to try to repro locally or just let MsQuic CI/CD do the heavy lifting? If you want to use MsQuic, yeah, just make a draft PR and I will approve the automation to run.

wbl · 2024-07-19T17:47:54Z

I think I've got to play with it locally just to understand. But wait,

cd /__w/msquic/msquic/build/linux/x86_openssl3/src/tools/interopserver && /usr/bin/cmake -E cmake_link_script CMakeFiles/quicinteropserver.dir/link.txt --verbose=1
/usr/bin/c++ --sysroot=/ -Og -fno-omit-frame-pointer -ggdb3 CMakeFiles/quicinteropserver.dir/InteropServer.cpp.o -o /__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3/quicinteropserver -Wl,-rpath,"$ORIGIN:__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3" /__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3/libmsquic.so.2.4.0 ../../../obj/Debug/libplatform.a ../../../obj/Debug/liblogging.a ../../../_deps/opensslquic-build/openssl3/lib/libssl.a /usr/lib/x86_64-linux-gnu/libcrypto.so -ldl /usr/lib/x86_64-linux-gnu/libatomic.so.1 /usr/lib/x86_64-linux-gnu/libnuma.so.1

Is it just me or is it linking the new libssl.a to the system libcrypto.so? Then I'm not surprised it doesn't work, as those got introduced in patch openssl@5c42ced

nibanks · 2024-07-19T17:53:25Z

I think I've got to play with it locally just to understand. But wait,

cd /__w/msquic/msquic/build/linux/x86_openssl3/src/tools/interopserver && /usr/bin/cmake -E cmake_link_script CMakeFiles/quicinteropserver.dir/link.txt --verbose=1
/usr/bin/c++ --sysroot=/ -Og -fno-omit-frame-pointer -ggdb3 CMakeFiles/quicinteropserver.dir/InteropServer.cpp.o -o /__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3/quicinteropserver -Wl,-rpath,"$ORIGIN:__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3" /__w/msquic/msquic/artifacts/bin/linux/x86_Debug_openssl3/libmsquic.so.2.4.0 ../../../obj/Debug/libplatform.a ../../../obj/Debug/liblogging.a ../../../_deps/opensslquic-build/openssl3/lib/libssl.a /usr/lib/x86_64-linux-gnu/libcrypto.so -ldl /usr/lib/x86_64-linux-gnu/libatomic.so.1 /usr/lib/x86_64-linux-gnu/libnuma.so.1

Is it just me or is it linking the new libssl.a to the system libcrypto.so? Then I'm not surprised it doesn't work, as those got introduced in patch openssl@5c42ced

Ah, you're right! It's all our builds with the -UseSystemOpenSSLCrypto flag, which statically links libssl, but dynamically links libcrytpo (for FIPS). This passed on previous versions/commits, but is failing now. How are we supposed to handle this?

wbl · 2024-07-19T18:03:38Z

My first reaction is why on earth would you expect this to work out nicely, and why do you need it: is there some other way to get what you want here? Note that the fips module is entirely unaffected by this issue as that dynamically loads with a much more restricted interface.

wbl · 2024-07-19T18:04:23Z

Alternatively try 3.3 as the system version, but that might have other issues

nibanks · 2024-07-19T18:07:32Z

My first reaction is why on earth would you expect this to work out nicely, and why do you need it: is there some other way to get what you want here? Note that the fips module is entirely unaffected by this issue as that dynamically loads with a much more restricted interface.

We need to dynamically link to the system libcrypto for FIPS compliance (because we can't bring our own binary). But we cannot link to the system libssl because it's not the quictls fork. This has worked out so far for previous v1 and v3 versions of openssl.

Alternatively try 3.3 as the system version, but that might have other issues

I don't think we had to specify anything version specific in the past. Any suggestions on how we'd test this out?

wbl · 2024-07-19T18:13:01Z

My first reaction is why on earth would you expect this to work out nicely, and why do you need it: is there some other way to get what you want here? Note that the fips module is entirely unaffected by this issue as that dynamically loads with a much more restricted interface.

We need to dynamically link to the system libcrypto for FIPS compliance (because we can't bring our own binary). But we cannot link to the system libssl because it's not the quictls fork. This has worked out so far for previous v1 and v3 versions of openssl.

That's odd: they were supposed to have limited fips issues to fips.so, but maybe you're doing something different.

Alternatively try 3.3 as the system version, but that might have other issues

I don't think we had to specify anything version specific in the past. Any suggestions on how we'd test this out?

Ideally just an apt-get update and install the new version ahead of running the existing script. Not really sure about where it is in ubuntu.

tmshort · 2024-08-12T14:42:22Z

What's the status of this PR?

wbl · 2024-08-14T07:58:12Z

I'd like more than just Rich's approval on it, given the size and scope and the number of decisions I had to make that people could possibly have issues with. Please take a look and either approve or say what you want to change.

tatsuhiro-t · 2024-08-23T09:18:37Z

Built fine with the latest ngtcp2 and all tests passed. Thank you for great work.
Once thing, that is I do not know whether it is the issue of original openssl, but the generated pkgconfig file is broken. libdir lacks lib directory.

nibanks · 2024-08-23T12:55:10Z

I just pushed the latest to microsoft/msquic#4274 to run it all again. If only our 'use system libcrypto' tests fail, I'm fine for merging this. I still need to figure out a solution for that though...

richsalz · 2024-08-23T13:35:22Z

@nibanks I agree, let's merge this once your results are known.

wbl · 2024-09-16T18:03:39Z

@nibanks it looks like it's just the usesystem openssl tests failing, but I'm not that familiar with your tests.

@tatsuhiro-t pkgconfig is a bit of a black box to me I'm afraid. If you would be so kind, open another issue after we merge this where we can solve that problem.

nibanks · 2024-09-16T22:49:10Z

I'm sorry. I just haven't had the time to follow up on this. I will see if I can make progress tomorrow morning, otherwise, don't wait on me.

tatsuhiro-t · 2024-09-17T08:43:14Z

@wbl All right. Lets move on. Will check after the merge to check that it is still an issue.

rapsys · 2024-10-27T19:23:11Z

I have two questions related to this merge, not sure it's the best place to ask, sorry in advance.

Would it be possible to get a release with quic patchset on top of openssl-3.3.2, it has two CVE fixed in it ?

Do we need to build this quictls with enable-quic & no-boring-ssl-api ?

I tried applying openssl 3.3.0->3.3.2 patchset on top of quictls-3.3.0 but when enabling quic with boring ssl api disable, I get these errors:

Test Summary Report
-------------------
25-test_verify.t                      (Wstat: 512 (exited 2) Tests: 193 Failed: 2)
  Failed tests:  127, 129
  Non-zero exit status: 2
70-test_quic_multistream.t            (Wstat: 256 (exited 1) Tests: 2 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
70-test_quic_tserver.t                (Wstat: 256 (exited 1) Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
75-test_quicapi.t                     (Wstat: 512 (exited 2) Tests: 2 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
90-test_quicfaults.t                  (Wstat: 512 (exited 2) Tests: 2 Failed: 2)
  Failed tests:  1-2
  Non-zero exit status: 2
90-test_sslapi.t                      (Wstat: 512 (exited 2) Tests: 4 Failed: 2)
  Failed tests:  1, 3
  Non-zero exit status: 2
Files=314, Tests=2882, 436 wallclock secs ( 7.35 usr  0.40 sys + 358.88 cusr 35.17 csys = 401.80 CPU)
Result: FAIL

richsalz · 2024-10-29T18:07:42Z

It has been merged, but we're no longer actively working on 3.3+quic. We are instead focused on the quictls/quictls fork.

If the community wants to take over maintenance of this branch, feel free. We can add committers as needed.

wbl force-pushed the wladd/quic-on-3.3-dirty branch from d84337d to 9f3c339 Compare May 9, 2024 16:48

tmshort and others added 5 commits May 9, 2024 09:49

QUIC: change which QUIC we use for tests

fcb02e5

QUIC: use a more modern C standard

49f05ab

QUIC: use our quic, not theirs for the basic-gcc test

2e815f7

QUIC: see if spurious change to fips-checkums.yml was cause

feace9d

QUIC: remove duplicate documentation causing test failure

fe8489f

wbl mentioned this pull request May 15, 2024

Sorting out our plans for 3.2 quictls/quictls#14

Closed

Watson Ladd added 2 commits May 20, 2024 10:51

QUIC: more doc cleanup

c912fa2

QUIC: docs again

fbb31ba

duke8253 mentioned this pull request May 23, 2024

Replace deprecated apis and update lib versions yahoo/proxy-verifier#298

Merged

wbl requested review from kaduk, tmshort and nibanks June 12, 2024 17:22

QUIC: adjust CHANGES.md

44c3d53

wbl requested a review from richsalz June 13, 2024 23:06

richsalz approved these changes Jun 14, 2024

View reviewed changes

QUIC: respond to Rich's comments

fa5a7c5

bneradt mentioned this pull request Aug 11, 2024

Should we document that this project is abandoned? #164

Closed

wbl merged commit 96b7f14 into openssl-3.3.0+quic Sep 23, 2024
192 of 195 checks passed

richsalz deleted the wladd/quic-on-3.3-dirty branch November 6, 2024 12:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC for 3.3.0 #159

QUIC for 3.3.0 #159

wbl commented May 8, 2024

wbl commented May 8, 2024

wbl commented May 13, 2024

wbl commented May 20, 2024 •

edited

Loading

tmshort commented May 22, 2024

tmshort commented May 22, 2024

tmshort commented Jun 3, 2024

wbl commented Jun 3, 2024

richsalz left a comment

richsalz Jun 13, 2024

richsalz Jun 13, 2024

richsalz Jun 14, 2024

richsalz Jun 14, 2024

richsalz Jun 14, 2024

richsalz Jun 14, 2024

wbl commented Jun 26, 2024

nibanks commented Jul 17, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

tmshort commented Aug 12, 2024

wbl commented Aug 14, 2024

tatsuhiro-t commented Aug 23, 2024

nibanks commented Aug 23, 2024

richsalz commented Aug 23, 2024

wbl commented Sep 16, 2024

nibanks commented Sep 16, 2024

tatsuhiro-t commented Sep 17, 2024

rapsys commented Oct 27, 2024

richsalz commented Oct 29, 2024

QUIC for 3.3.0 #159

QUIC for 3.3.0 #159

Conversation

wbl commented May 8, 2024

wbl commented May 8, 2024

wbl commented May 13, 2024

wbl commented May 20, 2024 • edited Loading

tmshort commented May 22, 2024

tmshort commented May 22, 2024

tmshort commented Jun 3, 2024

wbl commented Jun 3, 2024

richsalz left a comment

Choose a reason for hiding this comment

richsalz Jun 13, 2024

Choose a reason for hiding this comment

richsalz Jun 13, 2024

Choose a reason for hiding this comment

richsalz Jun 14, 2024

Choose a reason for hiding this comment

richsalz Jun 14, 2024

Choose a reason for hiding this comment

richsalz Jun 14, 2024

Choose a reason for hiding this comment

richsalz Jun 14, 2024

Choose a reason for hiding this comment

wbl commented Jun 26, 2024

nibanks commented Jul 17, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

wbl commented Jul 19, 2024

nibanks commented Jul 19, 2024

wbl commented Jul 19, 2024

tmshort commented Aug 12, 2024

wbl commented Aug 14, 2024

tatsuhiro-t commented Aug 23, 2024

nibanks commented Aug 23, 2024

richsalz commented Aug 23, 2024

wbl commented Sep 16, 2024

nibanks commented Sep 16, 2024

tatsuhiro-t commented Sep 17, 2024

rapsys commented Oct 27, 2024

richsalz commented Oct 29, 2024

wbl commented May 20, 2024 •

edited

Loading