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

[ResponseOps] https proxy agent options assignment is made in the wrong place #196602

Open
pmuellr opened this issue Oct 16, 2024 · 3 comments
Open
Labels
bug Fixes for quality problems that affect the customer experience Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)

Comments

@pmuellr
Copy link
Member

pmuellr commented Oct 16, 2024

The code below is where we set the custom host ssl options in the agent, before making requests:

if (agentOptions) {
httpsAgent.options = {
...httpsAgent.options,
...agentOptions,
};
}

Unfortunately, this seems to be a no-op. By the time the ultimate socket is opened to the endpoint (via CONNECT to the proxy), the options are no longer there, and in fact the default. In this case, the important ones being SSL-related (rejectUnauthorized, ca, etc).

Not clear if this is because we are setting them AFTER we create the agent, or if it's an inherent bug in the agent itself. It doesn't look to me like the assignment of the options does anything useful within the agent, and doesn't seem to be used when constructing parameters for the ultimate TLS connection, so feels like the first (options set AFTER agent created). Quite possible this did work at some time, but was dependent on how the agent is actually coded, and doesn't work in newer releases of the agent.

You can see in this PR, we set rejectUnauthorized in the constructor options, which is what I expect we should do. Line 47 of x-pack/plugins/actions/server/builtin_action_types/lib/get_proxy_agents.ts:

https://github.com/elastic/kibana/pull/86415/files#diff-3904070ded12b9cd170538e0e852699e3bb3924b5d45795a8d0a71572da9bc5d

However, shortly after, in this PR, we change it to set the options after the constructor call. Line 113 of the same file.

https://github.com/elastic/kibana/pull/96630/files#diff-e4b9b3cd3d50b6c092a98136bba35c1b001e6602bb102257bf22a564437db5c3

Sometime later, the code was changed to what it is today:

const httpAgent = new HttpProxyAgent(proxySettings.proxyUrl);
const httpsAgent = new HttpsProxyAgent({
host: proxyUrl.hostname,
port: Number(proxyUrl.port),
protocol: proxyUrl.protocol,
headers: proxySettings.proxyHeaders,
// do not fail on invalid certs if value is false
...proxyNodeSSLOptions,
}) as unknown as HttpsAgent;
// vsCode wasn't convinced HttpsProxyAgent is an https.Agent, so we convinced it
if (agentOptions) {
httpsAgent.options = {
...httpsAgent.options,
...agentOptions,
};
}

@pmuellr pmuellr added Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Oct 16, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@pmuellr
Copy link
Member Author

pmuellr commented Oct 16, 2024

I tried moving the agentOptions into the options section just above it, just below ...proxyNodeSSLOptions. I can see that the value does seem to be looked at by the agent code now, however I'm not seeing it passed on the ultimate connect call to the endpoint, which makes me think this isn't quite correct either.

I think I'm not going to be happy unless we have some automated tests, testing this stuff. It looks like we do already have some tests set up, so it's not clear why they are working - or perhaps they are skipped or this setup isn't otherwise used.

const customHostSettingsValue = [
{
url: tlsWebhookServers.rejectUnauthorizedFalse,
ssl: {
verificationMode: 'none',
},
},

@mikecote mikecote added the bug Fixes for quality problems that affect the customer experience label Oct 18, 2024
@pmuellr
Copy link
Member Author

pmuellr commented Oct 22, 2024

Ah, turns out it's worse than I thought: TooTallNate/proxy-agents#111 (comment)

TL;DR: for axios, you need to use an https agent to pass TLS options for the target connect. For https-proxy-agent, it only supports setting options for the proxy, not the target. The assumption on the proxy package is "you can do this from your https client". Which is true if you are using an https client with explicit TLS options in it's "connect" or "configure" stage. Axios though, does not.

The linked reference includes some potential work-arounds:

  • use hpagent instead of https-proxy-agent
  • use the subclass trick with callback
  • see if we can chain multiple agents together, by putting a plain old https agent with the TLS options "in front of" (I think?) the https-proxy-agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams)
Projects
None yet
Development

No branches or pull requests

3 participants