Conversation
cloutierMat
added
semver: patch
cloutierMat
added
the
review: merge when ready
simonrw
left a comment
simonrw
left a comment
There was a problem hiding this comment.
I am ok with this change given the current api but this envar is not one of ours - it's from requests itself. It's a shame we are hard coding these variables rather than reading from the standard environment. Ho hum...
Let's just address my suggestion otherwise
| if ca_cert := os.getenv("REQUESTS_CA_BUNDLE"): | ||
| LOG.debug("Creating External AWS Client with REQUESTS_CA_BUNDLE=%s", ca_cert) | ||
|
|
||
| super().__init__(use_ssl=True, verify=ca_cert or True, session=session, config=config) |
There was a problem hiding this comment.
Should we use USE_SSL here for the ssl flag?
There was a problem hiding this comment.
It's a shame we are hard coding these variables rather than reading from the standard environment. Ho hum...
I agree this isn't the cleanest, but this is what is currently used and documented in LocalStack. I thought of using AWS_CA_BUNDLE instead, but it seemed quite repetitive with what is already implemented for ca bundles.
Should we use USE_SSL here for the ssl flag?
Good point. Is there any security concern for the user to do so? I guess they are in control of setting the env, so probably not.
2 participants
Motivation
This PR enforces custom CA Certificates to the
ExternalBypassDnsClientFactory. This change will enable our users using proxy with ssl termination to register their own CA bundles.Changes
The CA bundle provided with
REQUESTS_CA_BUNDLEwill now be used to configure the connections of theExternalBypassDnsClientFactoryTests
Related
fixes UNC-137
upstream test full-run: 19842076873