Fixing secrets retrieval bug (#2861)

mathewc · mathewc · commit 64fe2bba5e88 · 2018-05-21T14:22:55.000-07:00
diff --git a/src/WebJobs.Script.WebHost/Controllers/KeysController.cs b/src/WebJobs.Script.WebHost/Controllers/KeysController.cs
@@ -23,14 +23,12 @@ public class KeysController : ApiController
         private const string MasterKeyName = "_master";
 
         private static readonly Lazy<Dictionary<string, string>> EmptyKeys = new Lazy<Dictionary<string, string>>(() => new Dictionary<string, string>());
-        private readonly WebScriptHostManager _scriptHostManager;
         private readonly ISecretManager _secretManager;
         private readonly TraceWriter _traceWriter;
         private readonly ILogger _logger;
 
-        public KeysController(WebScriptHostManager scriptHostManager, ISecretManager secretManager, TraceWriter traceWriter, ILoggerFactory loggerFactory)
+        public KeysController(ISecretManager secretManager, TraceWriter traceWriter, ILoggerFactory loggerFactory)
         {
-            _scriptHostManager = scriptHostManager;
             _secretManager = secretManager;
             _traceWriter = traceWriter.WithDefaults($"{ScriptConstants.TraceSourceSecretManagement}.Api");
             _logger = loggerFactory?.CreateLogger(ScriptConstants.LogCategoryKeysController);
@@ -40,8 +38,8 @@ public KeysController(WebScriptHostManager scriptHostManager, ISecretManager sec
         [Route("admin/functions/{name}/keys")]
         public async Task<IHttpActionResult> Get(string name)
         {
-            IDictionary<string, string> functionKeys = await GetFunctionKeys(name);
-            return GetKeysResult(functionKeys);
+            var keys = await GetFunctionKeys(name);
+            return GetKeysResult(keys);
         }
 
         [HttpGet]
@@ -50,7 +48,7 @@ public async Task<IHttpActionResult> Get()
         {
             string hostKeyScope = GetHostKeyScopeForRequest();
 
-            Dictionary<string, string> keys = await GetHostSecretsByScope(hostKeyScope);
+            var keys = await GetHostSecretsByScope(hostKeyScope);
             return GetKeysResult(keys);
         }
 
@@ -59,7 +57,7 @@ public async Task<IHttpActionResult> Get()
         public async Task<IHttpActionResult> Get(string functionName, string name)
         {
             IDictionary<string, string> functionKeys = await GetFunctionKeys(functionName);
-            if (functionKeys.TryGetValue(name, out string keyValue))
+            if (functionKeys != null && functionKeys.TryGetValue(name, out string keyValue))
             {
                 var keyResponse = ApiModelUtility.CreateApiModel(new { name = name, value = keyValue }, Request);
                 return Ok(keyResponse);
@@ -88,11 +86,6 @@ public async Task<IHttpActionResult> GetHostKey(string name)
 
         private async Task<IDictionary<string, string>> GetFunctionKeys(string functionName)
         {
-            if (!_scriptHostManager.Instance.IsFunction(functionName))
-            {
-                throw new HttpResponseException(HttpStatusCode.NotFound);
-            }
-
             return await _secretManager.GetFunctionSecretsAsync(functionName);
         }
 
@@ -134,6 +127,11 @@ private string GetHostKeyScopeForRequest()
 
         private IHttpActionResult GetKeysResult(IDictionary<string, string> keys)
         {
+            if (keys == null)
+            {
+                return NotFound();
+            }
+
             keys = keys ?? EmptyKeys.Value;
             var keysContent = new
             {
@@ -157,11 +155,6 @@ private async Task<IHttpActionResult> PutKeyAsync(string keyName, Key key, strin
 
         private async Task<IHttpActionResult> AddOrUpdateSecretAsync(string keyName, string value, string keyScope, ScriptSecretsType secretsType)
         {
-            if (secretsType == ScriptSecretsType.Function && keyScope != null && !_scriptHostManager.Instance.IsFunction(keyScope))
-            {
-                return NotFound();
-            }
-
             KeyOperationResult operationResult;
             if (secretsType == ScriptSecretsType.Host && string.Equals(keyName, MasterKeyName, StringComparison.OrdinalIgnoreCase))
             {
@@ -231,10 +224,9 @@ private async Task<IHttpActionResult> DeleteFunctionSecretAsync(string keyName,
                 return BadRequest("Invalid key name.");
             }
 
-            if ((secretsType == ScriptSecretsType.Function && !_scriptHostManager.Instance.IsFunction(keyScope)) ||
-                !await _secretManager.DeleteSecretAsync(keyName, keyScope, secretsType))
+            if (!await _secretManager.DeleteSecretAsync(keyName, keyScope, secretsType))
             {
-                // Either the function or the key were not found
+                // the key was not found
                 return NotFound();
             }
 
diff --git a/src/WebJobs.Script.WebHost/Security/SecretManager.cs b/src/WebJobs.Script.WebHost/Security/SecretManager.cs
@@ -73,6 +73,7 @@ public async virtual Task<HostSecretsInfo> GetHostSecretsAsync()
 
                 if (hostSecrets == null)
                 {
+                    // host secrets do not yet exist so generate them
                     _traceWriter.Verbose(Resources.TraceHostSecretGeneration);
                     _logger?.LogDebug(Resources.TraceHostSecretGeneration);
                     hostSecrets = GenerateHostSecrets();
@@ -132,6 +133,7 @@ public async virtual Task<IDictionary<string, string>> GetFunctionSecretsAsync(s
                 FunctionSecrets secrets = await LoadFunctionSecretsAsync(functionName);
                 if (secrets == null)
                 {
+                    // no secrets exist for this function so generate them
                     string message = string.Format(Resources.TraceFunctionSecretGeneration, functionName);
                     _traceWriter.Verbose(message, traceProperties);
 
diff --git a/test/WebJobs.Script.Tests/Controllers/Admin/KeysControllerTests.cs b/test/WebJobs.Script.Tests/Controllers/Admin/KeysControllerTests.cs
@@ -26,55 +26,48 @@ public class KeysControllerTests : IDisposable
     {
         private readonly ScriptSettingsManager _settingsManager;
         private readonly TempDirectory _secretsDirectory = new TempDirectory();
-        private Mock<ScriptHost> _hostMock;
-        private Mock<WebScriptHostManager> _managerMock;
         private Collection<FunctionDescriptor> _testFunctions;
-        private Dictionary<string, Collection<string>> _testFunctionErrors;
         private KeysController _testController;
         private Mock<ISecretManager> _secretsManagerMock;
 
         public KeysControllerTests()
         {
             _settingsManager = ScriptSettingsManager.Instance;
             _testFunctions = new Collection<FunctionDescriptor>();
-            _testFunctionErrors = new Dictionary<string, Collection<string>>();
 
             var config = new ScriptHostConfiguration();
             config.TraceWriter = new TestTraceWriter(TraceLevel.Info);
             var environment = new NullScriptHostEnvironment();
             var eventManager = new Mock<IScriptEventManager>();
-            _hostMock = new Mock<ScriptHost>(MockBehavior.Strict, new object[] { environment, eventManager.Object, config, null, null });
-            _hostMock.Setup(p => p.Functions).Returns(_testFunctions);
-            _hostMock.Setup(p => p.FunctionErrors).Returns(_testFunctionErrors);
 
             WebHostSettings settings = new WebHostSettings();
             settings.SecretsPath = _secretsDirectory.Path;
             _secretsManagerMock = new Mock<ISecretManager>(MockBehavior.Strict);
-            _managerMock = new Mock<WebScriptHostManager>(MockBehavior.Strict, new object[] { config, new TestSecretManagerFactory(_secretsManagerMock.Object), eventManager.Object, _settingsManager, settings });
-            _managerMock.SetupGet(p => p.Instance).Returns(_hostMock.Object);
 
             var traceWriter = new TestTraceWriter(TraceLevel.Verbose);
 
-            _testController = new KeysController(_managerMock.Object, _secretsManagerMock.Object, traceWriter, null);
-
-            // setup some test functions
-            string errorFunction = "ErrorFunction";
-            var errors = new Collection<string>();
-            errors.Add("A really really bad error!");
-            _testFunctionErrors.Add(errorFunction, errors);
+            _testController = new KeysController(_secretsManagerMock.Object, traceWriter, null);
 
             var keys = new Dictionary<string, string>
             {
                 { "key1", "secret1" }
             };
-            _secretsManagerMock.Setup(p => p.GetFunctionSecretsAsync(errorFunction, false)).ReturnsAsync(keys);
+            _secretsManagerMock.Setup(p => p.GetFunctionSecretsAsync("TestFunction1", false)).ReturnsAsync(keys);
+
+            keys = new Dictionary<string, string>
+            {
+                { "key1", "secret1" }
+            };
+            _secretsManagerMock.Setup(p => p.GetFunctionSecretsAsync("TestFunction2", false)).ReturnsAsync(keys);
+
+            _secretsManagerMock.Setup(p => p.GetFunctionSecretsAsync("DNE", false)).ReturnsAsync((IDictionary<string, string>)null);
         }
 
         [Fact]
-        public async Task GetKeys_FunctionInError_ReturnsKeys()
+        public async Task GetKeys_ReturnsKeys()
         {
             _testController.Request = new HttpRequestMessage(HttpMethod.Get, "https://local/admin/functions/keys");
-            var result = (OkNegotiatedContentResult<ApiModel>)(await _testController.Get("ErrorFunction"));
+            var result = (OkNegotiatedContentResult<ApiModel>)(await _testController.Get("TestFunction1"));
 
             var content = (JObject)result.Content;
             var keys = content["keys"];
@@ -83,28 +76,36 @@ public async Task GetKeys_FunctionInError_ReturnsKeys()
         }
 
         [Fact]
-        public async Task PutKey_FunctionInError_Succeeds()
+        public async Task GetKeys_NoSecrets_ReturnsNotFound()
+        {
+            var result = await _testController.Get("DNE");
+
+            Assert.True(result is NotFoundResult);
+        }
+
+        [Fact]
+        public async Task PutKey_Succeeds()
         {
             _testController.Request = new HttpRequestMessage(HttpMethod.Get, "https://local/admin/functions/keys/key2");
 
             var key = new Key("key2", "secret2");
             var keyOperationResult = new KeyOperationResult(key.Value, OperationResult.Updated);
-            _secretsManagerMock.Setup(p => p.AddOrUpdateFunctionSecretAsync(key.Name, key.Value, "ErrorFunction", ScriptSecretsType.Function)).ReturnsAsync(keyOperationResult);
+            _secretsManagerMock.Setup(p => p.AddOrUpdateFunctionSecretAsync(key.Name, key.Value, "TestFunction1", ScriptSecretsType.Function)).ReturnsAsync(keyOperationResult);
 
-            var result = (OkNegotiatedContentResult<ApiModel>)(await _testController.Put("ErrorFunction", key.Name, key));
+            var result = (OkNegotiatedContentResult<ApiModel>)(await _testController.Put("TestFunction1", key.Name, key));
             var content = (JObject)result.Content;
             Assert.Equal("key2", content["name"]);
             Assert.Equal("secret2", content["value"]);
         }
 
         [Fact]
-        public async Task DeleteKey_FunctionInError_Succeeds()
+        public async Task DeleteKey_Succeeds()
         {
             _testController.Request = new HttpRequestMessage(HttpMethod.Get, "https://local/admin/functions/keys/key2");
 
-            _secretsManagerMock.Setup(p => p.DeleteSecretAsync("key2", "ErrorFunction", ScriptSecretsType.Function)).ReturnsAsync(true);
+            _secretsManagerMock.Setup(p => p.DeleteSecretAsync("key2", "TestFunction1", ScriptSecretsType.Function)).ReturnsAsync(true);
 
-            var result = (StatusCodeResult)(await _testController.Delete("ErrorFunction", "key2"));
+            var result = (StatusCodeResult)(await _testController.Delete("TestFunction1", "key2"));
             Assert.Equal(HttpStatusCode.NoContent, result.StatusCode);
         }
 

Original file line number	Diff line number	Diff line change
`@@ -23,14 +23,12 @@ public class KeysController : ApiController`
`23`	`23`	`private const string MasterKeyName = "_master";`
`24`	`24`
`25`	`25`	`private static readonly Lazy<Dictionary<string, string>> EmptyKeys = new Lazy<Dictionary<string, string>>(() => new Dictionary<string, string>());`
`26`		`- private readonly WebScriptHostManager _scriptHostManager;`
`27`	`26`	`private readonly ISecretManager _secretManager;`
`28`	`27`	`private readonly TraceWriter _traceWriter;`
`29`	`28`	`private readonly ILogger _logger;`
`30`	`29`
`31`		`- public KeysController(WebScriptHostManager scriptHostManager, ISecretManager secretManager, TraceWriter traceWriter, ILoggerFactory loggerFactory)`
	`30`	`+ public KeysController(ISecretManager secretManager, TraceWriter traceWriter, ILoggerFactory loggerFactory)`
`32`	`31`	`{`
`33`		`- _scriptHostManager = scriptHostManager;`
`34`	`32`	`_secretManager = secretManager;`
`35`	`33`	`_traceWriter = traceWriter.WithDefaults($"{ScriptConstants.TraceSourceSecretManagement}.Api");`
`36`	`34`	`_logger = loggerFactory?.CreateLogger(ScriptConstants.LogCategoryKeysController);`
`@@ -40,8 +38,8 @@ public KeysController(WebScriptHostManager scriptHostManager, ISecretManager sec`
`40`	`38`	`[Route("admin/functions/{name}/keys")]`
`41`	`39`	`public async Task<IHttpActionResult> Get(string name)`
`42`	`40`	`{`
`43`		`- IDictionary<string, string> functionKeys = await GetFunctionKeys(name);`
`44`		`- return GetKeysResult(functionKeys);`
	`41`	`+ var keys = await GetFunctionKeys(name);`
	`42`	`+ return GetKeysResult(keys);`
`45`	`43`	`}`
`46`	`44`
`47`	`45`	`[HttpGet]`
`@@ -50,7 +48,7 @@ public async Task<IHttpActionResult> Get()`
`50`	`48`	`{`
`51`	`49`	`string hostKeyScope = GetHostKeyScopeForRequest();`
`52`	`50`
`53`		`- Dictionary<string, string> keys = await GetHostSecretsByScope(hostKeyScope);`
	`51`	`+ var keys = await GetHostSecretsByScope(hostKeyScope);`
`54`	`52`	`return GetKeysResult(keys);`
`55`	`53`	`}`
`56`	`54`
`@@ -59,7 +57,7 @@ public async Task<IHttpActionResult> Get()`
`59`	`57`	`public async Task<IHttpActionResult> Get(string functionName, string name)`
`60`	`58`	`{`
`61`	`59`	`IDictionary<string, string> functionKeys = await GetFunctionKeys(functionName);`
`62`		`- if (functionKeys.TryGetValue(name, out string keyValue))`
	`60`	`+ if (functionKeys != null && functionKeys.TryGetValue(name, out string keyValue))`
`63`	`61`	`{`
`64`	`62`	`var keyResponse = ApiModelUtility.CreateApiModel(new { name = name, value = keyValue }, Request);`
`65`	`63`	`return Ok(keyResponse);`
`@@ -88,11 +86,6 @@ public async Task<IHttpActionResult> GetHostKey(string name)`
`88`	`86`
`89`	`87`	`private async Task<IDictionary<string, string>> GetFunctionKeys(string functionName)`
`90`	`88`	`{`
`91`		`- if (!_scriptHostManager.Instance.IsFunction(functionName))`
`92`		`- {`
`93`		`- throw new HttpResponseException(HttpStatusCode.NotFound);`
`94`		`- }`
`95`		`-`
`96`	`89`	`return await _secretManager.GetFunctionSecretsAsync(functionName);`
`97`	`90`	`}`
`98`	`91`
`@@ -134,6 +127,11 @@ private string GetHostKeyScopeForRequest()`
`134`	`127`
`135`	`128`	`private IHttpActionResult GetKeysResult(IDictionary<string, string> keys)`
`136`	`129`	`{`
	`130`	`+ if (keys == null)`
	`131`	`+ {`
	`132`	`+ return NotFound();`
	`133`	`+ }`
	`134`	`+`
`137`	`135`	`keys = keys ?? EmptyKeys.Value;`
`138`	`136`	`var keysContent = new`
`139`	`137`	`{`
`@@ -157,11 +155,6 @@ private async Task<IHttpActionResult> PutKeyAsync(string keyName, Key key, strin`
`157`	`155`
`158`	`156`	`private async Task<IHttpActionResult> AddOrUpdateSecretAsync(string keyName, string value, string keyScope, ScriptSecretsType secretsType)`
`159`	`157`	`{`
`160`		`- if (secretsType == ScriptSecretsType.Function && keyScope != null && !_scriptHostManager.Instance.IsFunction(keyScope))`
`161`		`- {`
`162`		`- return NotFound();`
`163`		`- }`
`164`		`-`
`165`	`158`	`KeyOperationResult operationResult;`
`166`	`159`	`if (secretsType == ScriptSecretsType.Host && string.Equals(keyName, MasterKeyName, StringComparison.OrdinalIgnoreCase))`
`167`	`160`	`{`
`@@ -231,10 +224,9 @@ private async Task<IHttpActionResult> DeleteFunctionSecretAsync(string keyName,`
`231`	`224`	`return BadRequest("Invalid key name.");`
`232`	`225`	`}`
`233`	`226`
`234`		`- if ((secretsType == ScriptSecretsType.Function && !_scriptHostManager.Instance.IsFunction(keyScope)) \|\|`
`235`		`- !await _secretManager.DeleteSecretAsync(keyName, keyScope, secretsType))`
	`227`	`+ if (!await _secretManager.DeleteSecretAsync(keyName, keyScope, secretsType))`
`236`	`228`	`{`
`237`		`- // Either the function or the key were not found`
	`229`	`+ // the key was not found`
`238`	`230`	`return NotFound();`
`239`	`231`	`}`
`240`	`232`
Original file line number	Diff line number	Diff line change
`@@ -73,6 +73,7 @@ public async virtual Task<HostSecretsInfo> GetHostSecretsAsync()`
`73`	`73`
`74`	`74`	`if (hostSecrets == null)`
`75`	`75`	`{`
	`76`	`+ // host secrets do not yet exist so generate them`
`76`	`77`	`_traceWriter.Verbose(Resources.TraceHostSecretGeneration);`
`77`	`78`	`_logger?.LogDebug(Resources.TraceHostSecretGeneration);`
`78`	`79`	`hostSecrets = GenerateHostSecrets();`
`@@ -132,6 +133,7 @@ public async virtual Task<IDictionary<string, string>> GetFunctionSecretsAsync(s`
`132`	`133`	`FunctionSecrets secrets = await LoadFunctionSecretsAsync(functionName);`
`133`	`134`	`if (secrets == null)`
`134`	`135`	`{`
	`136`	`+ // no secrets exist for this function so generate them`
`135`	`137`	`string message = string.Format(Resources.TraceFunctionSecretGeneration, functionName);`
`136`	`138`	`_traceWriter.Verbose(message, traceProperties);`
`137`	`139`