From 0348d3c0b50c239cf640876fa0fa40d6e8aaf287 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 4 Apr 2024 18:58:21 +0200 Subject: [PATCH 1/5] Fix SslStreamCertificateContext OCSP test failures --- .../SslStreamCertificateContext.Linux.cs | 119 +++++++++++------- ...lStreamCertificateContextOcspLinuxTests.cs | 40 +++--- 2 files changed, 97 insertions(+), 62 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index 82ba564cc0a66..2c1a3cbef9c58 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -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; @@ -115,6 +116,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran if (task.IsCompletedSuccessfully) { + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"Got OCSP response."); return task.Result; } } @@ -122,6 +124,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran { } + if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "No OCSP response available."); return null; } @@ -136,11 +139,13 @@ 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. @@ -171,6 +176,7 @@ 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(pending); } @@ -208,14 +214,15 @@ 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(pending); } - private async Task FetchOcspAsync() + private Task FetchOcspAsync() { Debug.Assert(_rootCertificate != null); X509Certificate2? caCert = _privateIntermediateCertificates.Length > 0 ? _privateIntermediateCertificates[0] : _rootCertificate; @@ -235,7 +242,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran if (subject == 0 || issuer == 0) { _staplingForbidden = true; - return null; + return Task.FromResult(null); } IntPtr[] issuerHandles = ArrayPool.Shared.Rent(_privateIntermediateCertificates.Length + 1); @@ -245,64 +252,82 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran } issuerHandles[_privateIntermediateCertificates.Length] = _rootCertificate.Handle; - using (SafeOcspRequestHandle ocspRequest = Interop.Crypto.X509BuildOcspRequest(subject, issuer)) - { - byte[] rentedBytes = ArrayPool.Shared.Rent(Interop.Crypto.GetOcspRequestDerSize(ocspRequest)); - int encodingSize = Interop.Crypto.EncodeOcspRequest(ocspRequest, rentedBytes); - ArraySegment encoded = new ArraySegment(rentedBytes, 0, encodingSize); + TaskCompletionSource completionSource = new TaskCompletionSource(); - ArraySegment rentedChars = UrlBase64Encoding.RentEncode(encoded); - byte[]? ret = null; + _pendingDownload = completionSource.Task; + FetchOcspAsyncCore(completionSource); + return completionSource.Task; - for (int i = 0; i < _ocspUrls.Count; i++) + async void FetchOcspAsyncCore(TaskCompletionSource 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.Shared.Rent(Interop.Crypto.GetOcspRequestDerSize(ocspRequest)); + int encodingSize = Interop.Crypto.EncodeOcspRequest(ocspRequest, rentedBytes); + ArraySegment encoded = new ArraySegment(rentedBytes, 0, encodingSize); + + ArraySegment rentedChars = UrlBase64Encoding.RentEncode(encoded); + byte[]? ret = null; - if (ret is not 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.Shared.Return(issuerHandles); + ArrayPool.Shared.Return(rentedBytes); + ArrayPool.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.Shared.Return(issuerHandles); - ArrayPool.Shared.Return(rentedBytes); - ArrayPool.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; } } diff --git a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs index 5e31aafc5cce4..b652a535791f7 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs @@ -12,12 +12,24 @@ using System.Net.Security; using Xunit; +using Xunit.Abstractions; +using TestUtilities; namespace System.Net.Security.Tests; [PlatformSpecific(TestPlatforms.Linux)] -public class SslStreamCertificateContextOcspLinuxTests +public class SslStreamCertificateContextOcspLinuxTests : IDisposable { + TestEventListener _listener; + + public SslStreamCertificateContextOcspLinuxTests(ITestOutputHelper output) => _listener = new TestEventListener(output, new[] { "Private.InternalDiagnostics.System.Net.Security" }); + + public void Dispose() + { + _listener.Dispose(); + } + + [Fact] public async Task OfflineContext_NoFetchOcspResponse() { @@ -77,7 +89,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) => @@ -95,7 +106,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) => @@ -110,12 +120,12 @@ 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); @@ -123,7 +133,6 @@ await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/97779")] public async Task RefreshOcspResponse_AfterExpiration() { await SimpleTest(PkiOptions.OcspEverywhere, async (root, intermediate, endEntity, ctxFactory, responder) => @@ -139,13 +148,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); }); } From c5b3535314f7878753e93aed397aaa1e42aae7ed Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 4 Apr 2024 19:14:45 +0200 Subject: [PATCH 2/5] Fix code style --- .../SslStreamCertificateContext.Linux.cs | 45 +++++++++++++++---- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index 2c1a3cbef9c58..6b91066290b6f 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -116,7 +116,10 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran if (task.IsCompletedSuccessfully) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"Got OCSP response."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Got OCSP response."); + } return task.Result; } } @@ -124,7 +127,10 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran { } - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "No OCSP response available."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, "No OCSP response available."); + } return null; } @@ -139,13 +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."); + 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"); + 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. @@ -176,7 +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."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Pending download task exists."); + } return new ValueTask(pending); } @@ -214,7 +229,10 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran if (pending is null || pending.IsFaulted) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, $"Starting new OCSP download task."); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Starting new OCSP download task."); + } pending = FetchOcspAsync(); } } @@ -297,7 +315,10 @@ async void FetchOcspAsyncCore(TaskCompletionSource completionSource) _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}"); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Received {ret.Length} B OCSP response, Expiration: {_ocspExpiration}, Next refresh: {_nextDownload}"); + } break; } } @@ -317,7 +338,10 @@ async void FetchOcspAsyncCore(TaskCompletionSource completionSource) // 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}"); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"OCSP response fetch failed, backing off, Next refresh = {_nextDownload}"); + } } _pendingDownload = null; @@ -325,7 +349,10 @@ async void FetchOcspAsyncCore(TaskCompletionSource completionSource) } catch (Exception ex) { - if (NetEventSource.Log.IsEnabled()) NetEventSource.Error(this, $"OCSP refresh failed: {ex}"); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(this, $"OCSP refresh failed: {ex}"); + } completionSource.SetException(ex); } } From 87b34fa88d728f7fa91c1b927f6c248dc7a0ba8f Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 4 Apr 2024 19:15:56 +0200 Subject: [PATCH 3/5] Remove unwanted change --- .../SslStreamCertificateContextOcspLinuxTests.cs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs index b652a535791f7..ec2d7399b19c2 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs @@ -13,23 +13,12 @@ using Xunit; using Xunit.Abstractions; -using TestUtilities; namespace System.Net.Security.Tests; [PlatformSpecific(TestPlatforms.Linux)] public class SslStreamCertificateContextOcspLinuxTests : IDisposable { - TestEventListener _listener; - - public SslStreamCertificateContextOcspLinuxTests(ITestOutputHelper output) => _listener = new TestEventListener(output, new[] { "Private.InternalDiagnostics.System.Net.Security" }); - - public void Dispose() - { - _listener.Dispose(); - } - - [Fact] public async Task OfflineContext_NoFetchOcspResponse() { From 7eb8ce408b97328c1a9bab67912d0781671876f2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 4 Apr 2024 19:17:02 +0200 Subject: [PATCH 4/5] fixup! Fix code style --- .../UnitTests/SslStreamCertificateContextOcspLinuxTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs index ec2d7399b19c2..7a048d80a136c 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs @@ -17,7 +17,7 @@ namespace System.Net.Security.Tests; [PlatformSpecific(TestPlatforms.Linux)] -public class SslStreamCertificateContextOcspLinuxTests : IDisposable +public class SslStreamCertificateContextOcspLinuxTests { [Fact] public async Task OfflineContext_NoFetchOcspResponse() From df1af10d95457568ea5351baf6af4329c40123c0 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 4 Apr 2024 19:36:56 +0200 Subject: [PATCH 5/5] fixup! Remove unwanted change --- .../tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs index 7a048d80a136c..7d1f33a32b32f 100644 --- a/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs +++ b/src/libraries/System.Net.Security/tests/UnitTests/SslStreamCertificateContextOcspLinuxTests.cs @@ -12,7 +12,6 @@ using System.Net.Security; using Xunit; -using Xunit.Abstractions; namespace System.Net.Security.Tests;