grpc / grpc-go

The Go language implementation of gRPC. HTTP/2 based RPC
https://grpc.io
Apache License 2.0
21.03k stars 4.37k forks source link

client: Add CallOption for setting authority; allow even without WithInsecure #3444

Closed carnott-snap closed 2 years ago

carnott-snap commented 4 years ago

Use case(s) - what problem will this feature solve?

I would like to write a grpc.XxxClientInterceptor that is able to safely extract and interact with the authority header.

Proposed Solution

Since this information is currently hidden inside grpc.ClientConn, and that struct is an input to the grpc.XxxYyyInterceptor types, it seems reasonable to add a method to extract it.

func (cc *ClientConn) Authority() string {
        return cc.authority
}

Alternatives Considered

Func

Instead of a method, this could be implemented as a function, but does not read as well.

func Authority(cc *ClientConn) string {
  return cc.authority
}

Field

Currently authority is immutable, thus thread safe. Exposing a field would be easier than a method, but would break both guarantees.

type ClientConn struct {
    Authority string

    // ... other fields elided ...
}

Parameter

Since both grpc.XxxClientInterceptor types are EXPERIMENTAL APIs, you could just expose the value directly as a parameter. This seems pretty heavy for something that is not always required, and it double embeds the value (authority and cc.authority), so more edge cases and failure modes need be tested.

type UnaryClientInterceptor func(ctx context.Context, method, authority string, req, reply interface{}, cc *ClientConn, invoker UnaryInvoker, opts ...CallOption) error

type StreamClientInterceptor func(ctx context.Context, desc *StreamDesc, cc *ClientConn, method, authority string, streamer Streamer, opts ...CallOption) (ClientStream, error)

Additional Context

Authority is an h2 pseudo header set by grpc.WithAuthority. This grpc.DialOption binds the given string to grpc.dialOptions.authority, then grpc.Dial and grpc.DialContext bind against grpc.ClientConn.authority when called.

dfawley commented 4 years ago

Note that it's actually possible for the name resolver to override the authority by setting the ServerName field in resolver.Address:

https://github.com/grpc/grpc-go/blob/master/clientconn.go#L1219

This means exposing ClientConn.authority directly could result in an inconsistency.

The real authority is only passed outside gRPC in the credentials.ClientHandshake for now. That could then be retrieved by accessing the Peer if the credentials included it in the AuthInfo (our TLS credentials currently do not; this could potentially be added, or you could implement your own credentials wrapping ours and storing the authority in AuthInfo to be retrieved later).

The Peer is currently visible in a few ways:

  1. in unary RPCs via the Peer CallOption, only after the RPC completes. This means a unary interceptor also can't access it until after the RPC completes.

  2. in streaming RPCs while the RPC is in flight. This means a streaming interceptor can access it early in the RPC's lifecycle.

  3. to stats handlers when OutHeader is called at the start of the RPC.

What are you ultimately trying to accomplish by accessing the authority?

carnott-snap commented 4 years ago

We have use cases where grpc.Dial's target must differ from the authority header. This requirement exists because our reverse proxy uses authority for routing (similar to how the host header can be used), and target for DNS and TLS SNI. This works as expected using grpc.Dial("proxy.net", grpc.WithAuthority("service.net")), but is obscured from interceptors.

menghanl commented 4 years ago

What are you trying to do with authority in the interceptors? Is it just for logging or other purposes?

carnott-snap commented 4 years ago

What are you trying to do with authority in the interceptors? Is it just for logging or other purposes?

Logging is certainly an important feature, but more generally we desire to customise the outgoing request based on the authority header, as it is used in authentication on the receiving end.

After some discussions with colleagues, it became clear that grpc.XxxClientInterceptor may not be the right abstraction level for what we are targeting. Our changes need to happen at the http layer, including but not limited to header manipulation, and it seems like grpc.XxxClientInterceptors are more about modifications at the grpc layer; modifying methods, req/resp validation, etc. Furthermore, the manipulations we want are agnostic to unary/stream. So, it seemed to us that implementing a new grpc.HTTPInterceptor may actually suit our needs better, thoughts?

dfawley commented 4 years ago

Implementing an HTTP-level interceptor is not something we have plans or resources for at this time. It sounds like you may need a L7 proxy to do what you need, or possibly to reassess your design since this doesn't sound like anything we've heard requested before.

stale[bot] commented 4 years ago

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

carnott-snap commented 4 years ago

I should be a little more concrete, since I think my abstract discussion was both a hard piviot and may have scared you off. We are not looking to implement custom HTTP primitives, just interact with H2 headers via metadata.MD. It would be nice to have defined the exact scope for the grpc.XxxClientInterceptors, but it is experimental, so that is reasonable.

For our use case we need:

This reverse proxy listens on proxy.net and uses the host/authority header to route to upstream resources, like service.net. It also checks metadata/header information for service.net. Is there a more canonical way to do this?

stale[bot] commented 4 years ago

This issue is labeled as requiring an update from the reporter, and no update has been received after 7 days. If no update is provided in the next 7 days, this issue will be automatically closed.

carnott-snap commented 4 years ago

I believe I have provided satisfactory clarification, but if not please let me know what more would be helpful.

dfawley commented 4 years ago

This works as expected using grpc.Dial("proxy.net", grpc.WithAuthority("service.net")), but is obscured from interceptors.

This doesn't sound right to me. The WithAuthority dial option does not have any effect unless WithInsecure is being used as well, which should only be the case for tests.

I think there are a few different options you have here:

  1. Use the proxy environment variables to configure your proxy, and have the client dial the service name directly. This would mean you need to deploy your service's certs to the proxy (so it can terminate TLS). Now the authority header would match the service, and you can route via that.

  2. Use method to route instead of the authority header. The biggest limitation this approach has is that if you have the same service exposed by multiple backends (e.g. if all servers need to expose the reflection service or the health service), then there would be no way to route those properly.

  3. Use a custom header, not ":authority", for routing.

Of the three, I believe the first is the most commonly-used approach.

carnott-snap commented 4 years ago

This doesn't sound right to me. The WithAuthority dial option does not have any effect unless WithInsecure is being used as well, which should only be the case for tests.

A cursory browse through our infrastructure indicates that most calls are insecure, because TLS is managed outside of the Go runtime. Is there a better way to set things up, so that host/:authority is still correctly populated, but also exposes it to the interceptor? For the record most calls look like this:

grpc.Dial("proxy.net", grpc.WithAuthority("service.net"), grpc.WithInsecure())

We also serve non gRPC through this proxy, so there is a desire to constrain ourselves to HTTP primitives that can be implemented for both traffic types. This pattern of request hosts in the dns resolution/tls differing from the request body is a long standing pattern that has worked well for us. Is this an antipattern, disallowed, or bad practice in gRPC specifically for some reason that does not exist in HTTP? Or is it just the Go gRPC implementation that has issues?

That being said, I will also comment on your options:

  1. Use the proxy environment variables to configure your proxy

This option seems problematic/moot based on the external TLS termination described above. Can you elaborate on how this would change in that world?

  1. Use method to route instead of the authority header.

I can confirm this will not work for our infrastructure.

  1. Use a custom header, not ":authority", for routing.

As described above, this is a big departure from prior art. If there is a strong argument it is possible, but it feels like the tail wagging the dog in our infrastructure.

Could this be configured for the whole grpc.ClientConn or would it need to be included in the context.Context for every call to grpc.ClientConn.Invoke?

dfawley commented 4 years ago

Have you looked into using PerRPCCredentials? This would allow you to set custom headers for RPCs on the connection. You would need to initialize them per-ClientConn, as we currently don't have any plumbing of the authority into the PerRPCCredentials (though it may be possible to add a field to the context.RequestInfo it is able to extract from the context that contains it).

carnott-snap commented 4 years ago

