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

HTTP2 over Unix Domain Sockets requires :authority header #32326

Closed
entropitor opened this issue Mar 17, 2020 · 15 comments
Closed

HTTP2 over Unix Domain Sockets requires :authority header #32326

entropitor opened this issue Mar 17, 2020 · 15 comments

Comments

@entropitor
Copy link

  • Version: 13.10.1
  • Platform: mac, linux
  • Subsystem: /

What steps will reproduce the bug?

It seems that opening an HTTP2 server over unix domain sockets requires the :authority header to be set to localhost. However this breaks grpc as most implementations don't send that header

See also https://github.com/entropitor/grpc-js-bug-258

When adding -authority localhost to the unix one, it works:

grpcurl -d '{"name": "test"}' -import-path ./src/proto -proto greeting.proto -plaintext -unix -authority localhost /tmp/test.sock Greeter/SayHello

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

The -authority flag / header should not be passed

What do you see instead?

A connection reset because the authority header is not passed

Additional information

See also ticket grpc/grpc-node#258 (comment)

@mildsunrise
Copy link
Member

According to RFC7540, either :authority or host must always be present in an HTTP/2 request. HTTP/1.1 also requires the Host header.

Node.JS currently forbids the host header and forces :authority to be present, this is a pending issue (#29858). But those implementations should be sending a value for host or :authority, could you confirm this is the case?

@entropitor
Copy link
Author

Okay, I've investigated a bit more and it seems like it's failing because the go-client sets :authority to the file path (e.g. /tmp/test.sock) and since that contains slashes, nodeJS rejects it as the file path is not a valid authority 😞

I guess NodeJS is following the spec. It's just a bit pitty but I guess it's nothing node can reasonably do 😭

@mildsunrise
Copy link
Member

Since there is no standard for how to do HTTP/2 on UDS, maybe we could exceptionally (for UDS only) allow / and other characters in :authority, or lift the requirement for :authority.

It would break applications that forward requests to a regular TCP session, among other things. I'm not sure it's a good idea.

@dougwilson
Copy link
Member

I mean, it's not that UDS is just unspecified, but the rfc defines it to specifically be over TCP, I believe:

An HTTP/2 connection is an application-layer protocol running on top of a TCP connection ([TCP]).

This is similar to how the new HTTP/3 is defined to be over QUIC, but differs from HTTP/1 which says any reliable transport.

(All of the above is just my own understanding of the specs)

@gireeshpunathil
Copy link
Member

AFAIK, UDS that are SOCK_STREAM essentially rides on TCP protocol, with only difference in the actual carrier (disk instead of network card)

@dougwilson
Copy link
Member

Ultimately, the thing in question here is the authority. It sounds like you are saying the authority would be the same, as you are saying the only difference between UDS and TCP is the actual carrier. So should the authority for UDS be localhost ?

@gireeshpunathil
Copy link
Member

gireeshpunathil commented Mar 27, 2020

So should the authority for UDS be localhost ?

Yes, I think so.

@dougwilson
Copy link
Member

Yea, that is what I'm kind of thinking too. The HTTP/2 is pretty specific about what is allowed in the :authority field, so it does really seem like the clients would need to change in this case.

@mildsunrise
Copy link
Member

:authority should identify the endpoint you are connecting to. Using localhost for all UDS sessions doesn't feel right... but +1 anyway for practicality.

AFAIK, UDS that are SOCK_STREAM essentially rides on TCP protocol, with only difference in the actual carrier (disk instead of network card)

UDS has stream semantics, but shouldn't use TCP at all.

@gireeshpunathil
Copy link
Member

@mildsunrise -

but shouldn't use TCP at all.

can you better qualify shouldn't for my understanding? do you mean UDS stream semantics DO NOT use TCP, or do you mean it does, but IT IS NOT SUPPOSED to use?

if you mean the first, can you point me to a doc that says so? when I checked, all the life cycle management functions of UDS invoke the same TCP APIs - socket, bind, setsockopt ...

@addaleax
Copy link
Member

@gireeshpunathil socket, bind, setsockopt etc. are not TCP APIs, they are shared by all socket types.

However, setsockopt() with TCP-specific parameters will fail on a UDS stream and vice versa, because they are different protocols.

I don’t know how much code is shared between the TCP and Unix domain socket streams implementations in kernels, but I can’t imagine it’s much given how much more complex TCP is, and @mildsunrise knows better than me anyway.

@mildsunrise
Copy link
Member

Yes... in fact UDS skips the whole network stack (interfaces, firewall, routing, traffic control, etc). They're more like a FIFO once the connection is established, AFAIK.

@entropitor
Copy link
Author

Could anybody give me pointers as to where I would need to start if I want to temporarily support this anyway in a fork?

(Given this will take a long while to be fixed in upstream go grpc library, let alone kubernetes, we're probably speaking > 1 year before we can use normal node, so in between a fork might be useful, but I wouldn't know where to start)

@sam-github
Copy link
Contributor

It shouldn't take greater than a year for Node.js to fix an interop problem with go. I think what's needed is just a plan of what to do. Worst/most-complex case, node could have a go-compatible but standard non-conformant behaviour that was enabled with either a http2 runtime option or CLI/env var setting.

@entropitor
Copy link
Author

That would be awesome.

This is the actual bug in grpc-go (grpc/grpc-go#2628). The problem is that they send the file path as authority header with the slashes in it which is not a valid header, so node should accept this in this mode.

Potentially this mode could be automatically enabled for UDS (which doesn't seem to have a real spec anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants