Repository: netty/netty
Issue: HTTP/2 Draft 17
Labels: feature

New drafts are out.  We should evaluate and make necessary updates.

https://tools.ietf.org/html/draft-ietf-httpbis-http2-17
~~https://tools.ietf.org/html/draft-ietf-httpbis-header-compression-11~~
https://tools.ietf.org/html/draft-ietf-httpbis-header-compression-12

We should coordinate with https://github.com/twitter/hpack if necessary.


Issue comments:
@nmittler @jpinner FYI.

HPACK-11 didn't last very long :)  We now have draft 12.

Note that the issue description and links have been updated.

@Scottmitch ha! ... thanks for keeping on top of this!

Will release hpack-1.0.0 once the spec gets published as an RFC. There will likely be minor API breaking since "Header Table" has been renamed in the spec to "Dynamic Table" (i.e. maxHeaderTableSize -> maxDynamicTableSize).

@jpinner - That shouldn't be a problem.  Thanks for the heads up.

I have seen a lot of chatter on the email thread about fixing static tables...do you think there is a draft 13 on the horizon?

I am reviewing the draft 17 specification.  There are a few changes to the spec which appear to be made for clerical purposes but may have unintended implications.  I have opened issues on the http2-spec to get more clarity.
~~https://github.com/http2/http2-spec/issues/726~~ - No change. Spec clarified to indicate perspective of the stream states being called out.
~~https://github.com/http2/http2-spec/issues/727~~ - No change. Spec clarified to expand stream states which SETTINGS can impact for window update.
~~https://github.com/http2/http2-spec/issues/730~~ - No change. Redundancy in spec removed in draft 17, but there was still text from previous drafts which keeps 0 padding restriction.

@Scottmitch great work ... thanks for doing this!

@nmittler - What are your thoughts on the following addition in [section 8.2](https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-8.2):

> The server MUST include a value in the ":authority" header field for
>    which the server is authoritative (see Section 10.1).  A client MUST
>    treat a PUSH_PROMISE for which the server is not authoritative as a
>    stream error (Section 5.4.2) of type PROTOCOL_ERROR.

