QUIC Demuxer and Record Layer (RX+TX) #18949

hlandau · 2022-08-03T13:07:54Z

Here we go.

RX only for now aside from some small currently-vestigial pieces of TX functionality.

Key update is mandatory AIUI but involves interaction between the TX/RX pieces so the plan is:

Develop the TX side on top of this PR and update this PR
Add tests which get the TX and RX sides talking to each other
Key update and key update tests

So this shouldn't be merged yet (well, it could be, and I just make a new PR), but reviews are welcome.

This includes a demuxer because implementing the QRL resulted in 90% of the functionality of a demuxer and it made no sense not to include it, since there's necessary interaction between the two.

mattcaswell

Mostly just looked at the headers so far.

include/internal/quic_demux.h

include/internal/quic_record.h

ssl/quic/quic_record_util.c

include/internal/quic_types.h

include/internal/quic_demux.h

include/internal/quic_types.h

include/internal/quic_wire_pkt.h

hlandau · 2022-08-04T09:39:33Z

Not sure what's going on with a/ub/msan. I can't replicate it on my machine with either clang or GCC sanitizers, and the error reported isn't even a sanitizer error... the test is supposed to be deterministic. Hm...

hlandau · 2022-08-04T09:40:16Z

Just some nits for now.

hlandau · 2022-08-04T11:47:57Z

I've looked at the use of OSSL_RECORD_METHOD by TLS and KTLS... IMO it's going to be premature to do something like this for now, especially since the KTLS interface for QUIC literally doesn't exist yet. It makes more sense when needing to support four versions of TLS, DTLS plus KTLS, but the scope of variation for QUIC is a lot narrower. I think this can wait for now; moreover, it's either a question of refactoring this now with an unknown design space (future KTLS APIs) which will probably have to be redone later, or later when we actually know what's needed. So, I'd like to avoid introducing polymorphism into the QRL for now.

The msan issue is weird. Going to have to try and repro it on a build server.

mattcaswell · 2022-08-04T12:05:28Z

I've looked at the use of OSSL_RECORD_METHOD by TLS and KTLS... IMO it's going to be premature to do something like this for now, especially since the KTLS interface for QUIC literally doesn't exist yet. It makes more sense when needing to support four versions of TLS, DTLS plus KTLS, but the scope of variation for QUIC is a lot narrower. I think this can wait for now; moreover, it's either a question of refactoring this now with an unknown design space (future KTLS APIs) which will probably have to be redone later, or later when we actually know what's needed. So, I'd like to avoid introducing polymorphism into the QRL for now.

Fair enough.

hlandau · 2022-08-04T12:13:05Z

The test issue was caused by random test ordering. Fixed.

hlandau · 2022-08-04T12:48:53Z

Updated, refactored.

The other KDF function is similar to tls13_generate_secret but not a match. It doesn't use the same label found in that function, and uses HKDF instead of TLS1_3_KDF. So I've kept this separate.

I think this settles the outstanding feedback.

hlandau · 2022-08-11T12:59:30Z

Updated:

Adds TX side.
The QRL has been split into independent RX (QRX) and TX (QTX) objects as I'm now satisfied they can be separated reasonably cleanly.
QRL (the RX side) has been renamed to QRX.
Some tests for the TX side.
Key expiration functionality has been added.
A preview of the API and logical model for key update can be viewed in the header files quic_record_rx.h and quic_record_tx.h. No implementation yet.

hlandau · 2022-08-11T13:02:24Z

Also while I remember: @paulidale, let me know if it would be more helpful for the iovecs to be a linked list instead of an array.

levitte · 2022-08-15T05:12:47Z

Do I understand correctly that this replaces #18870?

include/internal/quic_record_rx.h

hlandau · 2022-08-15T06:52:01Z

Do I understand correctly that this replaces #18870?

Yeah, it does.

include/internal/quic_record_rx.h

ssl/quic/quic_wire_pkt.c

hlandau · 2022-08-15T15:21:52Z

Updated:

Adds an RX time field to the OSSL_QRX_PKT structure.
Adds a timekeeping argument to ossl_demux_new which is used to determine
packet reception time.
Adds a decoded PN field to the OSSL_QRX_PKT structure.
This has to be decoded by the QRX anyway, and its omission was an oversight.
Key update support for the TX side.
Minor refactoring.

ssl/quic/quic_record_rx.c

…toring

hlandau · 2022-08-26T11:21:33Z

Updated (nits).

t8m

Nits mostly. Great work!

ssl/quic/quic_record_rx.c

t8m · 2022-08-26T10:27:44Z

ssl/quic/quic_record_rx.c

+    /* Clients should never receive 0-RTT packets. */
+    if (rxe->hdr.type == QUIC_PKT_TYPE_0RTT)


Given we will need at least rudimentary server for testing, should this be prepared to handle server side? I.e., have a client flag in the OSSL_QRX?

Yeah, seems reasonable. I'll get on this at some point.

/* TODO(QUIC): ... */ would WFM too.

ssl/quic/quic_record_rx.c

ssl/quic/quic_wire_pkt.c

…toring

hlandau · 2022-08-26T14:38:04Z

Updated, minor changes.

…toring

hlandau · 2022-08-26T16:55:17Z

Added comment.

include/internal/quic_demux.h

openssl-machine · 2022-09-02T08:00:15Z

This pull request is ready to merge

t8m · 2022-09-02T08:05:12Z

Merged to master branch. Thank you for this great work!

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18949)

- Adds an RX time field to the OSSL_QRX_PKT structure. - Adds a timekeeping argument to ossl_demux_new which is used to determine packet reception time. - Adds a decoded PN field to the OSSL_QRX_PKT structure. This has to be decoded by the QRX anyway, and its omission was an oversight. - Key update support for the TX side. - Minor refactoring. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18949)

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

- Adds an RX time field to the OSSL_QRX_PKT structure. - Adds a timekeeping argument to ossl_demux_new which is used to determine packet reception time. - Adds a decoded PN field to the OSSL_QRX_PKT structure. This has to be decoded by the QRX anyway, and its omission was an oversight. - Key update support for the TX side. - Minor refactoring. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

- Adds an RX time field to the OSSL_QRX_PKT structure. - Adds a timekeeping argument to ossl_demux_new which is used to determine packet reception time. - Adds a decoded PN field to the OSSL_QRX_PKT structure. This has to be decoded by the QRX anyway, and its omission was an oversight. - Key update support for the TX side. - Minor refactoring. Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

hlandau added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: feature The issue/pr requests/adds a feature labels Aug 3, 2022

hlandau requested review from t8m and mattcaswell August 3, 2022 13:07

hlandau self-assigned this Aug 3, 2022

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 3, 2022

hlandau force-pushed the quic-rlrx2 branch 2 times, most recently from 65818dc to fe3b438 Compare August 3, 2022 14:18

mattcaswell requested changes Aug 3, 2022

View reviewed changes

mattcaswell mentioned this pull request Aug 8, 2022

TX packetiser (QUIC) #18570

Closed

2 tasks

hlandau changed the title ~~QUIC Demuxer and Record Layer (RX Side)~~ QUIC Demuxer and Record Layer (RX+TX) Aug 11, 2022

levitte reviewed Aug 15, 2022

View reviewed changes

include/internal/quic_record_rx.h Show resolved Hide resolved

levitte mentioned this pull request Aug 15, 2022

Implement the RX Depacketizer #18838

Closed

levitte reviewed Aug 15, 2022

View reviewed changes

include/internal/quic_record_rx.h Outdated Show resolved Hide resolved

levitte reviewed Aug 15, 2022

View reviewed changes

ssl/quic/quic_wire_pkt.c Show resolved Hide resolved

hlandau force-pushed the quic-rlrx2 branch from d95ed96 to 3674365 Compare August 15, 2022 15:21

hlandau mentioned this pull request Aug 15, 2022

