gh-127712: Fix `secure` argument of `logging.handlers.SMTPHandler` #127726

s-hamann · 2024-12-07T20:03:12Z

Python 3.12 removed support for the keyfile and certfile parameters in smtplib.SMTP.starttls(), requiring a ssl.SSLContext instead. SMTPHandler now creates a context from the secure tuple and passes that to starttls.

This PR basically restores SMTPHandler functionality as it was in Python 3.11.
It is to note that, contrary to the documentation, using a single-value tuple for secure does not work. Instead, ssl._create_unverified_context raises ValueError: certfile must be specified.
However, this also happened with Python 3.11, so I did not change it. The only difference is, with Python 3.11 the target SMTP server needed to actually support STARTTLS to encounter the exception. Now it is raised before checking the server for STARTTLS support.
This is simply because the ssl.SSLContext is created earlier.

Off topic: Personally, I believe the secure argument to SMTPHandler is confusing and somewhat limiting. I suggest deprecating it in favour of something more versatile. Allow passing a context, maybe?

Issue: logging.handlers.SMTPHandler: secure argument is broken in Python 3.12+ #127712

ghost · 2024-12-07T20:03:15Z

All commit authors signed the Contributor License Agreement.

ZeroIntensity

Thanks for contributing! Happy to see new people 😄
(FWIW, you don't need to force-push, everything gets squash merged.)

In general, the fix itself looks good. I just have a few nitpicks related to formatting and performance.

Lib/logging/handlers.py

Misc/NEWS.d/next/Library/2024-12-07-20-33-43.gh-issue-127712.Uzsij4.rst

Python 3.12 removed support for the `keyfile` and `certfile` parameters in `smtplib.SMTP.starttls()`, requiring a `ssl.SSLContext` instead. `SMTPHandler` now creates a context from the `secure` tuple and passes that to `starttls`.

picnixz

I've never used this interface but it looks fine. Can we however have additional tests for this please? the regression should have been detected but apparently not.

ZeroIntensity

The fix makes sense to me. (We need tests, but I'll just wait for @picnixz's approval before I ping a core dev.)

s-hamann · 2024-12-10T16:19:31Z

Can we however have additional tests for this please? the regression should have been detected but apparently not.

Does that mean me? I'm all for tests, but I'm not familiar with Python's test infrastructure and I believe the mock SMTP server does not currently handle STARTTLS. So, I'm at at bit of a loss on how to test this properly.

ZeroIntensity · 2024-12-10T19:34:30Z

You'll probably want to add the test to SMTPHandlerTest. You can find most of the things that you need to know about testing in the devguide.

s-hamann · 2024-12-12T20:03:54Z

Thanks for the hints. The test case itself seems rather straightforward. Making TestSMTPServer support STARTTLS looks more complex. I'll try to implement that, but I don't think I'll get around to it this year. If anyone else wants to tackle this, I'd be grateful. This is somewhat more involved than the quick and easy bug fix I had in mind ;)

ZeroIntensity · 2024-12-13T13:38:10Z

Ah yeah, that's why there wasn't a test before. I'm OK with merging this as-is, and then letting someone with a bit more knowledge of the codebase deal with getting support for SSL in the tests. @picnixz WDYT?

picnixz · 2024-12-13T17:55:59Z

I prefer having tests and this PR close to each other in the commit history. I've never used that parameter so I'll have a look tomorrow at the internals just to check and I'll tell you my conclusions (we are backporting this bugfix to a stable version so I'd prefer having tests just in case, but if they're too hard to write, I'll see if I can write them myself).

vsajip

