tls_inspector listener_filter is automatically added when using http_inspector

[ggreenway]

Description: When the http_inspector filter is used with a filter_chain_match rule using application_protocols, the tls_inspector listener_filter is automatically inserted as well.

Repro steps: Run the following config:

---
node:
  id: "id"
  cluster: cluster
admin:
  access_log_path: "admin_access.log"
  address:
    socket_address:
      address: 0.0.0.0
      port_value: 8080
cluster_manager:
  outlier_detection:
    event_log_path: "outlierevents.log"
static_resources:
  listeners:
  - name: mylistener
    address:
      socket_address:
        address: 0.0.0.0
        port_value: 80
    listener_filters:
    - name: envoy.listener.http_inspector
    filter_chains:
    - filter_chain_match:
        application_protocols:
        - "http/1.1"
        - "http/1.0"
      filters:
      - name: envoy.http_connection_manager
        config:
          stat_prefix: stats
          http_filters:
          - name: envoy.router
          route_config:
            name: local_route
            virtual_hosts:
            - name: local_service
              domains: "*"
              routes:
              - match:
                  prefix: "/"
                direct_response:
                  status: 404
                  body:
                    inline_string: "Error!\n\n"
    - filter_chain_match:
        application_protocols:
        - "h2c"
      filters:
      - name: envoy.http_connection_manager
        config:
          stat_prefix: stats
          http_filters:
          - name: envoy.router
          route_config:
            name: local_route
            virtual_hosts:
            - name: local_service
              domains: "*"
              routes:
              - match:
                  prefix: "/"
                route:
                  cluster: mycluster
  clusters:
  - name: mycluster
    connect_timeout: 1s
    type: STATIC
    http2_protocol_options: {}
    load_assignment:
      cluster_name: mycluster
      endpoints:
      - lb_endpoints:
        - endpoint:
            address:
              socket_address:
                address: 127.0.0.1
                port_value: 8001

Logs: [2020-03-05 13:00:32.620][26037][warning][config] [source/server/listener_impl.cc:299] adding listener '0.0.0.0:80': 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)

[ggreenway]

This is a minor annoyance, and possibly a minor performance degradation. Mostly just something I noticed and thought "that's not what I wanted to have happen".

[mattklein123]

cc @PiotrSikora is this intentional?

[ggreenway]

Here's what I think happened:

Long ago, the tls_inspector was added, and at the time, it was the only filter that would set application_protocols. So to make config less error-prone, we added auto-insertion of tls_inspector if a config was using a filter_chain_match that looked at application_protocols

Then later, the http_inspector was added, which can also set application_protocols. At that point, the auto-insertion of tls_inspector became incorrect in some cases. However, just removing the auto-insertion would break existing configs that depend on it.

Some options that I see:

• Do nothing at all and live with it. Potential downsides:
  • Possible (very remote) security vulnerability
  • tls_inspector doesn't have enough data to decide what the protocol is and holds up the connection waiting for more data (but the protocol, which is not TLS at all, maybe doesn't have any more client data until there's a server response?). I haven't seen this happen in practice, but it's a remote possibility. My guess is that if the first few bytes aren't correct, the tls_inspector will reject it pretty quickly in practice.
• Add config to the listener to disable auto-insertion
• CLI arg to disable auto-insertion
• Deprecate auto-insertion, follow the deprecation schedule

[PiotrSikora]

@ggreenway yeah, that's the reason why this happens.

However, I'd argue that this behavior is still correct given pasted configuration, since it depends on the application_protocols, which can be populated by the TLS inspector.

If you want to disable TLS inspector, then you can limit the scope of the match to a specific transport protocol (e.g. transport_protocol: envoy.transport_sockets.raw_buffer), and TLS inspector won't be auto-injected anymore.

[ggreenway]

Oh, that's a good catch regarding transport_protocol match. Thanks!

However, I'd argue that this behavior is still correct given pasted configuration, since it depends on the application_protocols, which can be populated by the TLS inspector.

My counter-argument is that Envoy doesn't have enough information here to know whether it is correct to do this. In many cases it is correct; in a case like the one I pasted above (where there's no TLS happening at all, either sniffing or terminating), it is incorrect.

But the workaround of making a more specific match is good enough for me; I prefer that to adding another config option at this point.