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

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.

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

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

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

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

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

This looks basically reasonable, I think. It will be nice to have!

Some things I'd like changed/improved first, though:

 - what happens if only one of --keyfile or --certfile is given on the command line? There should probably be blackbox tests for that showing that a sensible error is given.
 - your split of classes in blackbox/test_serve.py doesn't feel right to me. Certainly it's not right for "TestBzrServeCommon" to have a "assertInetServerShutsdownCleanly" method. Perhaps put a short docstring on each class in that file explaining what aspect of the code each is responsible for, and that may make the right shape clearer.
 - I'd rather call the "bare" scenario "tcp", I think.
 - you lost the useful comments like "python versions prior to 2.6 don't have ssl ..." when refactoring that code into osutils.ssl_wrap_socket. Please reinstate them, they are quite helpful!
 - The second part of RemoteSSLTransport's docstring doesn't seem to make sense here; I think it's just copied and pasted from RemoteTCPTransport's docstring. It's probably reasonable to just delete that part of the docstring.

Also, I wonder if the url scheme this uses ("bzrs://") is the best name, and if using the same default port number is desireable? We should ask for feedback from users and the mailing list on those questions (but we can merge with the current choices initially, to help get that feedback).

Finally, it would be lovely to have some description of this in the documentation, particularly some guidance on how to generate a keyfile/certfile pair. But again, that can be added after the feature is merged.

I think using the same port is fine; after all, bzr:// doesn't look like
an ssh handshake ;)

-Rob

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Andrew Bennetts wrote:
> Review: Needs Fixing
> This looks basically reasonable, I think. It will be nice to have!
>
> Some things I'd like changed/improved first, though:
>
> - what happens if only one of --keyfile or --certfile is given on the command line? There should probably be blackbox tests for that showing that a sensible error is given.
> - your split of classes in blackbox/test_serve.py doesn't feel right to me. Certainly it's not right for "TestBzrServeCommon" to have a "assertInetServerShutsdownCleanly" method. Perhaps put a short docstring on each class in that file explaining what aspect of the code each is responsible for, and that may make the right shape clearer.
> - I'd rather call the "bare" scenario "tcp", I think.
> - you lost the useful comments like "python versions prior to 2.6 don't have ssl ..." when refactoring that code into osutils.ssl_wrap_socket. Please reinstate them, they are quite helpful!
> - The second part of RemoteSSLTransport's docstring doesn't seem to make sense here; I think it's just copied and pasted from RemoteTCPTransport's docstring. It's probably reasonable to just delete that part of the docstring.
>
> Also, I wonder if the url scheme this uses ("bzrs://") is the best name, and if using the same default port number is desireable? We should ask for feedback from users and the mailing list on those questions (but we can merge with the current choices initially, to help get that feedback).

Along these lines, would it be possible to have a STARTTLS mode instead?

I would say that if you are going to use SSL the standard way is to use
a different port, because the client needs to know *before* connecting
whether it needs to start the encryption or not.

While with TLS, you can connect to the normal channel, start encryption,
and then go with it.

It is probably trickier to hook up, but in the end you just insert
something before the 'socket.readv()' call that decrypts the data. It
certainly seems like it should be possible.

>
> Finally, it would be lovely to have some description of this in the documentation, particularly some guidance on how to generate a keyfile/certfile pair. But again, that can be added after the feature is merged.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqJXiUACgkQJdeBCYSNAANyFACfU1WBbeHJU7DHvE74nJy+ZspH
T7QAoJX1ERMMTn1luDcrPC3BV8KxyHu+
=YPTb
-----END PGP SIGNATURE-----

Thanks a lot for your review. It was very helpful. I believe have
addressed all your concerns. Branch pushed and resubmitted on LP.

Andrew Bennetts <email address hidden> writes:

> - what happens if only one of --keyfile or --certfile is given on the
> command line? There should probably be blackbox tests for that
> showing that a sensible error is given.

I extended the ui with sanity checks for these options and these are now
alsotested for in the test suite.

> - your split of classes in blackbox/test_serve.py doesn't feel right
> to me. Certainly it's not right for "TestBzrServeCommon" to have a
> "assertInetServerShutsdownCleanly" method. Perhaps put a short
> docstring on each class in that file explaining what aspect of the
> code each is responsible for, and that may make the right shape
> clearer.

Indeed, that was a refactoring brain fart. Fixed.

> - I'd rather call the "bare" scenario "tcp", I think.

I changed the names to "tcp" and "ssl/tcp".

> - you lost the useful comments like "python versions prior to 2.6
> don't have ssl ..." when refactoring that code into
> osutils.ssl_wrap_socket. Please reinstate them, they are quite
> helpful!

Done.

> - The second part of RemoteSSLTransport's docstring doesn't seem to
> make sense here; I think it's just copied and pasted from
> RemoteTCPTransport's docstring. It's probably reasonable to just
> delete that part of the docstring.

Done.

> Also, I wonder if the url scheme this uses ("bzrs://") is the best
> name, and if using the same default port number is desireable? We
> should ask for feedback from users and the mailing list on those
> questions (but we can merge with the current choices initially, to
> help get that feedback).

robertc thinks using the same port is fine; I am agnostic. I'll of
course change the scheme to anything that you guys deem more
appropriate: I was simply following the usual naming pattern for
protocols over SSL.

> Finally, it would be lovely to have some description of this in the
> documentation, particularly some guidance on how to generate a
> keyfile/certfile pair.

Done (briefly, with just a ref to an online tutorial for the last
part).

Cheers,

--Denys

John A Meinel <email address hidden> writes:

> Along these lines, would it be possible to have a STARTTLS mode
> instead?

That is what I would have preferred, but it is more difficult to hook up
because the protocol must be extended to negotiate the upgrade to an
encrypted channel. I am planning to look into it, but that's future
work.

> I would say that if you are going to use SSL the standard way is to use
> a different port, because the client needs to know *before* connecting
> whether it needs to start the encryption or not.

hmm... Don't you think that, typically, a client connects using a URL,
and that it is the scheme of that URL that indicates whether SSL must be
used or not?

In this case, the proposed scheme is bzrs://.

Cheers,

--Denys

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Denys Duchier wrote:
> John A Meinel <email address hidden> writes:
>
>> Along these lines, would it be possible to have a STARTTLS mode
>> instead?
>
> That is what I would have preferred, but it is more difficult to hook up
> because the protocol must be extended to negotiate the upgrade to an
> encrypted channel. I am planning to look into it, but that's future
> work.
>
>> I would say that if you are going to use SSL the standard way is to use
>> a different port, because the client needs to know *before* connecting
>> whether it needs to start the encryption or not.
>
> hmm... Don't you think that, typically, a client connects using a URL,
> and that it is the scheme of that URL that indicates whether SSL must be
> used or not?
>
> In this case, the proposed scheme is bzrs://.
>
> Cheers,
>
> --Denys

So all I really know is that both imaps and https use a separate port to
handle secure traffic.

I suppose it boils down to if someone would ever want to expose *both*
bzr:// *and* bzrs:// from the same machine.

For example, one could expose anonymous, read-only bzr:// and
authenticated writeable bzrs://.

Since you cannot connect to the same port with non-SSL and SSL (versus
TLS), that means you need separate ports.

Now is this use case valid? I don't really know. I certainly know a lot
of places that allow both IMAP and IMAPS, and HTTP and HTTPS from the
same server.

Also, if someone knew that a given host allowed connections, but they
weren't sure whether it was encrypted or not, you'd probably get a very
strange error if you tried "bzr log bzrs://" and it wasn't encrypted or
"bzr log bzr://" and it was. (I haven't looked at your patch, but is the
result of that connection tested?)

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkqJkX4ACgkQJdeBCYSNAAPLogCg2jjjhe3bfHG1jPYwPiuFVgZ6
XqoAn14i+/dBYjchfyFc156pAUwkKA1r
=BzQn
-----END PGP SIGNATURE-----