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.

Sign up for GitHub

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

Jump to bottom

QUIC Conformance Updates: Miscellaneous #21547

Closed
wants to merge 20 commits into from
Closed

QUIC Conformance Updates: Miscellaneous #21547

wants to merge 20 commits into from

Conversation

hlandau
Copy link
Member

@hlandau hlandau commented Jul 25, 2023

Fixes openssl/project#36

Built on top of the shutdown drainage PR.

b164838bd3 QUIC: Test crypto stream FC limits
b365463e35 QUIC CONFORMANCE: Test that CRYPTO frames with bad offsets/lengths are rejected
aeaba0530c QUIC WIRE: RFC 9000 s. 19.6
0a6cf56c01 QUIC CHANNEL: Apply flow control to CRYPTO streams
17bc244a94 QUIC FC: Rename stream count mode to reflect actual function
6a33ae1f74 QUIC CHANNEL: Fix typo
999bd0e071 QUIC QRX: Test for 1-RTT processing restriction
5bda0280cf QUIC QRX: Don't process 1-RTT packets until handshake is complete
673b5d6b0b QUIC QRX: Enforce PN monotonicity with key updates
c37568f726 QUIC CHANNEL, TXP: Discard INITIAL EL correctly
58e143afb5 QUIC: Update no-TPARAM test for correct error code
39d6fa0a7d QUIC TLS: Report TLS errors properly as QUIC protocol errors
910f584b19 QUIC CHANNEL: Send correct alert code if no TPARAMs received
80bf44c8f7 QUIC TXP: Allow PATH_RESPONSE to force padding
01d9ead721 QUIC CFQ: Unreliable transmission for PATH_RESPONSE
dfd66568ef QUIC: Echo PATH_CHALLENGE frames as PATH_RESPONSE frames

@hlandau 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 tests: present The PR has suitable tests present labels Jul 25, 2023
@hlandau hlandau self-assigned this Jul 25, 2023
@arapov arapov mentioned this pull request Jul 25, 2023
Closed
mattcaswell
mattcaswell approved these changes Jul 28, 2023
View reviewed changes
ssl/quic/quic_txp.c Outdated Show resolved Hide resolved
ssl/quic/quic_tls.c Outdated Show resolved Hide resolved
ssl/quic/quic_tls.c Show resolved Hide resolved
test/quic_multistream_test.c

if (!TEST_true(WPACKET_quic_write_vlint(&wpkt, OSSL_QUIC_FRAME_TYPE_CRYPTO))
|| !TEST_true(WPACKET_quic_write_vlint(&wpkt, (((uint64_t)1) << 62) - 1))
|| !TEST_true(WPACKET_quic_write_vlint(&wpkt, 1))
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 additionally checking maximum offset and maximum length would be worthwhile? i.e. to check no overflows etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will come in the Assorted Tests PR.

@hlandau
Copy link
Member Author

hlandau commented Jul 28, 2023

Updated with better error handling (last commit).

@t8m t8m added the triaged: feature The issue/pr requests/adds a feature label Jul 31, 2023
@hlandau
Copy link
Member Author

hlandau commented Aug 1, 2023

@paulidale Updated.

t8m
t8m approved these changes Aug 1, 2023
View reviewed changes
Copy link
Member

@t8m t8m left a comment

Choose a reason for hiding this comment

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

Approved with the one comment nit fixed.

ssl/quic/quic_rx_depack.c Outdated Show resolved Hide resolved
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Aug 1, 2023
mattcaswell
mattcaswell approved these changes Aug 2, 2023
View reviewed changes
Copy link
Member

@mattcaswell mattcaswell left a comment

Choose a reason for hiding this comment

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

Approved assuming the nit identified by @t8m is fixed.

@mattcaswell mattcaswell 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 Aug 2, 2023
@hlandau
Copy link
Member Author

hlandau commented Aug 2, 2023

Rebased and updated to fix nit.

@hlandau
Copy link
Member Author

hlandau commented Aug 2, 2023

Regression during rebase, investigating.

@hlandau hlandau force-pushed the quic-conformance-misc branch 2 times, most recently from bbbcaa2 to 422dd29 Compare August 3, 2023 10:57
@hlandau
Copy link
Member Author

hlandau commented Aug 3, 2023 •
edited

Rebased again and fixed bug around QUIC_TLS error handling.

I've substantially rethought how to do QUIC_TLS error handling, so I recommend reviewing the last commit closely. It also adds some new APIs to libcrypto.

9916a76026 QUIC TLS: Rethink error handling

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Aug 3, 2023
@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@hlandau hlandau removed the approval: done This pull request has the required number of approvals label Aug 3, 2023
@mattcaswell mattcaswell removed the approval: otc review pending This pull request needs review by an OTC member label Aug 7, 2023
@mattcaswell
Copy link
Member

@t8m - can you reconfirm your earlier approval?

@t8m t8m 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 Aug 7, 2023
@mattcaswell
Copy link
Member

Pushed. Thanks!

@mattcaswell mattcaswell closed this Aug 8, 2023
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC: Echo PATH_CHALLENGE frames as PATH_RESPONSE frames
7eb330f 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CFQ: Unreliable transmission for PATH_RESPONSE
371c295 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC TXP: Allow PATH_RESPONSE to force padding
c5cb85b 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CHANNEL: Send correct alert code if no TPARAMs received
3ad5711 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC TLS: Report TLS errors properly as QUIC protocol errors
80bcc4f 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC: Update no-TPARAM test for correct error code
457678d 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CHANNEL, TXP: Discard INITIAL EL correctly
3eb0f9a 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC QRX: Enforce PN monotonicity with key updates
0c1cc36 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC QRX: Don't process 1-RTT packets until handshake is complete
2a6f1f2 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC QRX: Test for 1-RTT processing restriction
869ab3e 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CHANNEL: Fix typo
7c793cd 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC FC: Rename stream count mode to reflect actual function
1051b4a 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CHANNEL: Apply flow control to CRYPTO streams
098914d 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC WIRE: RFC 9000 s. 19.6
67e72ed 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CONFORMANCE: Test that CRYPTO frames with bad offsets/lengths ar…
27c2f62 
…e rejected

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC: Test crypto stream FC limits
ab6c634 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC CHANNEL: Improve error reporting
741170b 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC: Move string conversion functions into a source file
3989224 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC: Fix nit
828c9c6 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
openssl-machine pushed a commit that referenced this pull request Aug 8, 2023
@hlandau @mattcaswell
QUIC TLS: Rethink error handling
7a2bb21 
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21547)
t8m added a commit to t8m/openssl that referenced this pull request Aug 9, 2023
@t8m
Remove TODO(QUIC) about raising errors from ossl_quic_tls_tick()
74ed3c9 
This was already resolved by openssl#21547
t8m added a commit to t8m/openssl that referenced this pull request Aug 16, 2023
@t8m
Remove TODO(QUIC) about raising errors from ossl_quic_tls_tick()
9adbcaf 
This was already resolved by openssl#21547
openssl-machine pushed a commit that referenced this pull request Aug 23, 2023
@t8m
Remove TODO(QUIC) about raising errors from ossl_quic_tls_tick()
68b9a32 
This was already resolved by #21547

Reviewed-by: Hugo Landau <hlandau@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #21700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulidale paulidale paulidale left review comments

@t8m t8m t8m approved these changes

@mattcaswell mattcaswell mattcaswell approved these changes

Assignees

@hlandau hlandau

Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch severity: fips change The pull request changes FIPS provider sources tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
Project Board
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

QUIC Conformance Updates: Miscellaneous
5 participants
@hlandau @openssl-machine @mattcaswell @t8m @paulidale