From f5ff42ca8399c4243c42a449c0ae0abc11b28d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=B8ren=20Eggert=20Lundsteen=20Olsen?= Date: Fri, 2 Oct 2026 13:03:44 +0200 Subject: [PATCH] Review 1.3: an unreadable agent key file is kept, not overwritten LoadOrCreate replaced a key file it could not decrypt, which made the old key unrecoverable even when only the machine (DPAPI) had changed and the file could have been put back. The file is now moved aside as credential-key.bin.unreadable-* and the RSA object of the failed import is disposed. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/servere-og-databaser.md | 2 +- .../Configuration/AgentCredentialKey.cs | 19 +++++++++++++------ .../InstanceConfigurationAgentTests.cs | 2 ++ 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/docs/servere-og-databaser.md b/docs/servere-og-databaser.md index 018eb94..7e31e41 100644 --- a/docs/servere-og-databaser.md +++ b/docs/servere-og-databaser.md @@ -61,7 +61,7 @@ En adgangskode krypteres i webbrugerfladen, i det øjeblik den modtages, med **a - Agentens nøglepar ligger i `C:\ProgramData\SQLInsight\keys`, beskyttet med DPAPI (LocalMachine) og en mappe, kun SYSTEM, Administrators og agentens konto har adgang til. Den offentlige nøgle står i `insight.AgentKeys`. - Den krypterede adgangskode er bundet til server, port og login. Ændres en af dem, skal adgangskoden indtastes igen, så den aldrig kan sendes til en anden server, end den blev indtastet til. -- Geninstalleres agentens server (eller slettes nøglen), laves en ny nøgle. Gemte adgangskoder kan så ikke længere bruges; siden viser det, og de indtastes igen. Windows-godkendelse berøres ikke. +- Geninstalleres agentens server (eller slettes nøglen), laves en ny nøgle. Gemte adgangskoder kan så ikke længere bruges; siden viser det, og de indtastes igen. Windows-godkendelse berøres ikke. En nøglefil, agenten ikke kan læse (fx kopieret fra en anden server), overskrives ikke, men gemmes som `credential-key.bin.unreadable-` ved siden af den nye. Hvorfor ikke ASP.NET Data Protection eller DPAPI alene: agent og web kan ligge på forskellige servere og køre som forskellige konti, og webbrugerfladen skal aldrig kunne læse en adgangskode. Med et nøglepar har webbrugerfladen kun den offentlige nøgle. diff --git a/src/SQLInsight.Agent/Configuration/AgentCredentialKey.cs b/src/SQLInsight.Agent/Configuration/AgentCredentialKey.cs index 07bfe47..82be929 100644 --- a/src/SQLInsight.Agent/Configuration/AgentCredentialKey.cs +++ b/src/SQLInsight.Agent/Configuration/AgentCredentialKey.cs @@ -39,16 +39,19 @@ public sealed partial class AgentCredentialKey : IDisposable /// SubjectPublicKeyInfo (DER): what SQLInsight Web seals passwords with. public byte[] PublicKey { get; } - /// The key in , created the first time. A file that cannot be read is replaced. + /// + /// The key in , created the first time. A file that cannot be decrypted is kept beside + /// the new key (credential-key.bin.unreadable-…), not overwritten, so an administrator can still restore it. + /// public static AgentCredentialKey LoadOrCreate(string directory, ILogger? logger = null) { ArgumentException.ThrowIfNullOrEmpty(directory); var path = Path.Combine(directory, FileName); if (File.Exists(path)) { + var rsa = RSA.Create(); try { - var rsa = RSA.Create(); var pkcs8 = Unprotect(File.ReadAllBytes(path)); try { @@ -63,10 +66,14 @@ public sealed partial class AgentCredentialKey : IDisposable } catch (CryptographicException ex) { - // Copied from another computer, or damaged: passwords sealed for it cannot be opened anyway. + // Copied from another computer (DPAPI LocalMachine), or damaged: passwords sealed for it cannot be opened + // here. Keep the file, in case it is the machine that changed and the file can be put back. + rsa.Dispose(); + var kept = path + ".unreadable-" + DateTime.UtcNow.ToString("yyyyMMddHHmmss", System.Globalization.CultureInfo.InvariantCulture); + File.Move(path, kept, overwrite: true); if (logger is not null) { - LogUnreadable(logger, ex, path); + LogUnreadable(logger, ex, path, kept); } } } @@ -165,6 +172,6 @@ public sealed partial class AgentCredentialKey : IDisposable } } - [LoggerMessage(Level = LogLevel.Warning, Message = "The credential key {Path} could not be read; a new key is created. SQL login passwords entered before must be entered again.")] - private static partial void LogUnreadable(ILogger logger, Exception exception, string path); + [LoggerMessage(Level = LogLevel.Warning, Message = "The credential key {Path} could not be read (kept as {Kept}); a new key is created. SQL login passwords entered before must be entered again.")] + private static partial void LogUnreadable(ILogger logger, Exception exception, string path, string kept); } diff --git a/tests/SQLInsight.Agent.Tests/InstanceConfigurationAgentTests.cs b/tests/SQLInsight.Agent.Tests/InstanceConfigurationAgentTests.cs index da51669..f938021 100644 --- a/tests/SQLInsight.Agent.Tests/InstanceConfigurationAgentTests.cs +++ b/tests/SQLInsight.Agent.Tests/InstanceConfigurationAgentTests.cs @@ -55,6 +55,8 @@ public sealed class AgentCredentialKeyTests : IDisposable using var replaced = AgentCredentialKey.LoadOrCreate(_directory); Assert.NotEqual(oldId, replaced.KeyId); + // The unreadable file is kept beside the new key, not overwritten, in case it can be restored. + Assert.Single(Directory.GetFiles(_directory, AgentCredentialKey.FileName + ".unreadable-*")); } [Fact]