Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
carnott-snap opened this issue Mar 11, 2020 · 30 comments
Labels
P2 Type: API Change Breaking API changes (experimental APIs only!) Type: Feature New features or improvements in behavior

Comments

@carnott-snap
Copy link

carnott-snap commented Mar 11, 2020

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.

@carnott-snap carnott-snap added the Type: Feature New features or improvements in behavior label Mar 11, 2020
@dfawley
Copy link
Member

dfawley commented Mar 12, 2020

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
Copy link
Author

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
Copy link
Contributor

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

@carnott-snap
Copy link
Author

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
Copy link
Member

dfawley commented Mar 13, 2020

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
Copy link

stale bot commented Mar 20, 2020

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.

@stale stale bot added the stale label Mar 20, 2020
@carnott-snap
Copy link
Author

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:

  • access to the client/request metadata, chiefly for extracting the host/authority header information
    • this information is verified by a proxy, thus the discussion about H2
  • the ability to set custom headers
    • the header will bind in information about where there request is going, and will also be consumed by the proxy

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 stale bot removed the stale label Mar 24, 2020
@stale
Copy link

stale bot commented Mar 31, 2020

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.

@stale stale bot added the stale label Mar 31, 2020
@carnott-snap
Copy link
Author

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

@dfawley
Copy link
Member

dfawley commented Apr 9, 2020

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
Copy link
Author

carnott-snap commented Apr 10, 2020

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
Copy link
Member

dfawley commented Apr 10, 2020

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
Copy link
Author

carnott-snap commented Apr 13, 2020

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

dfawley commented Apr 16, 2020

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
Copy link
Author

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?

@dfawley
Copy link
Member

dfawley commented Apr 20, 2020

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.

@dfawley dfawley changed the title expose authority to client interceptors client: Add CallOption for setting authority; allow even without WithInsecure Apr 20, 2020
@dfawley dfawley removed their assignment Apr 20, 2020
@carnott-snap
Copy link
Author

carnott-snap commented Apr 22, 2020

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:

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

@carnott-snap
Copy link
Author

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

@dfawley
Copy link
Member

dfawley commented Apr 24, 2020

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

  • NoSecurity is plaintext (h2c).
  • 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).
  • PrivacyAndIntegrity provides encryption (TLS).

@carnott-snap
Copy link
Author

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
Copy link
Author

carnott-snap commented Apr 24, 2020

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
Copy link
Member

dfawley commented Apr 24, 2020

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
Copy link

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

and CallOption::withAutority()

@easwars
Copy link
Contributor

easwars commented Sep 30, 2021

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.

  • Add a call option to override the :authority header on a per-RPC basis.
    • An optional interface will be added, to be implemented by AuthInfo implementations, to validate this override.
      • Implementations will need to perform a hostname validation check on the peer certificate, received during the handshake, to validate this override.
      • RPCs will fail if this call option is set, but:
        • AuthInfo for the subChannel does not implement this interface
        • validation by AuthInfo fails
      • Existing TLS based credentials implementations will be enhanced to support this.
      • Insecure credentials will allow for any authority value.

@easwars
Copy link
Contributor

easwars commented May 17, 2022

Closing this in favor of #5361 which captures only the tasks required to support this call option.

@easwars easwars closed this as completed May 17, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
P2 Type: API Change Breaking API changes (experimental APIs only!) Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

5 participants