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: add support for arbitrary auth methods #59219

Closed
sandrotosi opened this issue Jun 6, 2012 · 30 comments
Closed

smtplib: add support for arbitrary auth methods #59219

sandrotosi opened this issue Jun 6, 2012 · 30 comments
Assignees
Labels
expert-email stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@sandrotosi
Copy link
Contributor

sandrotosi commented Jun 6, 2012

BPO 15014
Nosy @warsaw, @bitdancer, @sandrotosi, @zvyn
Files
  • smtplib.patch: Implements new member functions for each auth-method in SMTP.
  • smtplibAuthobj.patch: add support for arbitrary auth methods via authenticate(..., authobject)
  • smtplib_auth.patch
  • smtplib_auth_060614.patch
  • smtplib_auth.patch
  • 15014-auth-init-resp.diff
  • 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 = 'https://github.com/warsaw'
    closed_at = <Date 2015-07-09.14:58:32.328>
    created_at = <Date 2012-06-06.08:11:21.135>
    labels = ['type-feature', 'library', 'expert-email']
    title = 'smtplib: add support for arbitrary auth methods'
    updated_at = <Date 2015-07-09.14:58:32.327>
    user = 'https://github.com/sandrotosi'

    bugs.python.org fields:

    activity = <Date 2015-07-09.14:58:32.327>
    actor = 'barry'
    assignee = 'barry'
    closed = True
    closed_date = <Date 2015-07-09.14:58:32.328>
    closer = 'barry'
    components = ['Library (Lib)', 'email']
    creation = <Date 2012-06-06.08:11:21.135>
    creator = 'sandro.tosi'
    dependencies = []
    files = ['34279', '34287', '35392', '35500', '35842', '39882']
    hgrepos = []
    issue_num = 15014
    keywords = ['patch']
    message_count = 30.0
    messages = ['162396', '162407', '212663', '212667', '212668', '212720', '212726', '212735', '212739', '218707', '219315', '219845', '219876', '219886', '222205', '222206', '245648', '245649', '245650', '245652', '245653', '245654', '246419', '246433', '246436', '246437', '246438', '246443', '246473', '246505']
    nosy_count = 7.0
    nosy_names = ['barry', 'r.david.murray', 'jesstess', 'sandro.tosi', 'catalin.iacob', 'python-dev', 'zvyn']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'needs patch'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue15014'
    versions = ['Python 3.5', 'Python 3.6']

    @sandrotosi
    Copy link
    Contributor Author

    sandrotosi commented Jun 6, 2012

    Hello,
    I'm writing some tests from an MTA, and so I'm using smtplib. Sadly the login() method doesn't allow to choose the auth method to use (but it's selected from a static list compared with what's advertized from the MTA) while it would be useful to be able to choose the AUTH method to use.

    @sandrotosi sandrotosi added the stdlib Python modules in the Lib dir label Jun 6, 2012
    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 6, 2012

    Yes, this is a need I also ran into, but hadn't gotten around to submitting a bug for. If we are going to do this, though, I think we should do it right and add the ability to support arbitrary login methods. An example of one way to do this is the imaplib authobj protocol.

    As things are, I wound up implementing XYMCOOKIE login using the even-more-primitive-than-SMTP-cmd-level operation 'do_cmd'.

    @bitdancer bitdancer changed the title smtplib: allow to choose auth method on login() smtplib: add support for arbitrary auth methods Jun 6, 2012
    @bitdancer bitdancer added type-feature A feature request or enhancement expert-email labels Jun 6, 2012
    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Mar 3, 2014

    I implemented one approach to solve this by writing new member functions for each method (see the patch attached). Bonus: It does not change the usage of login() in any way (which uses the new functions internally).
    Another option would be to make those functions private/put them into login() and provide an optional keyarg. Or auth objects, but as far as I can see only 3 methods are relevant so it might be an overkill?

    So that's the easy way to fix it, would be glad if it helps!

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 3, 2014

    There is a lot of repeated code in those methods, which becomes a maintenance burden. That is one reason the imaplib authobj approach seems interesting to me. Also, going the authobj route would mean imaplib and smptlib had a consistent interface for this, and consistency is good where it makes sense.

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 3, 2014

    Oh, sorry. I meant to start out that message by saying, thanks for the patch. Your idea is indeed interesting from the standpoint of not needing to change the rest of the API, but....and then the rest of my message.

    In other words, I appreciate your work and your approach, but I think the reasons I outline argue for a different approach.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Mar 4, 2014

    I looked into imaplib a bit to see how the problem is solved there; what I observed:

    • login() does 'PLAIN' only (and does not use authobj but smtplib would)
    • there exists an extra function login_cram_md5() for 'CRAM-MD5'

    So I guess the right way to do it would be to write an authenticate() method as in imaplib and use it in new member functions of the form login_method. In contrast to imaplib login() could still be used to probe through the three main methods (to avoid breaking backward compatibility).

    I'll try to implement this later today, so if you think it's completely dumb and read this before a patch is applied feel free to give me a quick reply to stop me from wasting my time :)

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 4, 2014

    Ah, I was going on imperfect memory of how imaplib worked without looking it up. And I won't have time to do so today probably.

    Probably we should decide on the exact API before you implement anything.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Mar 4, 2014

    Maybe I am a bit hyperactive, but I already was at it when receiving your message, so I finished it. Anyhow most of what I have done is completely independent of the API chosen. What I did:

    1. I implemented a new authenticate(self, mechanism, authobject) as in imaplib which can be used for all methods
    2. I implemented authobjects for the three most important mechanisms
    3. login() has a keyarg 'preferred_auth' which is a list of auth-methods (strings)

    I think 1. and 2. make sense regardless of how the API should look like in the end, because it's the only way without any code-redundancy. 3 is more a proposal.

    @bitdancer
    Copy link
    Member

    bitdancer commented Mar 4, 2014

    OK, that makes sense to me. I'll take a look at it as soon as I can.

    @bitdancer
    Copy link
    Member

    bitdancer commented May 17, 2014

    OK, I've finally had time to review this, sorry for the delay.

    The impalib mechanism is tailored to how imap works (that's the whole thing about "continuation response". smtplib auth works a bit differently, and your adaptation looks OK to me. The accompanying docstrings are a bit confusing, though, since they copy the imaplib language, which isn't entirely applicable. Instead of talking about a 'continuation response', it should talk about a '334 challenge response', I think.

    Also, both the imaplib and smptlib methods are named after the corresponding command names in the protocol. So for imaplib we have 'authenticate', but for smtplib it should be 'auth'.

    I also think we should expose the supported auth methods as part of the public API.

    I think exposing the preferred auth list is premature, since if we are going to do that we need to have a way to add elements to that list for the users's own auth methods and we don't have that. So let's confine this issue to adding the 'auth' method and using it in the login method.

    The tests should include a new test of calling the auth method directly.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented May 28, 2014

    Done.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 5, 2014

    I made some review comments.

    Also, we need doc updates, including a what's new entry.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 6, 2014

    Ah, you are right, I wasn't looking at the full context of the diff when I made the comment about backward compatibility. So ignore that part :)

    On the other hand, exposing perferred_auth on the class would be a simple API for allowing auth extension (since one really needs to subclass to add an authobject, given that it needs access to self.user and self.password). But, like I said before, that can be a separate issue.

    @zvyn
    Copy link
    Mannequin

    zvyn mannequin commented Jun 6, 2014

    Here comes the patch implementing your suggestions. Changing the API to make adding new auth methods and still using login() would only require to make preferred_auth accessable as you mentioned. Using custom authobjects is possible with this patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 3, 2014

    New changeset 42917d774476 by R David Murray in branch 'default':
    bpo-15014: Add 'auth' command to implement auth mechanisms and use it in login.
    http://hg.python.org/cpython/rev/42917d774476

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 3, 2014

    Thanks, Milan.

    I made some tweaks...mostly documentation and code style (your code style wasn't wrong, I just have some slightly different preferences with regards to line folding). I also eliminated the authobjects test method, since I didn't see that it added anything to duplicate the code from the login method to test them...they get tested via the login methods. Instead I added the loop to the test for the auth method, so that we test that calling the public API with the auth objects that are documented as part of the public API work. That is, if someone were to change login such that the method names changed but login still worked, the auth test method will throw an error because the documented method names changed.

    I'm attaching the final patch here so you can look at the differences via the reitveld patch-diff function, if you want to.

    @warsaw
    Copy link
    Member

    warsaw commented Jun 22, 2015

    I believe this change broke RFC 4954's AUTH command when the optional initial-response is expected. $4 "The AUTH Command" says:

    AUTH mechanism [initial-response]
    ...
    initial-response: An optional initial client response. If present,
    this response MUST be encoded as described in Section 4 of [BASE64]
    or contain a single character "="

    It's possible that some SMTP servers only look for a string like

    AUTH PLAIN <base64-gobbledygook>

    and won't issue a challenge if it's missing. Such an example is found in the lazr.smtptest and used by the testing SMTPd in GNU Mailman. It's possible that the servers are not standards compliant (I haven't completely groked RFC 4422), but still, since this is a backward incompatible change and breaks existing code, there should be some way of making smtplib.login() provide the initial-response, and it probably ought to be the default for backward compatibility.

    @warsaw warsaw reopened this Jun 22, 2015
    @warsaw
    Copy link
    Member

    warsaw commented Jun 22, 2015

    Here's a rough thought for a fix. Some auth_*() methods require a challenge, but some don't, e.g. auth_plain(). Let's allow authobject() to be called with challenge=None. If they allow an initial-response, then they can just return the response bytes. If they return None, then they don't allow an initial-response and so must wait for the challenge (i.e. 334 response code).

    I think that would at least restore backward compatibility for AUTH PLAIN.

    @warsaw
    Copy link
    Member

    warsaw commented Jun 22, 2015

    Also, smtpd is not compatible with auth challenges because found_terminator() doesn't know that the response its getting isn't a command but instead a challenge response. So really we need another bug to track fixes to smtpd.py to handle challenge responses. It makes no sense for smtpd and smtplib not to be compatible.

    (Who wants to reimplement smtpd in asyncio? :)

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 22, 2015

    Sounds reasonable. I'll suggest a slight variation. We change the authobj signature to challenge=None, then the first thing we do in auth is 'initial_response = authobj()'. The return value can be the empty string or a real initial value, and we send the auth command with ' '.join(mechanism, initial_response).strip(). Then we do the challenge part only if we get the 334.

    There's already an open issue for smtpd auth, with at least a preliminary patch, but it got blocked a bit by Martin quoting an RFC...

    @bitdancer
    Copy link
    Member

    bitdancer commented Jun 22, 2015

    The smtpd issue is bpo-21935.

    @warsaw
    Copy link
    Member

    warsaw commented Jun 22, 2015

    On Jun 22, 2015, at 10:00 PM, R. David Murray wrote:

    We change the authobj signature to challenge=None, then the first thing we do
    in auth is 'initial_response = authobj()'. The return value can be the empty
    string or a real initial value, and we send the auth command with '
    '.join(mechanism, initial_response).strip(). Then we do the challenge part
    only if we get the 334.

    Sounds good to me. I'll see if I can work up a patch. I have a hack in my
    testing code to work around the lack of auth in smtpd. It's not pretty but it
    works.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 7, 2015

    I have a patch to support initial-response, which I'll be posting here after a bit of clean up and a full (local) test run, with documentation. I ended up adding a keyword argument initial_response_ok=True to .login() and .auth(). The reason for this is that some clients may wish to force smtplib to do challenge/response.

    By default, mechanisms that support it (e.g. AUTH PLAIN) will send the initial response. Setting initial_response_ok=False in either SMTP.auth() or SMTP.login() will prevent smtplib from sending the initial response, and instead deal with challenge/response.

    I made initial_response_ok a keyword argument.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 7, 2015

    Attached patch includes test, documentation, and implementation. While this is technically a new feature, it fixes a regression in Python 3.5 w.r.t. 3.4. I'll email python-dev with a request for beta exemption.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 7, 2015

    I don't see any need to add the is_initial_auth_ok flag. Either the auth method returns something that is not None (initial auth is OK), or it doesn't (initial auth is not OK).

    Providing for that initial return value from the authobj is still a change to the new feature, but I don't think fixes to API bugs require an exception to the features rule, especially when they represent a fix to a regression.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 7, 2015

    On Jul 07, 2015, at 05:59 PM, R. David Murray wrote:

    I don't see any need to add the is_initial_auth_ok flag. Either the auth
    method returns something that is not None (initial auth is OK), or it doesn't
    (initial auth is not OK).

    I added this because test_smtplib itself expects some challenge/response even
    for AUTH PLAIN. I could have done more surgery on test_smtplib to avoid this,
    but on thinking about it, I decided that it makes sense to allow for the
    override. smtplib is often used in test scenarios, so imagine testing an SMTP
    server implementation. To get full coverage you'd want to test both
    initial-response and challenge/response even for auth methods like PLAIN.
    Indeed, I've been thinking about writing an asyncio-based smtpd, and it would
    be nice to be able to handle both cases without having to derive from class
    SMTP and re-implementing auth_plain and auth_login.

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 7, 2015

    OK, that makes sense. I'll try to give this a thorough review soon.

    @warsaw
    Copy link
    Member

    warsaw commented Jul 7, 2015

    Thanks!

    @bitdancer
    Copy link
    Member

    bitdancer commented Jul 8, 2015

    Only one question, about a possible missing test. Otherwise this looks good to me.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 9, 2015

    New changeset 97a29b86a2dc by Barry Warsaw in branch '3.5':

    New changeset 2d9003d44694 by Barry Warsaw in branch 'default':

    @warsaw warsaw closed this as completed Jul 9, 2015
    @warsaw warsaw self-assigned this Jul 9, 2015
    @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
    expert-email stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants