Add fingerprints check to Tryton client

[h.goebel, 2010-03-01 14:19:35]

Sorry to say, but this patch is plain rubbish! SSL already implements
*everything* required for checking the identify of remote side. Use the
standards instead of implementing you own system.

Rule of thumb in security: do not implement you own stuff if there is tested and
reliable solution available.

[ced, 2010-03-01 15:47:35]

On 2010/03/01 14:19:35, h.goebel wrote:
> Sorry to say, but this patch is plain rubbish! SSL already implements
> *everything* required for checking the identify of remote side. Use the
> standards instead of implementing you own system.
> 
> Rule of thumb in security: do not implement you own stuff if there is tested
and
> reliable solution available.

You can keep for your-self this kind of non-constructive remarks.
I guess you want that we use CA check but it is not workable for SMB.
Or if you have an other way, explain it instead of just critic.

[h.goebel, 2010-03-01 16:19:31]

On 2010/03/01 15:47:35, ced wrote:

> You can keep for your-self this kind of non-constructive remarks.
> I guess you want that we use CA check but it is not workable for SMB.
> Or if you have an other way, explain it instead of just critic.

Why should I spend my time giving ideas an advice where you simply ignore it?
See the fruitless discussion about SSL implementation where you not even took
the time to answer whether you agree on the concept?

No, I'm not talking about "CA check", but about using certificate to verify the
remote identify. This can be implemented in a single line of code and is usable
fro SMB quite well.

But I'm not discussing concepts here. If you are interested in my experience
start a thread on tryton-dev wo others can discuss, too.

[ced, 2010-03-01 16:37:51]

On 2010/03/01 16:19:31, h.goebel wrote:
> On 2010/03/01 15:47:35, ced wrote:
> 
> > You can keep for your-self this kind of non-constructive remarks.
> > I guess you want that we use CA check but it is not workable for SMB.
> > Or if you have an other way, explain it instead of just critic.
> 
> Why should I spend my time giving ideas an advice where you simply ignore it?

I don't care. But it seems to care about having a specific implementation so
explain it.

> See the fruitless discussion about SSL implementation where you not even took
> the time to answer whether you agree on the concept?

It was already answered a long time ago. And nobody will answer negatively to
your questions because they are trivial.

> 
> No, I'm not talking about "CA check", but about using certificate to verify
the
> remote identify.

Which certificate? You mean a client side certificate?

> This can be implemented in a single line of code and is usable
> fro SMB quite well.

If it is a single line of code, why do you show it?

> 
> But I'm not discussing concepts here. If you are interested in my experience
> start a thread on tryton-dev wo others can discuss, too.

You started the discussion.

[h.goebel, 2010-03-05 09:28:55]

Implementation still has some deficit. I'll post on tryton-dev next days.

[h.goebel, 2010-03-14 18:14:17]

http://codereview.appspot.com/223066/diff/5001/6001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/5001/6001#newcode810
tryton/common/common.py:810: sys.exit()
You should be nice to the user. Quitting the application is not nice.

Untested: Additionally the current implementation will take the chance of
changing the server at all: Client starts, connects to server
(server_version()), exception is raised, client quits.

http://codereview.appspot.com/223066/diff/5001/6002
File tryton/fingerprints.py (right):

http://codereview.appspot.com/223066/diff/5001/6002#newcode28
tryton/fingerprints.py:28: continue
a) Merge these two try-except clauses.
b) remove second argument from split(), its of no use if the data is wrong.

http://codereview.appspot.com/223066/diff/5001/6003
File tryton/pysocket.py (right):

http://codereview.appspot.com/223066/diff/5001/6003#newcode19
tryton/pysocket.py:19: import sha
try:
   from hashlib import sha1
except:
   from sha import new as sha1

http://codereview.appspot.com/223066/diff/5001/6003#newcode54
tryton/pysocket.py:54: self.ca_certs = None
This is bad! This falls back *silently*. If the file is gone, security is gone,
too. Prefer to put the filename (or a bool option) into the config file.
*If* the certs-file is optional, it should be decided on a higher level (rpc.py)
Making security-relevant decisions in a low-level module is not a good thing.

http://codereview.appspot.com/223066/diff/5001/6003#newcode91
tryton/pysocket.py:91: sock.connect((host, int(port)))
Duplicate code: Same code occurs a view lines above. See issue181110 for code
cleanup.

http://codereview.appspot.com/223066/diff/5001/6003#newcode102
tryton/pysocket.py:102: if ssl:
This is hard to understand and thus hard to maintain. For code cleanup please
see issue 181110.

http://codereview.appspot.com/223066/diff/5001/6003#newcode108
tryton/pysocket.py:108: peercert = self.ssl_sock.getpeercert(True)
a) May return None if the server did not present a certificate.
b) Certificate is not validated, not even the date valid.

http://codereview.appspot.com/223066/diff/5001/6003#newcode109
tryton/pysocket.py:109: def format_hash(value):
remove this, this is ugly, very hard to understand and undocumented (and
unnecessary when using digest())

http://codereview.appspot.com/223066/diff/5001/6003#newcode116
tryton/pysocket.py:116: fingerprint =
format_hash(sha1.new(peercert).hexdigest())
a) Use digest() instead of hexdigest() if sou want to format the digest
yourself.
b) use sha1 as imported in this review instead of the if/else case.

