etcd icon indicating copy to clipboard operation
etcd copied to clipboard

[3.4] etcdserver: fix broken APIv2 with long URI

Open Hexta opened this issue 2 years ago • 10 comments

APIv2 doesn't support operations with URI length > 4k. This could be relatively easy achieved by CAS operations with value size about 2k.

This is not documented issue caused by using cmux to serve HTTP1 and HTTP2 protocols on the same port (2379 by default). cmux has hardcoded max request size for HTTP1 matcher and returns false if size is greater. HTTP1Fast matcher from cmux validates requests only against known HTTP methods (GET, POST, PUT, DELETE, OPTIONS,..) and doesn't have such limit.

Fixes #14023

Hexta avatar May 09 '22 08:05 Hexta

FYI V2 API was deprecated for couple of releases and is already removed on main branch, the only remaining things are for serving /metrics and /version endpoints.

If you would like to continue with a PR, for example to backport it to old releases, please add tests.

serathius avatar May 09 '22 08:05 serathius

@serathius I've changed destination branch of the PR to release-3.4 (for version which we actually use on the production) and added a test. Pls advise me if I did it in the wrong way.

Hexta avatar May 09 '22 20:05 Hexta

@Hexta Do you see any real issue in your test or just by reviewing the source code?

Note that the cmux.HTTP1() is just reading the first line in the HTTP header. In the following example, it's just reading the first line GET /examples/my-awesome-test/ HTTP/1.1. Also see matchers.go#L94

GET /examples/my-awesome-test/ HTTP/1.1
Host: code.tutsplus.com
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en-us,en;q=0.5
Accept-Encoding: gzip,deflate
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7
Keep-Alive: 300
Connection: keep-alive
Cookie: PHPSESSID=r2t5uvjq435r4q7ib3vtdjq120
Pragma: no-cache
Cache-Control: no-cache

ahrtr avatar May 09 '22 21:05 ahrtr

@ahrtr Try to do the same test with URI longer than 4096. Yes, I've the real issue on the production.

Hexta avatar May 10 '22 04:05 Hexta

@ahrtr Try to do the same test with URI longer than 4096. Yes, I've the real issue on the production.

Got it. So you run into this issue when there is a long long prevValue?

Just as @serathius pointed out, v2 API is deprecated, and the v2 server & etcdctl v2 commands have already been removed in main branch (3.6). Can you migrate to the v3 API? You have to perform the migration someday, it's just a matter of time.

ahrtr avatar May 10 '22 06:05 ahrtr

+1 to migration to v3.

  1. v3.4 is rather EOL. We cherry pick there security patches, but not semantical improvements. I'm a little bit concerned that Http1Fast might bring unexpected semantical differences. To be on the safe side, it should be flag gated... but than it becomes substantial change in v3.4.

  2. If we decided to merge - we would need to update the changelog file as well for v3.4.

ptabor avatar May 10 '22 12:05 ptabor

That's really ironic. We started migration to v3 and replaced etcd proxy with etcd gateway to provide APIv2 and v3 to our services simultaneously. And we faced with broken CAS operations with values >4K which is much lower than documented etcd limits.

Hexta avatar May 11 '22 16:05 Hexta

I'm sorry, I don't understand. Do you mean that you application was working correctly over V2 API and not hitting the limites, and the problem started when the traffic becomes forwarded through the gateway ?

ptabor avatar May 13 '22 10:05 ptabor

@ptabor Exactly.

Hexta avatar May 16 '22 11:05 Hexta

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Sep 20 '22 21:09 stale[bot]