Add resource filter for ARM Extension API requests

mathewc · mathewc · commit eb1138c60314 · 2019-12-09T10:14:08.000-08:00
diff --git a/src/WebJobs.Script.WebHost/App_Start/WebApiConfig.cs b/src/WebJobs.Script.WebHost/App_Start/WebApiConfig.cs
@@ -8,6 +8,7 @@
 using Autofac.Integration.WebApi;
 using Microsoft.Azure.WebJobs.Script.Config;
 using Microsoft.Azure.WebJobs.Script.WebHost.Controllers;
+using Microsoft.Azure.WebJobs.Script.WebHost.Filters;
 using Microsoft.Azure.WebJobs.Script.WebHost.Handlers;
 
 namespace Microsoft.Azure.WebJobs.Script.WebHost
@@ -41,14 +42,14 @@ public static void Register(HttpConfiguration config, ScriptSettingsManager sett
             // Invoke registration callback
             dependencyCallback?.Invoke(builder, settings);
 
+            // Web API configuration and services
             var container = builder.Build();
             config.DependencyResolver = new AutofacWebApiDependencyResolver(container);
             config.Formatters.Add(new PlaintextMediaTypeFormatter());
             config.Services.Replace(typeof(IExceptionHandler), new ExceptionProcessingHandler(config));
             AddMessageHandlers(config);
-
-            // Web API configuration and services
-
+            AddFilters(config);
+            
             // Web API routes
             config.MapHttpAttributeRoutes();
 
@@ -74,6 +75,11 @@ public static void Register(HttpConfiguration config, ScriptSettingsManager sett
             config.InitializeReceiveSalesforceWebHooks();
         }
 
+        private static void AddFilters(HttpConfiguration config)
+        {
+            config.Filters.Add(new ArmExtensionResourceFilter());
+        }
+
         private static void AddMessageHandlers(HttpConfiguration config)
         {
             config.MessageHandlers.Add(new SystemTraceHandler(config));
diff --git a/src/WebJobs.Script.WebHost/Controllers/KeysController.cs b/src/WebJobs.Script.WebHost/Controllers/KeysController.cs
@@ -21,6 +21,7 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost.Controllers
 {
     [JwtAuthentication]
     [AuthorizationLevel(AuthorizationLevel.Admin)]
+    [ResourceContainsSecrets]
     public class KeysController : ApiController
     {
         private const string MasterKeyName = "_master";
diff --git a/src/WebJobs.Script.WebHost/Filters/ArmExtensionResourceFilter.cs b/src/WebJobs.Script.WebHost/Filters/ArmExtensionResourceFilter.cs
@@ -0,0 +1,51 @@
+﻿using System;
+using System.Net;
+using System.Net.Http;
+using System.Threading;
+using System.Threading.Tasks;
+using System.Web.Http.Controllers;
+using System.Web.Http.Filters;
+using Microsoft.Azure.WebJobs.Script.WebHost.Properties;
+
+namespace Microsoft.Azure.WebJobs.Script.WebHost.Filters
+{
+    /// <summary>
+    /// Resource filter used to ensure secrets aren't returned for GET requests made via the Functions ARM extension
+    /// API (hostruntime), unless properly authorized.
+    /// </summary>
+    /// <remarks>
+    /// All our first class ARM APIs handle RBAC naturally. For the hostruntime bridge, the runtime collaborates
+    /// based on request details coming from ARM/Geo.
+    /// </remarks>
+    public sealed class ArmExtensionResourceFilter : IActionFilter
+    {
+        public bool AllowMultiple => false;
+
+        public async Task<HttpResponseMessage> ExecuteActionFilterAsync(HttpActionContext actionContext, CancellationToken cancellationToken, Func<Task<HttpResponseMessage>> continuation)
+        {
+            // We only want to apply this filter for GET extension ARM requests that were forwarded directly to us via
+            // hostruntime bridge, not hostruntime requests initiated internally by the geomaster. The latter requests
+            // won't have the x-ms-arm-request-tracking-id header.
+            var request = actionContext.Request;
+            bool isArmExtensionRequest = request.HasHeader(ScriptConstants.AntaresARMRequestTrackingIdHeader) &&
+                request.HasHeader(ScriptConstants.AntaresARMExtensionsRouteHeader);
+
+            if (isArmExtensionRequest && request.Method == HttpMethod.Get)
+            {
+                // requests made by owner/co-admin are not filtered
+                if (!request.HasHeaderValue(ScriptConstants.AntaresClientAuthorizationSourceHeader, "legacy"))
+                {
+                    var actionDescriptor = actionContext.ActionDescriptor as ReflectedHttpActionDescriptor;
+                    if (actionDescriptor != null && actionDescriptor.MethodInfo != null &&
+                        Utility.GetHierarchicalAttributeOrNull<ResourceContainsSecretsAttribute>(actionDescriptor.MethodInfo) != null)
+                    {
+                        // if the resource returned by the action contains secrets, fail the request
+                        return request.CreateResponse(HttpStatusCode.Unauthorized, Resources.UnauthorizedArmExtensionResourceRequest);
+                    }
+                }
+            }
+
+            return await continuation();
+        }
+    }
+}
diff --git a/src/WebJobs.Script.WebHost/GlobalSuppressions.cs b/src/WebJobs.Script.WebHost/GlobalSuppressions.cs
@@ -130,4 +130,5 @@
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Sas", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.WebHost.BlobStorageSasSecretsRepository.#.ctor(System.String,System.String,System.String)")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Design", "CA1054:UriParametersShouldNotBeStrings", MessageId = "1#", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.WebHost.BlobStorageSasSecretsRepository.#.ctor(System.String,System.String,System.String)")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Usage", "CA2214:DoNotCallOverridableMethodsInConstructors", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.WebHost.BlobStorageSecretsRepository.#.ctor(System.String,System.String,System.String)")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1703:ResourceStringsShouldBeSpelledCorrectly", MessageId = "hostruntime", Scope = "resource", Target = "Microsoft.Azure.WebJobs.Script.WebHost.Properties.Resources.resources")]
 
diff --git a/src/WebJobs.Script.WebHost/Properties/Resources.Designer.cs b/src/WebJobs.Script.WebHost/Properties/Resources.Designer.cs
diff --git a/src/WebJobs.Script.WebHost/Properties/Resources.resx b/src/WebJobs.Script.WebHost/Properties/Resources.resx
@@ -319,4 +319,7 @@
   <data name="TraceStaleHostSecretRefresh" xml:space="preserve">
     <value>Stale host secrets detected. Refreshing secrets.</value>
   </data>
+  <data name="UnauthorizedArmExtensionResourceRequest" xml:space="preserve">
+    <value>GET requests for this resource via the hostruntime extensions API are not authorized. Please use an alternate first class ARM API.</value>
+  </data>
 </root>
diff --git a/src/WebJobs.Script.WebHost/ResourceContainsSecretsAttribute.cs b/src/WebJobs.Script.WebHost/ResourceContainsSecretsAttribute.cs
@@ -0,0 +1,16 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+using System;
+
+namespace Microsoft.Azure.WebJobs.Script.WebHost
+{
+    /// <summary>
+    /// Attribute applied to actions to indicate whether the resource returned by an action
+    /// contains secrets.
+    /// </summary>
+    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
+    public sealed class ResourceContainsSecretsAttribute : Attribute
+    {
+    }
+}
diff --git a/src/WebJobs.Script.WebHost/WebJobs.Script.WebHost.csproj b/src/WebJobs.Script.WebHost/WebJobs.Script.WebHost.csproj
@@ -508,6 +508,7 @@
     <Compile Include="Extensions\HttpRouteCollectionExtensions.cs" />
     <Compile Include="Extensions\HttpRouteFactoryExtensions.cs" />
     <Compile Include="Extensions\IEnumerableTasksExtensions.cs" />
+    <Compile Include="Filters\ArmExtensionResourceFilter.cs" />
     <Compile Include="Filters\EnableDebugModeAttribute.cs" />
     <Compile Include="Filters\RequiresRunningHostAttribute.cs" />
     <Compile Include="HostNameProvider.cs" />
@@ -517,6 +518,7 @@
     <Compile Include="Management\SyncTriggersResult.cs" />
     <Compile Include="Management\WebFunctionsManager.cs" />
     <Compile Include="Models\FunctionMetadataResponse.cs" />
+    <Compile Include="ResourceContainsSecretsAttribute.cs" />
     <Compile Include="Security\SecretsUtility.cs" />
     <Compile Include="Security\SimpleWebTokenHelper.cs" />
     <Compile Include="StandbyManager.cs" />
diff --git a/src/WebJobs.Script/Extensions/HttpRequestMessageExtensions.cs b/src/WebJobs.Script/Extensions/HttpRequestMessageExtensions.cs
@@ -12,6 +12,16 @@ namespace Microsoft.Azure.WebJobs.Script
 {
     public static class HttpRequestMessageExtensions
     {
+        public static bool HasHeader(this HttpRequestMessage request, string headerName)
+        {
+            return !string.IsNullOrEmpty(request.GetHeaderValueOrDefault(headerName));
+        }
+
+        public static bool HasHeaderValue(this HttpRequestMessage request, string headerName, string value)
+        {
+            return string.Equals(request.GetHeaderValueOrDefault(headerName), value, StringComparison.OrdinalIgnoreCase);
+        }
+
         public static bool MatchRoute(this HttpRequestMessage request, string route)
         {
             return request.RequestUri.LocalPath.Trim('/').ToLowerInvariant().Equals(route.Trim('/').ToLowerInvariant());
diff --git a/src/WebJobs.Script/GlobalSuppressions.cs b/src/WebJobs.Script/GlobalSuppressions.cs
@@ -246,4 +246,6 @@
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.ScriptHost.#ConfigureLogging()")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Dir", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Scale.ApplicationPerformanceCounters.#RemoteDirMonitorLimit")]
 [assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Dir", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.Scale.ApplicationPerformanceCounters.#RemoteDirMonitors")]
-[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Sas", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.EnvironmentSettingNames.#AzureWebJobsSecretStorageSas")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1704:IdentifiersShouldBeSpelledCorrectly", MessageId = "Sas", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.EnvironmentSettingNames.#AzureWebJobsSecretStorageSas")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "ARM", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.ScriptConstants.#AntaresARMRequestTrackingIdHeader")]
+[assembly: System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1709:IdentifiersShouldBeCasedCorrectly", MessageId = "ARM", Scope = "member", Target = "Microsoft.Azure.WebJobs.Script.ScriptConstants.#AntaresARMExtensionsRouteHeader")]
diff --git a/src/WebJobs.Script/ScriptConstants.cs b/src/WebJobs.Script/ScriptConstants.cs
@@ -65,6 +65,9 @@ public static class ScriptConstants
         public const string ColdStartEventName = "ColdStart";
         public const string ShutdownRecoveryEventName = "ShutdownRecovery";
 
+        public const string AntaresARMRequestTrackingIdHeader = "x-ms-arm-request-tracking-id";
+        public const string AntaresARMExtensionsRouteHeader = "X-MS-VIA-EXTENSIONS-ROUTE";
+        public const string AntaresClientAuthorizationSourceHeader = "X-MS-CLIENT-AUTHORIZATION-SOURCE";
         public const string AntaresLogIdHeaderName = "X-ARR-LOG-ID";
         public const string AntaresScaleOutHeaderName = "X-FUNCTION-SCALEOUT";
         public const string AntaresColdStartHeaderName = "X-MS-COLDSTART";
diff --git a/src/WebJobs.Script/Utility.cs b/src/WebJobs.Script/Utility.cs
@@ -41,6 +41,39 @@ public static bool IsDynamic
             }
         }
 
+        /// <summary>
+        /// Walk from the method up to the containing type, looking for an instance
+        /// of the specified attribute type, returning it if found.
+        /// </summary>
+        /// <param name="method">The method to check.</param>
+        internal static T GetHierarchicalAttributeOrNull<T>(MethodInfo method) where T : Attribute
+        {
+            return (T)GetHierarchicalAttributeOrNull(method, typeof(T));
+        }
+
+        /// <summary>
+        /// Walk from the method up to the containing type, looking for an instance
+        /// of the specified attribute type, returning it if found.
+        /// </summary>
+        /// <param name="method">The method to check.</param>
+        /// <param name="type">The attribute type to look for.</param>
+        internal static Attribute GetHierarchicalAttributeOrNull(MethodInfo method, Type type)
+        {
+            var attribute = method.GetCustomAttribute(type);
+            if (attribute != null)
+            {
+                return attribute;
+            }
+
+            attribute = method.DeclaringType.GetCustomAttribute(type);
+            if (attribute != null)
+            {
+                return attribute;
+            }
+
+            return null;
+        }
+
         public static string GetSettingFromConfigOrEnvironment(string settingName)
         {
             string configValue = ConfigurationManager.AppSettings[settingName];
diff --git a/test/WebJobs.Script.Tests.Integration/SamplesEndToEndTests.cs b/test/WebJobs.Script.Tests.Integration/SamplesEndToEndTests.cs
@@ -51,6 +51,111 @@ public SamplesEndToEndTests(TestFixture fixture)
             _settingsManager = ScriptSettingsManager.Instance;
         }
 
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_NonExtensionRoute_Succeeds()
+        {
+            // when request not made via ARM extensions route, expect success
+            HttpRequestMessage request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            HttpResponseMessage response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_GetSecrets_NonAdmin_Unauthorized()
+        {
+            // when GET request for secrets is made via ARM extensions route, expect unauthorized
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await this._fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
+            string content = await response.Content.ReadAsStringAsync();
+            Assert.Equal($"\"{Microsoft.Azure.WebJobs.Script.WebHost.Properties.Resources.UnauthorizedArmExtensionResourceRequest}\"", content);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_GetSecrets_Admin_Succeeds()
+        {
+            // owner or co-admin always authorized
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            request.Headers.Add(ScriptConstants.AntaresClientAuthorizationSourceHeader, "Legacy");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_GetSecrets_Internal_Succeeds()
+        {
+            // hostruntime requests made internally by Geo (not over hostruntime bridge) are not filtered
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_GetSecrets_NoKey_Unauthorized()
+        {
+            // without master key the request is unauthorized (before the filter is even run)
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(ScriptConstants.AntaresClientAuthorizationSourceHeader, "Legacy");
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_InvalidKey_Unauthorized()
+        {
+            // with an invalid master key the request is unauthorized (before the filter is even run)
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/keys");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, "invalid");
+            request.Headers.Add(ScriptConstants.AntaresClientAuthorizationSourceHeader, "Legacy");
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_NonGet_Succeeds()
+        {
+            // if the extensions request is anything other than a GET, it is not filtered
+            var request = new HttpRequestMessage(HttpMethod.Delete, "admin/host/keys/dne");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "true");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
+        }
+
+        [Fact]
+        public async Task ArmExtensionsResourceFilter_GetNonSecretResource_Succeeds()
+        {
+            // resources that don't return secrets aren't restricted
+            var request = new HttpRequestMessage(HttpMethod.Get, "admin/host/ping");
+            request.Headers.Add(AuthorizationLevelAttribute.FunctionsKeyHeaderName, MasterKey);
+            request.Headers.Add(ScriptConstants.AntaresARMExtensionsRouteHeader, "1");
+            request.Headers.Add(ScriptConstants.AntaresARMRequestTrackingIdHeader, "1234");
+            var response = await _fixture.HttpClient.SendAsync(request);
+
+            Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        }
+
         [Fact]
         public async Task EventHubTrigger()
         {

Original file line number	Diff line number	Diff line change
`@@ -21,6 +21,7 @@ namespace Microsoft.Azure.WebJobs.Script.WebHost.Controllers`
`21`	`21`	`{`
`22`	`22`	`[JwtAuthentication]`
`23`	`23`	`[AuthorizationLevel(AuthorizationLevel.Admin)]`
	`24`	`+ [ResourceContainsSecrets]`
`24`	`25`	`public class KeysController : ApiController`
`25`	`26`	`{`
`26`	`27`	`private const string MasterKeyName = "_master";`
Original file line number	Diff line number	Diff line change
`@@ -12,6 +12,16 @@ namespace Microsoft.Azure.WebJobs.Script`
`12`	`12`	`{`
`13`	`13`	`public static class HttpRequestMessageExtensions`
`14`	`14`	`{`
	`15`	`+ public static bool HasHeader(this HttpRequestMessage request, string headerName)`
	`16`	`+ {`
	`17`	`+ return !string.IsNullOrEmpty(request.GetHeaderValueOrDefault(headerName));`
	`18`	`+ }`
	`19`	`+`
	`20`	`+ public static bool HasHeaderValue(this HttpRequestMessage request, string headerName, string value)`
	`21`	`+ {`
	`22`	`+ return string.Equals(request.GetHeaderValueOrDefault(headerName), value, StringComparison.OrdinalIgnoreCase);`
	`23`	`+ }`
	`24`	`+`
`15`	`25`	`public static bool MatchRoute(this HttpRequestMessage request, string route)`
`16`	`26`	`{`
`17`	`27`	`return request.RequestUri.LocalPath.Trim('/').ToLowerInvariant().Equals(route.Trim('/').ToLowerInvariant());`