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

Expose Interface for App to Set Stateless Reset Key #3879

Merged
merged 18 commits into from
Sep 28, 2023

Conversation

ukingliu
Copy link
Contributor

@ukingliu ukingliu commented Sep 25, 2023

Description

At present, each MsQuic server utilizes a distinct stateless reset key. When the MsQuic client and server successfully complete the handshake process, they establish communication through an encrypted channel, utilizing an encryption key generated from the respective stateless reset key for their interactions. However, in the event of a server crash followed by a restart, the client remains unaware of this disruption, leaving it unable to communicate with the server due to the unavailability of the encryption key. To resolve this issue, we have introduced an interface that allows the App to set the same stateless reset key on the MsQuic server side.

Fixes #1719.

Testing

We have introduced two new test cases. The first one is designed to validate the length of the stateless reset key to ensure its validity. The second test case focuses on validating the behavior when the stateless reset key is changed and the server is subsequently shut down silently, leading to a client timeout instead of abort scenario.

Documentation

No.

@ukingliu ukingliu requested a review from a team as a code owner September 25, 2023 15:48
src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/inc/msquic.h Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/core/library.c Outdated Show resolved Hide resolved
src/inc/msquic.h Outdated Show resolved Hide resolved
src/manifest/clog.sidecar Outdated Show resolved Hide resolved
ukingliu and others added 4 commits September 25, 2023 09:45
How about we use the same way as the random initial reset key, we call  MsQuicLibraryFreePartitions() to set all PerProc->ResetTokenHash to NULL

Co-authored-by: Nick Banks <nibanks@microsoft.com>
src/core/library.c Outdated Show resolved Hide resolved
src/inc/msquic.h Outdated Show resolved Hide resolved
src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #3879 (6be46b1) into main (226138d) will decrease coverage by 0.26%.
Report is 24 commits behind head on main.
The diff coverage is 94.73%.

@@            Coverage Diff             @@
##             main    #3879      +/-   ##
==========================================
- Coverage   86.80%   86.54%   -0.26%     
==========================================
  Files          56       56              
  Lines       16693    16712      +19     
==========================================
- Hits        14490    14464      -26     
- Misses       2203     2248      +45     
Files Coverage Δ
src/core/library.c 83.90% <94.73%> (-0.30%) ⬇️

... and 11 files with indirect coverage changes

src/manifest/clog.sidecar Outdated Show resolved Hide resolved
@ukingliu ukingliu changed the title [draft] Allow stateless reset token key to be set #1719 Allow stateless reset token key to be set #1719 Sep 26, 2023
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

Great job! Just few small things left and this should be good to merge.

src/test/MsQuicTests.h Outdated Show resolved Hide resolved
src/inc/msquic.h Show resolved Hide resolved
src/core/library.c Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
src/test/lib/DataTest.cpp Outdated Show resolved Hide resolved
@nibanks
Copy link
Member

nibanks commented Sep 26, 2023

Looks like the CheckDotnet test failed because you need to update the generated file again. Please run .\scripts\generate-dotnet.ps1.

@nibanks nibanks added Area: API Area: Core Related to the shared, core protocol logic labels Sep 26, 2023
@nibanks nibanks changed the title Allow stateless reset token key to be set #1719 Expose Interface for App to Set Stateless Reset Key Sep 26, 2023
ukingliu and others added 4 commits September 26, 2023 10:15
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
Co-authored-by: Nick Banks <nibanks@microsoft.com>
@nibanks
Copy link
Member

nibanks commented Sep 27, 2023

CheckDotnet is still failing. Please fix that up.

CIFuzz is failing because spinquic is setting the global paramter before a registration is created (good catch):

==40==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000[110](https://github.com/microsoft/msquic/actions/runs/6321366569/job/17165278108?pr=3879#step:5:111) (pc 0x7f686229bfc4 bp 0x7f685c3f4cd0 sp 0x7f685c3f4c08 T10)
==40==The signal is caused by a READ memory access.
==40==Hint: address points to the zero page.
SCARINESS: 10 (null-deref)
    #0 0x7f686229bfc4 in __pthread_mutex_lock (/lib/x86_64-linux-gnu/libpthread.so.0+0xafc4) (BuildId: 7b4536f41cdaa5888408e82d0836e33dcf436466)
    #1 0x4eb114 in __interceptor_pthread_mutex_lock /src/llvm-project/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc
    #2 0x584379 in QuicLibrarySetGlobalParam /src/msquic/src/core/library.c:1135:13
    #3 0x86fb89 in MsQuicSetParam /src/msquic/src/core/api.c:1383:18
    #4 0x57a997 in RunThread(void*) /src/./msquic/src/tools/spin/spinquic.cpp:1330:18
    #5 0x7f6862299608 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8608) (BuildId: 7b4536f41cdaa5888408e82d0836e33dcf436466)
    #6 0x7f6862193132 in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x11f132) (BuildId: 1878e6b475720c7c51969e69ab2d276fae6d1dee)

The code in library.c needs to handle this case (just fail with invalid state if not initialized yet).

@nibanks
Copy link
Member

nibanks commented Sep 27, 2023

Oh, and we do have preexisting docs for parameters in settings.md. Please update those docs for this new setting.

src/test/lib/ApiTest.cpp Outdated Show resolved Hide resolved
@ukingliu ukingliu enabled auto-merge (squash) September 28, 2023 15:16
docs/Settings.md Outdated Show resolved Hide resolved
Copy link
Member

@nibanks nibanks left a comment

Choose a reason for hiding this comment

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

Great job! Thanks for doing this!

@ukingliu ukingliu merged commit 649f971 into main Sep 28, 2023
398 of 400 checks passed
@ukingliu ukingliu deleted the user/yuqliu/resettoken branch September 28, 2023 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: API Area: Core Related to the shared, core protocol logic
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow stateless reset token key to be set
3 participants