[MERGE] GSSAPI authentication support for FTP

[John Arbash Meinel]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

It seems LarstiQ was at lunch for a *long* time, so here goes:

Jelmer Vernooij wrote:
| Am Mittwoch, den 02.07.2008, 01:26 -0400 schrieb Martin Pool:
|> Martin Pool has voted resubmit.
|> Status is now: Resubmit
|
|> +
|> +    def gssapi_login(self, user):
|> +        # Try GSSAPI login first
|> +        resp = self.sendcmd('AUTH GSSAPI')
|> +        if resp[:3] == '334':
|> +            rc, self.vc = kerberos.authGSSClientInit("ftp@%s" %
|> self.host)
|> +            if kerberos.authGSSClientStep(self.vc, "") != 1:
|> +                while resp[:3] in ('334', '335'):
|> +                    authdata = kerberos.authGSSClientResponse(self.vc)
|> +                    resp = self.sendcmd('ADAT ' + authdata)
|> +                    if resp[:9] in ('235 ADAT=', '335 ADAT='):
|> +                        rc = kerberos.authGSSClientStep(self.vc,
|> resp[9:])
|> +                        if not ((resp[:3] == '235' and rc == 1) or
|> +                                (resp[:3] == '335' and rc == 0)):
|> +                            raise AssertionError
|> +            info("Authenticated as %s" %
|> kerberos.authGSSClientUserName(
|> +                    self.vc))
|> +
|> +            # Monkey patch ftplib
|> +            self.putcmd = self.mic_putcmd
|> +            self.getline = self.mic_getline
|> +            self.sendcmd('USER ' + user)
|> +            return resp
|>
|> When you get something unexpected back it would be nice to raise an
|> error
|> that contains e.g. resp so that if a user hits it we have some data to
|> go
|> on - and anyhow the message might help them understand what's going
|> wrong.
|> Perhaps a generic TransportError would be better than AssertionError.
|>
|> What happens if you get a response other than 334?  I would guess
|> there's
|> one that means 'not supported' which you should ignore but maybe the
|> others should at least give a warning?
| I'd rather not warn since I suspect different servers to react quite
| differently to this despite the standard. I've added a mutter for now,
| since I suspect a warning may be annoying to the majority of users of
| the ftp protcol. If it turns out GSSAPI doesn't work unexpectedly for
| people, we can always convert it into a warning.

The one thing I would request is a way to *not* use ftp+gssapi. Right
now you are registering it as a handler for plain "ftp://" whenever the
'kerberos' library is available.

For http, you can use http+urllib / http+pycurl. I guess the best thing
here would be to add ftp+ftplib ?


I might also extend this error:
+if getattr(kerberos, "authGSSClientWrap", None) is None:
+    raise errors.DependencyNotPresent('kerberos',
+                                      "missing encryption functions")

"missing encryption functions authGSSClientWrap".

+        # Try GSSAPI login first
+        resp = self.sendcmd('AUTH GSSAPI')
+        if resp[:3] == '334':
+            rc, self.vc = kerberos.authGSSClientInit("ftp@%s" % self.host)

^- I would really like to see those have better documentation about what
"334" means.
Also, if the response was "3344" this would hit. Is it better to do:
resp[:4] == '334 '

Regardless, the better thing to do (python wise) is:

if resp.startswith('334'):

I think you should generally switch things to:

if resp.startswith('SOME STRING'), you have a lot of places that are
hard to maintain if a string changes size slightly. And they all should
have at least a little bit of documentation as to what is going on. Like
# 235 server wants more FOO


+                        if not ((resp[:3] == '235' and rc == 1) or
+                                (resp[:3] == '335' and rc == 0)):
+                            raise AssertionError

^- This sounds like a real interaction issue. And not just a plain
"Assertion". At the minimum, you should have:

raise AssertionError('server gave an invalid response: %s' % resp).

But honestly, it seems like you need a real exception here, not just
AssertionError.


Also, is 'ftp at host' always the correct client string? It at least looks
a lot like a user at host sort of key. But I don't know much about kerberos
& ftp.


+class GSSAPIFtpTransport(FtpTransport):
+    def _create_connection(self, credentials=None):

^- You should really have a docstring, and at a minimum, you need a
blank space here.

+        auth = config.AuthenticationConfig()
+        if user is None:
+            user = auth.get_user('ftp', self._host, port=self._port)
+            if user is None:
+                # Default to local user
+                user = getpass.getuser()

^- Should we default to local user, or should we default to 'anonymous'?



It would also be nice to have at least 1 direct test that the
GSSAPIFtpTransport tries to gssapi_login when it goes to _create_connection.

You could make GSSAPIFtp a helper class on GSSAPIFtpTransport and
override it with a logging class in the test suite.

BB:resubmit

John
=:->

|
| [...]
|
|> Other than that it looks pretty good to me.
|
| Thanks. I've attached an updated patch that fixes the issues you
| mentioned.
|
| Cheers,
|
| Jelmer
|

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkh/xbYACgkQJdeBCYSNAAPLzwCbBQqW/hYMYxUH9XNopke4xTRy
QaQAn2FW2QwzNA7WukqBx4j3DRlVpbJ7
=zCH/
-----END PGP SIGNATURE-----