[MERGE] GSSAPI authentication support for FTP

[Jelmer Vernooij]

Thanks for reviewing. 

Am Donnerstag, den 17.07.2008, 17:20 -0500 schrieb John Arbash Meinel:
> 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 ?
Since GSSAPI is used on top of ftplib, it seems rather strange to make
 ftp+ftplib not use GSSAPI. I've added a ftp+nogssapi that doesn't use GSSAPI.

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

> +        # 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.
I've now documented the FTP response codes that are being used.

> 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
The reason I used resp[:3] is because that is what is used in ftplib
 everywhere. I'd prefer to stay consistent with that, unless you insist.

> +                        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).
Fixed, I'm now raising ftplib.error_resp, which is used in ftplib
generally for unexpected responses.

> 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.
Yeah - ftp is the service name. It's common practice in the Kerberos
world to have that hardcoded. Every server out there will be using that
principal.

> +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.
Fixed.

Cheers,

Jelmer
-- 
Jelmer Vernooij <jelmer at samba.org> - http://samba.org/~jelmer/
Jabber: jelmer at jabber.fsfe.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bzr-ftp-gssapi.diff
Type: text/x-patch
Size: 21182 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080828/ac261bf0/attachment-0001.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 315 bytes
Desc: Dies ist ein digital signierter Nachrichtenteil
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080828/ac261bf0/attachment-0001.pgp