From 0fdb2841b72f0cded9879f99faffd6a770832c63 Mon Sep 17 00:00:00 2001 From: Timothy Date: Sun, 12 Jul 2026 00:17:44 +0200 Subject: [PATCH] security(#197): harden ApiKeyProvider persistence + add provider tests (review fixes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cold adversarial review of PR #292 = MERGEABLE-WITH-NITS (no BLOCKER/HIGH). Addresses: - M1: ApiKeyProvider's never-empty invariant was untested. Add ApiKeyProviderTests covering WriteKey precedence, load-existing, empty-file regenerate, generate+persist, 0600 mode, and still-usable-key-when-persist-fails. ResolveKey extracted to an internal seam taking the key path (InternalsVisibleTo ErsatzTV.Tests). - L2: write-then-chmod race — the key was briefly world-readable. Persist now creates the file 0600 atomically via FileStreamOptions.UnixCreateMode (then re-asserts). - L3: a transient read error on an EXISTING key file silently regenerated + clobbered it (invalidating every client key). ResolveKey now rethrows on an unreadable existing file (fail loud) and only regenerates when the file is absent or empty. L4 (LocalhostOnly XFF-spoof under default trust-all) and N5 (length oracle on a fixed-width key) accepted as documented/cosmetic. Refs #197 #280 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../Services/ApiKeyProviderTests.cs | 115 ++++++++++++++++++ ErsatzTV/Services/ApiKeyProvider.cs | 74 +++++++---- 2 files changed, 166 insertions(+), 23 deletions(-) create mode 100644 ErsatzTV.Tests/Services/ApiKeyProviderTests.cs diff --git a/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs b/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs new file mode 100644 index 000000000..13163c22d --- /dev/null +++ b/ErsatzTV.Tests/Services/ApiKeyProviderTests.cs @@ -0,0 +1,115 @@ +using System.Collections.Generic; +using System.IO; +using ErsatzTV.Services; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using NUnit.Framework; +using Shouldly; + +namespace ErsatzTV.Tests.Services; + +[TestFixture] +public class ApiKeyProviderTests +{ + private DirectoryInfo _tempDir = null!; + private string _keyPath = null!; + + [SetUp] + public void SetUp() + { + _tempDir = Directory.CreateTempSubdirectory("etv-apikey-tests"); + _keyPath = Path.Combine(_tempDir.FullName, "api.key"); + } + + [TearDown] + public void TearDown() + { + try + { + _tempDir.Delete(recursive: true); + } + catch + { + // best-effort cleanup + } + } + + private static IConfiguration Config(params (string Key, string Value)[] settings) + { + var dict = new Dictionary(); + foreach ((string key, string value) in settings) + { + dict[key] = value; + } + + return new ConfigurationBuilder().AddInMemoryCollection(dict).Build(); + } + + [Test] + public void Configured_WriteKey_Takes_Precedence_And_Does_Not_Write_A_File() + { + IConfiguration config = Config((ApiKeyProvider.WriteKeyConfigurationKey, " from-config ")); + + string key = ApiKeyProvider.ResolveKey(config, _keyPath, NullLogger.Instance); + + key.ShouldBe("from-config"); // trimmed + File.Exists(_keyPath).ShouldBeFalse(); + } + + [Test] + public void Existing_Key_File_Is_Loaded() + { + File.WriteAllText(_keyPath, " persisted-key \n"); + + string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance); + + key.ShouldBe("persisted-key"); + } + + [Test] + public void Empty_Key_File_Is_Regenerated() + { + File.WriteAllText(_keyPath, " \n"); + + string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance); + + key.ShouldMatch("^[0-9a-f]{64}$"); + File.ReadAllText(_keyPath).Trim().ShouldBe(key); // the empty file was overwritten + } + + [Test] + public void Generates_And_Persists_A_256_Bit_Key_When_None_Exists() + { + string key = ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance); + + key.ShouldMatch("^[0-9a-f]{64}$"); // 32 bytes hex + File.Exists(_keyPath).ShouldBeTrue(); + File.ReadAllText(_keyPath).Trim().ShouldBe(key); + } + + [Test] + [Platform(Exclude = "Win", Reason = "Unix file mode is not applicable on Windows")] + public void Persisted_Key_File_Is_Owner_Read_Write_Only() + { + ApiKeyProvider.ResolveKey(Config(), _keyPath, NullLogger.Instance); + + UnixFileMode mode = File.GetUnixFileMode(_keyPath); + mode.ShouldBe(UnixFileMode.UserRead | UnixFileMode.UserWrite); + } + + [Test] + public void Returns_A_Usable_Key_Even_When_Persist_Fails() + { + // Make persistence fail by giving the key a parent path that is a FILE, not a directory: + // Directory.CreateDirectory / file creation then throws, Persist swallows it, and the + // resolver must still return the in-memory generated key (the never-empty invariant, #280). + string fileAsParent = Path.Combine(_tempDir.FullName, "not-a-dir"); + File.WriteAllText(fileAsParent, "x"); + string unwritable = Path.Combine(fileAsParent, "api.key"); + + string key = ApiKeyProvider.ResolveKey(Config(), unwritable, NullLogger.Instance); + + key.ShouldMatch("^[0-9a-f]{64}$"); + File.Exists(unwritable).ShouldBeFalse(); + } +} diff --git a/ErsatzTV/Services/ApiKeyProvider.cs b/ErsatzTV/Services/ApiKeyProvider.cs index 7ac9c5d4f..210164cfd 100644 --- a/ErsatzTV/Services/ApiKeyProvider.cs +++ b/ErsatzTV/Services/ApiKeyProvider.cs @@ -30,14 +30,14 @@ public sealed class ApiKeyProvider : IApiKeyProvider // Defense-in-depth default: gate reads too. Operators behind an authenticating proxy who // want anonymous catalog reads can opt out with Api:RequireKeyForReads=false. RequireKeyForReads = configuration.GetValue(RequireKeyForReadsConfigurationKey, true); - ApiKey = ResolveKey(configuration, logger); + ApiKey = ResolveKey(configuration, FileSystemLayout.ApiKeyPath, logger); } public string ApiKey { get; } public bool RequireKeyForReads { get; } - private static string ResolveKey(IConfiguration configuration, ILogger logger) + internal static string ResolveKey(IConfiguration configuration, string keyFilePath, ILogger logger) { string configured = configuration[WriteKeyConfigurationKey]; if (!string.IsNullOrWhiteSpace(configured)) @@ -46,31 +46,41 @@ public sealed class ApiKeyProvider : IApiKeyProvider return configured.Trim(); } - string path = FileSystemLayout.ApiKeyPath; - - try + if (File.Exists(keyFilePath)) { - if (File.Exists(path)) + string existing; + try { - string existing = File.ReadAllText(path).Trim(); - if (!string.IsNullOrWhiteSpace(existing)) - { - logger.LogInformation("Loaded persisted API key from {Path}", path); - return existing; - } + existing = File.ReadAllText(keyFilePath).Trim(); } - } - catch (Exception ex) - { - logger.LogWarning(ex, "Failed to read persisted API key from {Path}; generating a new one", path); + catch (Exception ex) + { + // The file is present but unreadable (e.g. wrong permissions). Fail loud rather than + // silently generate + overwrite it, which would invalidate every client's stored key. + logger.LogError( + ex, + "API key file {Path} exists but could not be read; refusing to overwrite it. Fix its " + + "permissions or set {ConfigurationKey}.", + keyFilePath, + WriteKeyConfigurationKey); + throw; + } + + if (!string.IsNullOrWhiteSpace(existing)) + { + logger.LogInformation("Loaded persisted API key from {Path}", keyFilePath); + return existing; + } + + logger.LogWarning("Persisted API key file {Path} was empty; generating a new key", keyFilePath); } string generated = GenerateKey(); - Persist(path, generated, logger); + Persist(keyFilePath, generated, logger); return generated; } - private static string GenerateKey() => + internal static string GenerateKey() => // 256 bits of entropy, rendered as lowercase hex so it is trivial to copy/paste with no // URL-/header-unsafe characters. Convert.ToHexString(RandomNumberGenerator.GetBytes(32)).ToLowerInvariant(); @@ -85,12 +95,30 @@ public sealed class ApiKeyProvider : IApiKeyProvider Directory.CreateDirectory(directory); } - File.WriteAllText(path, key); - - // Owner read/write only (0600); no-op / unsupported on Windows. - if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) + if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows)) { - File.SetUnixFileMode(path, UnixFileMode.UserRead | UnixFileMode.UserWrite); + File.WriteAllText(path, key); + } + else + { + // Create the file owner-only (0600) up front so the key is never briefly world-readable + // between write and chmod. SetUnixFileMode afterwards re-asserts it if the file pre-existed + // (UnixCreateMode only applies to a newly-created file, not a truncated one). + const UnixFileMode ownerReadWrite = UnixFileMode.UserRead | UnixFileMode.UserWrite; + var options = new FileStreamOptions + { + Mode = FileMode.Create, + Access = FileAccess.Write, + UnixCreateMode = ownerReadWrite + }; + + using (var stream = new FileStream(path, options)) + using (var writer = new StreamWriter(stream)) + { + writer.Write(key); + } + + File.SetUnixFileMode(path, ownerReadWrite); } logger.LogWarning(