CVE-2023-39441: Allows to choose SSL context for SMTP provider by potiuk · Pull Request #33075 · apache/airflow

Conversation

This change add two options to choose from when SSL SMTP connection is created:

• default - for balance between compatibility and security
• none - in case compatibility with existing infrastructure is preferred

The fallback is:

• The Airflow "email", “ssl_context”
• “default”

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Also for @hussein-awala -> I am not sure if this approach is best and how it will work with #30531 - I just figured that we have to use “smtp_provider” section rather than “smtp” from the core - and I think we should follow similar approach like here - where “smtp” will go to “pre_2_7_0” defaults, but the provider will use the “smtp_provider” section and fall back to the “smtp” one, similarly as I am falling back now to “email” / “ssl_context” - would love to hear from you on that one.

As explained above, I am trying to make chain of defaults this way and added unit tests covering the behaviour

This change add two options to choose from when SSL SMTP connection is created:

* default - for balance between compatibility and security * none - in case compatibility with existing infrastructure is preferred

The fallback is:

* The Airflow "email", “ssl_context” * “default”

Co-authored-by: Ephraim Anierobi [email protected]

@hussein-awala -> would you like to comment on that one, re #30531 - or you are ok for now. I’d love to merge that one and implement IMAP change as well so that we can release new providers today :)

Let me merge that one now, I think we can always change it in the future.

potiuk deleted the use-default-context-for-ssl-for-smtp-provider branch

August 4, 2023 10:30

I’m late to the party.

I just figured that we have to use “smtp_provider” section rather than “smtp” from the core - and I think we should follow similar approach like here - where “smtp” will go to “pre_2_7_0” defaults, but the provider will use the “smtp_provider” section and fall back to the “smtp” one, similarly as I am falling back now to “email” / “ssl_context” - would love to hear from you on that one.

For smtp_provider, I think it would be better to use the connection extras to configure ssl_context (as we do with the other configurations), instead of using Airlfow config (or even provider config).
https://github.com/potiuk/airflow/blob/ca2f3013bcb123c4b3973a5b85de77094bf2c459/airflow/providers/smtp/hooks/smtp.py#L321-L355

Since we cannot use the operator/hook without a connection, IMO it’s better if we provide a single way to configure them.

WDYT? could we move this conf to the connection before releasing the provider?

Discussion in Slack follows

ephraimbuddy pushed a commit that referenced this pull request

Aug 8, 2023

* Allows to choose SSL context for SMTP provider

This change add two options to choose from when SSL SMTP connection is created:

* default - for balance between compatibility and security * none - in case compatibility with existing infrastructure is preferred

The fallback is:

* The Airflow "email", “ssl_context” * “default”

* Update airflow/providers/smtp/CHANGELOG.rst

Co-authored-by: Ephraim Anierobi [email protected] (cherry picked from commit e20325d)

Reviewers

ephraimbuddy ephraimbuddy approved these changes

pankajkoti pankajkoti approved these changes

utkarsharma2 utkarsharma2 approved these changes

hussein-awala Awaiting requested review from hussein-awala hussein-awala is a code owner

eladkal Awaiting requested review from eladkal

pierrejeambrun Awaiting requested review from pierrejeambrun