QUIC Frame Encoding and Decoding Functions by hlandau · Pull Request #18795 · openssl/openssl

hlandau · 2022-07-13T17:21:54Z

Note that this is built on top of the ACKM PR.

I'm holding off on including packet header functions for now until I can ask @mattcaswell if he wants them, since he owns the record layer.

This is done/ready for review so I'm not marking it as a draft, but it shouldn't be merged until ACKM is merged.

crypto/packet.c

mattcaswell

Nice code!

mattcaswell · 2022-07-18T11:40:05Z

include/internal/packet.h

+ * |*data|
+ */
+__owur static ossl_inline int PACKET_peek_net_8(const PACKET *pkt,
+                                                uint64_t *data)


Our existing use of unsigned long for the net_4/net_3 versions of these functions seems dubious. We probably should use fixed width types instead. But that is for another PR.

mattcaswell · 2022-07-18T13:53:29Z

include/internal/quic_wire.h

+ * e.g. multiple packets).
+ *
+ * Note also that this means f->len is always valid after this function returns
+ * successfully, regardless of the value of f->has_len.


Given that f->len is always set anyway - I'm wondering why f->has_len is useful when decoding.

has_len specifies whether the length field is or was included in the encoding. If it isn't, the frame runs to the end of the packet. Maybe could be named less confusingly (has_explicit_len?)

I understand what it does. My question really was why we care how it was encoded. Surely the presence or otherwise of the length field is only useful during the decoding itself? i.e. not something we need to pass around and remember after we've decoded it.

We also need a way to tell the encoder whether to include the length field. Keeping it symmetrical on the decode side seems the least surprising thing to do, as I see it.

include/internal/quic_wire.h

ssl/quic/quic_wire.c

test/quic_wire_test.c

mattcaswell · 2022-07-18T15:10:19Z

test/quic_wire_test.c

+    0x01
+};
+
+/* 3. ACK */


I feel like there are perhaps more test cases required here:

E.g. ack frame with no gaps, maximal/minimal ack delay values, no ecn data, etc

The ACK frames are definitely the hairiest frames to encode and decode. There are more tests specifically for ACK below, but more would probably be good.

ACKs with and without ECN are tested, but we can probably test ACK delays more. I'll take this as an action item.

mattcaswell · 2022-07-18T15:18:56Z

test/quic_wire_test.c

+    BUF_MEM_free(buf);
+    return testresult;
+}
+


We don't seem to have any test cases for decoding illegal frame encodings?

Good point. I'll add some.

test/quic_wire_test.c

include/internal/quic_wire.h

levitte

There are a number of places where I'd expect size_t to be used, rather than uint64_t. All the maxes, primarily...

include/internal/quic_wire.h

ssl/quic/quic_wire.c

t8m · 2022-07-21T09:42:26Z

include/internal/quic_wire.h

+    uint64_t        len;        /* Length of data in bytes */
+    const uint8_t  *data;
+
+    char            has_len;    /* 1 if len field is valid */


Is the comment right? Below we say that len is always valid on decoding and has_len indicates it was in the frame.

Added comment, and renamed to has_explicit_len since the current naming is confusing.

include/internal/quic_wire.h

t8m · 2022-07-21T09:47:43Z

include/internal/quic_wire.h

+ * frame consumes one byte; num_bytes specifies the number of bytes of padding
+ * to write.
+ */
+int ossl_quic_wire_encode_frame_padding(WPACKET *pkt, size_t num_bytes);


Should it be named ossl_quic_wire_encode_frames_padding when it creates potentially multiple frames.

Maybe just ossl_wire_encode_padding? I'll change it.

include/internal/quic_wire.h

This adds functions for encoding and decoding QUIC frames.

hlandau · 2022-07-22T14:54:19Z

Updated. Rebased, and no longer based on top of ACKM. Some small definitions in ACKM this relied on are now incorporated in this PR. ACKM will be rebased on top of this soon.

I've added testing for malformed frame decoding. Testing of ACK frames with no gaps is already tested.

include/internal/quic_wire.h

hlandau · 2022-07-25T06:29:46Z

Updated.

paulidale

Looks good. Mostly nits but a couple of more meaningful comments.

I've not gone through comparing the produced output to the formal frame definitions.

include/internal/quic_types.h

include/internal/quic_wire.h

paulidale · 2022-07-26T02:25:32Z

include/internal/quic_wire.h

+typedef struct ossl_quic_frame_new_conn_id_st {
+    uint64_t              seq_num;
+    uint64_t              retire_prior_to;
+    const unsigned char  *conn_id;


Is a fixed array appropriate here?

It would be equally valid. Any preference?

The ID cache proposal now uses fixed arrays. We might as well be consistent and use them everywhere (and pray the standard doesn't increase the size soon). Not that I'm against length + pointer. The reduction in mallocs is probably a good thing.

BTW: I'm currently proposing an array 21 long, with the size in the first byte and the up to 20 octets in the remaining (zero filled). That way one memcmp is enough to compare two. It's a bit kludgy but workable.

That's a bit obtuse. Since we will be representing connection IDs all over the place, I'll introduce a structure:

#define OSSL_QUIC_MAX_CONN_ID_LEN 20 typedef struct ossl_quic_conn_id_st { unsigned char id_len; unsigned char id[OSSL_QUIC_MAX_CONN_ID_LEN]; } OSSL_QUIC_CONN_ID;

include/internal/quic_wire.h

include/internal/time.h

ssl/quic/quic_wire.c

paulidale · 2022-07-26T02:58:48Z

ssl/quic/quic_wire.c

+            || !PACKET_get_quic_vlint(pkt, &len_))
+        return NULL;
+
+    if (len_ > SIZE_MAX


Prediction: some compilers will complain about a guaranteed true condition and we'll need to change this in the future...

Sadly, nothing comes to mind except preprocessor lines :(

Reversing the condition and excluding equality len_ < SIZE_MAX sort of circumvents this but is really ugly.

I guess we shouldn't reduce the correctness of the code to satisfy compiler warnings since this is putting the cart before the horse. I'll leave this as it is for now...

If compilers start complaining about this it will definitely happen elsewhere in the codebase too, and it will probably be more productive to figure out a general strategy for it at this time.

I'm fine with this. I wasn't expecting a resolution -- I couldn't think of one.
Once we get warnings, we can address them.

We've had some previously BTW which we've managed to avoid.

openssl-machine · 2022-07-29T05:00:14Z

This pull request is ready to merge

paulidale · 2022-07-29T06:28:50Z

Merged.

This adds functions for encoding and decoding QUIC frames. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from #18795)

This adds functions for encoding and decoding QUIC frames. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl/openssl#18795)

This adds functions for encoding and decoding QUIC frames. Reviewed-by: Tomas Mraz <[email protected]> Reviewed-by: Paul Dale <[email protected]> (Merged from openssl#18795)

From OpenSSL commit dffafaf48174497a724d546c3483d2493fc9b64c, openssl/openssl#18795 This adds functions for encoding and decoding QUIC frames. Modify the existing include/internal/packet.h with additional PACKET_peek_quic_vlint and PACKET_skip_quic_vlint functions.

hlandau added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels Jul 13, 2022

hlandau requested review from paulidale and t8m July 13, 2022 17:21

hlandau self-assigned this Jul 13, 2022

hlandau force-pushed the quic-wire-frames branch from 1739ace to 30f4fe9 Compare July 13, 2022 17:24

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

kroeckx reviewed Jul 14, 2022

View reviewed changes

crypto/packet.c Show resolved Hide resolved

mattcaswell requested changes Jul 18, 2022

View reviewed changes

levitte reviewed Jul 20, 2022

View reviewed changes

include/internal/quic_wire.h Show resolved Hide resolved

levitte reviewed Jul 21, 2022

View reviewed changes

levitte mentioned this pull request Jul 21, 2022

Implement the RX Depacketizer #18838

Closed

t8m requested changes Jul 21, 2022

View reviewed changes

hlandau added 3 commits July 22, 2022 13:39

QUIC Frame Encoding and Decoding Functions

f543e83

This adds functions for encoding and decoding QUIC frames.

fixup! QUIC Frame Encoding and Decoding Functions

e27c7ac

fixup! QUIC Frame Encoding and Decoding Functions

c2d62d0

fixup! QUIC Frame Encoding and Decoding Functions

f6949e9

hlandau force-pushed the quic-wire-frames branch from 30f4fe9 to f6949e9 Compare July 22, 2022 14:58

fixup! QUIC Frame Encoding and Decoding Functions

a4964ab

t8m reviewed Jul 22, 2022

View reviewed changes

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

fixup! QUIC Frame Encoding and Decoding Functions

72e07c7

fixup! QUIC Frame Encoding and Decoding Functions

84f7f3f

t8m approved these changes Jul 25, 2022

View reviewed changes

t8m removed the approval: otc review pending label Jul 25, 2022

paulidale reviewed Jul 26, 2022

View reviewed changes

fixup! QUIC Frame Encoding and Decoding Functions

bf6bd9c

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 Jul 28, 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 Jul 29, 2022

paulidale closed this Jul 29, 2022

levitte mentioned this pull request Sep 6, 2022

Add a record layer design document #17969

Closed

Uh oh!

Comments

Conversation

hlandau commented Jul 13, 2022

Uh oh!

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

levitte left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hlandau commented Jul 22, 2022

Uh oh!

Uh oh!

hlandau commented Jul 25, 2022

Uh oh!

paulidale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte left a comment •

edited

Loading