Keep the storage browser out of containers holding secrets

src/DfE.CheckPerformanceData.Web/Admin/StorageBrowserOptions.cs

@@ -0,0 +1,26 @@
+namespace DfE.CheckPerformanceData.Web.Admin;
+
+/// <summary>
+/// Containers the storage browser must never reach.
+/// </summary>
+/// <remarks>
+/// The storage-admin section grant decides who may open the browser. It says nothing about what
+/// the browser may touch, so every container in the account was reachable — including the one
+/// holding the Data Protection keyring, which protects authentication cookies, session state and
+/// antiforgery tokens. Reading it allows those to be decrypted, replacing it allows them to be
+/// forged, and deleting it invalidates every one of them at once.
+///
+/// A secret has no business being served by the application that holds it, whoever is asking.
+/// Configurable rather than hard-coded so a future secret container is covered by a setting
+/// instead of a release.
+/// </remarks>
+public sealed class StorageBrowserOptions
+{
+    public const string SectionName = "StorageBrowser";
+
+    /// <summary>
+    /// Container names the browser refuses to list, read, write or delete. Matched
+    /// case-insensitively.
+    /// </summary>
+    public string[] ProtectedContainers { get; set; } = ["data-protection-keys"];
+}

src/DfE.CheckPerformanceData.Web/Controllers/StorageAdminController.cs

@@ -4,14 +4,27 @@
 using DfE.CheckPerformanceData.Web.Admin.Nav;
 using DfE.CheckPerformanceData.Web.Controllers.ViewModels;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.Options;
 
 namespace DfE.CheckPerformanceData.Web.Controllers;
 
 // Blob-storage browser and per-blob preview / download / delete. Gated by the storage-admin
 // section grant.
 [RequireAdminSection(AdminNavKeys.StorageAdmin)]
-public sealed class StorageAdminController(IReadOnlyDictionary<string, BlobServiceClient> storageAccounts) : Controller
+public sealed class StorageAdminController(
+    IReadOnlyDictionary<string, BlobServiceClient> storageAccounts,
+    IOptions<StorageBrowserOptions> browserOptions) : Controller
 {
+    // The keyring and anything else configured as secret. Every route consults this before it
+    // resolves a container, so a protected blob is never opened, written or removed — not opened
+    // and then withheld. Kept in one place because six separate guards drift apart.
+    private readonly HashSet<string> _protectedContainers =
+        new(browserOptions.Value.ProtectedContainers ?? [], StringComparer.OrdinalIgnoreCase);
+
+    // 404 rather than 403, matching what a non-granted admin section returns: a refusal that
+    // confirms the container exists is a smaller leak than the contents, but it is still a leak.
+    private bool IsProtected(string containerName) => _protectedContainers.Contains(containerName);
+
     private static readonly IReadOnlyDictionary<string, string> DisplayNames = new Dictionary<string, string>
     {
         ["app"] = "App Storage",
@@ -35,7 +48,10 @@ public async Task<IActionResult> Containers(string account)
 
         var containers = new List<string>();
         await foreach (var item in client.GetBlobContainersAsync())
+        {
+            if (IsProtected(item.Name)) continue;
             containers.Add(item.Name);
+        }
 
         return View(new StorageContainerListViewModel
         {
@@ -48,7 +64,9 @@ public async Task<IActionResult> Containers(string account)
     [HttpGet("admin/storage/{account}/{containerName}")]
     public async Task<IActionResult> Container(string account, string containerName, [FromQuery] string? prefix, CancellationToken cancellationToken = default)
     {
-        var client = GetClient(account);
+        if (IsProtected(containerName)) return NotFound();
+
+                var client = GetClient(account);
         if (client is null) return NotFound();
 
         var container = client.GetBlobContainerClient(containerName);
@@ -95,7 +113,9 @@ public async Task<IActionResult> Container(string account, string containerName,
     [HttpGet("admin/storage/{account}/{containerName}/preview")]
     public async Task<IActionResult> Preview(string account, string containerName, [FromQuery] string blob)
     {
-        var client = GetClient(account);
+        if (IsProtected(containerName)) return NotFound();
+
+                var client = GetClient(account);
         if (client is null) return NotFound();
 
         var container = client.GetBlobContainerClient(containerName);
@@ -137,7 +157,9 @@ public async Task<IActionResult> Preview(string account, string containerName, [
     [HttpGet("admin/storage/{account}/{containerName}/download")]
     public async Task<IActionResult> Download(string account, string containerName, [FromQuery] string blob)
     {
-        var client = GetClient(account);
+        if (IsProtected(containerName)) return NotFound();
+
+                var client = GetClient(account);
         if (client is null) return NotFound();
 
         var container = client.GetBlobContainerClient(containerName);
@@ -155,7 +177,9 @@ public async Task<IActionResult> Download(string account, string containerName,
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> Delete(string account, string containerName, string blobName, [FromForm] string? prefix = null)
     {
-        var client = GetClient(account);
+        if (IsProtected(containerName)) return NotFound();
+
+                var client = GetClient(account);
         if (client is null) return NotFound();
 
         var container = client.GetBlobContainerClient(containerName);
@@ -168,7 +192,9 @@ public async Task<IActionResult> Delete(string account, string containerName, st
     [ValidateAntiForgeryToken]
     public async Task<IActionResult> Upload(string account, string containerName, List<IFormFile> files, [FromForm] string? prefix, [FromForm] string? folder)
     {
-        var client = GetClient(account);
+        if (IsProtected(containerName)) return NotFound();
+
+                var client = GetClient(account);
         if (client is null) return NotFound();
 
         var container = client.GetBlobContainerClient(containerName);

src/DfE.CheckPerformanceData.Web/Startup/BlobStorageExtensions.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using Azure.Storage.Blobs;
+using DfE.CheckPerformanceData.Web.Admin;
 using DfE.CheckPerformanceData.Application.RequestSubmission;
 using DfE.CheckPerformanceData.Infrastructure.BlobStorage;
 using DfE.CheckPerformanceData.Infrastructure.Ingress;
@@ -17,6 +18,12 @@ public static IServiceCollection AddCpdBlobStorage(this IServiceCollection servi
         services.AddSingleton(_ =>
             new BlobServiceClient(configuration.GetConnectionString("AzureStorage")));
 
+        // The storage browser's deny-list. Bound rather than hard-coded so an environment can add
+        // a secret container without a release; the built-in default already covers the keyring,
+        // so an environment that binds nothing is still protected.
+        services.Configure<StorageBrowserOptions>(
+            configuration.GetSection(StorageBrowserOptions.SectionName));
+
         services.AddSingleton<IReadOnlyDictionary<string, BlobServiceClient>>(_ =>
         {
             var clients = new Dictionary<string, BlobServiceClient>();

tests/DfE.CheckPerformanceData.UnitTests/Web/StorageAdminControllerTests.cs

@@ -1,10 +1,13 @@
 using Azure;
 using Azure.Storage.Blobs;
 using Azure.Storage.Blobs.Models;
+using DfE.CheckPerformanceData.Web.Admin;
 using DfE.CheckPerformanceData.Web.Admin.Nav;
 using DfE.CheckPerformanceData.Web.Controllers;
+using DfE.CheckPerformanceData.Web.Controllers.ViewModels;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Mvc;
+using Microsoft.Extensions.Options;
 using NSubstitute;
 
 namespace DfE.CheckPerformanceData.Application.UnitTests.Web;
@@ -34,8 +37,15 @@ public void StorageBrowserNavEntry_HasCorrectKeyAndParent()
 
 public sealed class StorageAdminControllerTests
 {
-    private static StorageAdminController BuildSut(IReadOnlyDictionary<string, BlobServiceClient> accounts) =>
-        new(accounts)
+    private static StorageAdminController BuildSut(
+        IReadOnlyDictionary<string, BlobServiceClient> accounts,
+        params string[] protectedContainers) =>
+        new(accounts, Options.Create(new StorageBrowserOptions
+        {
+            ProtectedContainers = protectedContainers.Length > 0
+                ? protectedContainers
+                : new StorageBrowserOptions().ProtectedContainers
+        }))
         {
             ControllerContext = new ControllerContext
             {
@@ -102,3 +112,168 @@ public async Task Delete_UnknownAccount_ReturnsNotFound()
         Assert.IsType<NotFoundResult>(result);
     }
 }
+
+
+// The Data Protection keyring lives in blob storage beside ordinary application data, and the
+// browser reached it exactly as it reaches anything else: an administrator could read the key
+// descriptors in the preview pane, download keys.xml, delete it — invalidating every session and
+// antiforgery token at once — or upload a replacement and mint tokens at will. The section grant
+// governs who may use the browser, not what the browser may touch, and this is the second gate.
+//
+// Every route is covered rather than just the two the assessment happened to exercise: six
+// separate guards drift, and a container that is off-limits has to be off-limits for listing,
+// reading, writing and deleting alike.
+public sealed class StorageAdminProtectedContainerTests
+{
+    private const string Keyring = "data-protection-keys";
+
+    private static StorageAdminController BuildSut(
+        BlobServiceClient client, params string[] protectedContainers) =>
+        new(new Dictionary<string, BlobServiceClient> { ["app"] = client },
+            Options.Create(new StorageBrowserOptions
+            {
+                ProtectedContainers = protectedContainers.Length > 0
+                    ? protectedContainers
+                    : new StorageBrowserOptions().ProtectedContainers
+            }))
+        {
+            ControllerContext = new ControllerContext { HttpContext = new DefaultHttpContext() }
+        };
+
+    // The keyring is protected out of the box. An environment that has to add another secret
+    // container should not have to discover that the default was empty.
+    [Fact]
+    public void TheKeyringIsProtectedByDefault()
+    {
+        Assert.Contains(Keyring, new StorageBrowserOptions().ProtectedContainers);
+    }
+
+
+    // The container must not appear in the browser at all. Refusing the routes but still listing
+    // it advertises where the keyring lives and invites someone to go looking for a way in.
+    [Fact]
+    public async Task Containers_DoesNotListProtectedContainers()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+        service.GetBlobContainersAsync(
+                Arg.Any<BlobContainerTraits>(), Arg.Any<BlobContainerStates>(),
+                Arg.Any<string>(), Arg.Any<CancellationToken>())
+            .Returns(AsyncPageable<BlobContainerItem>.FromPages([
+                Page<BlobContainerItem>.FromValues([
+                    BlobsModelFactory.BlobContainerItem("window-123", null),
+                    BlobsModelFactory.BlobContainerItem(Keyring, null),
+                    BlobsModelFactory.BlobContainerItem("rules-config", null),
+                ], null, Substitute.For<Response>())]));
+
+        var result = await BuildSut(service).Containers("app");
+
+        var model = Assert.IsType<StorageContainerListViewModel>(
+            Assert.IsType<ViewResult>(result).Model);
+        Assert.DoesNotContain(Keyring, model.Containers);
+        Assert.Contains("window-123", model.Containers);
+        Assert.Contains("rules-config", model.Containers);
+    }
+
+    [Fact]
+    public async Task Preview_OfAProtectedContainer_Is404_AndReadsNothing()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+        var container = Substitute.For<BlobContainerClient>();
+        service.GetBlobContainerClient(Keyring).Returns(container);
+
+        var result = await BuildSut(service).Preview("app", Keyring, "keys.xml");
+
+        Assert.IsType<NotFoundResult>(result);
+        // The refusal has to come before the blob is touched — a protected blob that is read and
+        // then withheld has still been read.
+        service.DidNotReceive().GetBlobContainerClient(Keyring);
+    }
+
+    [Fact]
+    public async Task Download_OfAProtectedContainer_Is404_AndReadsNothing()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        var result = await BuildSut(service).Download("app", Keyring, "keys.xml");
+
+        Assert.IsType<NotFoundResult>(result);
+        service.DidNotReceive().GetBlobContainerClient(Keyring);
+    }
+
+    // Deleting the keyring is not a leak but an outage: every issued cookie and antiforgery token
+    // becomes undecryptable at once.
+    [Fact]
+    public async Task Delete_InAProtectedContainer_Is404_AndDeletesNothing()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        var result = await BuildSut(service).Delete("app", Keyring, "keys.xml");
+
+        Assert.IsType<NotFoundResult>(result);
+        service.DidNotReceive().GetBlobContainerClient(Keyring);
+    }
+
+    // Worse than reading it: a replacement keyring lets tokens be forged rather than merely read.
+    [Fact]
+    public async Task Upload_ToAProtectedContainer_Is404_AndWritesNothing()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        var result = await BuildSut(service).Upload("app", Keyring, [], null, null);
+
+        Assert.IsType<NotFoundResult>(result);
+        service.DidNotReceive().GetBlobContainerClient(Keyring);
+    }
+
+    [Fact]
+    public async Task Container_ForAProtectedContainer_Is404()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        var result = await BuildSut(service).Container("app", Keyring, null);
+
+        Assert.IsType<NotFoundResult>(result);
+        service.DidNotReceive().GetBlobContainerClient(Keyring);
+    }
+
+    // Blob container names are lower-case by Azure's rules, but the guard compares strings and a
+    // configured entry could be typed in any case.
+    [Theory]
+    [InlineData("Data-Protection-Keys")]
+    [InlineData("DATA-PROTECTION-KEYS")]
+    public async Task ProtectedMatching_IgnoresCase(string requested)
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        Assert.IsType<NotFoundResult>(await BuildSut(service).Download("app", requested, "keys.xml"));
+    }
+
+    // The deny-list is configuration so a future secret container is covered without a code change.
+    [Fact]
+    public async Task AConfiguredContainer_IsProtectedToo()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+
+        var result = await BuildSut(service, "secrets-vault").Download("app", "secrets-vault", "x.txt");
+
+        Assert.IsType<NotFoundResult>(result);
+    }
+
+    // The guard must not turn the browser off. An ordinary container still resolves, and the
+    // unknown-account 404 must keep coming from the account check rather than the new one.
+    [Fact]
+    public async Task AnOrdinaryContainer_IsStillReachable()
+    {
+        var service = Substitute.For<BlobServiceClient>();
+        var container = Substitute.For<BlobContainerClient>();
+        var blob = Substitute.For<BlobClient>();
+        service.GetBlobContainerClient("window-123").Returns(container);
+        container.GetBlobClient("draft.json").Returns(blob);
+        blob.DeleteIfExistsAsync(Arg.Any<DeleteSnapshotsOption>(), Arg.Any<BlobRequestConditions>(),
+            Arg.Any<CancellationToken>()).Returns(Response.FromValue(true, Substitute.For<Response>()));
+
+        var result = await BuildSut(service).Delete("app", "window-123", "draft.json");
+
+        Assert.IsType<RedirectResult>(result);
+    }
+}