From bb54241611bfb84f009ac57fea20979dd5e4676e Mon Sep 17 00:00:00 2001 From: Idel Pivnitskiy Date: Fri, 9 Sep 2022 12:24:50 -0500 Subject: [PATCH] Revert default HTTP/2 settings sent by the client (#2346) Motivation: #2341 changed default HTTP/2 settings that client sends to notify the server it does not support Server Push. Modification: - Set `ENABLE_PUSH=0, MAX_CONCURRENT_STREAMS=0` in settings frame that client sends to a server; - Validate custom initial settings that users pass to `H2ProtocolConfig` and throw if users try to override ENABLE_PUSH/MAX_CONCURRENT_STREAMS on the client or set ENABLE_PUSH other than 0 for the server; - Include content message in GO_AWAY frame if client receives a Push Stream frame; Result: Default client behavior doesn't change, custom settings validated to prevent programming mistakes. --- .../netty/H2ClientParentChannelInitializer.java | 7 ++++--- .../servicetalk/http/netty/HttpClientConfig.java | 16 +++++++++++++++- .../io/servicetalk/http/netty/HttpConfig.java | 15 ++++++--------- .../servicetalk/http/netty/HttpServerConfig.java | 11 ++++++++++- 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java index 9952d02912..ed8d845e57 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentChannelInitializer.java @@ -27,6 +27,7 @@ import io.netty.handler.codec.http2.Http2FrameCodecBuilder; import io.netty.handler.codec.http2.Http2MultiplexHandler; +import static io.netty.buffer.ByteBufUtil.writeAscii; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.servicetalk.http.netty.H2ServerParentChannelInitializer.initFrameLogger; import static io.servicetalk.http.netty.H2ServerParentChannelInitializer.toNettySettings; @@ -37,8 +38,7 @@ final class H2ClientParentChannelInitializer implements ChannelInitializer { H2ClientParentChannelInitializer(final H2ProtocolConfig config) { this.config = config; - final io.servicetalk.http.api.Http2Settings h2Settings = config.initialSettings(); - nettySettings = toNettySettings(h2Settings); + nettySettings = toNettySettings(config.initialSettings()).pushEnabled(false).maxConcurrentStreams(0L); } @Override @@ -93,7 +93,8 @@ private H2PushStreamHandler() { @Override public void channelRegistered(final ChannelHandlerContext ctx) { // See SETTINGS_ENABLE_PUSH in https://tools.ietf.org/html/rfc7540#section-6.5.2 - ctx.writeAndFlush(new DefaultHttp2GoAwayFrame(PROTOCOL_ERROR)); + ctx.writeAndFlush(new DefaultHttp2GoAwayFrame(PROTOCOL_ERROR, + writeAscii(ctx.alloc(), "Server Push is not supported"))); // Http2ConnectionHandler.processGoAwayWriteResult will close the connection after GO_AWAY is flushed } } diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java index a6efeec79e..e9a610601e 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpClientConfig.java @@ -15,6 +15,7 @@ */ package io.servicetalk.http.netty; +import io.servicetalk.http.api.Http2Settings; import io.servicetalk.tcp.netty.internal.TcpClientConfig; import io.servicetalk.transport.api.ClientSslConfig; import io.servicetalk.transport.api.DelegatingClientSslConfig; @@ -22,6 +23,7 @@ import java.util.List; import javax.annotation.Nullable; +import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH; import static io.servicetalk.http.netty.HttpServerConfig.httpAlpnProtocols; import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV4Address; import static io.servicetalk.utils.internal.NetworkUtils.isValidIpV6Address; @@ -41,7 +43,19 @@ final class HttpClientConfig { HttpClientConfig() { tcpConfig = new TcpClientConfig(); - protocolConfigs = new HttpConfig(); + protocolConfigs = new HttpConfig(h2Config -> { + final Http2Settings settings = h2Config.initialSettings(); + final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH); + if (pushEnabled != null && pushEnabled != 0) { + throw new IllegalArgumentException("Server Push is not supported by the client, " + + "expected SETTINGS_ENABLE_PUSH value is null or 0, settings=" + settings); + } + final Long maxConcurrentStreams = settings.maxConcurrentStreams(); + if (maxConcurrentStreams != null && maxConcurrentStreams != 0) { + throw new IllegalArgumentException("Server Push is not supported by the client, " + + "expected MAX_CONCURRENT_STREAMS value is null or 0, settings=" + settings); + } + }); } HttpClientConfig(final HttpClientConfig from) { diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpConfig.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpConfig.java index 078c3c1912..ddb457414b 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpConfig.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpConfig.java @@ -15,13 +15,12 @@ */ package io.servicetalk.http.netty; -import io.servicetalk.http.api.Http2Settings; import io.servicetalk.http.api.HttpProtocolConfig; import java.util.List; +import java.util.function.Consumer; import javax.annotation.Nullable; -import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH; import static io.servicetalk.http.netty.HttpProtocolConfigs.h1Default; import static java.util.Arrays.asList; import static java.util.Collections.emptyList; @@ -30,6 +29,7 @@ import static java.util.Objects.requireNonNull; final class HttpConfig { + private final Consumer h2ConfigValidator; @Nullable private H1ProtocolConfig h1Config; @Nullable @@ -37,13 +37,15 @@ final class HttpConfig { private List supportedAlpnProtocols; private boolean allowDropTrailers; - HttpConfig() { + HttpConfig(final Consumer h2ConfigValidator) { + this.h2ConfigValidator = requireNonNull(h2ConfigValidator); h1Config = h1Default(); h2Config = null; supportedAlpnProtocols = emptyList(); } HttpConfig(final HttpConfig from) { + this.h2ConfigValidator = from.h2ConfigValidator; this.h1Config = from.h1Config; this.h2Config = from.h2Config; this.supportedAlpnProtocols = from.supportedAlpnProtocols; @@ -107,12 +109,7 @@ private void h2Config(final H2ProtocolConfig h2Config) { if (this.h2Config != null) { throw new IllegalArgumentException("Duplicated configuration for HTTP/2 was found"); } - final Http2Settings settings = h2Config.initialSettings(); - final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH); - if (pushEnabled != null && pushEnabled != 0) { - throw new IllegalArgumentException("Http2Settings push is enabled but not supported. settings=" + settings); - } - + h2ConfigValidator.accept(h2Config); this.h2Config = h2Config; supportedAlpnProtocols = h1Config == null ? singletonList(h2Config.alpnId()) : unmodifiableList(asList(h1Config.alpnId(), h2Config.alpnId())); diff --git a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServerConfig.java b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServerConfig.java index 05c5e3ad51..a0b2bf2446 100644 --- a/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServerConfig.java +++ b/servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpServerConfig.java @@ -15,6 +15,7 @@ */ package io.servicetalk.http.netty; +import io.servicetalk.http.api.Http2Settings; import io.servicetalk.http.api.HttpLifecycleObserver; import io.servicetalk.tcp.netty.internal.TcpServerConfig; import io.servicetalk.transport.api.DelegatingServerSslConfig; @@ -26,6 +27,7 @@ import java.util.Map.Entry; import javax.annotation.Nullable; +import static io.netty.handler.codec.http2.Http2CodecUtil.SETTINGS_ENABLE_PUSH; import static java.util.Objects.requireNonNull; final class HttpServerConfig { @@ -37,7 +39,14 @@ final class HttpServerConfig { HttpServerConfig() { tcpConfig = new TcpServerConfig(); - httpConfig = new HttpConfig(); + httpConfig = new HttpConfig(h2Config -> { + final Http2Settings settings = h2Config.initialSettings(); + final Long pushEnabled = settings.settingValue(SETTINGS_ENABLE_PUSH); + if (pushEnabled != null && pushEnabled != 0) { + throw new IllegalArgumentException( + "Server cannot set SETTINGS_ENABLE_PUSH value other than 0, settings=" + settings); + } + }); } TcpServerConfig tcpConfig() {