trevp / tlslite

TLS Library in python
Other
235 stars 67 forks source link

Record layer refactor #94

Closed tomato42 closed 6 years ago

tomato42 commented 9 years ago

Move record encryption and decryption to separate class, put it under full unit test coverage.

Based of off pull #92

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.77%) to 55.96% when pulling b5354b76ee9d3422759e63a87f0adc1ba7b80092 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling 307e662e669526b74e55168d7b653ee341d4abac on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

tomato42 commented 9 years ago

OK, I didn't expect to reach the half-way mark this soon, nice still... :smiley:

btw: the failure in travis-ci for b5354b7 commit was caused by gmail refusing a connection

tomato42 commented 9 years ago

@davidben: I have refactored TLSRecordLayer rather significantly, could you take a look at this patch set?

trevp commented 9 years ago

Haven't really looked at this, I guess you want me to merge 92 first, then this?

General comment: cause this is a security project, I have to review all changes to make sure subtle flaws aren't slipping in, so any large code movement is a lot of effort. Testing doesn't help with this much (security bugs can be very subtle and tricky). But I'll have more comments later.

tomato42 commented 9 years ago

I guess you want me to merge 92 first, then this?

It's not entirely necessary, but if you like the general direction where this is going, you can review and merge #92 first, and that will automatically reduce amount of commits to look at here. The EtM patches will be in turn based on this pull, so will have all commits. At the same time it will be a big patch set as long as this one remains unmerged.

I have to review all changes to make sure subtle flaws aren't slipping in, so any large code movement is a lot of effort

That's why I copied the source to _macThenEncrypt, calcPendingStates (in a29b07c) and _decryptThenMAC (in ab5e431) mostly unchanged from TLSRecordLayer, and then edited it in later commits when it already had full test coverage, exactly so that it would be easier to see if I moved everything necessary.

This patches are in-order, I'm not sure why github shows patches in #79 in seemingly random order.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.

coveralls commented 9 years ago

Coverage Status

Coverage increased (+13.82%) to 56.01% when pulling e12b3c730306579f98174e21b8f1842a9b1e7d47 on tomato42:record-layer-refactor into e4604fa1fef2d3bb6b778e348b60a1c253ec491f on trevp:master.