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

Fix SslStreamCertificateContext OCSP test failures #100644

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Fix SslStreamCertificateContext OCSP test failures #100644

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0348d3c
Fix SslStreamCertificateContext OCSP test failures
rzikm Apr 4, 2024
c5b3535
Fix code style
rzikm Apr 4, 2024
87b34fa
Remove unwanted change
rzikm Apr 4, 2024
7eb8ce4
fixup! Fix code style
rzikm Apr 4, 2024
df1af10
fixup! Remove unwanted change
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
146 changes: 99 additions & 47 deletions ...ibraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Security.Cryptography;
using System.Security.Cryptography.X509Certificates;
using System.Text;
using System.Net;
using System.Threading.Tasks;
using Microsoft.Win32.SafeHandles;

Expand Down Expand Up @@ -115,13 +116,21 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran

if (task.IsCompletedSuccessfully)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"Got OCSP response.");
}
return task.Result;
}
}
catch
{
}

if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, "No OCSP response available.");
}
return null;
}

Expand All @@ -136,11 +145,19 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran

if (now > _ocspExpiration)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, "Cached OCSP response expired, fetching fresh staple.");
}
return DownloadOcspAsync();
}

if (now > _nextDownload)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, "Starting async refresh of OCSP staple");
}
// Calling DownloadOcsp will activate a Task to initiate
// in the background. Further calls will attach to the
// same Task if it's still running.
Expand Down Expand Up @@ -171,6 +188,10 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran

if (pending is not null && !pending.IsFaulted)
{
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"Pending download task exists.");
}
return new ValueTask<byte[]?>(pending);
}

Expand Down Expand Up @@ -208,14 +229,18 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran

if (pending is null || pending.IsFaulted)
{
_pendingDownload = pending = FetchOcspAsync();
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"Starting new OCSP download task.");
}
pending = FetchOcspAsync();
}
}

return new ValueTask<byte[]?>(pending);
}

private async Task<byte[]?> FetchOcspAsync()
private Task<byte[]?> FetchOcspAsync()
{
Debug.Assert(_rootCertificate != null);
X509Certificate2? caCert = _privateIntermediateCertificates.Length > 0 ? _privateIntermediateCertificates[0] : _rootCertificate;
Expand All @@ -235,7 +260,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
if (subject == 0 || issuer == 0)
{
_staplingForbidden = true;
return null;
return Task.FromResult<byte[]?>(null);
}

IntPtr[] issuerHandles = ArrayPool<IntPtr>.Shared.Rent(_privateIntermediateCertificates.Length + 1);
Expand All @@ -245,64 +270,91 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran
}
issuerHandles[_privateIntermediateCertificates.Length] = _rootCertificate.Handle;

using (SafeOcspRequestHandle ocspRequest = Interop.Crypto.X509BuildOcspRequest(subject, issuer))
{
byte[] rentedBytes = ArrayPool<byte>.Shared.Rent(Interop.Crypto.GetOcspRequestDerSize(ocspRequest));
int encodingSize = Interop.Crypto.EncodeOcspRequest(ocspRequest, rentedBytes);
ArraySegment<byte> encoded = new ArraySegment<byte>(rentedBytes, 0, encodingSize);
TaskCompletionSource<byte[]?> completionSource = new TaskCompletionSource<byte[]?>();

ArraySegment<char> rentedChars = UrlBase64Encoding.RentEncode(encoded);
byte[]? ret = null;
_pendingDownload = completionSource.Task;
FetchOcspAsyncCore(completionSource);
return completionSource.Task;
Comment on lines +273 to +277
Copy link
Member

Choose a reason for hiding this comment

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

Does anything bad happen if something starts waiting on _pendingDownload while the TCS's task is still PendingActivation?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't think of anything bad happening, if the TCS wasn't completed yet, then the behavior should be equivalent to waiting for a regular Task returned from an async method (rest of the calling async method being as continuation etc.)


for (int i = 0; i < _ocspUrls.Count; i++)
async void FetchOcspAsyncCore(TaskCompletionSource<byte[]?> completionSource)
{
try
{
string url = MakeUrl(_ocspUrls[i], rentedChars);
ret = await System.Net.Http.X509ResourceClient.DownloadAssetAsync(url, TimeSpan.MaxValue).ConfigureAwait(false);
using SafeOcspRequestHandle ocspRequest = Interop.Crypto.X509BuildOcspRequest(subject, issuer);
byte[] rentedBytes = ArrayPool<byte>.Shared.Rent(Interop.Crypto.GetOcspRequestDerSize(ocspRequest));
int encodingSize = Interop.Crypto.EncodeOcspRequest(ocspRequest, rentedBytes);
ArraySegment<byte> encoded = new ArraySegment<byte>(rentedBytes, 0, encodingSize);

if (ret is not null)
ArraySegment<char> rentedChars = UrlBase64Encoding.RentEncode(encoded);
byte[]? ret = null;

for (int i = 0; i < _ocspUrls.Count; i++)
{
if (!Interop.Crypto.X509DecodeOcspToExpiration(ret, ocspRequest, subject, issuerHandles.AsSpan(0, _privateIntermediateCertificates.Length + 1), out DateTimeOffset expiration))
{
ret = null;
continue;
}
string url = MakeUrl(_ocspUrls[i], rentedChars);
ret = await System.Net.Http.X509ResourceClient.DownloadAssetAsync(url, TimeSpan.MaxValue).ConfigureAwait(false);

// Swap the working URL in as the first one we'll try next time.
if (i != 0)
if (ret is not null)
{
string tmp = _ocspUrls[0];
_ocspUrls[0] = _ocspUrls[i];
_ocspUrls[i] = tmp;
if (!Interop.Crypto.X509DecodeOcspToExpiration(ret, ocspRequest, subject, issuerHandles.AsSpan(0, _privateIntermediateCertificates.Length + 1), out DateTimeOffset expiration))
{
ret = null;
continue;
}

// Swap the working URL in as the first one we'll try next time.
if (i != 0)
{
string tmp = _ocspUrls[0];
_ocspUrls[0] = _ocspUrls[i];
_ocspUrls[i] = tmp;
}

DateTimeOffset nextCheckA = DateTimeOffset.UtcNow.Add(DefaultOcspRefreshInterval);
DateTimeOffset nextCheckB = expiration.Subtract(MinRefreshBeforeExpirationInterval);

_ocspResponse = ret;
_ocspExpiration = expiration;
_nextDownload = nextCheckA < nextCheckB ? nextCheckA : nextCheckB;
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"Received {ret.Length} B OCSP response, Expiration: {_ocspExpiration}, Next refresh: {_nextDownload}");
}
break;
}
}

DateTimeOffset nextCheckA = DateTimeOffset.UtcNow.Add(DefaultOcspRefreshInterval);
DateTimeOffset nextCheckB = expiration.Subtract(MinRefreshBeforeExpirationInterval);
issuerHandles.AsSpan().Clear();
ArrayPool<IntPtr>.Shared.Return(issuerHandles);
ArrayPool<byte>.Shared.Return(rentedBytes);
ArrayPool<char>.Shared.Return(rentedChars.Array!);
GC.KeepAlive(TargetCertificate);
GC.KeepAlive(_privateIntermediateCertificates);
GC.KeepAlive(_rootCertificate);
GC.KeepAlive(caCert);

_ocspResponse = ret;
_ocspExpiration = expiration;
_nextDownload = nextCheckA < nextCheckB ? nextCheckA : nextCheckB;
break;
if (ret == null)
{
// All download attempts failed, don't try again for 5 seconds.
// This backoff will be applied only if the OCSP staple is not expired.
// If it is expired, we will force-refresh it during next GetOcspResponseAsync call.
_nextDownload = DateTimeOffset.UtcNow.Add(RefreshAfterFailureBackOffInterval);
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Info(this, $"OCSP response fetch failed, backing off, Next refresh = {_nextDownload}");
}
}
}

issuerHandles.AsSpan().Clear();
ArrayPool<IntPtr>.Shared.Return(issuerHandles);
ArrayPool<byte>.Shared.Return(rentedBytes);
ArrayPool<char>.Shared.Return(rentedChars.Array!);
GC.KeepAlive(TargetCertificate);
GC.KeepAlive(_privateIntermediateCertificates);
GC.KeepAlive(_rootCertificate);
GC.KeepAlive(caCert);

_pendingDownload = null;
if (ret == null)
_pendingDownload = null;
completionSource.SetResult(ret);
}
catch (Exception ex)
{
// All download attempts failed, don't try again for 5 seconds.
// This backoff will be applied only if the OCSP staple is not expired.
// If it is expired, we will force-refresh it during next GetOcspResponseAsync call.
_nextDownload = DateTimeOffset.UtcNow.Add(RefreshAfterFailureBackOffInterval);
if (NetEventSource.Log.IsEnabled())
{
NetEventSource.Error(this, $"OCSP refresh failed: {ex}");
}
completionSource.SetException(ex);
}
return ret;
}
}

Expand Down
26 changes: 12 additions & 14 deletions ...ibraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@ await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/97836")]
public async Task FetchOcspResponse_FirstInvalidThenValid()
{
await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity, ctxFactory, responder) =>
Expand All @@ -95,7 +94,6 @@ await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/97779")]
public async Task RefreshOcspResponse_BeforeExpiration()
{
await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity, ctxFactory, responder) =>
Expand All @@ -110,20 +108,19 @@ await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity

intermediate.RevocationExpiration = DateTimeOffset.UtcNow.AddDays(1);

// first call will dispatch a download and return the cached response, the first call after
// the pending download finishes will return the updated response
byte[] ocsp2 = ctx.GetOcspResponseNoWaiting();
Assert.Equal(ocsp, ocsp2);
// First call will dispatch a download. It most likely will return the
// previous cached response, but if the current thread gets delayed
// it may actually return the fresh OCSP staple so we won't check the result
ctx.GetOcspResponseNoWaiting();

// The download should succeed
// The pending download should eventually succeed
byte[] ocsp3 = await ctx.WaitForPendingOcspFetchAsync();
Assert.NotNull(ocsp3);
Assert.NotEqual(ocsp, ocsp3);
});
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/97779")]
public async Task RefreshOcspResponse_AfterExpiration()
{
await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity, ctxFactory, responder) =>
Expand All @@ -139,13 +136,14 @@ await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity

intermediate.RevocationExpiration = DateTimeOffset.UtcNow.AddDays(1);

// The cached OCSP is expired, so the first call will dispatch a download and return the cached response,
byte[] ocsp = ctx.GetOcspResponseNoWaiting();
Assert.Null(ocsp);
// The cached OCSP is expired, so the first call will dispatch a download.
// It most likely will return null, but if the current thread gets delayed
// it may actually return the fresh OCSP staple so we won't check the result
ctx.GetOcspResponseNoWaiting();

// The download should succeed
byte[] ocsp2 = await ctx.WaitForPendingOcspFetchAsync();
Assert.NotNull(ocsp2);
// The pending download should eventually succeed
byte[] ocsp = await ctx.WaitForPendingOcspFetchAsync();
Assert.NotNull(ocsp);
});
}

Expand Down
Loading