-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
[release/7.0] set session ID when TLS resume is enabled #75507
Conversation
Co-authored-by: Jeremy Barton <jbarton@microsoft.com>
Tagging subscribers to this area: @dotnet/ncl, @vcsjones Issue DetailsBackport of #75435 to release/7.0 /cc @wfurt Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
please let us know @danmoseley if you approve it. |
@@ -655,7 +655,7 @@ void CryptoNative_SslSetVerifyPeer(SSL* ssl) | |||
SSL_set_verify(ssl, SSL_VERIFY_PEER, verify_callback); | |||
} | |||
|
|||
int CryptoNative_SslCtxSetCaching(SSL_CTX* ctx, int mode, int cacheSize, SslCtxNewSessionCallback newSessionCb, SslCtxRemoveSessionCallback removeSessionCb) | |||
int CryptoNative_SslCtxSetCaching(SSL_CTX* ctx, int mode, int cacheSize, int contextIdLength, uint8_t* contextId, SslCtxNewSessionCallback newSessionCb, SslCtxRemoveSessionCallback removeSessionCb) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: contextId
can be const pointer as well. As far as I remember SSL_CTX_set_session_id_context
takes a const pointer as parameter.
There are few timeouts in the CI on Linux (https://helixre107v0xdeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-75507-merge-6f98f16b4dcf4e7aa6/System.Net.Http.Functional.Tests.Attempt.3/3/console.8029c881.log?helixlogtype=result), but they don't feel related (not all failed tests use HTTPS). The OSX installer build and test failure is unrelated. I will request rerun of those two legs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Approved: clearly we can't ship with this regression. It would be great to find a functional test we could check in (not necessarily with this PR) |
@carlossanlop it is ready for merge - approval, CR & tests green. |
I talk to @karelz and we will open new issue for tracking for the missing test. I think we may need to update test in general - most of the functional tests are just one try and pass or fail. That is trickier with the possible resume as the behavior may change after reuse. |
All good. Merging now. |
Backport of #75435 to release/7.0
fixes #75079
/cc @wfurt
Customer Impact
Kestrel (and perhaps other .NET servers) fail to reliably negotiate TLS when running on Linux and client certificates are allowed or requested.
This is regression caused by 7.0 changes.
Reported by 1st party trying to move to .NET 7.
Testing
Testing is manual so far + automated functional tests pass. This was reported on Linux server with Windows clients and I failed so far to construct functional test that would reproduce the origin issue. I used local setup with Kestrel on Linux and Windows client to verify the fix.
Risk
small. This basically initializes some property on SSL_CTX so it can be later to possibly perform TLS resume.