[MERGE/RFC] GSSAPI authentication support for FTP

[Vincent Ladeuil]

>>>>> "Jelmer" == Jelmer Vernooij <jelmer at samba.org> writes:

    Jelmer> The attached patch adds support for GSSAPI
    Jelmer> authentication and encryption in the FTP transport,
    Jelmer> as documented in RFC2228.

Nice.

    Jelmer> GSSAPI is mainly used for Kerberos.

    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 ?

    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/>

    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 ?

    Jelmer> +        wrapped = kerberos.authGSSClientResponse(self.vc)
    Jelmer> +        ftplib.FTP.putcmd(self, "MIC " + wrapped)
    Jelmer> +
    Jelmer> +    def mic_getline(self):
    Jelmer> +        resp = ftplib.FTP.getline(self)
    Jelmer> +        assert resp[:4] == '631 '
    Jelmer> +        rc = kerberos.authGSSClientUnwrap(self.vc, resp[4:].strip("\r\n"))
    Jelmer> +        response = base64.b64decode(kerberos.authGSSClientResponse(self.vc))
    Jelmer> +        return response
    Jelmer> +
    Jelmer> +    def gssapi_login(self, user):
    Jelmer> +        # Try GSSAPI login first
    Jelmer> +        resp = self.sendcmd('AUTH GSSAPI')
    Jelmer> +        if resp[:3] == '334':
    Jelmer> +            rc, self.vc = kerberos.authGSSClientInit("ftp@%s" % self.host)
    Jelmer> +
    Jelmer> +            if kerberos.authGSSClientStep(self.vc, "") != 1:
    Jelmer> +                while resp[:3] in ('334', '335'):
    Jelmer> +                    authdata = kerberos.authGSSClientResponse(self.vc)
    Jelmer> +                    resp = self.sendcmd('ADAT ' + authdata)
    Jelmer> +                    if resp[:9] in ('235 ADAT=', '335 ADAT='):
    Jelmer> +                        rc = kerberos.authGSSClientStep(self.vc, resp[9:])
    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 ?).


    Jelmer> +            info("Authenticated as %s" % kerberos.authGSSClientUserName(
    Jelmer> +                    self.vc))
    Jelmer> +
    Jelmer> +            # Monkey patch ftplib
    Jelmer> +            self.putcmd = self.mic_putcmd
    Jelmer> +            self.getline = self.mic_getline
    Jelmer> +
    Jelmer> +            self.sendcmd('USER ' + user)
    Jelmer> +            return resp
    Jelmer> +
    Jelmer> +
    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 class ?

<snip/>

    Jelmer> +def get_test_permutations():
    Jelmer> +    """Return the permutations to be used in testing."""
    Jelmer> +    from bzrlib import tests
    Jelmer> +    if tests.FTPServerFeature.available():
    Jelmer> +        from bzrlib.tests import ftp_server
    Jelmer> +        return [(SecureFtpTransport, ftp_server.FTPServer)]
    Jelmer> +    else:
    Jelmer> +        # Dummy server to have the test suite report the number of tests
    Jelmer> +        # needing that feature. We raise UnavailableFeature from methods before
    Jelmer> +        # the test server is being used. Doing so in the setUp method has bad
    Jelmer> +        # side-effects (tearDown is never called).
    Jelmer> +        class UnavailableFTPServer(object):
    Jelmer> +
    Jelmer> +            def setUp(self):
    Jelmer> +                pass
    Jelmer> +
    Jelmer> +            def tearDown(self):
    Jelmer> +                pass
    Jelmer> +
    Jelmer> +            def get_url(self):
    Jelmer> +                raise tests.UnavailableFeature(tests.FTPServerFeature)
    Jelmer> +
    Jelmer> +            def get_bogus_url(self):
    Jelmer> +                raise tests.UnavailableFeature(tests.FTPServerFeature)
    Jelmer> +
    Jelmer> +        return [(FtpTransport, UnavailableFTPServer)]

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

Hth,

        Vincent