Stricter data protection errors

src/SharedWeb/Utilities/DataProtectionServiceCollectionExtensions.cs

@@ -0,0 +1,102 @@
+using System.Security.Cryptography;
+using System.Security.Cryptography.X509Certificates;
+using Azure;
+using Azure.Storage.Blobs;
+using Bit.Core.Settings;
+using Bit.Core.Utilities;
+using Microsoft.AspNetCore.DataProtection;
+using Microsoft.AspNetCore.Hosting;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Hosting;
+
+namespace Bit.SharedWeb.Utilities;
+
+public static class DataProtectionServiceCollectionExtensions
+{
+    public static void AddCustomDataProtectionServices(
+        this IServiceCollection services, IWebHostEnvironment env, GlobalSettings globalSettings)
+    {
+        var builder = services.AddDataProtection().SetApplicationName("Bitwarden");
+
+        if (globalSettings.SelfHosted && CoreHelpers.SettingHasValue(globalSettings.DataProtection.Directory))
+        {
+            builder.PersistKeysToFileSystem(new DirectoryInfo(globalSettings.DataProtection.Directory));
+        }
+
+        if (!globalSettings.SelfHosted && CoreHelpers.SettingHasValue(globalSettings.Storage?.ConnectionString))
+        {
+            X509Certificate2? dataProtectionCert = null;
+            if (CoreHelpers.SettingHasValue(globalSettings.DataProtection.CertificateThumbprint))
+            {
+                dataProtectionCert = CoreHelpers.GetCertificate(
+                    globalSettings.DataProtection.CertificateThumbprint)
+                    ?? throw new InvalidOperationException(
+                        $"No data protection certificate could be found with thumbprint '{globalSettings.DataProtection.CertificateThumbprint}'.");
+            }
+            else if (CoreHelpers.SettingHasValue(globalSettings.DataProtection.CertificatePassword))
+            {
+                dataProtectionCert = DownloadRequiredCertFromBlobStorage(
+                    globalSettings.Storage.ConnectionString,
+                    "certificates",
+                    globalSettings.DataProtection.BlobName,
+                    globalSettings.DataProtection.CertificatePassword,
+                    "protect"
+                );
+            }
+
+            if (!env.IsDevelopment())
+            {
+                if (dataProtectionCert is null)
+                {
+                    throw new InvalidOperationException("A data protection certificate could not be acquired and one is required when running in non-development cloud environments. Please make sure your configuration has a valid connection string to azure blob storage.");
+                }
+
+                builder
+                    .PersistKeysToAzureBlobStorage(globalSettings.Storage.ConnectionString, "aspnet-dataprotection", "keys.xml")
+                    .ProtectKeysWithCertificate(dataProtectionCert);
+
+                if (globalSettings.DataProtection.UnprotectCertificates.Length > 0)
+                {
+                    var unprotectCertificates = globalSettings.DataProtection.UnprotectCertificates
+                        .Index()
+                        .Select(i => DownloadRequiredCertFromBlobStorage(
+                            globalSettings.Storage.ConnectionString,
+                            "certificates",
+                            i.Item.FileName,
+                            i.Item.Password,
+                            $"Unprotect {i.Index}"
+                        )).ToArray();
+
+                    builder.UnprotectKeysWithAnyCertificate(unprotectCertificates);
+                }
+            }
+        }
+    }
+
+    private static X509Certificate2 DownloadRequiredCertFromBlobStorage(
+        string connectionString,
+        string container,
+        string file,
+        string password,
+        string context)
+    {
+        try
+        {
+            var blobServiceClient = new BlobServiceClient(connectionString);
+            var containerClient = blobServiceClient.GetBlobContainerClient(container);
+            var blobClient = containerClient.GetBlobClient(file);
+
+            using var memoryStream = new MemoryStream();
+            blobClient.DownloadTo(memoryStream);
+            return X509CertificateLoader.LoadPkcs12(memoryStream.ToArray(), password);
+        }
+        catch (RequestFailedException ex)
+        {
+            throw new InvalidOperationException($"Unable to download certificate from azure blob storage: {context}", ex);
+        }
+        catch (CryptographicException ex)
+        {
+            throw new InvalidOperationException($"Unable to load certificate downloaded from azure blob storage; verify the password is correct and the blob contains valid PKCS#12 data: {context}", ex);
+        }
+    }
+}

src/SharedWeb/Utilities/ServiceCollectionExtensions.cs

@@ -4,7 +4,6 @@
 using System.Net;
 using System.Reflection;
 using System.Security.Claims;
-using System.Security.Cryptography.X509Certificates;
 using AspNetCoreRateLimit;
 using Bit.Core.AdminConsole.AbilitiesCache;
 using Bit.Core.AdminConsole.Models.Business.Tokenables;
@@ -492,56 +491,7 @@ public static void AddIdentityAuthenticationServices(
         }
     }
 
-    public static void AddCustomDataProtectionServices(
-        this IServiceCollection services, IWebHostEnvironment env, GlobalSettings globalSettings)
-    {
-        var builder = services.AddDataProtection().SetApplicationName("Bitwarden");
 
-        if (globalSettings.SelfHosted && CoreHelpers.SettingHasValue(globalSettings.DataProtection.Directory))
-        {
-            builder.PersistKeysToFileSystem(new DirectoryInfo(globalSettings.DataProtection.Directory));
-        }
-
-        if (!globalSettings.SelfHosted && CoreHelpers.SettingHasValue(globalSettings.Storage?.ConnectionString))
-        {
-            X509Certificate2 dataProtectionCert = null;
-            if (CoreHelpers.SettingHasValue(globalSettings.DataProtection.CertificateThumbprint))
-            {
-                dataProtectionCert = CoreHelpers.GetCertificate(
-                    globalSettings.DataProtection.CertificateThumbprint);
-            }
-            else if (CoreHelpers.SettingHasValue(globalSettings.DataProtection.CertificatePassword))
-            {
-                dataProtectionCert = CoreHelpers.GetBlobCertificateAsync(
-                    globalSettings.Storage.ConnectionString,
-                    "certificates",
-                    globalSettings.DataProtection.BlobName,
-                    globalSettings.DataProtection.CertificatePassword)
-                    .GetAwaiter().GetResult();
-            }
-
-            if (!env.IsDevelopment())
-            {
-                builder
-                    .PersistKeysToAzureBlobStorage(globalSettings.Storage.ConnectionString, "aspnet-dataprotection", "keys.xml")
-                    .ProtectKeysWithCertificate(dataProtectionCert);
-
-                if (globalSettings.DataProtection.UnprotectCertificates.Length > 0)
-                {
-                    var unprotectCertificates = Task.WhenAll(globalSettings.DataProtection.UnprotectCertificates
-                        .Select(ci => CoreHelpers.GetBlobCertificateAsync(
-                            globalSettings.Storage.ConnectionString,
-                            "certificates",
-                            ci.FileName,
-                            ci.Password
-                        ))
-                    ).GetAwaiter().GetResult();
-
-                    builder.UnprotectKeysWithAnyCertificate(unprotectCertificates);
-                }
-            }
-        }
-    }
 
     public static IIdentityServerBuilder AddIdentityServerCertificate(
         this IIdentityServerBuilder identityServerBuilder, IWebHostEnvironment env, GlobalSettings globalSettings)

test/SharedWeb.Test/DataProtectionServicesTests.cs

@@ -1,5 +1,6 @@
 using System.Security.Cryptography;
 using System.Security.Cryptography.X509Certificates;
+using Azure;
 using Azure.Storage.Blobs;
 using Bit.Core.Settings;
 using Bit.SharedWeb.Utilities;
@@ -334,6 +335,152 @@ await certificates.Value.UploadBlobAsync(
         Assert.Equal("UpdatedConfig", revertedAppProtector.Unprotect(updatedConfigData));
     }
 
+    [Fact]
+    public async Task UnprotectCertificateMissingFromBlobStorage_Throws()
+    {
+        // The previous implementation returned null on BlobNotFound and that null reached
+        // UnprotectKeysWithAnyCertificate, so pods came up, failed to unprotect existing keys
+        // with a NullReferenceException, and auto-generated their own keys — producing a
+        // divergent key ring across a rolling deploy. The current implementation throws
+        // InvalidOperationException at startup with the call-site context ("Unprotect 0") in
+        // the message so operators can tell which entry went wrong from the log alone.
+        await using var azurite = new ContainerBuilder("mcr.microsoft.com/azure-storage/azurite")
+            .WithPortBinding(10000, true)
+            .Build();
+
+        await azurite.StartAsync();
+
+        var azuriteConnectionString = $"DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://{azurite.Hostname}:{azurite.GetMappedPublicPort(10000)}/devstoreaccount1;";
+
+        var blobServiceClient = new BlobServiceClient(azuriteConnectionString);
+
+        var certificates = await blobServiceClient.CreateBlobContainerAsync("certificates");
+        var dataProtection = await blobServiceClient.CreateBlobContainerAsync("aspnet-dataprotection");
+
+        await certificates.Value.UploadBlobAsync("dataprotection.pfx", new BinaryData(FakeInitialCert));
+        await dataProtection.Value.UploadBlobAsync("keys.xml", new BinaryData(KeysData));
+
+        // The unprotect cert blob "mynewcert.pfx" is intentionally never uploaded.
+        var exception = Assert.Throws<InvalidOperationException>(() => CreateApp(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:Storage:ConnectionString", azuriteConnectionString },
+            { "GlobalSettings:DataProtection:CertificatePassword", "Alongside-Unworthy-Query3-Cozy" },
+            { "GlobalSettings:DataProtection:UnprotectCertificates:0:FileName", "mynewcert.pfx" },
+            { "GlobalSettings:DataProtection:UnprotectCertificates:0:Password", "Undergrad-Police0-Maturely-Countless" },
+        }));
+        Assert.Contains("Unable to download certificate from azure blob storage", exception.Message);
+        Assert.Contains("Unprotect 0", exception.Message);
+        Assert.IsType<RequestFailedException>(exception.InnerException);
+    }
+
+    [Fact]
+    public async Task UnprotectCertificatePasswordIncorrect_Throws()
+    {
+        // The previous implementation swallowed the X509 load failure and returned null, which
+        // reached UnprotectKeysWithAnyCertificate with the same downstream effects as a missing
+        // blob: pods failed to unprotect existing keys with a NullReferenceException and
+        // auto-generated their own divergent keys across a rolling deploy. The current
+        // implementation throws InvalidOperationException at startup with the call-site context
+        // ("Unprotect 0") in the message and the underlying CryptographicException as the inner
+        // exception so operators can tell which entry went wrong from the log alone.
+        await using var azurite = new ContainerBuilder("mcr.microsoft.com/azure-storage/azurite")
+            .WithPortBinding(10000, true)
+            .Build();
+
+        await azurite.StartAsync();
+
+        var azuriteConnectionString = $"DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://{azurite.Hostname}:{azurite.GetMappedPublicPort(10000)}/devstoreaccount1;";
+
+        var blobServiceClient = new BlobServiceClient(azuriteConnectionString);
+
+        var certificates = await blobServiceClient.CreateBlobContainerAsync("certificates");
+        var dataProtection = await blobServiceClient.CreateBlobContainerAsync("aspnet-dataprotection");
+
+        await certificates.Value.UploadBlobAsync("dataprotection.pfx", new BinaryData(FakeInitialCert));
+        await dataProtection.Value.UploadBlobAsync("keys.xml", new BinaryData(KeysData));
+
+        // The unprotect cert blob is uploaded, but the configured password does not match the
+        // password the cert was exported with ("Alongside-Unworthy-Query3-Cozy").
+        await certificates.Value.UploadBlobAsync("mynewcert.pfx", new BinaryData(FakeInitialCert));
+
+        var exception = Assert.Throws<InvalidOperationException>(() => CreateApp(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:Storage:ConnectionString", azuriteConnectionString },
+            { "GlobalSettings:DataProtection:CertificatePassword", "Alongside-Unworthy-Query3-Cozy" },
+            { "GlobalSettings:DataProtection:UnprotectCertificates:0:FileName", "mynewcert.pfx" },
+            { "GlobalSettings:DataProtection:UnprotectCertificates:0:Password", "Wrong-Password-For-Cert" },
+        }));
+        Assert.Contains("Unable to load certificate downloaded from azure blob storage", exception.Message);
+        Assert.Contains("Unprotect 0", exception.Message);
+        Assert.IsType<CryptographicException>(exception.InnerException);
+    }
+
+    [Fact]
+    public async Task ProtectionCertificateMissingFromBlobStorage_Throws()
+    {
+        // The previous implementation passed a null certificate to ProtectKeysWithCertificate
+        // and the call threw with an opaque error. The current implementation throws
+        // InvalidOperationException with the call-site context ("protect") in the message and
+        // the underlying RequestFailedException as the inner exception so operators can tell
+        // which cert went wrong from the log alone.
+        await using var azurite = new ContainerBuilder("mcr.microsoft.com/azure-storage/azurite")
+            .WithPortBinding(10000, true)
+            .Build();
+
+        await azurite.StartAsync();
+
+        var azuriteConnectionString = $"DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://{azurite.Hostname}:{azurite.GetMappedPublicPort(10000)}/devstoreaccount1;";
+
+        var blobServiceClient = new BlobServiceClient(azuriteConnectionString);
+        await blobServiceClient.CreateBlobContainerAsync("certificates");
+        await blobServiceClient.CreateBlobContainerAsync("aspnet-dataprotection");
+
+        // The protection cert blob (default name "dataprotection.pfx") is intentionally never uploaded.
+        var exception = Assert.Throws<InvalidOperationException>(() => CreateApp(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:Storage:ConnectionString", azuriteConnectionString },
+            { "GlobalSettings:DataProtection:CertificatePassword", "Alongside-Unworthy-Query3-Cozy" },
+        }));
+        Assert.Contains("Unable to download certificate from azure blob storage", exception.Message);
+        Assert.Contains("protect", exception.Message);
+        Assert.IsType<RequestFailedException>(exception.InnerException);
+    }
+
+    [Fact]
+    public async Task ProtectionCertificatePasswordIncorrect_Throws()
+    {
+        // The previous implementation swallowed the X509 load failure and returned null, which
+        // ProtectKeysWithCertificate rejected with an opaque error. The current implementation
+        // throws InvalidOperationException with the call-site context ("protect") in the message
+        // and the underlying CryptographicException as the inner exception so operators can tell
+        // which cert went wrong from the log alone.
+        await using var azurite = new ContainerBuilder("mcr.microsoft.com/azure-storage/azurite")
+            .WithPortBinding(10000, true)
+            .Build();
+
+        await azurite.StartAsync();
+
+        var azuriteConnectionString = $"DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==;BlobEndpoint=http://{azurite.Hostname}:{azurite.GetMappedPublicPort(10000)}/devstoreaccount1;";
+
+        var blobServiceClient = new BlobServiceClient(azuriteConnectionString);
+
+        var certificates = await blobServiceClient.CreateBlobContainerAsync("certificates");
+        await blobServiceClient.CreateBlobContainerAsync("aspnet-dataprotection");
+
+        // The protection cert blob is uploaded, but the configured password does not match the
+        // password the cert was exported with ("Alongside-Unworthy-Query3-Cozy").
+        await certificates.Value.UploadBlobAsync("dataprotection.pfx", new BinaryData(FakeInitialCert));
+
+        var exception = Assert.Throws<InvalidOperationException>(() => CreateApp(new Dictionary<string, string?>
+        {
+            { "GlobalSettings:Storage:ConnectionString", azuriteConnectionString },
+            { "GlobalSettings:DataProtection:CertificatePassword", "Wrong-Password-For-Cert" },
+        }));
+        Assert.Contains("Unable to load certificate downloaded from azure blob storage", exception.Message);
+        Assert.Contains("protect", exception.Message);
+        Assert.IsType<CryptographicException>(exception.InnerException);
+    }
+
     private record TestSetupContext(BlobContainerClient Certificates, BlobContainerClient DataProtection, IConfigurationBuilder Config);
 
     private record TestRunContext(IServiceProvider Services, IDataProtector Protector);