kaxil commented on code in PR #67214:
URL: https://github.com/apache/airflow/pull/67214#discussion_r3307009265
##########
task-sdk/src/airflow/sdk/api/client.py:
##########
@@ -1123,7 +1129,7 @@ def __init__(self, *, base_url: str | None, dry_run: bool
= False, token: str, *
else:
kwargs["base_url"] = base_url
# Call via the class to avoid binding lru_cache wires to this
instance.
- kwargs["verify"] =
type(self)._get_ssl_context_cached(certifi.where(), API_SSL_CERT_PATH)
+ kwargs["verify"] =
type(self)._get_ssl_context_cached(API_SSL_CA_FILE_PATH, API_SSL_CERT_PATH)
if API_CLIENT_SSL_CERT or API_CLIENT_SSL_KEY:
Review Comment:
Adding to Pierre's question: the change here is also a regression for the
no-private-CA case. `ssl.create_default_context(cafile=API_SSL_CA_FILE_PATH)`
REPLACES the trust store, it doesn't augment it. So when a user does set
`ssl_ca_file`, the worker drops every public root cert -- I measured 120 certs
from `certifi.where()` vs 1 when `cafile` is the user's CA. Any worker code
that talks to S3/GCS/an OIDC endpoint through the same context loses
verification.
If we end up keeping a worker-side knob (which Pierre's right to question,
given `ssl_cert` already calls `load_verify_locations`), the shape should be
`cafile=certifi.where()` + `ctx.load_verify_locations(user_ca)` -- additive,
not replacement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
