Do not require authority for UNIX domain sockets #487

saschagrunert · 2020-09-16T13:23:13Z

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 · 2020-09-16T15:18:50Z

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 · 2020-09-17T16:43:49Z

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 · 2020-09-17T18:17:48Z

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 · 2020-09-20T09:26:02Z

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 · 2020-09-21T12:42:23Z

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

saschagrunert · 2020-10-08T16:26:48Z

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 · 2020-10-08T18:34:21Z

src/server.rs

+            // When connecting to a UNIX Domain Socket (UDS), then we might get a path for the
+            // authority field. If it's a local path and exists, then we do not error in that case
+            // and assume an UDS.
+            if !Path::new(authority.as_str()).exists() {


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

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

saschagrunert · 2020-10-08T19:23:16Z

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.

This fixes connections where a local UNIX domain socket path is provided, where the authority contains the full path to the *.sock file or is empty. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert · 2020-10-30T10:39:55Z

PTAL @seanmonstar

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

Ruilkyu · 2021-03-05T13:13:47Z

Has 0.4.0 solved the problem？

arthurlm · 2021-03-22T15:49:06Z

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 · 2021-05-04T09:12:53Z

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

BogdanIonesq · 2021-08-23T16:10:34Z

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 · 2021-09-09T12:00:43Z

@seanmonstar PTAL

noboruma · 2021-12-22T14:52:23Z

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 · 2022-03-23T22:53:44Z

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 · 2022-03-24T08:47:09Z

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 · 2022-03-24T13:36:03Z

@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 · 2022-03-24T16:26:25Z

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 · 2022-03-26T08:10:21Z

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

arthurlm · 2022-03-28T13:34:34Z

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:

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).

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.

RFC 7540: HTTP V2@section-8.3. 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 · 2022-04-01T08:19:42Z

@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.

Copy from hyperium#487 Thanks for the help from Sascha Grunert.

Forsworns · 2022-11-18T05:42:34Z

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

saschagrunert force-pushed the authority branch from c4b2913 to 86979b3 Compare September 16, 2020 13:25

saschagrunert mentioned this pull request Sep 16, 2020

Make unix domain socket work with crictl containers/containrs#34

Open

saschagrunert force-pushed the authority branch 2 times, most recently from 95a0d59 to 69444f5 Compare September 16, 2020 15:08

saschagrunert force-pushed the authority branch 2 times, most recently from 0447e60 to 7482820 Compare September 21, 2020 12:33

saschagrunert force-pushed the authority branch from 7482820 to 1fc278f Compare September 21, 2020 12:43

seanmonstar reviewed Oct 8, 2020

View reviewed changes

saschagrunert force-pushed the authority branch 3 times, most recently from c993b8c to 1fc278f Compare October 8, 2020 19:22

saschagrunert force-pushed the authority branch from 1fc278f to 3a7d79b Compare October 30, 2020 10:15

saschagrunert changed the title ~~Do not always require an authority~~ Do not require authority for UNIX domain sockets Oct 30, 2020

saschagrunert force-pushed the authority branch 3 times, most recently from 812e70d to 3354233 Compare October 30, 2020 10:33

Do not always require an authority

1f99ad3

This fixes connections where a local UNIX domain socket path is provided, where the authority contains the full path to the *.sock file or is empty. Signed-off-by: Sascha Grunert <sgrunert@suse.com>

saschagrunert force-pushed the authority branch from 3354233 to 1f99ad3 Compare October 30, 2020 10:34

BogdanIonesq mentioned this pull request Aug 22, 2021

Stream terminated by RST_STREAM with PROTOCOL_ERROR when listening on unix domain socket hyperium/tonic#742

Closed

doffi88 mentioned this pull request Nov 3, 2021

Issue on gRPC over unix domain socket hyperium/tonic#826

Closed

saschagrunert mentioned this pull request Nov 4, 2021

Choose RPC framework containers/conmon-rs#3

Closed

arthurlm mentioned this pull request Apr 1, 2022

Deny requerst if :authority field is invalid only with CONNECT method #612

Open

rahulanand16nov mentioned this pull request May 30, 2022

Request with | (pipe) character in authority header is auto-rejected by Limitador Kuadrant/limitador#53

Closed

bachp approved these changes Aug 11, 2022

View reviewed changes

mingliangli-lml approved these changes Aug 24, 2022

View reviewed changes

mingliangli-lml added a commit to mingliangli-lml/h2 that referenced this pull request Aug 24, 2022

Do not require authority for UNIX domain sockets

613eb4f

Copy from hyperium#487 Thanks for the help from Sascha Grunert.

Forsworns mentioned this pull request Nov 18, 2022

FATA[0000] getting the runtime version: rpc error: code = Internal desc = stream terminated by RST_STREAM with error code: PROTOCOL_ERROR hyperium/tonic#576

Closed

kriswuollett mentioned this pull request Jan 26, 2024

Unix Domain Socket connection should use localhost authority header convention by default fullstorydev/grpcurl#442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not require authority for UNIX domain sockets #487

Do not require authority for UNIX domain sockets #487

saschagrunert commented Sep 16, 2020 •

edited

saschagrunert commented Sep 16, 2020 •

edited

seanmonstar commented Sep 17, 2020

saschagrunert commented Sep 17, 2020

saschagrunert commented Sep 20, 2020

saschagrunert commented Sep 21, 2020

saschagrunert commented Oct 8, 2020

seanmonstar left a comment

seanmonstar Oct 8, 2020

saschagrunert Oct 30, 2020

saschagrunert commented Oct 8, 2020

saschagrunert commented Oct 30, 2020

Ruilkyu commented Mar 5, 2021

arthurlm commented Mar 22, 2021

nox commented May 4, 2021

BogdanIonesq commented Aug 23, 2021

saschagrunert commented Sep 9, 2021

noboruma commented Dec 22, 2021 •

edited

bachp commented Mar 23, 2022

arthurlm commented Mar 24, 2022 •

edited

bachp commented Mar 24, 2022 •

edited

arthurlm commented Mar 24, 2022 •

edited

bachp commented Mar 26, 2022

arthurlm commented Mar 28, 2022 •

edited

bachp commented Apr 1, 2022

mingliangli-lml left a comment

Forsworns commented Nov 18, 2022

Do not require authority for UNIX domain sockets #487

Are you sure you want to change the base?

Do not require authority for UNIX domain sockets #487

Conversation

saschagrunert commented Sep 16, 2020 • edited

saschagrunert commented Sep 16, 2020 • edited

seanmonstar commented Sep 17, 2020

saschagrunert commented Sep 17, 2020

saschagrunert commented Sep 20, 2020

saschagrunert commented Sep 21, 2020

saschagrunert commented Oct 8, 2020

seanmonstar left a comment

Choose a reason for hiding this comment

seanmonstar Oct 8, 2020

Choose a reason for hiding this comment

saschagrunert Oct 30, 2020

Choose a reason for hiding this comment

saschagrunert commented Oct 8, 2020

saschagrunert commented Oct 30, 2020

Ruilkyu commented Mar 5, 2021

arthurlm commented Mar 22, 2021

nox commented May 4, 2021

BogdanIonesq commented Aug 23, 2021

saschagrunert commented Sep 9, 2021

noboruma commented Dec 22, 2021 • edited

bachp commented Mar 23, 2022

arthurlm commented Mar 24, 2022 • edited

bachp commented Mar 24, 2022 • edited

arthurlm commented Mar 24, 2022 • edited

bachp commented Mar 26, 2022

arthurlm commented Mar 28, 2022 • edited

bachp commented Apr 1, 2022

mingliangli-lml left a comment

Choose a reason for hiding this comment

Forsworns commented Nov 18, 2022

saschagrunert commented Sep 16, 2020 •

edited

saschagrunert commented Sep 16, 2020 •

edited

noboruma commented Dec 22, 2021 •

edited

arthurlm commented Mar 24, 2022 •

edited

bachp commented Mar 24, 2022 •

edited

arthurlm commented Mar 24, 2022 •

edited

arthurlm commented Mar 28, 2022 •

edited