NO-SNOW: CRL cherry picks to AIO #2624
Conversation
| logger.debug( | ||
| "The certificate revocation check was successful. No additional checks will be performed." | ||
| ) | ||
|
|
There was a problem hiding this comment.
CRL glue code in AIO
sfc-gh-turbaszek
force-pushed
the
turbaszek-crl-cherry-picks
branch
5 times, most recently
from
1c99adf to
f692c62
Compare
|
|
||
| def validate_crl(self, protocol: ResponseHandler, req: ClientRequest): | ||
| # Resolve CA file path from environment variables or use certifi default | ||
| cafile_for_ctx = resolve_cafile({"ca_certs": certifi.where()}) |
There was a problem hiding this comment.
Will this cover all env vars being used to pass certs location?
In requests.request we have this parameter passed later to conn.ca_certs if specified.
verify = (
os.environ.get("REQUESTS_CA_BUNDLE")
or os.environ.get("CURL_CA_BUNDLE")
or verify
)
I see that we use resolve_cafile which parses env vars, but should we support CURL_CA_BUNDLE as well there? Also do we maintain the same hierarchy - is certify.where used when no env vars are provided or other way around?
There was a problem hiding this comment.
Should we add os.environ.get("CURL_CA_BUNDLE") to resolve_cafile implementation to keep the logic in single place?
There was a problem hiding this comment.
Thats one thing - add in implementation. Having tests for passing certs in such way maybe a JIRA to be created - since we support it by our code but don't test it.
Also the mentioned hierarchy should be tested imo - if certify.where() overrides the env vars or is it the other way around. I have concerns this may differ between async and sync code.
There was a problem hiding this comment.
The async code tries to duplicate sync version:
# Ensure CA bundle default if not provided
if not params.get("ca_certs"):
params["ca_certs"] = certifi.where()
cafile_for_ctx = resolve_cafile(params)
Created SNOW-2681235 for CURL_CA_BUNDLE support. For now, I would suggest to keep this out of aio equation.
There was a problem hiding this comment.
Sure, fyi below is how I was able to understand the urllib3 flow for ca_certs - since we do not have adapters for aiohttp those steps we may want to replicate ourselves.
Specifically the fact, that 2 env vars overwrite the certify.where result.
But if we want to do that in the follow-up Jira I am okay with that.
| Priority | Source | File:Line | Usage |
|---|---|---|---|
| 1 | session.get(verify="/path") | sessions.py:777 | Request-level override |
| 2 | session.verify = "/path" | sessions.py:426+777 | Session-level default |
| 3 | REQUESTS_CA_BUNDLE env | sessions.py:769 | Only if verify=True/None |
| 4 | CURL_CA_BUNDLE env | sessions.py:770 | Only if verify=True/None |
| 5 | certifi.where() no 1 | adapters.py:295 | Via DEFAULT_CA_BUNDLE_PATH |
| 6 | certifi.where() no 2 | ssl_wrap_socket.py:176 | Direct call fallback |
| 7 | SSL_CERT_FILE env | ssl_wrap_socket.py:62 | Via resolve_cafile() |
|
Fyi: for our tests most SSL checks will happen in synch init_test_schema fixture - which is autoused for the whole testing session. Just so we are aware we need really good tests for that as async code isn't using it under the hood in tests. |
| session_manager = Mock() | ||
| session_manager.clone = lambda max_retries: "session_manager" | ||
|
|
||
| return Auth.base_auth_data( |
There was a problem hiding this comment.
We may be lacking import of this Auth class
sfc-gh-turbaszek
force-pushed
the
turbaszek-crl-cherry-picks
branch
from
4da8202 to
df35306
Compare
sfc-gh-fpawlowski
left a comment
sfc-gh-fpawlowski
left a comment
There was a problem hiding this comment.
Some nits left in comments, but seems good already to me
sfc-gh-turbaszek
force-pushed
the
turbaszek-crl-cherry-picks
branch
from
14d3c6d to
2770866
Compare
Co-authored-by: James Kasten <james.kasten@snowflake.com>
Co-authored-by: Tomasz Urbaszek <tomasz.urbaszek@snowflake.com>
sfc-gh-turbaszek
force-pushed
the
turbaszek-crl-cherry-picks
branch
from
b7a0b1b to
f4622e5
Compare
No description provided.