Opened 5 years ago

Last modified 15 months ago

#5531 enhancement new

Add anonymous support to Conch SSHFactory

Reported by: Ying Li Owned by: Ying Li
Priority: normal Milestone:
Component: conch Keywords:
Cc: z3p, erikj@… Branch: branches/conch-anonymous-5531-5
branch-diff, diff-cov, branch-cov, buildbot
Author: cyli

Description

#4753 adds (or will add) cred plugins to conch, but conch doesn't support anonymous login yet.

Change History (39)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: z3p added

comment:2 Changed 5 years ago by Ying Li

Author: cyli
Branch: branches/conch-anonymous-5531

(In [33706]) Branching to 'conch-anonymous-5531'

comment:3 Changed 5 years ago by Ying Li

(In [33718]) Add support for 'none' authentication method in t.c.ssh.userauth.SSHUserAuthServer, which will allow for anonymous authentication

refs #5531

comment:4 Changed 5 years ago by Ying Li

(In [33719]) Add extra tests to cover the none authentication method case both with and without a recognized service request. Also, clean up some lines over 80, and some whitespace.

refs #5531

comment:5 Changed 5 years ago by Ying Li

In the tests, I changed the service name that is recognized by the FakeTransport's fake factory's getService method to be 'fakeservice', rather than 'none', since 'none' is also the name of the anonymous authentication method and it was getting confusing.

comment:6 Changed 5 years ago by Ying Li

Keywords: review added

comment:7 Changed 5 years ago by Ying Li

Also wanted to mention RFC 4252 section 5.2 says:

5.2.  The "none" Authentication Request

   A client may request a list of authentication 'method name' values
   that may continue by using the "none" authentication 'method name'.

   If no authentication is needed for the user, the server MUST return
   SSH_MSG_USERAUTH_SUCCESS.  Otherwise, the server MUST return
   SSH_MSG_USERAUTH_FAILURE and MAY return with it a list of methods
   that may continue in its 'authentications that can continue' value.

   This 'method name' MUST NOT be listed as supported by the server.

So it seems like returning SSH_MSG_AUTH_SUCCESS without a list of methods (if the portal supports IAnonymous, and the realm supports anonymous avatars, and the requested service is valid) seems to be valid behaviour.

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

Keywords: review removed
Owner: set to Ying Li

Thanks!

  1. While testing this out, I noticed a problem with #4753. Normally it's possible to add multiple credentials checker using the strcred Options mixin. The override of addChecker in conch means that each new --auth option overrides the previous one, so only one checker can be added. It's still possible to test the anonymous support here, but it'd be nice to support multiple checkers in conch. Can you file a ticket for fixing this?
  2. The anonymous avatarId should be twisted.cred.checkers.ANONYMOUS not a string like "anonymous".
  3. The tests could re-use twisted.cred.checkers.AllowAnonymousAccess instead of re-implementing it as twisted.conch.test.test_userauth.AnonymousChecker.
  4. You could invent a new credentials interface for UnsupportedAuthenticationChecker, instead of re-using one that perhaps could accidentally be supported someday. A one-off test-only interface would be more obviously unsupported.
  5. test_noneAuthentication_nonRecognizedService and test_noneAuthentication_recognizedService are misnamed (as are some existing tests in the module). Get rid of the second underscore.
  6. SSHUserAuthServerWithAnonTestCase.expectedAuthentications included "none", but I think according to the RFC (5.2), it MUST NOT. I'm not exactly sure what behavior this triggers in clients though, and it's hard to test because of point 1 of this review.
  7. _ebNoneNotSupported is too broad. A portal.login can fail for reasons other than the credentials object not being supported. Some checking of the exception type is merited, along with appropriate logic for different failure types.
  8. The new auth_none gets called too often, I think. Actually, potentially all of the auth_* methods might get called too often. tryAuth seems to let the client invoke whatever authentication mechanism they ask for, whether the server intends to offer it or not. A new check here, restricting the potential auth_ methods that can actually be invoked to those the server thinks the client should be allowed to invoke would be a good addition. Possibly this works as a separate ticket, since it's not really a new problem here, it's just more obvious now.
  9. There's a new, unused import of pdb at the top of userauth.py

A limitation of the SSH protocol seems to be that you can either have anonymous access or authenticated access. There seems to be no way to advertise both at the same time, nor any way to re-advertise the possibility of anonymous access after having advertised authenticated access. It seems like it might be possible to make the decision of how to proceed based on the username presented by the client (since that is provided before the critical point in the conversation), but cred makes this challenging.

Possibly some kind of pre-authenticated credentials object could be used to probe for the existence of an authentication username (log into the portal with the pre-authenticated credentials: if it succeeds, the username exists; if it fails, it does not). Then, given that information, a decision could be made about whether to allow the connection anonymously (if no such account exists) or to challenge the client to prove their identity (in the case where the account does exist). This is a little bit gross, and has a usability problem where clients attempting anonymous access may be unexpectedly challenged to prove their identity if they accidentally collide with an existing username.

Those seem like problems to solve later, though. Even being able to have an all-anonymous Conch server is a step in the right direction.

comment:9 Changed 5 years ago by Ying Li

Branch: branches/conch-anonymous-5531branches/conch-anonymous-5531-2

(In [35288]) Branching to 'conch-anonymous-5531-2'

comment:10 Changed 5 years ago by Ying Li

(1) - Done: http://twistedmatrix.com/trac/ticket/5881, and submitted for review

comment:11 Changed 5 years ago by Ying Li

(In [35298]) Commit changes from original branch refs #5531

comment:12 Changed 5 years ago by Ying Li

(In [35299]) Simple fixes like pyflakes and naming refs #5531

comment:13 Changed 5 years ago by Ying Li

Addressing some of the easier fixes:

  1. done: http://twistedmatrix.com/trac/ticket/5881, and submitted for review
  2. done.
  3. done.
  4. I interpreted "more unsupported" to be better, so I made a new one-off credentials interface in the test suite.
  5. fixed
  6. More on this later
  7. More on this later
  8. More on this later
  9. fixed

comment:14 Changed 5 years ago by Ying Li

(In [35301]) Do not return none in auth method list refs #5531

comment:15 Changed 5 years ago by Ying Li

Branch: branches/conch-anonymous-5531-2branches/conch-anonymous-5531-3

(In [35302]) Branching to 'conch-anonymous-5531-3'

comment:16 Changed 5 years ago by Ying Li

(In [35306]) Commits from previous branch refs #5531

comment:17 Changed 5 years ago by Ying Li

(In [35307]) prevent users from having both anonymous and authenticated access refs #5531

comment:18 Changed 5 years ago by Ying Li

Keywords: review added

Ok, needed to have the changes from #5881 in this, but:

In regards to point (6), I've added a new list of supported authentications that are RFC compliant, and those will be the only methods that ever get returned to the client.

In regards to point (8), I'm not sure if it was changed after this ticket was reviewed, but tryAuth checks to see if the method is supported before calling auth_none. Supported methods are built from the specified authentication methods on the portal.

In regards to point (7), since tryAuth already checks to see if the method is supported before calling it auth_*, I've just taken out _ebNoneNotSupported entirely since it seems extraneous.

comment:20 Changed 5 years ago by therve

Keywords: review removed

Not a review, but I tried using the branch, and I'm not sure what to expect. When using "twistd conch --auth=anonymous", it fails with exceptions.TypeError: must be string, not tuple because we try to pass ANONYMOUS to the UnixConchUser. OTOH, I don't know what's the expected behavior. Isn't there a missing part of configuration to tell what to do with anonymous access? Thanks!

comment:21 Changed 5 years ago by therve

Keywords: review added

comment:22 in reply to:  20 Changed 5 years ago by Ying Li

Huh... no, this is just broken. [35307] prevented me from being able to use multiple auths, which worked (well, obscured this bug)

comment:23 Changed 5 years ago by Ying Li

Keywords: review removed

comment:24 Changed 5 years ago by Ying Li

As per therve's suggestion, will just give the anonymous user access to whichever account is running conch. This doesn't seem ideal, but it seems like everything else would require a more code/complexity) and should possibly be handled in a different ticket?

comment:25 Changed 5 years ago by Ying Li

Branch: branches/conch-anonymous-5531-3branches/conch-anonymous-5531-4

(In [35773]) Branching to 'conch-anonymous-5531-4'

comment:26 Changed 5 years ago by Ying Li

(In [35774]) Commit changes from previous branch refs #5531

comment:27 Changed 5 years ago by Ying Li

(In [35775]) Set anonymous account to the account of the user running conch refs #5531

comment:28 Changed 5 years ago by Ying Li

Keywords: review added

(In 35776) skip conch unix tests if not on a posix environment #refs 5531 (this isn't from a hook - I messed up the reference)

comment:29 Changed 5 years ago by Ying Li

sudo twistd -n conch [whatever other options] --auth=anonymous: logging into this server will give you a shell as the user who started the process, which perhaps isn't the best, but perhaps alright for now.

This doesn't introduce new API that we may need to change later.

Buildbot results

comment:30 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Ying Li

Thanks. Sorry about the long review turn-around. :(

logging into this server will give you a shell as the user who started the process,

I'll note that "the user who started the process" is always root, since root is needed for request_shell to succeed. This makes me wonder if a good follow-up ticket would be an anonymous avatar type with some behavior differing from SSHSessionForUnixConchUser. I also think this means we need a warning on Conch somewhere pointing out that anonymous logins grant root shells and you might want to think twice before running one of these.

More generally, I'd like to see some howto-style documentation for using auth plugins with conch. Unfortunately there appears to be no existing howto-style documentation about the Conch server to add this to, we'll need to start a new document. :/

Code-wise:

  1. twisted/conch/test/test_unix.py introduces a duplicated skip message. Can you refactor so the message text only needs to be listed once? Bonus points if you also avoid duplicating the definition of this message with respect to other test modules.
  2. Both test methods of UnixSSHRealmTestCase are missing docstrings and not named according to the convention (test_...).
  3. RealmNoAnonymous in twisted/conch/test/test_userauth.py has a docstring that says it isn't used by the tests... This is confusing, since it is used. Can you clarify this?
  4. _checkSuccess in test_userauth.py is missing a docstring
  5. in twisted/conch/ssh/userauth.py, the docstring for _ebBadAuth needs to be updated to be correct with respect to the 'none' auth method.

There are a bunch of failures on buildbot, but I think that's just because the branch is old. Please merge forward and trigger another build before merging to trunk to make sure. After fixing the above, and if that build turns out green, please merge. Thanks!

comment:31 Changed 4 years ago by Ying Li

Branch: branches/conch-anonymous-5531-4branches/conch-anonymous-5531-5

(In [37527]) Branching to 'conch-anonymous-5531-5'

comment:32 Changed 4 years ago by Ying Li

(In [37530]) Merge forward. refs #5531

comment:33 Changed 4 years ago by Ying Li

(In [37557]) Fix docstring for mock Realm and RealmNoAnonymous. refs #5531

comment:34 Changed 4 years ago by Ying Li

(In [37561]) More docstring fixes. refs #5531

comment:35 Changed 4 years ago by Ying Li

(In [37566]) Disable the conch plugin from enabling anonymous access. refs #5531

comment:36 Changed 4 years ago by Ying Li

(In [37567]) Remove unused import. refs #5531

comment:37 Changed 4 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted

Sorry for the great delay in getting back to this ticket. As discussed with exarkun, going to explicitly not implement anonymous login via the conch tap and also UnixSSHRealm (this one was not discussed).

[buildbot results: http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/conch-anonymous-5531-5]

comment:38 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Ying Li
  1. There are missing docstrings for RealmNoAnonymous.requestAvatar and FakeUnixConchUser, and a missing blank line (see twistedchecker, ignoring the boguse epytext errors).
  2. (optional) In SSHUserAuthServer._ebBadAuth, it would be nice if there were a single log message that was a) structured b) has all the relevant information. In particular, the traceback likely should be given by log.err.
    • But this is probably beyond the scope of this ticket, since the code is merely being rearranged here.
  3. Is there any reason for rfcSupportedAuthentications to be public?
  4. There doesn't currently appear to be anything platform specific in twisted/conch/test/test_unix.py.
  5. SSHUserAuthServerBaseTestCase probably wants to be a mixin, that doesn't inherit from TestCase (so that trial doesn't see it).
  6. Sorting the authentication method in setUp feels like a code smell, to me (particularly, sorthing attributes of the objects under test). I'd guess if you order the checkers in the same order as the expected authentication things would match up.

Please merge after addressing 1 and 3-6.

comment:39 Changed 15 months ago by erikj

Cc: erikj@… added

Is this something that can be progressed? I'd quite like to see anonymous support in conch.ssh and would be happy to try and help fix this up. I'm not particularly familiar with the code, so I'm not sure how much work would be required to get this patch up to date with trunk.

Note: See TracTickets for help on using tickets.