dynamic_modules: add remote HTTP support for asyncDataSource module binary

[kanurag94]

Commit Message: dynamic_modules: add remote HTTP support for asyncDataSource module binary
Additional Description: Extends the asyncDataSource support (added in #43333 for local.filename) to support fetching dynamic module binaries from remote HTTP sources via module.remote in DynamicModuleConfig. The module is downloaded asynchronously during listener initialization with SHA256 verification, written to a temporary file, and loaded via dlopen. If the remote fetch fails, the filter is not installed and requests pass through (fail-open).
Risk Level: Low
Testing: Added unit tests + Manually tested
Docs Changes: API docs already a part of this PR
Release Notes: Added

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #43818 was opened by kanurag94.

see: more, trace.

[kanurag94]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the ability to fetch dynamic modules from remote HTTP sources asynchronously. However, critical security vulnerabilities were identified in the caching and temporary file handling mechanisms. These include the use of the world-writable /tmp directory with predictable filenames and a lack of integrity verification for cached files, which could lead to local privilege escalation and arbitrary code execution. The staging file creation is also vulnerable to symlink attacks and race conditions. Furthermore, a critical use-after-free bug was found in the asynchronous callback implementation.

[gemini-code-assist]

The lambda for the RemoteAsyncDataProvider captures context and scope by reference. These references point to objects that are typically allocated on the stack during the configuration loading process. Since the lambda is executed asynchronously after the remote data fetch completes, the original stack frame will be gone, leading to dangling references and a use-after-free. This is a critical issue that could lead to crashes.

To resolve this, you should avoid capturing stack variables by reference in an asynchronous callback. Instead, you should extract the required long-lived objects (e.g., from context.api(), context.mainThreadDispatcher(), etc.) and capture those. This will likely require refactoring buildFilterFactoryCallback to accept these individual components instead of the entire ServerFactoryContext.

[kanurag94]

• ServerFactoryContext is long-lived and not stack allocated.
• stats::scope is owned by listener and is again long lived.
• RemoteAsyncDataProvider is fired during init so both references are guaranteed valid.

This comment seems wrong.

[kanurag94]

/retest

[kanurag94]

/retest

[repokitteh-read-only]

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/coverage.yaml).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #43818 was ready_for_review by kanurag94.

see: more, trace.

[agrawroh]

Do we really have to decrease the coverage here? is there no way to have an integration test to cover everything?

[kanurag94]

The coverage drop comes mainly from defensive paths:

• mkstemp() failure -- don't know how clearly but probably a exhaustion of FDs should only trigger this.
• write() failure -- needs interrupt signals or disk permission issues
• std::filesystem::permissions/rename failure -- needs mocking read only permissions.
• weak_ptr expiry -- needs a race condition to trigger.

They'd need exceptional situations to happen. All of them are very tought for UTs so I lowered the coverage instead.

Writing an integration test is quite hard for this one, if the approach looks good -- I can give it a try.

I have tried to increase coverage back with some possible paths that can be tested.

[wbpcode]

Because the following lambda will be called at any threads, I think there may result in unexpected contention? here?

[async_state](Http::FilterChainFactoryCallbacks& callbacks) -> void {
    if (async_state->filter_factory_cb) {
      async_state->filter_factory_cb(callbacks);
    }
  };

As the first version, to simplify the review, we I think we and use a lock and left a TODO here. Then, we can resolve the lock with another PR. cc @agrawroh WDYT?

[wbpcode]

wait, the remote fetching will pause the warming...so, it may be safe

[agrawroh]

@wbpcode The remote fetching will pause the warming...so, it may be safe...

Yes, that's my impression as well.

[wbpcode]

After re-thinking of the implementation, I changed a little of my mind. There three different solutions for async data fetching:

1. The current RemoteAsyncDataProvider. It will block the warming of filter_chain/listener until initial fetching success or failure. But it cannot support the per route level filter configuration or route level filter chain (by composite/filter_chain or any similar thing)
2. always nack if cache miss and start an sync task at background. Accept the configuration until the data is downloaded. Will result in some configuration rejections and in worst case, may block bootstrap of Envoy if the remote data server is down.
3. always accept the configuration first and fetch the data at background. May result in in consistent behavior or even security problem.

I am thinking maybe 2 is better way to go or hybriding 1 and 2. WDYT? cc @agrawroh @kanurag94

[kanurag94]

After re-thinking of the implementation, I changed a little of my mind. There three different solutions for async data fetching:

1. The current RemoteAsyncDataProvider. It will block the warming of filter_chain/listener until initial fetching success or failure. But it cannot support the per route level filter configuration or route level filter chain (by composite/filter_chain or any similar thing)
2. always nack if cache miss and start an sync task at background. Accept the configuration until the data is downloaded. Will result in some configuration rejections and in worst case, may block bootstrap of Envoy if the remote data server is down.
3. always accept the configuration first and fetch the data at background. May result in in consistent behavior or even security problem.

I am thinking maybe 2 is better way to go or hybriding 1 and 2. WDYT? cc @agrawroh @kanurag94

I agree. I wanted to implement 1 in this PR, and 2 in a follow up since we discussed splitting it into two PRs when I raised #43333 (nack_on_cache_miss). I can add the functionality here if needed. Or document the per-route level behavior (though I will raise a PR very soon).

[wbpcode]

After re-thinking of the implementation, I changed a little of my mind. There three different solutions for async data fetching:

1. The current RemoteAsyncDataProvider. It will block the warming of filter_chain/listener until initial fetching success or failure. But it cannot support the per route level filter configuration or route level filter chain (by composite/filter_chain or any similar thing)
2. always nack if cache miss and start an sync task at background. Accept the configuration until the data is downloaded. Will result in some configuration rejections and in worst case, may block bootstrap of Envoy if the remote data server is down.
3. always accept the configuration first and fetch the data at background. May result in in consistent behavior or even security problem.

I am thinking maybe 2 is better way to go or hybriding 1 and 2. WDYT? cc @agrawroh @kanurag94

I agree. I wanted to implement 1 in this PR, and 2 in a follow up since we discussed splitting it into two PRs when I raised #43333 (nack_on_cache_miss). I can add the functionality here if needed. Or document the per-route level behavior (though I will raise a PR very soon).

SGTM. :)

[agrawroh]

@kanurag94 @wbpcode

Option 2 would require a module cache which is non-trivial so I'm okay with this PR in it's current form, and I agree with the plan to add Option 2 as a follow-up. Per-route limitation is pre-existing and it's acceptable.

[wbpcode]

LGTM. Thanks.

[wbpcode]

I think than we can add in following PRs:

• cache support
• nack on cache miss

[isker]

Could the "from bytes" support added here be used to enable local inline_bytes support as well? https://www.envoyproxy.io/docs/envoy/v1.37.1/api-v3/config/core/v3/base.proto#envoy-v3-api-msg-config-core-v3-datasource

[kanurag94]

Could the "from bytes" support added here be used to enable local inline_bytes support as well? https://www.envoyproxy.io/docs/envoy/v1.37.1/api-v3/config/core/v3/base.proto#envoy-v3-api-msg-config-core-v3-datasource

We discussed this in review and concluded that it may not be actually ever useful to have inline_bytes except for completion. #43333 (comment)