tls_inspector needlessly injected, causing incorrect behavior

[howardjohn]

Title: tls_inspector needlessly injected, causing incorrect behavior

Description:

When a filter chain is configured with: HTTP inspector, no tls inspector, a match on application_protocol, no match on transport_protocol, we get the following log:
adding listener '0.0.0.0:15006': filter chain match rules require TLS Inspector listener filter, but it isn't configured, trying to inject it (this might fail if Envoy is compiled without it)

This is incorrect - tls inspector should not be needed here, since the http inspector will add the application protocol. This is caused by the code here:

envoy/source/server/listener_impl.cc

Line 41 in 22683a0

bool needTlsInspector(const envoy::config::listener::v3::Listener& config) {

I would expect in this case, no TLS inspector is added. As a meta comment, automatically injecting this seems inconsistent with Envoys "fail fast" mentality but probably hard to change the behavior now.

Please note this is note benign - normally we would not care about the tls inspector being added since at worst its a trivial performance impact. However, we have recently moved to using ListenerFilterChainMatchPredicate to selectively enable listener filters to allow server first protocols to succeed. Because this is automatically injected, these are timing out here, forcing us to explicitly add the tls inspector in all cases.

Interestingly, we could just add transport_protocol = "raw_buffer" to the match, but then we will break all of our other matches due to #12572

Repro steps:

This config will easily reproduce

admin:
  access_log_path: /tmp/admin_access.log
  address:
    socket_address:
      address: 127.0.0.1
      port_value: 9901
      protocol: TCP
static_resources:
  listeners:
  - address:
      socketAddress:
        address: 0.0.0.0
        portValue: 15006
    filterChains:
    - filterChainMatch:
        applicationProtocols:
        - http/1.0
        - http/1.1
        - h2c
      filters:
      - name: envoy.filters.network.tcp_proxy
        typedConfig:
          '@type': type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy
          cluster: InboundPassthroughClusterIpv6
          statPrefix: InboundPassthroughClusterIpv6
      name: virtualInbound
    listenerFilters:
    - name: envoy.filters.listener.http_inspector
      typedConfig:
        '@type': type.googleapis.com/envoy.extensions.filters.listener.http_inspector.v3.HttpInspector

[lambdai]

tls_inspector was the only listener filter to set apln in the past, so auto tls_inspector injection is added.
Nowadays it's no longer true since http_inspector could also set alpn.
Can we add a flag to disable the auto injection? @htuch

[PiotrSikora]

That's actually a bit more complicated.

One of the application protocols in the example is http/1.1. This value can be set by HTTP inspector for plaintext HTTP/1.1, as well as by TLS inspector for HTTP/1.1 over TLS. Since this filter chain matches all transport protocols, it should match both of those cases, but if you disable TLS inspector, then it won't match HTTP/1.1 over TLS, which is technically incorrect behavior.

As @howardjohn pointed out, configuring this filter chain to match only raw_buffer transport protocol will stop TLS inspector from being auto-injected, and it will result in the correct behavior.

If you want to match only plaintext HTTP, that's the correct way to do it.

[lambdai]

@PiotrSikora From envoy's perspective, you are right. Envoy doesn't know the best strategy.
there are cases where tls_inspector should be added as you described
there are cases tls_inspector should not be added, as John mentioned adding raw_buffer has the risk.

I am proposing add the flag in api: let control plane decide if tls_inspector.
If control plane is smart enough to know this api, the control plane must do the right thing :)

[PiotrSikora]

If control plane is smart enough, it should set raw_buffer ;)

[howardjohn]

We are talking about a medium smart control plane - smart enough to set a flag but not smart enough for filter match raw buffer 🙂

We can easily set the tls inspector directly or possibly get the raw buffer working so I wouldn't do anything subpar to help us. Initially I thought the current behavior was wrong but now I am not so sure what the correct behavior is

[htuch]

