Skip to content

Implement Http2Headers.isEmpty#10663

Merged
temawi merged 1 commit intogrpc:masterfrom
pkoenig10:isEmpty
Nov 14, 2023
Merged

Implement Http2Headers.isEmpty#10663
temawi merged 1 commit intogrpc:masterfrom
pkoenig10:isEmpty

Conversation

@pkoenig10
Copy link
Contributor

@pkoenig10 pkoenig10 commented Nov 13, 2023 •
edited
Loading

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.

@pkoenig10 pkoenig10 mentioned this pull request Nov 13, 2023
Merged
@idelpivnitskiy idelpivnitskiy mentioned this pull request Nov 13, 2023
Closed
@idelpivnitskiy
Copy link

idelpivnitskiy commented Nov 13, 2023

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

@pkoenig10
Copy link
Contributor Author

pkoenig10 commented Nov 13, 2023

Done

@temawi temawi self-requested a review November 13, 2023 18:11
@temawi temawi added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Nov 13, 2023
@temawi
Copy link
Contributor

temawi commented Nov 13, 2023

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

@sdelamo
Copy link

sdelamo commented Nov 14, 2023

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

@temawi temawi merged commit 90e76a1 into grpc:master Nov 14, 2023
@temawi
Copy link
Contributor

temawi commented Nov 14, 2023

@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
ejona86 reviewed Nov 14, 2023
View reviewed changes
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@idelpivnitskiy
Copy link

idelpivnitskiy commented Nov 15, 2023

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

@laurit laurit mentioned this pull request Nov 16, 2023
Merged
1 task
@pkoenig10 pkoenig10 deleted the isEmpty branch November 16, 2023 15:42
@abelsromero
Copy link

abelsromero commented Nov 17, 2023

@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
Copy link
Member

ejona86 commented Nov 17, 2023

@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
Copy link

abelsromero commented Nov 18, 2023

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.

@sdelamo sdelamo mentioned this pull request Nov 18, 2023
Closed
@krickert
Copy link

krickert commented Nov 19, 2023

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

@Hc747
Copy link

Hc747 commented Nov 20, 2023

Awaiting this fix too

ejona86 pushed a commit to ejona86/grpc-java that referenced this pull request Nov 20, 2023
@pkoenig10 @ejona86
Implement Http2Headers.isEmpty (grpc#10663)
347fcc3
@ejona86 ejona86 mentioned this pull request Nov 20, 2023
Merged
@ejona86 ejona86 added the TODO:backport PR needs to be backported. Removed after backport complete label Nov 20, 2023
ejona86 pushed a commit that referenced this pull request Nov 20, 2023
@pkoenig10 @ejona86
Implement Http2Headers.isEmpty (#10663)
85d3611
@ejona86 ejona86 removed the TODO:backport PR needs to be backported. Removed after backport complete label Nov 20, 2023
This was referenced Nov 27, 2023
Closed
Closed
This was referenced Nov 27, 2023
Closed
Closed
@pkoenig10 pkoenig10 mentioned this pull request Nov 27, 2023
Closed
@SkyeBeFreeman SkyeBeFreeman mentioned this pull request Nov 28, 2023
Closed
@ejona86
Copy link
Member

ejona86 commented Nov 28, 2023

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.

@houssain-barouni houssain-barouni mentioned this pull request Dec 5, 2023
Merged
1 task
@primabrucexu primabrucexu mentioned this pull request Dec 19, 2023
Closed
@Koziad Koziad mentioned this pull request Dec 21, 2023
Merged
@bswami50 bswami50 mentioned this pull request Jan 18, 2024
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ejona86 ejona86 ejona86 left review comments

@temawi temawi temawi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

grpc-java is not compatible with the latest Netty version 4.1.101.Final

9 participants

@pkoenig10 @idelpivnitskiy @temawi @sdelamo @abelsromero @ejona86 @krickert @Hc747 @grpc-kokoro