Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#3939 defect closed fixed (fixed)

PLAINAuthenticator in twisted.mail.imap4 does not follow RFCs

Reported by: khorn Owned by:
Priority: normal Milestone:
Component: mail Keywords:
Cc: jesstess, Jean-Paul Calderone Branch:
Author:

Description

PLAINAuthenticator.challengeResponse() uses the following statement to send auth credentials to the server

return '%s\0%s\0' % (self.user, secret)

which would give auth credentials of the form:

authid<NUL>password<NUL>

(where <NUL> is the NUL character)

However, both RFC2595 and RFC4616 (both define the PLAIN SASL mechanism), say that credentials should be passed this way:

[authzid]<NUL>authnid<NUL>password

(where <NUL> is the NUL character and [authzid] is optional)

Now even if one was to leave the authzid out of the equation, you would end up with something like this:

<NUL>authnid<NUL>password

and the version Twisted's IMAP code uses appears to be invalid.

NOTE: PLAINCredentials may also need to be modified to follow RFCs

Attachments (3)

SASL_PLAIN_fix.patch (897 bytes) - added by khorn 10 years ago.
possible fix to PLAIN auth to match RFCs
SASL_PLAIN_fix_w_tests.patch (2.2 KB) - added by khorn 10 years ago.
new patch with unit tests included
SASL_PLAIN_fix_w_tests.2.patch (2.5 KB) - added by khorn 10 years ago.
patch with unit tests following review

Download all attachments as: .zip

Change History (16)

Changed 10 years ago by khorn

Attachment: SASL_PLAIN_fix.patch added

possible fix to PLAIN auth to match RFCs

comment:1 Changed 10 years ago by khorn

Attached a patch which contains a possible fix to this issue.

NOTE: I have not tested this patch to any real extent!

comment:2 Changed 10 years ago by khorn

ran 'trial twisted' on Win32 with my patch with the following results:

Ran 5655 tests in 393.390s

PASSED (skips=962, expectedFailures=20, successes=4673)

comment:3 Changed 10 years ago by khorn

Keywords: review added
Owner: Jean-Paul Calderone deleted

putting in 'review' as per step 7 here: http://twistedmatrix.com/trac/wiki/TwistedDevelopment

comment:4 Changed 10 years ago by jesstess

Cc: jesstess added
Keywords: review removed
Owner: set to khorn

Thanks for the patch.

The patch applies cleanly and doesn't break existing tests, and I checked the RFCs against the current implementation and can confirm that the nulls aren't in the correct place.

It needs to be accompanied by a unit test, though.

comment:5 Changed 10 years ago by khorn

I'm awfully busy this week, but I'll see if I can't add tests next week.

Changed 10 years ago by khorn

new patch with unit tests included

comment:6 Changed 10 years ago by khorn

Keywords: review added
Owner: khorn deleted

OK, so it was more like "next month" instead of "next week", still...

New patch attached with a couple unit tests. Placing back in review status.

Question for reviewers: are these tests in an acceptable location, or would you rather see them someplace else?

comment:7 Changed 10 years ago by sii

I'll just add that the client fix works for me to. I came up with same fix independently when researching the problem before having checked the tracker. I haven't tested the server fix, but it also looks correct.

comment:8 Changed 10 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: set to khorn

Thanks for the new patch. Sorry for the delay in getting this review done.

The tests seem to be in a fine place. And the tests themselves seem almost right. Just a few minor points:

  1. All test cases and test methods should have a docstring. These should describe what's being tested. This helps future maintainers understand the intent of the test better.
  2. The naming convention for test methods is "test_foo" - we use an underscore here to indicate that these methods are automatically discovered (like remote_foo, if you've used pb or xmlrpc with Twisted at all)
  3. Locals in the tests should use camel case, though - eg expectedResponse
  4. The commented out lines with the old, wrong values don't need to be included.

Thanks again.

comment:9 in reply to:  8 Changed 10 years ago by khorn

Replying to exarkun:

Thanks for the new patch. Sorry for the delay in getting this review done.

It happens. :)

The tests seem to be in a fine place. And the tests themselves seem almost right. Just a few minor points:

  1. All test cases and test methods should have a docstring. These should describe what's being tested. This helps future maintainers understand the intent of the test better.

Fixed.

  1. The naming convention for test methods is "test_foo" - we use an underscore here to indicate that these methods are automatically discovered (like remote_foo, if you've used pb or xmlrpc with Twisted at all)

Fixed. Sorry, was going by the other tests in the file, rather than the most recent coding style.

  1. Locals in the tests should use camel case, though - eg expectedResponse

Fixed.

  1. The commented out lines with the old, wrong values don't need to be included.

Doh! Thought I had taken those out. Fixed.

Thanks again.

De nada. New patch comin...

Changed 10 years ago by khorn

patch with unit tests following review

comment:10 Changed 10 years ago by khorn

Keywords: review added
Owner: khorn deleted

back to review queue

comment:11 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [27451]) Change PLAINAuthenticator to be RFC 2595 compliant

Author: khorn, exarkun Reviewer: exarkun, itamar Fixes: #3939

Change twisted.mail.imap4.PLAINAuthenticator and PLAINCredentials so that they generate and parse challenge and response strings with NULs in the proper places.

comment:12 Changed 8 years ago by <automation>

comment:13 Changed 3 years ago by hawkowl

Keywords: review removed

[mass edit] Removing review from closed tickets.

Note: See TracTickets for help on using tickets.