Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#2682 defect closed fixed (fixed)

twisted.conch.ssh.userauth is poorly tested.

Reported by: z3p Owned by:
Priority: high Milestone:
Component: conch Keywords: soc2007
Cc: therve, Thijs Triemstra, Jean-Paul Calderone Branch: branches/userauth-2682-7
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

Description

The reasons are too large to enumerate, but twisted.conch.ssh.userauth needs better tests.

Attachments (1)

userauth-imports-2682.patch (7.6 KB) - added by Thijs Triemstra 9 years ago.
Adding patch that removes the remaining relative imports

Download all attachments as: .zip

Change History (56)

comment:1 Changed 10 years ago by z3p

Status: newassigned
Summary: twisted.conch.ssh.userauth has poor teststwisted.conch.ssh.userauth is poorly tested.

comment:2 Changed 10 years ago by z3p

Keywords: review added
Owner: z3p deleted
Priority: highhighest
Status: assignednew

Ready for review in branches/userauth-2862.

comment:3 Changed 10 years ago by therve

Cc: therve added
  • The import () syntax is not supported under python2.3
  • LoopbackTestCase.test_loopback has a problem when run 3 times, but I wasn't able to spot it. To reproduce, run trial -u twisted.conch.test.test_userauth.LoopbackTestCase.test_loopback. Overall, this test looks very complicated.

That's it for now.

comment:4 Changed 10 years ago by z3p

I'm not sure how to make it less complicated than it is, but I've fixed the problem where it would crash after 3 runs. I've also fixed the import () syntax.

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

Owner: set to Jean-Paul Calderone
Status: newassigned

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

Keywords: review removed
Owner: changed from Jean-Paul Calderone to z3p
Status: assignednew

Hmmm. Changes to checkers.py probably look good (although that class could use some docstrings), but I'm not really sure why SSHProtocolChecker exists in the first place. At most, it seems like a unit test utility class. Does it belong somewhere beneath the twisted.conch.test package instead?

In userauth.py, the relative imports should probably be replaced with absolute imports (at least the ones which are changing, but I don't think there's a reason not to fix them all). Also please add some whitespace around + in SSHUserAuthClient._setNewPass. In SSHUserAuthServer.ssh_USERAUTH_REQUEST, there is a change to fix what I think was a NameError for ConchError - calling self._ebBadAuth with error.ConchError(...) instead of ConchError(...) - but there's no test coverage for this line. SSHUserAuthServer and SSHUserAuthClient are missing class docstrings (include attribute docs).

In test_userauth.py, the stub transport and userauth classes are defined with simple suites - put the pass statements on the next line (better, replace them with docstrings). Also, ClientUserAuth is missing a bunch of docstrings, and getPrivateKey has a line >80 cols. I'd expand "KI" in "test_removeKIIfUnencrypted". test_loginTimeout and test_canceLoginTimeout should both use a fake clock to make their operation deterministic - see twisted.internet.task.Clock. _ function in test_USERAUTH_SUCCESS should be renamed. test_keyboard_interactive should be renamed test_keyboardInteractive. Some whitespace before LoopbackTestCase would be nice. :) The nested Factory and Service classes are missing docstrings. _ function in makeConnection in setUp in test_loopback should be renamed.

test_loopback is still doing a lot of stuff. Much of this could be dropped if the test didn't try to implement so much of the SSH transport interface (which clearly isn't the same as ITCPTransport). Is there a reason an existing conch class can't be wrapped around the underlying loopback transport and that object exposed to the instances in the test method? If this could be done, it should remove the need for the entire setUp function, at which point the test method is almost reasonably sized.

comment:7 Changed 10 years ago by z3p

Keywords: review added
Owner: z3p deleted

Re: SSHProtocolChecker: it's been in checkers because it's (potentially) useful as a checker which needs multiple authentications to succeed.

The other changes have been made and I'm putting this back up for review.

comment:8 in reply to:  2 ; Changed 10 years ago by Peaker

Replying to z3p:

Ready for review in branches/userauth-2862.

  1. Is that a typo here or in the branch name (2862 instead of 2682)?
  2. Is this branch still the right place to review?

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

Replying to Peaker:

Replying to z3p:

Ready for review in branches/userauth-2862.

  1. Is that a typo here or in the branch name (2862 instead of 2682)?
  2. Is this branch still the right place to review?

Yes, that's a typo. The correct branch is userauth-2682.

comment:10 Changed 10 years ago by radix

Keywords: review removed
Owner: set to z3p
  1. Indentation in userauth.py is slightly incorrect; second two lines should be indented one more space.

+ b = (NS(self.transport.sessionID) + chr(MSG_USERAUTH_REQUEST) + + NS(self.user) + NS(self.instance.name) + NS('publickey') + + '\xff' + NS(keyType) + NS(publicKey))

  1. Docstrings are missing from functions that have been changed. Notably, can you please add a docstring to SSHProtocolChecker (and/or requestAvatarId) which describes the entire process of authentication, including how areDone is used (because areDone looks dubious at best and really needs a good explanation if it's to stay).
  1. @type nextService: C{stR} in userauth.py

4.

@ivar supportedAuthentications: A list of the supported authentication

methods.

@type supportedAuthentications: C{list}

List of what? I guess strings.

  1. I can't read all of this at once, I need some ice cream. Thanks.

comment:11 Changed 10 years ago by z3p

Keywords: review added
Owner: changed from z3p to radix

1-4 are taken care of. As for 5, if you come visit me here in Northampton I'll buy you a cone. :) Back up for review.

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

Keywords: review removed
Owner: changed from radix to z3p

I can sort of see what role SSHProtocolChecker is filling now. It doesn't seem like a very nice one. It also violates the interface required of checkers (by raising NotEnoughAuthentication) and doesn't work with concurrent logins for the same avatar identifier. It's not new in the branch, so this doesn't need to hold up this ticket, I think, but it seems like it should be removed at some point, or at least drastically changed (I'm not sure what shape would make sense, though). The only thing to note here is that the dependence of the new test_loopback on SSHProtocolChecker will make it harder to remove SSHProtocolChecker in the future. AFAIK there is no other code in Twisted which uses it.

There is some trailing whitespace in files changed in this branch and some pyflakes warnings (not all of which are real problems, but some are) as well. There's also a backslash line continuation which should be removed in test_loopback.

The rest looks okay.

comment:13 Changed 10 years ago by z3p

Keywords: review added
Owner: z3p deleted

Ready for review in userauth-2682-2.

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

Keywords: review removed
Owner: set to z3p

userauth-2682-2 was created from a revision of trunk which had the #2679 branch merged, so this code doesn't work merged to trunk at the moment. There should probably be a -3 made after #2679 is re-merged.

comment:15 Changed 10 years ago by z3p

Keywords: review added
Owner: z3p deleted

userauth-2682-2 merges to trunk just fine. I added an extra test since the last review, for old getPrivateKey() behavior which the cftp tests required.

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

Keywords: review removed
Owner: set to z3p

Lots of test failures on the debian-py2.4-select slave.

Mostly shaped like this:

[ERROR]: twisted.conch.test.test_keys.KeyTestCase.test_fromFile

Traceback (most recent call last):
  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/conch/test/test_keys.py", line 567, in test_fromFile
    self.assertEquals(keys.Key.fromFile(self.keyFile),
  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/conch/ssh/keys.py", line 58, in fromFile
    return Class.fromString(file(filename).read(), type, passphrase)
  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/conch/ssh/keys.py", line 84, in fromString
    return method(data)
  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/conch/ssh/keys.py", line 234, in _fromString_PRIVATE_LSH
    kd[name] = common.getMP(common.NS(data))[0]
  File "/srv/bb-slave/Run/slave/full2.4/Twisted/twisted/conch/ssh/common.py", line 91, in _fastgetMP
    mp.append(long(gmpy.mpz(data[c+4:c+l+4][::-1]+'\x00')))
exceptions.ValueError: string without NULL characters expected

comment:17 Changed 10 years ago by z3p

author: z3p
Branch: branches/userauth-2682-3

(In [22470]) Branching to 'userauth-2682-3'

comment:18 Changed 10 years ago by z3p

Keywords: review added
Owner: z3p deleted

A quick merge-forward picked up a fix for that bug.

comment:19 Changed 10 years ago by therve

Keywords: review removed
Owner: set to z3p
  • in tryAuth, second and third ConchError are untested
  • in _cbFinishedAuth, the ConchError is untested
  • _pamConv ConchErrors are untested
  • ssh_USERAUTH_INFO_RESPONSE use a bare expect, not tested either
  • _ebAuth is untested
  • ssh_USERAUTH_FAILURE both ValueErrors are untested
  • _cbUserauthFailure StopIteration is untested
  • in ssh_USERAUTH_PK_OK, the case where the return value of self.signData is false is untested
  • in _cbGetPublicKey, the case where publicKey is not a Key is untested
  • in auth_password, the return 0 case is untested
  • in signData, the case where self.getPrivateKey returns None is untested.
  • getPublicKey is not tested. The comment in it should be removed.
  • getPrivateKey, getPassword and getGenericAnswers are untested.
  • the assert in ssh_USERAUTH_INFO_RESPONSE should be removed

Thanks!

comment:20 Changed 10 years ago by z3p

(In [22566]) checkpointing for the night

Refs #2682

review items this fixes: # in tryAuth, second and third ConchError are untested # in _cbFinishedAuth, the ConchError is untested # _pamConv ConchErrors are untested # ssh_USERAUTH_INFO_RESPONSE use a bare expect, not tested either # _ebAuth is untested # the assert in ssh_USERAUTH_INFO_RESPONSE should be removed

comment:21 Changed 10 years ago by z3p

Keywords: review added

comment:22 Changed 10 years ago by therve

There is a small conflict with trunk, it would be great if you could resolve it.

comment:23 Changed 10 years ago by z3p

Branch: branches/userauth-2682-3branches/userauth-2682-4

(In [23057]) Branching to 'userauth-2682-4'

comment:24 Changed 10 years ago by z3p

Resolved in the new branch.

comment:25 Changed 10 years ago by therve

Keywords: review removed
Owner: changed from z3p to therve
  • key authentication with the conch client is broken
  • the bare except in ssh_USERAUTH_INFO_RESPONSE is unfortunate: it masks the wrong ConchError (it should be error.ConchError). The solution is maybe to use log.err in _ebBadAuth if the error is not ConchError (instead of log.msg(reason.getTraceback()))
  • trailing whitespaces in test_userauth
  • ClientUserAuth.getPassword has a useless blank line
  • SSHUserAuthServerTestCase docstring is not well formatted
  • many blank lines are missing. Sometimes there are a little bit too much :)
  • ssh_USERAUTH_SUCCESS has a commented line
  • the _pamDeferred attribute is not well managed in SSHUserAuthServer. It should take a default value, not be deleted when unused (rule: if you use hasattr, there is probably something wrong).
  • ssh_USERAUTH_PK_OK could be split in 3 smaller functions
  • auth_password is documented to return a bool, so it should use True and False. This is applicable to some other functions as well.
  • the code at the end of userauth should use locals() instead of importing userauth. It should delete the temporary variables too.

That's it for now!

comment:26 Changed 10 years ago by therve

Owner: changed from therve to z3p

comment:27 Changed 10 years ago by therve

Priority: highesthigh

Changed 9 years ago by Thijs Triemstra

Attachment: userauth-imports-2682.patch added

Adding patch that removes the remaining relative imports

comment:28 Changed 9 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review added

comment:29 Changed 9 years ago by therve

Keywords: review removed

Thanks for the patch, but I don't think that'll help z3p much on these branches. Also, the status review should be given on the current branch addressing the problem, not on a patch on the current branch. If you want do help, getting in touch with z3p and applying the change directly could be great. Sorry about this.

comment:30 Changed 9 years ago by z3p

Branch: branches/userauth-2682-4branches/userauth-2682-5

(In [24598]) Branching to 'userauth-2682-5'

comment:31 Changed 9 years ago by z3p

Status: newassigned

A change outside Conch (I imagine to Trial) has made one of my test-cases start failing. LoopbackTestCase.test_loopback is failing because the warning isn't being raised in client.startService() anymore. I'm not sure what to do about it; it's the only thing keeping me from putting this back up for review.

comment:32 Changed 9 years ago by Jean-Paul Calderone

It looks like the reason the warning isn't being emitted there might be that SSHUserAuthClient.serviceStarted isn't directly related to the deprecation; instead, it does some I/O, waits for a response from the server, then possibly does something that's deprecated. I think it's probably loopback which changed to break this test, by not delivering data between connected protocols as quickly as it used to. I suspect it'd be better to test for the warning in a much simpler more specific test, perhaps one which calls tryAuth or auth_publickey directly.

comment:33 Changed 9 years ago by z3p

I agree that testing that warning in a more specific test is a good idea. How else can I get that warning (which I know will happen at some point) to not appear in the Trial output?

comment:34 Changed 9 years ago by Jean-Paul Calderone

Use twisted.trial.util.suppress to create a suppression.

comment:35 Changed 9 years ago by Jean-Paul Calderone

I was reflecting a bit further on my last comment.

Another solution which suggests itself is this. If there is a more specific test (a unit test, if you will ;) for the warning being emitted in this case, then there's really no reason for these other larger tests, tests primarily for other functionality, to exercise the deprecated code paths. Perhaps instead of suppressing the warning in the tests, the implementation should be fixed so as not to emit it in these cases?

comment:36 Changed 9 years ago by z3p

Branch: branches/userauth-2682-5branches/userauth-2682-6

(In [25049]) Branching to 'userauth-2682-6'

comment:37 Changed 9 years ago by z3p

Keywords: review added
Owner: z3p deleted
Status: assignednew

Ready for review.

comment:38 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to z3p
  1. Coding standard stuff
    1. update copyright date in modified files
    2. SSHUserAuthServer.tryAuth formats a string to pass to error.ConchError by passing a string as the right hand argument to % - it should pass a one tuple instead.
    3. SSHProtocolChecker.requestAvatarId needs a docstring
    4. So does SSHProtocolChecker._cbGoodAuthentication
    5. The formatting of the first line of SSHProtocolChecker.areDone's docstring doesn't conform to the coding standard.
    6. A lot of C{} markup could be L{} markup instead. For example, in the docstring of SSHUserAuthServer, L{twisted.cred.portal.Portal} would be good.
    7. SSHUserAuthServer.serviceStopped needs a docstring
  2. Incompatible changes
    1. SSHProtocolChecker.areDone's signature changed. The docstring of areDone encourages users to override the method, so old-style implementations should be supported (deprecated, if you want). Using a new method name might be simpler than trying to inspect parameter count. Up to you, though.
  3. Some untested codepaths in userauth.py
    1. in SSHUserAuthServer.serviceStarted, the checks for 'password' and 'keyboard-interactive' being in self.supportedAuthentications are only tested for the positive case, not the negative case. Before those, the i in self.interfaceToMethod check is only tested for the positive case as well.
    2. in SSHUserAuthClient.tryAuth, the false case for the if f: conditional isn't tested.
    3. in SSHUserAuthClient.ssh_USERAUTH_PK_OK the false case for the if func is not None: conditional isn't tested.
  4. SSHUserAuthClient.ssh_USERAUTH_FAILURE might be much simplified by the use of twisted.python.util.dsu.
  5. SSHProtocolChecker.areDone's docstring says "checkers must return the same type of avatar". Does it mean "the same avatarId"?
  6. SSHUserAuthServer.auth_password misspells authentication in its docstring
  7. all of test_userauth is pretty quick now, except for LoopbackTestCase.test_loopback. I think that's because it's hitting the bad password delay. It'd be great if this test didn't have to sleep for 1 second.
  8. SSHUserAuthServer._cbFinishedAuth sets isAuthorized on its transport. This would be kind of a crummy API, except it doesn't actually seem to be an API for anything? I don't see isAuthorized read or written anywhere else in conch. I guess this is leftover cruft?
  9. SSHUserAuthServer.authenticatedWith is documented as a list... but of what?

When using an OpenSSH client against a Conch server, password authentication is broken. However, it seems broken in trunk too. If this is right and I'm not testing it wrong somehow, please file a new ticket.

The branch looks like it is getting pretty close. Nice work.

comment:39 Changed 9 years ago by z3p

What would a test for the various negative cases look like? Trying them and making sure an exception isn't raised?

Also, I just tried to connect to doc/conch/examples/sshsimpleserver.py with OpenSSH 5.1p1 and password authentication worked fine (both in the branch and in trunk). What's not working for you?

comment:40 Changed 9 years ago by z3p

Keywords: review added
Owner: z3p deleted

I added some whitebox tests for the negative cases. Since I can't reproduce the password authentication issue and haven't gotten more information, I'm putting this back up for review.

comment:41 Changed 9 years ago by therve

Here we go:

try:
    import Crypto.Cipher.DES3, Crypto.Cipher.XOR
    from twisted.conch.ssh.common import NS
    from twisted.conch.checkers import SSHProtocolChecker
    from twisted.conch.ssh import keys, userauth, transport
    from twisted.conch.test import keydata
except ImportError:
    keys = None

This could lead to unnoticed skipped tests if there is an import error in any of the file unrelated to Crypto. Can you move the imports into an else clause?

test_requestRaisesConchError looks a bit fragile. I think it would be better to fire a test global Deferred with the failure in mockEbBadAuth, and use assertFailure on that Deferred.

In the same spirit, test__pamConvErrors and test_tryAuthEdgeCases should use assertFailure.

More later, I hope.

comment:42 Changed 9 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: set to z3p
  1. SSHProtocolChecker.requestAvatarId's docstring starts with a typo
  2. SSHProtocolChecker._ebGoodAuthentication's docstring has some ungrammar near its start - if the a checker
  3. twisted.conch.test.test_userauth.ClientUserAuth.getPublicKey documents and sets usedKeys, but this doesn't seem to be used by anything.
  4. twisted.conch.test.test_userauth.LoopbackTestCase.test_loopback has a suppression for all deprecation warnings. This seems a bit excessive.
  5. SSHUserAuthServer._cbFinishedAuth still sets isAuthorized.

comment:43 Changed 9 years ago by Jean-Paul Calderone

Ah, one other thing, there are a lot of test failures in the branch. These seem to be things which have since been fixed in trunk. It would be good to merge the branch forward once more so as to be able to verify this, though.

comment:44 Changed 9 years ago by z3p

Branch: branches/userauth-2682-6branches/userauth-2682-7

(In [26137]) Branching to 'userauth-2682-7'

comment:45 Changed 9 years ago by z3p

Keywords: review added
Owner: z3p deleted

I addressed the comments from exarkun and therve, and there don't seem to be any test failures. I think it's ready for review.

comment:46 Changed 9 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:47 Changed 9 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to z3p
Status: assignednew

Some things I noticed during this review which it'd be great to get fixed separately from this set of changes:

  1. twisted.conch.checkers.SSHPublicKeyDatabase._cbRequestAvatarId doesn't have good coverage for the if not validKey: case nor for the exception handler or the final return statement. Also, logging the error and returning a failure is probably redundant. If the goal is to report the unexpected exception, it should probably be logged and then a /different/ failure returned (to normalize the type for whatever code invoked this).
  2. No test coverage for the final return statement in twisted.conch.checkers.SSHProtocolChecker.requestAvatarId.
  3. No test coverage for twisted.conch.checkers.SSHProtocolChecker.areDone.
  4. The logging going on in twisted.conch.ssh.userauth.SSHUserAuthServer._ebBadAuth is weird in a few ways:
    1. reason.check(IgnoreAuthentication) is better than reason.type == IgnoreAuthentication
    2. Failures should generally be log.err()'d, not log.msg(str(...))'d.
    3. If the wrong password was given, this is probably worth logging specifically without involving the string representation of a Failure instance or a traceback. Something that is more easily recognizable as a user error, not a programming error.
  5. pamauth is imported in twisted.conch.checkers but not used (and there's no such module, I think). I think this means pam authentication is broken in the server. This doesn't seem to be new in this branch, though.

Other things to fix:

  1. 5. SSHUserAuthServer._cbFinishedAuth still sets isAuthorized. I know this has been through a lot of reviews and you've done a huge amount of work on it, but it's really frustrating to see points from previous reviews go unaddressed (twice in this case) when re-reviewing code.
  2. twisted.conch.ssh.userauth.SSHUserAuthClient._cbGetPublicKey and _cbSignData emit deprecation warnings without a version number. They should say when the deprecation was first introduced - probably 9.0.
  3. twisted.conch.test.test_userauth.SSHUserAuthServerTestCase.test_requestRaisesConchError defines a _callback which is unused. Also, it doesn't return the result of assertFailure, so it isn't waiting for the Deferred.
  4. test__pamConvErrors also doesn't return the Deferreds which come back from its two assertFailure calls.
  5. Nor does test_tryAuthEdgeCases.
  6. Each of the three modified files should have the date in the copyright header updated to include 2009.

comment:48 Changed 9 years ago by z3p

Keywords: review added
Owner: z3p deleted

I added a bunch of tests for SSHPrivateKeyDatabase and SSHProtocolChecker, and I believe I've addressed the other points. Ready for review.

comment:49 Changed 9 years ago by therve

Keywords: review removed
Owner: set to z3p

1)

+        self._cancelLoginTimeout = self.clock.callLater(self.loginTimeout,
                                                     self.timeoutAuthentication)

Indentation is a bit weird on the second line.

2)

+        f= getattr(self,'auth_%s' % (kind,), None)

Please a space before the equal sign.

3) test_requestAvatarId, test_requestAvatarId_without_signature and test_requestAvatarId_with_not_enough_authentication are missing docstrings.

I think I'm mostly happy with the branch, and we should move on. Please fix the above points, make a buildbot run, and merge is the supported ones are green. Thanks!

comment:50 Changed 9 years ago by z3p

(In [26456]) remove space before equals sign

Refs #2682

comment:51 Changed 9 years ago by z3p

(In [26457]) fix indentation

Refs #2682

comment:52 Changed 9 years ago by z3p

(In [26458]) docstrings

Refs #2682

comment:53 Changed 9 years ago by z3p

Resolution: fixed
Status: newclosed

(In [26534]) Merge 'userauth-2682-7'

Author: z3p Reviewers: exarkun, radix, therve

Fixes #2682

This updates the SSHv2 user authentication framework in Conch tohave a full set of unittests.

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

#3871 is a regression introduced by this change.

comment:55 Changed 7 years ago by <automation>

Owner: z3p deleted
Note: See TracTickets for help on using tickets.