Merge pull request #359 from MerginMaps/fix_token_reuse

Fix shared context issues for security salts

Author: MarcelGeo

.prod.env

@@ -61,6 +61,12 @@ SECRET_KEY=fixme
 
 #BEARER_TOKEN_EXPIRATION=3600 * 12  # in seconds
 
+#SECURITY_BEARER_SALT=NODEFAULT
+SECURITY_BEARER_SALT=fixme
+
+#SECURITY_EMAIL_SALT=NODEFAULT
+SECURITY_EMAIL_SALT=fixme
+
 #SECURITY_PASSWORD_SALT=NODEFAULT
 SECURITY_PASSWORD_SALT=fixme
 

server/.test.env

@@ -20,3 +20,6 @@ GLOBAL_WORKSPACE='mergin'
 GLOBAL_STORAGE=104857600
 COLLECT_STATISTICS=0
 GEODIFF_WORKING_DIR=/tmp/geodiff
+SECURITY_BEARER_SALT='bearer'
+SECURITY_EMAIL_SALT='email'
+SECURITY_PASSWORD_SALT='password'

server/mergin/.env

@@ -1,6 +1,8 @@
 GEODIFF_LOGGER_LEVEL="2"
 # only for dev - should be overwritten in production
 SECRET_KEY='top-secret'
+SECURITY_BEARER_SALT='top-secret'
+SECURITY_EMAIL_SALT='top-secret'
 SECURITY_PASSWORD_SALT='top-secret'
 MAIL_DEFAULT_SENDER=''
 FLASK_DEBUG=0

server/mergin/app.py

@@ -211,6 +211,7 @@ def load_user_from_header(header_val):  # pylint: disable=W0613,W0612
             try:
                 data = decode_token(
                     app.app.config["SECRET_KEY"],
+                    app.app.config["SECURITY_BEARER_SALT"],
                     header_val,
                     app.app.config["BEARER_TOKEN_EXPIRATION"],
                 )

server/mergin/auth/app.py

@@ -80,17 +80,15 @@ def authenticate(login, password):
         return user
 
 
-def generate_confirmation_token(app, email):
+def generate_confirmation_token(app, email, salt):
     serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"])
-    return serializer.dumps(email, salt=app.config["SECURITY_PASSWORD_SALT"])
+    return serializer.dumps(email, salt=salt)
 
 
-def confirm_token(token, expiration=3600 * 24 * 3):
+def confirm_token(token, salt, expiration=3600):
     serializer = URLSafeTimedSerializer(current_app.config["SECRET_KEY"])
     try:
-        email = serializer.loads(
-            token, salt=current_app.config["SECURITY_PASSWORD_SALT"], max_age=expiration
-        )
+        email = serializer.loads(token, salt=salt, max_age=expiration)
     except:
         return
     return email
@@ -103,7 +101,12 @@ def send_confirmation_email(app, user, url, template, header, **kwargs):
     """
     from ..celery import send_email_async
 
-    token = generate_confirmation_token(app, user.email)
+    salt = (
+        app.config["SECURITY_EMAIL_SALT"]
+        if url == "confirm-email"
+        else app.config["SECURITY_PASSWORD_SALT"]
+    )
+    token = generate_confirmation_token(app, user.email, salt)
     confirm_url = f"{url}/{token}"
     html = render_template(
         template, subject=header, confirm_url=confirm_url, user=user, **kwargs

server/mergin/auth/bearer.py

@@ -7,8 +7,7 @@
 from flask.sessions import TaggedJSONSerializer
 
 
-def decode_token(secret_key, token, max_age=None):
-    salt = "bearer-session"
+def decode_token(secret_key, salt, token, max_age=None):
     serializer = TaggedJSONSerializer()
     signer_kwargs = {"key_derivation": "hmac", "digest_method": hashlib.sha1}
     s = URLSafeTimedSerializer(
@@ -17,8 +16,7 @@ def decode_token(secret_key, token, max_age=None):
     return s.loads(token, max_age=max_age)
 
 
-def encode_token(secret_key, data):
-    salt = "bearer-session"
+def encode_token(secret_key, salt, data):
     serializer = TaggedJSONSerializer()
     signer_kwargs = {"key_derivation": "hmac", "digest_method": hashlib.sha1}
     s = URLSafeTimedSerializer(

server/mergin/auth/config.py

@@ -6,6 +6,8 @@
 
 
 class Configuration(object):
+    SECURITY_BEARER_SALT = config("SECURITY_BEARER_SALT")
+    SECURITY_EMAIL_SALT = config("SECURITY_EMAIL_SALT")
     SECURITY_PASSWORD_SALT = config("SECURITY_PASSWORD_SALT")
     BEARER_TOKEN_EXPIRATION = config(
         "BEARER_TOKEN_EXPIRATION", default=3600 * 12, cast=int

server/mergin/auth/controller.py

@@ -39,6 +39,9 @@
 from ..sync.utils import files_size
 
 
+EMAIL_CONFIRMATION_EXPIRATION = 12 * 3600
+
+
 # public endpoints
 def user_profile(user, return_all=True):
     """Return user profile in json format
@@ -143,7 +146,11 @@ def login_public():  # noqa: E501
                 "email": user.email,
                 "expire": str(expire),
             }
-            token = encode_token(current_app.config["SECRET_KEY"], token_data)
+            token = encode_token(
+                current_app.config["SECRET_KEY"],
+                current_app.config["SECURITY_BEARER_SALT"],
+                token_data,
+            )
 
             data = user_profile(user)
             data["session"] = {"token": token, "expire": expire}
@@ -297,7 +304,7 @@ def password_reset():  # pylint: disable=W0613,W0612
 
 
 def confirm_new_password(token):  # pylint: disable=W0613,W0612
-    email = confirm_token(token)
+    email = confirm_token(token, salt=current_app.config["SECURITY_PASSWORD_SALT"])
     if not email:
         abort(400, "Invalid token")
 
@@ -315,7 +322,11 @@ def confirm_new_password(token):  # pylint: disable=W0613,W0612
 
 
 def confirm_email(token):  # pylint: disable=W0613,W0612
-    email = confirm_token(token)
+    email = confirm_token(
+        token,
+        expiration=EMAIL_CONFIRMATION_EXPIRATION,
+        salt=current_app.config["SECURITY_EMAIL_SALT"],
+    )
     if not email:
         abort(400, "Invalid token")
 
@@ -375,7 +386,9 @@ def register_user():  # pylint: disable=W0613,W0612
     if form.validate():
         user = User.create(form.username.data, form.email.data, form.password.data)
         user_created.send(user, source="admin")
-        token = generate_confirmation_token(current_app, user.email)
+        token = generate_confirmation_token(
+            current_app, user.email, current_app.config["SECURITY_EMAIL_SALT"]
+        )
         confirm_url = f"confirm-email/{token}"
         html = render_template(
             "email/user_created.html",

server/mergin/tests/test_auth.py

@@ -4,6 +4,7 @@
 
 from datetime import datetime, timedelta
 import os
+import time
 import pytest
 import json
 from flask import url_for
@@ -12,7 +13,7 @@
 from unittest.mock import patch
 
 from mergin.tests import test_workspace
-
+from ..auth.app import generate_confirmation_token, confirm_token
 from ..auth.models import User, UserProfile, LoginHistory
 from ..auth.tasks import anonymize_removed_users
 from ..app import db
@@ -152,26 +153,20 @@ def test_user_register(client, username, email, pwd, expected):
 
 
 def test_confirm_email(app, client):
-    serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"])
-    token = serializer.dumps(
-        "mergin@mergin.com", salt=app.config["SECURITY_PASSWORD_SALT"]
-    )
-    resp = client.post(url_for("/.mergin_auth_controller_confirm_email", token=token))
-    assert resp.status_code == 200
-
     user = User.query.filter_by(username="mergin").first()
-    # tests with old registered user
+    token = generate_confirmation_token(
+        app, user.email, app.config["SECURITY_EMAIL_SALT"]
+    )
     user.verified_email = False
-    user.registration_date = datetime.utcnow() - timedelta(days=1)
     db.session.commit()
-    resp = client.post(url_for("/.mergin_auth_controller_confirm_email", token=token))
-    assert resp.status_code == 200
 
-    # try again with freshly registered user
-    user.verified_email = False
-    user.registration_date = datetime.utcnow()
-    db.session.add(user)
-    db.session.commit()
+    # verify token can't be used in different context
+    resp = client.post(
+        url_for("/.mergin_auth_controller_confirm_new_password", token=token),
+        json={"password": "ilovemergin#0", "confirm": "ilovemergin#0"},
+    )
+    assert resp.status_code == 400
+
     resp = client.post(url_for("/.mergin_auth_controller_confirm_email", token=token))
     assert resp.status_code == 200
 
@@ -187,21 +182,35 @@ def test_confirm_email(app, client):
     resp = client.post(
         url_for(
             "/.mergin_auth_controller_confirm_email",
-            token=serializer.dumps(
-                "tests@mergin.com", salt=app.config["SECURITY_PASSWORD_SALT"]
+            token=generate_confirmation_token(
+                app, "tests@mergin.com", app.config["SECURITY_EMAIL_SALT"]
             ),
         )
     )
     assert resp.status_code == 404
 
+    # test expired token
+    token = generate_confirmation_token(
+        app, user.email, app.config["SECURITY_EMAIL_SALT"]
+    )
+    time.sleep(2)
+    assert not confirm_token(
+        token=token, expiration=1, salt=app.config["SECURITY_EMAIL_SALT"]
+    )
+
 
 def test_confirm_password(app, client):
-    serializer = URLSafeTimedSerializer(app.config["SECRET_KEY"])
-    token = serializer.dumps(
-        "mergin@mergin.com", salt=app.config["SECURITY_PASSWORD_SALT"]
+    user = User.query.filter_by(username="mergin").first()
+    token = generate_confirmation_token(
+        app, user.email, app.config["SECURITY_PASSWORD_SALT"]
     )
 
     form_data = {"password": "ilovemergin#0", "confirm": "ilovemergin#0"}
+
+    # verify token can't be used in different context
+    resp = client.post(url_for("/.mergin_auth_controller_confirm_email", token=token))
+    assert resp.status_code == 400
+
     resp = client.post(
         url_for("/.mergin_auth_controller_confirm_new_password", token=token),
         data=json.dumps(form_data),
@@ -221,8 +230,8 @@ def test_confirm_password(app, client):
     resp = client.post(
         url_for(
             "/.mergin_auth_controller_confirm_new_password",
-            token=serializer.dumps(
-                "tests@mergin.com", salt=app.config["SECURITY_PASSWORD_SALT"]
+            token=generate_confirmation_token(
+                app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"]
             ),
         ),
         data=json.dumps(form_data),
@@ -240,8 +249,8 @@ def test_confirm_password(app, client):
     resp = client.post(
         url_for(
             "/.mergin_auth_controller_confirm_new_password",
-            token=serializer.dumps(
-                "tests@mergin.com", salt=app.config["SECURITY_PASSWORD_SALT"]
+            token=generate_confirmation_token(
+                app, "tests@mergin.com", app.config["SECURITY_PASSWORD_SALT"]
             ),
         )
     )