Relax authority strictness for interoperability with other (existing) clients

[gila]

Feature Request

During the implementation of a CSI driver, and working with existing gRPC clients that require a UDS socket, all requests are failed with reason=PROTOCOL_ERROR.

Here is an example when testing the "Identity" service of such a driver:

$ ./csc -e /var/tmp/csi.sock identity plugin-info                                                          [±h2 ●●]
WARN[0000] debug mode enabled
DEBU[0000] assigned the root context
DEBU[0000] enabled request ID injector
DEBU[0000] enabled request logging
INFO[0000] /csi.v1.Identity/GetPluginInfo: REQ 0001: XXX_NoUnkeyedLiteral={}, XXX_sizecache=0
DEBU[0000] parsed endpoint info                          addr=/var/tmp/csi.sock proto=unix timeout=1m0s
stream terminated by RST_STREAM with error code: PROTOCOL_ERROR

From the server-side of things with RUST_LOG set to trace we can see:

DEBUG h2::server] malformed headers: malformed authority (b"/var/tmp/csi.sock"): invalid uri character
[2020-01-18T14:26:10Z TRACE h2::proto::streams::send] send_reset(..., reason=PROTOCOL_ERROR, stream=StreamId(1), ..., is_reset=false; is_closed=false; pending_send.is_empty=true; state=State { inner: Open { local: AwaitingHeaders, remote: Streaming } }

So the problem is pretty clear straight from here. We are sending an invalid (in context from HTTP for sure) an invalid authority and we error out at https://github.com/hyperium/http/blob/master/src/uri/authority.rs#L96

So in principle, the system works as designed. However according to RFC-3986:

For example, the "file" URI  scheme is defined so that no authority, an empty host, and
"localhost" all mean the end-user's machine, whereas the "http"   scheme considers a missing 
authority or empty host invalid.

Unfortunately, it seems to be the case that google, instead of using the "file://" scheme has invented its own scheme "unix://" there is a known issue for this filed here: grpc/grpc-go#2628

Regardless of the scheme used here it still would not work as the only scheme considered (and it looks to be implicit) is the http scheme, which makes sense as h2 states to be an HTTP server.

Currently, the way to we work around this is to patch h2 but this is suboptimal, and I can imagine other users might run into this problem as well. Although the problem itself simple, finding the right solution is not as the h2 crate and the HTTP crate are written to deal with HTTP.

Motivation

Would like to use tonic without the need to patch h2 or http

Proposal

Find an away to change/register handlers for schemes other than the http:// scheme or at least support unix:// and/or file:// ?

[LucioFranco]

Yeah, I think this issue would be better to be filed in h2 rather than her. So tonic itself is generic over the transport so you are not forced to use h2 but the transport layer that is provided (as an optional feature) uses hyper/h2 so I think it would make sense to see if you can apply the change there.

[gila]

@LucioFranco Thank you for your swift reply! I also thought that that would be the case, I'll ask there as well thanks again.

[saschagrunert]

@gila do you have any workaround for this? (Facing the same issue now)

[gila]

Hi @saschagrunert,

So the good news is -- that the grpc-go folks have fixed the problem of setting an empty authority header. The problem is, and depending on your use case, I have no idea when that will land in any of the containers/distro's you might be using.

So in the meantime, I've patched h2 to set it for us when it is empty. Doing so we were able to implement a CSI driver with tonic just fine. It's ugly, but it works. Perhaps for the time being you can do something similar to unblock your self. Below the hack, I applied for your convenience. Depending on the exact version of h2/tonic you are using it might be a bit different for the current version, this is against h2 0.2.6 IIRC.

From 8d17dfb3409ddaea3a9d7a723e7a011475770758 Mon Sep 17 00:00:00 2001
From: Jeffry Molanus <[email protected]>
Date: Mon, 4 Nov 2019 12:51:22 +0100
Subject: [PATCH] Allow authority header to be empty

---
 src/server.rs | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/src/server.rs b/src/server.rs
index 3c093f7e..75a8a0e4 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -1394,13 +1394,12 @@ impl proto::Peer for Peer {
         // header
         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,
-                )
-            })?);
+            parts.authority = Some(
+                match maybe_authority {
+                    Ok(val) => val,
+                    Err(_) => uri::Authority::from_maybe_shared(Bytes::from("example.com")).unwrap(),
+                }
+            );
         }

[saschagrunert]

Thank you for the quick reply @gila ❤️

So the good news is -- that the grpc-go folks have fixed the problem of setting an empty authority header.

Do you have a PR at hand where this got merged? 😇

[gila]

grpc/grpc-go#3730 :)

[saschagrunert]

grpc/grpc-go#3730 :)

I still get the same error even if I apply the patch to tools like crictl:

[2020-09-16T10:18:04Z DEBUG h2::server] malformed headers: malformed authority (b"/var/run/user/1000/cri/cri.sock"): invalid uri character

Edit: With the latest tonic master it seems to work.