[MERGE/RFC] GSSAPI authentication support for FTP

[Martin Pool]

Martin Pool has voted resubmit.
Status is now: Resubmit
Comment:
=== added file 'bzrlib/transport/ftp/_gssapi.py'
+import base64, ftplib, getpass, socket
+

The coding standard is that the system imports should be before the 
bzrlib
imports.

+class SecureFtp(ftplib.FTP):
+    """Extended version of ftplib.FTP that can authenticate using 
GSSAPI."""

This class name is a bit generic, I would expect SecureFtp means SFTP or
maybe ftp-over-ssl.  Also there should be a blank line after the
docstring.

+    def mic_getline(self):
+        resp = ftplib.FTP.getline(self)
+        if resp[:4] != '631 ':
+            raise AssertionError
+        rc = kerberos.authGSSClientUnwrap(self.vc, 
resp[4:].strip("\r\n"))
+        response = 
base64.b64decode(kerberos.authGSSClientResponse(self.vc))
+        return response
+
+    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?


+class SecureFtpTransport(FtpTransport):
+    def _create_connection(self, credentials=None):
+        """Create a new connection with the provided credentials.
+
+        :param credentials: The credentials needed to establish the 
connection.
+
+        :return: The created connection and its associated credentials.
+
+        The credentials are only the password as it may have been 
entered
+        interactively by the user and may be different from the one 
provided
+        in base url at transport creation time.
+        """
+        if credentials is None:
+            user, password = self._user, self._password
+        else:
+            user, password = credentials
+

The docstring seems inconsistent with how `credentials` is actually
used...

Other than that it looks pretty good to me.

--
Martin


For details, see: 
http://bundlebuggy.aaronbentley.com/request/%3C1213308156.6975.30.camel%40ganieda.vernstok.nl%3E