Should we be providing an additional interface used to verify "authority" that the user must provide?  Or should we follow the approach we used with the "cacheability" which is just to add [comments](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java#L160)?  Verifying authority can be relatively involved and context dependent, but we could have a simple interface which forces the issue (which we decided against doing for the "cacheability" case).

+1 for a simple interface.  Something like:

``` java
interface AuthorityVerifier {
    void verifyAuthority(String authority) throws Http2Exception;
}
```

And they provide a default that always allows the authority.

I suppose we could do something similar for cacheability, but let's leave it as-is for now.

WDYT?

@nmittler - Yes I like making it explicit...comments are too easy to miss.

Draft changes have been cherry-picked. I'm going to open a separate issue for updating HPACK.

See https://github.com/netty/netty/issues/3535 for HPACK updates.


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
index d96dd9e24f5..feff3c14bae 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
@@ -19,6 +19,7 @@
 import static io.netty.handler.codec.http2.Http2Error.STREAM_CLOSED;
 import static io.netty.handler.codec.http2.Http2Exception.connectionError;
 import static io.netty.handler.codec.http2.Http2Exception.streamError;
+import static io.netty.handler.codec.http2.Http2PromisedRequestVerifier.ALWAYS_VERIFY;
 import static io.netty.handler.codec.http2.Http2Stream.State.CLOSED;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 import io.netty.buffer.ByteBuf;
@@ -42,6 +43,7 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
     private final Http2ConnectionEncoder encoder;
     private final Http2FrameReader frameReader;
     private final Http2FrameListener listener;
+    private final Http2PromisedRequestVerifier requestVerifier;
     private boolean prefaceReceived;
 
     /**
@@ -53,6 +55,7 @@ public static class Builder implements Http2ConnectionDecoder.Builder {
         private Http2ConnectionEncoder encoder;
         private Http2FrameReader frameReader;
         private Http2FrameListener listener;
+        private Http2PromisedRequestVerifier requestVerifier = ALWAYS_VERIFY;
 
         @Override
         public Builder connection(Http2Connection connection) {
@@ -89,6 +92,12 @@ public Builder encoder(Http2ConnectionEncoder encoder) {
             return this;
         }
 
+        @Override
+        public Http2ConnectionDecoder.Builder requestVerifier(Http2PromisedRequestVerifier requestVerifier) {
+            this.requestVerifier = requestVerifier;
+            return this;
+        }
+
         @Override
         public Http2ConnectionDecoder build() {
             return new DefaultHttp2ConnectionDecoder(this);
@@ -105,6 +114,7 @@ protected DefaultHttp2ConnectionDecoder(Builder builder) {
         lifecycleManager = checkNotNull(builder.lifecycleManager, "lifecycleManager");
         encoder = checkNotNull(builder.encoder, "encoder");
         listener = checkNotNull(builder.listener, "listener");
+        requestVerifier = checkNotNull(builder.requestVerifier, "requestVerifier");
         if (connection.local().flowController() == null) {
             connection.local().flowController(
                     new DefaultHttp2LocalFlowController(connection, encoder.frameWriter()));
@@ -508,6 +518,22 @@ public void onPushPromiseRead(ChannelHandlerContext ctx, int streamId, int promi
                       parentStream.id(), parentStream.state());
             }
 
+            if (!requestVerifier.isAuthoritative(ctx, headers)) {
+                throw streamError(promisedStreamId, PROTOCOL_ERROR,
+                        "Promised request on stream %d for promised stream %d is not authoritative",
+                        streamId, promisedStreamId);
+            }
+            if (!requestVerifier.isCacheable(headers)) {
+                throw streamError(promisedStreamId, PROTOCOL_ERROR,
+                        "Promised request on stream %d for promised stream %d is not known to be cacheable",
+                        streamId, promisedStreamId);
+            }
+            if (!requestVerifier.isSafe(headers)) {
+                throw streamError(promisedStreamId, PROTOCOL_ERROR,
+                        "Promised request on stream %d for promised stream %d is not known to be safe",
+                        streamId, promisedStreamId);
+            }
+
             // Reserve the push stream based with a priority based on the current stream's priority.
             connection.remote().reservePushStream(promisedStreamId, parentStream);
 
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
index 70f62dc4e1a..4f4a3b31dbc 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
@@ -86,7 +86,7 @@ public void addHeader(byte[] key, byte[] value, boolean sensitive) {
             // Default handler for any other types of errors that may have occurred. For example,
             // the the Header builder throws IllegalArgumentException if the key or value was invalid
             // for any reason (e.g. the key was an invalid pseudo-header).
-            throw connectionError(PROTOCOL_ERROR, e, e.getMessage());
+            throw connectionError(COMPRESSION_ERROR, e, e.getMessage());
         } finally {
             try {
                 in.close();
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
index fc330620d95..308ac81ea6c 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2CodecUtil.java
@@ -37,8 +37,8 @@ public final class Http2CodecUtil {
     public static final int CONNECTION_STREAM_ID = 0;
     public static final int HTTP_UPGRADE_STREAM_ID = 1;
     public static final String HTTP_UPGRADE_SETTINGS_HEADER = "HTTP2-Settings";
-    public static final String HTTP_UPGRADE_PROTOCOL_NAME = "h2c-16";
-    public static final String TLS_UPGRADE_PROTOCOL_NAME = "h2-16";
+    public static final String HTTP_UPGRADE_PROTOCOL_NAME = "h2c-17";
+    public static final String TLS_UPGRADE_PROTOCOL_NAME = "h2-17";
 
     public static final int PING_FRAME_PAYLOAD_LENGTH = 8;
     public static final short MAX_UNSIGNED_BYTE = 0xFF;
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionDecoder.java
index 9741c50aa4d..ee6604ae7d3 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionDecoder.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionDecoder.java
@@ -32,7 +32,6 @@ public interface Http2ConnectionDecoder extends Closeable {
      * Builder for new instances of {@link Http2ConnectionDecoder}.
      */
     interface Builder {
-
         /**
          * Sets the {@link Http2Connection} to be used when building the decoder.
          */
@@ -63,6 +62,11 @@ interface Builder {
          */
         Builder encoder(Http2ConnectionEncoder encoder);
 
+        /**
+         * Sets the {@link Http2PromisedRequestVerifier} used when building the decoder.
+         */
+        Builder requestVerifier(Http2PromisedRequestVerifier requestVerifier);
+
         /**
          * Creates a new decoder instance.
          */
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
index a834b866d0c..26072a02eb6 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameListener.java
@@ -22,7 +22,6 @@
  * An listener of HTTP/2 frames.
  */
 public interface Http2FrameListener {
-
     /**
      * Handles an inbound {@code DATA} frame.
      *
@@ -157,11 +156,8 @@ void onPriorityRead(ChannelHandlerContext ctx, int streamId, int streamDependenc
     /**
      * Handles an inbound PUSH_PROMISE frame. Only called if END_HEADERS encountered.
      * <p>
-     * Promised requests MUST be cacheable
-     * (see <a href="https://tools.ietf.org/html/rfc7231#section-4.2.3">[RFC7231], Section 4.2.3</a>) and
-     * MUST be safe (see <a href="https://tools.ietf.org/html/rfc7231#section-4.2.1">[RFC7231], Section 4.2.1</a>).
-     * If these conditions do not hold the application MUST throw a {@link Http2Exception.StreamException} with
-     * error type {@link Http2Error#PROTOCOL_ERROR}.
+     * Promised requests MUST be authoritative, cacheable, and safe.
+     * See <a href="https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-8.2">[RFC http2], Seciton 8.2</a>.
      * <p>
      * Only one of the following methods will be called for each HEADERS frame sequence.
      * One will be called when the END_HEADERS flag has been received.
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2PromisedRequestVerifier.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2PromisedRequestVerifier.java
new file mode 100644
index 00000000000..927781f910a
--- /dev/null
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2PromisedRequestVerifier.java
@@ -0,0 +1,72 @@
+/*
+ * Copyright 2015 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License, version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at:
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package io.netty.handler.codec.http2;
+
+import io.netty.channel.ChannelHandlerContext;
+
+/**
+ * Provides an extensibility point for users to define the validity of push requests.
+ * @see <a href="https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-8.2">[RFC http2], Section 8.2</a>.
+ */
+public interface Http2PromisedRequestVerifier {
+    /**
+     * Determine if a {@link Http2Headers} are authoritative for a particular {@link ChannelHandlerContext}.
+     * @param ctx The context on which the {@code headers} where received on.
+     * @param headers The headers to be verified.
+     * @return {@code true} if the {@code ctx} is authoritative for the {@code headers}, {@code false} otherwise.
+     * @see
+     * <a href="https://tools.ietf.org/html/draft-ietf-httpbis-http2-17#section-10.1">[RFC http2], Section 10.1</a>.
+     */
+    boolean isAuthoritative(ChannelHandlerContext ctx, Http2Headers headers);
+
+    /**
+     * Determine if a request is cacheable.
+     * @param headers The headers for a push request.
+     * @return {@code true} if the request associated with {@code headers} is known to be cacheable,
+     * {@code false} otherwise.
+     * @see <a href="https://tools.ietf.org/html/rfc7231#section-4.2.3">[RFC 7231], Section 4.2.3</a>.
+     */
+    boolean isCacheable(Http2Headers headers);
+
+    /**
+     * Determine if a request is safe.
+     * @param headers The headers for a push request.
+     * @return {@code true} if the request associated with {@code headers} is known to be safe,
+     * {@code false} otherwise.
+     * @see <a href="https://tools.ietf.org/html/rfc7231#section-4.2.1">[RFC 7231], Section 4.2.1</a>.
+     */
+    boolean isSafe(Http2Headers headers);
+
+    /**
+     * A default implementation of {@link Http2PromisedRequestVerifier} which always returns positive responses for
+     * all verification challenges.
+     */
+    Http2PromisedRequestVerifier ALWAYS_VERIFY = new Http2PromisedRequestVerifier() {
+        @Override
+        public boolean isAuthoritative(ChannelHandlerContext ctx, Http2Headers headers) {
+            return true;
+        }
+
+        @Override
+        public boolean isCacheable(Http2Headers headers) {
+            return true;
+        }
+
+        @Override
+        public boolean isSafe(Http2Headers headers) {
+            return true;
+        }
+    };
+}


Test patch: