Skip to content

Commit

Permalink
Revert default HTTP/2 settings sent by the client (#2346)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
idelpivnitskiy authored Sep 9, 2022
1 parent b7b71bc commit bb54241
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,15 @@
*/
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;

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;
Expand All @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -30,20 +29,23 @@
import static java.util.Objects.requireNonNull;

final class HttpConfig {
private final Consumer<H2ProtocolConfig> h2ConfigValidator;
@Nullable
private H1ProtocolConfig h1Config;
@Nullable
private H2ProtocolConfig h2Config;
private List<String> supportedAlpnProtocols;
private boolean allowDropTrailers;

HttpConfig() {
HttpConfig(final Consumer<H2ProtocolConfig> 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;
Expand Down Expand Up @@ -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()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {
Expand All @@ -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() {
Expand Down

0 comments on commit bb54241

Please sign in to comment.