Skip to content

Commit f3170f5

Browse files
committed
Warn user if two apps are sharing the same secrets. Fixes #2740
1 parent 2c6849f commit f3170f5

File tree

4 files changed

+66
-8
lines changed

4 files changed

+66
-8
lines changed

src/WebJobs.Script.WebHost/Properties/Resources.Designer.cs

Lines changed: 10 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/WebJobs.Script.WebHost/Properties/Resources.resx

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,8 +117,11 @@
117117
<resheader name="writer">
118118
<value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
119119
</resheader>
120+
<data name="ErrorSameSecrets" xml:space="preserve">
121+
<value>Two or more function apps are sharing the same secrets ({0})</value>
122+
</data>
120123
<data name="ErrorTooManySecretBackups" xml:space="preserve">
121-
<value>Repository has more than {0} non-decryptable secrets backups ({1}).</value>
124+
<value>Repository has more than {0} non-decryptable secrets backups ({1}). {2}</value>
122125
</data>
123126
<data name="FunctionSecretsSchemaV0" xml:space="preserve">
124127
<value>{

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

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ private async Task PersistSecretsAsync<T>(T secrets, string keyScope = null, boo
387387

388388
if (secretBackups.Length >= ScriptConstants.MaximumSecretBackupCount)
389389
{
390-
string message = string.Format(Resources.ErrorTooManySecretBackups, ScriptConstants.MaximumSecretBackupCount, string.IsNullOrEmpty(keyScope) ? "host" : keyScope);
390+
string message = string.Format(Resources.ErrorTooManySecretBackups, ScriptConstants.MaximumSecretBackupCount, string.IsNullOrEmpty(keyScope) ? "host" : keyScope, await AnalizeSnapshots<T>(secretBackups));
391391
_logger?.LogDebug(message);
392392
throw new InvalidOperationException(message);
393393
}
@@ -451,6 +451,30 @@ private void OnSecretsChanged(object sender, SecretsChangedEventArgs e)
451451
}
452452
}
453453

454+
private async Task<string> AnalizeSnapshots<T>(string[] secretBackups) where T : ScriptSecrets
455+
{
456+
string analizeResult = string.Empty;
457+
try
458+
{
459+
List<T> shapShots = new List<T>();
460+
foreach (string secretPath in secretBackups)
461+
{
462+
string secretString = await _repository.ReadAsync(ScriptSecretsType.Function, Path.GetFileNameWithoutExtension(secretPath));
463+
shapShots.Add(ScriptSecretSerializer.DeserializeSecrets<T>(secretString));
464+
}
465+
string[] hosts = shapShots.Select(x => x.HostName).Distinct().ToArray();
466+
if (hosts.Length > 1)
467+
{
468+
analizeResult = string.Format(Resources.ErrorSameSecrets, string.Join(",", hosts));
469+
}
470+
}
471+
catch
472+
{
473+
// best effort
474+
}
475+
return analizeResult;
476+
}
477+
454478
public async Task PurgeOldSecretsAsync(string rootScriptPath, ILogger logger)
455479
{
456480
if (!Directory.Exists(rootScriptPath))

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

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,13 @@
77
using System.Linq;
88
using System.Security.Cryptography;
99
using System.Threading.Tasks;
10+
using Microsoft.Azure.WebJobs.Logging;
1011
using Microsoft.Azure.WebJobs.Script.Config;
1112
using Microsoft.Azure.WebJobs.Script.WebHost;
1213
using Microsoft.Azure.WebJobs.Script.WebHost.Properties;
14+
using Microsoft.Extensions.Logging;
1315
using Microsoft.Extensions.Logging.Abstractions;
16+
using Microsoft.WebJobs.Script.Tests;
1417
using Moq;
1518
using Newtonsoft.Json;
1619
using WebJobs.Script.Tests;
@@ -548,7 +551,8 @@ public async Task GetHostSecrets_WhenTooManyBackups_ThrowsException()
548551
using (var directory = new TempDirectory())
549552
{
550553
string functionName = "testfunction";
551-
string expectedTraceMessage = string.Format(Resources.ErrorTooManySecretBackups, ScriptConstants.MaximumSecretBackupCount, functionName);
554+
string expectedTraceMessage = string.Format(Resources.ErrorTooManySecretBackups, ScriptConstants.MaximumSecretBackupCount, functionName,
555+
string.Format(Resources.ErrorSameSecrets, "test0,test1"));
552556
string functionSecretsJson =
553557
@"{
554558
'keys': [
@@ -564,13 +568,17 @@ public async Task GetHostSecrets_WhenTooManyBackups_ThrowsException()
564568
}
565569
]
566570
}";
567-
571+
ILoggerFactory loggerFactory = new LoggerFactory();
572+
TestLoggerProvider loggerProvider = new TestLoggerProvider();
573+
loggerFactory.AddProvider(loggerProvider);
574+
var logger = loggerFactory.CreateLogger(LogCategories.CreateFunctionCategory("Test1"));
568575
IDictionary<string, string> functionSecrets;
569576
ISecretsRepository repository = new FileSystemSecretsRepository(directory.Path);
570577

571-
using (var secretManager = new SecretManager(_settingsManager, repository, null))
578+
using (var secretManager = new SecretManager(_settingsManager, repository, logger))
572579
{
573-
await Assert.ThrowsAsync<InvalidOperationException>(async () =>
580+
InvalidOperationException ioe = null;
581+
try
574582
{
575583
for (int i = 0; i < ScriptConstants.MaximumSecretBackupCount + 20; i++)
576584
{
@@ -582,12 +590,26 @@ await Assert.ThrowsAsync<InvalidOperationException>(async () =>
582590
await Task.Delay(500);
583591
}
584592

593+
string hostName = "test" + (i % 2).ToString();
594+
_settingsManager.SetSetting(EnvironmentSettingNames.AzureWebsiteHostName, hostName);
585595
functionSecrets = await secretManager.GetFunctionSecretsAsync(functionName);
586596
}
587-
});
597+
}
598+
catch (InvalidOperationException ex)
599+
{
600+
ioe = ex;
601+
}
602+
finally
603+
{
604+
_settingsManager.SetSetting(EnvironmentSettingNames.AzureWebsiteHostName, null);
605+
}
588606
}
589607

590608
Assert.True(Directory.GetFiles(directory.Path, $"{functionName}.{ScriptConstants.Snapshot}*").Length >= ScriptConstants.MaximumSecretBackupCount);
609+
Assert.True(loggerProvider.GetAllLogMessages().Any(
610+
t => t.Level == LogLevel.Debug && t.FormattedMessage.IndexOf(expectedTraceMessage, StringComparison.OrdinalIgnoreCase) > -1),
611+
"Expected Trace message not found");
612+
591613
}
592614
}
593615

0 commit comments

Comments
 (0)