Authentication model for Extension WebHooks #1659
Conversation
|
@watashiSHUN FYI |
| return response; | ||
| // key names for extension webhooks are named by convention | ||
| string keyName = $"{name}_extension".ToLowerInvariant(); | ||
| if (!this.Request.HasAuthorizationLevel(AuthorizationLevel.System, keyName)) |
There was a problem hiding this comment.
This change requires ALL extension WebHooks to present a valid system key with the correct ID as a code query param or via headers
There was a problem hiding this comment.
This will break existing DTF and O3656 uses.
I think we may need a corresponding update in WebJobsSdkExtensionHookProvider to append this key.
In reply to: 125786600 [](ancestors = 125786600)
There was a problem hiding this comment.
Resolved - now appending code to URI automatically. Also now creating the secret on demand, removing the requirement for explicit provisioning.
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| namespace Microsoft.Azure.WebJobs.Script.WebHost.Controllers | |||
| { | |||
| [SystemAuthorizationLevel(ScriptConstants.SwaggerDocumentationKey)] | |||
There was a problem hiding this comment.
Merging this behavior into the base attribute where it makes more sense, and we avoid duplication
| return; | ||
| } | ||
|
|
||
| if (requestAuthorizationLevel < Level) |
There was a problem hiding this comment.
Now we require an exact level match - this is the key change
|
|
||
| internal static Task<AuthorizationLevel> GetAuthorizationLevelAsync(HttpRequestMessage request, ISecretManager secretManager, string functionName = null) | ||
| internal static Task<KeyAuthorizationResult> GetAuthorizationResultAsync(HttpRequestMessage request, ISecretManager secretManager, string keyName = null, string functionName = null) |
There was a problem hiding this comment.
All this refactoring to return a structured result containing the matched key ID supports the changes in this PR, but will also be needed for the EasyAuth updates I'll be doing soon (where we want to be able to flow the matched KeyId to the user code).
mathewc
force-pushed
the
hook_auth
branch
3 times, most recently
from
1da1228 to
a464c82
Compare
| return new Uri($"{scheme}://{hostName}/admin/extensions/{name}"); | ||
| private async Task<string> GetOrCreateExtensionKey(string extensionName) | ||
| { | ||
| var hostSecrets = _secretManager.GetHostSecretsAsync().GetAwaiter().GetResult(); |
There was a problem hiding this comment.
GetAwaiter [](start = 67, length = 10)
illegal call
|
|
||
| var handler = provider.GetHandlerOrNull(name); | ||
| if (handler != null) | ||
| { | ||
| var response = await handler.ConvertAsync(this.Request, token); | ||
| return response; | ||
| string keyName = WebJobsSdkExtensionHookProvider.GetKeyName(name); |
There was a problem hiding this comment.
string keyName = WebJobsSdkExtensionHookProvider.GetKeyName(name); [](start = 16, length = 66)
I need to double check whether this breaks the existing webhooks
| @@ -52,8 +61,30 @@ internal static Uri GetExtensionWebHookRoute(string name) | |||
|
|
|||
| bool isLocalhost = hostName.StartsWith("localhost:", StringComparison.OrdinalIgnoreCase); | |||
There was a problem hiding this comment.
This code is not ran when tested locally:
var hostName = settings.GetSetting(EnvironmentSettingNames.AzureWebsiteHostName);
will return null
if you don't set your WEBSITE_HOSTNAME = localhost:12345 explicitly on your local env
We should consider hostName == null -->localhost
| requestAuthorizationLevel = await GetAuthorizationLevelAsync(request, secretManager, EvaluateKeyMatch); | ||
| request.SetAuthorizationLevel(requestAuthorizationLevel); | ||
| var result = await GetAuthorizationResultAsync(request, secretManager, EvaluateKeyMatch, KeyName); | ||
| requestAuthorizationLevel = result.AuthorizationLevel; |
There was a problem hiding this comment.
I don't think we need this assignment
requestAuthorizationLevel is not being used in the rest of the function body
in the old code, it is used to compare against Level:
if (requestAuthorizationLevel < Level)
but in the new code
if (!request.HasAuthorizationLevel(Level))
is extracting requestAuthorizationLevel itself
Adding an authentication model for extension WebHooks, requiring that calls into the extension hook path are authenticated with a valid SystemKey. This key will need to be provisioned as part of the WebHook setup.
As part of this, also making level handling more robust / logical. Our previous inequality logic would allow a function to be invoked with a level >= the required level. That made sense when the levels were simply {Anonymous, Function, Admin}. However now that we have System (added recently) we have {Anonymous, Function, System, Admin}. This means that currently a System key can invoke any Function, which isn't what we want. We can make these changes now in a safe, non-breaking way, because System keys are currently only used for Swagger, and are not exposed generally. The proposal is also to use them for extension WebHooks, so it is important to fix this now.