[WIP] Add new integration and e2e tests for invalid client api version

[jmhbnz]

This pull request adds new integration and e2e tests to verify etcd won't panic when encountering an invalid client api version which could result in a security denial of service issue with malicious client taking down the etcd cluster.

Fixes #13592

[jmhbnz]

Hey @ahrtr - Following on from our quick message yesterday I can confirm that when using clientv3 the scenario is not possible as even with the above test injecting invalid utf8 client version this is subsequently overwritten down the call stack by

etcd/client/v3/ctx.go

Line 49 in 6f899a7

copied.Set(rpctypes.MetadataClientAPIVersionKey, version.APIVersion)

So I think this should probably be two tests, one verifying that the above overwrite continues to happen, and a second test with a more malicious injection of a bad value maybe sidestepping clientv3 to ensure etcd server end handles and does not panic but I will definitely need some guidance on how to approach that.

[ahrtr]

Probably you can intentionally change the version.APIVersion value something like below,

invalidVersion := []byte{0x01, 0x02, 0x03}
version.APIVersion = string(invalidVersion)

[ahrtr]

The other way is to follow the same pattern as https://github.com/ahrtr/etcd-issues/blob/master/issues/13553/app_send_invalid_client_api_version.c, but it seems a little complicated; so I tend not to spend too much effort in this direction.

[jmhbnz]

Hey @ahrtr - A quick update on this, following your suggestion on setting version.APIVersion I was able to finish off two integration tests that I'm pretty happy with. Interestingly in grpc stream.go there is a metadata validation that rejects if the header contains non printable characters so I still think we need an e2e test that goes straight to etcdserver not via clientv3.

I started tinkering with this to see if we could reuse the existing curl e2e test approach, let me know what you think of this 🙏🏻

[ahrtr]

if we could reuse the existing curl e2e test approach

I am afraid using curl might not be able to intentionally inject an invalid client api version. If it's too difficult or needs big effort, probably we can just close the issue #13592. WDYT?

[jmhbnz]

I am afraid using curl might not be able to intentionally inject an invalid client api version. If it's too difficult or needs big effort, probably we can just close the issue #13592. WDYT?

I think you're right, we can set simple http headers with curl but I don't think that is going to be enough. How about using https://github.com/fullstorydev/grpcurl to talk direct to etcdserver in an e2e? It looks relatively straightforward I'm just not sure what parameter to pass for -import-path or -protoset because it appears etcd does not support the reflection API:

 ~  grpcurl -plaintext localhost:2379 list
Failed to list services: server does not support the reflection API

Edit: For clarity, with grpcurl utility we are able to set metadata for grpc request so we can set the client-api-version. Refer fullstorydev/grpcurl#352

[jmhbnz]

Hey @ahrtr - On second thoughts, I'm not sure that adding more complexity to the e2e tests makes sense at the moment so if you are ok with it I think maybe we close this and the issue as not being a priority at the moment.

[ahrtr]

if you are ok with it I think maybe we close this and the issue as not being a priority at the momen

It's OK to close it. Sorry for the confusion.