Opened 9 years ago

Closed 6 years ago

#6176 enhancement closed fixed (fixed)

Port twisted.cred.credentials to Python 3

Reported by: Itamar Turner-Trauring Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Branch: branches/tccredentials-py3-6176-3
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl


twisted.cred.credentials should run on Python 3.

Change History (20)

comment:1 Changed 8 years ago by Ihar Hrachyshka

Owner: set to Ihar Hrachyshka
Status: newassigned

comment:2 Changed 8 years ago by Ihar Hrachyshka

Owner: Ihar Hrachyshka deleted
Status: assignednew

This also depends on #5203 being fixed (unicode vs. str in py3k is not considered in filepath code).

comment:3 Changed 6 years ago by hawkowl

Owner: set to hawkowl

comment:4 Changed 6 years ago by hawkowl

Author: hawkowl
Branch: branches/tccredentials-py3-6176

(In [44283]) Branching to tccredentials-py3-6176.

comment:5 Changed 6 years ago by hawkowl

Owner: hawkowl deleted

Buildbots are green, coverage looks good, please review.

comment:6 Changed 6 years ago by hawkowl

Keywords: review added

comment:7 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Thanks for working on this. Changes looks good.

Thistedchecker is red... at least networkFormat needs a docstring

I know that this is not specific to current changes.

I am a bit nervous while reviewing this as I see that some tests just reuse the logic from the implementation so for example let's say that binascii.hexlify is broken, than the test will still pass as it uses the same broken implementation. Most of the time this is ok, but not for code related to authentication.

So instead of computing expected in test code using the same methods and logic from the implementation I was expecting to see hardcoded expected values.

