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

Update dtls max version for dtls1.3 #22275

Closed

Conversation

fwh-dc
Copy link
Contributor

@fwh-dc fwh-dc commented Oct 4, 2023

Another dtls 1.3 specific update. It is dependent on #22259

@fwh-dc fwh-dc changed the title Update dtls max version Update dtls max version for dtls1.3 Oct 4, 2023
@t8m t8m added branch: master Merge to master branch triaged: feature The issue/pr requests/adds a feature labels Oct 9, 2023
@fwh-dc fwh-dc mentioned this pull request Oct 17, 2023
@t8m t8m added this to the DTLS-1.3 milestone Oct 18, 2023
ssl/statem/statem_clnt.c Outdated Show resolved Hide resolved
@mattcaswell mattcaswell added the approval: review pending This pull request needs review by a committer label Nov 29, 2023
@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

@openssl-machine
Copy link
Collaborator

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

@fwh-dc fwh-dc force-pushed the update-dtls-max-internal-version branch from 9abeed3 to 86e2b86 Compare March 21, 2024 12:00
@fwh-dc fwh-dc changed the base branch from master to feature/dtls-1.3 March 21, 2024 12:01
@fwh-dc
Copy link
Contributor Author

fwh-dc commented Mar 21, 2024

Test failures are relevant. We need to merge a couple of other PR's before this is working.

@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 21, 2024
@mattcaswell
Copy link
Member

Test failures are relevant. We need to merge a couple of other PR's before this is working.

Will those failure go away with a rebase now?

@fwh-dc
Copy link
Contributor Author

fwh-dc commented Apr 3, 2024

Test failures are relevant. We need to merge a couple of other PR's before this is working.

Will those failure go away with a rebase now?

No. We need #22364 to be merged first. Which is dependent on #22366.

And then I expect there are some test cases that needs to be limited to DTLS 1.2 for it to work. I'll add that work to this PR to fix the pipeline.

@fwh-dc
Copy link
Contributor Author

fwh-dc commented Apr 19, 2024

@t8m @mattcaswell

A quick update on this.

Please prioritise reviewing the following:
#22936
#22401
#22380
#22360

When they are merged I'll do a rebase of this branch and make some adjustments to make the tests pass (some has to be forced to run on DTLS 1.2 for now). That will enable the dtls 1.3 feature branch to negotiate dtls 1.3. And then we can make the required adjustments to make it correct according to the RFC.

@mattcaswell
Copy link
Member

Please prioritise reviewing the following:

All of these are now merged

@fwh-dc
Copy link
Contributor Author

fwh-dc commented Apr 22, 2024

Yes thanks. I'll update this PR today or tomorrow.

@mattcaswell
Copy link
Member

Note that #24161 is going to cause problems for this one. My current plan is:

Rebase the feature/dtls-1.3 branch later today on master (there will be no conflicts from this)
Merge #24161 to master
Once #24226 is approved I can rebase the feature/dtls-1.3 branch again which will bring #24161 onto this feature branch

At that point you will need to update this PR again to fix any resulting issues.

@mattcaswell
Copy link
Member

feature/dtls-1.3 has been rebased to just before #24161 merge
#24161 has been merged to master
#24226 has been approved - awaiting 24h timer before merge

@mattcaswell
Copy link
Member

Once #24226 is approved I can rebase the feature/dtls-1.3 branch again which will bring #24161 onto this feature branch

This has now also been done.

So this PR needs to be rebased against the latest feature branch. It will also need some amendment to handle the changes from #24161

@fwh-dc fwh-dc force-pushed the update-dtls-max-internal-version branch from 86e2b86 to 7c0015a Compare April 24, 2024 14:02
@fwh-dc
Copy link
Contributor Author

fwh-dc commented Apr 24, 2024

@mattcaswell Please review again. I've marked a couple of tests for reenablement and some has been forced to DTLS 1.2. Please let me know what you think.

@t8m
Copy link
Member

t8m commented Apr 24, 2024

At least some of the CI failures look relevant.

@fwh-dc fwh-dc force-pushed the update-dtls-max-internal-version branch 2 times, most recently from 80c9564 to 93cb4a5 Compare May 3, 2024 08:28
@fwh-dc
Copy link
Contributor Author

fwh-dc commented May 8, 2024

@mattcaswell @t8m

I think I found a fix for the memory leaks, but the patch in 6adbb30 seems to break compilation. TBH I don't understand why. Maybe you know what's causing it?

@mattcaswell
Copy link
Member

It is because the test file test/tls13secretstest.c mocks outs some internal functions and links in ssl/tls13_enc.c directly:

openssl/test/build.info

Lines 1028 to 1038 in deaa83a

# We disable this test completely in a shared build because it deliberately
# redefines some internal libssl symbols. This doesn't work in a non-shared
# build
IF[{- !$disabled{shared} -}]
PROGRAMS{noinst}=tls13secretstest
SOURCE[tls13secretstest]=tls13secretstest.c
DEFINE[tls13secretstest]=OPENSSL_NO_KTLS
SOURCE[tls13secretstest]= ../ssl/tls13_enc.c ../crypto/packet.c ../crypto/quic_vlint.c
INCLUDE[tls13secretstest]=.. ../include ../apps/include
DEPEND[tls13secretstest]=../libcrypto ../libssl libtestutil.a
ENDIF

To fix it you will need to supply dummy implementations of dtls1_clear_received_buffer and dtls1_clear_sent_buffer in tls13secretstest.c:

void dtls1_clear_received_buffer(SSL_CONNECTION *s)
{
}

void dtls1_clear_sent_buffer(SSL_CONNECTION *s)
{
}

@fwh-dc fwh-dc force-pushed the update-dtls-max-internal-version branch from 6adbb30 to 4cbeb3e Compare May 8, 2024 09:31
@fwh-dc
Copy link
Contributor Author

fwh-dc commented May 8, 2024

@mattcaswell @t8m

This is ready for review once again. I have added some handling of middlebox compatibility and fixed the memory leak. Please let me know what you think.

@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 May 8, 2024
@openssl-machine 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 May 9, 2024
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell
Copy link
Member

Pushed. Thanks.

openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
…DTLS 1.3

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
…erver_version()

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
…rom compilation path.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
openssl-machine pushed a commit that referenced this pull request May 10, 2024
…r objects.

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #22275)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge tests: present The PR has suitable tests present triaged: feature The issue/pr requests/adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants