diff --git a/.circleci/config.yml b/.circleci/config.yml index fcdcb24cc5..08a68150e6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -230,7 +230,7 @@ jobs: - run: name: Install libqt5designer5 - command: sudo apt-get update && sudo apt-get install -y libqt5designer5 + command: sudo apt-get update --allow-releaseinfo-change && sudo apt-get install -y libqt5designer5 - run: name: Install requirements diff --git a/securedrop/alembic/env.py b/securedrop/alembic/env.py index c16d34a5af..d89e752f16 100644 --- a/securedrop/alembic/env.py +++ b/securedrop/alembic/env.py @@ -47,7 +47,7 @@ def run_migrations_offline(): """ url = config.get_main_option("sqlalchemy.url") context.configure( - url=url, target_metadata=target_metadata, literal_binds=True) + url=url, target_metadata=target_metadata, compare_type=True, literal_binds=True) with context.begin_transaction(): context.run_migrations() @@ -69,6 +69,7 @@ def run_migrations_online(): context.configure( connection=connection, target_metadata=target_metadata, + compare_type=True, render_as_batch=True ) diff --git a/securedrop/alembic/versions/de00920916bf_updates_journalists_otp_secret_length_.py b/securedrop/alembic/versions/de00920916bf_updates_journalists_otp_secret_length_.py new file mode 100644 index 0000000000..7a3bb635de --- /dev/null +++ b/securedrop/alembic/versions/de00920916bf_updates_journalists_otp_secret_length_.py @@ -0,0 +1,38 @@ +"""Updates journalists.otp_secret length from 16 to 32 + +Revision ID: de00920916bf +Revises: 1ddb81fb88c2 +Create Date: 2021-05-21 15:51:39.202368 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'de00920916bf' +down_revision = '1ddb81fb88c2' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('journalists', schema=None) as batch_op: + batch_op.alter_column('otp_secret', + existing_type=sa.VARCHAR(length=16), + type_=sa.String(length=32), + existing_nullable=True) + + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + with op.batch_alter_table('journalists', schema=None) as batch_op: + batch_op.alter_column('otp_secret', + existing_type=sa.String(length=32), + type_=sa.VARCHAR(length=16), + existing_nullable=True) + + # ### end Alembic commands ### diff --git a/securedrop/models.py b/securedrop/models.py index 8b3a8b1ddf..82451409ca 100644 --- a/securedrop/models.py +++ b/securedrop/models.py @@ -27,7 +27,7 @@ from typing import Callable, Optional, Union, Dict, List, Any from logging import Logger -from pyotp import OTP +from pyotp import TOTP, HOTP LOGIN_HARDENING = True @@ -404,7 +404,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=pyotp.random_base32) is_totp = Column(Boolean, default=True) # type: Column[Optional[bool]] hotp_counter = Column(Integer, default=0) # type: Column[Optional[int]] last_token = Column(String(6)) @@ -556,6 +556,9 @@ def valid_password(self, passphrase: 'Optional[str]') -> bool: "Should never happen: pw_salt is none for legacy Journalist {}".format(self.id) ) + # For type checking + assert isinstance(self.pw_hash, bytes) + is_valid = pyotp.utils.compare_digest( self._scrypt_hash(passphrase, self.pw_salt), self.pw_hash) @@ -584,14 +587,14 @@ def set_hotp_secret(self, otp_secret: str) -> None: self.hotp_counter = 0 @property - def totp(self) -> 'OTP': + def totp(self) -> 'TOTP': if self.is_totp: return pyotp.TOTP(self.otp_secret) else: raise ValueError('{} is not using TOTP'.format(self)) @property - def hotp(self) -> 'OTP': + def hotp(self) -> 'HOTP': if not self.is_totp: return pyotp.HOTP(self.otp_secret) else: @@ -698,6 +701,8 @@ def login(cls, # Prevent TOTP token reuse if user.last_token is not None: + # For type checking + assert isinstance(token, str) if pyotp.utils.compare_digest(token, user.last_token): raise BadTokenException("previously used two-factor code " "{}".format(token)) diff --git a/securedrop/requirements/python3/securedrop-app-code-requirements.in b/securedrop/requirements/python3/securedrop-app-code-requirements.in index 579653c64a..ce3a04fa18 100644 --- a/securedrop/requirements/python3/securedrop-app-code-requirements.in +++ b/securedrop/requirements/python3/securedrop-app-code-requirements.in @@ -18,7 +18,7 @@ mod_wsgi passlib pretty-bad-protocol>=3.1.1 psutil>=5.6.6 -pyotp +pyotp>=2.6.0 qrcode redis>=3.3.6 rq>=1.1.0 diff --git a/securedrop/requirements/python3/securedrop-app-code-requirements.txt b/securedrop/requirements/python3/securedrop-app-code-requirements.txt index 416bc57c0f..91d2b96ea9 100644 --- a/securedrop/requirements/python3/securedrop-app-code-requirements.txt +++ b/securedrop/requirements/python3/securedrop-app-code-requirements.txt @@ -211,9 +211,9 @@ psutil==5.7.0 \ pycparser==2.18 \ --hash=sha256:99a8ca03e29851d96616ad0404b4aad7d9ee16f25c9f9708a11faf2810f7b226 # via cffi -pyotp==2.2.6 \ - --hash=sha256:8f0df1fcf9e86cec41f0a31c91212b1a04fca6dd353426917222b21864b9310b \ - --hash=sha256:dd9130dd91a0340d89a0f06f887dbd76dd07fb95a8886dc4bc401239f2eebd69 +pyotp==2.6.0 \ + --hash=sha256:9d144de0f8a601d6869abe1409f4a3f75f097c37b50a36a3bf165810a6e23f28 \ + --hash=sha256:d28ddfd40e0c1b6a6b9da961c7d47a10261fb58f378cb00f05ce88b26df9c432 # via -r requirements/python3/securedrop-app-code-requirements.in python-dateutil==2.7.2 \ --hash=sha256:3220490fb9741e2342e1cf29a503394fdac874bc39568288717ee67047ff29df \ diff --git a/securedrop/tests/migrations/migration_de00920916bf.py b/securedrop/tests/migrations/migration_de00920916bf.py new file mode 100644 index 0000000000..afa9002110 --- /dev/null +++ b/securedrop/tests/migrations/migration_de00920916bf.py @@ -0,0 +1,73 @@ +# -*- coding: utf-8 -*- +import random +import uuid + +from sqlalchemy import text + +from db import db +from journalist_app import create_app +from .helpers import random_chars + +random.seed('くコ:彡') + + +class Helper: + + def __init__(self): + self.journalist_id = None + + def create_journalist(self, otp_secret="ABCDEFGHIJKLMNOPQRSTUVWXYZ234567"): + if self.journalist_id is not None: + raise RuntimeError('Journalist already created') + + params = { + 'uuid': str(uuid.uuid4()), + 'username': random_chars(50), + 'session_nonce': 0, + 'otp_secret': otp_secret + } + sql = '''INSERT INTO journalists (uuid, username, otp_secret, session_nonce) + VALUES (:uuid, :username, :otp_secret, :session_nonce) + ''' + self.journalist_id = db.engine.execute(text(sql), **params).lastrowid + + +class UpgradeTester(Helper): + """ + Checks schema to verify that the otp_secret varchar "length" has been updated. + Varchar specified length isn't enforced by sqlite but it's good to verify that + the migration worked as expected. + """ + + def __init__(self, config): + Helper.__init__(self) + self.config = config + self.app = create_app(config) + + def load_data(self): + with self.app.app_context(): + self.create_journalist() + + def check_upgrade(self): + with self.app.app_context(): + journalists_sql = "SELECT * FROM journalists" + journalist = db.engine.execute(text(journalists_sql)).first() + assert len(journalist['otp_secret']) == 32 # Varchar ignores length + + +class DowngradeTester(Helper): + + def __init__(self, config): + Helper.__init__(self) + self.config = config + self.app = create_app(config) + + def load_data(self): + with self.app.app_context(): + self.create_journalist() + + def check_downgrade(self): + with self.app.app_context(): + journalists_sql = "SELECT * FROM journalists" + journalist = db.engine.execute(text(journalists_sql)).first() + assert len(journalist['otp_secret']) == 32 # Varchar ignores length