Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -171,18 +171,22 @@ internal abstract class SafeFreeCredentials : SafeHandle
{
#endif

internal DateTime _expiry;
internal Interop.SspiCli.CredHandle _handle; //should be always used as by ref in PInvokes parameters

protected SafeFreeCredentials() : base(IntPtr.Zero, true)
{
_handle = default;
_expiry = DateTime.MaxValue;
}

public override bool IsInvalid
{
get { return IsClosed || _handle.IsZero; }
}

public DateTime Expiry => _expiry;

#if DEBUG
public new IntPtr DangerousGetHandle()
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Runtime.ConstrainedExecution;
using System.Runtime.InteropServices;
Expand All @@ -18,8 +19,13 @@ internal abstract class SafeFreeCredentials : DebugSafeHandle
internal abstract class SafeFreeCredentials : SafeHandle
{
#endif
internal DateTime _expiry;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this is tricky but AFAIK Unix does not have the same issue as Windows. I'm wondering if we should (need to?) replicate the logic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I wasn't sure how to do this so that the code compiles for both platforms, since the code for SslSessionsCache where I access the Expiry property is not platform specific. However, it would be possible to always leave MaxValue on non-windows platforms and set the expiry in SslStreamPal.Windows.cs


public DateTime Expiry => _expiry;

protected SafeFreeCredentials(IntPtr handle, bool ownsHandle) : base(handle, ownsHandle)
{
_expiry = DateTime.MaxValue;
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,8 @@ internal void Close()
{
if (!_remoteCertificateExposed)
{
_remoteCertificate?.Dispose();
_remoteCertificate = null;
_remoteCertificate?.Dispose();
_remoteCertificate = null;
}

_securityContext?.Dispose();
Expand Down Expand Up @@ -607,9 +607,7 @@ private bool AcquireClientCredentials(ref byte[]? thumbPrint)
_sslAuthenticationOptions.CertificateContext = SslStreamCertificateContext.Create(selectedCert!);
}

_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext,
_sslAuthenticationOptions.EnabledSslProtocols, _sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);

_credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions);
thumbPrint = guessedThumbPrint; // Delay until here in case something above threw.
}
}
Expand Down Expand Up @@ -713,14 +711,65 @@ private bool AcquireServerCredentials(ref byte[]? thumbPrint)
}
else
{
_credentialsHandle = SslStreamPal.AcquireCredentialsHandle(_sslAuthenticationOptions.CertificateContext, _sslAuthenticationOptions.EnabledSslProtocols,
_sslAuthenticationOptions.EncryptionPolicy, _sslAuthenticationOptions.IsServer);
_credentialsHandle = AcquireCredentialsHandle(_sslAuthenticationOptions);
thumbPrint = guessedThumbPrint;
}

return cachedCred;
}

private static SafeFreeCredentials AcquireCredentialsHandle(SslAuthenticationOptions sslAuthenticationOptions)
{
SafeFreeCredentials cred = SslStreamPal.AcquireCredentialsHandle(sslAuthenticationOptions.CertificateContext, sslAuthenticationOptions.EnabledSslProtocols,
sslAuthenticationOptions.EncryptionPolicy, sslAuthenticationOptions.IsServer);

if (sslAuthenticationOptions.CertificateContext != null)
{
//
// Since the SafeFreeCredentials can be cached and reused, it may happen on long running processes that some cert on
// the chain expires and all subsequent connections would send expired intermediate certificates. Find the earliest
// NotAfter timestamp on the chain and use it as expiration timestamp for the credentials.
// This provides an opportunity to recreate the credentials with an alternative (and still valid)
// certificate chain.
//
SslStreamCertificateContext certificateContext = sslAuthenticationOptions.CertificateContext;
cred._expiry = GetExpiryTimestamp(certificateContext);

if (cred._expiry < DateTime.UtcNow)
{
//
// The CertificateContext from auth options is recreated just before creating the SafeFreeCredentials. However, in case when
// it was provided by the user code, it may still contain the (now expired) certificate chain. Such expiration timestamp would
// effectively disable caching as it would lead to creating new credentials for each connection. We attempt to recover by creating
// a temporary certificate context (which builds a new chain with hopefully more recent chain).
//
certificateContext = SslStreamCertificateContext.Create(
certificateContext.Certificate,
new X509Certificate2Collection(certificateContext.IntermediateCertificates),
trust: certificateContext.Trust);

cred._expiry = GetExpiryTimestamp(certificateContext);
}

static DateTime GetExpiryTimestamp(SslStreamCertificateContext certificateContext)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can possibly also add it to SslStreamCertificateContext itself. It may be used multiple times but I guess the difference would be probably very small.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I considered it, but I couldn't think of other uses elsewhere. We can always move it later when we need it.

{
DateTime expiry = certificateContext.Certificate.NotAfter;

foreach (X509Certificate2 cert in certificateContext.IntermediateCertificates)
{
if (cert.NotAfter < expiry)
{
expiry = cert.NotAfter;
}
}

return expiry.ToUniversalTime();
}
}

return cred;
}

//
internal ProtocolToken NextMessage(ReadOnlySpan<byte> incomingBuffer)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,9 @@ public bool Equals(SslCredKey other)

//SafeCredentialReference? cached;
SafeFreeCredentials? credentials = GetCachedCredential(key);
if (credentials == null || credentials.IsClosed || credentials.IsInvalid)
if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < DateTime.UtcNow)
{
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Not found or invalid, Current Cache Coun = {s_cachedCreds.Count}");
if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(null, $"Not found or invalid, Current Cache Count = {s_cachedCreds.Count}");
return null;
}

Expand Down Expand Up @@ -169,12 +169,13 @@ internal static void CacheCredential(SafeFreeCredentials creds, byte[]? thumbPri

SafeFreeCredentials? credentials = GetCachedCredential(key);

if (credentials == null || credentials.IsClosed || credentials.IsInvalid)
DateTime utcNow = DateTime.UtcNow;
if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < utcNow)
{
lock (s_cachedCreds)
{
credentials = GetCachedCredential(key);
if (credentials == null || credentials.IsClosed || credentials.IsInvalid)
if (credentials == null || credentials.IsClosed || credentials.IsInvalid || credentials.Expiry < utcNow)
{
SafeCredentialReference? cached = SafeCredentialReference.CreateReference(creds);

Expand Down