Skip to content

Commit ab25f8f

Browse files
committed
Refactor CryptoUtil.display_id() to simplify source creation logic
1 parent 362c5c6 commit ab25f8f

14 files changed

+138
-121
lines changed

securedrop/crypto_util.py

-38
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,7 @@
55

66
import pretty_bad_protocol as gnupg
77
import os
8-
import io
98
import re
10-
from random import SystemRandom
119

1210
from datetime import date
1311
from flask import current_app
@@ -19,8 +17,6 @@
1917

2018
import rm
2119

22-
from models import Source
23-
2420

2521
# monkey patch to work with Focal gnupg.
2622
# https://github.com/isislovecruft/python-gnupg/issues/250
@@ -31,15 +27,6 @@
3127
# to fix GPG error #78 on production
3228
os.environ['USERNAME'] = 'www-data'
3329

34-
# SystemRandom sources from the system rand (e.g. urandom, CryptGenRandom, etc)
35-
# It supplies a CSPRNG but with an interface that supports methods like choice
36-
random = SystemRandom()
37-
38-
# safe characters for every possible word in the wordlist includes capital
39-
# letters because codename hashes are base32-encoded with capital letters
40-
DICEWARE_SAFE_CHARS = (' !#%$&)(+*-1032547698;:=?@acbedgfihkjmlonqpsrutwvyxzA'
41-
'BCDEFGHIJKLMNOPQRSTUVWXYZ')
42-
4330

