Please review reworked SSL support on client side.

This patch bundle implements the first step of a new SSL connection system on the client side. It allows the user to choose between secure and insecure connection and makes Tryton obey to this. Tryton does no longer auto-discover SSL nor does it change from a secure to an insecure connection automatically.

The fall back has been a major security issue: If the connection was started secure, but the server (or an eavesdropper) switched of SSL, the socket would silently reconnect without encryption. The user did not have any chance to avoid this. (The status bar icon is updated _after_ data has been send.)

Change Details:
* GUI: New CheckButton to allow the user selecting "Secure
  Connection". (common.common.request_server())

Under the hood:

* rpc.server_version is the function which is called first when
  connection to a server -- no matter wether it's a login, a create,
  drop or restore dialog. THus only this function takes an argument
  "secure" which may chance the secure-state of the connection. All
  other functions (esp. login, db_list, db_exec) are not changing this
  status.

* pysocket.py: Switched to OpenSSL, since the Python standard module
  ssl is only available from Python 2.6 up. Unfortunatly OpenSSL has a
  ready dull, quite low-level interface.

* pysocket.py: Do not auto-detect SSL and not *NOT* fall back to
  insecure connection if server does not use SSL.

* ipv6.py: New module.

Some refactoring has been done, too:

* common/common.py: New class ConnectionDescription to remove
  redundant code of build and parsing netrpc-"urls" and some other
  related code.

* gui/*: Dalogs Login, Create, Drop, Restore now use this
  ConnectionDescription and return hostname and port already separated
  instead of a sting.

* pysocket: some code cleanup. E.g. moved the "if ssl" statement out
  of the inner loops.

* rpc.py: some code cleanup. Esp. moved the redundant code for
  connecting the PySocket() into a common function.

[udono, 2010-01-02 08:35:39]

http://codereview.appspot.com/181110/diff/1/2
File tryton/common/common.py (right):

http://codereview.appspot.com/181110/diff/1/2#newcode183
tryton/common/common.py:183: connDesc =
ConnectionDescription.from_url(server_widget.get_text())
camelCase we use only on class definitions. Please use for variable names:
conn_desc or connection_description
s.a. http://code.google.com/p/tryton/wiki/CodingGuidelines

[h.goebel, 2010-01-02 10:39:48]

udo.spallek@googlemail.com schrieb:
> camelCase we
> use only on class definitions. Please use for variable names:
> conn_desc or connection_description s.a.
> http://code.google.com/p/tryton/wiki/CodingGuidelines

There simply is no rule about naming variables in the CodingGuidelines. :-)

-- 
Schönen Gruß - Regards
Hartmut Goebel
Dipl.-Informatiker (univ.), CISSP, CSSLP

Goebel Consult
Spezialist für IT-Sicherheit in komplexen Umgebungen
http://www.goebel-consult.de

Monatliche Kolumne: http://www.cissp-gefluester.de/
Goebel Consult mit Mitglied bei http://www.7-it.de

[ced, 2010-01-02 16:19:39]

http://codereview.appspot.com/181110/diff/1/2
File tryton/common/common.py (right):

http://codereview.appspot.com/181110/diff/1/2#newcode170
tryton/common/common.py:170: entry_secure = gtk.CheckButton()
A check box is really not user friendly.
As I already said, it is enough to prevent to drop a SSL connection after the
connection lost.

http://codereview.appspot.com/181110/diff/1/4
File tryton/gui/main.py (right):

http://codereview.appspot.com/181110/diff/1/4#newcode1186
tryton/gui/main.py:1186: info = '\n' + rpc._SOCK.host
Why removing ssl information?

http://codereview.appspot.com/181110/diff/1/9
File tryton/ipv6.py (right):

http://codereview.appspot.com/181110/diff/1/9#newcode3
tryton/ipv6.py:3: # terms.
Use the same header than other files.

http://codereview.appspot.com/181110/diff/1/9#newcode9
tryton/ipv6.py:9: class IPv6Mixin:
What is the usage ?

http://codereview.appspot.com/181110/diff/1/10
File tryton/pysocket.py (right):

http://codereview.appspot.com/181110/diff/1/10#newcode15
tryton/pysocket.py:15: from OpenSSL import SSL
There is no need for OpenSSL, it adds a new dependecy.
The default ssl from python works well.

http://codereview.appspot.com/181110/diff/1/10#newcode46
tryton/pysocket.py:46: while 1:
It is preferred while True
And it is not necessary to loop there.

http://codereview.appspot.com/181110/diff/1/10#newcode50
tryton/pysocket.py:50: import sys ; print >> sys.stderr, e
Don't use print statement

[h.goebel, 2010-01-06 16:20:36]

http://codereview.appspot.com/181110/diff/1/2
File tryton/common/common.py (right):

http://codereview.appspot.com/181110/diff/1/2#newcode170
tryton/common/common.py:170: entry_secure = gtk.CheckButton()
On 2010/01/02 16:19:39, ced wrote:
> A check box is really not user friendly.

How should the user choose between secure and insecure connection then?

> As I already said, it is enough to prevent to drop a SSL connection after the
> connection lost.

No, it is not. The user has to have control of whether a secure connection is to
be used. I must not (no, never) be left to some remote system, since the remote
system may be the eavesdropper.

There have been some publications last year about people using
http://www.paypal.com instead of https:// and expecting to get redirected to
https://... These papers discuss how easy an eavesdropper can use his to
intercept communication (and make payments for his own sake).

http://codereview.appspot.com/181110/diff/1/4
File tryton/gui/main.py (right):

http://codereview.appspot.com/181110/diff/1/4#newcode1186
tryton/gui/main.py:1186: info = '\n' + rpc._SOCK.host
On 2010/01/02 16:19:39, ced wrote:
> Why removing ssl information?

Typo / Bug.

http://codereview.appspot.com/181110/diff/1/9
File tryton/ipv6.py (right):

http://codereview.appspot.com/181110/diff/1/9#newcode9
tryton/ipv6.py:9: class IPv6Mixin:
On 2010/01/02 16:19:39, ced wrote:
> What is the usage ?

Adding IPv6 to a class using a Mixin.

http://codereview.appspot.com/181110/diff/1/10
File tryton/pysocket.py (right):

http://codereview.appspot.com/181110/diff/1/10#newcode15
tryton/pysocket.py:15: from OpenSSL import SSL
On 2010/01/02 16:19:39, ced wrote:
> There is no need for OpenSSL, it adds a new dependecy.
> The default ssl from python works well.

Module ssl is only available from Python 2.6 up. See description for this
changeset.

http://codereview.appspot.com/181110/diff/1/10#newcode46
tryton/pysocket.py:46: while 1:
On 2010/01/02 16:19:39, ced wrote:
> It is preferred while True
> And it is not necessary to loop there.

This loop is necessary, since self.sock.recv() may fail with weird errors just
of OpenSSLs internal state. You may get errors like the WantReadError even if
the server has send an answer.

http://codereview.appspot.com/181110/diff/1/10#newcode50
tryton/pysocket.py:50: import sys ; print >> sys.stderr, e
On 2010/01/02 16:19:39, ced wrote:
> Don't use print statement

Tesing left over.

[ced, 2010-01-06 17:32:32]

http://codereview.appspot.com/181110/diff/1/2
File tryton/common/common.py (right):

http://codereview.appspot.com/181110/diff/1/2#newcode170
tryton/common/common.py:170: entry_secure = gtk.CheckButton()
On 2010/01/06 16:20:36, h.goebel wrote:
> On 2010/01/02 16:19:39, ced wrote:
> > A check box is really not user friendly.
> 
> How should the user choose between secure and insecure connection then?

The user doesn't have the choice. The server is not able to use both kind of
connection.

> 
> > As I already said, it is enough to prevent to drop a SSL connection after
the
> > connection lost.
> 
> No, it is not. The user has to have control of whether a secure connection is
to
> be used. I must not (no, never) be left to some remote system, since the
remote
> system may be the eavesdropper.
> 
> There have been some publications last year about people using
> http://www.paypal.com instead of https:// and expecting to get redirected to
> https://... These papers discuss how easy an eavesdropper can use his to
> intercept communication (and make payments for his own sake).

Tryton is not a browser. You must specify the port. And the issue is about the
authentication of the server.

http://codereview.appspot.com/181110/diff/1/4
File tryton/gui/main.py (right):

http://codereview.appspot.com/181110/diff/1/4#newcode1186
tryton/gui/main.py:1186: info = '\n' + rpc._SOCK.host
On 2010/01/06 16:20:36, h.goebel wrote:
> On 2010/01/02 16:19:39, ced wrote:
> > Why removing ssl information?
> 
> Typo / Bug.

I don't understand.

http://codereview.appspot.com/181110/diff/1/9
File tryton/ipv6.py (right):

http://codereview.appspot.com/181110/diff/1/9#newcode9
tryton/ipv6.py:9: class IPv6Mixin:
On 2010/01/06 16:20:36, h.goebel wrote:
> On 2010/01/02 16:19:39, ced wrote:
> > What is the usage ?
> 
> Adding IPv6 to a class using a Mixin.

Where is it used?

http://codereview.appspot.com/181110/diff/1/10
File tryton/pysocket.py (right):

http://codereview.appspot.com/181110/diff/1/10#newcode12
tryton/pysocket.py:12: import tryton.ipv6 as ipv6
I would prefere to keep pysocket independent.
Like that you can use it to write scripts.

http://codereview.appspot.com/181110/diff/1/10#newcode15
tryton/pysocket.py:15: from OpenSSL import SSL
On 2010/01/06 16:20:36, h.goebel wrote:
> On 2010/01/02 16:19:39, ced wrote:
> > There is no need for OpenSSL, it adds a new dependecy.
> > The default ssl from python works well.
> 
> Module ssl is only available from Python 2.6 up. See description for this
> changeset.

The existing ssl code works on python 2.4 and 2.5

http://codereview.appspot.com/181110/diff/1/10#newcode46
tryton/pysocket.py:46: while 1:
On 2010/01/06 16:20:36, h.goebel wrote:
> On 2010/01/02 16:19:39, ced wrote:
> > It is preferred while True
> > And it is not necessary to loop there.
> 
> This loop is necessary, since self.sock.recv() may fail with weird errors just
> of OpenSSLs internal state. You may get errors like the WantReadError even if
> the server has send an answer.

A little comment would be welcome as we never had this issue.

[ced, 2010-01-08 08:34:43]

http://codereview.appspot.com/181110/diff/1/10
File tryton/pysocket.py (right):

http://codereview.appspot.com/181110/diff/1/10#newcode12
tryton/pysocket.py:12: import tryton.ipv6 as ipv6
On 2010/01/06 17:32:32, ced wrote:
> I would prefere to keep pysocket independent.
> Like that you can use it to write scripts.

You can add an ipv6 option to PySocket like that the file will no more depend on
ipv6 module