Implement Http2Headers.isEmpty

[pkoenig10]

Fixes #10665

Using grpc-netty with Netty 4.1.101.Final results in the following error:

io.grpc.StatusRuntimeException: UNKNOWN
	at app//io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:275)
	at app//io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:256)
	at app//io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:169)
	// Remaining stacktrace omitted
Caused by: java.lang.UnsupportedOperationException
	at io.grpc.netty.AbstractHttp2Headers.isEmpty(AbstractHttp2Headers.java:40)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onHeadersRead(DefaultHttp2ConnectionDecoder.java:419)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onHeadersRead(DefaultHttp2ConnectionDecoder.java:352)
	at io.netty.handler.codec.http2.Http2InboundFrameLogger$1.onHeadersRead(Http2InboundFrameLogger.java:56)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader$2.processFragment(DefaultHttp2FrameReader.java:476)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readHeadersFrame(DefaultHttp2FrameReader.java:484)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:253)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:159)
	at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:188)
	at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:393)
	at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:453)
	// Remaining stacktrace omitted

In netty/netty@2657079, Netty introduced code that calls Http2Headers.isEmpty. However the gRPC Http2Headers implementations do not implement this method. I assume this is simply an oversight.

AbstractHttp2Headers feels a fairly brittle. There are a number of methods here that are not implemented by the concrete implementations. Future Netty versions could begin calling these methods and cause similar failures. I wonder if it might be better to eliminate AbstractHttp2Headers to ensure that any unsupported methods are unsupported intentionally rather that accidentally. It seems like this isn't the first time there have been issues like this, see #7953.

[idelpivnitskiy]

Thanks for the PR @pkoenig10!
Please update your PR description with "Resolves: #10665" comment to relate your PR to an open issue.

[pkoenig10]

Done

[temawi]

Thanks for jumping on this one @pkoenig10, I'll get it merged.

[sdelamo]

@temawi would it be possible to merge this and release a new version? We need this fix for Micronaut gRPC. 🙏

[temawi]

@sdelamo Merging is easy (and done) but I hesitate with the patch release as v1.60.0 is scheduled to come out in two weeks (11/28). Hoping you can wait for that.

[ejona86]

LGTM

[idelpivnitskiy]

Any plans to backport this in prior versions? If yes, up to what version going backward and what is the timeline for those releases?

[abelsromero]

@sdelamo Merging is easy (and done) but I hesitate with the patch release as v1.60.0 is scheduled to come out in two weeks (11/28). Hoping you can wait for that.

About this, I am sorry to ask. But some products are being forced to bump netty due to CVE-2023-34062. That means that if you are integrating grpc-java, it is either; grpc does not work or you are left with a serious CVE.
In light of that, would it be possible for grpc-java to release sooner than planned?

[ejona86]

@abelsromero, that's unfortunate. It looks like io.projectreactor.netty is regularly performing non-essential upgrades in a patch releases, which is especially bad for a security release. Oh... they are following the Netty release branches, propagating the Netty choice of not having patch releases. And yes, that regularly causes problems for security fixes.

Does 1.1.13 actually fix the security issue? I see nothing in the release notes related to the described CVE nor any commit since 1.1.12 that seems relevant. Looking through 1.0.x has less noise, because they forward-port instead of back-port changes, but nothing jumps out even when looking at the diffs. I'd suggest asking them what fixed the issue, as it looks like they may have made a mistake. (Dealing with security stuff requires going through different processes, and it is easy to make mistakes as you do it less frequently.)

There is grpc-netty-shaded, which most users seem to be using and so wouldn't be impacted. But yeah, we'll probably need to do a patch release.

[abelsromero]

Does 1.1.13 actually fix the security issue? I see nothing in the release notes related to the described CVE nor any commit since 1.1.12 that seems relevant. Looking through 1.0.x has less noise, because they forward-port instead of back-port changes, but nothing jumps out even when looking at the diffs. I'd suggest asking them what fixed the issue, as it looks like they may have made a mistake. (Dealing with security stuff requires going through different processes, and it is easy to make mistakes as you do it less frequently.)

Yes, that was already confirmed by the team. I went through the same steps as you described already. Some CVEs are fixed without much mention to prevent abuse, at least, that's what I take from the closed-door conversations. If you know what to look for, you can find the commit.

At the end, I was requesting this in nice-2-have approach. The release is 10 days away, if it creates a big hassle for you, we'll deal with any inconveniences during the period 🤷. Mostly internal meetings and maybe some customers running a scanner asking when are we going to release our product.

[krickert]

This pull request would fix grpc in micronaut. Just tested it.

[Hc747]

Awaiting this fix too

[ejona86]

I just released 1.59.1 (it is available on Maven Central; it doesn't matter if it is indexed by search.maven.org; just try using it). It has this backport.