Opened 4 years ago

Closed 4 years ago

#8081 enhancement closed fixed (fixed)

Deprecate encryption/decryption support in twisted.persisted.sob

Reported by: Adi Roiban Owned by: Adi Roiban
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/persisted-sob-AES-deprecate-8081
branch-diff, diff-cov, branch-cov, buildbot
Author: adiroiban

Description

twisted/persisted/sob.py depends on PyCrypto for AES and we are on the path of removing PyCrypto as a dependency.

glyph

We need to just remove that functionality. It's not a good way to encrypt stuff. It's using MD5 as a KDF, it's doing its own wacky custom padding with spaces, it's using ECB mode... it basically isn't even encryption at all. And I hope nobody is actually using pickling of application state any more - we should deprecate persistence entirely.

Change History (7)

comment:1 Changed 4 years ago by Adi Roiban

Author: adiroiban
Branch: branches/persisted-sob-AES-deprecate-8081

(In [46040]) Branching to persisted-sob-AES-deprecate-8081.

comment:2 Changed 4 years ago by Adi Roiban

Scope

This branch deprecates the encryption usage in twisted.persisted.sob.

The encryption is weak and we want to remote the PyCrypto dependency.

Changes

raise a warning when encryption/decryption private methods are used.

Move all crypto related tests into a common test suite.

How to test

Check that changes make sense.

Thanks!

comment:3 Changed 4 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

I have run the test on my local system and I hope they will also pass on builders.

Please see previous comment and review.

Thanks!

comment:4 Changed 4 years ago by hawkowl

Keywords: review removed
Owner: set to Adi Roiban

Hi Adi, thanks for this.

  • In the docstring, "The encyption used by these method is weak." should be "The encryption used by these methods are weak."
  • Rather than have Crypto and skipCrypto, elsewhere we have a try, except ImportError, else pattern where there is only skipCrypto, which is set to None if it imports fine. Then there's only one variable required.

Please fix these small issues up and merge. It's good to get this junk deprecated.

comment:5 Changed 4 years ago by Adi Roiban

Thanks for the review.

Will also update the proposed deprecation docs to include this note.

I think that we need a better API on top of flushWarnings which will automatically filter only warnings from the test.

Please note that my tests also check for the lengths of all warnings... so it should fail it if catches other non-related warnings.

Will fix and merge.

Thanks!

comment:6 Changed 4 years ago by Adi Roiban

flushWarnings with a target is a PITA but I have fixed it.

I agree that flushAll is bad.

Thanks again!

comment:7 Changed 4 years ago by Adi Roiban

Resolution: fixed
Status: newclosed

(In [46165]) Merge persisted-sob-AES-deprecate-8081: Deprecate encryption/decryption support in twisted.persisted.sob.

Author: adiroiban Reviewer: hawkowl Fixes: #8081

Note: See TracTickets for help on using tickets.