From bb692edeaf7ff4e2a7c101015597d94a0793d2fc Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 10 Jun 2024 15:03:36 +0200 Subject: [PATCH 01/15] Containers: insecure registries: allow https (ignore cert errors), and accept config from envvar. --- .../ContainerHelpers.cs | 24 ------- .../FallbackToHttpMessageHandler.cs | 54 ++++++++++++++++ .../LocalDaemons/DockerCli.cs | 4 +- .../Registry/DefaultRegistryAPI.cs | 62 ++++++++++++------- .../Registry/Registry.cs | 28 ++++++++- .../DockerRegistryManager.cs | 12 ++-- .../RegistryTests.cs | 12 ++-- 7 files changed, 135 insertions(+), 61 deletions(-) create mode 100644 src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs diff --git a/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs b/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs index c27d1ea25874..2635de9cc818 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs @@ -144,30 +144,6 @@ internal static bool IsValidImageTag(string imageTag) return ReferenceParser.anchoredTagRegexp.IsMatch(imageTag); } - - /// - /// Given an already-validated registry domain, this is our hueristic to determine what HTTP protocol should be used to interact with it. - /// If the domain is localhost, we default to HTTP. Otherwise, we check the Docker config to see if the registry is marked as insecure. - /// This is primarily for testing - in the real world almost all usage should be through HTTPS! - /// - internal static Uri TryExpandRegistryToUri(string alreadyValidatedDomain) - { - string prefix = "https"; - if (alreadyValidatedDomain.StartsWith("localhost", StringComparison.Ordinal)) - { - prefix = "http"; - } - - //check the docker config to see if the registry is marked as insecure - else if (DockerCli.IsInsecureRegistry(alreadyValidatedDomain)) - { - prefix = "http"; - } - - - return new Uri($"{prefix}://{alreadyValidatedDomain}"); - } - /// /// Ensures a given environment variable is valid. /// diff --git a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs new file mode 100644 index 000000000000..26bf2815ceb0 --- /dev/null +++ b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs @@ -0,0 +1,54 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net; +using Microsoft.Extensions.Logging; +using Microsoft.NET.Build.Containers.Resources; + +namespace Microsoft.NET.Build.Containers; + +/// +/// A delegating handler that falls back from https to http for a specific hostname. +/// +internal sealed partial class FallbackToHttpMessageHandler : DelegatingHandler +{ + private readonly string _host; + private readonly int _port; + private bool _fallbackToHttp; + + public FallbackToHttpMessageHandler(string host, int port, HttpMessageHandler innerHandler, ILogger logger) : base(innerHandler) + { + _host = host; + _port = port; + } + + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + if (request.RequestUri is null) + { + throw new ArgumentException(Resource.GetString(nameof(Strings.NoRequestUriSpecified)), nameof(request)); + } + + bool canFallback = request.RequestUri.Host == _host && request.RequestUri.Port == _port && request.RequestUri.Scheme == "https"; + bool canRetry = true; + do + { + try + { + if (canFallback && _fallbackToHttp) + { + var uriBuilder = new UriBuilder(request.RequestUri); + uriBuilder.Scheme = "http"; + request.RequestUri = uriBuilder.Uri; + canRetry = false; + } + + return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + } + catch (HttpRequestException re) when (canFallback && canRetry && re.HttpRequestError == HttpRequestError.SecureConnectionError) + { + _fallbackToHttp = true; + } + } while (true); + } +} diff --git a/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs b/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs index fb0e09b87d5a..367eb06a6d02 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/LocalDaemons/DockerCli.cs @@ -232,7 +232,7 @@ public static bool IsInsecureRegistry(string registryDomain) { if (property.Value.ValueKind == JsonValueKind.Object && property.Value.TryGetProperty("Secure", out var secure) && !secure.GetBoolean()) { - if (property.Name.Equals(registryDomain, StringComparison.Ordinal)) + if (property.Name.Equals(registryDomain, StringComparison.OrdinalIgnoreCase)) { return true; } @@ -248,7 +248,7 @@ public static bool IsInsecureRegistry(string registryDomain) { if (property.Value.ValueKind == JsonValueKind.Object && property.Value.TryGetProperty("Insecure", out var insecure) && insecure.GetBoolean()) { - if (property.Name.Equals(registryDomain, StringComparison.Ordinal)) + if (property.Name.Equals(registryDomain, StringComparison.OrdinalIgnoreCase)) { return true; } diff --git a/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs b/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs index 885879b29b07..1915fac419eb 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs @@ -22,12 +22,11 @@ internal class DefaultRegistryAPI : IRegistryAPI // Making this a round 30 for convenience. private static TimeSpan LongRequestTimeout = TimeSpan.FromMinutes(30); - internal DefaultRegistryAPI(string registryName, Uri baseUri, ILogger logger) + internal DefaultRegistryAPI(string registryName, Uri baseUri, bool isInsecureRegistry, ILogger logger) { - bool isAmazonECRRegistry = baseUri.IsAmazonECRRegistry(); _baseUri = baseUri; _logger = logger; - _client = CreateClient(registryName, baseUri, logger, isAmazonECRRegistry); + _client = CreateClient(registryName, baseUri, isInsecureRegistry, logger); Manifest = new DefaultManifestOperations(_baseUri, registryName, _client, _logger); Blob = new DefaultBlobOperations(_baseUri, registryName, _client, _logger); } @@ -36,28 +35,13 @@ internal DefaultRegistryAPI(string registryName, Uri baseUri, ILogger logger) public IManifestOperations Manifest { get; } - private static HttpClient CreateClient(string registryName, Uri baseUri, ILogger logger, bool isAmazonECRRegistry = false) + private static HttpClient CreateClient(string registryName, Uri baseUri, bool isInsecureRegistry, ILogger logger) { - var innerHandler = new SocketsHttpHandler() - { - UseCookies = false, - // the rest of the HTTP stack has an very long timeout (see below) but we should still have a reasonable timeout for the initial connection - ConnectTimeout = TimeSpan.FromSeconds(30) - }; - - // Ignore certificate for https localhost repository. - if (baseUri.Host == "localhost" && baseUri.Scheme == "https") - { - innerHandler.SslOptions = new System.Net.Security.SslClientAuthenticationOptions() - { - RemoteCertificateValidationCallback = (object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) - => (sender as SslStream)?.TargetHostName == "localhost" - }; - } + HttpMessageHandler innerHandler = CreateHttpHandler(baseUri, isInsecureRegistry, logger); HttpMessageHandler clientHandler = new AuthHandshakeMessageHandler(registryName, innerHandler, logger); - if (isAmazonECRRegistry) + if (baseUri.IsAmazonECRRegistry()) { clientHandler = new AmazonECRMessageHandler(clientHandler); } @@ -71,4 +55,40 @@ private static HttpClient CreateClient(string registryName, Uri baseUri, ILogger return client; } + + private static HttpMessageHandler CreateHttpHandler(Uri baseUri, bool allowInsecure, ILogger logger) + { + var socketsHttpHandler = new SocketsHttpHandler() + { + UseCookies = false, + // the rest of the HTTP stack has an very long timeout (see below) but we should still have a reasonable timeout for the initial connection + ConnectTimeout = TimeSpan.FromSeconds(30) + }; + + if (!allowInsecure) + { + return socketsHttpHandler; + } + + socketsHttpHandler.SslOptions = new System.Net.Security.SslClientAuthenticationOptions() + { + RemoteCertificateValidationCallback = (object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) => + { + if (sslPolicyErrors == SslPolicyErrors.None) + { + return true; + } + + // Ignore certificate errors for the hostname. + if ((sender as SslStream)?.TargetHostName == baseUri.Host) + { + return true; + } + + return false; + } + }; + + return new FallbackToHttpMessageHandler(baseUri.Host, baseUri.Port, socketsHttpHandler, logger); + } } diff --git a/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs b/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs index 3d783698710e..165f4960630d 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs @@ -71,7 +71,7 @@ internal sealed class Registry public string RegistryName { get; } internal Registry(string registryName, ILogger logger, IRegistryAPI? registryAPI = null, RegistrySettings? settings = null) : - this(ContainerHelpers.TryExpandRegistryToUri(registryName), logger, registryAPI, settings) + this(new Uri($"https://{registryName}"), logger, registryAPI, settings) { } internal Registry(Uri baseUri, ILogger logger, IRegistryAPI? registryAPI = null, RegistrySettings? settings = null) @@ -87,7 +87,8 @@ internal Registry(Uri baseUri, ILogger logger, IRegistryAPI? registryAPI = null, _logger = logger; _settings = settings ?? new RegistrySettings(); - _registryAPI = registryAPI ?? new DefaultRegistryAPI(RegistryName, BaseUri, logger); + bool isInsecureRegistry = IsInsecureRegistry(RegistryName); + _registryAPI = registryAPI ?? new DefaultRegistryAPI(RegistryName, BaseUri, isInsecureRegistry, logger); } private static string DeriveRegistryName(Uri baseUri) @@ -105,6 +106,29 @@ private static string DeriveRegistryName(Uri baseUri) } } + private static bool IsInsecureRegistry(string registryName) + { + // Allow insecure access to 'localhost'. + if (registryName.StartsWith("localhost:", StringComparison.OrdinalIgnoreCase) || + registryName.Equals("localhost:", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + // SDK_CONTAINER_INSECURE_REGISTRIES is a semicolon separated list of insecure registry names. + string? insecureRegistriesEnv = Environment.GetEnvironmentVariable("SDK_CONTAINER_INSECURE_REGISTRIES"); + if (insecureRegistriesEnv is not null) + { + string[] insecureRegistries = insecureRegistriesEnv.Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + if (Array.Exists(insecureRegistries, registry => registryName.Equals(registry, StringComparison.OrdinalIgnoreCase))) + { + return true; + } + } + + return DockerCli.IsInsecureRegistry(registryName); + } + public Uri BaseUri { get; } /// diff --git a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs index 547cf83cb6e6..a736e3415699 100644 --- a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs +++ b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs @@ -119,15 +119,15 @@ public static void ShutdownDockerRegistry(ITestOutputHelper testOutput) } } - private static void EnsureRegistryLoaded(string registryBaseUri, string? containerRegistryId, ILogger logger, ITestOutputHelper testOutput) + private static void EnsureRegistryLoaded(string registryName, string? containerRegistryId, ILogger logger, ITestOutputHelper testOutput) { const int registryLoadMaxRetry = 10; const int registryLoadTimeout = 1000; //ms using HttpClient client = new(); - using HttpRequestMessage request = new(HttpMethod.Get, new Uri(ContainerHelpers.TryExpandRegistryToUri(registryBaseUri), "/v2/")); + using HttpRequestMessage request = new(HttpMethod.Get, new Uri($"https://{registryName}/v2/")); - logger.LogInformation("Checking if the registry '{registry}' is available.", registryBaseUri); + logger.LogInformation("Checking if the registry '{registry}' is available.", registryName); int attempt = 1; while (attempt <= registryLoadMaxRetry) @@ -141,14 +141,14 @@ private static void EnsureRegistryLoaded(string registryBaseUri, string? contain if (response.IsSuccessStatusCode) { - logger.LogInformation("The registry '{registry}' is available after {timeout} ms.", registryBaseUri, attempt * registryLoadTimeout); + logger.LogInformation("The registry '{registry}' is available after {timeout} ms.", registryName, attempt * registryLoadTimeout); return; } - logger.LogWarning("The registry '{registry} is not loaded after {timeout} ms. Returned status code: {statusCode}.", registryBaseUri, attempt * registryLoadTimeout, response.StatusCode); + logger.LogWarning("The registry '{registry} is not loaded after {timeout} ms. Returned status code: {statusCode}.", registryName, attempt * registryLoadTimeout, response.StatusCode); } catch (Exception ex) { - logger.LogWarning(ex, "The registry '{registry} is not loaded after {timeout} ms.", registryBaseUri, attempt * registryLoadTimeout); + logger.LogWarning(ex, "The registry '{registry} is not loaded after {timeout} ms.", registryName, attempt * registryLoadTimeout); } attempt++; } diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 6bd38995df9f..2b54480a83a0 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -77,7 +77,7 @@ public async Task RegistriesThatProvideUploadSizePrefersFullUploadWhenChunkSizeI var layerDigest = "sha256:fafafafafafafafafafafafafafafafa"; var mockLayer = new Mock(MockBehavior.Strict); var chunkSizeLessThanContentLength = 10000; - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; mockLayer .Setup(l => l.OpenBackingFile()).Returns(new MemoryStream(new byte[100000])); mockLayer @@ -112,7 +112,7 @@ public async Task RegistriesThatFailAtomicUploadFallbackToChunked() var mockLayer = new Mock(MockBehavior.Strict); var contentLength = 100000; var chunkSizeLessThanContentLength = 100000; - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; mockLayer .Setup(l => l.OpenBackingFile()).Returns(new MemoryStream(new byte[contentLength])); mockLayer @@ -147,7 +147,7 @@ public async Task ChunkedUploadCalculatesChunksCorrectly() var mockLayer = new Mock(MockBehavior.Strict); var contentLength = 1000000; var chunkSize = 100000; - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; mockLayer .Setup(l => l.OpenBackingFile()).Returns(new MemoryStream(new byte[contentLength])); mockLayer @@ -222,7 +222,7 @@ public async Task PushAsync_ForceChunkedUpload() Mock mockLayer = new(MockBehavior.Strict); int contentLength = 1000000; int chunkSize = 100000; - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; mockLayer .Setup(l => l.OpenBackingFile()).Returns(new MemoryStream(new byte[contentLength])); mockLayer @@ -323,7 +323,7 @@ public async Task CanParseRegistryDeclaredChunkSize_None() public async Task UploadBlobChunkedAsync_NormalFlow() { ILogger logger = _loggerFactory.CreateLogger(nameof(UploadBlobChunkedAsync_NormalFlow)); - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; int contentLength = 50000000; int chunkSize = 10000000; @@ -357,7 +357,7 @@ public async Task UploadBlobChunkedAsync_NormalFlow() public async Task UploadBlobChunkedAsync_Failure() { ILogger logger = _loggerFactory.CreateLogger(nameof(UploadBlobChunkedAsync_NormalFlow)); - var registryUri = ContainerHelpers.TryExpandRegistryToUri("public.ecr.aws"); + var registryUri = new Uri("https://public.ecr.aws");; int contentLength = 50000000; int chunkSize = 10000000; From 70a2773219e12c8c10b38fafdf635d1bb0317b9c Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 11 Jun 2024 13:30:43 +0200 Subject: [PATCH 02/15] Add tests. --- .../FallbackToHttpMessageHandler.cs | 4 +- .../Registry/Registry.cs | 28 +-- .../Registry/RegistrySettings.cs | 50 +++++- .../RegistryTests.cs | 160 ++++++++++++++++++ 4 files changed, 211 insertions(+), 31 deletions(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs index 26bf2815ceb0..3af2b8e34558 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs @@ -30,7 +30,7 @@ protected override async Task SendAsync(HttpRequestMessage } bool canFallback = request.RequestUri.Host == _host && request.RequestUri.Port == _port && request.RequestUri.Scheme == "https"; - bool canRetry = true; + bool canRetry = canFallback; do { try @@ -45,7 +45,7 @@ protected override async Task SendAsync(HttpRequestMessage return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); } - catch (HttpRequestException re) when (canFallback && canRetry && re.HttpRequestError == HttpRequestError.SecureConnectionError) + catch (HttpRequestException re) when (canRetry && re.HttpRequestError == HttpRequestError.SecureConnectionError) { _fallbackToHttp = true; } diff --git a/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs b/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs index 165f4960630d..e728ca54f813 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/Registry/Registry.cs @@ -86,9 +86,8 @@ internal Registry(Uri baseUri, ILogger logger, IRegistryAPI? registryAPI = null, BaseUri = baseUri; _logger = logger; - _settings = settings ?? new RegistrySettings(); - bool isInsecureRegistry = IsInsecureRegistry(RegistryName); - _registryAPI = registryAPI ?? new DefaultRegistryAPI(RegistryName, BaseUri, isInsecureRegistry, logger); + _settings = settings ?? new RegistrySettings(RegistryName); + _registryAPI = registryAPI ?? new DefaultRegistryAPI(RegistryName, BaseUri, _settings.IsInsecure, logger); } private static string DeriveRegistryName(Uri baseUri) @@ -106,29 +105,6 @@ private static string DeriveRegistryName(Uri baseUri) } } - private static bool IsInsecureRegistry(string registryName) - { - // Allow insecure access to 'localhost'. - if (registryName.StartsWith("localhost:", StringComparison.OrdinalIgnoreCase) || - registryName.Equals("localhost:", StringComparison.OrdinalIgnoreCase)) - { - return true; - } - - // SDK_CONTAINER_INSECURE_REGISTRIES is a semicolon separated list of insecure registry names. - string? insecureRegistriesEnv = Environment.GetEnvironmentVariable("SDK_CONTAINER_INSECURE_REGISTRIES"); - if (insecureRegistriesEnv is not null) - { - string[] insecureRegistries = insecureRegistriesEnv.Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); - if (Array.Exists(insecureRegistries, registry => registryName.Equals(registry, StringComparison.OrdinalIgnoreCase))) - { - return true; - } - } - - return DockerCli.IsInsecureRegistry(registryName); - } - public Uri BaseUri { get; } /// diff --git a/src/Containers/Microsoft.NET.Build.Containers/Registry/RegistrySettings.cs b/src/Containers/Microsoft.NET.Build.Containers/Registry/RegistrySettings.cs index 3ac8f41ff254..19e5b6eef6a3 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/Registry/RegistrySettings.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/Registry/RegistrySettings.cs @@ -8,6 +8,20 @@ namespace Microsoft.NET.Build.Containers; internal class RegistrySettings { + public RegistrySettings(string? registryName = null, IEnvironmentProvider? environment = null) + { + environment ??= new EnvironmentProvider(); + + ChunkedUploadSizeBytes = environment.GetEnvironmentVariableAsNullableInt(EnvVariables.ChunkedUploadSizeBytes); + ForceChunkedUpload = environment.GetEnvironmentVariableAsBool(EnvVariables.ForceChunkedUpload, defaultValue: false); + ParallelUploadEnabled = environment.GetEnvironmentVariableAsBool(EnvVariables.ParallelUploadEnabled, defaultValue: true); + + if (registryName is not null) + { + IsInsecure = IsInsecureRegistry(environment, registryName); + } + } + private const int DefaultChunkSizeBytes = 1024 * 64; private const int FiveMegs = 5_242_880; @@ -17,12 +31,12 @@ internal class RegistrySettings /// /// Our default of 64KB is very conservative, so raising this to 1MB or more can speed up layer uploads reasonably well. /// - internal int? ChunkedUploadSizeBytes { get; init; } = Env.GetEnvironmentVariableAsNullableInt(EnvVariables.ChunkedUploadSizeBytes); + internal int? ChunkedUploadSizeBytes { get; init; } /// /// Allows to force chunked upload for debugging purposes. /// - internal bool ForceChunkedUpload { get; init; } = Env.GetEnvironmentVariableAsBool(EnvVariables.ForceChunkedUpload, defaultValue: false); + internal bool ForceChunkedUpload { get; init; } /// /// Whether we should upload blobs in parallel (enabled by default, but disabled for certain registries in conjunction with the explicit support check below). @@ -30,7 +44,12 @@ internal class RegistrySettings /// /// Enabling this can swamp some registries, so this is an escape hatch. /// - internal bool ParallelUploadEnabled { get; init; } = Env.GetEnvironmentVariableAsBool(EnvVariables.ParallelUploadEnabled, defaultValue: true); + internal bool ParallelUploadEnabled { get; init; } + + /// + /// Allows ignoring https certificate errors and changing to http when the endpoint is not an https endpoint. + /// + internal bool IsInsecure { get; init; } internal struct EnvVariables { @@ -38,5 +57,30 @@ internal struct EnvVariables internal const string ForceChunkedUpload = "SDK_CONTAINER_DEBUG_REGISTRY_FORCE_CHUNKED_UPLOAD"; internal const string ParallelUploadEnabled = "SDK_CONTAINER_REGISTRY_PARALLEL_UPLOAD"; + + internal const string InsecureRegistries = "SDK_CONTAINER_INSECURE_REGISTRIES"; + } + + private static bool IsInsecureRegistry(IEnvironmentProvider environment, string registryName) + { + // Always allow insecure access to 'localhost'. + if (registryName.StartsWith("localhost:", StringComparison.OrdinalIgnoreCase) || + registryName.Equals("localhost", StringComparison.OrdinalIgnoreCase)) + { + return true; + } + + // SDK_CONTAINER_INSECURE_REGISTRIES is a semicolon separated list of insecure registry names. + string? insecureRegistriesEnv = environment.GetEnvironmentVariable(EnvVariables.InsecureRegistries); + if (insecureRegistriesEnv is not null) + { + string[] insecureRegistries = insecureRegistriesEnv.Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + if (Array.Exists(insecureRegistries, registry => registryName.Equals(registry, StringComparison.OrdinalIgnoreCase))) + { + return true; + } + } + + return DockerCli.IsInsecureRegistry(registryName); } } diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 2b54480a83a0..16e9f9847d68 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -2,8 +2,13 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Net; +using System.Net.Security; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; +using Microsoft.DotNet.Cli.Utils; using Microsoft.Extensions.Logging; using Microsoft.NET.Build.Containers.Resources; +using System.Net.Sockets; using Moq; namespace Microsoft.NET.Build.Containers.UnitTests; @@ -390,11 +395,166 @@ public async Task UploadBlobChunkedAsync_Failure() api.Verify(api => api.Blob.Upload.UploadChunkAsync(It.IsIn(absoluteUploadUri, uploadPath), It.IsAny(), It.IsAny()), Times.Exactly(1)); } + [InlineData(true, true)] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + [Theory] + public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) + { + ILogger logger = _loggerFactory.CreateLogger(nameof(InsecureRegistry)); + + // Start a dummy HTTP server that response with 200 OK. + using TcpListener listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + IPEndPoint endpoint = (listener.LocalEndpoint as IPEndPoint)!; + Uri registryUri = new Uri($"https://{endpoint.Address}:{endpoint.Port}"); + SslServerAuthenticationOptions? sslOptions = null!; + if (serverIsHttps) + { + var key = RSA.Create(2048); + var request = new CertificateRequest("CN=localhost", key, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); + X509Certificate2 serverCertificate = request.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(1)); + sslOptions = new SslServerAuthenticationOptions() + { + ServerCertificate = serverCertificate, + ClientCertificateRequired = false + }; + } + _ = Task.Run(async () => + { + while (true) + { + using TcpClient client = await listener.AcceptTcpClientAsync(); + try + { + using Stream stream = serverIsHttps ? new SslStream(client.GetStream(), leaveInnerStreamOpen: false) : client.GetStream(); + if (stream is SslStream sslStream) + { + await sslStream.AuthenticateAsServerAsync(sslOptions!, default(CancellationToken)); + } + await stream.WriteAsync("HTTP/1.0 200 OK\r\nContent-Length: 0\r\n\r\n"u8.ToArray()); + } + catch + { } + } + }); + + RegistrySettings settings = new() + { + IsInsecure = isInsecureRegistry + }; + Registry registry = new(registryUri, logger, settings: settings); + + // Make a request. + Task getManifest = registry.GetImageManifestAsync(repositoryName: "dotnet/runtime", reference: "latest", runtimeIdentifier: "linux-x64", manifestPicker: null!, cancellationToken: default!); + + if (isInsecureRegistry) + { + // Falls back to http (when serverIsHttps is false) or ignores https certificate errors (when serverIsHttps is true). + // Results in throwing: CONTAINER2003: The manifest for dotnet/runtime:latest from registry hwas an unknown type. + await Assert.ThrowsAsync(() => getManifest); + } + else + { + // Does not fall back and throws HttpRequestException. + var requestException = await Assert.ThrowsAsync(() => getManifest); + Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); + } + } + + [InlineData("localhost", null, true)] + [InlineData("localhost:5000", null, true)] + [InlineData("public.ecr.aws", null, false)] + [InlineData("public.ecr.aws", "public.ecr.aws", true)] + [InlineData("public.ecr.aws", "Public.ecr.aws", true)] // ignore case + [InlineData("public.ecr.aws", "public.ecr.aws;docker.io", true)] // multiple registries + [InlineData("public.ecr.aws", ";public.ecr.aws ; docker.io ", true)] // ignore whitespace + [InlineData("public.ecr.aws", "public.ecr.aws2;docker.io ", false)] // full name match + [Theory] + public void IsRegistryInsecure(string registryName, string? insecureRegistriesEnvvar, bool expectedInsecure) + { + var environment = new Dictionary(); + if (insecureRegistriesEnvvar is not null) + { + environment["SDK_CONTAINER_INSECURE_REGISTRIES"] = insecureRegistriesEnvvar; + } + var registrySettings = new RegistrySettings(registryName, new MockEnvironmentProvider(environment)); + + Assert.Equal(expectedInsecure, registrySettings.IsInsecure); + } private static NextChunkUploadInformation ChunkUploadSuccessful(Uri requestUri, Uri uploadUrl, int? contentLength, HttpStatusCode code = HttpStatusCode.Accepted) { return new(uploadUrl); } + private class MockEnvironmentProvider : IEnvironmentProvider + { + private readonly IDictionary _environmentVariables; + + public MockEnvironmentProvider(IDictionary environmentVariables) + { + _environmentVariables = environmentVariables; + } + + public bool GetEnvironmentVariableAsBool(string name, bool defaultValue) + { + string? str = GetEnvironmentVariable(name); + if (string.IsNullOrEmpty(str)) + { + return defaultValue; + } + + switch (str.ToLowerInvariant()) + { + case "true": + case "1": + case "yes": + return true; + case "false": + case "0": + case "no": + return false; + default: + return defaultValue; + } + } + + public string? GetEnvironmentVariable(string name) + { + string? value; + _environmentVariables.TryGetValue(name, out value); + return value; + } + + public string? GetEnvironmentVariable(string variable, EnvironmentVariableTarget target) + => GetEnvironmentVariable(variable); + + public int? GetEnvironmentVariableAsNullableInt(string variable) + { + if (GetEnvironmentVariable(variable) is string strValue && int.TryParse(strValue, out int intValue)) + { + return intValue; + } + + return null; + } + + public void SetEnvironmentVariable(string variable, string value, EnvironmentVariableTarget target) + => throw new NotImplementedException(); + + public IEnumerable ExecutableExtensions + => throw new NotImplementedException(); + + public string GetCommandPath(string commandName, params string[] extensions) + => throw new NotImplementedException(); + + public string GetCommandPathFromRootPath(string rootPath, string commandName, params string[] extensions) + => throw new NotImplementedException(); + + public string GetCommandPathFromRootPath(string rootPath, string commandName, IEnumerable extensions) + => throw new NotImplementedException(); + } } From 51b89f8c53ab0da1669fa15c19bb2861f37ed7b3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 14 Jun 2024 16:27:15 +0200 Subject: [PATCH 03/15] Fix Windows test issue. --- .../Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 16e9f9847d68..9f4c075dbe5d 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -415,6 +415,10 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) var key = RSA.Create(2048); var request = new CertificateRequest("CN=localhost", key, HashAlgorithmName.SHA256, RSASignaturePadding.Pkcs1); X509Certificate2 serverCertificate = request.CreateSelfSigned(DateTimeOffset.Now, DateTimeOffset.Now.AddYears(1)); + + // https://stackoverflow.com/questions/72096812/loading-x509certificate2-from-pem-file-results-in-no-credentials-are-available/72101855#72101855 + serverCertificate = new X509Certificate2(serverCertificate.Export(X509ContentType.Pfx)); + sslOptions = new SslServerAuthenticationOptions() { ServerCertificate = serverCertificate, From d38eddc153015f8b902272c7c211c889a69637a4 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 14 Jun 2024 18:20:12 +0200 Subject: [PATCH 04/15] Try fix tests on Windows. --- test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 9f4c075dbe5d..ded5e0abf2f6 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -437,6 +437,7 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) { await sslStream.AuthenticateAsServerAsync(sslOptions!, default(CancellationToken)); } + await stream.ReadAtLeastAsync(new byte[1], 1); // Wait for the request. await stream.WriteAsync("HTTP/1.0 200 OK\r\nContent-Length: 0\r\n\r\n"u8.ToArray()); } catch From 750d566bf8b9b69dcda9b057109926f32b98a500 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 14 Jun 2024 18:23:11 +0200 Subject: [PATCH 05/15] PR feedback. --- .../Registry/DefaultRegistryAPI.cs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs b/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs index 1915fac419eb..71b937233fe4 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/Registry/DefaultRegistryAPI.cs @@ -72,23 +72,28 @@ private static HttpMessageHandler CreateHttpHandler(Uri baseUri, bool allowInsec socketsHttpHandler.SslOptions = new System.Net.Security.SslClientAuthenticationOptions() { - RemoteCertificateValidationCallback = (object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) => - { - if (sslPolicyErrors == SslPolicyErrors.None) - { - return true; - } - - // Ignore certificate errors for the hostname. - if ((sender as SslStream)?.TargetHostName == baseUri.Host) - { - return true; - } - - return false; - } + RemoteCertificateValidationCallback = IgnoreCertificateErrorsForSpecificHost(baseUri.Host) }; return new FallbackToHttpMessageHandler(baseUri.Host, baseUri.Port, socketsHttpHandler, logger); } + + private static RemoteCertificateValidationCallback IgnoreCertificateErrorsForSpecificHost(string host) + { + return (object sender, X509Certificate? certificate, X509Chain? chain, SslPolicyErrors sslPolicyErrors) => + { + if (sslPolicyErrors == SslPolicyErrors.None) + { + return true; + } + + // Ignore certificate errors for the hostname. + if ((sender as SslStream)?.TargetHostName == host) + { + return true; + } + + return false; + }; + } } From d9a005b4ab330048caedf215cce314659c52f4a3 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 14 Jun 2024 20:57:18 +0200 Subject: [PATCH 06/15] Fix EnsureRegistryLoaded. --- .../DockerRegistryManager.cs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs index a736e3415699..f7092132c208 100644 --- a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs +++ b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryManager.cs @@ -65,7 +65,7 @@ public static async Task StartAndPopulateDockerRegistry(ITestOutputHelper testOu using var reader = new StringReader(processResult.StdOut!); s_registryContainerId = reader.ReadLine(); - EnsureRegistryLoaded(LocalRegistry, s_registryContainerId, logger, testOutput); + EnsureRegistryLoaded(new Uri($"http://{LocalRegistry}"), s_registryContainerId, logger, testOutput); foreach (string? tag in new[] { Net6ImageTag, Net7ImageTag, Net8ImageTag, Net9PreviewImageTag }) { @@ -119,15 +119,15 @@ public static void ShutdownDockerRegistry(ITestOutputHelper testOutput) } } - private static void EnsureRegistryLoaded(string registryName, string? containerRegistryId, ILogger logger, ITestOutputHelper testOutput) + private static void EnsureRegistryLoaded(Uri registryBaseUri, string? containerRegistryId, ILogger logger, ITestOutputHelper testOutput) { const int registryLoadMaxRetry = 10; const int registryLoadTimeout = 1000; //ms using HttpClient client = new(); - using HttpRequestMessage request = new(HttpMethod.Get, new Uri($"https://{registryName}/v2/")); + using HttpRequestMessage request = new(HttpMethod.Get, new Uri(registryBaseUri, "/v2/")); - logger.LogInformation("Checking if the registry '{registry}' is available.", registryName); + logger.LogInformation("Checking if the registry '{registry}' is available.", registryBaseUri); int attempt = 1; while (attempt <= registryLoadMaxRetry) @@ -141,14 +141,14 @@ private static void EnsureRegistryLoaded(string registryName, string? containerR if (response.IsSuccessStatusCode) { - logger.LogInformation("The registry '{registry}' is available after {timeout} ms.", registryName, attempt * registryLoadTimeout); + logger.LogInformation("The registry '{registry}' is available after {timeout} ms.", registryBaseUri, attempt * registryLoadTimeout); return; } - logger.LogWarning("The registry '{registry} is not loaded after {timeout} ms. Returned status code: {statusCode}.", registryName, attempt * registryLoadTimeout, response.StatusCode); + logger.LogWarning("The registry '{registry} is not loaded after {timeout} ms. Returned status code: {statusCode}.", registryBaseUri, attempt * registryLoadTimeout, response.StatusCode); } catch (Exception ex) { - logger.LogWarning(ex, "The registry '{registry} is not loaded after {timeout} ms.", registryName, attempt * registryLoadTimeout); + logger.LogWarning(ex, "The registry '{registry} is not loaded after {timeout} ms.", registryBaseUri, attempt * registryLoadTimeout); } attempt++; } From b21f93b44606f1f7dab2e1a83f88a70b37ce7499 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Fri, 14 Jun 2024 23:53:03 +0200 Subject: [PATCH 07/15] Update WriteToPrivateBasicRegistry test. --- .../DockerRegistryTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryTests.cs b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryTests.cs index 63943a768d4b..10651a56e32a 100644 --- a/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.IntegrationTests/DockerRegistryTests.cs @@ -74,7 +74,7 @@ public async Task WriteToPrivateBasicRegistry() // login to that registry ContainerCli.LoginCommand(_testOutput, "--username", "testuser", "--password", "testpassword", registryName).Execute().Should().Pass(); // push an image to that registry using username/password - Registry localAuthed = new(new Uri($"https://{registryName}"), logger, settings: new() { ParallelUploadEnabled = false, ForceChunkedUpload = true }); + Registry localAuthed = new(new Uri($"https://{registryName}"), logger, settings: new(registryName) { ParallelUploadEnabled = false, ForceChunkedUpload = true }); var ridgraphfile = ToolsetUtils.GetRuntimeGraphFilePath(); Registry mcr = new(DockerRegistryManager.BaseImageSource, logger); From b08fbeb75dce41873db70ed47ee137327cbbd408 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Sat, 15 Jun 2024 08:31:45 +0200 Subject: [PATCH 08/15] Include HttpRequestError in AuthHandshakeMessageHandler logging. --- .../AuthHandshakeMessageHandler.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs index 10ff6c46a554..fbbdc9fada47 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs @@ -365,7 +365,7 @@ protected override async Task SendAsync(HttpRequestMessage catch (HttpRequestException e) when (e.InnerException is IOException ioe && ioe.InnerException is SocketException se) { retryCount += 1; - _logger.LogInformation("Encountered a SocketException with message \"{message}\". Pausing before retry.", se.Message); + _logger.LogInformation("Encountered a HttpRequestException {error} with message \"{message}\". Pausing before retry.", e.HttpRequestError, se.Message); _logger.LogTrace("Exception details: {ex}", se); await Task.Delay(TimeSpan.FromSeconds(1.0 * Math.Pow(2, retryCount)), cancellationToken).ConfigureAwait(false); From d304ebc73a723d2fd5c933170ae6fce47918f334 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Sat, 15 Jun 2024 11:14:58 +0200 Subject: [PATCH 09/15] Allow ApplicationException. --- .../RegistryTests.cs | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index ded5e0abf2f6..7d543a0c56e6 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -463,8 +463,17 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) else { // Does not fall back and throws HttpRequestException. - var requestException = await Assert.ThrowsAsync(() => getManifest); - Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); + try + { + var requestException = await Assert.ThrowsAsync(() => getManifest); + Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); + } + catch when (!serverIsHttps) + { + // We're talking https to an http server. The server may respond in a way that triggers + // the retry handling in AuthHandshakeMessageHandler which then throws an ApplicationException. + await Assert.ThrowsAsync(() => getManifest); + } } } From b457f5e9e50c3cfd48309a99b8a3dfa1ca004b26 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Sat, 15 Jun 2024 13:10:49 +0200 Subject: [PATCH 10/15] Move comment. --- test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 7d543a0c56e6..e3fbcd8b4d34 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -462,9 +462,9 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) } else { - // Does not fall back and throws HttpRequestException. try { + // Does not fall back and throws HttpRequestException. var requestException = await Assert.ThrowsAsync(() => getManifest); Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); } From 673ad29918e67623266deacdbb693d49156c520a Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Sat, 15 Jun 2024 19:43:13 +0200 Subject: [PATCH 11/15] Use the fallback exception filter in the test. --- .../FallbackToHttpMessageHandler.cs | 38 +++++++++-- .../RegistryTests.cs | 68 +++++++++++++++---- 2 files changed, 87 insertions(+), 19 deletions(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs index 3af2b8e34558..7a4d0575eb57 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs @@ -30,25 +30,49 @@ protected override async Task SendAsync(HttpRequestMessage } bool canFallback = request.RequestUri.Host == _host && request.RequestUri.Port == _port && request.RequestUri.Scheme == "https"; - bool canRetry = canFallback; do { try { if (canFallback && _fallbackToHttp) { - var uriBuilder = new UriBuilder(request.RequestUri); - uriBuilder.Scheme = "http"; - request.RequestUri = uriBuilder.Uri; - canRetry = false; + FallbackToHttp(request); + canFallback = false; } return await base.SendAsync(request, cancellationToken).ConfigureAwait(false); } - catch (HttpRequestException re) when (canRetry && re.HttpRequestError == HttpRequestError.SecureConnectionError) + catch (HttpRequestException re) when (canFallback && ShouldAttemptFallbackToHttp(re)) { - _fallbackToHttp = true; + try + { + // Try falling back. + FallbackToHttp(request); + HttpResponseMessage response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); + + // Fall back was successful. Use http for all new requests. + _fallbackToHttp = true; + + return response; + } + catch + { } + + // Falling back didn't work, throw original exception. + throw; } } while (true); } + + internal static bool ShouldAttemptFallbackToHttp(HttpRequestException exception) + { + return exception.HttpRequestError == HttpRequestError.SecureConnectionError; + } + + private static void FallbackToHttp(HttpRequestMessage request) + { + var uriBuilder = new UriBuilder(request.RequestUri!); + uriBuilder.Scheme = "http"; + request.RequestUri = uriBuilder.Uri; + } } diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index e3fbcd8b4d34..69b749940dd9 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -395,12 +395,13 @@ public async Task UploadBlobChunkedAsync_Failure() api.Verify(api => api.Blob.Upload.UploadChunkAsync(It.IsIn(absoluteUploadUri, uploadPath), It.IsAny(), It.IsAny()), Times.Exactly(1)); } - [InlineData(true, true)] - [InlineData(true, false)] - [InlineData(false, true)] - [InlineData(false, false)] + [InlineData(true, true, true)] + [InlineData(false, true, true)] + [InlineData(true, false, true)] + [InlineData(false, false, true)] + [InlineData(false, false, false)] [Theory] - public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) + public async Task InsecureRegistry(bool isInsecureRegistry, bool serverIsHttps, bool httpServerCloseAbortive) { ILogger logger = _loggerFactory.CreateLogger(nameof(InsecureRegistry)); @@ -437,8 +438,20 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) { await sslStream.AuthenticateAsServerAsync(sslOptions!, default(CancellationToken)); } - await stream.ReadAtLeastAsync(new byte[1], 1); // Wait for the request. - await stream.WriteAsync("HTTP/1.0 200 OK\r\nContent-Length: 0\r\n\r\n"u8.ToArray()); + byte[] buffer = new byte[10]; + await stream.ReadAtLeastAsync(buffer, buffer.Length); // Wait for the request. + // Repond if we see '/v2/' in the buffer (since we expect that as part of the request path). + if (buffer.AsSpan().IndexOf("/v2/"u8) != 0) + { + await stream.WriteAsync("HTTP/1.0 200 OK\r\nContent-Length: 0\r\n\r\n"u8.ToArray()); + } + else + { + if (httpServerCloseAbortive) + { + client.GetStream().Close(timeout: 0); + } + } } catch { } @@ -462,17 +475,48 @@ public async Task InsecureRegistry(bool serverIsHttps, bool isInsecureRegistry) } else { - try + if (serverIsHttps) { // Does not fall back and throws HttpRequestException. var requestException = await Assert.ThrowsAsync(() => getManifest); Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); } - catch when (!serverIsHttps) + else { - // We're talking https to an http server. The server may respond in a way that triggers - // the retry handling in AuthHandshakeMessageHandler which then throws an ApplicationException. - await Assert.ThrowsAsync(() => getManifest); + // We're using https against an http server. + var re = await Assert.ThrowsAnyAsync(() => getManifest); + try + { + // Verify the exception is one that FallbackToHttpMessageHandler would fall back for. + Assert.True(FallbackToHttpMessageHandler.ShouldAttemptFallbackToHttp(re)); + } + catch + { + // Log a message describing the exception that would not cause the FallbackToHttpMessageHandler to fall back. + StringBuilder sb = new(); + sb.AppendLine("Exception is not fallback exception:"); + Exception? e = re; + while (e != null) + { + switch (e) + { + case SocketException socketException: + sb.AppendLine($"{nameof(SocketException)}({socketException.SocketErrorCode}) - {e.Message}"); + break; + case HttpRequestException requestException: + sb.AppendLine($"{nameof(HttpRequestException)}({requestException.HttpRequestError}) - {e.Message}"); + break; + default: + sb.AppendLine($"{e.GetType().Name} - {e.Message}"); + break; + } + + e = e.InnerException; + } + logger.LogError(sb.ToString()); + + throw; + } } } } From 156615206387a517f882b9533356d93328cec10b Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Mon, 17 Jun 2024 12:54:51 +0200 Subject: [PATCH 12/15] Refactor test. --- .../RegistryTests.cs | 60 +++++++++---------- 1 file changed, 27 insertions(+), 33 deletions(-) diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 69b749940dd9..41f740cf1c8b 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -475,48 +475,42 @@ public async Task InsecureRegistry(bool isInsecureRegistry, bool serverIsHttps, } else { - if (serverIsHttps) + // Does not fall back and throws HttpRequestException with SecureConnectionError. + Exception? exception = await Assert.ThrowsAnyAsync(() => getManifest); + try { - // Does not fall back and throws HttpRequestException. - var requestException = await Assert.ThrowsAsync(() => getManifest); + Assert.IsType(exception); + HttpRequestException requestException = (HttpRequestException)exception; Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError); + + // The FallbackToHttpMessageHandler should fall back (if this registry was configured as insecure). + Assert.True(FallbackToHttpMessageHandler.ShouldAttemptFallbackToHttp(requestException)); } - else + catch { - // We're using https against an http server. - var re = await Assert.ThrowsAnyAsync(() => getManifest); - try - { - // Verify the exception is one that FallbackToHttpMessageHandler would fall back for. - Assert.True(FallbackToHttpMessageHandler.ShouldAttemptFallbackToHttp(re)); - } - catch + // Log a message describing the exception. + StringBuilder sb = new(); + sb.AppendLine("Exception is not fallback exception:"); + while (exception != null) { - // Log a message describing the exception that would not cause the FallbackToHttpMessageHandler to fall back. - StringBuilder sb = new(); - sb.AppendLine("Exception is not fallback exception:"); - Exception? e = re; - while (e != null) + switch (exception) { - switch (e) - { - case SocketException socketException: - sb.AppendLine($"{nameof(SocketException)}({socketException.SocketErrorCode}) - {e.Message}"); - break; - case HttpRequestException requestException: - sb.AppendLine($"{nameof(HttpRequestException)}({requestException.HttpRequestError}) - {e.Message}"); - break; - default: - sb.AppendLine($"{e.GetType().Name} - {e.Message}"); - break; - } - - e = e.InnerException; + case SocketException socketException: + sb.AppendLine($"{nameof(SocketException)}({socketException.SocketErrorCode}) - {exception.Message}"); + break; + case HttpRequestException requestException: + sb.AppendLine($"{nameof(HttpRequestException)}({requestException.HttpRequestError}) - {exception.Message}"); + break; + default: + sb.AppendLine($"{exception.GetType().Name} - {exception.Message}"); + break; } - logger.LogError(sb.ToString()); - throw; + exception = exception.InnerException; } + logger.LogError(sb.ToString()); + + throw; } } } From bbe30578e37333d06973d0e09853c3962d60a592 Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 18 Jun 2024 11:14:11 +0200 Subject: [PATCH 13/15] Add some logging to the fallback handler. --- .../FallbackToHttpMessageHandler.cs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs index 7a4d0575eb57..123f50e7aa85 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs @@ -14,12 +14,14 @@ internal sealed partial class FallbackToHttpMessageHandler : DelegatingHandler { private readonly string _host; private readonly int _port; + private readonly ILogger _logger; private bool _fallbackToHttp; public FallbackToHttpMessageHandler(string host, int port, HttpMessageHandler innerHandler, ILogger logger) : base(innerHandler) { _host = host; _port = port; + _logger = logger; } protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) @@ -47,16 +49,20 @@ protected override async Task SendAsync(HttpRequestMessage try { // Try falling back. + _logger.LogTrace("Attempt to fall back to http for {uri}.", request.RequestUri); FallbackToHttp(request); HttpResponseMessage response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); // Fall back was successful. Use http for all new requests. + _logger.LogTrace("Fall back to http for {uri} was successful.", request.RequestUri); _fallbackToHttp = true; return response; } - catch - { } + catch (Exception ex) + { + _logger.LogInformation(ex, "Fall back to http for {uri} failed with message \"{message}\".", request.RequestUri, ex.Message); + } // Falling back didn't work, throw original exception. throw; From 2f1c890214c8a8ddd6828cafe7b9012c211ba93c Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 18 Jun 2024 11:17:01 +0200 Subject: [PATCH 14/15] Use the same uri in all logged messages. --- .../FallbackToHttpMessageHandler.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs index 123f50e7aa85..5f7754d58bb1 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/FallbackToHttpMessageHandler.cs @@ -46,22 +46,23 @@ protected override async Task SendAsync(HttpRequestMessage } catch (HttpRequestException re) when (canFallback && ShouldAttemptFallbackToHttp(re)) { + string uri = request.RequestUri.ToString(); try { // Try falling back. - _logger.LogTrace("Attempt to fall back to http for {uri}.", request.RequestUri); + _logger.LogTrace("Attempt to fall back to http for {uri}.", uri); FallbackToHttp(request); HttpResponseMessage response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); // Fall back was successful. Use http for all new requests. - _logger.LogTrace("Fall back to http for {uri} was successful.", request.RequestUri); + _logger.LogTrace("Fall back to http for {uri} was successful.", uri); _fallbackToHttp = true; return response; } catch (Exception ex) { - _logger.LogInformation(ex, "Fall back to http for {uri} failed with message \"{message}\".", request.RequestUri, ex.Message); + _logger.LogInformation(ex, "Fall back to http for {uri} failed with message \"{message}\".", uri, ex.Message); } // Falling back didn't work, throw original exception. From 924929d4c4a88a58321ad0633055ec9ff56a94bf Mon Sep 17 00:00:00 2001 From: Tom Deseyn Date: Tue, 18 Jun 2024 23:08:01 +0200 Subject: [PATCH 15/15] Fix test. --- .../AuthHandshakeMessageHandler.cs | 6 +++++- .../RegistryTests.cs | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs b/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs index fbbdc9fada47..543db320b5c4 100644 --- a/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs +++ b/src/Containers/Microsoft.NET.Build.Containers/AuthHandshakeMessageHandler.cs @@ -333,6 +333,7 @@ protected override async Task SendAsync(HttpRequestMessage } int retryCount = 0; + List? requestExceptions = null; while (retryCount < MaxRequestRetries) { @@ -364,6 +365,9 @@ protected override async Task SendAsync(HttpRequestMessage } catch (HttpRequestException e) when (e.InnerException is IOException ioe && ioe.InnerException is SocketException se) { + requestExceptions ??= new(); + requestExceptions.Add(e); + retryCount += 1; _logger.LogInformation("Encountered a HttpRequestException {error} with message \"{message}\". Pausing before retry.", e.HttpRequestError, se.Message); _logger.LogTrace("Exception details: {ex}", se); @@ -374,7 +378,7 @@ protected override async Task SendAsync(HttpRequestMessage } } - throw new ApplicationException(Resource.GetString(nameof(Strings.TooManyRetries))); + throw new ApplicationException(Resource.GetString(nameof(Strings.TooManyRetries)), new AggregateException(requestExceptions!)); } [GeneratedRegex("(?\\w+)=\"(?[^\"]*)\"(?:,|$)")] diff --git a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs index 41f740cf1c8b..df519b55f71c 100644 --- a/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs +++ b/test/Microsoft.NET.Build.Containers.UnitTests/RegistryTests.cs @@ -479,6 +479,14 @@ public async Task InsecureRegistry(bool isInsecureRegistry, bool serverIsHttps, Exception? exception = await Assert.ThrowsAnyAsync(() => getManifest); try { + // The AuthHandshakeMessageHandler may reach its retry limit and throw an ApplicationException. + if (exception is ApplicationException) + { + // Find the exception for the first failed attempt. + exception = (exception.InnerException as AggregateException)?.InnerExceptions.FirstOrDefault(); + Assert.NotNull(exception); + } + Assert.IsType(exception); HttpRequestException requestException = (HttpRequestException)exception; Assert.Equal(HttpRequestError.SecureConnectionError, requestException.HttpRequestError);