proxy protocol: new feature auto-detect proxy protocol#18951
proxy protocol: new feature auto-detect proxy protocol#18951ggreenway merged 61 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
|
Hi @kdorosh, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
…-fork into auto_detect_proxy_protocol Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
| if (nread < 1) { | ||
| ENVOY_LOG(debug, "failed to read proxy protocol (no bytes read)"); | ||
| return ReadOrParseState::Error; | ||
| } else if (nread < PROXY_PROTO_V2_HEADER_LEN && config_.get()->detectProxyProtocol()) { |
There was a problem hiding this comment.
if the client send a partial v1 protocol header, I feel this will skip the process the v1 protocol header. it would be great to add an unitest for this case.
There was a problem hiding this comment.
thanks for the feedback, addressed in ed4fb54
| // NOTICE: only enable if ALL traffic to the listener comes from a trusted source. | ||
| // Defaults to false. If true, attempt to detect proxy protocol if present, and allow | ||
| // requests through if proxy protocol is not used on the connection. | ||
| bool detect_proxy_protocol = 2; |
There was a problem hiding this comment.
I feel the major goal is allow the requests through if proxy protocol is not used. And actually the whole proxy protocol filter is about detect the proxy protocol, so this option name feels confuse. Should we call it something like allow_requests_with_proxy_protocol?
There was a problem hiding this comment.
+1 please clarify the name.
There was a problem hiding this comment.
I like that better as well, addressed in bcc7aef
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
adisuissa
left a comment
There was a problem hiding this comment.
Thanks for working on this.
Left a few comments.
| // The list of rules to apply to requests. | ||
| repeated Rule rules = 1; | ||
|
|
||
| // NOTICE: only enable if ALL traffic to the listener comes from a trusted source. |
There was a problem hiding this comment.
Should probably in an .. attention:: clause.
See example.
I also suggest to rephrase the comment, starting with with it does, its default value, and then the notice part.
There was a problem hiding this comment.
Thanks for the example, addressed in 91c9a1b
| // NOTICE: only enable if ALL traffic to the listener comes from a trusted source. | ||
| // Defaults to false. If true, attempt to detect proxy protocol if present, and allow | ||
| // requests through if proxy protocol is not used on the connection. | ||
| bool detect_proxy_protocol = 2; |
There was a problem hiding this comment.
+1 please clarify the name.
| Stats::Scope& scope, | ||
| const envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol& proto_config) | ||
| : stats_{ALL_PROXY_PROTOCOL_STATS(POOL_COUNTER(scope))} { | ||
| detect_proxy_protocol_ = proto_config.detect_proxy_protocol(); |
There was a problem hiding this comment.
Should be set in the c'tor's member initialization list above.
| * Filter configuration that determines if we should pass-through failed proxy protocol | ||
| * requests. Should only be configured to true for trusted downstreams. | ||
| */ | ||
| bool detectProxyProtocol() const; |
There was a problem hiding this comment.
The name here should also be clear (following the api field name change).
| disconnect(); | ||
| } | ||
|
|
||
| TEST_P(ProxyProtocolTest, DetectNoProxyProtocol) { |
There was a problem hiding this comment.
Please add a comment on what the test is validating.
There was a problem hiding this comment.
Addressed in 4259f1a, please let me know if this doesn't clarify what might have been confusing
| // Release the file event so that we do not interfere with the connection read events. | ||
| io_handle.resetFileEvents(); | ||
| cb_->continueFilterChain(true); | ||
| return ReadOrParseState::Done; |
There was a problem hiding this comment.
This method doesn't need to return anything, and the returned status should probably be only at the end of Filter::onReadWorker()
|
Just realized there was a race where some of the comments were addressed in a recent commit. |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
|
@adisuissa all comments should be addressed |
| proto_config.set_allow_requests_without_proxy_protocol(true); | ||
| connect(true, &proto_config); | ||
|
|
||
| write("PROXY TCP4"); |
There was a problem hiding this comment.
We should test the case of the first write is not full v1 protocol signature, like just one char 'P'
There was a problem hiding this comment.
I'm actually not sure this is possible simultaneously with the fragmented/partial protocol without the possibility for false positive, e.g.
TEST_P(ProxyProtocolTest, TinyPartialV1ReadWithAllowNoProxyProtocolThisFails) {
envoy::extensions::filters::listener::proxy_protocol::v3::ProxyProtocol proto_config;
proto_config.set_allow_requests_without_proxy_protocol(true);
connect(true, &proto_config);
write("P"); // matches the beginning of v1 proxy protocol
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
write("BOGUS");
expectData("PBOGUS"); // we will consume `P` and upstream gets `BOGUS` not `PBOGUS`
disconnect();
}this is why in the initial implementation I required that the initial read from recv have enough bytes available on the initial MSG_PEEK to make the determination on the v1/v2 proxy header.
We can leave the PR as it is (or perhaps preferably throw a hard error if we consumed some bytes and then realize we had a false positive?) with the understanding that there could be false positives on identifying proxy protocol, additionally with the understanding that this is quite rare and will not likely happen in practice (except perhaps for single byte reads of P and HTTP POST/PUT request?). In exchange, we get support for fragmented/partial reads. Thoughts?
Also willing to go back to my original version (before 6a0d49b which addresses the initial concern with partial reads) which addresses the potential false positive here
There was a problem hiding this comment.
I'm not following. How would the filter consume the P only?
There was a problem hiding this comment.
Running the test above, we do recv with MSG_PEEK and just get P here.
Then before we continue the loop, we read that byte for real (no MSG_PEEK) here. Thus when we do our second recv and we get BOGUS we have already consumed P. We correctly identify that the request is not proxy protocol, but the request forwarded upstream has a missing byte(s) (in this case, just P).
The only way I see to avoid false positives identifying the header and always send the correct response up the filter chain is to require that the initial MSG_PEEK has enough bytes to detect v1/v2 header (at most 16, which seems reasonable for this opt-in only codepath)
There was a problem hiding this comment.
Oh, I see.
I think you need to fix this case. You may have to restructure the code such that no bytes are read (without MSG_PEEK) until we've decided 100% whether this is a proxy header or not.
I think it's probably good enough to look for either PROXY_PROTO_V1_SIGNATURE or PROXY_PROTO_V2_SIGNATURE. If the connections starts with either of those, but then doesn't have a valid full proxy protocol header, abort the connection as we currently do. If the connection starts with bytes that match neither of those signatures, and the new option is enabled, don't read() any bytes and continue the filter chain. Will that work?
| return ReadOrParseState::Error; | ||
| } else if (nread < PROXY_PROTO_V2_HEADER_LEN && | ||
| config_.get()->allowRequestsWithoutProxyProtocol()) { | ||
| if (nread < PROXY_PROTO_V1_SIGNATURE_LEN || |
There was a problem hiding this comment.
Although it is rare case, there still have the chance of receiving the partial v1 signature, like just a 'P' char, the left of the signature received by the second recv call. But this check will just skip the left of signature checking.
we could return SkipFilterError at https://github.com/envoyproxy/envoy/pull/18951/files#diff-a35fc10b1ce97239d02ac1791aac05069eaf253d8efc77cefe93749f901e36abL446
Then it will got all the data, and compared the signature of v1 and v2.
But if we only receive very short data from the client (short than both v1 and v2 signature), then I feel we only can waiting for a timeout of listener filter. But that is bad for the usecase of issue.
Another idea is if the nread is less than v1 and v2 signature, then we compare part of signature, like if the first byte is X', then compare to the first byte of v1 signature, it is P`. Also compare the first byte of v2 signature, then we know it can't be the proxy protocol.
There was a problem hiding this comment.
@soulxu can you update the link from
we could return SkipFilterError at https://github.com/envoyproxy/envoy/pull/18951/files#diff-a35fc10b1ce97239d02ac1791aac05069eaf253d8efc77cefe93749f901e36abL446
Then it will got all the data, and compared the signature of v1 and v2.
to a link to main / a commit and where you're proposing this change? I think with the recent commits to this PR that the link provided is dated, I'm not sure where you're proposing we return the error
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
|
I like the current version better, due to simplicity. In nearly all cases, all the bytes for comparision will arrive in the first packet, and we're only comparing roughly two machine words of data (on 64-bit); I think the performance difference will be unmeasurable. I think the simplicity out-weights the small amount of duplication (of cpu cycles and code) in this case. |
Agree with this version is more simple. @kdorosh thanks for your patience and trying out two versions, currently code is LGTM now |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
ggreenway
left a comment
There was a problem hiding this comment.
Ugh; I wrote all this yesterday and forgot to hit "submit review". Sorry for the delay.
/wait
| // For more information on the security implications of this feature, see | ||
| // https://www.haproxy.org/download/2.1/doc/proxy-protocol.txt | ||
| // | ||
| // While incredibly rare, requests of 12 or fewer bytes that match the |
There was a problem hiding this comment.
I think you can remove "While incredibly rare" from the docs.
Also, I think this paragraph should either have attention:: or note:: header above it.
| * dns_resolver: added :ref:`include_unroutable_families<envoy_v3_api_field_extensions.network.dns_resolver.apple.v3.AppleDnsResolverConfig.include_unroutable_families>` to the Apple DNS resolver. | ||
| * ext_proc: added support for per-route :ref:`grpc_service <envoy_v3_api_field_extensions.filters.http.ext_proc.v3.ExtProcOverrides.grpc_service>`. | ||
| * on_demand: :ref:`OnDemand <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.OnDemand>` got extended to hold configuration for on-demand cluster discovery. A similar message for :ref:`per-route configuration <envoy_v3_api_msg_extensions.filters.http.on_demand.v3.PerRouteConfig>` is also added. | ||
| * proxy protocol: added support for allowing requests without proxy protocol on the listener from trusted downstreams as an opt-in flag. |
There was a problem hiding this comment.
Please add a link to the new config setting.
| return Network::FilterStatus::StopIteration; | ||
| } else if (read_state == ReadOrParseState::TryAgainLater) { | ||
| return Network::FilterStatus::StopIteration; | ||
| } else if (read_state == ReadOrParseState::SkipFilter) { |
There was a problem hiding this comment.
Can you make this into a switch/case on read_state? Then the compiler can warn if not all cases are handled.
| if (!matchv2 && !matchv1) { | ||
| // The bytes we have seen so far do not match v1 or v2 proxy protocol, so we can safely | ||
| // short-circuit | ||
| ENVOY_LOG(debug, "request does not use v1 or v2 proxy protocol, forwarding as is"); |
There was a problem hiding this comment.
This should be at trace level because it is expected to happen in this configuration
|
|
||
| private: | ||
| absl::flat_hash_map<uint8_t, KeyValuePair> tlv_types_; | ||
| const bool allow_requests_without_proxy_protocol_{}; |
There was a problem hiding this comment.
This doesn't need the {} initializer at the end because it is always initialized in the constructor.
| std::string msg = "data"; | ||
| EXPECT_GT(PROXY_PROTO_V1_SIGNATURE_LEN, | ||
| msg.length()); // Ensure we attempt parsing byte by byte using `search_index_` | ||
| EXPECT_GT(PROXY_PROTO_V2_SIGNATURE_LEN, msg.length()); |
| EXPECT_GT(PROXY_PROTO_V2_SIGNATURE_LEN, msg.length()); | ||
|
|
||
| write(msg); | ||
|
|
| write(msg); | ||
|
|
||
| expectData(msg); | ||
|
|
| connect(true, &proto_config); | ||
|
|
||
| std::string msg = "more data more data more data"; | ||
| EXPECT_GT(msg.length(), |
| } | ||
|
|
||
| TEST_P(ProxyProtocolTest, V2ShortV4WithAllowNoProxyProtocol) { | ||
| // An ipv4/tcp connection that has incorrect addr-len encoded |
There was a problem hiding this comment.
An ipv4/tcp PROXY header that....
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
@ggreenway no problem at all, thanks again for the feedback and detailed review! |
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
ggreenway
left a comment
There was a problem hiding this comment.
Just one final nit.
@adisuissa can you review the API?
| } else if (read_state == ReadOrParseState::SkipFilter) { | ||
| case ReadOrParseState::SkipFilter: | ||
| return Network::FilterStatus::Continue; | ||
| default: |
There was a problem hiding this comment.
Please remove the default case. Then the compiler will warn/error if there are missing enum cases. I think it's possible that you won't need the return outside the switch either, if all cases are handled and all of them have a return.
There was a problem hiding this comment.
addressed in 5c50e23
unfortunately the compiler isn't smart enough to tell that every case is implemented and returns; thus we need the ghost "default" case at the end of the function after the switch statement
source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc: In member function 'virtual Envoy::Network::FilterStatus Envoy::Extensions::ListenerFilters::ProxyProtocol::Filter::onData(Envoy::Network::ListenerFilterBuffer&)':
source/extensions/filters/listener/proxy_protocol/proxy_protocol.cc:93:1: error: control reaches end of non-void function [-Werror=return-type]
93 | }
| ^I removed the default case regardless and confirmed that compiler will fail if any case is not implemented
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
|
/retest |
|
Retrying Azure Pipelines: |
|
@adisuissa ci is passing; I think we need a final approval from you here since you requested changes before? Thanks! |
…voyproxy#18951) Allows users to opt-in to functionality to auto-detect proxy protocol if present, and skip the filter if it's not present. Signed-off-by: Kevin Dorosh <kevin.dorosh@solo.io>
Signed-off-by: Kevin Dorosh kevin.dorosh@solo.io
Commit Message: Auto-detect proxy protocol
Additional Description: Allows users to opt-in to functionality to auto-detect proxy protocol if present, and skip the filter if it's not present.
Risk Level: Low, opt-in new feature only hits new codepath if configured
Testing: Added tests for new codepath that cover both passthrough and fragmented proxy protocol scenarios
Docs Changes: N/A, outside of auto-generated API docs
Fixes: #18888