Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226 #5958

evilaliv3 · 2021-05-20T15:36:23Z

Status

Ready for review

Description of Changes

Resolves #5933

This clean patch already includes revisions as by #5934 and is ready for merge con current devel branch.

\cc @eloquence @zenmonkeykstop

Testing

dev environment:

set up a dev environment from this branch with make dev
A login attempt with the default journalist account and an incorrect TOTP code fails
The journalist account can successfully log in using its existing 16-char shared secret to generate a valid TOTP code
The larger QR code and longer shared secret are displayed correctly
The journalist account's TOTP credentials can successfully be updated using the QR code and an authenticator app
The journalist account's TOTP credentials can successfully be updated using the text shared secret and an authenticator app

upgrade scenario:

set up a prod VM environment using the latest release (2.0.2)
set up a JI account with TOTP (and another using HOTP, if a U2F key is available)
verify that you can log in using the account credentials
build packages from this branch and use the upgrade scenario to update the prod VMs using the packages
Verify that the securedrop-app-code update completed successfully
verify that you can still log in using the account credentials
Verify that you can successfully update the account's TOTP via the JI, and that you get a 32-char shared secret
If an account using HOTP was set up, verify that it can be updated.
Verify that a new account can be created and that its TOTP secret is 32-chars and valid.

Deployment

Existing instances will have 16-char OTP secrets - these will still be valid, but any new or updated secrets will be 32 chars.

Checklist

If you made changes to the server application code:

Linting (make lint) and tests (make test) pass in the development container

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

Choose one of the following:

I have opened a PR in the docs repo for these changes, or will do so later
I would appreciate help with the documentation
These changes do not require documentation

If you added or updated a code dependency:

Choose one of the following:

I have performed a diff review and pasted the contents to the packaging wiki
I would like someone else to do the diff review

zenmonkeykstop · 2021-05-20T19:03:16Z

securedrop/models.py

@@ -566,7 +576,7 @@ def valid_password(self, passphrase: 'Optional[str]') -> bool:
        return is_valid

    def regenerate_totp_shared_secret(self) -> None:
-        self.otp_secret = pyotp.random_base32()
+        self.otp_secret = generate_otp_secret()


We have an issue open to eliminate the pyotp dependency altogether ( #5613 ) would you be open to expanding this PR to cover that, or to additions to the PR by our team to get rid of pytop altogether?

I'm not able to move this forward right know but please feel free to add other changes and i may be able to support with the review. thank you

zenmonkeykstop · 2021-05-20T19:03:21Z

securedrop/models.py

@@ -403,7 +413,7 @@ class Journalist(db.Model):
    is_admin = Column(Boolean)  # type: Column[Optional[bool]]
    session_nonce = Column(Integer, nullable=False, default=0)

-    otp_secret = Column(String(16), default=pyotp.random_base32)
+    otp_secret = Column(String(32), default=generate_otp_secret)


Varchars in sqlite ignore the length constraint, but a db migration is a good idea regardless. Happy to add it, as it's more of a maintenance task.

zenmonkeykstop · 2021-05-20T19:03:24Z

securedrop/models.py

+    Generate an OTP secret of 160 bits encoded base32
+    """
+    symbols = string.ascii_uppercase + string.digits
+    return ''.join(secrets.choice(symbols) for i in range(32))


For readability it might make sense to define the secret length like OTP_LENGTH = 32 and use that throughout.

i was in favour of the simple number and a comment to not confuse further developers thinking that this could be tweaked.

zenmonkeykstop · 2021-05-20T19:03:27Z

securedrop/models.py

+    """
+    Generate an OTP secret of 160 bits encoded base32
+    """
+    symbols = string.ascii_uppercase + string.digits


D'oh, my bad - this is 36 symbols. Currently the secret keys being generated use [ascii_upper]+[2.3,4,5,6,7], which makes sense in terms of readability (don't wanna mix up 0 and O or 1 and l).

(This plus alembic migration woes covers off the app-test failures in CI.)

Correct, this is what pyotp does ABCDEFGHIJKLMNOPQRSTUVWXYZ234567

As for the symbol choice i consider that who speculated on this idea when defining RFC 4648 Base32 didnt really evaluate end users; in my opinion end users wont know anyway that we are not using 0 ad 1 and will confuse O with 0 and 1 with I anyway.

I would suggest to use ABCDEFGHJKLMNPQRSTUVWXYZ23456789 that does not include any of the symbols O 0 I 1

In relation to this change i've opened a pull on pyotp so to get to understand if any drawback could exist in this variation: pyauth/pyotp#119

Probably the first thing that comes to mind is interoperability with apps - just as an example google authenticator considers 8 and 9 illegal chars in secret keys. If that's the set that pyotp uses it's probably worth sticking to it, so the only ux change is that secrets are longer.

Nice catch. I was actually expecting this but did not find confirmations in the RFC about this limitation so i did not try.

Thank you @zenmonkeykstop .

I've corrected the patch accordingly.

lgtm-com · 2021-05-20T21:42:17Z

This pull request introduces 1 alert when merging 4e9735e into eb6f4f8 - view on LGTM.com

new alerts:

1 for Unused import

codecov-commenter · 2021-05-22T03:48:59Z

Codecov Report

Merging #5958 (022792a) into develop (eb6f4f8) will decrease coverage by 0.04%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           develop    #5958      +/-   ##
===========================================
- Coverage    85.30%   85.25%   -0.05%     
===========================================
  Files           53       54       +1     
  Lines         3878     3894      +16     
  Branches       481      482       +1     
===========================================
+ Hits          3308     3320      +12     
- Misses         457      461       +4     
  Partials       113      113

Impacted Files	Coverage Δ
...920916bf_updates_journalists_otp_secret_length_.py	`66.66% <66.66%> (ø)`
securedrop/models.py	`91.71% <100.00%> (+0.06%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb6f4f8...022792a. Read the comment docs.

More work required to remove pyotp altogether

zenmonkeykstop · 2021-08-16T21:50:32Z

Hey @evilaliv3, after looking at what would be involved in using cryptography in lieu of pyotp, it seems simpler just to update the dependency. This also lets us use their secret generator which now enforces a 32-char length while preserving compatibility with authenticator apps &c.

There will be some additional work on docs and the diff review for the pyotp update, but if you're happy with this approach I can take care of that. Feel free to let me know your thoughts!

kushaldas · 2021-08-17T05:40:06Z

Will push a change for typelint errors.

lgtm-com · 2021-08-17T05:56:41Z

This pull request introduces 1 alert when merging a8f57b5 into 6df6672 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-08-17T06:09:00Z

This pull request introduces 1 alert when merging b9d4795 into 6df6672 - view on LGTM.com

new alerts:

1 for Unused import

lgtm-com · 2021-08-17T09:03:32Z

This pull request introduces 2 alerts when merging 4b461bd into 6df6672 - view on LGTM.com

new alerts:

2 for Unused import

kushaldas · 2021-08-17T09:55:52Z

Changes pushed so far:

Type annotation fixes
Debian update command needed change due to new Debian stable release
Alembic heads merge (there were 2 heads)
Putting one type annotation fix under conditional block to fix broken tests (all of them)

The heads situation before

(securedrop-app-code) root@69d52d7a527b:~/code/securedrop/securedrop# alembic history
92fba0be98e9 -> de00920916bf (head), Updates journalists.otp_secret length from 16 to 32
b060f38c0c31 -> 1ddb81fb88c2 (head), unique_index_for_instanceconfig_valid_until
92fba0be98e9 -> b060f38c0c31, drop Source.flagged
48a75abc0121 -> 92fba0be98e9 (branchpoint), Added organization_name field in instance_config table
35513370ba0d -> 48a75abc0121, add seen tables
523fff3f969c -> 35513370ba0d, add Source.deleted_at
3da3fcab826a -> 523fff3f969c, add versioned instance config
60f41bb14d98 -> 3da3fcab826a, delete orphaned submissions and replies
a9fe328b053a -> 60f41bb14d98, Add Session Nonce To Journalist
b58139cfdc8c -> a9fe328b053a, Migrations for SecureDrop's 0.14.0 release
f2833ac34bb6 -> b58139cfdc8c, add checksum columns and revoke token table
Revision ID: b58139cfdc8c
Revises: f2833ac34bb6
Create Date: 2019-04-02 10:45:05.178481
6db892e17271 -> f2833ac34bb6, add UUID column for users table
e0a525cbab83 -> 6db892e17271, add reply UUID
2d0ce3ee5bdc -> e0a525cbab83, add column to track source deletion of replies
fccf57ceef02 -> 2d0ce3ee5bdc, added passphrase_hash column to journalists table
3d91d6948753 -> fccf57ceef02, create submission uuid column
faac8092c123 -> 3d91d6948753, Create source UUID column
15ac9509fc68 -> faac8092c123, enable security pragmas
<base> -> 15ac9509fc68, init

Current error status from alembic migration

Right now there will be a few migration related failures as the column size is changed in the original work on the PR.
Like:

tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[e28559912ae5] FAILED                                                                         [ 50%]                                                                      [ 60%]
tests/test_alembic.py::test_schema_unchanged_after_up_then_downgrade[1ddb81fb88c2] FAILED

[<TracebackEntry /root/code/securedrop/securedrop/tests/test_alembic.py:159>, <TracebackEntry /root/code/securedrop/securedrop/tests/test_alembic.py:61>]
test_schema_unchanged_after_up_then_downgrade[1ddb81fb88c2] failed; it passed 0 out of the required 1 times.
	<class 'AssertionError'>
	Schema for ('table', 'journalists', 'journalists') did not match:
Left:
CREATE TABLE "journalists" (
	id INTEGER NOT NULL, 
	uuid VARCHAR(36) NOT NULL, 
	username VARCHAR(255) NOT NULL, 
	first_name VARCHAR(255), 
	last_name VARCHAR(255), 
	pw_salt BLOB, 
	pw_hash BLOB, 
	passphrase_hash VARCHAR(256), 
	is_admin BOOLEAN, 
	session_nonce INTEGER NOT NULL, 
	otp_secret VARCHAR(32), 
	is_totp BOOLEAN, 
	hotp_counter INTEGER, 
	last_token VARCHAR(6), 
	created_on DATETIME, 
	last_access DATETIME, 
	PRIMARY KEY (id), 
	CHECK (is_admin IN (0, 1)), 
	CHECK (is_totp IN (0, 1)), 
	UNIQUE (uuid), 
	UNIQUE (username)
)
Right:
CREATE TABLE journalists (
	id INTEGER NOT NULL, 
	uuid VARCHAR(36) NOT NULL, 
	username VARCHAR(255) NOT NULL, 
	first_name VARCHAR(255), 
	last_name VARCHAR(255), 
	pw_salt BLOB, 
	pw_hash BLOB, 
	passphrase_hash VARCHAR(256), 
	is_admin BOOLEAN, 
	session_nonce INTEGER NOT NULL, 
	otp_secret VARCHAR(16), 
	is_totp BOOLEAN, 
	hotp_counter INTEGER, 
	last_token VARCHAR(6), 
	created_on DATETIME, 
	last_access DATETIME, 
	PRIMARY KEY (id), 
	UNIQUE (username), 
	UNIQUE (uuid), 
	CHECK (is_admin IN (0, 1)), 
	CHECK (is_totp IN (0, 1))
)

lgtm-com · 2021-08-17T09:56:27Z

This pull request introduces 2 alerts when merging fe9bf16 into 6df6672 - view on LGTM.com

new alerts:

2 for Unused import

lgtm-com · 2021-08-17T13:40:59Z

This pull request introduces 2 alerts when merging d676621 into 6df6672 - view on LGTM.com

new alerts:

2 for Unused import

… RFC4226

…rnalists.otp_secret length change

We need to assert the type of the variable for any Optional type.

A merge version is recommended for situations where alembic history has multiple heads, but in this case that was breaking the upgrade/downgrade tests. Since the db changes in question are not yet in prod, it's ok to reorder the history instead - so the otp_secret change has been moved from its own alembic branch to the head of the existing one.

conorsch

Looks great! Only ran through the dev portion of the test plan, but all that checked out just fine. The VM-based scenarios we can touch on again as part of release QA.

Thanks, @evilaliv3, for submitting this, and for your patience during deep review. Thanks also to @zenmonkeykstop and @kushaldas for giving it lots of attention. 😃

evilaliv3 requested a review from a team as a code owner May 20, 2021 15:36

eloquence added this to Ready for Review in SecureDrop Team Board May 20, 2021

zenmonkeykstop requested changes May 20, 2021

View reviewed changes

zenmonkeykstop previously approved these changes May 25, 2021

View reviewed changes

eloquence moved this from Ready for Review to SecureDrop Sprint #71 (5/20-6/2) in SecureDrop Team Board Jun 3, 2021

eloquence moved this from SecureDrop Sprint #71 (5/20-6/2) to Near Term - SecureDrop Core in SecureDrop Team Board Jun 3, 2021

eloquence moved this from Near Term - SecureDrop Core to Next sprint candidates in SecureDrop Team Board Jul 7, 2021

eloquence moved this from Next sprint candidates to Near Term - SecureDrop Core in SecureDrop Team Board Jul 7, 2021

kushaldas moved this from Near Term - SecureDrop Server to Under Review in SecureDrop Team Board Aug 17, 2021

evilaliv3 and others added 10 commits August 31, 2021 21:01

Bump 2FA secret bit length from 80 bits to 160 bits as recommended by…

d806f76

… RFC4226

Adds type comparison to alembic migrations and adds migration for jou…

4e86002

…rnalists.otp_secret length change

Added migration test for otp_secret length change.

5d33f26

updated pyotp to 2.6.0, used its secret generator with 32-char default

752e8eb

Adds more type checking for latest mypy

e0cbfa4

We need to assert the type of the variable for any Optional type.

Fixes #6070 we can again use update command

62d5738

Adds alembic head merge

6c93f91

Adds validation for variable before assert statement

bca62ae

Puts assert statement in right condition block

6ef77a8

placate flake8

f5370fb

conorsch approved these changes Sep 1, 2021

View reviewed changes

conorsch merged commit 45cfee5 into freedomofpress:develop Sep 1, 2021

SecureDrop Team Board automation moved this from Under Review to Done Sep 1, 2021

zenmonkeykstop added this to the 2.1.0 milestone Sep 7, 2021

cfm mentioned this pull request Oct 8, 2021

Release SecureDrop 2.1.0 #6103

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226 #5958

Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226 #5958

evilaliv3 commented May 20, 2021 •

edited by zenmonkeykstop

Loading

zenmonkeykstop May 20, 2021

evilaliv3 May 20, 2021 •

edited

Loading

zenmonkeykstop May 20, 2021

zenmonkeykstop May 20, 2021

evilaliv3 May 20, 2021

zenmonkeykstop May 20, 2021

zenmonkeykstop May 20, 2021

evilaliv3 May 20, 2021 •

edited

Loading

evilaliv3 May 20, 2021

zenmonkeykstop May 20, 2021 •

edited

Loading

evilaliv3 May 21, 2021

lgtm-com bot commented May 20, 2021

codecov-commenter commented May 22, 2021 •

edited

Loading

zenmonkeykstop commented Aug 16, 2021

kushaldas commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

kushaldas commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

conorsch left a comment

Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226 #5958

Bump 2FA secret bit length from 80 to 160 bits as recommended by RFC4226 #5958

Conversation

evilaliv3 commented May 20, 2021 • edited by zenmonkeykstop Loading

Status

Description of Changes

Testing

dev environment:

upgrade scenario:

Deployment

Checklist

If you made changes to the server application code:

If you made non-trivial code changes:

If you added or updated a code dependency:

Choose a reason for hiding this comment

evilaliv3 May 20, 2021 • edited Loading

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

Choose a reason for hiding this comment

evilaliv3 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zenmonkeykstop May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented May 20, 2021

codecov-commenter commented May 22, 2021 • edited Loading

Codecov Report

zenmonkeykstop commented Aug 16, 2021

kushaldas commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

kushaldas commented Aug 17, 2021

The heads situation before

Current error status from alembic migration

lgtm-com bot commented Aug 17, 2021

lgtm-com bot commented Aug 17, 2021

conorsch left a comment

Choose a reason for hiding this comment

evilaliv3 commented May 20, 2021 •

edited by zenmonkeykstop

Loading

evilaliv3 May 20, 2021 •

edited

Loading

evilaliv3 May 20, 2021 •

edited

Loading

zenmonkeykstop May 20, 2021 •

edited

Loading

codecov-commenter commented May 22, 2021 •

edited

Loading