[MERGE/RFC] GSSAPI authentication support for FTP

[Jelmer Vernooij]

Am Donnerstag, den 12.06.2008, 23:17 +0200 schrieb Vincent Ladeuil:
> >>>>> "Jelmer" == Jelmer Vernooij <jelmer at samba.org> writes:

>     Jelmer> The new transport class builds on top of the existing one and will try
>     Jelmer> secure authentication before any other authentication. If GSSAPI is
>     Jelmer> not available, it will also fall back to regular authentication.
> 
>     Jelmer> The existing FTP tests are being run against the new
>     Jelmer> secure FTP server already.
> 
> But since medusa doesn't support GSSAPI, you're testing only the
> fallback mechanism here, isn't it ?
Yep, that's right.

>     Jelmer> However, I couldn't find a good way to add tests for
>     Jelmer> the GSSAPI authentication in medusa, which seems to
>     Jelmer> be written around the idea that everything has a
>     Jelmer> username and password.
> 
>     Jelmer> What amount of testing is required here? Would it be
>     Jelmer> sufficient if I just tested that SecureFtp's methods
>     Jelmer> attempt to send the right data?
> 
> If you have setup such a server locally, you may be able to
> automate the tests by using
> https://code.launchpad.net/~vila/bzr/local-test-server and adding
> the missing bits for your server and be able to exercise your
> implementation against the real server.
> 
> That will not be the same thing than providing the test in bzr
> itself, but at least anyone will be able to reproduce your tests.
> 
> <snip/>
Thanks, I wasn't aware of that plugin! It'd certainly be interesting for
bzr-svn testing. 

However, I'm not sure bootstrapping a Kerberos server is a good idea :-)

>     Jelmer> +class SecureFtp(ftplib.FTP):
>     Jelmer> +    """Extended version of ftplib.FTP that can authenticate using GSSAPI."""
>     Jelmer> +    def mic_putcmd(self, line):
>     Jelmer> +        rc = kerberos.authGSSClientWrap(self.vc, 
>     Jelmer> +            base64.b64encode(line), 'jelmer at VERNSTOK.NL')
> 
> Really ?
Whoops, fixed now :-)


>     Jelmer> +                        assert ((resp[:3] == '235' and rc == 1) or 
>     Jelmer> +                                (resp[:3] == '335' and rc == 0))
> 
> raise AssertionError is preferred to stay active while running
> under python -o (I thought the bzr test suite catched that ?).
Fixed, thanks.

>     Jelmer> +class SecureFtpTransport(FtpTransport):
>     Jelmer> +    def _create_connection(self, credentials=None):
>     Jelmer> +        """Create a new connection with the provided credentials.
>     Jelmer> +
>     Jelmer> +        :param credentials: The credentials needed to establish the connection.
>     Jelmer> +
>     Jelmer> +        :return: The created connection and its associated credentials.
>     Jelmer> +
>     Jelmer> +        The credentials are only the password as it may have been entered
>     Jelmer> +        interactively by the user and may be different from the one provided
>     Jelmer> +        in base url at transport creation time.
>     Jelmer> +        """
>     Jelmer> +        if credentials is None:
>     Jelmer> +            user, password = self._user, self._password
>     Jelmer> +        else:
>     Jelmer> +            user, password = credentials
>     Jelmer> +
>     Jelmer> +        auth = config.AuthenticationConfig()
>     Jelmer> +        if user is None:
>     Jelmer> +            user = auth.get_user('ftp', self._host, port=self._port)
>     Jelmer> +            if user is None:
>     Jelmer> +                # Default to local user
>     Jelmer> +                user = getpass.getuser()
>     Jelmer> +
>     Jelmer> +        mutter("Constructing FTP instance against %r" %
>     Jelmer> +               ((self._host, self._port, user, '********',
>     Jelmer> +                self.is_active),))
>     Jelmer> +        try:
>     Jelmer> +            connection = SecureFtp()
> 
> A lot of duplication to be able to insert that one ? Why not add
> a _connection_class attribute to the base clas ?
It's not just that - it's also the login logic. the only other thing
that could be abstracted away is the handling of the socket exceptions.

> The dummy server was added to provide feedback about the skipped
> tests when medusa wasn't available, I'm not sure the cost of
> duplicating the code is worth the benefit of doubling the
> reported skipped tests here...
Makes sense - I've removed it here.

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-krb5.diff
Type: text/x-patch
Size: 17594 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080613/5b546855/attachment-0001.bin