From 38164d15cc14778afd5420044841011b9a101887 Mon Sep 17 00:00:00 2001 From: Kenneth Skovhede Date: Tue, 10 Jun 2025 09:40:35 +0200 Subject: [PATCH] Improve secret matching This PR extends the allowed characters in names for secret keys to also include `-` and `_`, allowing keys like `$my-secret-key` which would previously not be matched. This PR also adds detection of partial matches, so keys that start with the prefix, but do not match are reported as a warning. This could happen if the input key is `$my:secret` because the `:` is not matched. To prevent logging sensitive information, only the number of partial matches is reported, not the values. It is possible that the user intended for such a value to be a matched key, but it is also possible that this value was meant to be verbatim. We can consider if this should stop the process instead of just emitting a warning, as the user should fix the issue in either case. The fix for this would be to use the secret provider pattern and change to something that is not partially matched. --- .../Library/Main/SecretProviderHelper.cs | 39 +++++- .../UnitTest/SecretProviderHelperTests.cs | 132 ++++++++++++++---- 2 files changed, 138 insertions(+), 33 deletions(-) diff --git a/Duplicati/Library/Main/SecretProviderHelper.cs b/Duplicati/Library/Main/SecretProviderHelper.cs index 60f5f1eb7..1a23b43cd 100644 --- a/Duplicati/Library/Main/SecretProviderHelper.cs +++ b/Duplicati/Library/Main/SecretProviderHelper.cs @@ -168,7 +168,7 @@ public static class SecretProviderHelper { // Unwrap ${} to support ${name is long} var suffix = string.Empty; - var matcher = @"(\w|/)"; + var matcher = @"(\w|_|-|/)"; if (matchpattern.EndsWith("{}") || matchpattern.EndsWith("()") || matchpattern.EndsWith("[]")) { suffix = matchpattern[^1..]; @@ -179,10 +179,13 @@ public static class SecretProviderHelper // For the values, they could be urls, so we need to look inside the strings var pattern = new Regex(@$"{Regex.Escape(matchpattern)}(?{matcher}+){Regex.Escape(suffix)}", RegexOptions.ExplicitCapture); + // Collect all partial matches, so we can report them later + var partialMatches = new HashSet(StringComparer.OrdinalIgnoreCase); + // When we get the secrets, replace these values var optionsMap = options .Where(x => !x.Key.StartsWith("secret-provider", StringComparison.OrdinalIgnoreCase)) - .Select(x => (x.Key, Secret: GetKey(x.Value, pattern))) + .Select(x => (x.Key, Secret: GetKeyEx(x.Value, pattern, matchpattern, partialMatches))) .Where(x => !string.IsNullOrWhiteSpace(x.Secret)) .GroupBy(x => x.Secret!) .ToDictionary(x => x.Key, x => x.Select(y => y.Key).ToArray()); @@ -192,7 +195,7 @@ public static class SecretProviderHelper .Where(x => !string.IsNullOrWhiteSpace(x.First?.Query)) .Select(x => (Source: x.Second, Params: HttpUtility.ParseQueryString(x.First!.Query))) .SelectMany(x => x.Params.AllKeys.Select(k => (Source: x.Source, Key: k, Value: x.Params[k]))) - .Select(x => (x.Source, x.Key, Secret: GetKey(x.Value, pattern))) + .Select(x => (x.Source, x.Key, Secret: GetKeyEx(x.Value, pattern, matchpattern, partialMatches))) .Where(x => !string.IsNullOrWhiteSpace(x.Secret)) .GroupBy(x => x.Secret!) .ToDictionary(x => x.Key, x => x.Select(y => (y.Source, y.Key)).ToArray()); @@ -203,7 +206,7 @@ public static class SecretProviderHelper .Where(x => !string.IsNullOrWhiteSpace(x.Value.Query)) .Select(x => (Source: x.Source, Params: x.Value.QueryParameters)) .SelectMany(x => x.Params.AllKeys.Select(k => (Source: x.Source, Key: k, Value: x.Params[k]))) - .Select(x => (x.Source, x.Key, Secret: GetKey(x.Value, pattern))) + .Select(x => (x.Source, x.Key, Secret: GetKeyEx(x.Value, pattern, matchpattern, partialMatches))) .Where(x => !string.IsNullOrWhiteSpace(x.Secret)) .GroupBy(x => x.Secret!) .ToDictionary(x => x.Key, x => x.Select(y => (y.Source, y.Key)).ToArray()); @@ -214,6 +217,10 @@ public static class SecretProviderHelper .Distinct() .ToArray(); + // If there are partial matches, log them + if (partialMatches.Count > 0) + Log.WriteWarningMessage(LOGTAG, "SecretProviderPartialMatches", null, "Found {0} partial secret matches where the value starts with the pattern {1}, value transformation not applied to those", partialMatches.Count, matchpattern); + if (secrets.Length == 0) return; @@ -257,6 +264,30 @@ public static class SecretProviderHelper return; } + /// + /// Gets the key from a value using the pattern, and also collects partial matches + /// + /// The value to get the key from + /// The pattern to use + /// The prefix to look for + /// A set to collect partial matches + /// >The key or null if not found + private static string? GetKeyEx(string? value, Regex pattern, string prefix, HashSet partialMatches) + { + if (string.IsNullOrWhiteSpace(value)) + return null; + + var match = GetKey(value, pattern); + if (match != null) + return match; + + // If the value starts with the prefix, but does not match the pattern, it is a partial match + if (value.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + partialMatches.Add(value); + + return null; + } + /// /// Gets the key from a value using the pattern /// diff --git a/Duplicati/UnitTest/SecretProviderHelperTests.cs b/Duplicati/UnitTest/SecretProviderHelperTests.cs index ccc80805b..dfdffb841 100644 --- a/Duplicati/UnitTest/SecretProviderHelperTests.cs +++ b/Duplicati/UnitTest/SecretProviderHelperTests.cs @@ -24,6 +24,7 @@ using System.Collections.Generic; using System.Threading; using System.Threading.Tasks; using Duplicati.Library.Interface; +using Duplicati.Library.Logging; using Duplicati.Library.Main; using Duplicati.Library.Utility; using NUnit.Framework; @@ -71,7 +72,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceSecretsWithDefaultSettings() + public async Task ReplaceSecretsWithDefaultSettings() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -92,7 +93,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=$key2&user=$key1&other=123") }; - SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); Assert.AreEqual("value1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -103,7 +104,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceSecretsWithSlashKeys() + public async Task ReplaceSecretsWithSlashKeys() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -124,7 +125,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=$key/with/slash&user=$key1&other=123") }; - SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); Assert.AreEqual("value1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -135,7 +136,39 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplacesSecretsWithUrlEscaping() + public async Task ReplaceSecretsWithDashKeys() + { + var secretProvider = new MockedSecretProvider(); + secretProvider.Secrets["key1"] = "secret1"; + secretProvider.Secrets["key-with-dash"] = "secret2"; + + var settings = new Dictionary + { + {"key1", "value1"}, + {"key2", "$key-with-dash"}, + {"key3", "value3$key"}, + }; + + var argsSys = new[] { + new System.Uri("test://host/?pass=$key-with-dash&user=$key1&other=123") + }; + + var argsInternal = new[] { + new Library.Utility.Uri("test://host?pass=$key-with-dash&user=$key1&other=123") + }; + + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); + + Assert.AreEqual("value1", settings["key1"]); + Assert.AreEqual("secret2", settings["key2"]); + Assert.AreEqual("value3$key", settings["key3"]); + Assert.AreEqual("test://host/?pass=secret2&user=secret1&other=123", argsSys[0].ToString()); + Assert.AreEqual("test://host?pass=secret2&user=secret1&other=123", argsInternal[0].ToString()); + } + + [Test] + [Category("SecretHelper")] + public async Task ReplacesSecretsWithUrlEscaping() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret 1%&+abc"; @@ -153,7 +186,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=$key1&user=user") }; - SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); // Check no escaping is done for options Assert.AreEqual("secret 1%&+abc", settings["key1"]); @@ -165,7 +198,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceSecretsWithExtendedPattern() + public async Task ReplaceSecretsWithExtendedPattern() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -187,7 +220,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=${key2}&user=${key1}&other=123"), }; - SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); Assert.AreEqual("value1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -198,7 +231,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceSecretsWithExtendedLongPattern() + public async Task ReplaceSecretsWithExtendedLongPattern() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -220,7 +253,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=:sec{key2}&user=:sec{key1}&other=123"), }; - SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); Assert.AreEqual("value1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -231,7 +264,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void CachedProviderRetainsPattern() + public async Task CachedProviderRetainsPattern() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["sec1"] = "secret1"; @@ -242,15 +275,15 @@ public class SecretProviderHelperTests : BasicSetupHelper }; var cachedProvider = SecretProviderHelper.WrapWithCache("", secretProvider, SecretProviderHelper.CachingLevel.InMemory, null, "salt", "!ext{}"); - cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None).Await(); + await cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None); - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); Assert.AreEqual("secret1", settings["key1"]); settings["key1"] = "!ext{sec1}"; - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); Assert.AreEqual("secret1", settings["key1"]); } @@ -276,7 +309,7 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=$key2&user=$key1&other=123"), }; - Assert.Throws(() => SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await()); + Assert.ThrowsAsync(() => SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None)); } [Test] @@ -301,12 +334,12 @@ public class SecretProviderHelperTests : BasicSetupHelper new Library.Utility.Uri("test://host?pass=$key2&user=$key1&other=123"), }; - Assert.Throws(() => SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None).Await()); + Assert.ThrowsAsync(() => SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None)); } [Test] [Category("SecretHelper")] - public void ReplaceUsesInMemoryCache() + public async Task ReplaceUsesInMemoryCache() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -319,16 +352,16 @@ public class SecretProviderHelperTests : BasicSetupHelper }; var cachedProvider = SecretProviderHelper.WrapWithCache("", secretProvider, SecretProviderHelper.CachingLevel.InMemory, null, "salt", null); - cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None).Await(); + await cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None); - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); secretProvider.Secrets = null; settings["key1"] = "$key1"; settings["key2"] = "$key2"; - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); Assert.AreEqual("secret1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -336,7 +369,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceUsesPersistedCache() + public async Task ReplaceUsesPersistedCache() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -350,16 +383,16 @@ public class SecretProviderHelperTests : BasicSetupHelper using var tempFolder = new TempFolder(); var cachedProvider = SecretProviderHelper.WrapWithCache("", secretProvider, SecretProviderHelper.CachingLevel.Persistent, tempFolder, "salt", null); - cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None).Await(); + await cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None); - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); secretProvider.Secrets = null; settings["key1"] = "$key1"; settings["key2"] = "$key2"; - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); Assert.AreEqual("secret1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); @@ -367,7 +400,7 @@ public class SecretProviderHelperTests : BasicSetupHelper [Test] [Category("SecretHelper")] - public void ReplaceUsesPersistedCacheOnRestart() + public async Task ReplaceUsesPersistedCacheOnRestart() { var secretProvider = new MockedSecretProvider(); secretProvider.Secrets["key1"] = "secret1"; @@ -381,9 +414,9 @@ public class SecretProviderHelperTests : BasicSetupHelper using var tempFolder = new TempFolder(); var cachedProvider = SecretProviderHelper.WrapWithCache("", secretProvider, SecretProviderHelper.CachingLevel.Persistent, tempFolder, "salt", null); - cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None).Await(); + await cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None); - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); settings["key1"] = "$key1"; settings["key2"] = "$key2"; @@ -393,12 +426,53 @@ public class SecretProviderHelperTests : BasicSetupHelper secretProvider.Secrets = null; secretProvider.ThrowOnInit = true; - cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None).Await(); + await cachedProvider.InitializeAsync(new System.Uri("mock://"), CancellationToken.None); - SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None).Await(); + await SecretProviderHelper.ApplySecretProviderAsync([], [], settings, null, cachedProvider, CancellationToken.None); Assert.AreEqual("secret1", settings["key1"]); Assert.AreEqual("secret2", settings["key2"]); } + private class LogCapture : ILogDestination + { + public List Messages { get; } = new(); + + public void WriteMessage(LogEntry entry) + { + Messages.Add(entry.AsString(true)); + } + } + + [Test] + [Category("SecretHelper")] + public async Task ReplaceWarnsOnPartialMatches() + { + var secretProvider = new MockedSecretProvider(); + secretProvider.Secrets["key1"] = "secret1"; + secretProvider.Secrets["key2"] = "secret2"; + + var settings = new Dictionary + { + {"key1", "value1"}, + {"key2", "$key)3"}, + }; + + var argsSys = new[] { + new System.Uri("test://host?pass=$key2&user=$key)4&other=123"), + }; + + var argsInternal = new[] { + new Library.Utility.Uri("test://host?pass=$key2&user=$key)5&other=123"), + }; + + var logCapture = new LogCapture(); + using var _ = Log.StartScope(logCapture, LogMessageType.Warning); + await SecretProviderHelper.ApplySecretProviderAsync(argsSys, argsInternal, settings, null, secretProvider, CancellationToken.None); + + Assert.AreEqual("value1", settings["key1"]); + Assert.AreEqual(logCapture.Messages.Count, 1); + Assert.That(logCapture.Messages[0].Contains("Found 3 partial")); + } + }