Add possibility to use http2 preface with https connection #4704

0lejk4 · 2023-02-26T20:18:33Z

Motivation:

Add the possibility to use http2 preface with HTTPS connection for cases when the server does not support ALPN negotiation

Modifications:

HttpClientPipelineConfigurator will try connecting with HTTP/2 Preface when the option is enabled and downgrade to HTTP/1 if the server does not support it.
Changed HttpSession.retryWithH1C to HttpSession.retryWith method that receives with which SessionProtocol to retry
Changed HttpSessionHandler to comply with HttpSession change

Result:

Closes Option to force HTTP/2 connection over TLS without negotiation #4702
Users can use the HTTP/2 Preface option for secure connections

CLAassistant · 2023-02-26T20:18:37Z

All committers have signed the CLA.

trustin · 2023-02-27T02:31:31Z

Hey, thanks a lot for your PR. I think it makes sense to add a way to deal with an HTTP/2 server that doesn't support ALPN. However, it'd be nice if this new behavior is enabled only when a user explicitly tells so. Currently, useHttp2Preface is true by default, which means the server that explicitly told the client that it doesn't support HTTP/2 will get HTTP/2 connection preface.

Therefore, could you avoid using useHttp2Preface as a way to control this behavior but add a new option to ClientFactoryOptions for this behavior? e.g. ClientFactoryOptions.USE_HTTP2_WITHOUT_ALPN (disabled by default)

I also would like to ask the following changes:

There are two ways to start an HTTP/2 connection - 1) HTTP/2 connection preface and 2) HTTP/1-to-2 upgrade request. Your PR supports only the former. Could you also handle the case (2) when useHttp2Preface is false?
Could you add a test case for the new option? It should test the following 4 scenarios:

HTTP/2 server without ALPN HTTP/1-only server without ALPN

useHttp2Preface = true
(Connection preface) ✔ ✔*

useHttp2Preface = false
(Upgrade request) ✔ ✔*

*: Also make sure SessionProtocolNegotiationCache is updated accordingly.

0lejk4 · 2023-02-27T08:20:00Z

Wanted to clarify, so I can reuse the Upgrade logic from:
https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java#L313
And extract it to some upgrade method and same for this:
https://github.com/line/armeria/blob/master/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java#L295
So the solution will look somewhat like this:

jrhee17 · 2023-02-27T08:30:56Z

So the solution will look somewhat like this:

I think the code you shared looks similar to how I imagined it 👍

…er that doesn't support ALPN.

0lejk4 · 2023-02-27T11:13:49Z

Committed the USE_HTTP2_WITHOUT_ALPN option support and now is time to deal with test cases for it. Is there some similar test case that I could look into? If not I guess I need to start the server with tlsSelfSigned and mock io.netty.handler.ssl.SslHandler#applicationProtocol on the client to simulate negotiation failure. Any thoughts on this?

ikhoon · 2023-02-27T13:33:37Z

Is there some similar test case that I could look into?

You can build a simple HTTP/2 over TLS server using the combination of NettyServerExtension, SelfSignedCertificateExtension and SimpleH2CServerHandler. A SniHandler and ApplicationProtocolNegotiationHandler need to be prepended to SimpleH2CServerHandler.

For example:

@RegisterExtension
static SelfSignedCertificateExtension ssc = new SelfSignedCertificateExtension();

@RegisterExtension
static NettyServerExtension h2Server = new NettyServerExtension() {
    @Override
    protected void configure(Channel ch) throws Exception {

        final SslContext sslContext = SslContextBuilder
                .forServer(ssc.privateKey(), ssc.certificate())
                .build();
        ch.pipeline().addLast(new SniHandler(new DomainWildcardMappingBuilder<>(sslContext).build()));
        ch.pipeline().addLast(new ApplicationProtocolNegotiationHandler("???") {

            @Override
            protected void configurePipeline(ChannelHandlerContext ctx, String protocol) throws Exception {
                ctx.pipeline().addLast(new H2ConnectionHandlerBuilder().build());
            }
        });
    }
};

