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

POC: allow connect options to be passed to the proxy #81

Closed
wants to merge 1 commit into from

Conversation

pmuellr
Copy link

@pmuellr pmuellr commented Jul 14, 2022

I'm looking to use this package in a product, but will need support to pass rejectUnauthorized, cert info, etc, into the proxy itself. Right now, looking for the best approach to do this, and this is what I have so far. Totally open for other approaches.

I noticed it's going to be hard to test things like rejectUnauthorized due to the various hacks in the tests - overriding DNS, etc :-) - been there, done that ❤️

Probably we'd want a separate set of tests, that don't set global env vars, override DNS, etc.

Copy link
Owner

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

This solution works great! Just a nit.
Can you also add some tests?

@pmuellr
Copy link
Author

pmuellr commented Jul 14, 2022

Ya, I have to test it in our product as well, see if it will do what we want, but I'd be adding more tests.

As mentioned ^^^, if we want to actually test rejectUnauthorized and checkServerIdentity(), that's likely not possible with the current tests with the overriding of env vars / DNS. Note that we don't HAVE to specifically test those properties (though they are the ones I'll be using), could test something else that wouldn't be impacted by the current overrides. Not seeing anything obvious though. I suspect rejectUnauthorized will be the most popular usage of this feature, so it does feel like we should test that.

If we wanted to create tests not using the current overrides, feels like we should create a new directory for the existing tests under /test, and then create another directory under /test for these new tests that don't use overrides, and then the npm tasks would run both set of tests independently. I think the NODE_EXTRA_CA_CERTS is the biggest problem, since I think it can ONLY be set from the CLI and can't be modified within the node runtime.

Alternatively, we could create some new keys/certs to just test those with the new proxy settings, not add those to the PEM file we're using but use them in the new tests, and also provide a way to turn the DNS override off (currently hard-coded "on" for the tests).

@delvedor
Copy link
Owner

I'm fine with two subfolders inside /test :)

@delvedor
Copy link
Owner

Closed in #72.

@delvedor delvedor closed this Oct 29, 2022
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