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

refactor(proxy): specify hosts for proxy exclusion instead of complete URLs #6957

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SanchithHegde
Copy link
Member

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This PR refactors the logic for excluding specific HTTP / HTTPS traffic from being proxied via the outgoing proxy. Specifically, the changes included are:

  1. We no longer need two clients (a proxied client and a non-proxied client), we can have the same client handle traffic as required.
  2. We need not specify complete URLs for proxy exclusion, we can just specify the domain names or IP addresses of the hosts to be excluded. This also handles excluding traffic from a subdomain if a parent domain is excluded.

This is achieved with reqwest::Proxy::no_proxy() and reqwest::NoProxy::from_string().

Additional Changes

  • This PR modifies the API contract
  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

This PR introduces a config option bypass_proxy_hosts instead of bypass_proxy_urls under the [proxy] config section.

Motivation and Context

This would help significantly reduce maintenance efforts on our side with respect to which URLs we wanted to be excluded from being proxied, since we previously had some of these URLs being specified in code. The PR helps ensure that all the domains to be excluded would be specified via configuration alone, while also allowing us to easily exclude subdomains (for example, excluding cluster.local would exclude all traffic to services within a Kubernetes cluster from being proxied).

Closes #1039.

How did you test it?

Locally, by setting up mitmproxy as a proxy for outgoing HTTP and HTTPS traffic. I also set up our card vault locally to simulate an internal service running close to the application.

I ran mitmproxy using the command:

mitmweb --listen-port 8090 --web-port 8091

Application was configured in the development.toml file to use the proxy:

[proxy]
http_url = "http://localhost:8090"
https_url = "http://localhost:8090"
bypass_proxy_hosts = "localhost, cluster.local"
  1. Creating a payment via Postman:

    Screenshot of 2 requests being proxied

    Two requests are listed on the mitmproxy web interface, one to api.stripe.com and one to webhook.site, indicating that they were proxied. In this case, all HTTPS traffic is being proxied.

  2. Saving a payment method in the locker:

    The locker host is initially configured as:

    [locker]
    host = "http://127.0.0.1:3001"
    mock_locker = false

    Since the locker host was 127.0.0.1 and only localhost was excluded from being proxied, the locker request is also being proxied:

    Screenshot of card vault request being proxied

    On updating the locker host to http://localhost:3001 in the development.toml file, the locker request is no longer proxied, the mitmproxy web interface is blank:

    Screenshot of a blank mitmproxy web interface

    As another confirmation that the locker call actually happened, I see logs emitted by the locker application with the same request ID as that returned by the router in its response headers.

    One more thing to note here is that we did not have to include the port number (localhost:3001) in the bypass_proxy_hosts list, only localhost was sufficient.

  3. Excluding stripe.com from being proxied and creating a payment:

    I updated the development.toml file to include stripe.com in bypass_proxy_hosts:

    bypass_proxy_hosts = "localhost, cluster.local, stripe.com"

    Even though we only excluded stripe.com and did not explicitly exclude api.stripe.com, the request to api.stripe.com was excluded as well. This can be compared with the screenshot in (1), the request to api.stripe.com is not listed:

    Screenshot of a request being proxied

(2) and (3) indicate that proxy exclusion is happening as expected.

Testing on our hosted environments

In case of our hosted environments, we would have to ensure that nothing with respect to outgoing traffic from the router application remains affected: if an outgoing proxy has been configured (correctly), all of the external HTTP(S) traffic (to connectors, etc.) must be proxied, while traffic to our internal services such as our locker or encryption service must be excluded from being proxied.

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed the submitted code
  • I added unit tests for my changes where possible

@SanchithHegde SanchithHegde added A-framework Area: Framework S-waiting-on-review Status: This PR has been implemented and needs to be reviewed M-configuration-changes Metadata: This PR involves configuration changes C-refactor Category: Refactor labels Dec 29, 2024
@SanchithHegde SanchithHegde added this to the December 2024 Release milestone Dec 29, 2024
@SanchithHegde SanchithHegde self-assigned this Dec 29, 2024
@SanchithHegde SanchithHegde requested a review from a team as a code owner December 29, 2024 12:26
Copy link

semanticdiff-com bot commented Dec 29, 2024

Review changes with  SemanticDiff

Changed Files
File Status
  crates/router/src/lib.rs  67% smaller
  crates/router/src/bin/scheduler.rs  51% smaller
  crates/router/src/services/api/client.rs  32% smaller
  crates/router/src/configs/settings.rs  28% smaller
  crates/router/src/services/openidconnect.rs  15% smaller
  crates/router/src/configs/defaults.rs  12% smaller
  crates/router/src/services/api.rs  5% smaller
  config/config.example.toml Unsupported file format
  config/deployments/env_specific.toml Unsupported file format
  crates/common_enums/src/enums.rs  0% smaller

@@ -623,7 +623,7 @@ pub struct Proxy {
pub http_url: Option<String>,
pub https_url: Option<String>,
pub idle_pool_connection_timeout: Option<u64>,
pub bypass_proxy_urls: Vec<String>,
pub bypass_proxy_hosts: Option<String>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not using a Vec or HashSet here (and splitting the string on commas), we pass the string directly to NoProxy::from_string(), which handles the splitting by comma and trimming whitespaces.

Comment on lines +32 to +33
let proxy_exclusion_config =
reqwest::NoProxy::from_string(&proxy_config.bypass_proxy_hosts.clone().unwrap_or_default());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using unwrap_or_default() here, since NoProxy::from_string() returns None if the string is empty, and since Proxy::no_proxy() accepts Option<NoProxy>.

proxy_client: reqwest::Client,
non_proxy_client: reqwest::Client,
whitelisted_urls: Vec<String>,
proxy_config: Proxy,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the proxy_config field, which is required when constructing a client where certificates are specified.

Comment on lines +198 to +199
pub fn new(proxy_config: &Proxy) -> CustomResult<Self, ApiClientError> {
let client = get_client_builder(proxy_config)?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to call get_client_builder(), since most of the code was duplicated.

Comment on lines 215 to +216
(Some(certificate), Some(certificate_key)) => {
let client_builder =
reqwest::Client::builder().redirect(reqwest::redirect::Policy::none());
let client_builder = get_client_builder(&self.proxy_config)?;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this to call get_client_builder() as well, since the get_reqwest_client() method did not previously configure proxies for the ClientBuilder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-framework Area: Framework C-refactor Category: Refactor M-configuration-changes Metadata: This PR involves configuration changes S-waiting-on-review Status: This PR has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Use proxy exclusion instead of a separate proxied client
4 participants