Alright, it seems like PerRPCCredentials meets the needs for header manipulation. What is the most canonical way to provide a distinct host/:authority header for routing purposes to grpc? Can this just be another entry in the map[string]string returned by GetRequestMetadata?

It is also interesting that PerRPCCredentials are evaluated after interceptors, I assume this is intentional or required?

menghanl commented 4 years ago

Re initialize PerRPCCredentials per-ClientConn: In this case, initialize interceptors per-ClientConn? If those headers are not security related, using interceptor seems more appropriate.

carnott-snap commented 4 years ago

Let us assume we have both: e.g. authentication and custom route tracing; does that change your answer? They would both like access to the actual service (:authority, host, etc.).

Part of my issue, I do not know how to do this canonically, but would like to keep within the bounds of the existing http interface that we have build internally.

dfawley commented 4 years ago

What is the most canonical way to provide a distinct host/:authority header for routing purposes to grpc? Can this just be another entry in the map[string]string returned by GetRequestMetadata?

I believe this will not work. If you set ":authority":[]string{"my host"} you will end up with the server seeing a multiple-value :authority header: one entry containing the client's authority and one with "my host".

If you want to set the authority header, you can continue using WithAuthority. After talking it over with some others here, I think it's fine if we allow that to be used even without WithInsecure (though since you are already using WithInsecure, it will work for you as-is).

carnott-snap commented 4 years ago

If you want to set the authority header, you can continue using WithAuthority. After talking it over with some others here, I think it's fine if we allow that to be used even without WithInsecure (though since you are already using WithInsecure, it will work for you as-is).

Sounds good, so a future release will add the ability to grpc.Dial("proxy.net", grpc.WithAuthority("service.net")), turns out we do have a few people managing tls within Go.

To confirm, is :authority the right way to do this, and if so, are we open to exposing it to PerRPCCredentials?

Also, for my metrics/logging ask, it seems like an interceptor is the best bet (correct me if I am wrong), so can we extract context.RequestInfo similar to PerRPCCredentials?

carnott-snap commented 4 years ago

One troublesome detail about PerRPCCredentials is that RequireTransportSecurity does not seem to have access to per rpc information. In our usecase, we actually may have variable transport security requirements based to which endpoint we are reaching out to. Is this intentionally out of scope for PerRPCCredentials?

carnott-snap commented 4 years ago

Also, I am unclear what the uri ...string parameter means for the cases of n != 1 and the docs seem to be lacking on the interface.

dfawley commented 4 years ago

Sounds good, so a future release will add the ability to grpc.Dial("proxy.net", grpc.WithAuthority("service.net")), turns out we do have a few people managing tls within Go.

This is currently unplanned (and untracked; I'll file a bug) work, but it does sound like something we should be doing.

To confirm, is :authority the right way to do this, and if so, are we open to exposing it to PerRPCCredentials?

To do what? Read the authority set by the client?

Also, for my metrics/logging ask, it seems like an interceptor is the best bet (correct me if I am wrong), so can we extract context.RequestInfo similar to PerRPCCredentials?

For interceptors to see it, one option would be to convert WithAuthority into a CallOption (wrapped in a WithDefaultCallOptions), and then you could introspect it along with the other call options via a type assertion. That would also give the ability to change authority per-call.

RequireTransportSecurity does not seem to have access to per rpc information. In our usecase, we actually may have variable transport security requirements based to which endpoint we are reaching out to. Is this intentionally out of scope for PerRPCCredentials?

Your best bet would be to have RequireTransportSecurity return false and enforce your security requirements in GetRequestMetadata. This is how the newer, level-based security checks work (ref).

Also, I am unclear what the uri ...string parameter means for the cases of n != 1 and the docs seem to be lacking on the interface.

As best I can tell, this was designed for some kind of extensibility, but I don't see how it could ever be used. gRPC always calls it with one value (ref, ref).

dfawley commented 4 years ago

This is currently unplanned (and untracked; I'll file a bug) work, but it does sound like something we should be doing.

Whoops -- we'll just use this bug for that.

carnott-snap commented 4 years ago

To do what? Read the authority set by the client?

Yes, in order to create the credential header, we need access to the value that is currently stored in the :authority pseudo header. I was citing your previous comment that this was an option:

[T]hough it may be possible to add a field to the context.RequestInfo it is able to extract from the context that contains it.

For interceptors to see it, one option would be to convert WithAuthority into a CallOption (wrapped in a WithDefaultCallOptions), and then you could introspect it along with the other call options via a type assertion. That would also give the ability to change authority per-call.

I am game, but I thought all the grpc.XxxOptions were opaque. How would one extract the string from a grpc.CallOption?

Your best bet would be to have RequireTransportSecurity return false and enforce your security requirements in GetRequestMetadata. This is how the newer, level-based security checks work (ref).

Can you confirm what the credentials.SecurityLevels mean? This was my basic understanding:

carnott-snap commented 4 years ago

This is way late in the conversation, but in Java, Channel::authority simply exists, not sure if that changes things.

dfawley commented 4 years ago

This is way late in the conversation, but in Java, Channel::authority simply exists, not sure if that changes things.

This assumes a per-channel authority; if we do support setting it per-call then that wouldn't be useful to us.

I am game, but I thought all the grpc.XxxOptions were opaque. How would one extract the string from a grpc.CallOption?

They are all implemented by exported types, so a type assertion can see the values. E.g. https://godoc.org/google.golang.org/grpc#FailFastCallOption

Can you confirm what the credentials.SecurityLevels mean? This was my basic understanding:

NoSecurity: h2c IntegrityOnly: h2+tls PrivacyAndIntegrity: h2+mtls

carnott-snap commented 4 years ago

This is way late in the conversation, but in Java, Channel::authority simply exists, not sure if that changes things.

Actually, disregard my comment, this seems to be a poor choice of name, as per the javadoc, this is `grpc.ClientConn.Target:

  /**
   * The authority of the destination this channel connects to. Typically this is in the format
   * {@code host:port}.
   *
   * @since 1.0.0
   */
carnott-snap commented 4 years ago

They are all implemented by exported types, so a type assertion can see the values. E.g. https://godoc.org/google.golang.org/grpc#FailFastCallOption

Alright, so you are suggesting creating a new exported type:

type AuthorityOption string

And using it like so:

grpc.WithUnaryInterceptor(func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoke grpc.UnaryInvoker, opts ...grpc.CallOption) error {
        for _, o := opts {
                if a, ok := o.(AuthorityOption); ok {
                        fmt.Println(string(a))
                }
        }
        return invoke(ctx, method, req, reply, cc, opts)
}),
  • IntegrityOnly provides message signing but not encryption (so a man-in-the-middle can observe but not modify data - this can be done with TLS with a NULL cypher, I think).

Is this possible with gRPC today? I can ignore it for PrivacyAndIntegrity, as that meets my needs, just curious.

dfawley commented 4 years ago

Alright, so you are suggesting creating a new exported type:

Yes, exactly (except we'd probably make it a struct to be like the other call option types).

Is this possible with gRPC today? I can ignore it for PrivacyAndIntegrity, as that meets my needs, just curious.

None of our credentials set this value, but it is valid for credentials to indicate they only provide message integrity but not encryption, and PerRPCCredentials can validate the value using credentials.CheckSecurityLevel. Presumably we should be making the TLS credential check for null codecs and report it appropriately. (Maybe that's a bug? @yihuazhang, can you comment on this?)

Ivansamara109 commented 3 years ago

This is way late in the conversation, but in Java, Channel::authority simply exists, not sure if that changes things.

and CallOption::withAutority()

easwars commented 3 years ago

Posting description of how to go about implementing this from #4717. We are going to have to kick this can down the road for the time being, but if someone wants to implement this, we will be happy to review.

easwars commented 2 years ago

Closing this in favor of https://github.com/grpc/grpc-go/issues/5361 which captures only the tasks required to support this call option.