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

Add support for mTLS #235

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

Add support for mTLS #235

wants to merge 4 commits into from

Conversation

mg-dd
Copy link

@mg-dd mg-dd commented Aug 22, 2023

This PR fixes two issues:

#194
#234

The background is that the TLS connection established by the proxy agents do not add the required parameters for mTLS / self-signed CAs. This PR adds a way to provide these parameters. The general approach was taken from the feedback from this PR: https://github.com/TooTallNate/proxy-agents/pull/195/files

ptal @TooTallNate

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2023

🦋 Changeset detected

Latest commit: 041a7c5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
https-proxy-agent Minor
socks-proxy-agent Minor
agent-base Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Aug 22, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
proxy-agents ✅ Ready (Inspect) Visit Preview Sep 4, 2023 9:13am

Copy link
Owner

@TooTallNate TooTallNate left a comment

Choose a reason for hiding this comment

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

Great looking PR! Thank you so much. I have some feedback, but once that's addressed I'll be happy to merge this.

packages/agent-base/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 119 to 121
servername: string | undefined,
opts: tls.ConnectionOptions,
socket?: net.Socket,
Copy link
Owner

Choose a reason for hiding this comment

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

Seems weird to me that socket is optional. Also, let's make it the first parameter:

Suggested change
servername: string | undefined,
opts: tls.ConnectionOptions,
socket?: net.Socket,
socket: net.Socket,
servername: string | undefined,
opts: tls.ConnectionOptions,

Copy link
Author

Choose a reason for hiding this comment

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

The reason it was optional was that pac-proxy-agent calls tls.connect without providing a socket. I've made your suggested changes and reverted the changes to pac-proxy-agent for now. Let me know how you would like to proceed in that regard.

Copy link
Owner

@TooTallNate TooTallNate Sep 4, 2023

Choose a reason for hiding this comment

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

I see. How about if we do something like this in pac-proxy-agent?

	socket = net.connect(opts);
	
	if (secureEndpoint) {
		const servername = opts.servername || opts.host;
		socket = this.upgradeSocketToTls(
			socket,
			servername,
			opts
		);
	}

Choose a reason for hiding this comment

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

@mg-dd Did this piece of feedback get addressed? I don't see any change in pac-proxy-agent so it looks like this still needs to be done?

packages/agent-base/src/index.ts Outdated Show resolved Hide resolved
packages/socks-proxy-agent/src/index.ts Show resolved Hide resolved
packages/socks-proxy-agent/src/index.ts Outdated Show resolved Hide resolved
@patrickheeney
Copy link

@mg-dd Great PR! I need something similar, do you need any help pushing this over the finish line?

@mg-dd
Copy link
Author

mg-dd commented Aug 29, 2023

@TooTallNate this is ready for another look. The only open question is how you would like to handle the TLS connection in pac-proxy-agent.

@brunobastosg
Copy link

Hi, any news on this? I also need this feature.

@amirbilu
Copy link

amirbilu commented Oct 4, 2023

Any news on this?

@cptRaclette
Copy link

@TooTallNate: Would love to see this feature in the proxy agents. Is there anything left to do?

@ErwanRaulo
Copy link

ErwanRaulo commented Dec 28, 2023

HI, any update about this PR ? 💯

@marcchambon
Copy link

Thank you soo much for maintaining this awesome project !
Any news ?

@sibelius
Copy link

can we move forward on this?

what is missing?

@pebie
Copy link

pebie commented Feb 1, 2024

Thank you for this PR !
@TooTallNate Any news about moving forward ?

🙏 🙏

@sibelius
Copy link

sibelius commented Apr 1, 2024

we are using a patch of this pull request in production and it is working well

@NasGldn
Copy link

NasGldn commented Jun 14, 2024

Hello, is there news about this ? We need it ! :) @TooTallNate

@szubster
Copy link

Any plans to get it merged?

@marcotranchino
Copy link

This would be really helpful.

marcotranchino added a commit to alphagov/pay-frontend that referenced this pull request Aug 9, 2024
…proxy

With this change, we have update the Apple Pay Merchant Validation
implementation in order to use `hpagent` and `axios` in the presence of an
egress proxy, as it's the case on our AWS environments.

This is needed because we want to remove the use of `requestretry`, however
`axios` has a problem preventing it from working with an egress proxy[1].

For this reason, we need to use an HttpsProxyAgent with it.

We would want to use `https-proxy-agent`, however it has its own problem[2].

While we wait for these issues to be fixed, we can use `hpagent` which has
been tested and works well with an egress proxy.

Further information in the JIRA ticket[3].

[1]
axios/axios#4531

[2]
TooTallNate/proxy-agents#235

[3]
https://payments-platform.atlassian.net/browse/PP-12853

Co-authored-by: Jonathan Harden <[email protected]>
Co-authored-by: Dominic Belcher <[email protected]>
Co-authored-by: Marco Tranchino <[email protected]>
marcotranchino added a commit to alphagov/pay-frontend that referenced this pull request Aug 9, 2024
With this change, we have update the Apple Pay Merchant Validation
implementation in order to use `hpagent` and `axios` in the presence of an
egress proxy, as it's the case on our AWS environments.

This is needed because we want to remove the use of `requestretry`, however
`axios` has a problem preventing it from working with an egress proxy[1].

For this reason, we need to use an HttpsProxyAgent with it.

We would want to use `https-proxy-agent`, however it has its own problem[2].

While we wait for these issues to be fixed, we can use `hpagent` which has
been tested and works well with an egress proxy.

Further information in the JIRA ticket[3].

[1]
axios/axios#4531

[2]
TooTallNate/proxy-agents#235

[3]
https://payments-platform.atlassian.net/browse/PP-12853

Co-authored-by: Jonathan Harden <[email protected]>
Co-authored-by: Dominic Belcher <[email protected]>
Co-authored-by: Marco Tranchino <[email protected]>
marcotranchino added a commit to alphagov/pay-frontend that referenced this pull request Aug 9, 2024
With this change, we have update the Apple Pay Merchant Validation
implementation in order to use `hpagent` and `axios` in the presence of an
egress proxy, as it's the case on our AWS environments.

This is needed because we want to remove the use of `requestretry`, however
`axios` has a problem preventing it from working with an egress proxy[1].

For this reason, we need to use an HttpsProxyAgent with it.

We would want to use `https-proxy-agent`, however it has its own problem[2].

While we wait for these issues to be fixed, we can use `hpagent` which has
been tested and works well with an egress proxy.

Further information in the JIRA ticket[3].

[1]
axios/axios#4531

[2]
TooTallNate/proxy-agents#235

[3]
https://payments-platform.atlassian.net/browse/PP-12853

Co-authored-by: Jonathan Harden <[email protected]>
Co-authored-by: Dominic Belcher <[email protected]>
Co-authored-by: Marco Tranchino <[email protected]>
@FewKinG
Copy link

FewKinG commented Nov 25, 2024

I am working in a scenario where we have to do mTLS through an outgoing (corporate) tunneling proxy.
This is kind of an exotic setup, I know, but freaking no proxy/agent implementation I can find (and that seems to be reasonably maintained) properly supports this.

This library with this PR is exactly what I need and it works! I applied the changes from this PR locally to test and verify. For now I will therefore use my custom copy with this patch to make it work. It seems like the author has addressed the issues with pac-proxy-agent at least in some way, could you please have another look, I would really love to use the "official" version of this as soon as this is merged.

Thank you!

@Chaz0r
Copy link

Chaz0r commented Nov 26, 2024

It really would be great for us to have this PR merged. We implemented that ourselfs now and it absolutely works. But for us it would be better to have this PR officially merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.