Skip to content

Commit 0238064

Browse files
mathewcmathewc
committed
SecretManager cache improvements #549 
1 parent eb1138c commit 0238064

File tree

2 files changed

+175
-7
lines changed

2 files changed

+175
-7
lines changed

‎src/WebJobs.Script.WebHost/Security/SecretManager.cs‎

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost
2121
{
2222
public class SecretManager : IDisposable, ISecretManager
2323
{
24-
private readonly ConcurrentDictionary<string, Dictionary<string, string>> _secretsMap = new ConcurrentDictionary<string, Dictionary<string, string>>();
24+
private readonly ConcurrentDictionary<string, Dictionary<string, string>> _secretsMap = new ConcurrentDictionary<string, Dictionary<string, string>>(StringComparer.OrdinalIgnoreCase);
2525
private readonly IKeyValueConverterFactory _keyValueConverterFactory;
2626
private readonly TraceWriter _traceWriter;
2727
private readonly ISecretsRepository _repository;
@@ -149,8 +149,8 @@ public async virtual Task<IDictionary<string, string>> GetFunctionSecretsAsync(s
149149
if (secrets == null)
150150
{
151151
// no secrets exist for this function so generate them
152-
string messageGeneratoin = string.Format(Resources.TraceFunctionSecretGeneration, functionName);
153-
_traceWriter.Info(messageGeneratoin, traceProperties);
152+
string messageGeneration = string.Format(Resources.TraceFunctionSecretGeneration, functionName);
153+
_traceWriter.Info(messageGeneration, traceProperties);
154154
secrets = GenerateFunctionSecrets();
155155

156156
await PersistSecretsAsync(secrets, functionName);
@@ -471,11 +471,15 @@ private async Task PersistSecretsAsync<T>(T secrets, string keyScope = null, boo
471471
// We want to store encryption keys hashes to investigate sudden regenerations
472472
string hashes = GetEncryptionKeysHashes();
473473
secrets.DecryptionKeyId = hashes;
474-
string message = $"Encription keys hashes: {hashes}";
474+
string message = $"Encryption keys hashes: {hashes}";
475475
_traceWriter.Info(message);
476476

477477
await _repository.WriteAsync(secretsType, keyScope, ScriptSecretSerializer.SerializeSecrets<T>(secrets));
478478
}
479+
480+
// do a direct/immediate cache update to avoid race conditions with
481+
// file based notifications.
482+
ClearCacheOnChange(secrets.SecretsType, keyScope);
479483
}
480484

481485
private HostSecrets ReadHostSecrets(HostSecrets hostSecrets)
@@ -516,17 +520,32 @@ internal static string GenerateSecret()
516520
}
517521

518522
private void OnSecretsChanged(object sender, SecretsChangedEventArgs e)
523+
{
524+
ClearCacheOnChange(e.SecretsType, e.Name);
525+
}
526+
527+
private void ClearCacheOnChange(ScriptSecretsType secretsType, string functionName)
519528
{
520529
// clear the cached secrets if they exist
521530
// they'll be reloaded on demand next time
522-
if (e.SecretsType == ScriptSecretsType.Host)
531+
if (secretsType == ScriptSecretsType.Host &&
532+
_hostSecrets != null)
523533
{
534+
_traceWriter.Info("Host keys change detected. Clearing cache.");
524535
_hostSecrets = null;
525536
}
526537
else
527538
{
528-
Dictionary<string, string> secrets;
529-
_secretsMap.TryRemove(e.Name, out secrets);
539+
if (!string.IsNullOrEmpty(functionName) && _secretsMap.ContainsKey(functionName))
540+
{
541+
_traceWriter.Info($"Function keys change detected. Clearing cache for function '{functionName}'.");
542+
_secretsMap.TryRemove(functionName, out _);
543+
}
544+
else if (_secretsMap.Any())
545+
{
546+
_traceWriter.Info("Function keys change detected. Clearing all cached function keys.");
547+
_secretsMap.Clear();
548+
}
530549
}
531550
}
532551

‎test/WebJobs.Script.Tests/Security/SecretManagerTests.cs‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -282,6 +282,155 @@ public async Task AddOrUpdateFunctionSecrets_WithFunctionNameAndNoSecret_Generat
282282
}
283283
}
284284

285+
[Fact]
286+
public async Task AddOrUpdateFunctionSecret_ClearsCache_WhenFunctionSecretAdded()
287+
{
288+
using (var directory = new TempDirectory())
289+
{
290+
CreateTestSecrets(directory.Path);
291+
292+
Mock<IKeyValueConverterFactory> mockValueConverterFactory = GetConverterFactoryMock(false);
293+
KeyOperationResult result;
294+
var traceWriter = new TestTraceWriter(TraceLevel.Verbose);
295+
ISecretsRepository repository = new FileSystemSecretsRepository(directory.Path);
296+
using (var secretManager = new SecretManager(repository, mockValueConverterFactory.Object, traceWriter))
297+
{
298+
var keys = await secretManager.GetFunctionSecretsAsync("testfunction");
299+
Assert.Equal(2, keys.Count);
300+
301+
// add a new key
302+
result = await secretManager.AddOrUpdateFunctionSecretAsync("function-key-3", "9876", "TestFunction", ScriptSecretsType.Function);
303+
}
304+
305+
string secretsJson = File.ReadAllText(Path.Combine(directory.Path, "testfunction.json"));
306+
var persistedSecrets = ScriptSecretSerializer.DeserializeSecrets<FunctionSecrets>(secretsJson);
307+
308+
Assert.Equal(OperationResult.Created, result.Result);
309+
Assert.Equal(result.Secret, "9876");
310+
311+
var logs = traceWriter.GetTraces();
312+
Assert.Equal(1, logs.Count(p => string.Equals(p.Message, "Function keys change detected. Clearing cache for function 'TestFunction'.", StringComparison.OrdinalIgnoreCase)));
313+
Assert.Equal(1, logs.Count(p => p.Message == "Function secret 'function-key-3' for 'TestFunction' Created."));
314+
}
315+
}
316+
317+
[Fact]
318+
public async Task AddOrUpdateFunctionSecret_ClearsCache_WhenHostLevelFunctionSecretAdded()
319+
{
320+
using (var directory = new TempDirectory())
321+
{
322+
CreateTestSecrets(directory.Path);
323+
324+
Mock<IKeyValueConverterFactory> mockValueConverterFactory = GetConverterFactoryMock(false);
325+
KeyOperationResult result;
326+
var traceWriter = new TestTraceWriter(TraceLevel.Verbose);
327+
ISecretsRepository repository = new FileSystemSecretsRepository(directory.Path);
328+
using (var secretManager = new SecretManager(repository, mockValueConverterFactory.Object, traceWriter))
329+
{
330+
var hostKeys = await secretManager.GetHostSecretsAsync();
331+
Assert.Equal(2, hostKeys.FunctionKeys.Count);
332+
333+
// add a new key
334+
result = await secretManager.AddOrUpdateFunctionSecretAsync("function-host-3", "9876", HostKeyScopes.FunctionKeys, ScriptSecretsType.Host);
335+
}
336+
337+
string secretsJson = File.ReadAllText(Path.Combine(directory.Path, "host.json"));
338+
var persistedSecrets = ScriptSecretSerializer.DeserializeSecrets<HostSecrets>(secretsJson);
339+
340+
Assert.Equal(OperationResult.Created, result.Result);
341+
Assert.Equal(result.Secret, "9876");
342+
343+
var logs = traceWriter.GetTraces();
344+
Assert.Equal(1, logs.Count(p => p.Message == "Host keys change detected. Clearing cache."));
345+
Assert.Equal(1, logs.Count(p => p.Message == "Host secret 'function-host-3' for 'functionkeys' Created."));
346+
}
347+
}
348+
349+
[Fact]
350+
public async Task AddOrUpdateFunctionSecret_ClearsCache_WhenHostSystemSecretAdded()
351+
{
352+
using (var directory = new TempDirectory())
353+
{
354+
CreateTestSecrets(directory.Path);
355+
356+
Mock<IKeyValueConverterFactory> mockValueConverterFactory = GetConverterFactoryMock(false);
357+
KeyOperationResult result;
358+
var traceWriter = new TestTraceWriter(TraceLevel.Verbose);
359+
ISecretsRepository repository = new FileSystemSecretsRepository(directory.Path);
360+
using (var secretManager = new SecretManager(repository, mockValueConverterFactory.Object, traceWriter))
361+
{
362+
var hostKeys = await secretManager.GetHostSecretsAsync();
363+
Assert.Equal(2, hostKeys.SystemKeys.Count);
364+
365+
// add a new key
366+
result = await secretManager.AddOrUpdateFunctionSecretAsync("host-system-3", "123", HostKeyScopes.SystemKeys, ScriptSecretsType.Host);
367+
}
368+
369+
string secretsJson = File.ReadAllText(Path.Combine(directory.Path, "host.json"));
370+
var persistedSecrets = ScriptSecretSerializer.DeserializeSecrets<HostSecrets>(secretsJson);
371+
372+
Assert.Equal(OperationResult.Created, result.Result);
373+
Assert.Equal(result.Secret, "123");
374+
375+
var logs = traceWriter.GetTraces();
376+
Assert.Equal(1, logs.Count(p => p.Message == "Host keys change detected. Clearing cache."));
377+
Assert.Equal(1, logs.Count(p => p.Message == "Host secret 'host-system-3' for 'systemkeys' Created."));
378+
}
379+
}
380+
381+
private void CreateTestSecrets(string path)
382+
{
383+
string hostSecrets =
384+
@"{
385+
'masterKey': {
386+
'name': 'master',
387+
'value': '1234',
388+
'encrypted': false
389+
},
390+
'functionKeys': [
391+
{
392+
'name': 'function-host-1',
393+
'value': '456',
394+
'encrypted': false
395+
},
396+
{
397+
'name': 'function-host-2',
398+
'value': '789',
399+
'encrypted': false
400+
}
401+
],
402+
'systemKeys': [
403+
{
404+
'name': 'host-system-1',
405+
'value': '654',
406+
'encrypted': false
407+
},
408+
{
409+
'name': 'host-system-2',
410+
'value': '321',
411+
'encrypted': false
412+
}
413+
]
414+
}";
415+
string functionSecrets =
416+
@"{
417+
'keys': [
418+
{
419+
'name': 'function-key-1',
420+
'value': '1234',
421+
'encrypted': false
422+
},
423+
{
424+
'name': 'function-key-2',
425+
'value': '5678',
426+
'encrypted': false
427+
}
428+
]
429+
}";
430+
File.WriteAllText(Path.Combine(path, ScriptConstants.HostMetadataFileName), hostSecrets);
431+
File.WriteAllText(Path.Combine(path, "testfunction.json"), functionSecrets);
432+
}
433+
285434
[Fact]
286435
public async Task AddOrUpdateFunctionSecrets_WithFunctionNameAndNoSecret_EncryptsSecretAndPersistsFile()
287436
{

0 commit comments

Comments
 (0)