Check OpenSSL environment variables before defaulting to certifi

[aiudirog]

Description

Updated the urllib3 and aiohttp backends to check OpenSSL environment variables SSL_CERT_FILE/DIR before defaulting to certify's CA bundle to allow environmental configuration of custom SSL certificates.

Issues Resolved

Closes #111

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[aiudirog]

I submitted the simplest implementation I could write but I do see several potential improvements depending on what people think:

• Delaying environment variable checks to when the OpenSearch client is initialized
  • This would allow for test cases which are currently tough to do with the checks at the module level
• Respecting other environment variables like REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE for better compatibility
  • This could quickly get out of hand though
• Moving the CA_CERTS declaration to a shared module (maybe opensearchpy.connection.base?) to DRY up the code
• Adding this to the requests backend as well to improve consistency

[harshavamsi]

@wbeckler @VachaShah could you approve running the workflows?

[axeoman]

Thanks a lot for the contribution! 👍

Moving the CA_CERTS declaration to a shared module (maybe opensearchpy.connection.base?) to DRY up the code

Yep, definitely would be good to have it in a single place.

Adding this to the requests backend as well to improve consistency

Yep, good idea too.

Respecting other environment variables like REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE for better compatibility
This could quickly get out of hand though

I would not go there until there are real issues reported. Are they?

Delaying environment variable checks to when the OpenSearch client is initialized
This would allow for test cases which are currently tough to do with the checks at the module level

Could you please elaborate? What tests do you have in mind?

[aiudirog]

Yep, definitely would be good to have it in a single place.

Sounds good. Is putting it in the connection base module good enough or would you like it in a distinct/different module? (Distinct seems overkill to me, but I could see the benefit)

I would not go there until there are real issues reported. Are they?

Not that I know of. The only potential issue I can think of would be that enabling this for the requests backend would override requests' default behavior by forcing certifi even when the CA_BUNDLE variables are set. Probably not an issue, since using requests should really be just an implementation detail, but it's probably something to track as a definite choice.

Could you please elaborate? What tests do you have in mind?

Just basic sanity tests to ensure the environment variables are properly read and the fallback to certify still occurs in their absence. Not strictly necessary, but nice to have.

[dblock]

LGTM. Add to CHANGELOG and USER_GUIDE?

[aiudirog]

@dblock Sorry for the delay, I've updated the docs. Please let me know if there is anything you'd like me to change.

[dblock]

LGTM, I think there's an opportunity to add a test here too

[aiudirog]

@dblock I added some tests to make sure each Connection implementation correctly handles the CA_CERTS constant, but I'm not sure how to test that SSL_CERT_FILE/DIR env vars are loaded properly without changing the implementation to delay loading the CA_CERTS until Connection instance initialization or adding much more complicated tests that either reload the related modules/start subprocesses. What's your opinion?

[dblock]

@dblock I added some tests to make sure each Connection implementation correctly handles the CA_CERTS constant, but I'm not sure how to test that SSL_CERT_FILE/DIR env vars are loaded properly without changing the implementation to delay loading the CA_CERTS until Connection instance initialization or adding much more complicated tests that either reload the related modules/start subprocesses. What's your opinion?

If you move the implementation of CA_CERTS = into a method, then you can test that method by calling it again and having all the right mocks. But I also think that we're trying to be too perfect and I am A-OK with this PR as is, it's great work!

[aiudirog]

@dblock I had the same thought originally, so I went ahead and implemented it with tests. Let me know what you think.

[dblock]

@dblock I had the same thought originally, so I went ahead and implemented it with tests. Let me know what you think.

Perfect. @VachaShah help CR and let's merge this?

[VachaShah]

LGTM, thank you @aiudirog!

[VachaShah]

Workflows are failing for certain python versions for the latest ubuntu-22.04. Related issue on actions/setup-python actions/setup-python#543

[aiudirog]

@VachaShah It doesn't look like those issues are going to be resolved soon, so I pushed a commit to implement the workarounds that were suggested

[dblock]

All good with me.

Let's document SSL configuration in https://github.com/opensearch-project/opensearch-py/blob/main/USER_GUIDE.md too, can be done in a future PR.