[ced, 2010-03-14 19:46:26]

http://codereview.appspot.com/223066/diff/5001/6001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/5001/6001#newcode810
tryton/common/common.py:810: sys.exit()
On 2010/03/14 18:14:17, h.goebel wrote:
> You should be nice to the user. Quitting the application is not nice.
> 
> Untested: Additionally the current implementation will take the chance of
> changing the server at all: Client starts, connects to server
> (server_version()), exception is raised, client quits.

It doesn't happen in login windows.

http://codereview.appspot.com/223066/diff/5001/6003
File tryton/pysocket.py (right):

http://codereview.appspot.com/223066/diff/5001/6003#newcode54
tryton/pysocket.py:54: self.ca_certs = None
On 2010/03/14 18:14:17, h.goebel wrote:
> This is bad! This falls back *silently*. If the file is gone, security is
gone,
> too. Prefer to put the filename (or a bool option) into the config file.
> *If* the certs-file is optional, it should be decided on a higher level
(rpc.py)
> Making security-relevant decisions in a low-level module is not a good thing.

It doesn't chance anything to put a flag in config file or making duck-typing on
ca file.

http://codereview.appspot.com/223066/diff/5001/6003#newcode108
tryton/pysocket.py:108: peercert = self.ssl_sock.getpeercert(True)
On 2010/03/14 18:14:17, h.goebel wrote:
> a) May return None if the server did not present a certificate.
> b) Certificate is not validated, not even the date valid.

Validation is done when certfile is provided.

http://codereview.appspot.com/223066/diff/5001/6003#newcode109
tryton/pysocket.py:109: def format_hash(value):
On 2010/03/14 18:14:17, h.goebel wrote:
> remove this, this is ugly, very hard to understand and undocumented (and
> unnecessary when using digest())

digest is not readable and format_hash format the hash with a common
representation.

[henear000, 2010-03-16 15:39:27]

http://codereview.appspot.com/223066/diff/22001/23001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/22001/23001#newcode816
tryton/common/common.py:816: hostname = rpc._SOCK.hostname
frivolous change, plz revert

http://codereview.appspot.com/223066/diff/22001/23001#newcode817
tryton/common/common.py:817: port = rpc._SOCK.port
or u might as well put portname here too

[ced, 2010-03-16 15:45:53]

http://codereview.appspot.com/223066/diff/22001/23001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/22001/23001#newcode816
tryton/common/common.py:816: hostname = rpc._SOCK.hostname
On 2010/03/16 15:39:27, henear000 wrote:
> frivolous change, plz revert

It is not. Fingerprint is checked by hostname and port. So we must keep hostname
when reconnecting.

http://codereview.appspot.com/223066/diff/22001/23001#newcode817
tryton/common/common.py:817: port = rpc._SOCK.port
On 2010/03/16 15:39:27, henear000 wrote:
> or u might as well put portname here too
There is no portname

[henear000, 2010-03-16 22:30:57]

looks good, but a few minor comments

http://codereview.appspot.com/223066/diff/22001/23001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/22001/23001#newcode808
tryton/common/common.py:808: from tryton.gui.main import Main
python standard says that all imports should appear at the top of the file

http://codereview.appspot.com/223066/diff/22001/23002
File tryton/fingerprints.py (right):

http://codereview.appspot.com/223066/diff/22001/23002#newcode39
tryton/fingerprints.py:39: assert len(value) == 59
make these constants.  don't use magic numbers

http://codereview.appspot.com/223066/diff/22001/23003
File tryton/pysocket.py (right):

http://codereview.appspot.com/223066/diff/22001/23003#newcode23
tryton/pysocket.py:23: MAX_SIZE = 999999999
Can you at least make this 1e10 so that you can easily tell how large this is?

http://codereview.appspot.com/223066/diff/22001/23003#newcode24
tryton/pysocket.py:24: MAX_LENGHT = len(str(MAX_SIZE))
this is spelled incorrectly: MAX_LENGTH

http://codereview.appspot.com/223066/diff/22001/23003#newcode34
tryton/pysocket.py:34: _class = getattr(mod, klass)
klass here? but class right below?  this is confusing

http://codereview.appspot.com/223066/diff/22001/23003#newcode124
tryton/pysocket.py:124: if self.fingerprints is not None:
redundant.  just 'if self.fingerprints:' ?

[ced, 2010-03-16 22:47:48]

Other comments are not linked to this patch.

http://codereview.appspot.com/223066/diff/22001/23001
File tryton/common/common.py (right):

http://codereview.appspot.com/223066/diff/22001/23001#newcode808
tryton/common/common.py:808: from tryton.gui.main import Main
On 2010/03/16 22:30:57, henear000 wrote:
> python standard says that all imports should appear at the top of the file

This is to avoid circular import

http://codereview.appspot.com/223066/diff/22001/23003
File tryton/pysocket.py (right):

http://codereview.appspot.com/223066/diff/22001/23003#newcode124
tryton/pysocket.py:124: if self.fingerprints is not None:
On 2010/03/16 22:30:57, henear000 wrote:
> redundant.  just 'if self.fingerprints:' ?

No, we want to test that a fingerprint object has been given.

[h.goebel, 2010-03-23 16:34:12]

On 2010/03/14 19:46:26, ced wrote:

> digest is not readable and format_hash format the hash with a common
> representation.

Formating a number is much more efficient than reformatting a string.