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

Iterate over all certificates in a trusted cert BIO, not just the first #522

Merged
merged 1 commit into from
Mar 28, 2022

Conversation

kw217
Copy link
Contributor

@kw217 kw217 commented Jan 10, 2022

Previously the code which loaded a trusted certificate from file only assumed that there was a single certificate in that file, meaning that using a certificate bundle for certificate verification would not work.

This fix allows the driver to read multiple trusted certificates out of a BIO and provision them in the trusted certificate store.

This code was already reviewed in #493 but that PR contained other fixes as well and has not yet been merged. Instead, this PR pulls that out separately. It is also now based on master (2.16.2).

Previously the code which loaded a trusted certificate from file only
assumed that there was a single certificate in that file, meaning that
using a certificate bundle for certificate verification would not work.

This fix allows the driver to read multiple trusted certificates out
of a BIO and provision them in the trusted certificate store.
@kw217
Copy link
Contributor Author

kw217 commented Jan 12, 2022

Looks like it's passing appveyor now - apologies, my first push had an issue that was caught by the UTs (turns out you have to call ERR_get_error() even if you don't use it, otherwise the next SSL function fails mysteriously).

This should now be ready to review.

@kw217
Copy link
Contributor Author

kw217 commented Jan 17, 2022

@mpenick looks like there's a problem with the Jenkins integration - I get a name resolution error when I click "Details" on the failure above.

@absurdfarce
Copy link
Collaborator

@kw217 Thanks for the contribution! And my sincere apologies for the lengthy delay in getting to this review.

I very much like the idea of pulling these changes out of the previous PR and managing them here. They're very self-contained (which this PR makes clear) and the implementation here is clean so I'm happy to merge this as it stands.

Regarding the test failures: the Jenkins build points to an internal DataStax Jenkins server which isn't available publicly. The good news is that the failures noted on that Jenkins run are due to a very minor formatting issue that only seems to occur on one of our test platforms. I'll include a fix for that issue in a short follow-up commit.

With that in mind, I believe we have what we need to merge this now. Thanks again for the PR!

@absurdfarce absurdfarce merged commit 88aa754 into datastax:master Mar 28, 2022
@absurdfarce absurdfarce mentioned this pull request Mar 28, 2022
@absurdfarce
Copy link
Collaborator

For sake of completeness the follow-up PR is here.

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