Merge lp:~denys.duchier/bzr/bzr.ssl into lp:~bzr/bzr/trunk-old

[Denys Duchier]

[Denys Duchier (denys.duchier)]

This branch introduces support for the bzr protocol over SSL.

Server Side
---------------

The smart server can be started with options --keyfile and --certfile to
request that client connections be SSL encrypted:

    bzr serve --keyfile FILE --certfile FILE ...

Client Side
--------------

a bzrs:// url causes the client to open an SSL connection to a bzr smart server.

[Martin Pool (mbp)]

Thanks, that's an interesting feature to add!

Does this do authentication at all, or do you plan to add it?

--
Martin <http://launchpad.net/~mbp/>

[Denys Duchier (denys.duchier)]

Martin Pool <email address hidden> writes:

> Does this do authentication at all, or do you plan to add it?

I wanted to use python's built-in SSL support. Verification of
certificates is only supported starting with python 2.6. Thus
certificate-based authentication is very easy to add, but can only be
activated for recent python distros. If there is interest, I can add
support for, it of course.

I am also interested in supporting client authentication and
authorization, but that's a more complex proposition and should be
attempted in a separate branch.

Cheers,

--Denys

[Vincent Ladeuil (vila)]

>>>>> "Denys" == Denys Duchier <email address hidden> writes:

    Denys> Martin Pool <email address hidden> writes:
    >> Does this do authentication at all, or do you plan to add it?

    Denys> I wanted to use python's built-in SSL support.
    Denys> Verification of certificates is only supported
    Denys> starting with python 2.6.

That's the plan for the http client anyway.

    Denys> Thus certificate-based authentication is very easy to
    Denys> add, but can only be activated for recent python
    Denys> distros. If there is interest, I can add support for,
    Denys> it of course.

Preliminary work on the test infrastructure is in
bzrlib/tests/ssl_certs, bzrlib/tests/https_server.py and all.

It would be nice to reuse/stay compatible with that.

It's already possible to create the key and cert files and
version the results instead of embedding opaque versions like you
did in creds.py.

    Denys> I am also interested in supporting client
    Denys> authentication and authorization, but that's a more
    Denys> complex proposition and should be attempted in a
    Denys> separate branch.

Indeed.

[Denys Duchier (denys.duchier)]

Vincent Ladeuil <email address hidden> writes:

> Preliminary work on the test infrastructure is in
> bzrlib/tests/ssl_certs, bzrlib/tests/https_server.py and all.
>
> It would be nice to reuse/stay compatible with that.
>
> It's already possible to create the key and cert files and
> version the results instead of embedding opaque versions like you
> did in creds.py.

Ah yes! I had not noticed ssl_certs. This is perfect; I'll use it
instead of my hack. Did you have more than this reuse in mind?

Cheers,

--Denys

[Vincent Ladeuil (vila)]

>>>>> "Denys" == Denys Duchier <email address hidden> writes:

    Denys> Ah yes! I had not noticed ssl_certs. This is
    Denys> perfect; I'll use it instead of my hack. Did you have
    Denys> more than this reuse in mind?

_ssl_wrap_socket in bzrlib/transport/http/_urllib2_wrappers.py
looks very much like your wrap_socket :-)

I'm pretty sure the expand* should go somewhere else, closer to
the CLI UI... i.e. higher in the stack.

Then, since this is now needed in two different places, that may
need to be moved to osutils.py instead.

Other than that, the way you reuse the tests in
blackbox/test_server.py is not exactly what we prefer :)

You want to move these tests in their own class and make them
parameterized instead.

So I'm voting resubmit (the points above are worth doing a new
submission with them fixed but the overall patch sounds good) and
with that sorted out, I'd leave it to Andrew to review the smart
server part of it.

 review: resubmit

 reviewer: spiv

review: Needs Resubmitting

[lp:~denys.duchier/bzr/bzr.ssl]

