From 6e89919e3265eadd33dc7e154ba291d46563f286 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Mon, 13 Sep 2021 11:30:19 -0700
Subject: [PATCH] netty: Requests with Connection header are malformed

Although this is part of HTTP/2 and should have already been handled
already, it was noticed as part of RBAC work to avoid matching
hop-by-hop headers. See gRFC A41.

Also add a warning if creating Metadata.Key for "Connection". Use this
to try to help diagnose a client if it happens to blindly copy headers
from HTTP/1, as PROTOCOL_ERROR is hard to debug.
---
 api/src/main/java/io/grpc/Metadata.java       | 12 ++++++++++++
 .../io/grpc/netty/GrpcHttp2HeadersUtils.java  |  5 +++++
 .../io/grpc/netty/NettyServerHandler.java     |  7 +++++++
 .../io/grpc/netty/NettyServerHandlerTest.java | 19 +++++++++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/api/src/main/java/io/grpc/Metadata.java b/api/src/main/java/io/grpc/Metadata.java
index e153fd5569..9c2a2227f8 100644
--- a/api/src/main/java/io/grpc/Metadata.java
+++ b/api/src/main/java/io/grpc/Metadata.java
@@ -40,6 +40,8 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Set;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import javax.annotation.concurrent.NotThreadSafe;
@@ -54,6 +56,7 @@ import javax.annotation.concurrent.NotThreadSafe;
  */
 @NotThreadSafe
 public final class Metadata {
+  private static final Logger logger = Logger.getLogger(Metadata.class.getName());
 
   /**
    * All binary headers should have this suffix in their names. Vice versa.
@@ -733,6 +736,15 @@ public final class Metadata {
     private static String validateName(String n, boolean pseudo) {
       checkNotNull(n, "name");
       checkArgument(!n.isEmpty(), "token must have at least 1 tchar");
+      if (n.equals("connection")) {
+        logger.log(
+            Level.WARNING,
+            "Metadata key is 'Connection', which should not be used. That is used by HTTP/1 for "
+            + "connection-specific headers which are not to be forwarded. There is probably an "
+            + "HTTP/1 conversion bug. Simply removing the Connection header is not enough; you "
+            + "should remove all headers it references as well. See RFC 7230 section 6.1",
+            new RuntimeException("exception to show backtrace"));
+      }
       for (int i = 0; i < n.length(); i++) {
         char tChar = n.charAt(i);
         if (pseudo && tChar == ':' && i == 0) {
diff --git a/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java b/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
index 4bdef93ad0..04cf7f4195 100644
--- a/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
+++ b/netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java
@@ -145,6 +145,11 @@ class GrpcHttp2HeadersUtils {
       return null;
     }
 
+    @Override
+    public boolean contains(CharSequence name) {
+      return get(name) != null;
+    }
+
     @Override
     public CharSequence status() {
       return get(Http2Headers.PseudoHeaderName.STATUS.value());
diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
index 6fca656e79..ee21129b67 100644
--- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
+++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
@@ -26,6 +26,7 @@ import static io.grpc.netty.Utils.CONTENT_TYPE_HEADER;
 import static io.grpc.netty.Utils.HTTP_METHOD;
 import static io.grpc.netty.Utils.TE_HEADER;
 import static io.grpc.netty.Utils.TE_TRAILERS;
+import static io.netty.handler.codec.http.HttpHeaderNames.CONNECTION;
 import static io.netty.handler.codec.http2.DefaultHttp2LocalFlowController.DEFAULT_WINDOW_UPDATE_RATIO;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -375,6 +376,12 @@ class NettyServerHandler extends AbstractNettyHandler {
   private void onHeadersRead(ChannelHandlerContext ctx, int streamId, Http2Headers headers)
       throws Http2Exception {
     try {
+      // Connection-specific header fields makes a request malformed. Ideally this would be handled
+      // by Netty. RFC 7540 section 8.1.2.2
+      if (headers.contains(CONNECTION)) {
+        resetStream(ctx, streamId, Http2Error.PROTOCOL_ERROR.code(), ctx.newPromise());
+        return;
+      }
 
       // Remove the leading slash of the path and get the fully qualified method name
       CharSequence path = headers.path();
diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
index 961f983d9c..efae5c0498 100644
--- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
+++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
@@ -537,6 +537,25 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
     stream = streamCaptor.getValue();
   }
 
+  @Test
+  public void headersWithConnectionHeaderShouldFail() throws Exception {
+    manualSetUp();
+    Http2Headers headers = new DefaultHttp2Headers()
+        .method(HTTP_METHOD)
+        .set(CONTENT_TYPE_HEADER, CONTENT_TYPE_GRPC)
+        .set(AsciiString.of("connection"), CONTENT_TYPE_GRPC)
+        .path(new AsciiString("/foo/bar"));
+    ByteBuf headersFrame = headersFrame(STREAM_ID, headers);
+    channelRead(headersFrame);
+
+    verifyWrite()
+        .writeRstStream(
+            eq(ctx()),
+            eq(STREAM_ID),
+            eq(Http2Error.PROTOCOL_ERROR.code()),
+            any(ChannelPromise.class));
+  }
+
   @Test
   public void keepAliveManagerOnDataReceived_headersRead() throws Exception {
     manualSetUp();