From d636e59e5653636fcf1897d16d825456c580cc57 Mon Sep 17 00:00:00 2001 From: Aishwarya Bhandari Date: Fri, 28 Jul 2023 15:41:42 -0700 Subject: [PATCH 1/5] changes --- .../Security/KeyManagement/SecretManager.cs | 24 +++++++-- .../Security/SecretManagerTests.cs | 51 ++++++++++++++++++- 2 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs index fd31240bb3..3d8e890030 100644 --- a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs +++ b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs @@ -90,12 +90,26 @@ public async virtual Task GetHostSecretsAsync() _logger.LogDebug("Loading host secrets"); hostSecrets = await LoadSecretsAsync(); - if (hostSecrets == null) + try + { + if (hostSecrets == null) + { + // host secrets do not yet exist so generate them + _logger.LogDebug(Resources.TraceHostSecretGeneration); + hostSecrets = GenerateHostSecrets(); + await PersistSecretsAsync(hostSecrets); + } + } + catch (Exception ex) { - // host secrets do not yet exist so generate them - _logger.LogDebug(Resources.TraceHostSecretGeneration); - hostSecrets = GenerateHostSecrets(); - await PersistSecretsAsync(hostSecrets); + _logger.LogDebug("Trying again to ensure potential race condition is handled correctly"); + hostSecrets = await LoadSecretsAsync(); + + if (hostSecrets == null) + { + _logger.LogError(ex, "Failed to load host secrets on second attempt"); + throw; + } } try diff --git a/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs b/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs index a73e7ea860..44912798db 100644 --- a/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs +++ b/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs @@ -447,6 +447,41 @@ public async Task GetHostSecretsAsync_SecretGenerationIsSerialized() } } + [Fact] + public async Task SecretsRepository_SimultaneousCreates_Throws_Conflict() + { + var mockValueConverterFactory = GetConverterFactoryMock(false, false); + var metricsLogger = new TestMetricsLogger(); + + // Test repository that will fail on WriteAsync() due to a conflict, but will replicate a success when LoadSecretsAsync() is called + // indicating that there was race condition + var testRepository = new TestSecretsRepository(true, true, true); + string testFunctionName = "host"; + + using (var secretManager = new SecretManager(testRepository, mockValueConverterFactory.Object, _logger, metricsLogger, _hostNameProvider, _startupContextProvider)) + { + var tasks = new List>(); + for (int i = 0; i < 2; i++) + { + tasks.Add(secretManager.GetHostSecretsAsync()); + } + + // Ensure nothing is there. + HostSecrets secretsContent = await testRepository.ReadAsync(ScriptSecretsType.Host, testFunctionName) as HostSecrets; + Assert.Null(secretsContent); + await Task.WhenAll(tasks); + + // verify all calls return the same result + var masterKey = tasks.First().Result.MasterKey; + var functionKey = tasks.First().Result.FunctionKeys.First(); + Assert.True(tasks.Select(p => p.Result).All(q => q.MasterKey == masterKey)); + Assert.True(tasks.Select(p => p.Result).All(q => q.FunctionKeys.First().Value == functionKey.Value)); + + // verify generated master and function keys are valid + tasks.Select(p => p.Result).All(q => ValidateHostSecrets(q)); + } + } + [Fact] public async Task GetHostSecrets_WhenNoHostSecretFileExists_GeneratesSecretsAndPersistsFiles() { @@ -532,7 +567,7 @@ public async Task AddOrUpdateFunctionSecret_WhenStorageWriteError_ThrowsExceptio KeyOperationResult result; - ISecretsRepository repository = new TestSecretsRepository(false, true, HttpStatusCode.InternalServerError); + ISecretsRepository repository = new TestSecretsRepository(false, true, false, HttpStatusCode.InternalServerError); using (var secretManager = CreateSecretManager(directory.Path, simulateWriteConversion: false, secretsRepository: repository)) { try @@ -1265,6 +1300,7 @@ private class TestSecretsRepository : ISecretsRepository private Random _rand = new Random(); private bool _enforceSerialWrites = false; private bool _forceWriteErrors = false; + private bool _shouldSuceedAfterFailing = false; private HttpStatusCode _httpstaus; public TestSecretsRepository(bool enforceSerialWrites) @@ -1272,10 +1308,11 @@ public TestSecretsRepository(bool enforceSerialWrites) _enforceSerialWrites = enforceSerialWrites; } - public TestSecretsRepository(bool enforceSerialWrites, bool forceWriteErrors, HttpStatusCode httpstaus = HttpStatusCode.InternalServerError) + public TestSecretsRepository(bool enforceSerialWrites, bool forceWriteErrors, bool shouldSucceedAfterFailing = false, HttpStatusCode httpstaus = HttpStatusCode.InternalServerError) : this(enforceSerialWrites) { _forceWriteErrors = forceWriteErrors; + _shouldSuceedAfterFailing = shouldSucceedAfterFailing; _httpstaus = httpstaus; } @@ -1319,6 +1356,11 @@ public async Task WriteAsync(ScriptSecretsType type, string functionName, Script { if (_forceWriteErrors) { + // Replicate making the first write fail, but succeed on the second attempt + if (_shouldSuceedAfterFailing) + { + await WriteAsyncHelper(type, functionName, secrets); + } throw new RequestFailedException((int)_httpstaus, "Error"); } @@ -1327,6 +1369,11 @@ public async Task WriteAsync(ScriptSecretsType type, string functionName, Script throw new Exception("Concurrent writes detected!"); } + await WriteAsyncHelper(type, functionName, secrets); + } + + private async Task WriteAsyncHelper(ScriptSecretsType type, string functionName, ScriptSecrets secrets) + { Interlocked.Increment(ref _writeCount); await Task.Delay(_rand.Next(100, 300)); From a3e3c31dd924246424f4e710a067ddb28f16d7c0 Mon Sep 17 00:00:00 2001 From: Aishwarya Bhandari <37918412+aishwaryabh@users.noreply.github.com> Date: Tue, 1 Aug 2023 11:18:27 -0700 Subject: [PATCH 2/5] Update src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs Co-authored-by: Lilian Kasem --- .../Security/KeyManagement/SecretManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs index 3d8e890030..192fd7421f 100644 --- a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs +++ b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs @@ -102,7 +102,7 @@ public async virtual Task GetHostSecretsAsync() } catch (Exception ex) { - _logger.LogDebug("Trying again to ensure potential race condition is handled correctly"); + _logger.LogDebug("Potential conflict, attempting to load host secrets for the second time."); hostSecrets = await LoadSecretsAsync(); if (hostSecrets == null) From d2a397d9faeb25ca3ec971d74476a3323bd95bb0 Mon Sep 17 00:00:00 2001 From: Aishwarya Bhandari Date: Wed, 2 Aug 2023 11:19:25 -0700 Subject: [PATCH 3/5] adding additional check --- .../Security/KeyManagement/SecretManager.cs | 28 ++++++++++++++----- .../Security/SecretManagerTests.cs | 27 ++++++++++++++++++ 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs index 192fd7421f..f89efd3f5f 100644 --- a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs +++ b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs @@ -16,7 +16,6 @@ using Microsoft.Azure.WebJobs.Script.WebHost.Properties; using Microsoft.Azure.WebJobs.Script.WebHost.Security; using Microsoft.Extensions.Logging; - using DataProtectionConstants = Microsoft.Azure.Web.DataProtection.Constants; namespace Microsoft.Azure.WebJobs.Script.WebHost @@ -172,14 +171,29 @@ public async virtual Task> GetFunctionSecretsAsync(s _logger.LogDebug($"Loading secrets for function '{functionName}'"); FunctionSecrets secrets = await LoadFunctionSecretsAsync(functionName); - if (secrets == null) + + try { - // no secrets exist for this function so generate them - string message = string.Format(Resources.TraceFunctionSecretGeneration, functionName); - _logger.LogDebug(message); - secrets = GenerateFunctionSecrets(); + if (secrets == null) + { + // no secrets exist for this function so generate them + string message = string.Format(Resources.TraceFunctionSecretGeneration, functionName); + _logger.LogDebug(message); + secrets = GenerateFunctionSecrets(); - await PersistSecretsAsync(secrets, functionName); + await PersistSecretsAsync(secrets, functionName); + } + } + catch (Exception ex) + { + _logger.LogDebug("Potential conflict, attempting to load function secrets for the second time."); + secrets = await LoadFunctionSecretsAsync(functionName); + + if (secrets == null) + { + _logger.LogError(ex, "Failed to load function secrets on second attempt"); + throw; + } } try diff --git a/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs b/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs index 44912798db..1ab4e2397c 100644 --- a/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs +++ b/test/WebJobs.Script.Tests/Security/SecretManagerTests.cs @@ -482,6 +482,33 @@ public async Task SecretsRepository_SimultaneousCreates_Throws_Conflict() } } + [Fact] + public async Task FunctionSecrets_SimultaneousCreates_Throws_Conflict() + { + var mockValueConverterFactory = GetConverterFactoryMock(false, false); + var metricsLogger = new TestMetricsLogger(); + var testRepository = new TestSecretsRepository(true, true, true); + string testFunctionName = $"TestFunction"; + + using (var secretManager = new SecretManager(testRepository, mockValueConverterFactory.Object, _logger, metricsLogger, _hostNameProvider, _startupContextProvider)) + { + var tasks = new List>>(); + for (int i = 0; i < 2; i++) + { + tasks.Add(secretManager.GetFunctionSecretsAsync(testFunctionName)); + } + + await Task.WhenAll(tasks); + + // verify all calls return the same result + Assert.Equal(1, testRepository.FunctionSecrets.Count); + var functionSecrets = (FunctionSecrets)testRepository.FunctionSecrets[testFunctionName]; + string defaultKeyValue = functionSecrets.Keys.Where(p => p.Name == "default").Single().Value; + SecretGeneratorTests.ValidateSecret(defaultKeyValue, SecretGenerator.FunctionKeySeed); + Assert.True(tasks.Select(p => p.Result).All(t => t["default"] == defaultKeyValue)); + } + } + [Fact] public async Task GetHostSecrets_WhenNoHostSecretFileExists_GeneratesSecretsAndPersistsFiles() { From 943d62ae48171223dd8c2561a0e61c24fe57fa51 Mon Sep 17 00:00:00 2001 From: Aishwarya Bhandari Date: Thu, 3 Aug 2023 14:24:24 -0700 Subject: [PATCH 4/5] changing wording --- .../Security/KeyManagement/SecretManager.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs index f89efd3f5f..d2e8f206f6 100644 --- a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs +++ b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs @@ -11,10 +11,13 @@ using System.Text; using System.Threading; using System.Threading.Tasks; +using DryIoc; +using Microsoft.Azure.Web.DataProtection; using Microsoft.Azure.WebJobs.Extensions.Http; using Microsoft.Azure.WebJobs.Script.Diagnostics; using Microsoft.Azure.WebJobs.Script.WebHost.Properties; using Microsoft.Azure.WebJobs.Script.WebHost.Security; +using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; using DataProtectionConstants = Microsoft.Azure.Web.DataProtection.Constants; @@ -101,12 +104,12 @@ public async virtual Task GetHostSecretsAsync() } catch (Exception ex) { - _logger.LogDebug("Potential conflict, attempting to load host secrets for the second time."); + _logger.LogDebug(ex, "Exception while generating host secrets. This can happen if another instance is also generating secrets. Attempting to read secrets again."); hostSecrets = await LoadSecretsAsync(); if (hostSecrets == null) { - _logger.LogError(ex, "Failed to load host secrets on second attempt"); + _logger.LogError("Host secrets are still null on second attempt."); throw; } } From 1c26a668024f079706246085e6ea6032d4ffd891 Mon Sep 17 00:00:00 2001 From: Aishwarya Bhandari Date: Fri, 4 Aug 2023 10:34:35 -0700 Subject: [PATCH 5/5] changing exception wording for generating function secrets --- .../Security/KeyManagement/SecretManager.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs index d2e8f206f6..cb0d1d0d43 100644 --- a/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs +++ b/src/WebJobs.Script.WebHost/Security/KeyManagement/SecretManager.cs @@ -189,12 +189,12 @@ public async virtual Task> GetFunctionSecretsAsync(s } catch (Exception ex) { - _logger.LogDebug("Potential conflict, attempting to load function secrets for the second time."); + _logger.LogDebug(ex, "Exception while generating function secrets. This can happen if another instance is also generating secrets. Attempting to read secrets again."); secrets = await LoadFunctionSecretsAsync(functionName); if (secrets == null) { - _logger.LogError(ex, "Failed to load function secrets on second attempt"); + _logger.LogError("Function secrets are still null on second attempt."); throw; } }