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

Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed #100513

Merged
merged 3 commits into from
Apr 8, 2024
Merged

Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed #100513

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e65d391
Avoid frequent calls to CertificateValidationPal.IsLocalCertificateUsed
rzikm Apr 2, 2024
8ab8f13
Merge branch 'main' into 95687-SslStreamIsMutuallyAuthenticated-calls…
rzikm Apr 2, 2024
ffe2a6a
Code review feedback
rzikm Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.Net.Security/src/System/Net/Security/SslStream.IO.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,9 @@ private async Task ForceAuthenticationAsync<TIOAdapter>(bool receiveFirst, byte[
}

token.ReleasePayload();

// reset the cached flag which has potentially outdated value.
_localClientCertificateUsed = null;
Copy link
Member

Choose a reason for hiding this comment

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

Are there threading issues here? bool? is two bool fields (one for null or not, one for the value) and they're not necessarily updated or read atomically. If someone was reading IsLocalClientCertificateUsed while this was being reset, might they end up reading an incorrect value, e.g. if this flipped the value from true to false before flipping the null state?

Copy link
Member Author

@rzikm rzikm Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The value can go only from false to true, and that happens only during renegotiation/post-handshake authentication, which is usually at most once per connection.

The only thing I can think of is

  • thread 1 writes HasValue = false (part of the null assignment)
  • thread 2 accesses IsLocalClientCertificateUsed, assigning HasValue = true, Value = true
  • thread 1 writes Value = false (if that is how null assignment work)

Actual calculation of the new value is expensive (which is why I wanted to cache it) so this ordering is not likely, but still possible in theory.

If you still see any concerns than I think we can either move to something like Lazy (at the cost of small allocation) or fetching the new value here (potential small perf hit on (windows) client in mutual auth scenario).

Copy link
Member

Choose a reason for hiding this comment

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

I'd just change the storage to something written atomically, e.g. use an int that's 0 for uninitialized, 1 for true, -1 for false.

}

if (NetEventSource.Log.IsEnabled())
Expand Down
7 changes: 6 additions & 1 deletion src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ internal static bool DisableTlsResume
private X509Certificate? _selectedClientCertificate;
private X509Certificate2? _remoteCertificate;
private bool _remoteCertificateExposed;
private bool? _localClientCertificateUsed;

// These are the MAX encrypt buffer output sizes, not the actual sizes.
private int _headerSize = 5; //ATTN must be set to at least 5 by default
Expand All @@ -82,11 +83,15 @@ internal X509Certificate? LocalServerCertificate
}
}

// IsLocalCertificateUsed is expensive, but it does not change during the lifetime of the SslStream except for renegotiation, so we
// can cache the value.
private bool IsLocalClientCertificateUsed => _localClientCertificateUsed ??= CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!);

internal X509Certificate? LocalClientCertificate
{
get
{
if (_selectedClientCertificate != null && CertificateValidationPal.IsLocalCertificateUsed(_credentialsHandle, _securityContext!))
if (_selectedClientCertificate != null && IsLocalClientCertificateUsed)
{
return _selectedClientCertificate;
}
Expand Down