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

Add feature flag to allow requests without proxy protocol #18888

Closed
kdorosh opened this issue Nov 3, 2021 · 3 comments · Fixed by #18951
Closed

Add feature flag to allow requests without proxy protocol #18888

kdorosh opened this issue Nov 3, 2021 · 3 comments · Fixed by #18951

Comments

@kdorosh
Copy link
Contributor

kdorosh commented Nov 3, 2021

Title: Add Feature Flag to auto-detect Proxy Protocol

Description:
The proposal here is to add a new feature toggle to the Proxy Protocol listener filter which attempts to parse the connection as proxy protocol (e.g. https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/listener/tls_inspector/tls_inspector.cc#L149 as example to peek at connection bytes) and upon failure, instead of erroring, will continue processing the connection without Proxy Protocol.

This feature toggle would be opt-in and marked for use only with trusted downstream traffic, as there are valid security concerns as listed in the proxy protocol spec:

Implementers should be very careful about not trying to automatically detect
whether they have to decode the header or not, but rather they must only rely
on a configuration parameter. Indeed, if the opportunity is left to a normal
client to use the protocol, he will be able to hide his activities or make them
appear as coming from someone else. However, accepting the header only from a
number of known sources should be safe.

The customer-provided use case:

solo-io/gloo#5403 (comment)

Proposed feature flag:

type ProxyProtocol struct {
	state         protoimpl.MessageState
	sizeCache     protoimpl.SizeCache
	unknownFields protoimpl.UnknownFields

	// The list of rules to apply to requests.
	Rules []*ProxyProtocol_Rule `protobuf:"bytes,1,rep,name=rules,proto3" json:"rules,omitempty"`
        // NEW FIELD: 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
        DetectProxyProtocol bool 
}

Rather than duplicate functionality in our own proxy protocol filter, I thought it would be preferable to explore this as an upstream contribution. Thoughts?

Thanks and best

[optional Relevant Links:]
Gloo Edge issue with the original feature request from customer solo-io/gloo#5403 (I work on the Gloo Edge API gateway built on envoy)

@kdorosh kdorosh added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Nov 3, 2021
@rgs1
Copy link
Member

rgs1 commented Nov 3, 2021

See #17286 for prior attempts.

@mattklein123 mattklein123 added area/proxy_proto and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Nov 3, 2021
@kdorosh
Copy link
Contributor Author

kdorosh commented Nov 5, 2021

@rgs1 @mattklein123 thank you for linking the PR for prior work to skip TLS with proxy protocol. It has proven a useful example as I have started the implementation.

My proposal here is slightly different than the original PR.

What I would like to do is add a new top level configuration to the proxy protocol filter that instructs the filter to attempt parsing as v1/v2 proxy protocol, and if both fail, pass along the original request as if it were not using proxy protocol.

The PR linked only instructs to skip on requests using TLS.

I was hoping to get thoughts on the proposal / api before finishing a PR.

Much appreciated, thanks

@mattklein123 mattklein123 added the help wanted Needs help! label Nov 8, 2021
@mattklein123
Copy link
Member

I think the proposal is reasonable.

@kdorosh kdorosh changed the title Add Feature Flag to auto-detect Proxy Protocol Add feature flag to allow requests without proxy protocol Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants