From faeddc365c602f8aa672b0f05f09af1605da46d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A0=20Casaj=C3=BAs?= Date: Mon, 3 Oct 2022 12:32:45 +0200 Subject: [PATCH] Display recovery codes for mfa only once (#1317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Recovery codes can only be shown after adding a 2FA code and cannot be seen afterwards * Added recovery codes fix * Updated models and script * Formatting * Format * Added base code * Updated wording * Set the config by default Co-authored-by: Adrià Casajús --- app/auth/views/recovery.py | 2 +- app/config.py | 9 +++ app/dashboard/__init__.py | 1 - app/dashboard/views/fido_setup.py | 8 +-- app/dashboard/views/mfa_setup.py | 7 +- app/dashboard/views/recovery_code.py | 30 -------- app/models.py | 31 ++++++-- ...17f_updated_recovery_code_string_length.py | 29 ++++++++ shell.py | 71 +++++++++++++------ templates/dashboard/recovery_code.html | 28 ++------ templates/dashboard/setting.html | 12 +--- tests/test.env | 4 +- 12 files changed, 137 insertions(+), 95 deletions(-) delete mode 100644 app/dashboard/views/recovery_code.py create mode 100644 migrations/versions/2022_092716_bd95b2b4217f_updated_recovery_code_string_length.py diff --git a/app/auth/views/recovery.py b/app/auth/views/recovery.py index 9e620e9f..6c3021a0 100644 --- a/app/auth/views/recovery.py +++ b/app/auth/views/recovery.py @@ -42,7 +42,7 @@ def recovery_route(): if recovery_form.validate_on_submit(): code = recovery_form.code.data - recovery_code = RecoveryCode.get_by(user_id=user.id, code=code) + recovery_code = RecoveryCode.find_by_user_code(user, code) if recovery_code: if recovery_code.used: diff --git a/app/config.py b/app/config.py index 97e58d88..99aebc5d 100644 --- a/app/config.py +++ b/app/config.py @@ -494,3 +494,12 @@ JOB_TAKEN_RETRY_WAIT_MINS = 30 # MEM_STORE MEM_STORE_URI = os.environ.get("MEM_STORE_URI", None) + +# Recovery codes hash salt +RECOVERY_CODE_HMAC_SECRET = os.environ.get("RECOVERY_CODE_HMAC_SECRET") or ( + FLASK_SECRET + "generatearandomtoken" +) +if not RECOVERY_CODE_HMAC_SECRET or len(RECOVERY_CODE_HMAC_SECRET) < 16: + raise RuntimeError( + "Please define RECOVERY_CODE_HMAC_SECRET in your configuration with a random string at least 16 chars long" + ) diff --git a/app/dashboard/__init__.py b/app/dashboard/__init__.py index a1c50611..cc19eaa2 100644 --- a/app/dashboard/__init__.py +++ b/app/dashboard/__init__.py @@ -23,7 +23,6 @@ from .views import ( mailbox_detail, refused_email, referral, - recovery_code, contact_detail, setup_done, batch_import, diff --git a/app/dashboard/views/fido_setup.py b/app/dashboard/views/fido_setup.py index e428ae87..4a225853 100644 --- a/app/dashboard/views/fido_setup.py +++ b/app/dashboard/views/fido_setup.py @@ -78,10 +78,10 @@ def fido_setup(): ) flash("Security key has been activated", "success") - if not RecoveryCode.filter_by(user_id=current_user.id).all(): - return redirect(url_for("dashboard.recovery_code_route")) - else: - return redirect(url_for("dashboard.fido_manage")) + recovery_codes = RecoveryCode.generate(current_user) + return render_template( + "dashboard/recovery_code.html", recovery_codes=recovery_codes + ) # Prepare information for key registration process fido_uuid = ( diff --git a/app/dashboard/views/mfa_setup.py b/app/dashboard/views/mfa_setup.py index b3b7a8fc..1c8a8bf7 100644 --- a/app/dashboard/views/mfa_setup.py +++ b/app/dashboard/views/mfa_setup.py @@ -8,6 +8,7 @@ from app.dashboard.base import dashboard_bp from app.dashboard.views.enter_sudo import sudo_required from app.db import Session from app.log import LOG +from app.models import RecoveryCode class OtpTokenForm(FlaskForm): @@ -39,8 +40,10 @@ def mfa_setup(): current_user.last_otp = token Session.commit() flash("MFA has been activated", "success") - - return redirect(url_for("dashboard.recovery_code_route")) + recovery_codes = RecoveryCode.generate(current_user) + return render_template( + "dashboard/recovery_code.html", recovery_codes=recovery_codes + ) else: flash("Incorrect token", "warning") diff --git a/app/dashboard/views/recovery_code.py b/app/dashboard/views/recovery_code.py deleted file mode 100644 index ace874b8..00000000 --- a/app/dashboard/views/recovery_code.py +++ /dev/null @@ -1,30 +0,0 @@ -from flask import render_template, flash, redirect, url_for, request -from flask_login import login_required, current_user - -from app.dashboard.base import dashboard_bp -from app.log import LOG -from app.models import RecoveryCode - - -@dashboard_bp.route("/recovery_code", methods=["GET", "POST"]) -@login_required -def recovery_code_route(): - if not current_user.two_factor_authentication_enabled(): - flash("you need to enable either TOTP or WebAuthn", "warning") - return redirect(url_for("dashboard.index")) - - recovery_codes = RecoveryCode.filter_by(user_id=current_user.id).all() - if request.method == "GET" and not recovery_codes: - # user arrives at this page for the first time - LOG.d("%s has no recovery keys, generate", current_user) - RecoveryCode.generate(current_user) - recovery_codes = RecoveryCode.filter_by(user_id=current_user.id).all() - - if request.method == "POST": - RecoveryCode.generate(current_user) - flash("New recovery codes generated", "success") - return redirect(url_for("dashboard.recovery_code_route")) - - return render_template( - "dashboard/recovery_code.html", recovery_codes=recovery_codes - ) diff --git a/app/models.py b/app/models.py index 77e112e7..07e6ec3c 100644 --- a/app/models.py +++ b/app/models.py @@ -2683,12 +2683,21 @@ class RecoveryCode(Base, ModelMixin): __table_args__ = (sa.UniqueConstraint("user_id", "code", name="uq_recovery_code"),) user_id = sa.Column(sa.ForeignKey(User.id, ondelete="cascade"), nullable=False) - code = sa.Column(sa.String(16), nullable=False) + code = sa.Column(sa.String(64), nullable=False) used = sa.Column(sa.Boolean, nullable=False, default=False) used_at = sa.Column(ArrowType, nullable=True, default=None) user = orm.relationship(User) + @classmethod + def _hash_code(cls, code: str) -> str: + code_hmac = hmac.new( + config.RECOVERY_CODE_HMAC_SECRET.encode("utf-8"), + code.encode("utf-8"), + "sha3_224", + ) + return base64.urlsafe_b64encode(code_hmac.digest()).decode("utf-8").rstrip("=") + @classmethod def generate(cls, user): """generate recovery codes for user""" @@ -2697,14 +2706,27 @@ class RecoveryCode(Base, ModelMixin): Session.flush() nb_code = 0 + raw_codes = [] while nb_code < _NB_RECOVERY_CODE: - code = random_string(_RECOVERY_CODE_LENGTH) - if not cls.get_by(user_id=user.id, code=code): - cls.create(user_id=user.id, code=code) + raw_code = random_string(_RECOVERY_CODE_LENGTH) + encoded_code = cls._hash_code(raw_code) + if not cls.get_by(user_id=user.id, code=encoded_code): + cls.create(user_id=user.id, code=encoded_code) + raw_codes.append(raw_code) nb_code += 1 LOG.d("Create recovery codes for %s", user) Session.commit() + return raw_codes + + @classmethod + def find_by_user_code(cls, user: User, code: str): + hashed_code = cls._hash_code(code) + # TODO: Only return hashed codes once there aren't unhashed codes in the db. + found_code = cls.get_by(user_id=user.id, code=hashed_code) + if found_code: + return found_code + return cls.get_by(user_id=user.id, code=code) @classmethod def empty(cls, user): @@ -3301,7 +3323,6 @@ class NewsletterUser(Base, ModelMixin): class ApiToCookieToken(Base, ModelMixin): - __tablename__ = "api_cookie_token" code = sa.Column(sa.String(128), unique=True, nullable=False) user_id = sa.Column(sa.ForeignKey(User.id, ondelete="cascade"), nullable=False) diff --git a/migrations/versions/2022_092716_bd95b2b4217f_updated_recovery_code_string_length.py b/migrations/versions/2022_092716_bd95b2b4217f_updated_recovery_code_string_length.py new file mode 100644 index 00000000..cea1aa0c --- /dev/null +++ b/migrations/versions/2022_092716_bd95b2b4217f_updated_recovery_code_string_length.py @@ -0,0 +1,29 @@ +"""Updated recovery code string length + +Revision ID: bd95b2b4217f +Revises: 9cc0f0712b29 +Create Date: 2022-09-27 16:14:35.021846 + +""" +import sqlalchemy_utils +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'bd95b2b4217f' +down_revision = '9cc0f0712b29' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute('ALTER TABLE recovery_code ALTER COLUMN code TYPE VARCHAR(64)') + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.execute('ALTER TABLE recovery_code ALTER COLUMN code TYPE VARCHAR(16)') + # ### end Alembic commands ### diff --git a/shell.py b/shell.py index 7939e69e..beff583a 100644 --- a/shell.py +++ b/shell.py @@ -4,27 +4,27 @@ import flask_migrate from IPython import embed from sqlalchemy_utils import create_database, database_exists, drop_database +from app import models from app.config import DB_URI -from app.db import Session -from app.email_utils import send_email, render -from app.log import LOG from app.models import * -from job_runner import ( - onboarding_pgp, - onboarding_browser_extension, - onboarding_mailbox, - onboarding_send_from_alias, -) -def create_db(): - if not database_exists(DB_URI): - LOG.d("db not exist, create database") - create_database(DB_URI) +if False: + # noinspection PyUnreachableCode + def create_db(): + if not database_exists(DB_URI): + LOG.d("db not exist, create database") + create_database(DB_URI) - # Create all tables - # Use flask-migrate instead of db.create_all() - flask_migrate.upgrade() + # Create all tables + # Use flask-migrate instead of db.create_all() + flask_migrate.upgrade() + + # noinspection PyUnreachableCode + def reset_db(): + if database_exists(DB_URI): + drop_database(DB_URI) + create_db() def change_password(user_id, new_password): @@ -33,10 +33,41 @@ def change_password(user_id, new_password): Session.commit() -def reset_db(): - if database_exists(DB_URI): - drop_database(DB_URI) - create_db() +def migrate_recovery_codes(): + last_id = -1 + while True: + recovery_codes = ( + RecoveryCode.filter(RecoveryCode.id > last_id) + .order_by(RecoveryCode.id) + .limit(100) + .all() + ) + batch_codes = len(recovery_codes) + old_codes = 0 + new_codes = 0 + last_code = None + last_code_id = None + for recovery_code in recovery_codes: + if len(recovery_code.code) == models._RECOVERY_CODE_LENGTH: + last_code = recovery_code.code + last_code_id = recovery_code.id + recovery_code.code = RecoveryCode._hash_code(recovery_code.code) + old_codes += 1 + Session.flush() + else: + new_codes += 1 + last_id = recovery_code.id + Session.commit() + LOG.i( + f"Updated {old_codes}/{batch_codes} for this batch ({new_codes} already updated)" + ) + if last_code is not None: + recovery_code = RecoveryCode.get_by(id=last_code_id) + assert RecoveryCode._hash_code(last_code) == recovery_code.code + LOG.i("Check is Good") + + if len(recovery_codes) == 0: + break if __name__ == "__main__": diff --git a/templates/dashboard/recovery_code.html b/templates/dashboard/recovery_code.html index 2265bfa7..9352ea6d 100644 --- a/templates/dashboard/recovery_code.html +++ b/templates/dashboard/recovery_code.html @@ -12,29 +12,15 @@ your account again. Each code can only be used once, make sure to store them in a safe place.

+

+ + If you had recovery codes before, they have been invalidated. + Store these codes in a safe place. You won't be able to retrieve them again! + +

-
- -
-
Warning: Generating new codes will invalidate the older ones.
-
- Back to the home page {% endblock %} diff --git a/templates/dashboard/setting.html b/templates/dashboard/setting.html index a35c32f1..6e7a67ca 100644 --- a/templates/dashboard/setting.html +++ b/templates/dashboard/setting.html @@ -102,8 +102,6 @@ {% else %} Disable TOTP - Recovery Codes {% endif %} @@ -124,8 +122,6 @@ {% else %} Manage WebAuthn - Recovery Codes {% endif %} @@ -264,14 +260,10 @@
Password
-
- You will receive an email containing instructions on how to change your password. -
+
You will receive an email containing instructions on how to change your password.
- +
diff --git a/tests/test.env b/tests/test.env index 84464e39..e942078b 100644 --- a/tests/test.env +++ b/tests/test.env @@ -61,4 +61,6 @@ PROTON_CLIENT_ID=to_fill PROTON_CLIENT_SECRET=to_fill PROTON_BASE_URL=https://localhost/api -POSTMASTER=postmaster@test.domain \ No newline at end of file +POSTMASTER=postmaster@test.domain + +RECOVERY_CODE_HMAC_SECRET=1234567890123456789 \ No newline at end of file