Why does not gateway forward headers as-is?

[ghost]

Hi,
Why does not gateway forward headers as-is? Is there a technical reason for this?

Thanks.

[tmc]

Forwarding as-is would potentially mean collisions in the 'grpc-*' space.

[tmc]

I would be open to reconsidering this but it would be a backwards incompatible change. Thoughts?

[tamalsaha]

@tmc , we are interested in this one, too. We have multi use-cases where we need headers to be forwarded to grpc server. Can this be added as a configuration so that users of this library can tell which Additional headers to forward. I can think of following options:

• specify a prefix
• specify an array of headers

[tamalsaha]

Here are our use-cases:

• Proposal: Authentication and Authorization support for Tiller helm/helm#1918 - forward k8s-* headers.
• gomodules@d3f3d34 . We maintain a fork needed to pass headers for our own api server.

[lucas-natraj]

@tmc just curious why the forwarding of headers would be a backwards incompatible change?
do you just mean that headers that were being stripped out before would now pass through?
not too sure where that would be a problem though.

as for the collisions in the 'grpc-*' space, couldn't we just strip out all incoming grpc-* headers. that appears to be the same approach that google takes on gcp to prevent external requests appearing to look like they came from Google's own internal proxies.

although i can appreciate a need for additional customizability, i curious whether a simpler pass-through (+ stripping headers with reserved prefixes) wouldn't suffice.

[tmc]

@lucas-natraj we'd have to keep sending with the grpcgateway- prefix as we do now which while a bit messy is probably ok. I'm open to a diff that forwards headers more directly.

[lucas-natraj]

perhaps i'm referring to something tangential but iirc the code seems to indicate 2 special prefixes.
the one that i'm using is the Grpc-Metadata- which is how I am able to to send custom headers (metadata) to my grpc backend. the important header i'm sending is an api key which is pretty important. to get that through the gateway i need to send it as Grpc-Metadata-x-api-key instead of just x-api-key. hence my desire to have custom headers pass through.
is this something conflicting with what you were mentioning?

[tamalsaha]

@lucas-natraj , I think you should use #336 . Then you will be able to send header as you wish, without gateway forcing you to change you header key. With that pr, you should initiate the Mux like below:

gwMux := runtime.NewServeMux(runtime.EqualFoldMatcher("x-api-key"))

On the grpc side, you will find this is the Context metadata as "x-api-key". This is what we are doing for our api.

[lucas-natraj]

@tamalsaha yeah i did see that, and i can totally see the usefulness of it. my comment was just questioning whether we could just allow custom headers directly (which appears to be the main issue at hand). seems like the only concern was that there would be collisions with an existing prefix. hence my suggestion to strip out any conflicting headers but still allow the ones that don't conflict.
again, i may be missing the point entirely. 😄

[tamalsaha]

I think given there is already a special prefix (grpc-*), the best case may be to allow custom headers via user configuration. Also, this avoid extra processing for detecting collision for users who do not need these extra headers.

[lucas-natraj]

hmm. i'm a tad confused..

• authorization, x-forwarded-host and x-forwarded-for -> special handling
• permanent -> gets a grpcgateway- prefix [e.g. origin -> grpcgateway-origin)
• Grpc-Metadata-* -> the prefix is stripped [e.g. Grpc-Metadata-x-api-key -> x-api-key)

i can see the need for adding a grpcgateway- prefix for headers that the gateway plays an active role in supplying -> so just blacklist any incoming headers that have a prefix of grpcgateway-.

then there are some headers that just don't make sense -> add them to a blacklist.

all other headers, however, seem pretty ok to just pass through unaltered.

i've read through the short discussion on #213 (perhaps there were other issues where this was hashed out?), but i'm still unsure why a whitelist approach was adopted, rather than a blacklist. was there a specific problem that the whitelist approach addresses?

either way, i'm late to the discussion, so i'll take whatever i can get.. 😄

[tmc]

@lucas-natraj You're right that this issue, IMO represents moving to a blacklist vs a whitelist. In general the likelihood of introducing unwanted outcomes with a blacklist are higher but if we feel like we cover our bases here I think we'll be fine. If we arrive at blacklist over whitelist we can do a release that mimics old and new behavior and then deprecate old behavior in a subsequent release.

[tmc]

I think a sane default here is to continue whitelisting and supply customization capabilities with approaches like #360.

That said it is an interesting question as to whether or not the grpcgateway- prefixing is desirable to maintain. Open to thoughts here.

[lucas-natraj]

yeah, i do understand..

my primary concerns with the current approach -

• on the REST client side it's kinda poopy (technical term) for callers to use a special prefix like Grpc-Metadata- just so that the server receives it appropriately. so, i'm not too sure who that special prefix is for, especially since it seems to leak out implementation details to callers. (refer Grpc-Metadata- functionality not matching documentation #245)
• on the grpc server side, receiving metadata with a grpcgateway- prefix is equally strange because the server doesn't / shouldn't care that there is a REST gateway in front of it.

so, at this point, i'm not entirely sure why either prefix (Grpc-Metadata- or grpcgateway-) actually exist.. 😄

i think that the pr from @tamalsaha does improve things, but i'm just concerned that it is really just layering on a patch for something that needn't have been there in the first place.. (if that makes sense..)

good discussion though.. but i'm happy to accept whatever.. it's not that big a deal anyway.

[tmc]

I'm curious what is a good example of what could go wrong if we eliminated Grpc-Metadata- and grpcgateway- (with selective blacklisting).
@achew22 I think you have some thoughts here. @yugui do you want to chime in?

[tamalsaha]

Since we are thinking this from first principles, I would like to give my 2cs.

In my view, grpc-gateway is a stopgap solution until everything speaks HTTP/2 natively. As a result, gateway should be invisible to the grpc server implementation. Nothing in my grpc server implementation should require knowing about the request is coming from gateway.

In that light I think it makes sense passing all headers from user request to grpc server with these changes:

• grpc-* space is restricted from user request
• The other common headers that make sense should be changed. We can follow what nginx does in reverse proxy mode. https://www.nginx.com/resources/admin-guide/reverse-proxy/#headers

So, I think that automatically adding grpcgateway- prefix, stripping Grpc-Metadata- prefix should be stopped.

Then we can also give user control via some pr like #336 if they want to do some additional processing. One such use-case we have is, we want to know the actual VERB used by json http endpoint. We add that using #336 .

gwMux := runtime.NewServeMux(
	runtime.WithMetadata(func(c context.Context, req *http.Request) metadata.MD {
		return metadata.Pairs("x-forwarded-method", req.Method)
	}))

The last thing is we should decide how the response headers returned from grpc server are passed back to user. Our use case here is, when rate limiting is enabled, our grpc server returns headers like

X-RateLimit-Limit: 1200
X-RateLimit-Remaining: 1193
X-RateLimit-Reset: 1402425459

How do to pass it back to the caller?

[achew22]

The logic behind not forwarding headers arbitrarily is that many people have some kind of internal header that does special stuff. An auth impersonation header is an example of something that
I was under the impression that the logic was to prevent people from being able to insert arbitrary headers in the stream at least without the gateway author doing something to defeat the mechanism.

If you had an "Authorization: token" header and another "Impersonation: $other_user_id". Inside the network, you would expect the service receiving the call to process the request as $other_user_id assuming the auth token granted impersonation permissions.

The way I thought that was currently implemented was with a find and replace where it took Grpc-Metadata-x and made it just x in the request. That way you had the x- namespace of headers that were manipulable by the client and it removed everything else.

Evidently my understanding is wrong. It looks like including a client side header of Grpc-Metadata-Impersonate would get you impersonation which is an... interesting result.

I think @lucas-natraj's analysis is correct. As I read it I think the behavior of passing through Grpc-Metadata is a bug or possibly better viewed as an implementation artifact from days before we thought about headers.

There should probably be a whitelist, a customizable whitelist at that. I wonder if this is such a common usecase that we should make a really easy way to do it. We could make it pluggable with the current implementation as the default and then change the default later.

Does anyone have any thoughts?

[tmc]

@achew22 thanks for your input. I agree with your and @lucas-natraj's analysis. @tamalsaha great proposal. @tamalsaha while related, can you start a new issue re: trailing metadata to http response code mapping? I assume it will mirror the request metadata population you've laid out.

I think the next steps I'd like to take are:

1. Make the header behavior pluggable and provide the current behavior as the default.
2. Supply the new planned behavior (grpc-* filtering and behaving like nginx in reverse-proxy mode) as an option.
3. In a later release move to the new behavior as default.