Merge lp:~denys.duchier/bzr/bzr.ssl into lp:~bzr/bzr/trunk-old
- bzr.ssl
- Merge into trunk-old
Status: | Work in progress |
---|---|
Proposed branch: | lp:~denys.duchier/bzr/bzr.ssl |
Merge into: | lp:~bzr/bzr/trunk-old |
Diff against target: | 581 lines |
To merge this branch: | bzr merge lp:~denys.duchier/bzr/bzr.ssl |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool | Needs Information | ||
John A Meinel | Needs Information | ||
Robert Collins (community) | Approve | ||
Review via email: mp+10254@code.launchpad.net |
This proposal supersedes a proposal from 2009-08-14.
Commit message
Description of the change
Denys Duchier (denys.duchier) wrote : Posted in a previous version of this proposal | # |
Martin Pool (mbp) wrote : Posted in a previous version of this proposal | # |
Thanks, that's an interesting feature to add!
Does this do authentication at all, or do you plan to add it?
--
Martin <http://
Denys Duchier (denys.duchier) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
>>>>> "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/
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) wrote : Posted in a previous version of this proposal | # |
Vincent Ladeuil <email address hidden> writes:
> Preliminary work on the test infrastructure is in
> bzrlib/
>
> 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) wrote : Posted in a previous version of this proposal | # |
>>>>> "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/
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/
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
Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal | # |
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/
- 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.
- The second part of RemoteSSLTransp
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.
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal | # |
I think using the same port is fine; after all, bzr:// doesn't look like
an ssh handshake ;)
-Rob
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal | # |
-----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/
> - 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.
> - The second part of RemoteSSLTransp
>
> 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://
iEYEARECAAYFAkq
T7QAoJX1ERMMTn1
=YPTb
-----END PGP SIGNATURE-----
Denys Duchier (denys.duchier) wrote : Posted in a previous version of this proposal | # |
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/
> to me. Certainly it's not right for "TestBzrServeCo
> "assertInetServ
> 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.
> helpful!
Done.
> - The second part of RemoteSSLTransp
> make sense here; I think it's just copied and pasted from
> RemoteTCPTransp
> 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
Denys Duchier (denys.duchier) wrote : Posted in a previous version of this proposal | # |
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
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal | # |
-----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://
iEYEARECAAYFAkq
XqoAn14i+
=BzQn
-----END PGP SIGNATURE-----
Robert Collins (lifeless) wrote : | # |
This looks broadly ok.
I wonder if it would be cleaner to pass in an object to run before_
That would appear to be more generically useful to me.
Secondly, perhaps bzr+ssl would be less likely to have people make typographical errors during copy and paste and so forth.
John A Meinel (jameinel) wrote : | # |
So, IIRC, my personal concern was that we shouldn't re-use the same port for bzr:// and bzrs:// access. (Note that we could use bzr+ssl:// but that is going to be *very* close to bzr+ssh://...)
If we want to re-use the port, then we should be supporting STARTTLS, otherwise we should use 2 ports.
I realize this isn't strictly required, but it seems to be a fairly standard way of handling ssl versus non-ssl access. (Consider http vs https, imap vs imaps)
Robert is the URL guru, though. So if he feels strongly that this is ok, then we can probably just do it.
John A Meinel (jameinel) wrote : | # |
So, IIRC, my personal concern was that we shouldn't re-use the same port for bzr:// and bzrs:// access. (Note that we could use bzr+ssl:// but that is going to be *very* close to bzr+ssh://...)
If we want to re-use the port, then we should be supporting STARTTLS, otherwise we should use 2 ports.
I realize this isn't strictly required, but it seems to be a fairly standard way of handling ssl versus non-ssl access. (Consider http vs https, imap vs imaps)
Robert is the URL guru, though. So if he feels strongly that this is ok, then we can probably just do it.
Martin Pool (mbp) wrote : | # |
Hi Denys,
Thanks for your patch.
If you have not yet executed our contributor agreement <https:/
I would probably prefer bzr+ssl too, especially because we generally have quite explicit url schemes already (bzr+ssh etc).
I agree with John that we should consider the consequences of reusing the same port for encrypted and non-encrypted access. In particular if someone tries to use bzr:// against an ssl port or vice versa, will they get a reasonable message or a confusing one? If it's not reasonably smooth, or plausibly able to be made smooth, then we could either use a different port or use a special command to start encryption.
I suspect if we do add this, the next thing people will want is to do inline authentication over that socket, which probably requires having commands to do so.
This should be mentioned in NEWS.
Martin Pool (mbp) wrote : | # |
Hi Denys,
I'm the patch pilot this week and I'd like to help you get your change into bzr. Are you feeling blocked on any of the items mentioned in this review, or is there anything I can do to help you?
Denys Duchier (denys.duchier) wrote : | # |
> I'm the patch pilot this week and I'd like to help you get your change into
> bzr. Are you feeling blocked on any of the items mentioned in this review, or
> is there anything I can do to help you?
I contributed when I had time, which was in august, 4 months ago. The academic term only
ended this friday, and I have now several big piles of exams to grade.
As for the contributor's agreement: there is no way that I can agree to clause 5. Not only
am I giving away all rights to my work to canonical, but then I should also forever in the future
sign and do whatever canonical deems necessary. that's just unacceptable. also impractical.
no amounts of reassurance can counterbalance the excessive broadness of these terms. especially
after we have seen with the rebranding of bzr as a canonical project just how much canonical
cared about the community of contributors. clearly, we are drones of no consequence. and in
this light, clause 5 looks like a particularly dangerous deal to enter into.
I am also not happy at all about clause 6. my contributions are made with the understanding,
or so I thought, that they are and will remain free software.
Martin Pool (mbp) wrote : | # |
Hi,
I'd like to apologize for this patch taking so long to be resolved. We have had a bad problem with things stalling in the past, and we are now trying to clear out our backlog and to be more expeditious in future. I'm willing to write tests or finish off patches and I've been doing that this week.
We have had feedback from several people that those clauses are over broad either intentionally or because of poor phrasing, and I do see what you mean and see it as a problem. We are working towards using a better text soon.
Balancing the interests of Canonical and of other community members is not easy but we are trying.
Unfortunately we can't merge this patch without assignment.
Denys Duchier (denys.duchier) wrote : | # |
Martin Pool <email address hidden> writes:
> The proposal to merge lp:~denys.duchier/bzr/bzr.ssl into lp:~bzr/bzr/trunk has been updated.
>
> Status: Needs review => Rejected
This says everything that I need to know about canonical's actual
willingness to get anything resolved.
I used to be a fan. what a waste. your choice.
--Denys
Martin Pool (mbp) wrote : | # |
2009/12/23 Denys Duchier <email address hidden>:
> Martin Pool <email address hidden> writes:
>
>> The proposal to merge lp:~denys.duchier/bzr/bzr.ssl into lp:~bzr/bzr/trunk has been updated.
>>
>> Â Â Status: Needs review => Rejected
>
> This says everything that I need to know about canonical's actual
> willingness to get anything resolved.
I didn't mean to say the conversation is over.
I would much rather get this resolved than reject it.
Please tell me what you think we should be doing?
--
Martin <http://
Denys Duchier (denys.duchier) wrote : | # |
Martin Pool <email address hidden> writes:
> I would much rather get this resolved than reject it.
That would have been much more believable had your very first move not
been to reject it.
> Please tell me what you think we should be doing?
You know very well the answer to that. I was quite clear about my
objections. Others have raised the very same objections before... with
just as little success.
--Denys
Martin Pool (mbp) wrote : | # |
> That would have been much more believable had your very first move not been to reject it.
Sometimes we prematurely or incorrectly set bugs to Wontfix, Invalid or Incomplete. This isn't meant to show any disrespect for the bug report or the reporter, or to shut down the discussion. We just try to triage boldly rather than leaving bugs or reviews noncommittal. If the status is actually wrong, it's easily reverted, and you know there have been many dozens of bugs and reviews that have flipped state.
I didn't mean to shut down the conversation; I appreciate your contribution both technically and at higher levels; I can see that flipping the state looked preemptory and I apologize for that.
My only point was that there was no point doing any more technical _code review_ as such at the moment.
> I was quite clear about my objections.
About the agreement text:
The point of clause 5 is that people will, for example, sign a legal deposition (at Canonical's expense, note) to help us defend the copyright if that proves necessary, not that they will do arbitrarily future acts. My personal (ianal) opinion is that anything we need people to do, we should do atomically with them making a particular contribution, and then not ask for or count on anything more in the future.
Clause 6: Bazaar will always be free, but we do want to retain the option to defend its copyright/patent status, to adapt to changes in free software licence practice, and to give a proprietary commercial licence for GPL-averse users. We could state this more clearly. I realize not everyone will agree with dual-licencing but to me it seems like a nice consequence of the GPL, modeled fairly successfully by other companies. (We have not actually done any of these yet.)
I think it is reasonable to be concerned about the broadness of these clauses and it would be better if the agreement were more clear. However, I can't rewrite the agreement myself. I am talking to people at Canonical about it and we are working with external laywers. This takes some time.
I think there are lots of people who will sign a reasonable assignment agreement but who won't sign an overbroad or vague one. In other contexts, I am one myself. I really do want to get this right and I'm working on it.
Denys Duchier (denys.duchier) wrote : | # |
Martin Pool <email address hidden> writes:
> My personal (ianal) opinion is that anything we need people to do, we
> should do atomically with them making a particular contribution, and
> then not ask for or count on anything more in the future.
I concur.
> Clause 6: Bazaar will always be free, but we do want to retain the
> option to defend its copyright/patent status, to adapt to changes in
> free software licence practice, and to give a proprietary commercial
> licence for GPL-averse users. We could state this more clearly. I
> realize not everyone will agree with dual-licencing but to me it seems
> like a nice consequence of the GPL, modeled fairly successfully by
> other companies. (We have not actually done any of these yet.)
It should be stated clearly. I am fine with dual-licensing; that's a
fair deal for supporting the development of free software.
> I think there are lots of people who will sign a reasonable assignment
> agreement but who won't sign an overbroad or vague one.
so do I. I suspect that what is taking time is clause 7, which isn't
particularly relevant for most people. why not publish an updated
version of the agreement, with clauses 5 & 6 fixed, and allow
contributors to update their earlier agreement by signing a new version
whenever one is published if they so choose? Such an arrangement would
be future-proof and would permit greater reactivity through small
incremental improvements.
--Denys
Unmerged revisions
- 4617. By Denys Duchier
-
tests for options --keyfile and --certfile
- 4616. By Denys Duchier
-
sanity check for --keyfile and --certfile options
- 4615. By Denys Duchier
-
document bzr server over SSL.
- 4614. By Denys Duchier
-
prune doc string for RemoteSSLTransport.
- 4613. By Denys Duchier
-
renamed: bare->tcp ssl->ssl/tcp.
- 4612. By Denys Duchier
-
improved refactoring. added doc comments.
- 4611. By Denys Duchier
-
reinstate useful comments on ssl_wrap_socket.
- 4610. By Denys Duchier
-
load_tests to multiply bzr tests bare and over SSL
- 4609. By Denys Duchier
-
cleaned up ssl tests
- 4608. By Denys Duchier
-
moved ssl_wrap_socket to osutils as suggested by Vincent Ladeuil
Preview Diff
1 | === modified file 'bzrlib/builtins.py' |
2 | --- bzrlib/builtins.py 2009-08-28 05:00:33 +0000 |
3 | +++ bzrlib/builtins.py 2009-08-31 04:35:59 +0000 |
4 | @@ -4726,6 +4726,12 @@ |
5 | '--allow-writes enables write access to the contents of ' |
6 | 'the served directory and below.' |
7 | ), |
8 | + Option('keyfile', |
9 | + help="Path to server's private key (for SSL).", |
10 | + type=str), |
11 | + Option('certfile', |
12 | + help="Path to server's certificate (for SSL).", |
13 | + type=str), |
14 | ] |
15 | |
16 | def get_host_and_port(self, port): |
17 | @@ -4748,10 +4754,32 @@ |
18 | return host, port |
19 | |
20 | def run(self, port=None, inet=False, directory=None, allow_writes=False, |
21 | - protocol=None): |
22 | + protocol=None, keyfile=None, certfile=None): |
23 | from bzrlib.transport import get_transport, transport_server_registry |
24 | if directory is None: |
25 | directory = os.getcwd() |
26 | + if keyfile: |
27 | + if not certfile: |
28 | + raise errors.BzrCommandError( |
29 | + "with --keyfile you must also supply --certfile") |
30 | + keyfile = os.path.expanduser(keyfile) |
31 | + txt = open(keyfile).read() |
32 | + if txt.find("-----BEGIN RSA PRIVATE KEY-----") < 0: |
33 | + raise errors.BzrCommandError( |
34 | + "%s does not contain a private key in PEM format" % keyfile) |
35 | + txt = None |
36 | + if certfile: |
37 | + certfile = os.path.expanduser(certfile) |
38 | + txt = open(certfile).read() |
39 | + if txt.find("-----BEGIN CERTIFICATE-----") < 0: |
40 | + raise errors.BzrCommandError( |
41 | + "%s does not contain a certificate in PEM format" |
42 | + % certfile) |
43 | + if not keyfile and txt.find("-----BEGIN RSA PRIVATE KEY-----") < 0: |
44 | + raise errors.BzrCommandError( |
45 | + "either you must supply --keyfile or the" |
46 | + " certificate file must also contain the private key") |
47 | + txt = None |
48 | if protocol is None: |
49 | protocol = transport_server_registry.get() |
50 | host, port = self.get_host_and_port(port) |
51 | @@ -4759,7 +4787,7 @@ |
52 | if not allow_writes: |
53 | url = 'readonly+' + url |
54 | transport = get_transport(url) |
55 | - protocol(transport, host, port, inet) |
56 | + protocol(transport, host, port, inet, keyfile=keyfile, certfile=certfile) |
57 | |
58 | |
59 | class cmd_join(Command): |
60 | |
61 | === modified file 'bzrlib/osutils.py' |
62 | --- bzrlib/osutils.py 2009-07-23 16:01:17 +0000 |
63 | +++ bzrlib/osutils.py 2009-08-31 04:35:59 +0000 |
64 | @@ -1900,3 +1900,29 @@ |
65 | if use_cache: |
66 | _cached_concurrency = concurrency |
67 | return concurrency |
68 | + |
69 | +_ssl_wrap_socket = None |
70 | + |
71 | +def ssl_wrap_socket(sock, keyfile=None, certfile=None, server_side=False): |
72 | + """Wrap SSL around a socket and return a SSLSocket object. |
73 | + |
74 | + :param sock: the original socket object. |
75 | + :param keyfile: path to the server's private key (only for a server). |
76 | + :param certfile: path to the server's certificate (only for a server). |
77 | + :param server_side: boolean (default False. Pass True for a server). |
78 | + """ |
79 | + global _ssl_wrap_socket |
80 | + if _ssl_wrap_socket is None: |
81 | + try: |
82 | + # python 2.6 introduced a better ssl package |
83 | + import ssl |
84 | + _ssl_wrap_socket = ssl.wrap_socket |
85 | + except ImportError: |
86 | + # python versions prior to 2.6 don't have ssl and ssl.wrap_socket |
87 | + # instead they use httplib.FakeSocket |
88 | + from httplib import FakeSocket |
89 | + def _ssl_wrap_socket(sock, keyfile=None, certfile=None, |
90 | + server_side=False): |
91 | + return FakeSocket(sock, socket.ssl(sock, keyfile, certfile)) |
92 | + return _ssl_wrap_socket(sock, keyfile=keyfile, certfile=certfile, |
93 | + server_side=server_side) |
94 | |
95 | === modified file 'bzrlib/smart/medium.py' |
96 | --- bzrlib/smart/medium.py 2009-08-07 05:56:29 +0000 |
97 | +++ bzrlib/smart/medium.py 2009-08-31 04:35:59 +0000 |
98 | @@ -211,6 +211,7 @@ |
99 | # None during interpreter shutdown. |
100 | from sys import stderr |
101 | try: |
102 | + self._before_serve() |
103 | while not self.finished: |
104 | server_protocol = self._build_protocol() |
105 | self._serve_one_request(server_protocol) |
106 | @@ -218,6 +219,11 @@ |
107 | stderr.write("%s terminating on exception %s\n" % (self, e)) |
108 | raise |
109 | |
110 | + def _before_serve(self): |
111 | + """Executed before the serve loop. Maybe used to setup e.g. ssl. |
112 | + """ |
113 | + pass |
114 | + |
115 | def _build_protocol(self): |
116 | """Identifies the version of the incoming request, and returns an |
117 | a protocol object that can interpret it. |
118 | @@ -260,7 +266,8 @@ |
119 | |
120 | class SmartServerSocketStreamMedium(SmartServerStreamMedium): |
121 | |
122 | - def __init__(self, sock, backing_transport, root_client_path='/'): |
123 | + def __init__(self, sock, backing_transport, root_client_path='/', |
124 | + keyfile=None, certfile=None): |
125 | """Constructor. |
126 | |
127 | :param sock: the socket the server will read from. It will be put |
128 | @@ -269,8 +276,17 @@ |
129 | SmartServerStreamMedium.__init__( |
130 | self, backing_transport, root_client_path=root_client_path) |
131 | sock.setblocking(True) |
132 | + self._keyfile = keyfile |
133 | + self._certfile = certfile |
134 | self.socket = sock |
135 | |
136 | + def _before_serve(self): |
137 | + """Setup ssl if a certificate was provided.""" |
138 | + if self._certfile: |
139 | + self.socket = osutils.ssl_wrap_socket( |
140 | + self.socket, keyfile=self._keyfile, certfile=self._certfile, |
141 | + server_side=True) |
142 | + |
143 | def _serve_one_request_unguarded(self, protocol): |
144 | while protocol.next_read_size(): |
145 | # We can safely try to read large chunks. If there is less data |
146 | @@ -815,13 +831,14 @@ |
147 | class SmartTCPClientMedium(SmartClientStreamMedium): |
148 | """A client medium using TCP.""" |
149 | |
150 | - def __init__(self, host, port, base): |
151 | + def __init__(self, host, port, base, use_ssl=False): |
152 | """Creates a client that will connect on the first use.""" |
153 | SmartClientStreamMedium.__init__(self, base) |
154 | self._connected = False |
155 | self._host = host |
156 | self._port = port |
157 | self._socket = None |
158 | + self._use_ssl = use_ssl |
159 | |
160 | def _accept_bytes(self, bytes): |
161 | """See SmartClientMedium.accept_bytes.""" |
162 | @@ -873,6 +890,9 @@ |
163 | err_msg = err.args[1] |
164 | raise errors.ConnectionError("failed to connect to %s:%d: %s" % |
165 | (self._host, port, err_msg)) |
166 | + if self._use_ssl: |
167 | + self._socket = osutils.ssl_wrap_socket(self._socket, |
168 | + server_side=False) |
169 | self._connected = True |
170 | |
171 | def _flush(self): |
172 | |
173 | === modified file 'bzrlib/smart/server.py' |
174 | --- bzrlib/smart/server.py 2009-07-20 11:27:05 +0000 |
175 | +++ bzrlib/smart/server.py 2009-08-31 04:35:59 +0000 |
176 | @@ -43,7 +43,7 @@ |
177 | """ |
178 | |
179 | def __init__(self, backing_transport, host='127.0.0.1', port=0, |
180 | - root_client_path='/'): |
181 | + root_client_path='/', keyfile=None, certfile=None): |
182 | """Construct a new server. |
183 | |
184 | To actually start it running, call either start_background_thread or |
185 | @@ -54,6 +54,8 @@ |
186 | :param port: TCP port to listen on, or 0 to allocate a transient port. |
187 | :param root_client_path: The client path that will correspond to root |
188 | of backing_transport. |
189 | + :param keyfile: Path to server's private key (for SSL). |
190 | + :param certfile: Path to server's certificate (for SSL). |
191 | """ |
192 | # let connections timeout so that we get a chance to terminate |
193 | # Keep a reference to the exceptions we want to catch because the socket |
194 | @@ -84,6 +86,8 @@ |
195 | self._started = threading.Event() |
196 | self._stopped = threading.Event() |
197 | self.root_client_path = root_client_path |
198 | + self._keyfile = keyfile |
199 | + self._certfile = certfile |
200 | |
201 | def serve(self, thread_name_suffix=''): |
202 | self._should_terminate = False |
203 | @@ -150,7 +154,10 @@ |
204 | |
205 | def get_url(self): |
206 | """Return the url of the server""" |
207 | - return "bzr://%s:%d/" % self._sockname |
208 | + if self._certfile: |
209 | + return "bzrs://%s:%d/" % self._sockname |
210 | + else: |
211 | + return "bzr://%s:%d/" % self._sockname |
212 | |
213 | def serve_conn(self, conn, thread_name_suffix): |
214 | # For WIN32, where the timeout value from the listening socket |
215 | @@ -158,7 +165,8 @@ |
216 | conn.setblocking(True) |
217 | conn.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1) |
218 | handler = medium.SmartServerSocketStreamMedium( |
219 | - conn, self.backing_transport, self.root_client_path) |
220 | + conn, self.backing_transport, self.root_client_path, |
221 | + keyfile=self._keyfile, certfile=self._certfile) |
222 | thread_name = 'smart-server-child' + thread_name_suffix |
223 | connection_thread = threading.Thread( |
224 | None, handler.serve, name=thread_name) |
225 | @@ -313,7 +321,8 @@ |
226 | return transport.get_transport(url) |
227 | |
228 | |
229 | -def serve_bzr(transport, host=None, port=None, inet=False): |
230 | +def serve_bzr(transport, host=None, port=None, inet=False, |
231 | + keyfile=None, certfile=None): |
232 | from bzrlib import lockdir, ui |
233 | from bzrlib.transport import get_transport |
234 | from bzrlib.transport.chroot import ChrootServer |
235 | @@ -328,7 +337,8 @@ |
236 | host = medium.BZR_DEFAULT_INTERFACE |
237 | if port is None: |
238 | port = medium.BZR_DEFAULT_PORT |
239 | - smart_server = SmartTCPServer(transport, host=host, port=port) |
240 | + smart_server = SmartTCPServer(transport, host=host, port=port, |
241 | + keyfile=keyfile, certfile=certfile) |
242 | trace.note('listening on port: %s' % smart_server.port) |
243 | # For the duration of this server, no UI output is permitted. note |
244 | # that this may cause problems with blackbox tests. This should be |
245 | |
246 | === modified file 'bzrlib/tests/blackbox/test_serve.py' |
247 | --- bzrlib/tests/blackbox/test_serve.py 2009-08-27 22:17:35 +0000 |
248 | +++ bzrlib/tests/blackbox/test_serve.py 2009-08-31 04:35:59 +0000 |
249 | @@ -40,7 +40,21 @@ |
250 | from bzrlib.transport import get_transport, remote |
251 | |
252 | |
253 | -class TestBzrServe(TestCaseWithTransport): |
254 | +class TestBzrServeCommon(TestCaseWithTransport): |
255 | + """Provides testing functionality common to all bzr server variations.""" |
256 | + |
257 | + def make_read_requests(self, branch): |
258 | + """Do some read only requests.""" |
259 | + branch.lock_read() |
260 | + try: |
261 | + branch.repository.all_revision_ids() |
262 | + self.assertEqual(_mod_revision.NULL_REVISION, |
263 | + _mod_revision.ensure_null(branch.last_revision())) |
264 | + finally: |
265 | + branch.unlock() |
266 | + |
267 | +class TestBzrServeInet(TestBzrServeCommon): |
268 | + """Tests for bzr server started by inetd.""" |
269 | |
270 | def assertInetServerShutsdownCleanly(self, process): |
271 | """Shutdown the server process looking for errors.""" |
272 | @@ -53,24 +67,6 @@ |
273 | self.assertEqual('', result[0]) |
274 | self.assertEqual('', result[1]) |
275 | |
276 | - def assertServerFinishesCleanly(self, process): |
277 | - """Shutdown the bzr serve instance process looking for errors.""" |
278 | - # Shutdown the server |
279 | - result = self.finish_bzr_subprocess(process, retcode=3, |
280 | - send_signal=signal.SIGINT) |
281 | - self.assertEqual('', result[0]) |
282 | - self.assertEqual('bzr: interrupted\n', result[1]) |
283 | - |
284 | - def make_read_requests(self, branch): |
285 | - """Do some read only requests.""" |
286 | - branch.lock_read() |
287 | - try: |
288 | - branch.repository.all_revision_ids() |
289 | - self.assertEqual(_mod_revision.NULL_REVISION, |
290 | - _mod_revision.ensure_null(branch.last_revision())) |
291 | - finally: |
292 | - branch.unlock() |
293 | - |
294 | def start_server_inet(self, extra_options=()): |
295 | """Start a bzr server subprocess using the --inet option. |
296 | |
297 | @@ -90,23 +86,6 @@ |
298 | transport = remote.RemoteTransport(url, medium=client_medium) |
299 | return process, transport |
300 | |
301 | - def start_server_port(self, extra_options=()): |
302 | - """Start a bzr server subprocess. |
303 | - |
304 | - :param extra_options: extra options to give the server. |
305 | - :return: a tuple with the bzr process handle for passing to |
306 | - finish_bzr_subprocess, and the base url for the server. |
307 | - """ |
308 | - # Serve from the current directory |
309 | - args = ['serve', '--port', 'localhost:0'] |
310 | - args.extend(extra_options) |
311 | - process = self.start_bzr_subprocess(args, skip_if_plan_to_signal=True) |
312 | - port_line = process.stderr.readline() |
313 | - prefix = 'listening on port: ' |
314 | - self.assertStartsWith(port_line, prefix) |
315 | - port = int(port_line[len(prefix):]) |
316 | - return process,'bzr://localhost:%d/' % port |
317 | - |
318 | def test_bzr_serve_inet_readonly(self): |
319 | """bzr server should provide a read only filesystem by default.""" |
320 | process, transport = self.start_server_inet() |
321 | @@ -124,35 +103,8 @@ |
322 | self.make_read_requests(branch) |
323 | self.assertInetServerShutsdownCleanly(process) |
324 | |
325 | - def test_bzr_serve_port_readonly(self): |
326 | - """bzr server should provide a read only filesystem by default.""" |
327 | - process, url = self.start_server_port() |
328 | - transport = get_transport(url) |
329 | - self.assertRaises(errors.TransportNotPossible, transport.mkdir, 'adir') |
330 | - self.assertServerFinishesCleanly(process) |
331 | - |
332 | - def test_bzr_serve_port_readwrite(self): |
333 | - # Make a branch |
334 | - self.make_branch('.') |
335 | - |
336 | - process, url = self.start_server_port(['--allow-writes']) |
337 | - |
338 | - # Connect to the server |
339 | - branch = Branch.open(url) |
340 | - self.make_read_requests(branch) |
341 | - self.assertServerFinishesCleanly(process) |
342 | - |
343 | - def test_bzr_serve_supports_protocol(self): |
344 | - # Make a branch |
345 | - self.make_branch('.') |
346 | - |
347 | - process, url = self.start_server_port(['--allow-writes', |
348 | - '--protocol=bzr']) |
349 | - |
350 | - # Connect to the server |
351 | - branch = Branch.open(url) |
352 | - self.make_read_requests(branch) |
353 | - self.assertServerFinishesCleanly(process) |
354 | +class TestBzrServeSSH(TestBzrServeCommon): |
355 | + """Tests for bzr server started through an ssh connection.""" |
356 | |
357 | def test_bzr_connect_to_bzr_ssh(self): |
358 | """User acceptance that get_transport of a bzr+ssh:// behaves correctly. |
359 | @@ -243,7 +195,123 @@ |
360 | self.command_executed) |
361 | |
362 | |
363 | +class TestBzrServeSSLOptions(TestBzrServeCommon): |
364 | + |
365 | + def __init__(self, *args, **kwargs): |
366 | + super(TestBzrServeSSLOptions, self).__init__(*args, **kwargs) |
367 | + from bzrlib.tests.ssl_certs import build_path |
368 | + self._keyfile = build_path("server_without_pass.key") |
369 | + self._certfile = build_path("server.crt") |
370 | + |
371 | + def test_ssl_key_no_cert(self): |
372 | + self.run_bzr_error( |
373 | + ["ERROR: with --keyfile you must also supply --certfile"], |
374 | + ["serve", "--keyfile", self._keyfile]) |
375 | + |
376 | + def test_ssl_badkey(self): |
377 | + self.run_bzr_error( |
378 | + ["ERROR: %s does not contain a private key in PEM format" |
379 | + % self._certfile], |
380 | + ["serve", |
381 | + "--keyfile" , self._certfile, |
382 | + "--certfile", self._certfile]) |
383 | + |
384 | + def test_ssl_badcert(self): |
385 | + self.run_bzr_error( |
386 | + ["ERROR: %s does not contain a certificate in PEM format" |
387 | + % self._keyfile], |
388 | + ["serve", |
389 | + "--keyfile" , self._keyfile, |
390 | + "--certfile", self._keyfile]) |
391 | + |
392 | + def test_ssl_key_not_in_cert(self): |
393 | + self.run_bzr_error( |
394 | + ["ERROR: either you must supply --keyfile or the" |
395 | + " certificate file must also contain the private key"], |
396 | + ["serve", "--certfile", self._certfile]) |
397 | + |
398 | + |
399 | +class TestBzrServePort(TestBzrServeCommon): |
400 | + """Tests for bzr server accepting connections on a port.""" |
401 | + |
402 | + def assertServerFinishesCleanly(self, process): |
403 | + """Shutdown the bzr serve instance process looking for errors.""" |
404 | + # Shutdown the server |
405 | + result = self.finish_bzr_subprocess(process, retcode=3, |
406 | + send_signal=signal.SIGINT) |
407 | + self.assertEqual('', result[0]) |
408 | + self.assertEqual('bzr: interrupted\n', result[1]) |
409 | + |
410 | + def start_server_port(self, extra_options=()): |
411 | + """Start a bzr server subprocess. |
412 | + |
413 | + :param extra_options: extra options to give the server. |
414 | + :return: a tuple with the bzr process handle for passing to |
415 | + finish_bzr_subprocess, and the base url for the server. |
416 | + """ |
417 | + # Serve from the current directory |
418 | + args = ['serve', '--port', 'localhost:0'] |
419 | + args.extend(extra_options) |
420 | + urlfmt = 'bzr://localhost:%d/' |
421 | + if self.use_ssl: |
422 | + from bzrlib.tests.ssl_certs import build_path |
423 | + keyfile = build_path('server_without_pass.key') |
424 | + certfile = build_path('server.crt') |
425 | + args.extend(("--keyfile", keyfile, |
426 | + "--certfile", certfile)) |
427 | + urlfmt = 'bzrs://localhost:%d/' |
428 | + process = self.start_bzr_subprocess(args, skip_if_plan_to_signal=True) |
429 | + port_line = process.stderr.readline() |
430 | + prefix = 'listening on port: ' |
431 | + self.assertStartsWith(port_line, prefix) |
432 | + port = int(port_line[len(prefix):]) |
433 | + return process, urlfmt % port |
434 | + |
435 | + def test_bzr_serve_port_readonly(self): |
436 | + """bzr server should provide a read only filesystem by default.""" |
437 | + process, url = self.start_server_port() |
438 | + transport = get_transport(url) |
439 | + self.assertRaises(errors.TransportNotPossible, transport.mkdir, 'adir') |
440 | + self.assertServerFinishesCleanly(process) |
441 | + |
442 | + def test_bzr_serve_port_readwrite(self): |
443 | + # Make a branch |
444 | + self.make_branch('.') |
445 | + |
446 | + process, url = self.start_server_port(['--allow-writes']) |
447 | + |
448 | + # Connect to the server |
449 | + branch = Branch.open(url) |
450 | + self.make_read_requests(branch) |
451 | + self.assertServerFinishesCleanly(process) |
452 | + |
453 | + def test_bzr_serve_supports_protocol(self): |
454 | + # Make a branch |
455 | + self.make_branch('.') |
456 | + |
457 | + process, url = self.start_server_port(['--allow-writes', |
458 | + '--protocol=bzr']) |
459 | + |
460 | + # Connect to the server |
461 | + branch = Branch.open(url) |
462 | + self.make_read_requests(branch) |
463 | + self.assertServerFinishesCleanly(process) |
464 | + |
465 | +def serve_port_scenarios(): |
466 | + return (('tcp', dict(use_ssl=False)), |
467 | + ('ssl/tcp' , dict(use_ssl=True))) |
468 | + |
469 | +def load_tests(basic_tests, module, loader): |
470 | + from bzrlib import tests |
471 | + suite = loader.suiteClass() |
472 | + serve_port_tests, remaining_tests = tests.split_suite_by_condition( |
473 | + basic_tests, tests.condition_isinstance(TestBzrServePort)) |
474 | + tests.multiply_tests(serve_port_tests, serve_port_scenarios(), suite) |
475 | + suite.addTest(remaining_tests) |
476 | + return suite |
477 | + |
478 | class TestCmdServeChrooting(TestCaseWithTransport): |
479 | + """Tests for bzr server with software chroot jail.""" |
480 | |
481 | def test_serve_tcp(self): |
482 | """'bzr serve' wraps the given --directory in a ChrootServer. |
483 | |
484 | === modified file 'bzrlib/transport/__init__.py' |
485 | --- bzrlib/transport/__init__.py 2009-08-04 11:40:59 +0000 |
486 | +++ bzrlib/transport/__init__.py 2009-08-31 04:35:59 +0000 |
487 | @@ -1826,9 +1826,14 @@ |
488 | register_transport_proto('bzr://', |
489 | help="Fast access using the Bazaar smart server.", |
490 | register_netloc=True) |
491 | +register_transport_proto('bzrs://', |
492 | + help="Fast access using the Bazaar smart server over SSL.", |
493 | + register_netloc=True) |
494 | |
495 | register_lazy_transport('bzr://', 'bzrlib.transport.remote', |
496 | 'RemoteTCPTransport') |
497 | +register_lazy_transport('bzrs://', 'bzrlib.transport.remote', |
498 | + 'RemoteSSLTransport') |
499 | register_transport_proto('bzr-v2://', register_netloc=True) |
500 | |
501 | register_lazy_transport('bzr-v2://', 'bzrlib.transport.remote', |
502 | |
503 | === modified file 'bzrlib/transport/http/_urllib2_wrappers.py' |
504 | --- bzrlib/transport/http/_urllib2_wrappers.py 2009-08-19 16:33:39 +0000 |
505 | +++ bzrlib/transport/http/_urllib2_wrappers.py 2009-08-31 04:35:59 +0000 |
506 | @@ -281,19 +281,6 @@ |
507 | self._wrap_socket_for_reporting(self.sock) |
508 | |
509 | |
510 | -# Build the appropriate socket wrapper for ssl |
511 | -try: |
512 | - # python 2.6 introduced a better ssl package |
513 | - import ssl |
514 | - _ssl_wrap_socket = ssl.wrap_socket |
515 | -except ImportError: |
516 | - # python versions prior to 2.6 don't have ssl and ssl.wrap_socket instead |
517 | - # they use httplib.FakeSocket |
518 | - def _ssl_wrap_socket(sock, key_file, cert_file): |
519 | - ssl_sock = socket.ssl(sock, key_file, cert_file) |
520 | - return httplib.FakeSocket(sock, ssl_sock) |
521 | - |
522 | - |
523 | class HTTPSConnection(AbstractHTTPConnection, httplib.HTTPSConnection): |
524 | |
525 | def __init__(self, host, port=None, key_file=None, cert_file=None, |
526 | @@ -314,7 +301,8 @@ |
527 | self.connect_to_origin() |
528 | |
529 | def connect_to_origin(self): |
530 | - ssl_sock = _ssl_wrap_socket(self.sock, self.key_file, self.cert_file) |
531 | + ssl_sock = osutils.ssl_wrap_socket( |
532 | + self.sock, self.key_file, self.cert_file) |
533 | # Wrap the ssl socket before anybody use it |
534 | self._wrap_socket_for_reporting(ssl_sock) |
535 | |
536 | |
537 | === modified file 'bzrlib/transport/remote.py' |
538 | --- bzrlib/transport/remote.py 2009-03-24 01:53:42 +0000 |
539 | +++ bzrlib/transport/remote.py 2009-08-31 04:35:59 +0000 |
540 | @@ -485,6 +485,15 @@ |
541 | return client_medium, None |
542 | |
543 | |
544 | +class RemoteSSLTransport(RemoteTransport): |
545 | + """Connection to smart server over ssl.""" |
546 | + |
547 | + def _build_medium(self): |
548 | + client_medium = medium.SmartTCPClientMedium( |
549 | + self._host, self._port, self.base, use_ssl=True) |
550 | + return client_medium, None |
551 | + |
552 | + |
553 | class RemoteTCPTransportV2Only(RemoteTransport): |
554 | """Connection to smart server over plain tcp with the client hard-coded to |
555 | assume protocol v2 and remote server version <= 1.6. |
556 | |
557 | === modified file 'doc/en/user-guide/server.txt' |
558 | --- doc/en/user-guide/server.txt 2009-02-22 16:54:02 +0000 |
559 | +++ doc/en/user-guide/server.txt 2009-08-31 04:35:59 +0000 |
560 | @@ -93,3 +93,21 @@ |
561 | |
562 | bzr log bzr://localhost:1234/branchname |
563 | |
564 | +Dedicated/SSL |
565 | +~~~~~~~~~~~~~ |
566 | + |
567 | +This mode is identical to the previous one but with communications encrypted |
568 | +using SSL. Clients must connect using ``bzrs://`` URLs. |
569 | + |
570 | +server:: |
571 | + |
572 | + bzr serve --keyfile=/srv/bzr/.ssl/private/server.key \ |
573 | + --certfile=/srv/bzr/.ssl/certs/server.crt \ |
574 | + --directory=/srv/bzr/repo |
575 | + |
576 | +client:: |
577 | + |
578 | + bzr log bzrs://host/branchname |
579 | + |
580 | +For a tutorial on creating a private key and a certificate for a server, see for |
581 | +example `Be your own Certificate Authority (CA) <http://www.g-loaded.eu/2005/11/10/be-your-own-ca/>`_. |
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.