Azure / azure-functions-durable-extension

Durable Task Framework extension for Azure Functions
MIT License
713 stars 267 forks source link

Disable management HTTP endpoints #838

Open scale-tone opened 5 years ago

scale-tone commented 5 years ago

I'm playing with DurableOrchestrationClient.CreateHttpManagementPayload(). It returns me some endpoints for managing a particular orchestration instance, like these: { "id": "c6521567-d9e8-45a1-a252-b09154c4d7b2", "statusQueryGetUri": "https://whatifdemofunctionapp.azurewebsites.net/runtime/webhooks/durabletask/instances/c6521567-d9e8-45a1-a252-b09154c4d7b2?taskHub=WhatIfDemoProd&connection=Storage&code=some-code", "sendEventPostUri": "https://whatifdemofunctionapp.azurewebsites.net/runtime/webhooks/durabletask/instances/c6521567-d9e8-45a1-a252-b09154c4d7b2/raiseEvent/{eventName}?taskHub=WhatIfDemoProd&connection=Storage&code=some-code", "terminatePostUri": "https://whatifdemofunctionapp.azurewebsites.net/runtime/webhooks/durabletask/instances/c6521567-d9e8-45a1-a252-b09154c4d7b2/terminate?reason={text}&taskHub=WhatIfDemoProd&connection=Storage&code=some-code", "rewindPostUri": "https://whatifdemofunctionapp.azurewebsites.net/runtime/webhooks/durabletask/instances/c6521567-d9e8-45a1-a252-b09154c4d7b2/rewind?reason={text}&taskHub=WhatIfDemoProd&connection=Storage&code=some-code" }

Then I take statusQueryGetUri from that response, remove instanceId from it and navigate to that URL ("https://whatifdemofunctionapp.azurewebsites.net/runtime/webhooks/durabletask/instances?taskHub=WhatIfDemoProd&connection=Storage&code=some-code") with my browser. Voila, the endpoint gives me statuses of all orchestrations.

I mean, is that secure?

Let's say, I am a customer, I made an order and I want to know the status of it via my mobile application, which queries that orchestration instance status URL periodically. Why should I be allowed to see other user's orders and even be allowed to cancel them? Shouldn't that code be tied to that particular instance?

Investigative information

To Reproduce

  1. Call DurableOrchestrationClient.CreateHttpManagementPayload() with some instanceId.
  2. Take returned statusQueryGetUri , remove instanceId value from it and make a GET request against it.

Expected behavior 401 Unauthorized or 403 Forbidden returned.

Actual behavior The endpoint successfully returns statuses of all orchestration instances.

olitomlinson commented 5 years ago

@scale-tone

The general public shouldn’t be able to query a Orchestration status endpoint directly - unless you are relying on the frowned upon security practise of ‘security by obscurity’ aka ‘Guess A Guid’ - I would not recommend this.

I would encourage you to proxy requests to the orchestration status endpoint through an upstream API, that can specifically perform authorisation and then authenticate the user against that particular orchestration ID. The {code} Key that is used to authorise calls to the Orchestration status endpoint is not intended to replace any Auth your customer facing application performs.

Also I would encourage you to not expose the exact orchestration status response payload directly to the mobile client as this is strongly coupling your presentation to the Durable Functions implementation, which is bad practice - bleeding out implementation which should be hidden etc.

scale-tone commented 5 years ago

The general public shouldn’t be able to query a Orchestration status endpoint directly

Totally agree, that it shouldn't. The problem is that it can, because the above mentioned endpoint is enabled by default, and as we now see, is only protected by a common "key". Hence the bug. Any ideas on how to disable that endpoint or how to revoke a leaked "key", btw.?

Also I would encourage you to not expose the exact orchestration status response payload directly to the mobile client

I would encourage myself and all others to do the same. The thing is though that the documentation encourages people to do the opposite. Also, the CreateHttpManagementPayload() method's signature is misleading: it takes an instanceId, which makes people think the returned URLs are instance-specific, while they're effectively not. Hence the bug.

olitomlinson commented 5 years ago

You are not supposed to expose function URLs to the public. Durable functions or normal functions, you don’t advertise these URLs.

Yes technically they are publicly addressable, but that doesn’t mean you should expose them.

It sounds like your problem is with azure functions Auth in general and not Durable Functions.

scale-tone commented 5 years ago

It sounds like your problem is with azure functions Auth in general and not Durable Functions.

Thank you, Oliver, for pointing me to my problem.

I just hope, that from the above bug report it is clear for the team, that this service endpoint isn't secure enough and should: a) either have a way to be disabled or b) have that "code" be instance- or user-specific and preferably with some configurable TTL.

Azure App Service Authentication Layer (codenamed EasyAuth) doesn't help either. Once configured for e.g. Facebook, it gives access to all your endpoints (including /durabletask/instances) to all Facebook users. I am a Facebook user and I can sniff that "code" out from my traffic => I got access to all other Facebook user's orchestrations.

cgillum commented 5 years ago

Hi @scale-tone. Generally speaking I agree with @olitomlinson. The Azure Functions host is not designed for fine-grained (e.g. per-user or per-instance) authorization. That's something you would need to build on your own as part of an API gateway. Rather, the Functions authorization model is designed for service-to-service authorization via shared secrets (API keys) which should only be shared with trusted sub-systems that you managed. These API keys should never be exposed to untrusted clients, like browsers or mobile apps.

I just hope, that from the above bug report it is clear for the team, that this service endpoint isn't secure enough

Assuming you agree that API keys should not be given to untrusted parties, why is the current design not secure enough? I think what you mean is that it's not flexible enough to support instance-specific authorization rules, and I would agree with this.

Azure App Service Authentication Layer (codenamed EasyAuth) doesn't help either.

As an aside, the social providers supported by Easy Auth are NOT intended to be used for authorization. Rather, they are for user authentication. Any authorization needs to be implemented in your application logic. We would therefore never create a design where you could use a Facebook identity to access a Function app because, as you mentioned, that makes no sense. More likely we would create some kind of an RBAC feature where specific identities could have roles, and you could assign roles to particular operations on the Azure Functions host and/or the Durable Functions extension. I think this is the feature request that you might be hinting towards.

scale-tone commented 5 years ago

These API keys should never be exposed to untrusted clients, like browsers or mobile apps.

Now it is clear to me, yes, and I hope, it is now more clear to everyone else. While it was hard to realize this by just looking through documentation and/or CreateHttpManagementPayload() method's signature (still, what's the point of passing an instanceId to it, when the result does not effectively depend on it?). Also, since that "code" is a very sensitive and powerful secret, I would prefer to keep it in a Key Vault, while the current design prevents me from doing that.

I think what you mean is that it's not flexible enough to support instance-specific authorization rules, and I would agree with this.

That's exactly what I meant, but also me as a developer I would like to have more control over this endpoint. When I write my own Function, I'm free to implement whatever authorization rules I like and I have full control over it's keys (via e.g. Azure Portal). While none of these two things are true about that management endpoint: even it's keys are very much hidden. How do I quickly revoke a leaked one? That's why I would sleep better if I had a way to disable that endpoint, at least in prod environment.

More likely we would create some kind of an RBAC feature where specific identities could have roles, and you could assign roles to particular operations on the Azure Functions host and/or the Durable Functions extension. I think this is the feature request that you might be hinting towards.

That would be a very useful feature as well.

cgillum commented 5 years ago

Thanks @scale-tone, it sounds like we're on the same page.

Hey @jeffhollan, is there any public documentation regarding API keys or even general security in Azure Functions? I did a quick search and I was only able to find some reference-style wiki pages in GitHub:

I may have noticed a blog post or two, but is there any other documentation which describes the types of keys that exist and the best practices around using them? If not, I think that's probably a good thing for us to make available to customers.

espray commented 5 years ago

I am with @scale-tone on this one. Because out-of-the-box I cant control the HTTP APIs authorization, I would prefer to disable those endpoints and implement my own.

espray commented 4 years ago

@scale-tone here is a proxy that intercepts the runtime/webhooks/durabletask/{*all} so you can disable the out-of-the-box durable apis

