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

Listener filter to detect protocol #7527

Closed
yxue opened this issue Jul 10, 2019 · 6 comments
Closed

Listener filter to detect protocol #7527

yxue opened this issue Jul 10, 2019 · 6 comments
Labels
enhancement Feature requests. Not bugs or questions.

Comments

@yxue
Copy link
Contributor

yxue commented Jul 10, 2019

Like tls inspector, we'd like to add another listener filter which will detect the protocol, e.g. HTTP, by sniffing the packet. The listener filter can add the value to application protocols or maybe other fields to guide the filter chain match.

From istio side, the change will simplify the user configuration. Would the community consider to add the feature? Please let me know and I'd like to implement it.

@PiotrSikora
Copy link
Contributor

Note that we might want to have a lot of reasonably small protocol-specific inspectors (e.g. envoy.filters.listener.http_inspector for HTTP, etc.) instead of a generic one.

e.g. HTTP inspector (which would work only for plaintext connections) should be able to set:

  • detected transport protocol -> raw_buffer,
  • requested application protocol -> either http/1.1 or h2,
  • requested server name -> extracted from Host or :authority headers.

@lizan
Copy link
Member

lizan commented Jul 10, 2019

An HTTP inspector sounds reasonable to me.

  • requested server name -> extracted from Host or :authority headers.

@PiotrSikora Not sure about this item, the Host or :authority is per request property (esp. HTTP/1.1 keep-alive or H2), which shouldn't stick into connection.

@PiotrSikora
Copy link
Contributor

@PiotrSikora Not sure about this item, the Host or :authority is per request property (esp. HTTP/1.1 keep-alive or H2), which shouldn't stick into connection.

That's fine, you can still latch to a filter chain that includes requested server name in it's server_names, otherwise you're going to be limited to having only a single HTTP filter chain per listener, so the selection is going to be persistent for the connection anyway.

Also, this is not very different from the existing case with TLS inspector, where requests are latched to a filter chain based on the SNI from the first request, even though subsequent requests can be for a different Host or :authority.

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Jul 11, 2019
@yxue
Copy link
Contributor Author

yxue commented Jul 12, 2019

I don't think we should extract Host or :authority from the requests. The Host and :authority are L7 information, I don't think we should use to it do filter chain match in L4. For example:

filter_chains:
    - filter_chain_match:
         server_names: a.com
    - filter_chain_match:
         server_names: b.com

The first request (host: a.com) matches the first filter chain. The second request (host: b.com) from the same connection will use the first filter chain which doesn't make much sense to me.

@PiotrSikora
Copy link
Contributor

PiotrSikora commented Jul 12, 2019

I don't think we should extract Host or :authority from the requests. The Host and :authority are L7 information, I don't think we should use to it do filter chain match in L4. For example:

filter_chains:
    - filter_chain_match:
         server_names: a.com
    - filter_chain_match:
         server_names: b.com

The first request (host: a.com) matches the first filter chain. The second request (host: b.com) from the same connection will use the first filter chain which doesn't make much sense to me.

How is this different from the case when you don't extract Host and/or :authority? Both requests are going to use the first filter chain anyway, since there won't be any other selector to act upon.

As mentioned previously, we already act on the TLS Server Name Indication in TLS Inspector, and if those request are sent over the same TLS connection, then both are going to be matched to the first filter chain.

@yxue
Copy link
Contributor Author

yxue commented Jul 12, 2019

I think TLS SNI is a little bit different than hosts. The TLS SNI sticks to each connection but host is per request. I am not sure if we should use host in filter chain match. I think server name in filter chain match should use some criteria that are constant for TCP connection, for example, destination IP:port or SNI.

lizan pushed a commit that referenced this issue Jul 19, 2019
)

Description: new listener filter for inspecting http protocol. 
  - Http1x: check request line
  - Http2: check connection preface
    
Pros: Performance; Cons:  False positive possibility

Risk Level: low
Testing: unit test, manual test
Docs Changes: Added
Release Notes: Added
#Issue: #7527

Signed-off-by: Yan Xue <[email protected]>
TAOXUY pushed a commit to TAOXUY/envoy that referenced this issue Jul 22, 2019
…voyproxy#7559)

Description: new listener filter for inspecting http protocol. 
  - Http1x: check request line
  - Http2: check connection preface
    
Pros: Performance; Cons:  False positive possibility

Risk Level: low
Testing: unit test, manual test
Docs Changes: Added
Release Notes: Added
#Issue: envoyproxy#7527

Signed-off-by: Yan Xue <[email protected]>
@yxue yxue closed this as completed Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

No branches or pull requests

4 participants