Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] yab server reflection call is not a valid HTTP request #418

Open
DheerendraRathor opened this issue Dec 17, 2024 · 2 comments
Open

Comments

@DheerendraRathor
Copy link

DheerendraRathor commented Dec 17, 2024

When yab makes grpc server reflection call, the :authority header is empty in the request.

As per HTTP2 spec, :authority header must not be empty if present.

Clients that generate HTTP/2 requests directly MUST use the ":authority" pseudo-header field to convey authority information, unless there is no authority information to convey (in which case it MUST NOT generate ":authority").

Repro step:

  • Make a grpc request without proto binary and yab makes a grpc server reflection call to service
  • Record request via wireshark or tcpdump and ":authority" header is empty.

This was discovered while testing yab against envoy proxy. Envoy has strict HTTP2 checks and it complains about invalid header.

{"application":"envoy","caller":"source/common/http/http2/codec_impl.cc:1250","callerFunction":"onError","message":"invalid http2: Invalid HTTP header field was received: frame type: 1, stream: 1, name: [:authority], value: []","level":"debug","pid":"34","threadId":"95","timestamp":"2024-12-17T09:09:21.490623000","ConnectionId":"10"}
{"application":"envoy","caller":"source/common/http/http2/codec_impl.cc:1256","callerFunction":"onInvalidFrame","message":"invalid frame: Invalid HTTP header field was received on stream 1","level":"debug","pid":"34","threadId":"95","timestamp":"2024-12-17T09:09:21.490744000","ConnectionId":"10"}

Captured request:
Screenshot 2024-12-17 at 3 16 44 PM

Hex dump

00000000  50 52 49 20 2a 20 48 54  54 50 2f 32 2e 30 0d 0a   PRI * HT TP/2.0..
00000010  0d 0a 53 4d 0d 0a 0d 0a                            ..SM.... 
00000018  00 00 00 04 00 00 00 00  00                        ........ .
    00000000  00 00 18 04 00 00 00 00  00 00 01 00 00 10 00 00   ........ ........
    00000010  08 00 00 00 00 00 03 7f  ff ff ff 00 04 10 00 00   ........ ........
    00000020  00 00 00 00 04 01 00 00  00 00 00 00 04 08 00 00   ........ ........
    00000030  00 00 00 0f ff 00 01                               .......
00000021  00 00 00 04 01 00 00 00  00 00 00 9d 01 04 00 00   ........ ........
00000031  00 01 83 86 45 ad 62 6b  2b 22 f6 16 5a 0a 44 98   ....E.bk +"..Z.D.
00000041  f5 2f dc 23 a2 b9 c6 be  e2 d9 dc b6 6d 2c b4 14   ./.#.... ....m,..
00000051  89 31 ea 63 71 6c ee 5b  36 96 5a 0a 44 98 f5 64   .1.cql.[ 6.Z.D..d
00000061  aa 53 ff 81 5f 8b 1d 75  d0 62 0d 26 3d 4c 4d 65   .S.._..u .b.&=LMe
00000071  64 7a 8d 9a ca c8 b4 c7  60 2b b4 d2 e1 5a 42 f7   dz...... `+...ZB.
00000081  40 02 74 65 86 4d 83 35  05 b1 1f 40 89 9a ca c8   @.te.M.5 ...@....
00000091  b2 4d 49 4f 6a 7f 86 65  f7 c2 e8 59 b7 40 87 b2   .MIOj..e ...Y.@..
000000A1  b2 2c 41 d1 41 6c 8a f4  38 d6 92 72 96 c2 d5 25   .,A.Al.. 8..r...%
000000B1  83 40 88 b2 b2 2c 82 d9  dc c4 2f 83 f0 7a 65 40   .@...,.. ../..ze@
000000C1  89 b2 b2 2c 5a 88 79 0d  54 df 84 ae c3 a4 ff 00   ...,Z.y. T.......
000000D1  00 0b 00 00 00 00 00 01  00 00 00 00 06 22 04 57   ........ .....".W
000000E1  6f 6d 66                                           omf
@biosvs
Copy link
Contributor

biosvs commented Dec 18, 2024

Hey @DheerendraRathor ,

Yab uses github.com/jhump/protoreflect, which uses a normal grpc client in turn. Moreover, afaik, grpc skips ":authority" header if client tries to set it.

But let's hope it's fixed in a new versions (as both libs are pretty outdated here). Once we upgraded protoreflect in monorepo (one of the near future tasks, internal reference: RPC-2400), we'll upgrade it and grpc-go in yab, and then double check if the problem is solved.

@DheerendraRathor
Copy link
Author

grpc provides grpc.WithAuthority(string) option to pass authority to grpc client via DialContext or NewClient.
Also it's not auto fixed in the new version. I locally upgraded all and it still has the empty authority header.

It should be unrelated to upgrade in internal monorepo since yab is entirely separate repo.

More people have stumped upon this in past as well. See #307

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants