-
Notifications
You must be signed in to change notification settings - Fork 848
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
Use vendored CA bundle for all requests #422
Conversation
I think what you have here makes sense. There are some pretty complex tradeoffs that are a little bit hazy — for example, if a root CA were to be revoked, is it more likely that we'd get the update out to our libraries first or that a user's system bundle would get updated first? The answer is very unclear because we have so little control. Even if we automated system that would rebundle certificates periodically, people wouldn't necessarily upgrade. All else being equal, I'd probably lean toward not bundling because at least some systems will have good package managers to help keep the certificate bundle more up-to-date than we likely would (e.g., Archlinux), and users with a bundle so outdated that they can't connect to Stripe could be instructed on how to download and set a fresh one. That said, given that we're already bundling in Ruby, I think it makes sense to try and built out a consistent approach, so I'd say go for it. I like that we give security-minded users who want to specify their own constrained bundle a way to do so through configuration.
I can't personally think of a way that this could break people, but yeah, it's something we should keep in mind. |
Alright, let's go for it. Thanks @brandur-stripe ! |
Out of curiosity, did you ever give any thought to checking if the user has specified a CA file via php.ini, and use that? Right now I'm using this code: It would be cool if you detected that ini setting in this library and use that by default instead. If you decide to go for that, please note there's actually 2 ini settings you should check. The php source shows how this works. |
@EatonZ Interesting! I didn't know about those php.ini settings. I think we should still use the vendored CA bundle by default though, as we know it to be compatible with Stripe's API. We could add a helper method to make it slightly easier to use the CA bundle specified in php.ini settings, but it's fairly trivial to do anyway so I don't think that would provide much value. |
@ob-stripe We encountered some problems because of the usage of Any chances this will be changed in a future version, or should we use the |
@tobiastom I don't have much experience with Phar archives. The CA bundle is not used by PHP itself, but rather by libcurl (via PHP's mod_curl), so I'm not sure that a Phar "URL" like Do you have any suggestions or recommendations for how this should be handled? |
@ob-stripe After writing it I realised myself that the path inside the phar would not work. We now configure the global CA bundle before we use any of the Stripe functionality. This means it's not updated with the Stripe dependency, but with the one from the operating system. It seems to work quite well for us and I don't think there is anything to improve this. Maybe this reference helps someone else in the future. :) Thanks for your help @ob-stripe! |
r? @brandur-stripe
cc @stripe/api-libraries
Currently, the library sends requests using the environment's default CA bundle. If the request fails due to an SSL verification error, the library will automatically retry the request using the vendored bundle.
This PR changes the behavior to use the vendored bundle by default for all requests, and adds an option to configure the path to a custom bundle too. This makes the PHP implementation consistent with the Ruby implementation.
While this should not have any user impact, I'm tagging it as WIP because I'm slightly nervous about this change given the number of TLS issues we've observed in the past. Part of me is definitely like "if it ain't broke don't fix it" 😐
For context, I'm working on adding request retries as requested in #394 and wanted to simplify the logic in order to avoid having two different retry mechanisms (one for cert verification issues and one for other connection/409 issues).