Add cross-platform support for SSL_CERT_FILE #32
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will be used shortly to load certs from an env var, on all platforms. This also simplifies the logic of unix's load_native_certs. Previously this had leftover parts of an implementation for partial results, but it was no longer used - first_error always took precendence.
djc
reviewed
Oct 13, 2021
|
Also if you can make sure all files end with a trailing newline, that would be nice. |
|
Newlines fixed |
djc
approved these changes
Oct 25, 2021
|
Thanks! |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #16
This includes a commit to extract file loading from the unix configuration. I've refactored and simplify that, which seemed to have some leftover complexity logic from partial loading, that was removed by #26. I've also updated the README, which referenced the old API that was replaced by #26.
The final commit is the actual change. I've had to add serial_tests on the tests here, because the env-var setting is global, so it breaks things when running tests in parallel. The tests use badssl.com (self-signed.badssl.com specifically) which is a site specifically for testing SSL client behaviour with various certificate scenarios.
I have only implemented SSL_CERT_FILE here, not SSL_CERT_DIR. There was some discussion about this at the start of #16, I think the conclusion was that it's much more complicated (see #9) and rarely worthwhile. Happy to discuss, but imo we should leave that as a future extension if/when somebody specifically requests it.
One very debatable point: this returns an error if the variable is set but points to a non-existent/unreadable/unparseable certificate file (rather than falling back to the existing platform logic). Is that the right decision? It makes it much easier to spot issues early if SSL_CERT_FILE was set incorrectly, but I imagine it might cause unexpected problems in some cases. What do you think?
If we're happy with that there's probably an argument for providing more specific error messages here at least, I can look into that if we're agreed that failing is the right choice.