Skip to content

Commit

Permalink
Make Linux fetch timeout behavior for chain building more consistent …
Browse files Browse the repository at this point in the history 
…with Windows

Change the UrlRetrievalTimeout behavior on Linux to match Windows'
existing behavior of using a per-operation timeout rather than cumulative.

Windows 8.1 and Windows 10 seem to have different upper limits for the UrlRetrievalTimeout.  Linux matches the Windows 10 version (which is lower: 1 minute).
  • Loading branch information
@vcsjones
vcsjones authored Aug 13, 2020
1 parent 196fd18 commit a24db1c
Show file tree
Hide file tree
Showing 7 changed files with 368 additions and 38 deletions.
30 changes: 30 additions & 0 deletions ...braries/Common/tests/System/Security/Cryptography/X509Certificates/RevocationResponder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ private readonly Dictionary<string, CertificateAuthority> _crlPaths

public bool RespondEmpty { get; set; }

public TimeSpan ResponseDelay { get; set; }
public DelayedActionsFlag DelayedActions { get; set; }

private RevocationResponder(HttpListener listener, string uriPrefix)
{
_listener = listener;
Expand Down Expand Up @@ -160,6 +163,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)

if (_aiaPaths.TryGetValue(url, out authority))
{
if (DelayedActions.HasFlag(DelayedActionsFlag.Aia))
{
Trace($"Delaying response by {ResponseDelay}.");
Thread.Sleep(ResponseDelay);
}

byte[] certData = RespondEmpty ? Array.Empty<byte>() : authority.GetCertData();

responded = true;
Expand All @@ -172,6 +181,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)

if (_crlPaths.TryGetValue(url, out authority))
{
if (DelayedActions.HasFlag(DelayedActionsFlag.Crl))
{
Trace($"Delaying response by {ResponseDelay}.");
Thread.Sleep(ResponseDelay);
}

byte[] crl = RespondEmpty ? Array.Empty<byte>() : authority.GetCrl();

responded = true;
Expand Down Expand Up @@ -211,6 +226,12 @@ private void HandleRequest(HttpListenerContext context, ref bool responded)

byte[] ocspResponse = RespondEmpty ? Array.Empty<byte>() : authority.BuildOcspResponse(certId, nonce);

if (DelayedActions.HasFlag(DelayedActionsFlag.Ocsp))
{
Trace($"Delaying response by {ResponseDelay}.");
Thread.Sleep(ResponseDelay);
}

responded = true;
context.Response.StatusCode = 200;
context.Response.StatusDescription = "OK";
Expand Down Expand Up @@ -352,5 +373,14 @@ private static void Trace(string trace)
Console.WriteLine(trace);
}
}

internal enum DelayedActionsFlag : byte
{
None = 0,
Ocsp = 0b1,
Crl = 0b10,
Aia = 0b100,
All = 0b11111111
}
}
}
21 changes: 9 additions & 12 deletions ...ography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CertificateAssetDownloader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ internal static class CertificateAssetDownloader
{
private static readonly Func<string, CancellationToken, byte[]>? s_downloadBytes = CreateDownloadBytesFunc();

internal static X509Certificate2? DownloadCertificate(string uri, ref TimeSpan remainingDownloadTime)
internal static X509Certificate2? DownloadCertificate(string uri, TimeSpan downloadTimeout)
{
byte[]? data = DownloadAsset(uri, ref remainingDownloadTime);
byte[]? data = DownloadAsset(uri, downloadTimeout);

if (data == null || data.Length == 0)
{
Expand All @@ -39,9 +39,9 @@ internal static class CertificateAssetDownloader
}
}

internal static SafeX509CrlHandle? DownloadCrl(string uri, ref TimeSpan remainingDownloadTime)
internal static SafeX509CrlHandle? DownloadCrl(string uri, TimeSpan downloadTimeout)
{
byte[]? data = DownloadAsset(uri, ref remainingDownloadTime);
byte[]? data = DownloadAsset(uri, downloadTimeout);

if (data == null)
{
Expand Down Expand Up @@ -77,9 +77,9 @@ internal static class CertificateAssetDownloader
return null;
}

internal static SafeOcspResponseHandle? DownloadOcspGet(string uri, ref TimeSpan remainingDownloadTime)
internal static SafeOcspResponseHandle? DownloadOcspGet(string uri, TimeSpan downloadTimeout)
{
byte[]? data = DownloadAsset(uri, ref remainingDownloadTime);
byte[]? data = DownloadAsset(uri, downloadTimeout);

if (data == null)
{
Expand All @@ -100,12 +100,11 @@ internal static class CertificateAssetDownloader
return resp;
}

private static byte[]? DownloadAsset(string uri, ref TimeSpan remainingDownloadTime)
private static byte[]? DownloadAsset(string uri, TimeSpan downloadTimeout)
{
if (s_downloadBytes != null && remainingDownloadTime > TimeSpan.Zero)
if (s_downloadBytes != null && downloadTimeout > TimeSpan.Zero)
{
long totalMillis = (long)remainingDownloadTime.TotalMilliseconds;
Stopwatch stopwatch = Stopwatch.StartNew();
long totalMillis = (long)downloadTimeout.TotalMilliseconds;
CancellationTokenSource? cts = totalMillis > int.MaxValue ? null : new CancellationTokenSource((int)totalMillis);

try
Expand All @@ -115,8 +114,6 @@ internal static class CertificateAssetDownloader
catch { }
finally
{
// TimeSpan.Zero isn't a worrisome value on the subtraction, it only means "no limit" on the original input.
remainingDownloadTime -= stopwatch.Elapsed;
cts?.Dispose();
}
}
Expand Down
17 changes: 13 additions & 4 deletions ...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/ChainPal.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace Internal.Cryptography.Pal
{
internal sealed partial class ChainPal
{
private static readonly TimeSpan s_maxUrlRetrievalTimeout = TimeSpan.FromMinutes(1);

public static IChainPal FromHandle(IntPtr chainContext)
{
throw new PlatformNotSupportedException();
Expand All @@ -35,10 +37,17 @@ public static IChainPal BuildChain(
TimeSpan timeout,
bool disableAia)
{
// An input value of 0 on the timeout is "take all the time you need".
if (timeout == TimeSpan.Zero)
{
timeout = TimeSpan.MaxValue;
// An input value of 0 on the timeout is treated as 15 seconds, to match Windows.
timeout = TimeSpan.FromSeconds(15);
}
else if (timeout > s_maxUrlRetrievalTimeout || timeout < TimeSpan.Zero)
{
// Windows has a max timeout of 1 minute, so we'll match. Windows also treats
// the timeout as unsigned, so a negative value gets treated as a large positive
// value that is also clamped.
timeout = s_maxUrlRetrievalTimeout;
}

// Let Unspecified mean Local, so only convert if the source was UTC.
Expand All @@ -55,14 +64,14 @@ public static IChainPal BuildChain(
{
}

TimeSpan remainingDownloadTime = timeout;
TimeSpan downloadTimeout = timeout;

OpenSslX509ChainProcessor chainPal = OpenSslX509ChainProcessor.InitiateChain(
((OpenSslX509CertificateReader)cert).SafeHandle,
customTrustStore,
trustMode,
verificationTime,
remainingDownloadTime);
downloadTimeout);

Interop.Crypto.X509VerifyStatusCode status = chainPal.FindFirstChain(extraStore);

Expand Down
13 changes: 6 additions & 7 deletions ...tem.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Unix/CrlCache.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static void AddCrlForCertificate(
SafeX509StoreHandle store,
X509RevocationMode revocationMode,
DateTime verificationTime,
ref TimeSpan remainingDownloadTime)
TimeSpan downloadTimeout)
{
// In Offline mode, accept any cached CRL we have.
// "CRL is Expired" is a better match for Offline than "Could not find CRL"
Expand All @@ -59,14 +59,13 @@ public static void AddCrlForCertificate(
return;
}

// Don't do any work if we're over limit or prohibited from fetching new CRLs
if (remainingDownloadTime <= TimeSpan.Zero ||
revocationMode != X509RevocationMode.Online)
// Don't do any work if we're prohibited from fetching new CRLs
if (revocationMode != X509RevocationMode.Online)
{
return;
}

DownloadAndAddCrl(url, crlFileName, store, ref remainingDownloadTime);
DownloadAndAddCrl(url, crlFileName, store, downloadTimeout);
}

private static bool AddCachedCrl(string crlFileName, SafeX509StoreHandle store, DateTime verificationTime)
Expand Down Expand Up @@ -156,11 +155,11 @@ private static void DownloadAndAddCrl(
string url,
string crlFileName,
SafeX509StoreHandle store,
ref TimeSpan remainingDownloadTime)
TimeSpan downloadTimeout)
{
// X509_STORE_add_crl will increase the refcount on the CRL object, so we should still
// dispose our copy.
using (SafeX509CrlHandle? crl = CertificateAssetDownloader.DownloadCrl(url, ref remainingDownloadTime))
using (SafeX509CrlHandle? crl = CertificateAssetDownloader.DownloadCrl(url, downloadTimeout))
{
// null is a valid return (e.g. no remainingDownloadTime)
if (crl != null && !crl.IsInvalid)
Expand Down
24 changes: 9 additions & 15 deletions ...tography.X509Certificates/src/Internal/Cryptography/Pal.Unix/OpenSslX509ChainProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal sealed class OpenSslX509ChainProcessor : IChainPal
private readonly SafeX509StackHandle _untrustedLookup;
private readonly SafeX509StoreCtxHandle _storeCtx;
private readonly DateTime _verificationTime;
private TimeSpan _remainingDownloadTime;
private readonly TimeSpan _downloadTimeout;
private WorkingChain? _workingChain;

private OpenSslX509ChainProcessor(
Expand All @@ -52,14 +52,14 @@ private OpenSslX509ChainProcessor(
SafeX509StackHandle untrusted,
SafeX509StoreCtxHandle storeCtx,
DateTime verificationTime,
TimeSpan remainingDownloadTime)
TimeSpan downloadTimeout)
{
_leafHandle = leafHandle;
_store = store;
_untrustedLookup = untrusted;
_storeCtx = storeCtx;
_verificationTime = verificationTime;
_remainingDownloadTime = remainingDownloadTime;
_downloadTimeout = downloadTimeout;
}

public void Dispose()
Expand Down Expand Up @@ -236,7 +236,7 @@ internal Interop.Crypto.X509VerifyStatusCode FindChainViaAia(

X509Certificate2? downloaded = DownloadCertificate(
authorityInformationAccess,
ref _remainingDownloadTime);
_downloadTimeout);

// The AIA record is contained in a public structure, so no need to clear it.
CryptoPool.Return(authorityInformationAccess.Array!, clearSize: 0);
Expand Down Expand Up @@ -366,7 +366,7 @@ internal void ProcessRevocation(
_store,
revocationMode,
_verificationTime,
ref _remainingDownloadTime);
_downloadTimeout);
}
}
}
Expand Down Expand Up @@ -655,7 +655,7 @@ private Interop.Crypto.X509VerifyStatusCode CheckOcsp(
return status;
}

if (revocationMode != X509RevocationMode.Online || _remainingDownloadTime <= TimeSpan.Zero)
if (revocationMode != X509RevocationMode.Online)
{
return Interop.Crypto.X509VerifyStatusCode.X509_V_ERR_UNABLE_TO_GET_CRL;
}
Expand Down Expand Up @@ -690,7 +690,7 @@ private Interop.Crypto.X509VerifyStatusCode CheckOcsp(
//
// So, for now, only try GET.
SafeOcspResponseHandle? resp =
CertificateAssetDownloader.DownloadOcspGet(requestUrl, ref _remainingDownloadTime);
CertificateAssetDownloader.DownloadOcspGet(requestUrl, _downloadTimeout);

using (resp)
{
Expand Down Expand Up @@ -1072,22 +1072,16 @@ private static X509ChainStatusFlags MapVerifyErrorToChainStatus(Interop.Crypto.X

private static X509Certificate2? DownloadCertificate(
ReadOnlyMemory<byte> authorityInformationAccess,
ref TimeSpan remainingDownloadTime)
TimeSpan downloadTimeout)
{
// Don't do any work if we're over limit.
if (remainingDownloadTime <= TimeSpan.Zero)
{
return null;
}

string? uri = FindHttpAiaRecord(authorityInformationAccess, Oids.CertificateAuthorityIssuers);

if (uri == null)
{
return null;
}

return CertificateAssetDownloader.DownloadCertificate(uri, ref remainingDownloadTime);
return CertificateAssetDownloader.DownloadCertificate(uri, downloadTimeout);
}

private static string? GetOcspEndpoint(SafeX509Handle cert)
Expand Down
Loading

0 comments on commit a24db1c

Please sign in to comment.