ephraimbuddy commented on code in PR #33108:
URL: https://github.com/apache/airflow/pull/33108#discussion_r1284293100
##########
airflow/providers/imap/CHANGELOG.rst:
##########
@@ -26,6 +26,18 @@
Changelog
---------
+In case of IMAP SSL connection, the default context now uses "default" context
+
+The "default" context is Python's ``default_ssl_context`` instead of
previously used "none". The
+``default_ssl_context`` provides a balance between security and compatibility
but in some cases,
+when certificates are old, self-signed or misconfigured, it might not work.
This can be configured
+by setting "ssl_context" in "imap" configuration of the provider. If it is not
explicitly set,
+it will default to "email", "ssl_context" setting in Airflow.
+
+Setting it to "none" brings back the "none" setting that was used in previous
versions of the provider,
+but it is not recommended due to security reasons ad this setting disables
validation
Review Comment:
```suggestion
but it is not recommended due to security reasons and this setting disables
validation
```
##########
airflow/providers/imap/hooks/imap.py:
##########
@@ -78,16 +79,34 @@ def get_conn(self) -> ImapHook:
return self
def _build_client(self, conn: Connection) -> imaplib.IMAP4_SSL |
imaplib.IMAP4:
- IMAP: type[imaplib.IMAP4_SSL] | type[imaplib.IMAP4]
- if conn.extra_dejson.get("use_ssl", True):
- IMAP = imaplib.IMAP4_SSL
- else:
- IMAP = imaplib.IMAP4
-
- if conn.port:
- mail_client = IMAP(conn.host, conn.port)
+ mail_client: imaplib.IMAP4_SSL | imaplib.IMAP4
+ use_ssl = conn.extra_dejson.get("use_ssl", True)
+ if use_ssl:
+ from airflow.configuration import conf
+
+ ssl_context_string = conf.get("imap", "SSL_CONTEXT", fallback=None)
+ if ssl_context_string is None:
+ ssl_context_string = conf.get("email", "SSL_CONTEXT",
fallback=None)
+ if ssl_context_string is None:
+ ssl_context_string = "default"
+ if ssl_context_string == "default":
+ ssl_context = ssl.create_default_context()
+ elif ssl_context_string == "none":
Review Comment:
```suggestion
elif ssl_context_string.lower() == "none":
```
Should we do it this way?
##########
airflow/providers/imap/CHANGELOG.rst:
##########
@@ -26,6 +26,18 @@
Changelog
---------
+In case of IMAP SSL connection, the default context now uses "default" context
Review Comment:
```suggestion
In case of IMAP SSL connection, the context now uses the "default" context
```
--
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]
