thrift_proxy: introduce header transport #4082
Conversation
Implements the Thrift "header" transport which primarily provides key/value pairs that may contain arbitrary message metadata. *Risk Level*: low *Testing*: added unit/integration tests *Docs Changes*: n/a *Release Notes*: added note about header transport support Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
|
@rgs1 or @fishcakez github won't let me directly request reviews or assign you, but if you wanted to take a look at this, I'd be grateful. |
|
@zuercher thanks for tagging us, I"ll look today. |
| } | ||
| metadata.setProtocol(proto); | ||
|
|
||
| int16_t num_xforms = drainVarIntI16(buffer, header_size, "transform count"); |
There was a problem hiding this comment.
Is this line vulnerable to a buffer read overflow? Here's the scenario I'm worried about:
- A malicious client sends a 15-byte message where
header_size= 1 - This message gets through all the checks on lines 57-66 above.
- Then line 72 removes the first 14 bytes, leaving 1 byte in
buffer. - Depending on the value in that 1 remaining byte,
drainVarInt16may try to read the byte after it, which is beyond the end ofbuffer.
There was a problem hiding this comment.
I don't believe so. We require that the full header (as given by header size) by available. As we read bytes from the header we decrement header_size (which is passed as a reference to drainVarIntI16). drainVarIntI16 is just drainVarIntI32 with an upper bound check. The latter requires remaining header_size to be at least 1 and then invokes BufferHelper::peekVarIntI32 which validates the buffer has enough data before reading each byte that composes the value. If the peek reports a buffer underflow (buffer doesn't have enough data to read the whole number) or requires more bytes than are left in the header (supposing the buffer has more data beyond the end of the header) we throw.
That said, I'll add a test that exercises this directly. Mostly as a remdinder to myself: It's basically this one except that the number of transforms should be truncated.
Signed-off-by: Stephan Zuercher <[email protected]>
| } // namespace | ||
|
|
||
| bool HeaderTransportImpl::decodeFrameStart(Buffer::Instance& buffer, MessageMetadata& metadata) { | ||
| if (buffer.length() < 14) { |
There was a problem hiding this comment.
nit: should this be a constant (e.g.: HEADER_LEN)?
There was a problem hiding this comment.
Added some constants for the various sizes this transport uses.
| envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy(), context), | ||
| namespace { | ||
|
|
||
| std::vector<envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_TransportType> |
There was a problem hiding this comment.
nit: should this be typedef'd (e.g.: TransportTypeVector)?
There was a problem hiding this comment.
It's only used here in this test, and I personally would expect TransportTypeVector to be a vector of the TransportType defined in thrift.h as opposed to the one from the proto definitions.
| return v; | ||
| } | ||
|
|
||
| std::vector<envoy::config::filter::network::thrift_proxy::v2alpha1::ThriftProxy_ProtocolType> |
Signed-off-by: Stephan Zuercher <[email protected]>
Signed-off-by: Stephan Zuercher <[email protected]>
|
Thanks for the updates. LGTM. |
|
@mattklein123 can I get you to take a quick pass at this next time you're reviewing PRs? |
|
@zuercher will do Thursday morning unless someone else gets to it sooner. Sorry for the delay. |
| // Flags: 2 bytes + | ||
| // Sequence number: 4 bytes | ||
| // Header data size: 2 bytes | ||
| constexpr uint64_t MIN_FRAME_START_SIZE_NO_HEADERS = 10; |
There was a problem hiding this comment.
nit: I think we are doing CamelCase now for constants. MinFrameStartSizeNoHeaders
| */ | ||
| class HeaderTransportImpl : public Transport { | ||
| public: | ||
| HeaderTransportImpl() {} |
Signed-off-by: Stephan Zuercher <[email protected]>
|
@mattklein123 addressed your nits, could you restamp this when you have a moment? |
* origin/master: (34 commits) fuzz: fixes oss-fuzz: 9895 (envoyproxy#4189) docs: added developer docs for pprof/tcmalloc testing (envoyproxy#4194) fix sometimes returns response body to HEAD requests (envoyproxy#3985) admin: fix config dump order (envoyproxy#4197) thrift: allow translation between downstream and upstream protocols (envoyproxy#4136) Use local_info.node() instead of bootstrap.node() whenever possible (envoyproxy#4120) upstream: allow extension protocol options to be used with http filters (envoyproxy#4165) [thrift_proxy] Replace local HeaderMap with Http::HeaderMap[Impl] (envoyproxy#4169) docs: improve gRPC-JSON filter doc (envoyproxy#4184) stats: refactoring MetricImpl without strings (envoyproxy#4190) fuzz: coverage profile generation instructions. (envoyproxy#4193) upstream: rebuild cluster when health check config is changed (envoyproxy#4075) build: use clang-6.0. (envoyproxy#4168) thrift_proxy: introduce header transport (envoyproxy#4082) tcp: allow connection pool callers to store protocol state (envoyproxy#4131) configs: match latest API changes (envoyproxy#4185) Fix wrong mock function name. (envoyproxy#4187) Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) Converting envoy configs to V2 (envoyproxy#2957) Add timestamp to HealthCheckEvent definition (envoyproxy#4119) ... Signed-off-by: Snow Pettersen <[email protected]>
…#1938) This is far from finished, but it reduces memory usage by ~10%. Pulling the following changes from github.com/envoyproxy/envoy: c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190) 36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193) ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075) 05c0d52 build: use clang-6.0. (envoyproxy#4168) 01f403e thrift_proxy: introduce header transport (envoyproxy#4082) 564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131) 3e1d643 configs: match latest API changes (envoyproxy#4185) bc6a10c Fix wrong mock function name. (envoyproxy#4187) e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182) 3d1325e Converting envoy configs to V2 (envoyproxy#2957) 8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119) 497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173) 6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171) 132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161) 7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155) 1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166) 2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157) 71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015) 3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179) 706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007) f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156) 27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130) 8c189a5 Make over provisioning factor configurable (envoyproxy#4003) 6c08fb4 Making test less flaky (envoyproxy#4149) 592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118) For istio/istio#7912. Signed-off-by: Piotr Sikora <[email protected]>
Implements the Thrift "header" transport which primarily provides
key/value pairs that may contain arbitrary message metadata.
Risk Level: low
Testing: added unit/integration tests
Docs Changes: n/a
Release Notes: added note about header transport support
Signed-off-by: Stephan Zuercher [email protected]