Do not require authority for UNIX domain sockets

[saschagrunert]

This fixes connections where paths to UNIX domain sockets (*.sock) are provided or the authority is empty.

Refers to hyperium/tonic#243, containers/containrs#34

[saschagrunert]

Test are failing now for sure. In case of an unix socket, we would only get a path and nothing else. Do you see any feasible way to implement this in h2?

[seanmonstar]

The description mentions not requiring an authority, but it looks like this changes it to stop checking if the provided authority is valid. So, in the Unix socket case, does it send an :authority that is not a normal looking domain name?

[saschagrunert]

The description mentions not requiring an authority, but it looks like this changes it to stop checking if the provided authority is valid. So, in the Unix socket case, does it send an :authority that is not a normal looking domain name?

Unfortunately not, the authority would be the complete path, like /path/to/my/socket.sock

[saschagrunert]

Would it be okay to early exit the validation path if the parsed URI is a path? Not sure how to classify a “path” though…

[saschagrunert]

@seanmonstar I checked for a path. If this exists locally, then we assume that the connection through the UDS should work.

[saschagrunert]

Is there anyone over here to give this a review? 😇

[seanmonstar]

As I mentioned in grpc/grpc-go#3854 (comment), I think it's fine to ignore the :authority if it's a blank value. I would expect a client to not send an invalid authority.

[seanmonstar]

This will do a blocking IO call, which we wouldn't want to do in an async environment.

[saschagrunert]

Removed in favor of a check for a .sock suffix.

[saschagrunert]

As I mentioned in grpc/grpc-go#3854 (comment), I think it's fine to ignore the :authority if it's a blank value. I would expect a client to not send an invalid authority.

Yes, but in case of a unix domain socket the authority is not blank. It's the socket path.

[saschagrunert]

PTAL @seanmonstar

I changed the check to either exclude *.sock authorities or if the authority is empty.

[Ruilkyu]

Has 0.4.0 solved the problem？

[arthurlm]

Just want to add that I am facing a similar issue using h2 to process gRPC requests using tonic.
I have also open an issue #525

I am using h2 v0.3.1.

I have fixed the problem by just sending a warning when :authority is invalid instead of rejecting the requests.
See my fix here: master...arthurlm:master

Maybe we should find another more elegant solution than just ignoring *.sock or other allowed list :authority pattern ?

[nox]

Didn't grpc-go change to not emit those malformed authority headers?

[BogdanIonesq]

Any idea when this could be merged? Calling gRPC methods from Go clients or grpcurl on unix domain sockets still does not work due to this issue.

[saschagrunert]

@seanmonstar PTAL

[noboruma]

For anyone trying to get that working, the workaround is to simply send a dummy authority field from the client side:

• For grpcurl: add -authority "dummy"
• For grpc-go: use grpc.Dial("unix:///blah.sock", grpc.WithAuthority("dummy"), grpc.WithInsecure())

[bachp]

Changing this on the client side is currently not an option as I can't patch every kubelet.

Currently this issue is preventig me from properly implementing a Kubernetes Device plugin in Rust using Tonic.

[arthurlm]

I am exactly in the same situation: kubelet is the client and patch it is not an option for me either.

This is why few month ago I have fork this repository to disable panic if authority field is Err (ie. It has failed to be "parsed" but still contains some data).

Moreover, from k8s point of view, authority field is not invalid. It represent namespace from where the request came from. This is not just a valid http authority but it still contains meaningful network information.

In my case, forking unlock me and I have been able to make k8s extension works correctly. But from my point of view this is too bad request fail because an optional field cannot be parsed.

[bachp]

@saschagrunert I think only checking for .sock is kind of brittle, there is nothing forcing a socket to end with .sock. A better heuristic might be to check for a leading /

@seanmonstar What would be the harm if we just ignore an invalid authority instead of failing with an error?

So the change would essentially become bachp@0a3f73b

[arthurlm]

Totally agree on the fact there is nothing that enforce a socket file ending with .sock.

A better check for that would be to check file descriptor exists and check it is a valid socket file (but it will still not works for unnamed unix socket).

The patch you have made is exactly the same I have suggest around 1 year ago 😉 2c26eda .
I have also edit it later to keep log in case authority is not a valid http host (see master...arthurlm:master).

If any maintainers want I would be happy to make a PR for this or you could take the solution of @bachp without the log.
It would avoid more people forking for this and let us use h2 with tonic to create nice kubernetes rust extension 😁.

[bachp]

@arthurlm I think logging it makes a lot of sense.

[arthurlm]

Before creating any PR, I have done more investigation to understand clearly what is happening.

In my extension service k8s case the :authority field received looks like extension/<target namespace>/<target service>.

---

Looking at RFCs:

1. RFC 7540: HTTP V2@section 8.1.2.3. Request Pseudo-Header Fields.

The ":authority" pseudo-header field includes the authority
portion of the target URI ([RFC3986], Section 3.2). The authority
MUST NOT include the deprecated "userinfo" subcomponent for "http"
or "https" schemed URIs.

All HTTP/2 requests MUST include exactly one valid value for the
":method", ":scheme", and ":path" pseudo-header fields, unless it is
a CONNECT request (Section 8.3). An HTTP request that omits
mandatory pseudo-header fields is malformed (Section 8.1.2.6).

2. RFC 3986: Uniform Resource Identifier@section 3.2. Authority

Many URI schemes include a hierarchical element for a naming
authority so that governance of the name space defined by the
remainder of the URI is delegated to that authority (which may, in
turn, delegate it further).

The authority component is preceded by a double slash ("//") and is
terminated by the next slash ("/"), question mark ("?"), or number
sign ("#") character, or by the end of the URI.

Non-validating parsers (those that merely separate a URI reference into
its major components) will often ignore the subcomponent structure of
authority, treating it as an opaque string from the double-slash to
the first terminating delimiter, until such time as the URI is
dereferenced.

3. RFC 7540: HTTP [email protected]. The CONNECT Method

In HTTP/2, the CONNECT method is used to establish a tunnel over a
single HTTP/2 stream to a remote host for similar purposes. The HTTP
header field mapping works as defined in Section 8.1.2.3 ("Request
Pseudo-Header Fields"), with a few differences. Specifically:

o The ":authority" pseudo-header field contains the host and port to
connect to (equivalent to the authority-form of the request-target
of CONNECT requests (see [RFC7230], Section 5.3)).

A CONNECT request that does not conform to these restrictions is
malformed (Section 8.1.2.6).

So, from my understanding:

• Following (1):

  • :authority field is not mandatory in HTTP2
  • It should be a valid URI without scheme and userinfo (so k8s might be sending valid content. Valid URI example: data:foobar)

• Following (2):

  • k8s is sending invalid content in :authority
  • data should be split as :authority = extension and :path = <target namespace>/<target service>
  • :authority might be treated as opaque string by URI parser

• Following (3):

  • :authority field should be a valid host and port in the case of CONNECT method.

NOTE: I have not read the whole RFCs but just read in details the mentioned sections. If anyone have better understanding of this RFCs, please feel free to comment 😉 !

---

Looking at current code:

• h2 parse :authority at

  h2/src/server.rs

  Lines 1467 to 1476 in 3383ef7

  	if let Some(authority) = pseudo.authority {
  	let maybe_authority = uri::Authority::from_maybe_shared(authority.clone().into_inner());
  	parts.authority = Some(maybe_authority.or_else(|why| {
  	malformed!(
  	"malformed headers: malformed authority ({:?}): {}",
  	authority,
  	why,
  	)
  	})?);
  	}

• h2 rely on http crate to parse content
• if uri::Authority::from_maybe_shared fail, h2 forward the error and make the whole request "malformed"
• In k8s case, parse fail because it contains / character.
• If parts contains valid data it will be used to enhanced b
  

  h2/src/server.rs

  Line 1434 in 3383ef7

  let mut b = Request::builder();

  
  

  h2/src/server.rs

  Line 1520 in 3383ef7

  b = b.uri(parts);

• b will finally be used to build return object reading b.body()
  

  h2/src/server.rs

  Lines 1522 to 1534 in 3383ef7

  	let mut request = match b.body(()) {
  	Ok(request) => request,
  	Err(e) => {
  	// TODO: Should there be more specialized handling for different
  	// kinds of errors
  	proto_err!(stream: "error building request: {}; stream={:?}", e, stream_id);
  	return Err(Error::library_reset(stream_id, Reason::PROTOCOL_ERROR));
  	}
  	};
  	*request.headers_mut() = fields;
  	Ok(request)

  
  So, b.headers() part does not seems to be used 🤔 ?

---

I now think we are here in a corner case.

• There are arguments telling both h2 and k8s are right.
• For now, built Uri object does not look like to be directly used (except to filter malformed HTTP request)
• Fix can be done at multiple level
  • http crate (with an "opaque URI" parser)
  • h2 crate
  • kubernetes and all its extensions projects (way more harder ...)
• (We are really far away here from *.sock issue 😉, which BTW does not seems described in RFCs)

---

@seanmonstar / hyperium maintainers: what is your point on view on this ?

Should we consider :authority as an opaque string ?
Should we just ignore it when Authority::from_maybe_shared fail ?

Should we fail request if :authority is malformed only on CONNECT method (see change arthurlm@986308c) ? It better respect RFC. I have also tested it with k8s extension service and it still works. I personally think this is the best choice we have here.

Any other ideas or comments ?

[bachp]

@arthurlm I like your proposal to only ignore the malformed authority on CONNECT. Maybe just open a new MR with the proposed change to bring it more visibility from the maintainers.

[mingliangli-lml]

I encountered the same problem, this solution can be applied to the scenario of UDS address.

[Forsworns]

@arthurlm 18 days before, k8s approves a PR related to this. Seems now tonic and h2 works well with it.

[cristiano-prato]

Hi, this issue is still present, is there any plan to fix?

[Noah-Kennedy]

@seanmonstar I'm not sure how you would prefer this be tackled, so I'll leave the review to you here.

[chensk]

I encountered the same problem. I'm writing a rust grpc server that implements bazel remote cache protocol. Bazel reports "PROTOCOL ERROR" when unix domain socket used. Port is fine. Are we going to fix that?