Move passphrase logic from CryptoUtil to dedicated class for type-checking #5600
Conversation
nabla-c0d3
force-pushed
the
refactor-passphrases-generation
branch
2 times, most recently
from
2eafa4b to
8ff3973
Compare
nabla-c0d3
changed the title
There was a problem hiding this comment.
This is great work. I have a lot of comments and questions, but that's because of what's being changed, more than how. This is already cleaner, and I'm excited about getting this merged and moving on to CryptoUtil itself.
Again, just because of what it's touching, we're going to have at least one other reviewer take a look, so hold off on making any changes until they weigh in. Please do respond with any thoughts you have on my review though.
Thanks again for your contributions.
|
This will unfortunately need to be rebased before it can be thoroughly tested. |
nabla-c0d3
force-pushed
the
refactor-passphrases-generation
branch
3 times, most recently
from
fad32e6 to
206c0bc
Compare
nabla-c0d3
force-pushed
the
refactor-passphrases-generation
branch
from
206c0bc to
d7d280b
Compare
Codecov Report
@@ Coverage Diff @@
## develop #5600 +/- ##
===========================================
+ Coverage 85.32% 85.38% +0.06%
===========================================
Files 50 51 +1
Lines 3679 3709 +30
Branches 460 464 +4
===========================================
+ Hits 3139 3167 +28
- Misses 440 441 +1
- Partials 100 101 +1
Continue to review full report at Codecov.
|
|
This should be ready for another review; I think I took care of all the feedback 👍 |
| shortest_word, | ||
| self._PASSPHRASE_WORDS_COUNT, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
See the comment above about Diceware entropy.
nabla-c0d3
force-pushed
the
refactor-passphrases-generation
branch
from
d7d280b to
1df827c
Compare
|
Ok thanks for looking into the Diceware questions. I changed the minimum word list size to 7300 and the minimum size of each word to 2. |
There was a problem hiding this comment.
Thanks @nabla-c0d3 for these changes and @rmol for the thorough review. Changes here look good to me, one minor nit re: a no-longer-accurate comment (inline), due to changes introduced during the PR review. Happy to append a commit to this branch if you'd like.
Thanks again your many contributions to SecureDrop, @nabla-c0d3 !
Move passphrase logic from CryptoUtil to PassphrasesGenerator Move passphrase logic from CryptoUtil to PassphrasesGenerator Re-add generate test Ensure passphrases are random Fix pylint false positive Fix test Rename to PassphraseGenerator Address feedback from review Fix flake8 Fix test Allow non-perfect word lists and re-enable french word list Update test parameters Bump minimum word count to 6000 Fix test Tweak word lists requirements Fix docstring
nabla-c0d3
force-pushed
the
refactor-passphrases-generation
branch
from
1df827c to
0269f6f
Compare
No problem! I fixed the comment just now. |
Status
Ready.
Description of Changes
Rather than fixing #5599 in a single PR which would require a lot of changes, this PR extracts the logic related to generating passphrases from the CryptoUtil class in order to:
Provide an example of the solution I described in CryptoUtil code is not type-checked due to dynamic lookup/usage #5599 to move away from
current_app.crypto_util(which prevents proper type-checking and code analysis). To keep this PR small I only changed the code for generating passphrases within CryptoUtil, instead of trying to fix all of CryptoUtil. Hence this PR is mainly about replacingcurrent_app.crypto_util.genrandomid()withPassphrasesGenerator.get_default().generate_passphrase().Refactor and clarify all code related to passphrases. For example the passphrase are sometimes called codename, secret, randomid, etc. in the code, which can be confusing when first looking at the code base. I introduced a DicewarePassphrase type to help clarify.
Consolidate the logic of managing words list and passphrases (and the related tests) into a single class; most of the logic was in CryptoUtil, but not all of it as can be seen in this PR.
If the approach I took with this PR (mainly replacing
current_app.crypto_util.genrandomid()withPassphrasesGenerator.get_default().generate_passphrase()) is approved, I can do the same for the rest of the CryptoUtil code.