It sounds like there is a reasonable control plane fix, is this really just a documentation issue?

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[howardjohn]

Just a followup: we did the workaround in the control plane and shipped a major regression despite writing (what we thought were) extensive tests, caused by my favorite other issue #12572. So yes, a smart control plane could workaround this, but not Istio 🙂.

This was our fault, but its also proving to be really hard to do the right thing

[howardjohn]

Also, the behavior to inject it seems inconsistent. If I set my filter chain matches to have "transportProtocol":"raw_buffer", one might expect this to reject all TLS traffic, since the tls_inspector is automagically injected in other cases. However, it isn't.

So if I don't set tls_inspector, raw_buffer will match anything, until I decide to add a field with app_protocol to any match in the filter chain, at which point it will suddenly stop matching since now we do have a tls inspector present, so we know its not raw_buffer.

Its essentially the same complaint I had in #12572 and #5355 (comment): changing one filter chain match has an outsized impact on every other match in the entire chain

[howardjohn]

The logic in

envoy/source/server/listener_impl.cc

Line 41 in 22683a0

bool needTlsInspector(const envoy::config::listener::v3::Listener& config) {

also doesn't take into account ListenerFilterChainMatchPredicate such that it may give false negatives

[PiotrSikora]

Also, the behavior to inject it seems inconsistent. If I set my filter chain matches to have "transportProtocol":"raw_buffer", one might expect this to reject all TLS traffic, since the tls_inspector is automagically injected in other cases. However, it isn't.

Without tls_inspector, Envoy doesn't have the ability to detect TLS traffic, so it cannot reject it.

So if I don't set tls_inspector, raw_buffer will match anything, until I decide to add a field with app_protocol to any match in the filter chain, at which point it will suddenly stop matching since now we do have a tls inspector present, so we know its not raw_buffer.

That's indeed unfortunate, but the simple solution is to always include tls_inspector.

The tls_inspector auto-injection was added to support backwards compatibility when we were adding support for multiple filter chains a few years ago, and it includes a warning since users were expected to review their configuration and include it explicitly if needed. It was always a bit of a hack, which is why it included a warning, so perhaps we should remove it, since users had enough time to notice it and fix their configuration. This could break existing configuration if the warning was ignored, though, so I'll defer to @htuch and @mattklein123 to make a call on that.

The logic in

envoy/source/server/listener_impl.cc

Line 41 in 22683a0

bool needTlsInspector(const envoy::config::listener::v3::Listener& config) {

also doesn't take into account ListenerFilterChainMatchPredicate such that it may give false negatives

That's because authors of ListenerFilterChainMatchPredicate ignored auto-injection, but to be fair, this feature has been broken at least a few times over the last 2 years, so it might be time to remove it.

[mattklein123]

The tls_inspector auto-injection was added to support backwards compatibility when we were adding support for multiple filter chains a few years ago, and it includes a warning since users were expected to review their configuration and include it explicitly if needed. It was always a bit of a hack, which is why it included a warning, so perhaps we should remove it, since users had enough time to notice it and fix their configuration. This could break existing configuration if the warning was ignored, though, so I'll defer to @htuch and @mattklein123 to make a call on that.

I would remove it but put the removal behind a feature flag. Then after a release we can remove it entirely.

[htuch]

The less magic in config transformation by Envoy the better, so I'm in favor of that.

[PiotrSikora]

It seems that we've reached consensus, thanks! @howardjohn does this work for Istio?

@tbarrella could you take care of this? We need to add a runtime flag to guard auto-injection of TLS inspector (it requires adding runtime flag check in ListenerImpl::buildTlsInspectorListenerFilter and adding relevant tests).

[howardjohn]

@PiotrSikora that will work for us, I think we will be fine with it auto injected or not by working around it in the control plane with explicit raw-buffer (except doing it right this time...). So it's not strictly required but it seems like the preferred behavior for us and others.

[tbarrella]

Yep, I think I can do this