You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by "huw0 (via GitHub)" <gi...@apache.org> on 2023/09/26 15:52:10 UTC
[GitHub] [cxf] huw0 opened a new pull request, #1454: Ensure correct ConnectionFactory ordering in Jetty transport so that …
huw0 opened a new pull request, #1454:
URL: https://github.com/apache/cxf/pull/1454
_I am waiting for my JIRA account to be approved. For now I've put the details below and I will update this later with a JIRA reference. Alternatively, I'm happy for you to do this if you'd prefer._
Hi,
Summary - I have been trying to update one of our apps using the Jetty backend to use http2. While doing this I've noticed that the protocol negotiation is in the wrong order which results in http/1.1 always being preferred where the client includes this in the ALPN handshake. This means that both browsers and cURL will never successfully negotiate h2/http2.
---
# The problem
The issue can be shown using curl.
`curl -ikv https://localhost:9000`
I've attached the full output of this command when running curl against the CXF sample `jax_rs_basic_http2_jetty`.
The important lines are:
```
* ALPN: offers h2,http/1.1
* ALPN: server accepted http/1.1
```
Compare this with the attached curl output from the CXF sample `jax_rs_basic_http2_netty`.
The important lines here are:
```
* ALPN: offers h2,http/1.1
* ALPN: server accepted h2
```
---
# The solution
The ALPN protocol selection takes place within ALPNServerConnection, which is part of the jetty-alpn-server module.
```
// RFC 7301 states that the server picks the protocol
// that it prefers that is also supported by the client.
for (String serverProtocol : serverProtocols)
{
if (clientProtocols.contains(serverProtocol))
{
ConnectionFactory factory = getConnector().getConnectionFactory(serverProtocol);
if (factory instanceof CipherDiscriminator && !((CipherDiscriminator)factory).isAcceptable(serverProtocol, tlsProtocol, tlsCipher))
{
if (LOG.isDebugEnabled())
LOG.debug("Protocol {} not acceptable to {} for {}/{} on {}", serverProtocol, factory, tlsProtocol, tlsCipher, getEndPoint());
continue;
}
negotiate
[output-jetty-original.txt](https://github.com/apache/cxf/files/12729574/output-jetty-original.txt)
[output-netty.txt](https://github.com/apache/cxf/files/12729575/output-netty.txt)
d = serverProtocol;
break;
}
}
```
As the code states, the server is responsible for picking the protocol and Jetty picks the first match in the server's list. From the log output of the same app the first in the list is http/1.1
`INFO: Started ServerConnector@11eadcba{ssl, (http/1.1, ssl, alpn, h2)}{0.0.0.0:9000}`
Therefore the solution is to ensure that when h2 is enabled inside `JettyHTTPServerEngine` -> `createConnectorJetty`, the HTTP2ServerConnectionFactory should always be added at the start of the list.
With this change, curl then successfully negotiates with h2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CXF-8936 Fix h2 protocol negotiation in Jetty Transport [cxf]
Posted by "huw0 (via GitHub)" <gi...@apache.org>.
huw0 commented on code in PR #1454:
URL: https://github.com/apache/cxf/pull/1454#discussion_r1344734046
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
I've made these changes. When running the CXF Jetty demo, the ordering now matches that of the Jetty example.
`INFO: Started ServerConnector@11eadcba{ssl, (ssl, alpn, h2, http/1.1)}{0.0.0.0:9000}`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] huw0 commented on a diff in pull request #1454: CXF-8936 Fix h2 protocol negotiation in Jetty Transport
Posted by "huw0 (via GitHub)" <gi...@apache.org>.
huw0 commented on code in PR #1454:
URL: https://github.com/apache/cxf/pull/1454#discussion_r1342159851
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
As far as I can tell, the ordering of the ALPN connection factory doesn't matter. From what I can see within Jetty's `AbstractConnector` the ordering of the connection factories doesn't matter at the point where ALPN is started.
```
if (ssl != null)
{
String next = ssl.getNextProtocol();
ConnectionFactory cf = getConnectionFactory(next);
if (cf == null)
throw new IllegalStateException("No protocol factory for SSL next protocol: '" + next + "' in " + this);
}
```
_In this case, `ssl.getNextProtocol()` will return ALPN_
However I am not that familiar with Jetty's internals and whether there is anywhere else where this would matter.
The key thing is just that http2 precedes http/1.1 in the connection factories list. There are four connection factories in total. The first of these is on line 691 and is outside of the `isHttp2Enabled` block.
```
if (tlsServerParameters != null) {
connectionFactories.add(httpFactory);
httpConfig.addCustomizer(new SecureRequestCustomizer(tlsServerParameters.isSniHostCheck()));
if (isHttp2Enabled(bus)) {
....
```
I would be happy to refactor so that the first connector (which is http/1.1) is added after the `isHttp2Enabled` block, which would remove the need to explicitly add http2 as the first item in the list.
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
Thanks for looking over this change.
As far as I can tell, the ordering of the ALPN connection factory doesn't matter. From what I can see within Jetty's `AbstractConnector` the ordering of the connection factories doesn't matter at the point where ALPN is started.
```
if (ssl != null)
{
String next = ssl.getNextProtocol();
ConnectionFactory cf = getConnectionFactory(next);
if (cf == null)
throw new IllegalStateException("No protocol factory for SSL next protocol: '" + next + "' in " + this);
}
```
_In this case, `ssl.getNextProtocol()` will return ALPN_
However I am not that familiar with Jetty's internals and whether there is anywhere else where this would matter.
The key thing is just that http2 precedes http/1.1 in the connection factories list. There are four connection factories in total. The first of these is on line 691 and is outside of the `isHttp2Enabled` block.
```
if (tlsServerParameters != null) {
connectionFactories.add(httpFactory);
httpConfig.addCustomizer(new SecureRequestCustomizer(tlsServerParameters.isSniHostCheck()));
if (isHttp2Enabled(bus)) {
....
```
I would be happy to refactor so that the first connector (which is http/1.1) is added after the `isHttp2Enabled` block, which would remove the need to explicitly add http2 as the first item in the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #1454: CXF-8936 Fix h2 protocol negotiation in Jetty Transport
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1454:
URL: https://github.com/apache/cxf/pull/1454#discussion_r1342160643
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
> I would be happy to refactor so that the first connector (which is http/1.1) is added after the isHttp2Enabled block, which would remove the need to explicitly add http2 as the first item in the list.
I think that would make sense, thank you. It also is aligned with Jetty examples [1]
[1] https://github.com/eclipse/jetty.project/blob/jetty-9.4.14.v20181114/examples/embedded/src/main/java/org/eclipse/jetty/embedded/Http2Server.java
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
> I would be happy to refactor so that the first connector (which is http/1.1) is added after the isHttp2Enabled block, which would remove the need to explicitly add http2 as the first item in the list.
I think that would make sense, thank you. It also will be aligned with Jetty examples [1]
[1] https://github.com/eclipse/jetty.project/blob/jetty-9.4.14.v20181114/examples/embedded/src/main/java/org/eclipse/jetty/embedded/Http2Server.java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CXF-8936 Fix h2 protocol negotiation in Jetty Transport [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta merged PR #1454:
URL: https://github.com/apache/cxf/pull/1454
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a diff in pull request #1454: CXF-8936 Fix h2 protocol negotiation in Jetty Transport
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1454:
URL: https://github.com/apache/cxf/pull/1454#discussion_r1342153331
##########
rt/transports/http-jetty/src/main/java/org/apache/cxf/transport/http_jetty/JettyHTTPServerEngine.java:
##########
@@ -697,7 +697,8 @@ AbstractConnector createConnectorJetty(SslContextFactory.Server sslcf, String ho
final SslConnectionFactory scf = new SslConnectionFactory(sslcf, alpn.getProtocol());
connectionFactories.add(scf);
Review Comment:
Thanks for the fix, @huw0 , I believe the ALPN should go before HTTP/2, wdyt?
```
connectionFactories.add(alpn);
connectionFactories.add(new HTTP2ServerConnectionFactory(httpConfig));
connectionFactories.add(scf);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org