I think writing a full-blown test (updating the TestSMTPServer to support starttls meaningfully) will be too much work for the benefit that it confers. ISTM this change just moves converting a ('private_key_file, cert_file) tuple to an SSL context into the handler, rather than it being done in the starttls` method in Python 3.11 and earlier. This is the code in 3.11:

cpython/Lib/smtplib.py

Lines 787 to 789 in 45db419

    
           if context is None: 
        
               context = ssl._create_stdlib_context(certfile=certfile, 
        
                                                    keyfile=keyfile)

So. I'm OK to merge this as is, without tests.

miss-islington-app · 2025-02-10T12:34:36Z

Thanks @s-hamann for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

…er` (pythonGH-127726) pythonGH-127712: Fix `secure` argument of `logging.handlers.SMTPHandler` Python 3.12 removed support for the `keyfile` and `certfile` parameters in `smtplib.SMTP.starttls()`, requiring a `ssl.SSLContext` instead. `SMTPHandler` now creates a context from the `secure` tuple and passes that to `starttls`. (cherry picked from commit d7672e5) Co-authored-by: s-hamann <10639154+s-hamann@users.noreply.github.com>

bedevere-app · 2025-02-10T12:34:56Z

GH-129955 is a backport of this pull request to the 3.13 branch.

…er` (pythonGH-127726) pythonGH-127712: Fix `secure` argument of `logging.handlers.SMTPHandler` Python 3.12 removed support for the `keyfile` and `certfile` parameters in `smtplib.SMTP.starttls()`, requiring a `ssl.SSLContext` instead. `SMTPHandler` now creates a context from the `secure` tuple and passes that to `starttls`. (cherry picked from commit d7672e5) Co-authored-by: s-hamann <10639154+s-hamann@users.noreply.github.com>

bedevere-app · 2025-02-10T12:35:09Z

GH-129956 is a backport of this pull request to the 3.12 branch.

…ler` (GH-127726) (GH-129956) (cherry picked from commit d7672e5)

…ler` (GH-127726) (GH-129955) (cherry picked from commit d7672e5)

s-hamann requested a review from vsajip as a code owner December 7, 2024 20:03

bedevere-app bot mentioned this pull request Dec 7, 2024

logging.handlers.SMTPHandler: secure argument is broken in Python 3.12+ #127712

Closed

bedevere-app bot added the awaiting review label Dec 7, 2024

ZeroIntensity reviewed Dec 10, 2024

View reviewed changes

Lib/logging/handlers.py Outdated Show resolved Hide resolved

Lib/logging/handlers.py Show resolved Hide resolved

Lib/logging/handlers.py Show resolved Hide resolved

Misc/NEWS.d/next/Library/2024-12-07-20-33-43.gh-issue-127712.Uzsij4.rst Outdated Show resolved Hide resolved

ZeroIntensity added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Dec 10, 2024

ZeroIntensity requested a review from picnixz December 10, 2024 03:11

picnixz reviewed Dec 10, 2024

View reviewed changes

ZeroIntensity approved these changes Dec 10, 2024

View reviewed changes

bedevere-app bot added awaiting core review and removed awaiting review labels Dec 10, 2024

vsajip approved these changes Feb 10, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Feb 10, 2025

vsajip merged commit d7672e5 into python:main Feb 10, 2025
46 checks passed

bedevere-app bot removed the awaiting merge label Feb 10, 2025

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Feb 10, 2025

bedevere-app bot removed the needs backport to 3.12 only security fixes label Feb 10, 2025

vsajip pushed a commit that referenced this pull request Feb 10, 2025

[3.12] gh-127712: Fix secure argument of `logging.handlers.SMTPHand…

2db4a9d

…ler` (GH-127726) (GH-129956) (cherry picked from commit d7672e5)

vsajip pushed a commit that referenced this pull request Feb 10, 2025

[3.13] gh-127712: Fix secure argument of `logging.handlers.SMTPHand…

fd665d5

…ler` (GH-127726) (GH-129955) (cherry picked from commit d7672e5)

	if context is None:
	context = ssl._create_stdlib_context(certfile=certfile,
	keyfile=keyfile)

Uh oh!

gh-127712: Fix secure argument of logging.handlers.SMTPHandler #127726

gh-127712: Fix secure argument of logging.handlers.SMTPHandler #127726

Uh oh!

Conversation

s-hamann commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 7, 2024 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZeroIntensity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-hamann commented Dec 10, 2024

Uh oh!

ZeroIntensity commented Dec 10, 2024

Uh oh!

s-hamann commented Dec 12, 2024

Uh oh!

ZeroIntensity commented Dec 13, 2024

Uh oh!

picnixz commented Dec 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsajip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

miss-islington-app bot commented Feb 10, 2025

Uh oh!

bedevere-app bot commented Feb 10, 2025

Uh oh!

bedevere-app bot commented Feb 10, 2025

Uh oh!

Uh oh!

gh-127712: Fix `secure` argument of `logging.handlers.SMTPHandler` #127726

gh-127712: Fix `secure` argument of `logging.handlers.SMTPHandler` #127726

s-hamann commented Dec 7, 2024 •

edited

Loading

ghost commented Dec 7, 2024 •

edited by ghost

Loading

ZeroIntensity left a comment •

edited

Loading

ZeroIntensity left a comment •

edited

Loading

picnixz commented Dec 13, 2024 •

edited

Loading