4431
def monkey_patch_delete_handle_status(
4532
self: gnupg._parsers.DeleteResult, key: str, value: str
@@ -84,8 +71,6 @@ class CryptoUtil:
8471

8572
def __init__(self,
8673
securedrop_root: str,
87-
nouns_file: str,
88-
adjectives_file: str,
8974
gpg_key_dir: str) -> None:
9075
self.__securedrop_root = securedrop_root
9176

@@ -109,12 +94,6 @@ def __init__(self,
10994
else:
11095
self.gpg = gpg_binary
11196

112-
with io.open(nouns_file) as f:
113-
self.nouns = f.read().splitlines()
114-
115-
with io.open(adjectives_file) as f:
116-
self.adjectives = f.read().splitlines()
117-
11897
self.redis = Redis(decode_responses=True)
11998

12099
# Make sure these pass before the app can run
@@ -123,23 +102,6 @@ def do_runtime_tests(self) -> None:
123102
if not rm.check_secure_delete_capability():
124103
raise AssertionError("Secure file deletion is not possible.")
125104

126-
def display_id(self) -> str:
127-
"""Generate random journalist_designation until we get an unused one"""
128-
129-
tries = 0
130-
131-
while tries < 50:
132-
new_designation = ' '.join([random.choice(self.adjectives),
133-
random.choice(self.nouns)])
134-
135-
collisions = Source.query.filter(Source.journalist_designation == new_designation)
136-
if collisions.count() == 0:
137-
return new_designation
138-
139-
tries += 1
140-
141-
raise ValueError("Could not generate unique journalist designation for new source")
142-
143105
def genkeypair(self, source_user: SourceUser) -> gnupg._parsers.GenKey:
144106
"""Generate a GPG key through batch file key generation.
145107
"""

securedrop/journalist_app/__init__.py

-2
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,6 @@ def create_app(config: 'SDConfig') -> Flask:
8080
# breaks code analysis tools) for code that uses current_app.storage; it should be refactored
8181
app.crypto_util = CryptoUtil(
8282
securedrop_root=config.SECUREDROP_ROOT,
83-
nouns_file=config.NOUNS,
84-
adjectives_file=config.ADJECTIVES,
8583
gpg_key_dir=config.GPG_KEY_DIR,
8684
)
8785

securedrop/loaddata.py

+2-6
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ def set_source_count(s: str) -> int:
116116

117117

118118
def add_journalist(
119-
username: str = "",
119+
username: str,
120120
is_admin: bool = False,
121121
first_name: str = "",
122122
last_name: str = "",
@@ -128,9 +128,6 @@ def add_journalist(
128128
test_password = "correct horse battery staple profanity oil chewy"
129129
test_otp_secret = "JHCOGO7VCER3EJ4L"
130130

131-
if not username:
132-
username = current_app.crypto_util.display_id()
133-
134131
journalist = Journalist(
135132
username=username,
136133
password=test_password,
@@ -258,7 +255,6 @@ def add_source() -> Tuple[Source, str]:
258255
source_user = create_source_user(
259256
db_session=db.session,
260257
source_passphrase=codename,
261-
source_app_crypto_util=current_app.crypto_util,
262258
source_app_storage=current_app.storage,
263259
)
264260
source = source_user.get_db_record()
@@ -319,7 +315,7 @@ def create_default_journalists() -> Tuple[Journalist, ...]:
319315
def add_journalists(args: argparse.Namespace) -> None:
320316
total = args.journalist_count
321317
for i in range(1, total + 1):
322-
add_journalist(progress=(i, total))
318+
add_journalist(username=f"journalist{str(i)}", progress=(i, total))
323319

324320

325321
def add_sources(args: argparse.Namespace, journalists: Tuple[Journalist, ...]) -> None:

securedrop/source_app/__init__.py

-2
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,6 @@ def setup_i18n() -> None:
7676
# breaks code analysis tools) for code that uses current_app.storage; it should be refactored
7777
app.crypto_util = CryptoUtil(
7878
securedrop_root=config.SECUREDROP_ROOT,
79-
nouns_file=config.NOUNS,
80-
adjectives_file=config.ADJECTIVES,
8179
gpg_key_dir=config.GPG_KEY_DIR,
8280
)
8381

securedrop/source_app/main.py

-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,6 @@ def create() -> werkzeug.Response:
8181
create_source_user(
8282
db_session=db.session,
8383
source_passphrase=codename,
84-
source_app_crypto_util=current_app.crypto_util,
8584
source_app_storage=current_app.storage,
8685
)
8786
except (SourcePassphraseCollisionError, SourceDesignationCollisionError) as e:

securedrop/source_user.py

+76-9
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22

33
from base64 import b32encode
44
from functools import lru_cache
5-
from typing import Optional
5+
from pathlib import Path
6+
from random import SystemRandom
7+
from typing import Optional, List
68
from typing import TYPE_CHECKING
79

810
from cryptography.hazmat.backends import default_backend
@@ -13,7 +15,6 @@
1315
import models
1416

1517
if TYPE_CHECKING:
16-
from crypto_util import CryptoUtil
1718
from passphrases import DicewarePassphrase
1819
from store import Storage
1920

@@ -67,23 +68,41 @@ class SourceDesignationCollisionError(Exception):
6768
def create_source_user(
6869
db_session: Session,
6970
source_passphrase: "DicewarePassphrase",
70-
source_app_crypto_util: "CryptoUtil",
7171
source_app_storage: "Storage",
7272
) -> SourceUser:
7373
# Derive the source's info from their passphrase
7474
scrypt_manager = _SourceScryptManager.get_default()
7575
filesystem_id = scrypt_manager.derive_source_filesystem_id(source_passphrase)
7676
gpg_secret = scrypt_manager.derive_source_gpg_secret(source_passphrase)
7777

78-
# Create a designation for the source
79-
try:
80-
# TODO: Move display_id() out of CryptoUtil
81-
journalist_designation = source_app_crypto_util.display_id()
82-
except ValueError:
78+
# Create a unique journalist designation for the source
79+
# TODO: Add unique=True to models.Source.journalist_designation to enforce uniqueness
80+
# as the logic below has a race condition (time we check VS time when we add to the DB)
81+
designation_generation_attempts = 0
82+
valid_designation = None
83+
designation_generator = _DesignationGenerator.get_default()
84+
while designation_generation_attempts < 50:
85+
# Generate a designation
86+
designation_generation_attempts += 1
87+
new_designation = designation_generator.generate_journalist_designation()
88+
89+
# Check to see if it's already used by an existing source
90+
existing_source_with_same_designation = db_session.query(
91+
models.Source
92+
).filter_by(journalist_designation=new_designation).one_or_none()
93+
if not existing_source_with_same_designation:
94+
# The designation is not already used - good to go
95+
valid_designation = new_designation
96+
break
97+
98+
if not valid_designation:
99+
# Could not generate a designation that is not already used
83100
raise SourceDesignationCollisionError()
84101

85102
# Store the source in the DB
86-
source_db_record = models.Source(filesystem_id, journalist_designation)
103+
source_db_record = models.Source(
104+
filesystem_id=filesystem_id, journalist_designation=valid_designation
105+
)
87106
db_session.add(source_db_record)
88107
try:
89108
db_session.commit()
@@ -164,3 +183,51 @@ def get_default(cls) -> "_SourceScryptManager":
164183
scrypt_p=config.SCRYPT_PARAMS["p"],
165184
)
166185
return _default_scrypt_mgr
186+
187+
188+
_default_designation_generator: Optional["_DesignationGenerator"] = None
189+
190+
191+
class _DesignationGenerator:
192+
193+
def __init__(self, nouns: List[str], adjectives: List[str]):
194+
self._random_generator = SystemRandom()
195+
196+
# Ensure that there are no empty lists or empty strings
197+
if not nouns:
198+
raise ValueError("Nouns word list is empty")
199+
shortest_noun = min(nouns, key=len)
200+
shortest_noun_length = len(shortest_noun)
201+
if shortest_noun_length < 1:
202+
raise ValueError("Nouns word list contains an empty string")
203+
204+
if not adjectives:
205+
raise ValueError("Adjectives word list is empty")
206+
shortest_adjective = min(adjectives, key=len)
207+
shortest_adjective_length = len(shortest_adjective)
208+
if shortest_adjective_length < 1:
209+
raise ValueError("Adjectives word list contains an empty string")
210+
211+
self._nouns = nouns
212+
self._adjectives = adjectives
213+
214+
def generate_journalist_designation(self) -> str:
215+
random_adjective = self._random_generator.choice(self._adjectives)
216+
random_noun = self._random_generator.choice(self._nouns)
217+
return f"{random_adjective} {random_noun}"
218+
219+
@classmethod
220+
def get_default(cls) -> "_DesignationGenerator":
221+
# Late import so _SourceScryptManager can be used without a config.py in the parent folder
222+
from sdconfig import config
223+
224+
global _default_designation_generator
225+
if _default_designation_generator is None:
226+
# Parse the nouns and adjectives files from the config
227+
nouns = Path(config.NOUNS).read_text().strip().splitlines()
228+
adjectives = Path(config.ADJECTIVES).read_text().strip().splitlines()
229+
230+
# Create the generator
231+
_default_designation_generator = cls(nouns=nouns, adjectives=adjectives)
232+
233+
return _default_designation_generator

securedrop/tests/conftest.py

-1
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,6 @@ def test_source(journalist_app: Flask) -> Dict[str, Any]:
239239
source_user = create_source_user(
240240
db_session=db.session,
241241
source_passphrase=passphrase,
242-
source_app_crypto_util=journalist_app.crypto_util,
243242
source_app_storage=journalist_app.storage,
244243
)
245244
journalist_app.crypto_util.genkeypair(source_user)

securedrop/tests/functional/test_source.py

+8-4
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import werkzeug
22

3+
import source_user
34
from ..test_journalist import VALID_PASSWORD
45
from . import source_navigation_steps, journalist_navigation_steps
56
from . import functional_test
@@ -11,14 +12,17 @@ class TestSourceInterfaceDesignationCollision(
1112
source_navigation_steps.SourceNavigationStepsMixin):
1213

1314
def start_source_server(self, source_port):
14-
self.source_app.crypto_util.adjectives = \
15-
self.source_app.crypto_util.adjectives[:1]
16-
self.source_app.crypto_util.nouns = self.source_app.crypto_util.nouns[:1]
15+
# Generator that always returns the same journalist designation
16+
source_user._default_designation_generator = source_user._DesignationGenerator(
17+
nouns=["accent"],
18+
adjectives=["tonic"],
19+
)
20+
1721
config.SESSION_EXPIRATION_MINUTES = self.session_expiration / 60.0
1822

1923
self.source_app.run(port=source_port, debug=True, use_reloader=False, threaded=True)
2024

21-
def test_display_id_designation_collisions(self):
25+
def test_journalist_designation_collisions(self):
2226
self._source_visits_source_homepage()
2327
self._source_chooses_to_submit_documents()
2428
self._source_continues_to_submit_page()

securedrop/tests/test_crypto_util.py

+1-40
Original file line numberDiff line numberDiff line change
@@ -6,25 +6,17 @@
66
import os
77
import pytest
88

9-
from flask import url_for, session
10-
119
from passphrases import PassphraseGenerator
1210

1311
from source_user import create_source_user
1412

1513

1614
os.environ['SECUREDROP_ENV'] = 'test' # noqa
17-
import crypto_util
1815

1916
from crypto_util import CryptoUtil, CryptoException
2017
from db import db
2118

2219

23-
def test_word_list_does_not_contain_empty_strings(journalist_app):
24-
assert '' not in journalist_app.crypto_util.nouns
25-
assert '' not in journalist_app.crypto_util.adjectives
26-
27-
2820
def test_encrypt_success(source_app, config, test_source):
2921
message = 'test'
3022

@@ -124,36 +116,12 @@ def test_basic_encrypt_then_decrypt_multiple_recipients(source_app,
124116
assert plaintext == message
125117

126118

127-
def test_display_id(source_app):
128-
id = source_app.crypto_util.display_id()
129-
id_words = id.split()
130-
131-
assert len(id_words) == 2
132-
assert id_words[0] in source_app.crypto_util.adjectives
133-
assert id_words[1] in source_app.crypto_util.nouns
134-
135-
136-
def test_display_id_designation_collisions(source_app):
137-
with source_app.test_client() as app:
138-
app.get(url_for('main.generate'))
139-
source_app.crypto_util.adjectives = source_app.crypto_util.adjectives[:1]
140-
source_app.crypto_util.nouns = source_app.crypto_util.nouns[:1]
141-
tab_id = next(iter(session['codenames'].keys()))
142-
app.post(url_for('main.create'), data={'tab_id': tab_id}, follow_redirects=True)
143-
144-
with pytest.raises(ValueError) as err:
145-
source_app.crypto_util.display_id()
146-
147-
assert 'Could not generate unique journalist designation for new source' in str(err)
148-
149-
150119
def test_genkeypair(source_app):
151120
with source_app.app_context():
152121
source_user = create_source_user(
153122
db_session=db.session,
154123
source_passphrase=PassphraseGenerator.get_default().generate_passphrase(),
155124
source_app_storage=source_app.storage,
156-
source_app_crypto_util=source_app.crypto_util,
157125
)
158126
source_app.crypto_util.genkeypair(source_user)
159127

@@ -186,7 +154,6 @@ def test_reply_keypair_creation_and_expiration_dates(source_app):
186154
db_session=db.session,
187155
source_passphrase=PassphraseGenerator.get_default().generate_passphrase(),
188156
source_app_storage=source_app.storage,
189-
source_app_crypto_util=source_app.crypto_util,
190157
)
191158
source_app.crypto_util.genkeypair(source_user)
192159

@@ -275,16 +242,10 @@ def test_get_pubkey(source_app, test_source):
275242
assert pubkey is None
276243

277244

278-
@given(
279-
name=text(alphabet=crypto_util.DICEWARE_SAFE_CHARS),
280-
secret=text(alphabet=crypto_util.DICEWARE_SAFE_CHARS),
281-
message=text()
282-
)
245+
@given(message=text())
283246
def test_encrypt_then_decrypt_gives_same_result(
284247
source_app,
285248
test_source,
286-
name,
287-
secret,
288249
message
289250
):
290251
"""Test that encrypting, then decrypting a string gives the original string.

securedrop/tests/test_manage.py

-1
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,6 @@ def test_were_there_submissions_today(source_app, config):
227227
source_user = create_source_user(
228228
db_session=db.session,
229229
source_passphrase=PassphraseGenerator.get_default().generate_passphrase(),
230-
source_app_crypto_util=source_app.crypto_util,
231230
source_app_storage=source_app.storage,
232231
)
233232
source = source_user.get_db_record()

0 commit comments

Comments
 (0)