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.py AUTH LOGIN code messed up sending login and password data since 3.5 #69632

Closed
merkel mannequin opened this issue Oct 20, 2015 · 15 comments
Closed

smtplib.py AUTH LOGIN code messed up sending login and password data since 3.5 #69632

merkel mannequin opened this issue Oct 20, 2015 · 15 comments
Labels
release-blocker topic-email type-bug An unexpected behavior, bug, or error

Comments

@merkel
Copy link
Mannequin

merkel mannequin commented Oct 20, 2015

BPO 25446
Nosy @warsaw, @larryhastings, @ned-deily, @bitdancer, @Rosuav
Files
  • smtplib.py: Fixed version of smtplib.py
  • smtplib-patch.issue25446.patch: patch including change proposal.
  • 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 2015-11-08.06:19:42.518>
    created_at = <Date 2015-10-20.15:41:32.351>
    labels = ['type-bug', 'expert-email', 'release-blocker']
    title = 'smtplib.py AUTH LOGIN code messed up sending login and password data since 3.5'
    updated_at = <Date 2016-08-30.21:05:21.739>
    user = 'https://bugs.python.org/merkel'

    bugs.python.org fields:

    activity = <Date 2016-08-30.21:05:21.739>
    actor = 'miohtama'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-11-08.06:19:42.518>
    closer = 'r.david.murray'
    components = ['email']
    creation = <Date 2015-10-20.15:41:32.351>
    creator = 'merkel'
    dependencies = []
    files = ['40828', '40829']
    hgrepos = []
    issue_num = 25446
    keywords = ['patch', '3.5regression']
    message_count = 15.0
    messages = ['253225', '253241', '253242', '253243', '253256', '253259', '253261', '253262', '253287', '253867', '253876', '254032', '254327', '254329', '273966']
    nosy_count = 8.0
    nosy_names = ['barry', 'larry', 'ned.deily', 'r.david.murray', 'python-dev', 'Rosuav', 'miohtama', 'merkel']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25446'
    versions = ['Python 3.5', 'Python 3.6']

    @merkel
    Copy link
    Mannequin Author

    merkel mannequin commented Oct 20, 2015

    class SMTP:
        def auth_login(self, challenge=None):

    The self.docmd should use cmd "AUTH" with parameter "LOGIN" + encoded login like

        (code, resp) = self.docmd("AUTH", "LOGIN " +
            encode_base64(self.user.encode('ascii'), eol=''))
    

    with

        def auth(self, mechanism, authobject, *, initial_response_ok=True):

    that should not send a "AUTH" in self.docmd in case the mechanism is 'LOGIN' and

            if initial_response is not None:

    meaning

                if mechanism == 'LOGIN':
                    (code, resp) = self.docmd(response)
                else:
                    (code, resp) = self.docmd("AUTH", mechanism + " " + response)

    Could someone kindly review, evtly come up with better suggestion?

    In short:
    $ diff /c/Python35/Lib/smtplib-old.py /c/Python35/Lib/smtplib.py
    630c630,633
    < (code, resp) = self.docmd("AUTH", mechanism + " " + response)
    ---

            if mechanism == 'LOGIN':
                (code, resp) = self.docmd(response)
            else:
                (code, resp) = self.docmd("AUTH", mechanism + " " + response)
    

    660c663
    < (code, resp) = self.docmd(
    ---

        (code, resp) = self.docmd("AUTH", "LOGIN " +
    

    @merkel merkel mannequin added topic-email type-bug An unexpected behavior, bug, or error labels Oct 20, 2015
    @bitdancer
    Copy link
    Member

    Does the fix at the end of bpo-15014 address your concern?

    @bitdancer
    Copy link
    Member

    Sorry, I misremenbered and thought that fix didn't make it in to 3.5, but in fact you are talking about the behavior of that fix.

    @bitdancer
    Copy link
    Member

    Can you explain more about the failure you are seeing?

    @merkel
    Copy link
    Mannequin Author

    merkel mannequin commented Oct 20, 2015

    Let us assume you want to establish a smtp session with AUTH LOGIN as the supported authentication type. Sample code to send mail: Typical preparation steps like

        with SMTP( mailserver, 587 ) as smtp:
          # smtp.set_debuglevel(1)
          smtp.ehlo()
          smtp.starttls()
          smtp.ehlo()
          smtp.login( account, password )

    If you try to login (last line in sample code above) at the smtp server then the smtp server will expect a command to be send like

    AUTH LOGIN <base64encodedaccountdata>

    Now switching from sample code to our smtplib.py:

    Since the "AUTH LOGIN" is missing in original Python35/Lib/smtplib.py in line...

    660c663
    < (code, resp) = self.docmd(
    ---

        (code, resp) = self.docmd("AUTH", "LOGIN " +
    

    ... the smtp server will answer that the library is sending an unknown command here. That is why I added... "AUTH", "LOGIN " + ...at this line.

    Line 660 in class SMTP: def auth_login is called before it reaches line 630 in class SMTP: def auth

    In case of authentication type AUTH LOGIN in line 630 you must not call with "AUTH", mechanism + " " +

    So the following changes have to be applied at least for AUTH LOGIN mechanism

    630c630,633
    < (code, resp) = self.docmd("AUTH", mechanism + " " + response)
    ---

            if mechanism == 'LOGIN':
                (code, resp) = self.docmd(response)
            else:
                (code, resp) = self.docmd("AUTH", mechanism + " " + response)
    

    The first change affecting line 660 described above will will imply that the you remove the AUTH mechanism in line 630. For mechanism LOGIN the base64 encoded password will be needed to be sent in 660...

    See possible fix in the diff above.

    To ease understanding the fix I will apply a running version of my local Lib/smtplib.py (instead of just providing the diff lines). Feel free to directly use the file.

    @merkel
    Copy link
    Mannequin Author

    merkel mannequin commented Oct 20, 2015

    Sample session log output showing the error with smtp.set_debuglevel(1):

    send: 'ehlo <mysmtpserver>\r\n'
    reply: b'250-<mysmtpserver> Hello [myIP4address]\r\n'
    reply: b'250-SIZE 53248000\r\n'
    reply: b'250-PIPELINING\r\n'
    reply: b'250-DSN\r\n'
    reply: b'250-ENHANCEDSTATUSCODES\r\n'
    reply: b'250-STARTTLS\r\n'
    reply: b'250-AUTH GSSAPI NTLM\r\n'
    reply: b'250-8BITMIME\r\n'
    reply: b'250-BINARYMIME\r\n'
    reply: b'250 CHUNKING\r\n'
    reply: retcode (250); Msg: b'<mysmtpserver> Hello [myIP4address]\
    nSIZE 53248000\nPIPELINING\nDSN\nENHANCEDSTATUSCODES\nSTARTTLS\nAUTH GSSAPI NTLM
    \n8BITMIME\nBINARYMIME\nCHUNKING'
    send: 'STARTTLS\r\n'
    reply: b'220 2.0.0 SMTP server ready\r\n'
    reply: retcode (220); Msg: b'2.0.0 SMTP server ready'
    send: 'ehlo [mymachinename]\r\n'
    reply: b'250-<mysmtpserver> Hello [myIP4address]\r\n'
    reply: b'250-SIZE 53248000\r\n'
    reply: b'250-PIPELINING\r\n'
    reply: b'250-DSN\r\n'
    reply: b'250-ENHANCEDSTATUSCODES\r\n'
    reply: b'250-AUTH GSSAPI NTLM LOGIN\r\n'
    reply: b'250-8BITMIME\r\n'
    reply: b'250-BINARYMIME\r\n'
    reply: b'250 CHUNKING\r\n'
    reply: retcode (250); Msg: b'<mysmtpserver> Hello [myIP4address]\
    nSIZE 53248000\nPIPELINING\nDSN\nENHANCEDSTATUSCODES\nAUTH GSSAPI NTLM LOGIN\n8B
    ITMIME\nBINARYMIME\nCHUNKING'
    send: '<base64encodedaccountnamedata>==\r\n'
    reply: b'500 5.3.3 Unrecognized command\r\n'
    reply: retcode (500); Msg: b'5.3.3 Unrecognized command'
    send: 'QUIT\r\n'
    reply: b'221 2.0.0 Service closing transmission channel\r\n'
    reply: retcode (221); Msg: b'2.0.0 Service closing transmission channel'
    Traceback (most recent call last):
      File "sendtestmail.py", line 172, in <module>
        announcement.sendMail(password)
      File "sendtestmail.py", line 97, in sendMail
        smtp.login( self.getShortAddressList()[0], password )
      File "c:\Python35\lib\smtplib.py", line 730, in login
        raise last_exception
      File "c:\Python35\lib\smtplib.py", line 721, in login
        initial_response_ok=initial_response_ok)
      File "c:\Python35\lib\smtplib.py", line 627, in auth
        initial_response = (authobject() if initial_response_ok else None)
      File "c:\Python35\lib\smtplib.py", line 664, in auth_login
        raise SMTPAuthenticationError(code, resp)
    smtplib.SMTPAuthenticationError: (500, b'5.3.3 Unrecognized command')

    due to missing AUTH LOGIN here as previously described...

    @bitdancer
    Copy link
    Member

    Ah, now I see what you are saying. How in the world did we miss that? Our unit tests must be broken too.

    @merkel
    Copy link
    Mannequin Author

    merkel mannequin commented Oct 20, 2015

    Change proposal attached as a unified diff / patch file.

    @bitdancer
    Copy link
    Member

    Thanks, but special-casing login in the 'auth' method means that the auth method isn't working right, since special-casing defeats the whole purpose of the auth mechanism.

    I think we need to change the logic in auth so that it is checking for a 334 even if it has been provided an initial response. That is, outdent the block that starts with the '# Server replies...' comment. Once that is done, auth_login becomes:

      def auth_login(self, challenge=None)
          if challenge is None:
              return encode_base64(self.user.encode('ascii'))
          else:
              return self.password

    We may also need to add a try/except around the base64.decodebytes in auth.

    And we need a unit test that demonstrates the current failure.

    I'm also wondering now about the ascii encoding on the challenge and response. Someone should check the RFC to see if those are limited to ascii or if they can contain other bytes. If they are limited to ascii we should stick in a comment to that effect with a pointer to the relevant RFC section.

    @larryhastings
    Copy link
    Contributor

    I think it's about time to think about releasing 3.5.1. But since this bug is marked as a "release blocker", 3.5.1 cannot be released until this is fixed. Arguably I can't even really make a schedule for 3.5.1 until it's fixed, or at least I'm reasonably confident I know when it'll be fixed.

    Any idea when this might be fixed?

    @bitdancer
    Copy link
    Member

    I will work on it this week, should have something committed before the end of next weekend.

    @larryhastings
    Copy link
    Contributor

    Okay, I'm scheduling 3.5.1rc1 on the assumption that you'll check in by next weekend. If you're going to slip please let me know and I'll slip accordingly.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Nov 8, 2015

    New changeset d13263ecf0c6 by R David Murray in branch '3.5':
    bpo-25446: Fix regression in smtplib's AUTH LOGIN support.
    https://hg.python.org/cpython/rev/d13263ecf0c6

    New changeset 7368b86432c6 by R David Murray in branch 'default':
    Merge: bpo-25446: Fix regression in smtplib's AUTH LOGIN support.
    https://hg.python.org/cpython/rev/7368b86432c6

    @bitdancer
    Copy link
    Member

    Fixed. I figured it was better not to wait for a review in this case...the fix is straightforward, but writing the test took quite a bit of work. The auth tests in smtplib are now much more robust, thanks in large part to Milan's code in bpo-21935.

    @miohtama
    Copy link
    Mannequin

    miohtama mannequin commented Aug 30, 2016

    I think there is something more in this.

    I am running Python 3.5.0 (default, Apr 24 2016, 12:47:36).

    Sparkpost servers require AUTH LOGIN approach as per their instructions https://support.sparkpost.com/customer/portal/articles/1988470-smtp-connection-problems

    When trying to use these (free) servers smtplib authentication will result to smtplib.SMTPAuthenticationError: (500, b'5.5.2 unrecognized command') like in the issue description earlier. I am still investigating this.

    @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
    release-blocker topic-email type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants