envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.67k stars 4.75k forks source link

Upgrade to Websocket doesn't ignore disabled 'typed_per_filter_config' for this route #35191

Closed DvoryankinEvgeny closed 1 month ago

DvoryankinEvgeny commented 1 month ago

Title: Upgrade to Websocket doesn't ignore disabled 'typed_per_filter_config' for this route

Description: We have a configuration where we have two http filters: basic_auth and router. We also have a set of routes:

This configuration works as expected:

curl http://localhost:8080/ 
User authentication failed. Missing username and password.%

Request to the root without credentials rejected

curl -u user:password http://localhost:8080
OK%

Request to the root with correct pair of username and password succeeded.(OK is direct_response from envoy)

curl http://localhost:8080/disable-basic-auth
OK%

Request without credentials to the endpoint with disabled basic authorization also works fine.

By analogy with those endpoints we have two endpoints for WebSocket connections with upgrade_configs and upgrade_type equal to websocket:

Request to the first Websocket endpoint finishes with 401 Unauthorized as expected because we didn't disabled basic auth for this endpoint.

websocat ws://localhost:8080/ws
websocat: WebSocketError: WebSocketError: Received unexpected status code (401 Unauthorized)

Request to /ws-disable-basic-auth is expected to be successfully passed to the upstream, but unfortunately it is not.

websocat ws://localhost:8080/ws-disable-basic-auth
websocat: WebSocketError: WebSocketError: Received unexpected status code (401 Unauthorized)

Repro steps:

  1. Save config from Config section into envoy.yaml file
  2. Create docker-compose.yaml file with following content:

    
    services:
    websocat_server:
    image: ghcr.io/vi/websocat
    command: [ "-s", "0.0.0.0:8082" ]
    ports:
      - 8082:8082
    networks:
      my_network:
        ipv4_address: 172.20.0.2
    
    envoy:
    image: envoyproxy/envoy:v1.30.4
    command:
      [
        "envoy",
        "-l",
        "trace",
        "-c",
        "/etc/envoy/envoy.yaml"
      ]
    volumes:
      - .envoy.yaml:/etc/envoy/envoy.yaml
    ports:
      - 8080:8080
      - 9902:9902
    depends_on:
      - websocat_server
    networks:
      my_network:
        ipv4_address: 172.20.0.3

networks: my_network: ipam: config:

3. Run `docker-compose up -d`
4. Run locally `websocat ws://localhost:8080/ws-disable-basic-auth` or curl equivalent `curl -v -H "Sec-WebSocket-Version: 13" -H "Sec-WebSocket-Key: bKAazDmMg16GANGcKoHGFg==" -H "Connection: Upgrade" -H "Upgrade: websocket" http://localhost:8080/ws-disable-basic-auth`

**Expected result:** basic_auth filter is disabled for websocket calls to `/ws-disable-basic-auth` endpoint
**Actual behaviour:** websocket calls to `/ws-disable-basic-auth` endpoint require basic auth

*Admin and Stats Output*:
[server_info.json](https://github.com/user-attachments/files/16237122/server_info.json)
[cluster.log](https://github.com/user-attachments/files/16237123/cluster.log)
[stats.log](https://github.com/user-attachments/files/16237124/stats.log)

*Config*:

admin: address: socket_address: address: 0.0.0.0 port_value: 9902 static_resources: listeners:

Logs: I attached two logs files:

DvoryankinEvgeny commented 1 month ago

To simplify reproducing the bug I implemented integration tests:

static const std::string my_config = R"EOF(
            name: typed-per-filter-config-for-ws
            virtual_hosts:
            - name: app-ws
              domains:
              - "*"
              routes:
              - match:
                  prefix: "/disable-basic-auth"
                direct_response:
                  status: 200
                  body:
                    inline_string: "OK"
                typed_per_filter_config:
                  envoy.filters.http.basic_auth:
                    "@type": type.googleapis.com/envoy.config.route.v3.FilterConfig
                    disabled: true
              - match:
                  prefix: "/ws-disable-basic-auth"
                route:
                  cluster: cluster_0
                  upgrade_configs:
                  - upgrade_type: websocket
                typed_per_filter_config:
                  envoy.filters.http.basic_auth:
                    "@type": type.googleapis.com/envoy.config.route.v3.FilterConfig
                    disabled: true
              - match:
                  prefix: "/ws"
                route:
                  cluster: cluster_0
                  upgrade_configs:
                  - upgrade_type: websocket
              - match:
                  prefix: "/"
                direct_response:
                  status: 200
                  body:
                    inline_string: "OK"
)EOF";

static const std::string my_filter = R"EOF(
              name: envoy.filters.http.basic_auth
              typed_config:
                "@type": type.googleapis.com/envoy.extensions.filters.http.basic_auth.v3.BasicAuth
                users:
                  inline_string: |-
                    user:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=
)EOF";

class MyIntegrationTest : public Envoy::HttpIntegrationTest, public testing::Test {
public:
  MyIntegrationTest()
      : HttpIntegrationTest(Http::CodecType::HTTP1, Network::Address::IpVersion::v4) {}

  void initializeRoutes() {
    config_helper_.addConfigModifier(
        [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager&
                hcm) -> void {
          auto route_config = hcm.mutable_route_config();
          TestUtility::loadFromYaml(my_config, *route_config);
        });
    initialize();
  }

  void addBasicAuthFilter() { config_helper_.prependFilter(my_filter); }

void performTest() {
    codec_client_ = makeHttpConnection(lookupPort("http"));

    Http::TestRequestHeaderMapImpl request_headers{
        {":method", "GET"},
        {":path", "/ws-disable-basic-auth"},
        {":scheme", "http"},
        {":authority", "host"},
        {"connection", "Upgrade"},
        {"upgrade", "websocket"},
        {"sec-websocket-key", "dGhlIHNhbXBsZSBub25jZQ=="},
        {"sec-websocket-version", "13"}};
    Http::TestRequestHeaderMapImpl response_headers{
        {":status", "101"},
        {"connection", "Upgrade"},
        {"upgrade", "websocket"},
        {"Sec-WebSocket-Accept", "EDJa7WCAQQzMCYNJM42Syuo9SqQ="}};

    auto encoder_decoder = codec_client_->startRequest(request_headers);
    request_encoder_ = &encoder_decoder.first;
    auto response = std::move(encoder_decoder.second);

    ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_));
    ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_));
    ASSERT_TRUE(upstream_request_->waitForHeadersComplete());

    // Accept websocket upgrade request
    upstream_request_->encodeHeaders(response_headers, false);
    response->waitForHeaders();
    EXPECT_EQ("101", response->headers().getStatusValue());
    cleanupUpstreamAndDownstream();
  }
};

TEST_F(MyIntegrationTest, SuccessfulWebsocketUpgradeWithoutBasicAuthFilter) {
  initializeRoutes();
  performTest();
}

TEST_F(MyIntegrationTest, WebsocketUpgradeFailsWhenBasicAuthFilterPresnted) {
  addBasicAuthFilter();
  initializeRoutes();
  performTest();
}
KBaichoo commented 1 month ago

Thank you for the reproduction @DvoryankinEvgeny . ISTM like a bug, possibly here: https://github.com/envoyproxy/envoy/blob/f79b881883e862bc0f7dc7f09d3bc811fb0944f6/source/common/http/filter_manager.cc#L1571

Where we create the upgraded filter chain and not setting the route info.

I wonder if you flip the filter to be default off but enabled for the routes if that would be a hacky workaround.

DvoryankinEvgeny commented 1 month ago

Thank you for the reproduction @DvoryankinEvgeny . ISTM like a bug, possibly here:

https://github.com/envoyproxy/envoy/blob/f79b881883e862bc0f7dc7f09d3bc811fb0944f6/source/common/http/filter_manager.cc#L1571

Where we create the upgraded filter chain and not setting the route info.

I wonder if you flip the filter to be default off but enabled for the routes if that would be a hacky workaround.

Hello Kevin, Thank you for the response. I am quite novice with the envoy, so I am struggling with making basic_auth disabled by default. If I add disabled: true like this:

http_filters:
  - name: envoy.filters.http.basic_auth
    disabled: true
    typed_config:
      "@type": type.googleapis.com/envoy.extensions.filters.http.basic_auth.v3.BasicAuth
      users:
        inline_string: |-
          user:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=

then envoy start fails with error _Empty route/virtual host per filter configuration for envoy.filters.http.basicauth filter Could you please help with configuration?

DvoryankinEvgeny commented 1 month ago

@KBaichoo hi, I think I found the problem. It seems to me, that the root case is here: https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/source/extensions/filters/network/http_connection_manager/config.cc#L810-L811 We pass an instance of Http::EmptyFilterChainOptions to the call of Http::FilterChainUtility::createFilterChainForFactories(). Implementation of Http::EmptyFilterChainOptions::filterDisabled() always returns a default-constructed absl::optional<bool>, so when we come to the FilterChainUtility::createFilterChainForFactories() call, https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/source/common/http/filter_chain_helper.cc#L17-L26 we call Http::EmptyFilterChainOptions::filterDisabled() to check if a filter is disabled, and since it always returns a default-constructed absl::optional<bool> we think, that all filters are enabled.

As one of the possible solutions I tried to extend the signature of the createUpgradeFilterChain() method declared here https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/envoy/http/filter_factory.h#L116-L119 with a new parameter const Http::FilterChainOptions& option = Http::EmptyFilterChainOptions{}. In the implementation of HttpConnectionManagerConfig::createUpgradeFilterChain I passed this new parameter option into the call Http::FilterChainUtility::createFilterChainForFactories instead of Http::EmptyFilterChainOptions here https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/source/extensions/filters/network/http_connection_manager/config.cc#L810-L811

In FilterManager::createFilterChain() moved up a little bit creation of FilterChainOptionsImpl option https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/source/common/http/filter_manager.cc#L1582-L1583 and passed this option into createUpgradeFilterChain call here https://github.com/envoyproxy/envoy/blob/189dd4700f823f2cbbef482c61d50a34e16ff4f2/source/common/http/filter_manager.cc#L1569-L1570

After all these manipulation my tests started to work as expected :)

So, I have two questions:

  1. Is it appropriate fix? Do you see any problems?
  2. Could you please suggest a proper place for my integration tests? Right now I put them into test/extensions/filters/http/basic_auth/basic_auth_integration_test.cc just because there already were some tests for basic auth.
DvoryankinEvgeny commented 1 month ago

I created a draft PR to show what I actually want to change. Hope it will simplify understanding. https://github.com/envoyproxy/envoy/pull/35292