Add prototype QUIC Record Layer API design #18870

Closed

hlandau force-pushed the quic-rlrx2 branch from 025e055 to f1421da Compare August 24, 2022 17:28

t8m reviewed Aug 26, 2022

View reviewed changes

ssl/quic/quic_record_rx.c Show resolved Hide resolved

t8m reviewed Aug 26, 2022

View reviewed changes

ssl/quic/quic_record_rx.c Show resolved Hide resolved

ssl/quic/quic_record_rx.c Outdated Show resolved Hide resolved

ssl/quic/quic_record_rx.c Outdated Show resolved Hide resolved

fixup! TX key update support, RX time and PN reporting, general refac…

e6fcb60

…toring

t8m requested changes Aug 26, 2022

View reviewed changes

fixup! TX key update support, RX time and PN reporting, general refac…

024f145

…toring

fixup! TX key update support, RX time and PN reporting, general refac…

dece70b

…toring

t8m approved these changes Aug 29, 2022

View reviewed changes

t8m removed the approval: otc review pending This pull request needs review by an OTC member label Aug 29, 2022

paulidale reviewed Aug 31, 2022

View reviewed changes

include/internal/quic_demux.h Show resolved Hide resolved

paulidale approved these changes Sep 1, 2022

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Sep 1, 2022

openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Sep 2, 2022

t8m closed this Sep 2, 2022

openssl-machine pushed a commit that referenced this pull request Sep 2, 2022

QUIC Demuxer and Record Layer (RX Side)

ec279ac

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18949)

openssl-machine pushed a commit that referenced this pull request Sep 2, 2022

QUIC Record Layer (Refactor and TX Side)

1957148

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from #18949)

sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022

QUIC Demuxer and Record Layer (RX Side)

431a122

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

sftcd pushed a commit to sftcd/openssl that referenced this pull request Sep 24, 2022

QUIC Record Layer (Refactor and TX Side)

5ce8ce5

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

QUIC Demuxer and Record Layer (RX Side)

5723ee8

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

QUIC Record Layer (Refactor and TX Side)

0c46ad2

Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Tomas Mraz <tomas@openssl.org> (Merged from openssl#18949)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QUIC Demuxer and Record Layer (RX+TX) #18949

QUIC Demuxer and Record Layer (RX+TX) #18949

hlandau commented Aug 3, 2022

mattcaswell left a comment

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

mattcaswell commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 11, 2022

hlandau commented Aug 11, 2022

levitte commented Aug 15, 2022

hlandau commented Aug 15, 2022

hlandau commented Aug 15, 2022

hlandau commented Aug 26, 2022

t8m left a comment

t8m Aug 26, 2022

hlandau Aug 26, 2022

t8m Aug 26, 2022

hlandau commented Aug 26, 2022

hlandau commented Aug 26, 2022

openssl-machine commented Sep 2, 2022

t8m commented Sep 2, 2022

		/* Clients should never receive 0-RTT packets. */
		if (rxe->hdr.type == QUIC_PKT_TYPE_0RTT)

QUIC Demuxer and Record Layer (RX+TX) #18949

QUIC Demuxer and Record Layer (RX+TX) #18949

Conversation

hlandau commented Aug 3, 2022

mattcaswell left a comment

Choose a reason for hiding this comment

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

mattcaswell commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 4, 2022

hlandau commented Aug 11, 2022

hlandau commented Aug 11, 2022

levitte commented Aug 15, 2022

hlandau commented Aug 15, 2022

hlandau commented Aug 15, 2022

hlandau commented Aug 26, 2022

t8m left a comment

Choose a reason for hiding this comment

t8m Aug 26, 2022

Choose a reason for hiding this comment

hlandau Aug 26, 2022

Choose a reason for hiding this comment

t8m Aug 26, 2022

Choose a reason for hiding this comment

hlandau commented Aug 26, 2022

hlandau commented Aug 26, 2022

openssl-machine commented Sep 2, 2022

t8m commented Sep 2, 2022