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

Added the ability for users to specify a CA bundle for HTTPS requests #483

Merged
merged 4 commits into from
Sep 26, 2019

Conversation

barrybarrette
Copy link
Contributor

@barrybarrette barrybarrette commented Sep 25, 2019

Proposal to Issue #481

Right now there is no way for a user to specify a CA bundle to replace the default one that request ships with.

According to the request docs you can specify it in the options object passed to request.

This change offers a simple solution by setting a path to your CA bundle as an environment variable:
TWILIO_CA_BUNDLE=/path/to/cert.pem

Test cases have been included to ensure that the specified CA bundle is used, and that the original behavior of not including it in options remains unchanged if it is not specified.

Contributing to Twilio

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

One request to update the docs. But overall LGTM. Nice work! (assuming you tested this locally and it resolves the issue you're having).

lib/base/RequestClient.js Show resolved Hide resolved
@barrybarrette
Copy link
Contributor Author

One request to update the docs

Can you point me to a specific place in the docs you'd like me to update? I'm not too familiar with this module yet.

assuming you tested this locally and it resolves the issue you're having

Yes it works like a charm locally 😄

@childish-sambino
Copy link
Contributor

@whitebarry I think sticking a block in/under here is fine: https://github.com/twilio/twilio-node#sample-usage

@barrybarrette
Copy link
Contributor Author

Ok added some info to the example file and also the README block for environment variables. I'm not exactly clear on the usage of the first 2 you mentioned so I left the descriptions for those as TODOs, hope that's OK.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

@whitebarry Updated the README.

One thing that did cross my mind: since the CA payload is added to every API request, it would be better to cache the file contents on the first load so subsequent requests don't need to read from disk. Not a blocker for this PR; just an optimization.

@barrybarrette
Copy link
Contributor Author

since the CA payload is added to every API request, it would be better to cache the file contents on the first load

I couldn't agree more. My latest change addresses this with a supporting test case.

Copy link
Contributor

@childish-sambino childish-sambino left a comment

Choose a reason for hiding this comment

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

👍

@childish-sambino childish-sambino merged commit 45440e8 into twilio:master Sep 26, 2019
@childish-sambino
Copy link
Contributor

FYI, next planned release is on 2019-10-02.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: community enhancement feature request not on Twilio's roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants