temporalio / temporal

Temporal service
https://docs.temporal.io
MIT License
11.6k stars 829 forks source link

Custom authentication between frontend and other services #4902

Open ndtretyak opened 1 year ago

ndtretyak commented 1 year ago

Is your feature request related to a problem? Please describe. There is a HeadersProvider interface in the go-sdk which I use together with custom Authorizer to implement custom authentication mechanism for workers. But I couldn't find anything similar to HeadersProvider in the server side code. So, I think there is no way to replace client certificates with other authentication method for the requests between Temporal's microservices.

Describe the solution you'd like I'd like to define some interceptor for all requests between Temporal's microservices, so that these requests contain additional information for custom authentication.

If you are okay with this idea, I'd be happy to work on it.

dnr commented 1 year ago

This isn't a priority for the team so it's unlikely to happen soon. We do recommend mTLS for securing the connections between services. Depending on the complexity, I think it's possible we could accept something like HeadersProvider as a contribution, but no guarantees. Before spending too much time on it, maybe a proposal with a little more detail (i.e. interfaces) would be helpful.

ndtretyak commented 1 year ago

I've made a draft PR https://github.com/temporalio/temporal/pull/4928 adding WithClientHeadersProvider option that accepts HeadersProvider interface from the go-sdk.

amitbet commented 1 year ago

How is it not a priority, if this means that you have to accept nil in the Authorizer in order to allow for inter-process communication? does this not negate the whole concept of the custom auth, and allow anyone without a token to pass?

dnr commented 1 year ago

You can use mTLS to secure inter-node communication within a temporal cluster. The Authorizer (custom or otherwise) is not used for inter-node communication, except for one case (server worker→frontend), and you can use internal-frontend to eliminate that case.

Supporting token-based authentication for worker→frontend is desirable, but the lack of it does not negate the existing security features for external client→frontend traffic.

amitbet commented 1 year ago

But even when implementing mTLS for worker->frontend and tokens for external, in order to accept the worker->frontend traffic without a token the Authorizer must allow nil token headers (it can't discern between external and internal traffic) - this means that malicious traffic coming from the outside can create a message with a nil header, and it will go right through auth... this makes the whole token based auth mechanism completely useless It might also mean that the Temporal cloud has a huge security vulnerability

dnr commented 12 months ago
  1. The Authorizer is not given TLS connection metadata or tokens, it's given claims from the ClaimMapper. The ClaimMapper is given both TLS metadata and authorization tokens, and can construct claims based on one or the other or both. E.g. it can require either mTLS or a valid token, but not neither; if there's no token, it can require mTLS, if there's no mTLS, it can require a token. (Yes, you can pass anything from ClaimMapper to Authorizer in the Extensions field but that doesn't change the point.)
  2. Using internal-frontend gives an easy way to differentiate internal and external traffic without writing a custom ClaimMapper. Internal traffic bypasses the ClaimMapper (i.e. it always gets full admin claims), external traffic does not. It's not the only way to do it, you can also use more complicated TLS configs, but internal-frontend is probably easier.
pilotofbalance commented 12 months ago

@dnr in case of implementing custom ClaimMapper, it is not given...not just an authorization.AuthInfo is empty, but GetClaims method is not triggered at all, so how token or mTLS could be checked for temporal services or web ui?

dnr commented 12 months ago

I'm not sure I understand. You can see the logic here: https://github.com/temporalio/temporal/blob/main/common/authorization/interceptor.go#L102

If a claim mapper and authorizer are set, and then if either 1. the connection has a client certificate, 2. the connection has an authorization header, or 3. the claim mapper returns false for AuthInfoRequired, then the claim mapper is called with whatever AuthInfo it can put together from those.

(Note that the change to allow customizing the name of the authorization header isn't released yet.)

I don't know that much about the web ui, but my understanding is that it will forward an authorization header with its requests.

pilotofbalance commented 12 months ago

@dnr I'm talking about this case: https://github.com/temporalio/samples-server/blob/main/extensibility/authorizer/myClaimMapper.go. Now if you are running only temporal default services + web ui, without any workers, so this method: func (c myClaimMapper) GetClaims(authInfo *authorization.AuthInfo) (*authorization.Claims, error) { is not even triggered, if it's not triggered, you can't really check client cert or auth header and return claims with some flag. Although this method is always triggered func (a *myAuthorizer) Authorize(_ context.Context, claims *authorization.Claims, target *authorization.CallTarget) (authorization.Result, error) { ,but with no claims (nil)

dnr commented 12 months ago

Can you confirm that the web ui sending a grpc header named authorization? If it is, and the claim mapper is still not getting called, then that's a bug and we should probably split it to another issue. My guess is that the web ui is not sending that header for some reason.

pilotofbalance commented 12 months ago

@dnr let's say I'm a hacker and I'm running my own web ui, I decided not to send authorization token to the temporal cluster? Temporal cluster should validate token or certificate in any case and not cos web ui or other internal temporal services have it...pls, try to run this code https://github.com/temporalio/samples-server/tree/main/extensibility/authorizer and debug ClaimMapper method you will see that it's never called.

dnr commented 12 months ago

If you don't send an authorization token and don't authenticate with a client cert, then as you said, the claim mapper is not called, and claims will be nil when passed to the Authorizer. The authorizer (either default or custom) should reject any request with nil claims, except for health checks. I still don't see any problem here.

pilotofbalance commented 12 months ago

@dnr so we are returning to the original issue, there is no option to send authorization token from "matching", "history" and "worker" services to the "frontend" and this is what this PR is about, as you said the only option is to configure mTLS, but then there is no point for custom authorizer and claimMapper implementation, they just useless..(btw you just mentioned health checks, how I can recognize health check in case of nil claims?)

dnr commented 12 months ago

Yes, we should fix the original issue. No, the lack of ability to send tokens for internode communication does not mean that custom authorizer+claimmappers are useless: it's perfectly reasonable to use mTLS to authenticate internode communication and tokens to authenticate external clients, and you can do that with the current state of the code.

For health checks, see https://github.com/temporalio/temporal/blob/main/common/authorization/frontend_api.go#L46. The default authorizer allows calls to those two methods even with no claims, and you probably want to have your custom authorizer do the same. This allows kubernetes or similar systems to do readiness probes without needing to use mTLS there.

pilotofbalance commented 12 months ago

@dnr I didn't find it in temporal documentation...you said that for custom Authorizer and ClaimMapper there have to be external frontend and for temporal services internal frontend with mTLS...may be it worth it to provide kinda example or at least HL documentation.. Default authorizer require "sub" claim which should be optional and not mandatory + in my case only I can't change token structure and include "permissions"