Skip to content

Use certificate thumbprints from entire chain in SSL_CTX cache#112858

Merged
rzikm merged 2 commits intodotnet:mainfrom
rzikm:112856-SslStreamNetworkStreamTest-failures-The-remote-certificate-is-invalid-because-of-errors-in-the-certificate-chain-PartialChain
Feb 25, 2025
Merged

Use certificate thumbprints from entire chain in SSL_CTX cache#112858
rzikm merged 2 commits intodotnet:mainfrom
rzikm:112856-SslStreamNetworkStreamTest-failures-The-remote-certificate-is-invalid-because-of-errors-in-the-certificate-chain-PartialChain

Conversation

@rzikm
Copy link
Member

@rzikm rzikm commented Feb 24, 2025

Fixes #112856.

Update the SSL context cache to utilize certificate thumbprints from the entire certificate chain. This mimics what we do for MsQuic

private readonly struct CacheKey : IEquatable<CacheKey>

Copilot AI review requested due to automatic review settings February 24, 2025 13:15
@rzikm rzikm requested a review from wfurt February 24, 2025 13:16
@rzikm
Copy link
Member Author

rzikm commented Feb 24, 2025

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

{
public readonly bool IsClient;
public readonly byte[]? CertificateThumbprint;
public readonly List<byte[]> CertificateThumbprints;
Copy link

Copilot AI Feb 24, 2025

Choose a reason for hiding this comment

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

The CertificateThumbprints field is declared as a public readonly List, but the list itself remains mutable. This can lead to unexpected behavior when this key is used in caching or dictionary operations, so consider exposing an immutable collection or making a defensive copy in the constructor.

Suggested change
public readonly List<byte[]> CertificateThumbprints;
public readonly ReadOnlyCollection<byte[]> CertificateThumbprints;

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

it seems like simple array would be sufficient as we know the count upfront...

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.

@rzikm
Copy link
Member Author

rzikm commented Feb 24, 2025

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ShreyasJejurkar
Copy link
Contributor

Nice branch name 😅😅

@rzikm
Copy link
Member Author

rzikm commented Feb 25, 2025

/ba-g No failures in System.Net.* namespace, rest of the failures should be unrelated

@rzikm rzikm merged commit d343214 into dotnet:main Feb 25, 2025
78 of 91 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SslStreamNetworkStreamTest failures "The remote certificate is invalid because of errors in the certificate chain: PartialChain"

4 participants