{
  "$schema": "http://json.schemastore.org/proxies",
  "proxies": {
    "runtime": {
      "desc": [ "disable durabletask endpoints" ],
      "matchCondition": {
        "route": "{url:regex(runtime)}/webhooks/durabletask/{*all}"
      },
      "responseOverrides": {
        "response.statusCode": "404",
        "response.statusReason": "Not Found"
      }
    }
  }
}
scale-tone commented 4 years ago

@espray, unfortunately it breaks Durable Functions themselves:

[25/10/2019 22:01:31] Executed 'Functions.orchestrations' (Failed, Id=012c5e24-08af-4fca-8dd7-6596979a76f3) [25/10/2019 22:01:31] System.Private.CoreLib: Exception while executing function: Functions.orchestrations. System.Private.CoreLib: Result: Failure Exception: Error: Webhook returned status code 404: Stack: Error: Webhook returned status code 404: at DurableOrchestrationClient.<anonymous> (C:\projects\CSA\github\DurableFunctionsMonitor\durablefunctionsmonitor.functions\node_modules\durable-functions\lib\src\durableorchestrationclient.js:170:43) at Generator.next (<anonymous>) at fulfilled (C:\projects\CSA\github\DurableFunctionsMonitor\durablefunctionsmonitor.functions\node_modules\durable-functions\lib\src\durableorchestrationclient.js:4:58) at process._tickCallback (internal/process/next_tick.js:68:7).

komayama commented 3 years ago

@cgillum I think I don't need to all response field the following document. https://docs.microsoft.com/en-us/azure/azure-functions/durable/durable-functions-http-api#response

We can get four fields by default, when call /runtime/webhooks/durabletask/orchestrators/{functionName} endpoint.

But if this durable functions management with another organization and they wrong call sendEventPostUri endpoint, I thought that durable functions unexpected processing. So we don't need any fields and hope restrict by host.json or some settings.

cgillum commented 3 years ago

@komayama We do not have a way to disable any of the supported URLs. However, if you just want to filter the set of returned URLs, you can do so using a custom HTTP trigger function, like this:

[FunctionName(nameof(Start))]
public static async Task<object> Start(
    [HttpTrigger(AuthorizationLevel.Function, methods: "post", Route = "start")] HttpRequest req,
    [DurableClient] IDurableClient starter)
{
    string instanceId = await starter.StartNewAsync("E1_HelloSequence", (object)null);
    HttpManagementPayload managementUrls = starter.CreateHttpManagementPayload(instanceId);

    // Return just a subset of the URLs in the response
    return new
    {
        managementUrls.Id,
        managementUrls.StatusQueryGetUri,
        managementUrls.TerminatePostUri,
    };
}

The response will look like this:

{
  "id": "6ebef6e169c64c3a936fcf3bc0537645",
  "statusQueryGetUri": "http://localhost:7071/runtime/webhooks/durabletask/instances/6ebef6e169c64c3a936fcf3bc0537645?taskHub=SampleHubVS&connection=Storage&code=xyz",
  "terminatePostUri": "http://localhost:7071/runtime/webhooks/durabletask/instances/6ebef6e169c64c3a936fcf3bc0537645/terminate?reason={text}&taskHub=SampleHubVS&connection=Storage&code=xyz"
}

However, this is not secure because a user could still manually construct the "sendEventPostUri" themselves. The only way to do this securely today is to return links to other HTTP trigger functions, instead of links to the built-in management HTTP APIs.

scale-tone commented 3 years ago

@komayama , @cgillum , I think, there is a way to block the entire management endpoint with a proxy, like this. That would, of course, only work if your Functions project is in .Net (otherwise your functions are going to be broken themselves).

Probably, the same approach could be used to block/filter individual endpoints/urls. Haven't tried myself though.

ConnorMcMahon commented 3 years ago

Recategorizing this issue as a feature request to be able to easily disable the HTTP management APIs in favor of exposing your own.

If we were to support this feature, we would want to ensure that localRpcEnabled=true for non-.NET customers, otherwise it would break client functionality.