class H2ConnectionHandler extends SimpleH2CServerHandler {
    H2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                        Http2Settings initialSettings) {
        super(decoder, encoder, initialSettings);
    }
}

class H2ConnectionHandlerBuilder
        extends AbstractHttp2ConnectionHandlerBuilder<H2ConnectionHandler, H2ConnectionHandlerBuilder> {

    @Override
    protected H2ConnectionHandler build() {
        return super.build();
    }

    @Override
    protected H2ConnectionHandler build(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                                        Http2Settings initialSettings) throws Exception {
        final H2ConnectionHandler handler = new H2ConnectionHandler(decoder, encoder,
                                                                    initialSettings);
        frameListener(handler);
        return handler;
    }
}
}

0lejk4 · 2023-02-27T16:14:28Z

Thanks, this seems to be working for the preface case, but for the upgrade case I tried this and it is not working:

import io.netty.handler.codec.http.HttpServerCodec;
import io.netty.handler.codec.http.HttpServerUpgradeHandler;
import io.netty.handler.codec.http2.*;

static final NettyServerExtension server = new NettyServerExtension() {
        @Override
        protected void configure(Channel ch) throws Exception {
            final SslContext sslContext = SslContextBuilder
                    .forServer(serverCertificate.privateKey(), serverCertificate.certificate())
                    .build();
            ch.pipeline().addLast(new SniHandler(new DomainWildcardMappingBuilder<>(sslContext).build()));
            ch.pipeline().addLast(new ApplicationProtocolNegotiationHandler("???") {
                @Override
                protected void configurePipeline(ChannelHandlerContext ctx, String protocol) throws Exception {
                    final H2ConnectionHandler handler = new H2ConnectionHandlerBuilder().build();
                    final HttpServerCodec sourceCodec = new HttpServerCodec();
                    final HttpServerUpgradeHandler upgradeHandler = new HttpServerUpgradeHandler(sourceCodec, c -> new Http2ServerUpgradeCodec(handler), 65536);

                    ctx.pipeline().addLast(new LoggingHandler(getClass()));
                    ctx.pipeline().addLast(upgradeHandler);
                }
            });
        }
    };

18:01:33.754 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 - R:/127.0.0.1:59674] USER_EVENT: SslHandshakeCompletionEvent(SUCCESS)
18:01:33.754 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 - R:/127.0.0.1:59674] READ COMPLETE
18:01:36.694 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 - R:/127.0.0.1:59674] USER_EVENT: SslCloseCompletionEvent(SUCCESS)
18:01:36.694 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 - R:/127.0.0.1:59674] READ COMPLETE
18:01:36.696 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 - R:/127.0.0.1:59674] READ COMPLETE
18:01:36.697 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 ! R:/127.0.0.1:59674] INACTIVE
18:01:36.697 [nioEventLoopGroup-3-1] DEBUG c.l.a.c.Http2ClientWithoutALPNTest$1$1 - [id: 0x37266fbf, L:/127.0.0.1:59673 ! R:/127.0.0.1:59674] UNREGISTERED

Do you have any ideas? Currently stuck on it(
Also, this only covers HTTP/2 server without ALPN and as per HTTP/1-only server without ALPN is there similar class like SimpleH2CServerHandler for it?

ikhoon · 2023-02-28T07:34:00Z

Do you have any ideas? Currently stuck on it(

A channelActive event makes UpgradeRequestHandler send OPTIONS * HTTP/1.1.

armeria/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

Lines 426 to 432 in f2da76b

    
                   /** 
        
                    * Sends the initial upgrade request, which is {@code "OPTIONS * HTTP/1.1"}. 
        
                    */ 
        
                   @Override 
        
                   public void channelActive(ChannelHandlerContext ctx) throws Exception { 
        
                       final FullHttpRequest upgradeReq = 
        
                               new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*");

It is working for HTTP since UpgradeRequestHandler is added before a channel gets active.
However, UpgradeRequestHandler is added after the TLS handshake for HTTPS, so UpgradeRequestHandler can't get a channelActive event. As a workaround, we can use handlerAdded() event for HTTPS.

trustin · 2023-02-28T09:07:34Z

The server we're trying to test should actually not support ALPN, so it's probably better assuming that you write a plaintext HTTP/2 server that supports upgrade requests, and then insert an SslHandler in the beginning of Netty pipeline, with ALPN disabled. You probably might want to check how we implemented plaintext HTTP in HttpServerPipelineConfigurator..?

0lejk4 · 2023-03-01T22:10:21Z

Do you have any ideas? Currently stuck on it(

A channelActive event makes UpgradeRequestHandler send OPTIONS * HTTP/1.1.

armeria/core/src/main/java/com/linecorp/armeria/client/HttpClientPipelineConfigurator.java

Lines 426 to 432 in f2da76b

/**

* Sends the initial upgrade request, which is {@code "OPTIONS * HTTP/1.1"}.

*/

@Override

public void channelActive(ChannelHandlerContext ctx) throws Exception {

final FullHttpRequest upgradeReq =

new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.OPTIONS, "*");

It is working for HTTP since UpgradeRequestHandler is added before a channel gets active.
However, UpgradeRequestHandler is added after the TLS handshake for HTTPS, so UpgradeRequestHandler can't get a channelActive event. As a workaround, we can use handlerAdded() event for HTTPS.

Yes, this worked, thanks, but making working test was not an easy challenge for me) It looks weird but this works:

class H2ConnectionHandler extends SimpleH2CServerHandler {
    protected final Http2Connection.PropertyKey streamKey;
    private final Http2Connection.PropertyKey upgradeKey;
    private final InboundHttpToHttp2Adapter adapter;

    H2ConnectionHandler(Http2ConnectionDecoder decoder, Http2ConnectionEncoder encoder,
                        Http2Settings initialSettings) {
        super(decoder, encoder, initialSettings);
        streamKey = connection().newKey();
        upgradeKey = connection().newKey();
        adapter = new InboundHttpToHttp2Adapter(connection(), this);
    }

    @Override
    public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exception {
        super.userEventTriggered(ctx, evt);
        if(evt instanceof UpgradeEvent) {
            UpgradeEvent upgrade = (UpgradeEvent) evt;
            try {
                ctx.fireUserEventTriggered(upgrade.retain());
                Http2Stream stream = connection().stream(HTTP_UPGRADE_STREAM_ID);
                upgrade.upgradeRequest().headers().setInt(HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), HTTP_UPGRADE_STREAM_ID);
                stream.setProperty(upgradeKey, true);
                adapter.channelRead(ctx, upgrade.upgradeRequest().retain());
            } finally {
                upgrade.release();
            }
        }
    }
}

Is there any simpler way to achieve this or this is good enough for this test case?

ikhoon · 2023-03-02T03:06:57Z

As Trustin said, a pipeline customer would be a simple solution to set up a test server. #4710
It would not take a long time to fix #4710. How about continuing your work when #4710 is handled? We are working on it ASAP.

0lejk4 · 2023-03-02T08:40:26Z

Understood! Good, then I`ll wait for it, thanks for the clarification.

trustin · 2023-05-30T08:39:45Z

@0lejk4 Sorry it took much longer than we expected 🙇 The PR that allows you to manipulate the Netty pipeline has now been merged: #4813

ikhoon · 2023-06-02T08:03:05Z

Gentle ping @0lejk4 😉

0lejk4 · 2023-06-08T19:26:36Z

Sorry for inconvenience, I am really sorry I couldn't help you in time. I see that you are finishing this on your own. Still glad this feature is coming to life, thank you for your work guys!

trustin

Left some naming convention comments, but it looks great. Thanks, @0lejk4 and @ikhoon! 🙇

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

core/src/main/java/com/linecorp/armeria/client/ClientFactoryOptions.java

core/src/main/java/com/linecorp/armeria/client/HttpClientFactory.java

core/src/main/java/com/linecorp/armeria/common/Flags.java

trustin

👍

jrhee17

Looks really nice 👍 Thanks @0lejk4 and @ikhoon ! 🙇 👍 🚀

ikhoon

Thanks, @0lejk4! 👍

minwoox

Thanks a lot! 👍

minwoox · 2023-06-13T09:02:10Z

Thanks, @0lejk4 for adding this feature. 🙇

line#4702: Add possibility to use http2 preface with https connection

76682ee

0lejk4 requested review from ikhoon, jrhee17, minwoox and trustin as code owners February 26, 2023 20:18

trustin added the new feature label Feb 27, 2023

line#4702: Add useHttp2WithoutALPN option to deal with an HTTP/2 serv…

63b42cc

…er that doesn't support ALPN.

correct pipeline handling for upgrade

f2da76b

trustin mentioned this pull request Mar 16, 2023

Provide a way to customize the netty pipeline for server-side #4734

Closed

minwoox added this to the 1.24.0 milestone Mar 23, 2023

ikhoon added 5 commits June 7, 2023 20:35

Merge branch 'main' into pr-4704-0lejk4-http2-https-preface

d92fe09

Test H2 without ALPN

83ffbbd

Add more tests

454bb02

Clear cache

bd0deb1

Merge branch 'main' into pr-4704-0lejk4-http2-https-preface

a90039c

trustin added 2 commits June 9, 2023 11:25

Update H2WithoutAlpnTest.java

5deb0a1

Update HttpClientPipelineConfigurator.java

318d7f5

trustin reviewed Jun 9, 2023

View reviewed changes

ikhoon added 2 commits June 9, 2023 12:14

Rename ALPN to Alpn

86fac62

private

870de88

trustin approved these changes Jun 9, 2023

View reviewed changes

rename needsRetryWith variable

49ede48

jrhee17 approved these changes Jun 12, 2023

View reviewed changes

ikhoon approved these changes Jun 12, 2023

View reviewed changes

minwoox approved these changes Jun 13, 2023

View reviewed changes

minwoox merged commit 2a33f07 into line:main Jun 13, 2023
12 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to use http2 preface with https connection #4704

Add possibility to use http2 preface with https connection #4704

0lejk4 commented Feb 26, 2023

CLAassistant commented Feb 26, 2023 •

edited

trustin commented Feb 27, 2023 •

edited

0lejk4 commented Feb 27, 2023

jrhee17 commented Feb 27, 2023

0lejk4 commented Feb 27, 2023

ikhoon commented Feb 27, 2023

0lejk4 commented Feb 27, 2023

ikhoon commented Feb 28, 2023

trustin commented Feb 28, 2023

0lejk4 commented Mar 1, 2023

ikhoon commented Mar 2, 2023

0lejk4 commented Mar 2, 2023

trustin commented May 30, 2023

ikhoon commented Jun 2, 2023

0lejk4 commented Jun 8, 2023

trustin left a comment

trustin left a comment

jrhee17 left a comment

ikhoon left a comment

minwoox left a comment

minwoox commented Jun 13, 2023

Add possibility to use http2 preface with https connection #4704

Add possibility to use http2 preface with https connection #4704

Conversation

0lejk4 commented Feb 26, 2023

CLAassistant commented Feb 26, 2023 • edited

trustin commented Feb 27, 2023 • edited

0lejk4 commented Feb 27, 2023

jrhee17 commented Feb 27, 2023

0lejk4 commented Feb 27, 2023

ikhoon commented Feb 27, 2023

0lejk4 commented Feb 27, 2023

ikhoon commented Feb 28, 2023

trustin commented Feb 28, 2023

0lejk4 commented Mar 1, 2023

ikhoon commented Mar 2, 2023

0lejk4 commented Mar 2, 2023

trustin commented May 30, 2023

ikhoon commented Jun 2, 2023

0lejk4 commented Jun 8, 2023

trustin left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented Jun 13, 2023

CLAassistant commented Feb 26, 2023 •

edited

trustin commented Feb 27, 2023 •

edited