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

Custom cert chain option for underlying request #148

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

Conversation

robfromca
Copy link

Add an additional option so you can pass certificates through
to the underlying request, for connecting to hosts with non-standard
or custom certificate chains.

Add an additional option so you can pass certificates through
to the underlying request, for connecting to hosts with non-standard
or custom certificate chains.
@gstroup
Copy link
Collaborator

gstroup commented Nov 8, 2016

Hey, how's it going? This change looks logical to me, but it's breaking some tests. Do you have time to fix tests, and maybe add one for your new case? I will try to take a look soon and merge this in. Thanks!

@gstroup
Copy link
Collaborator

gstroup commented Nov 8, 2016

Just ran the build again, and no failures. Looks like some other tests are flaky.

@monkpow
Copy link
Collaborator

monkpow commented Nov 9, 2016

@robfromca @gstroup I don't see this as a big advantage over

app.use('/', proxy('internalhost.example.com', {
  decorateRequest: function(proxyReq, originalReq) {
    proxyReq.ca =  [caCert, intermediaryCert]
    return proxyReq;
  }
})

and I don't see a specific way to test it -- we could verify that options.ca is populated, but I don't know how to write a test to verify that the certificate chain is functional.

For my money, I'd prefer to add documentation that this is a technique for adding certificate chains than take on additional code.

Happy to discuss more/be further educated ...

@robfromca
Copy link
Author

I'll give your suggestion a shot, that's all I was trying to do and didn't think of trying that. If it works I'll make a PR for the docs.

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.

3 participants