Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160

nabla-c0d3 · 2021-11-07T19:21:36Z

Status

Ready.

Description of Changes

This is the last PR for refactoring CryptoUtil and fixing type-checking, as described in #5599.

More specifically:

It hides the low-level encryption and decryption methods (encrypt() and decrypt()) by making them private. Instead, only higher-level, public methods can be used such as encrypt_source_file(), decrypt_journalist_reply(), etc.
- This makes it easier to understand the crypto-related code, and also to write/maintain it. Previously, it was easy to make a mistake when using encrypt() or decrypt(), for example by providing the wrong fingerprints.
It makes the test suite for the crypto-related code a bit more comprehensive. For example:
- It ensures that encrypted data can only be decrypted by the specific keys that were the intended recipients. This is what uncovered the issue described in Disable caching of GPG passphrases in the GPG agent (CryptoUtil 1/3) #6174 .
- It no longer imports the journalist secret key when setting up GPG for the test suite. Instead the secret key is imported (and then removed) in the specific tests that need it.

securedrop/encryption.py

nabla-c0d3 · 2021-11-09T05:50:22Z

securedrop/encryption.py

+_default_encryption_mgr: Optional["EncryptionManager"] = None
+
+
+class EncryptionManager:


Not sure about this name... open to changing it.

nabla-c0d3 · 2021-11-09T05:51:19Z

securedrop/journalist_app/__init__.py

-    app.crypto_util = CryptoUtil(
-        securedrop_root=config.SECUREDROP_ROOT,
-        gpg_key_dir=config.GPG_KEY_DIR,
-    )


The main goal of this PR: removing app.crypto_util.

nabla-c0d3 · 2021-11-09T05:51:39Z

securedrop/journalist_app/__init__.py

-        securedrop_root=config.SECUREDROP_ROOT,
-        gpg_key_dir=config.GPG_KEY_DIR,
-    )
+    app.storage = Storage(config.STORE_DIR, config.TEMP_DIR)


Storage no longer needs the config.JOURNALIST_KEY.

securedrop/source_app/main.py

nabla-c0d3 · 2021-11-09T05:53:16Z

securedrop/store.py

        # where files and directories are sent to be securely deleted
        self.__shredder_path = os.path.abspath(os.path.join(self.__storage_path, "../shredder"))
        if not os.path.exists(self.__shredder_path):
            os.makedirs(self.__shredder_path, mode=0o700)

+        # crash if we don't have a way to securely remove files
+        if not rm.check_secure_delete_capability():
+            raise AssertionError("Secure file deletion is not possible.")


Moved from CryptoUtil to here.

lgtm-com · 2021-11-13T22:43:30Z

This pull request introduces 1 alert when merging 8f3c8e5 into f34565d - view on LGTM.com

new alerts:

1 for Unused import

install_files/securedrop-app-code/debian/postinst

securedrop/encryption.py

nabla-c0d3 · 2021-11-20T21:54:10Z

securedrop/tests/conftest.py

-    journalist_secret_key_path = Path(__file__).parent / "files" / "test_journalist_key.sec"
-    journalist_secret_key = journalist_secret_key_path.read_text()
-    gpg.import_keys(journalist_secret_key)
-


Important change here: the journalist's private/secret key is no longer imported by default.

Having the journalist secret key available in GPG was making the test suite behave differently compared to web app. For example, data in tests could get decrypted using the journalist secret key instead of the expected source key.

nabla-c0d3 · 2021-11-20T21:54:53Z

securedrop/tests/test_encryption.py

+
+    The journalist secret key is removed at the end of this context manager in order to not impact
+    other decryption-related tests.
+    """


import_journalist_private_key() can be used when the journalist secret key needs to be available for a specific test.

nabla-c0d3 · 2021-12-11T10:18:16Z

securedrop/tests/conftest.py

+        # Reduce source GPG key length to speed up tests at the expense of security
+        with mock.patch.object(
+            EncryptionManager, "GPG_KEY_LENGTH", PropertyMock(return_value=1024)
+        ):


Moved from CryptoUtil to the test suite.

nabla-c0d3 · 2021-12-11T10:20:00Z

securedrop/tests/test_encryption.py

+
+        # And the source or anyone else is NOT able to decrypt the message
+        # For GPG 2.1+, a non-null passphrase _must_ be passed to decrypt()
+        assert not encryption_mgr._gpg.decrypt(encrypted_message, passphrase="test 123").ok


Example of a new test that ensures that only the right user/key can decrypt some data.

nabla-c0d3 · 2021-12-11T10:39:36Z

securedrop/tests/test_encryption.py

@@ -0,0 +1,351 @@
+import typing


Coverage report:

---------- coverage: platform linux, python 3.8.10-final-0 ----------- Name Stmts Miss Branch BrPart Cover ------------------------------------------------------------------------ encryption.py 123 9 28 4 91%

nabla-c0d3 · 2021-12-11T10:42:13Z

I ran the tests locally and also manually tested the app (make dev) - didn't run into any problems.

zenmonkeykstop

no issues found in code review
tests pass in staging env
upgrade scenario is successful and tests pass against upgrade env
manual tests of affected functionality look good.

LGTM, thanks for this!

nabla-c0d3 changed the title ~~Refactor key and encryption mgmt~~ Finish refactoring of CryptoUtil: refactor key management and encryption Nov 7, 2021

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch 2 times, most recently from 644e3e2 to adeb4a6 Compare November 8, 2021 01:19

nabla-c0d3 commented Nov 9, 2021

View reviewed changes

securedrop/encryption.py Show resolved Hide resolved

nabla-c0d3 commented Nov 9, 2021

View reviewed changes

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from adeb4a6 to c9578ac Compare November 13, 2021 21:47

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from 69aba24 to 6132be7 Compare November 16, 2021 05:40

eloquence added this to Under Review in SecureDrop Team Board Nov 17, 2021

zenmonkeykstop reviewed Nov 19, 2021

View reviewed changes

install_files/securedrop-app-code/debian/postinst Outdated Show resolved Hide resolved

zenmonkeykstop reviewed Nov 19, 2021

View reviewed changes

securedrop/encryption.py Outdated Show resolved Hide resolved

nabla-c0d3 mentioned this pull request Nov 20, 2021

Disable caching of GPG passphrases in the GPG agent (CryptoUtil 1/3) #6174

Merged

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from 6132be7 to 18d7a57 Compare November 20, 2021 19:24

nabla-c0d3 commented Nov 20, 2021

View reviewed changes

nabla-c0d3 changed the title ~~Finish refactoring of CryptoUtil: refactor key management and encryption~~ Finish refactoring of encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) Nov 22, 2021

nabla-c0d3 changed the title ~~Finish refactoring of encryption logic to fix type-checking and improve tests (CryptoUtil 3/3)~~ Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) Nov 22, 2021

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch 2 times, most recently from 7bb4b3b to 8f39600 Compare November 22, 2021 06:38

nabla-c0d3 mentioned this pull request Nov 23, 2021

Use the existing GPG fixture in the functional tests (CryptoUtil 2/3) #6184

Merged

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from 8f39600 to 9834cad Compare November 25, 2021 02:56

eloquence assigned zenmonkeykstop Dec 1, 2021

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch 3 times, most recently from 4222ede to 7a6bb0f Compare December 10, 2021 20:25

nabla-c0d3 commented Dec 11, 2021

View reviewed changes

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from 7a6bb0f to 859fbf1 Compare December 11, 2021 10:33

nabla-c0d3 commented Dec 11, 2021

View reviewed changes

Refactor encryption and key management logic to fix type-checking

6fa34d0

nabla-c0d3 force-pushed the refactor-key-and-encryption-mgmt branch from 859fbf1 to 6fa34d0 Compare December 11, 2021 10:45

nabla-c0d3 marked this pull request as ready for review December 11, 2021 11:03

nabla-c0d3 requested a review from a team as a code owner December 11, 2021 11:03

zenmonkeykstop approved these changes Dec 17, 2021

View reviewed changes

zenmonkeykstop merged commit 6cb4578 into freedomofpress:develop Dec 17, 2021

SecureDrop Team Board automation moved this from Under Review to Done Dec 17, 2021

zenmonkeykstop mentioned this pull request Dec 17, 2021

Update Flask version to 2.0.*, along with associated requirements #6154

Closed

5 tasks

nabla-c0d3 deleted the refactor-key-and-encryption-mgmt branch June 11, 2022 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160

Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160

nabla-c0d3 commented Nov 7, 2021 •

edited

nabla-c0d3 Nov 9, 2021

nabla-c0d3 Nov 9, 2021

nabla-c0d3 Nov 9, 2021

nabla-c0d3 Nov 9, 2021

lgtm-com bot commented Nov 13, 2021

nabla-c0d3 Nov 20, 2021

nabla-c0d3 Nov 20, 2021

nabla-c0d3 Dec 11, 2021

nabla-c0d3 Dec 11, 2021

nabla-c0d3 Dec 11, 2021

nabla-c0d3 commented Dec 11, 2021

zenmonkeykstop left a comment

		_default_encryption_mgr: Optional["EncryptionManager"] = None


		class EncryptionManager:

Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160

Refactor encryption logic to fix type-checking and improve tests (CryptoUtil 3/3) #6160

Conversation

nabla-c0d3 commented Nov 7, 2021 • edited

Status

Description of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Nov 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabla-c0d3 commented Dec 11, 2021

zenmonkeykstop left a comment

Choose a reason for hiding this comment

nabla-c0d3 commented Nov 7, 2021 •

edited