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

feat(websocket): Add proxy configuration #1536

Open
wants to merge 1 commit into
base: v1
Choose a base branch
from

Conversation

jnugh
Copy link
Contributor

@jnugh jnugh commented Jul 12, 2024

Allow usage of http(s) proxy for websocket traffic

I did not create an issue beforehand but I can create one if this is required.

This change adds functionality to connect to a websocket trough an http proxy (which are sometimes required in corporate environments 😿). It's probably also an interesting feature for debugging as it allows developers to intercept websocket traffic using tools like mitmproxy.

I also added two functions (reconfigure_tls_connector and reconfigure_proxy) as both settings might be changed by the end user - at least in our case. I tried to remove and re-add the plugin but this did not change the internal state as manage checks if an item is already present. I can move those to a different PR or implement this in a different way as well. Just let me know.

@jnugh jnugh requested a review from a team as a code owner July 12, 2024 14:09
* Allow usage of http(s) proxy for websocket traffic
@jnugh jnugh force-pushed the websocket-proxy branch from 1d1a1f4 to d25323b Compare July 12, 2024 14:10
@FabianLars
Copy link
Member

Thanks for contributing! I'm a bit swamped with v2 stuff and generally aren't experienced with proxies so reviewing may take a while. That said, did you see the PRs for the v2 http and updater plugins?
#824
#891

They allow configuring the proxy on each call from the js side. Does it make sense to follow that approach here too? This wouldn't work for the TlsConnector so i wouldn't mind sticking to a rust-only solution.

@jnugh
Copy link
Contributor Author

jnugh commented Jul 16, 2024

Hi, thanks for the quick reply. Don't worry we are using the PR branch in the meantime. I can rebase this onto v2 if this would be helpful.

Technically the configuration on a per-call basis would work. Even passing the TLS Connector settings would be possible. What we though was basically:
Both settings might contain sensitive credentials and we try not to keep passwords inside the rust context and use the credentials store of the operating system. Having a command to read the password would allow someone with access to the JS environment to read this password as well.

Maybe having both would be a good idea? Defining defaults in the rust context and allow overrides.

@FabianLars
Copy link
Member

Both settings might contain sensitive credentials and we try not to keep passwords inside the rust context and use the credentials store of the operating system. Having a command to read the password would allow someone with access to the JS environment to read this password as well.

Yeah, that was my concern as well but seeing the other plugins not caring about it i thought i may not be an issue. Maybe the auditors will scream at us once they look at the plugins too 🙃

Let's keep it on an on-demand basis and stick to rust-only, adding js support once requested.

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

Successfully merging this pull request may close these issues.

2 participants