just minor comments

  1. There is no news file.
  1. Hope that md5-sess fix will land soon and we can remove it from the diff
  1. I see the logic for __all3__ duplicated in many test module, maybe we need to refactor it into a helper method... not what all tests suites ends with 'Tests' maybe we can have something like `partialPy3Ported('SomeTests', 'AnotherTests') and the code can automatically remove all other tests suites.
  1. For class Testable(components.Adapter): maybe instead of pass we can add a docstring.
  1. Maybe you can implement networkFormat as networkFormat(format, *args) so that we don't need to put the tuple each time.


Please merge md5-sess fix and fix twistedchecker + newsfile and then resubmit for review.

comment:8 Changed 6 years ago by hawkowl

Branch: branches/tccredentials-py3-6176branches/tccredentials-py3-6176-2

(In [44322]) Branching to tccredentials-py3-6176-2.

comment:9 Changed 6 years ago by hawkowl

Keywords: review added

Thanks Adi!

  • Fixed the TwistedChecker errors.
  • Added a newsfile.
  • Removed the md5-sess from the diff when I merged forward.
  • Reimplemented networkFormat as a bytestring join operation, since it's much faster, and doesn't do encoding dances as much.
  • I think args vs *args is clearer... not sure.

Builders spun, please review.

comment:10 Changed 6 years ago by hawkowl

Owner: hawkowl deleted

comment:11 Changed 6 years ago by Adi Roiban

Changes looks good and I am +1 for merging them.

The newsfile is a bit vague to me. Maybe it should talk about the credential checkers available on Py3 as not all credential checkers are available yet.

I am still not happy with code duplication from __all3__ part and the manual work need to update the __all__ list. I don't think that this should stop this merge.

What do you say if we have something like this in compat... implemented in a separate ticket?

# Defined in compat
def testSuitesPortedToPython3(py3Names):
    Should be call at the end of a module.
    allNames = []
    for key, value in globals().items():
        if value is unittest.TestCase:
        if isinstance(value, unittest.TestCase):

    if not allNames:
        raise AssertionError('No test suite. Did you call this at the end of the module?')

    for name in allNames:
        if name not in py3Names:
            del globals()[name]

# called at the end of test module

since this is security related code I will leave this for another dev to review it.


comment:12 Changed 6 years ago by hawkowl

Branch: branches/tccredentials-py3-6176-2branches/tccredentials-py3-6176-3

(In [44584]) Branching to tccredentials-py3-6176-3.

comment:13 Changed 6 years ago by Moshe Zadka

In joinBytes, what is the use case for decoding ASCII rather than erroring out on unicode immediately? I would rather if someone tries passing in unicode, that it break cleanly rather than waiting for the first French person to come along. Worst, I'm not entirely sure this doesn't introduce a new and exciting side-channel attack by timing the exception. I'm not sure that it does, but I'm scared.

comment:14 Changed 6 years ago by hawkowl

The use case for decoding ASCII is the same as twisted.python.compat.networkString -- sometimes things give you ASCII encoded in a text string (like, an IP address). Plus, twisted.cred (should) only ever get bytes -- eg. from twisted.web.

As far as the side-channel attack... I'm not sure that we're side-channel attack resistant, anywhere. Especially when you put PyPy into the mix. :P

comment:15 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Thanks for the work on this, hawkowl.

  1. I am also pretty down on joinBytes. At every call site, there are bits of network input that are correctly typed as bytes (nonce) mixed in with other bits of input that are either clearly ASCII-only native strings (k, now, clientip) or API inputs which might easily incorrectly be non-ASCII unicode (.username, .realm, .password, v). It seems to me that the only function of joinBytes is to obscure encoding errors and make it harder to track down which one is the one that wasn't encoded correctly, by putting the encoding into a loop instead of a nice traceback pointing at the problematic line which is decoding a specific variable. I think this should be carefully done at each place where it's called, and I definitely don't want to expose a public API to encourage other code to start doing this.
  2. Minor stylistic nitpick; I think it should be from binascii import hexlify rather than binascii.hexlify.
  3. CramMD5Credentials needs attribute documentation for challenge and response. Sorry for the documentation burden, but this is definitely starting to be a subtle change.
  4. Please put a comment around the except (ImportError, SyntaxError) explaining what that's doing (presumably pamauth isn't ported)
  5. I don't understand the need for the TypeError in the from twisted.cred import checkers import; can we get rid of this? Adding blanket exception handlers around imports like this can make debugging really annoying. Presumably it's importable after the changes in this branch...

Some optional notes:

  1. I think we should generally prefer .format rather than % when we're formatting strings.
  2. Since you're changing types to bytes, might as well change it to L{bytes} rather than C{bytes} everywhere.
  3. I like adi's idea for a utility decorator making the manual (temporary) __all__ munging more declarative. Can you file a ticket for this?

I think this needs a re-review to see how it looks with joinBytes removed, but otherwise I think it is close to ready. I have really mixed feelings about username and password being bytes, but ... these data structures probably need to be re-invented from scratch to accommodate text input :-\.

comment:16 Changed 6 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted
Priority: lowestnormal

Okay, I think I've addressed nearly everything.

I think joinBytes is a bit misguided, yeah, and I've replaced it with b"whatever".join(). In the tests I've tried to replace them with thing + b":" + spam style to make it less likely that a formatting bug would slip through (as they were being constructed two different ways and must be the same).

I'll file a ticket for the __all__ stuff.

Builders are spun and a wonderful green, please review.

comment:17 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl


You did miss the binascii.hexlifyhexlify thing, but I guess that's fine.

In addition to filing a ticket for making the __all__ thing declarative, I realize that this ugliness is part of the same problem on the incremental tickets I was reviewing yesterday. It's simply too easy to miss this conditional and to assume that we have more coverage than we do, because the test module is marked as "ported".

I also think that the TypeError on import thing is mostly an accident and even with the explanation I don't like it; test code should have as few control structures in it as possible, and this adds a whole dimension for partial failure that isn't really valid.

But, okay, actionable feedback:

  1. I think that we should take the opportunity here to create a twisted.cred.test package and separate out the different module tests. So we should move CramMD5CredentialsTests and to a new twisted.cred.test.test_cramauth module, and perhaps take the opportunity as well (since this won't produce diff churn) to move test_digestauth to this package as well. Then test_cramauth and test_digestauth can be in dist3 and test_newcred won't be. This means you'll need to make CramMD5CredentialsTests twistedchecker-clean and write some docstrings, but that's probably less overall churn than the other unrelated changes in test_newcred to give it the illusion that it can be imported.
  2. Given the fresh perspective that seeing which test modules will actually be included, it is clear we are missing direct test coverage of our more basic credential types, like UsernamePassword. Maybe add a new test module, test_simpleauth, just to make sure the constructors and checkPassword and various provider/implementor checks and such work as expected.

One more submit for re-review. Thanks for slogging through this one; I feel like we are converging on a slightly better way to get these modules cleaned up and ported now.

comment:18 Changed 6 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Hi Glyph, thanks for the review!

  • I actually missed those ones, fixed that.
  • I moved the CRAM tests into their own file, and moved all the cred tests (test_digestauth and test_newcred) into the twisted.cred.test package.
  • I removed all of the churn from test_newcred.
  • Added tests for the basic things that I thought I could test well enough. The SSH stuff is excluded, since I am not sure about what it does or what it should do.
  • The PAM stuff is untested and I found a ticket advocating for its deprecation for horrendous security reasons. As such, I'm leaving it untouched.

Builders spun, please review.

comment:19 Changed 6 years ago by Glyph

Keywords: review removed
Owner: set to hawkowl

Hi hawkie. Thanks. Ready to merge but for a few minor issues:

  1. There are numerous twistedchecker errors, but only one (in twisted.cred.test) is in added rather than moved code.
    1. Please fix the one new error.
    2. Please file a fast-following ticket to fix the remainder of the warnings in test_digestauth since this stuff is fresh in your mind. test_newcred is probably too much to ask for though :-).
  2. Since the file is moving anyway, could you please rename test_newcred to test_cred in its new home? The "new" in the name conveys absolutely zero information.

Thanks, please land when done.

comment:20 Changed 6 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [44731]) Merge tccredentials-py3-6176-3: Port twisted.cred.credentials to Python 3

Author: hawkowl Reviewers: adiroiban, glyph Fixes: #6176

Note: See TracTickets for help on using tickets.