4610. By Denys Duchier on 2009-08-14

load_tests to multiply bzr tests bare and over SSL

4611. By Denys Duchier on 2009-08-17

reinstate useful comments on ssl_wrap_socket.

4612. By Denys Duchier on 2009-08-17

improved refactoring. added doc comments.

4613. By Denys Duchier on 2009-08-17

renamed: bare->tcp ssl->ssl/tcp.

4614. By Denys Duchier on 2009-08-17

prune doc string for RemoteSSLTransport.

4615. By Denys Duchier on 2009-08-17

document bzr server over SSL.

4616. By Denys Duchier on 2009-08-17

sanity check for --keyfile and --certfile options

4617. By Denys Duchier on 2009-08-17

tests for options --keyfile and --certfile

Unmerged revisions

4617. By Denys Duchier on 2009-08-17

tests for options --keyfile and --certfile

4616. By Denys Duchier on 2009-08-17

sanity check for --keyfile and --certfile options

4615. By Denys Duchier on 2009-08-17

document bzr server over SSL.

4614. By Denys Duchier on 2009-08-17

prune doc string for RemoteSSLTransport.

4613. By Denys Duchier on 2009-08-17

renamed: bare->tcp ssl->ssl/tcp.

4612. By Denys Duchier on 2009-08-17

improved refactoring. added doc comments.

4611. By Denys Duchier on 2009-08-17

reinstate useful comments on ssl_wrap_socket.

4610. By Denys Duchier on 2009-08-14

load_tests to multiply bzr tests bare and over SSL

4609. By Denys Duchier on 2009-08-14

cleaned up ssl tests

4608. By Denys Duchier on 2009-08-14

moved ssl_wrap_socket to osutils as suggested by Vincent Ladeuil

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-08-10 08:25:53 +0000
+++ bzrlib/builtins.py	2009-08-14 20:35:20 +0000
@@ -4701,6 +4701,12 @@
                     '--allow-writes enables write access to the contents of '
                     'the served directory and below.'
                 ),
+        Option('keyfile',
+               help="Path to server's private key (for SSL).",
+               type=str),
+        Option('certfile',
+               help="Path to server's certificate (for SSL).",
+               type=str),
         ]
 
     def get_host_and_port(self, port):
@@ -4723,10 +4729,14 @@
         return host, port
 
     def run(self, port=None, inet=False, directory=None, allow_writes=False,
-            protocol=None):
+            protocol=None, keyfile=None, certfile=None):
         from bzrlib.transport import get_transport, transport_server_registry
         if directory is None:
             directory = os.getcwd()
+        if keyfile:
+            keyfile = os.path.expanduser(keyfile)
+        if certfile:
+            certfile = os.path.expanduser(certfile)
         if protocol is None:
             protocol = transport_server_registry.get()
         host, port = self.get_host_and_port(port)
@@ -4734,7 +4744,7 @@
         if not allow_writes:
             url = 'readonly+' + url
         transport = get_transport(url)
-        protocol(transport, host, port, inet)
+        protocol(transport, host, port, inet, keyfile=keyfile, certfile=certfile)
 
 
 class cmd_join(Command):

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-07-23 16:01:17 +0000
+++ bzrlib/osutils.py	2009-08-14 20:35:20 +0000
@@ -1900,3 +1900,26 @@
     if use_cache:
         _cached_concurrency = concurrency
     return concurrency
+
+_ssl_wrap_socket = None
+
+def ssl_wrap_socket(sock, keyfile=None, certfile=None, server_side=False):
+    """Wrap SSL around a socket and return a SSLSocket object.
+
+    :param sock: the original socket object.
+    :param keyfile: path to the server's private key (only for a server).
+    :param certfile: path to the server's certificate (only for a server).
+    :param server_side: boolean (default False.  Pass True for a server).
+    """
+    global _ssl_wrap_socket
+    if _ssl_wrap_socket is None:
+        try:
+            import ssl
+            _ssl_wrap_socket = ssl.wrap_socket
+        except ImportError:
+            from httplib import FakeSocket
+            def _ssl_wrap_socket(sock, keyfile=None, certfile=None,
+                                 server_side=False):
+                return FakeSocket(sock, socket.ssl(sock, keyfile, certfile))
+    return _ssl_wrap_socket(sock, keyfile=keyfile, certfile=certfile,
+                            server_side=server_side)

=== modified file 'bzrlib/smart/medium.py'
--- bzrlib/smart/medium.py	2009-08-07 05:56:29 +0000
+++ bzrlib/smart/medium.py	2009-08-14 20:35:21 +0000
@@ -211,6 +211,7 @@
         # None during interpreter shutdown.
         from sys import stderr
         try:
+            self._before_serve()
             while not self.finished:
                 server_protocol = self._build_protocol()
                 self._serve_one_request(server_protocol)
@@ -218,6 +219,11 @@
             stderr.write("%s terminating on exception %s\n" % (self, e))
             raise
 
+    def _before_serve(self):
+        """Executed before the serve loop.  Maybe used to setup e.g. ssl.
+        """
+        pass
+
     def _build_protocol(self):
         """Identifies the version of the incoming request, and returns an
         a protocol object that can interpret it.
@@ -260,7 +266,8 @@
 
 class SmartServerSocketStreamMedium(SmartServerStreamMedium):
 
-    def __init__(self, sock, backing_transport, root_client_path='/'):
+    def __init__(self, sock, backing_transport, root_client_path='/',
+                 keyfile=None, certfile=None):
         """Constructor.
 
         :param sock: the socket the server will read from.  It will be put
@@ -269,8 +276,17 @@
         SmartServerStreamMedium.__init__(
             self, backing_transport, root_client_path=root_client_path)
         sock.setblocking(True)
+        self._keyfile = keyfile
+        self._certfile = certfile
         self.socket = sock
 
+    def _before_serve(self):
+        """Setup ssl if a certificate was provided."""
+        if self._certfile:
+            self.socket = osutils.ssl_wrap_socket(
+                self.socket, keyfile=self._keyfile, certfile=self._certfile,
+                server_side=True)
+
     def _serve_one_request_unguarded(self, protocol):
         while protocol.next_read_size():
             # We can safely try to read large chunks.  If there is less data
@@ -815,13 +831,14 @@
 class SmartTCPClientMedium(SmartClientStreamMedium):
     """A client medium using TCP."""
 
-    def __init__(self, host, port, base):
+    def __init__(self, host, port, base, use_ssl=False):
         """Creates a client that will connect on the first use."""
         SmartClientStreamMedium.__init__(self, base)
         self._connected = False
         self._host = host
         self._port = port
         self._socket = None
+        self._use_ssl = use_ssl
 
     def _accept_bytes(self, bytes):
         """See SmartClientMedium.accept_bytes."""
@@ -873,6 +890,9 @@
                 err_msg = err.args[1]
             raise errors.ConnectionError("failed to connect to %s:%d: %s" %
                     (self._host, port, err_msg))
+        if self._use_ssl:
+            self._socket = osutils.ssl_wrap_socket(self._socket,
+                                                   server_side=False)
         self._connected = True
 
     def _flush(self):

=== modified file 'bzrlib/smart/server.py'
--- bzrlib/smart/server.py	2009-07-20 11:27:05 +0000
+++ bzrlib/smart/server.py	2009-08-14 20:35:21 +0000
@@ -43,7 +43,7 @@
     """
 
     def __init__(self, backing_transport, host='127.0.0.1', port=0,
-                 root_client_path='/'):
+                 root_client_path='/', keyfile=None, certfile=None):
         """Construct a new server.
 
         To actually start it running, call either start_background_thread or
@@ -54,6 +54,8 @@
         :param port: TCP port to listen on, or 0 to allocate a transient port.
         :param root_client_path: The client path that will correspond to root
             of backing_transport.
+        :param keyfile: Path to server's private key (for SSL).
+        :param certfile: Path to server's certificate (for SSL).
         """
         # let connections timeout so that we get a chance to terminate
         # Keep a reference to the exceptions we want to catch because the socket
@@ -84,6 +86,8 @@
         self._started = threading.Event()
         self._stopped = threading.Event()
         self.root_client_path = root_client_path
+        self._keyfile = keyfile
+        self._certfile = certfile
 
     def serve(self, thread_name_suffix=''):
         self._should_terminate = False
@@ -150,7 +154,10 @@
 
     def get_url(self):
         """Return the url of the server"""
-        return "bzr://%s:%d/" % self._sockname
+        if self._certfile:
+            return "bzrs://%s:%d/" % self._sockname
+        else:
+            return "bzr://%s:%d/" % self._sockname
 
     def serve_conn(self, conn, thread_name_suffix):
         # For WIN32, where the timeout value from the listening socket
@@ -158,7 +165,8 @@
         conn.setblocking(True)
         conn.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
         handler = medium.SmartServerSocketStreamMedium(
-            conn, self.backing_transport, self.root_client_path)
+            conn, self.backing_transport, self.root_client_path,
+            keyfile=self._keyfile, certfile=self._certfile)
         thread_name = 'smart-server-child' + thread_name_suffix
         connection_thread = threading.Thread(
             None, handler.serve, name=thread_name)
@@ -313,7 +321,8 @@
         return transport.get_transport(url)
 
 
-def serve_bzr(transport, host=None, port=None, inet=False):
+def serve_bzr(transport, host=None, port=None, inet=False,
+              keyfile=None, certfile=None):
     from bzrlib import lockdir, ui
     from bzrlib.transport import get_transport
     from bzrlib.transport.chroot import ChrootServer
@@ -328,7 +337,8 @@
             host = medium.BZR_DEFAULT_INTERFACE
         if port is None:
             port = medium.BZR_DEFAULT_PORT
-        smart_server = SmartTCPServer(transport, host=host, port=port)
+        smart_server = SmartTCPServer(transport, host=host, port=port,
+                                      keyfile=keyfile, certfile=certfile)
         trace.note('listening on port: %s' % smart_server.port)
     # For the duration of this server, no UI output is permitted. note
     # that this may cause problems with blackbox tests. This should be

=== modified file 'bzrlib/tests/blackbox/test_serve.py'
--- bzrlib/tests/blackbox/test_serve.py	2009-07-20 11:27:05 +0000
+++ bzrlib/tests/blackbox/test_serve.py	2009-08-14 20:35:21 +0000
@@ -40,7 +40,7 @@
 from bzrlib.transport import get_transport, remote
 
 
-class TestBzrServe(TestCaseWithTransport):
+class TestBzrServeCommon(TestCaseWithTransport):
 
     def assertInetServerShutsdownCleanly(self, process):
         """Shutdown the server process looking for errors."""
@@ -71,6 +71,8 @@
         finally:
             branch.unlock()
 
+class TestBzrServeInet(TestBzrServeCommon):
+
     def start_server_inet(self, extra_options=()):
         """Start a bzr server subprocess using the --inet option.
 
@@ -90,23 +92,6 @@
         transport = remote.RemoteTransport(url, medium=client_medium)
         return process, transport
 
-    def start_server_port(self, extra_options=()):
-        """Start a bzr server subprocess.
-
-        :param extra_options: extra options to give the server.
-        :return: a tuple with the bzr process handle for passing to
-            finish_bzr_subprocess, and the base url for the server.
-        """
-        # Serve from the current directory
-        args = ['serve', '--port', 'localhost:0']
-        args.extend(extra_options)
-        process = self.start_bzr_subprocess(args, skip_if_plan_to_signal=True)
-        port_line = process.stderr.readline()
-        prefix = 'listening on port: '
-        self.assertStartsWith(port_line, prefix)
-        port = int(port_line[len(prefix):])
-        return process,'bzr://localhost:%d/' % port
-
     def test_bzr_serve_inet_readonly(self):
         """bzr server should provide a read only filesystem by default."""
         process, transport = self.start_server_inet()
@@ -124,35 +109,7 @@
         self.make_read_requests(branch)
         self.assertInetServerShutsdownCleanly(process)
 
-    def test_bzr_serve_port_readonly(self):
-        """bzr server should provide a read only filesystem by default."""
-        process, url = self.start_server_port()
-        transport = get_transport(url)
-        self.assertRaises(errors.TransportNotPossible, transport.mkdir, 'adir')
-        self.assertServerFinishesCleanly(process)
-
-    def test_bzr_serve_port_readwrite(self):
-        # Make a branch
-        self.make_branch('.')
-
-        process, url = self.start_server_port(['--allow-writes'])
-
-        # Connect to the server
-        branch = Branch.open(url)
-        self.make_read_requests(branch)
-        self.assertServerFinishesCleanly(process)
-
-    def test_bzr_serve_supports_protocol(self):
-        # Make a branch
-        self.make_branch('.')
-
-        process, url = self.start_server_port(['--allow-writes',
-                                               '--protocol=bzr'])
-
-        # Connect to the server
-        branch = Branch.open(url)
-        self.make_read_requests(branch)
-        self.assertServerFinishesCleanly(process)
+class TestBzrServeSSH(TestBzrServeCommon):
 
     def test_bzr_connect_to_bzr_ssh(self):
         """User acceptance that get_transport of a bzr+ssh:// behaves correctly.
@@ -244,6 +201,76 @@
             self.command_executed)
 
 
+class TestBzrServePort(TestBzrServeCommon):
+
+    def start_server_port(self, extra_options=()):
+        """Start a bzr server subprocess.
+
+        :param extra_options: extra options to give the server.
+        :return: a tuple with the bzr process handle for passing to
+            finish_bzr_subprocess, and the base url for the server.
+        """
+        # Serve from the current directory
+        args = ['serve', '--port', 'localhost:0']
+        args.extend(extra_options)
+        urlfmt = 'bzr://localhost:%d/'
+        if self.use_ssl:
+            from bzrlib.tests.ssl_certs import build_path
+            keyfile = build_path('server_without_pass.key')
+            certfile = build_path('server.crt')
+            args.extend(("--keyfile", keyfile,
+                         "--certfile", certfile))
+            urlfmt = 'bzrs://localhost:%d/'
+        process = self.start_bzr_subprocess(args, skip_if_plan_to_signal=True)
+        port_line = process.stderr.readline()
+        prefix = 'listening on port: '
+        self.assertStartsWith(port_line, prefix)
+        port = int(port_line[len(prefix):])
+        return process, urlfmt % port
+
+    def test_bzr_serve_port_readonly(self):
+        """bzr server should provide a read only filesystem by default."""
+        process, url = self.start_server_port()
+        transport = get_transport(url)
+        self.assertRaises(errors.TransportNotPossible, transport.mkdir, 'adir')
+        self.assertServerFinishesCleanly(process)
+
+    def test_bzr_serve_port_readwrite(self):
+        # Make a branch
+        self.make_branch('.')
+
+        process, url = self.start_server_port(['--allow-writes'])
+
+        # Connect to the server
+        branch = Branch.open(url)
+        self.make_read_requests(branch)
+        self.assertServerFinishesCleanly(process)
+
+    def test_bzr_serve_supports_protocol(self):
+        # Make a branch
+        self.make_branch('.')
+
+        process, url = self.start_server_port(['--allow-writes',
+                                               '--protocol=bzr'])
+
+        # Connect to the server
+        branch = Branch.open(url)
+        self.make_read_requests(branch)
+        self.assertServerFinishesCleanly(process)
+
+def serve_port_scenarios():
+    return (('bare', dict(use_ssl=False)),
+            ('ssl' , dict(use_ssl=True)))
+
+def load_tests(basic_tests, module, loader):
+    from bzrlib import tests
+    suite = loader.suiteClass()
+    serve_port_tests, remaining_tests = tests.split_suite_by_condition(
+        basic_tests, tests.condition_isinstance(TestBzrServePort))
+    tests.multiply_tests(serve_port_tests, serve_port_scenarios(), suite)
+    suite.addTest(remaining_tests)
+    return suite
+
 class TestCmdServeChrooting(TestCaseWithTransport):
 
     def test_serve_tcp(self):

=== modified file 'bzrlib/transport/__init__.py'
--- bzrlib/transport/__init__.py	2009-08-04 11:40:59 +0000
+++ bzrlib/transport/__init__.py	2009-08-14 20:35:21 +0000
@@ -1826,9 +1826,14 @@
 register_transport_proto('bzr://',
             help="Fast access using the Bazaar smart server.",
                          register_netloc=True)
+register_transport_proto('bzrs://',
+            help="Fast access using the Bazaar smart server over SSL.",
+                         register_netloc=True)
 
 register_lazy_transport('bzr://', 'bzrlib.transport.remote',
                         'RemoteTCPTransport')
+register_lazy_transport('bzrs://', 'bzrlib.transport.remote',
+                        'RemoteSSLTransport')
 register_transport_proto('bzr-v2://', register_netloc=True)
 
 register_lazy_transport('bzr-v2://', 'bzrlib.transport.remote',

=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2009-05-04 15:21:26 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2009-08-14 20:35:21 +0000
@@ -280,19 +280,6 @@
         self._wrap_socket_for_reporting(self.sock)
 
 
-# Build the appropriate socket wrapper for ssl
-try:
-    # python 2.6 introduced a better ssl package
-    import ssl
-    _ssl_wrap_socket = ssl.wrap_socket
-except ImportError:
-    # python versions prior to 2.6 don't have ssl and ssl.wrap_socket instead
-    # they use httplib.FakeSocket
-    def _ssl_wrap_socket(sock, key_file, cert_file):
-        ssl_sock = socket.ssl(sock, key_file, cert_file)
-        return httplib.FakeSocket(sock, ssl_sock)
-
-
 class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection):
 
     def __init__(self, host, port=None, key_file=None, cert_file=None,
@@ -313,7 +300,8 @@
             self.connect_to_origin()
 
     def connect_to_origin(self):
-        ssl_sock = _ssl_wrap_socket(self.sock, self.key_file, self.cert_file)
+        ssl_sock = osutils.ssl_wrap_socket(
+            self.sock, self.key_file, self.cert_file)
         # Wrap the ssl socket before anybody use it
         self._wrap_socket_for_reporting(ssl_sock)
 

=== modified file 'bzrlib/transport/remote.py'
--- bzrlib/transport/remote.py	2009-03-24 01:53:42 +0000
+++ bzrlib/transport/remote.py	2009-08-14 20:35:21 +0000
@@ -485,6 +485,19 @@
         return client_medium, None
 
 
+class RemoteSSLTransport(RemoteTransport):
+    """Connection to smart server over ssl.
+
+    This is essentially just a factory to get 'RemoteTransport(url,
+        SmartTCPClientMedium).
+    """
+
+    def _build_medium(self):
+        client_medium = medium.SmartTCPClientMedium(
+            self._host, self._port, self.base, use_ssl=True)
+        return client_medium, None
+
+
 class RemoteTCPTransportV2Only(RemoteTransport):
     """Connection to smart server over plain tcp with the client hard-coded to
     assume protocol v2 and remote server version <= 1.6.