diff --git a/src/DfE.CheckPerformanceData.Web/Admin/StorageBrowserOptions.cs b/src/DfE.CheckPerformanceData.Web/Admin/StorageBrowserOptions.cs new file mode 100644 index 00000000..47282309 --- /dev/null +++ b/src/DfE.CheckPerformanceData.Web/Admin/StorageBrowserOptions.cs @@ -0,0 +1,26 @@ +namespace DfE.CheckPerformanceData.Web.Admin; + +/// +/// Containers the storage browser must never reach. +/// +/// +/// 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. +/// +public sealed class StorageBrowserOptions +{ + public const string SectionName = "StorageBrowser"; + + /// + /// Container names the browser refuses to list, read, write or delete. Matched + /// case-insensitively. + /// + public string[] ProtectedContainers { get; set; } = ["data-protection-keys"]; +} diff --git a/src/DfE.CheckPerformanceData.Web/Controllers/StorageAdminController.cs b/src/DfE.CheckPerformanceData.Web/Controllers/StorageAdminController.cs index f809b06a..ee95680a 100644 --- a/src/DfE.CheckPerformanceData.Web/Controllers/StorageAdminController.cs +++ b/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 storageAccounts) : Controller +public sealed class StorageAdminController( + IReadOnlyDictionary storageAccounts, + IOptions 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 _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 DisplayNames = new Dictionary { ["app"] = "App Storage", @@ -35,7 +48,10 @@ public async Task Containers(string account) var containers = new List(); 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 Containers(string account) [HttpGet("admin/storage/{account}/{containerName}")] public async Task 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 Container(string account, string containerName, [HttpGet("admin/storage/{account}/{containerName}/preview")] public async Task 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 Preview(string account, string containerName, [ [HttpGet("admin/storage/{account}/{containerName}/download")] public async Task 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 Download(string account, string containerName, [ValidateAntiForgeryToken] public async Task 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 Delete(string account, string containerName, st [ValidateAntiForgeryToken] public async Task Upload(string account, string containerName, List 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); diff --git a/src/DfE.CheckPerformanceData.Web/Startup/BlobStorageExtensions.cs b/src/DfE.CheckPerformanceData.Web/Startup/BlobStorageExtensions.cs index 77ef310e..62019019 100644 --- a/src/DfE.CheckPerformanceData.Web/Startup/BlobStorageExtensions.cs +++ b/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( + configuration.GetSection(StorageBrowserOptions.SectionName)); + services.AddSingleton>(_ => { var clients = new Dictionary(); diff --git a/tests/DfE.CheckPerformanceData.UnitTests/Web/StorageAdminControllerTests.cs b/tests/DfE.CheckPerformanceData.UnitTests/Web/StorageAdminControllerTests.cs index 6a5d508f..ee70ea4f 100644 --- a/tests/DfE.CheckPerformanceData.UnitTests/Web/StorageAdminControllerTests.cs +++ b/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 accounts) => - new(accounts) + private static StorageAdminController BuildSut( + IReadOnlyDictionary 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(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 { ["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(); + service.GetBlobContainersAsync( + Arg.Any(), Arg.Any(), + Arg.Any(), Arg.Any()) + .Returns(AsyncPageable.FromPages([ + Page.FromValues([ + BlobsModelFactory.BlobContainerItem("window-123", null), + BlobsModelFactory.BlobContainerItem(Keyring, null), + BlobsModelFactory.BlobContainerItem("rules-config", null), + ], null, Substitute.For())])); + + var result = await BuildSut(service).Containers("app"); + + var model = Assert.IsType( + Assert.IsType(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(); + var container = Substitute.For(); + service.GetBlobContainerClient(Keyring).Returns(container); + + var result = await BuildSut(service).Preview("app", Keyring, "keys.xml"); + + Assert.IsType(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(); + + var result = await BuildSut(service).Download("app", Keyring, "keys.xml"); + + Assert.IsType(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(); + + var result = await BuildSut(service).Delete("app", Keyring, "keys.xml"); + + Assert.IsType(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(); + + var result = await BuildSut(service).Upload("app", Keyring, [], null, null); + + Assert.IsType(result); + service.DidNotReceive().GetBlobContainerClient(Keyring); + } + + [Fact] + public async Task Container_ForAProtectedContainer_Is404() + { + var service = Substitute.For(); + + var result = await BuildSut(service).Container("app", Keyring, null); + + Assert.IsType(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(); + + Assert.IsType(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(); + + var result = await BuildSut(service, "secrets-vault").Download("app", "secrets-vault", "x.txt"); + + Assert.IsType(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(); + var container = Substitute.For(); + var blob = Substitute.For(); + service.GetBlobContainerClient("window-123").Returns(container); + container.GetBlobClient("draft.json").Returns(blob); + blob.DeleteIfExistsAsync(Arg.Any(), Arg.Any(), + Arg.Any()).Returns(Response.FromValue(true, Substitute.For())); + + var result = await BuildSut(service).Delete("app", "window-123", "draft.json"); + + Assert.IsType(result); + } +}