Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

smtplib should support SSL contexts #53055

Closed
pitrou opened this issue May 24, 2010 · 18 comments
Closed

smtplib should support SSL contexts #53055

pitrou opened this issue May 24, 2010 · 18 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented May 24, 2010

BPO 8809
Nosy @terryjreedy, @jcea, @pitrou, @giampaolo, @bitdancer
Files
  • smtp_sslcontext.patch
  • smtp_sslcontext_updated.patch
  • smtp_sslcontext_updated2.patch
  • smtp_sslcontext_updated3.patch
  • smtp_sslcontext_test.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2011-05-18.16:05:43.624>
    created_at = <Date 2010-05-24.16:15:41.699>
    labels = ['type-feature', 'library']
    title = 'smtplib should support SSL contexts'
    updated_at = <Date 2011-05-18.16:17:41.783>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2011-05-18.16:17:41.783>
    actor = 'kasun'
    assignee = 'none'
    closed = True
    closed_date = <Date 2011-05-18.16:05:43.624>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2010-05-24.16:15:41.699>
    creator = 'pitrou'
    dependencies = []
    files = ['21730', '21737', '21781', '21791', '22003']
    hgrepos = []
    issue_num = 8809
    keywords = ['patch']
    message_count = 18.0
    messages = ['106369', '133463', '133474', '134098', '134120', '134121', '134124', '134181', '134443', '134479', '134481', '134528', '135529', '135561', '135981', '136248', '136249', '136250']
    nosy_count = 10.0
    nosy_names = ['terry.reedy', 'jcea', 'janssen', 'pitrou', 'giampaolo.rodola', 'r.david.murray', 'asdfasdfasdfasdfasdfasdfasdf', 'python-dev', 'thomas.scrace', 'kasun']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue8809'
    versions = ['Python 3.3']

    @pitrou
    Copy link
    Member Author

    pitrou commented May 24, 2010

    3.2 introduces SSL contexts, which allow bundling SSL configuration options, certificates and private keys into a single (potentially long-lived) structure.
    http://docs.python.org/dev/py3k/library/ssl.html#ssl.SSLContext

    The SMTP_SSL constructor should allow passing an SSL context object instead of a key/cert pair.

    @pitrou pitrou added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 24, 2010
    @pitrou pitrou changed the title smptlib should support SSL contexts smtplib should support SSL contexts Apr 10, 2011
    @thomasscrace
    Copy link
    Mannequin

    thomasscrace mannequin commented Apr 10, 2011

    Is anybody working on this issue? If not, I think it looks like it might be a nice one for me to tackle. I'll go ahead unless there are any objections.

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 10, 2011

    Is anybody working on this issue? If not, I think it looks like it
    might be a nice one for me to tackle. I'll go ahead unless there are
    any objections.

    Nobody is working on it AFAIK. Feel free to give it a try :)

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented Apr 19, 2011

    I did a patch that allows smtplib.SMTP_SSL constructor to accept an optional SSLContext

    @terryjreedy
    Copy link
    Member

    This sentence is awkward, at best.
    "Alternative to a private key and a certificate key an optional SSLContext could be specified."

    I suggest something like: "SSLcontext, also optional, is an alternative to keyfile and certfile; if it is specified, keyfile and certfile must be None."

    @asdfasdfasdfasdfasdfasdfasdf
    Copy link
    Mannequin

    It should also explain how the context can be used.
    An example of how to use it to establish a 'secured' connection would be a nice to have.

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented Apr 20, 2011

    I'm submitting a new patch with changes suggested by Terry. But I feel an example of how to use a SSLContext inside the Docstring of SMTP_SSL constructor is unnecessary since no smtp specific things are done to the passed SSLContext. Any ideas about this suggestion?

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 20, 2011

    Thanks for the patch. A couple of comments:

    • it would be nice to have some tests, but of course there are no tests for SMTP_SSL currently. If you are motivated, perhaps you could investigate into adding some.
    • there are some tab characters in your patch. Please only use spaces for indentation (see PEP-8 if you have any doubts); make sure your editor is configured for that :)
    • starttls() should probably get a context parameter too. You can take a look at imaplib for an example.
    • you need to update the API docs in Doc/library/smtplib.rst

    I don't think an example of using a context is necessary. By using proper ReST markup a link will be generated to the SSLContext documentation, and that's probably enough for people to understand.

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented Apr 26, 2011

    I did another patch based on feedback given. A test for SMTP_SSL wasn't added; will look into it later. Tab character that were present are removed. Also SSLContext support was added to starttls() but it is slightly different from the starttls() implementation of imaplib. Further feedback would be much appreciated.

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 26, 2011

    For the record, bpo-11927 reminds me that test_smtpnet actually has a trivial test for SMTP-over-SSL.

    @pitrou
    Copy link
    Member Author

    pitrou commented Apr 26, 2011

    So, thanks for the new patch. I tested it with my ISP's server and it works fine!

    Two remaining issues though:

    • in the SMTP_SSL constructor, you must add the "context" argument at the end of the argument list (after "timeout"), otherwise someone passing "timeout" as a positional argument will find their code breaking
    • in the doc, you need a "versionchanged" tag for the "context" argument (in both instances)

    Thank you again.

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented Apr 27, 2011

    Thanks for the quick review. I'm submitting a new patch with changes suggested.

    @pitrou
    Copy link
    Member Author

    pitrou commented May 8, 2011

    Thank you, the patch looks good. A simple test could probably be added to test_smtpnet, along the lines of the existing tests, do you want to tackle that?

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented May 9, 2011

    Yes, I would like to have a try at it

    @kasun
    Copy link
    Mannequin

    kasun mannequin commented May 14, 2011

    I added a test to smtpnet and submitting a separate patch for it as my test_smtpnet.py file was old and could have had conflicts.

    I didn't use keys and certificates for the SSLContext as those would have to be shipped with the source. Looking forward for reviews and suggestions.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 18, 2011

    New changeset 6737de76487b by Antoine Pitrou in branch 'default':
    Issue bpo-8809: The SMTP_SSL constructor and SMTP.starttls() now support
    http://hg.python.org/cpython/rev/6737de76487b

    @pitrou
    Copy link
    Member Author

    pitrou commented May 18, 2011

    Thank you Kasun. I did a couple of minor style fixes (whitespace at end of line, and spaces around the '=' assignment sign). I also added a test for starttls(), since it turns out gmail.com supports it on port 25.

    @pitrou pitrou closed this as completed May 18, 2011
    @kasun
    Copy link
    Mannequin

    kasun mannequin commented May 18, 2011

    Thanks Antoine

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants