From 0793017ee91b485cee8c5f5764341cb866334181 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Wed, 16 Feb 2022 00:56:14 +0100 Subject: [PATCH 1/9] Change the way an invite token is verified and transferred to a user, Make it possible to get hold of the current encrypted token and its contents at the endpoints and the key operations, Introduce new errors for exceptional situations regarding token and keys, Remove temporary_key from user model, Derive user key from users password, Generate user key pair using the derived user key, Include sensitive content in the encrypted token, Update development and test db setup accordingly, Update some existing tests accordingly, Clean up some unused imports and Fix minor syntax issues encountered --- dds_web/api/schemas/user_schemas.py | 9 +- dds_web/api/user.py | 25 +++-- dds_web/database/models.py | 12 +- dds_web/development/db_init.py | 21 +++- dds_web/errors.py | 48 +++++++- dds_web/security/auth.py | 38 +++++-- dds_web/security/project_user_keys.py | 153 ++++++++++++++++---------- dds_web/security/tokens.py | 2 +- tests/conftest.py | 32 +++++- tests/test_basic_api.py | 3 +- tests/test_project_user_keys.py | 75 +++++-------- tests/test_token.py | 14 ++- tests/test_user_invite_no_project.py | 1 - 13 files changed, 278 insertions(+), 155 deletions(-) diff --git a/dds_web/api/schemas/user_schemas.py b/dds_web/api/schemas/user_schemas.py index 8474f88eb..ecf2cc1fd 100644 --- a/dds_web/api/schemas/user_schemas.py +++ b/dds_web/api/schemas/user_schemas.py @@ -14,7 +14,7 @@ from dds_web import auth, db, utils from dds_web import errors as ddserr from dds_web.database import models -from dds_web.security.project_user_keys import transfer_invite_private_key_to_user +from dds_web.security.project_user_keys import verify_and_transfer_invite_to_user #################################################################################################### # SCHEMAS ################################################################################ SCHEMAS # @@ -196,9 +196,7 @@ def make_user(self, data, **kwargs): db.session.add(new_user) # Verify and transfer invite keys to the new user - temporary_key = dds_web.security.auth.verify_invite_key(token) - if temporary_key: - transfer_invite_private_key_to_user(invite, temporary_key, new_user) + if verify_and_transfer_invite_to_user(token, new_user, data.get("password")): for project_invite_key in invite.project_invite_keys: project_user_key = models.ProjectUserKeys( project_id=project_invite_key.project_id, @@ -208,9 +206,6 @@ def make_user(self, data, **kwargs): db.session.add(project_user_key) db.session.delete(project_invite_key) - # TODO decrypt the user private key using the temp key, - # derive a key from the password and encrypt the user private key with the derived key - flask.session.pop("invite_token", None) # Delete old invite diff --git a/dds_web/api/user.py b/dds_web/api/user.py index a3f1fd6f1..4a573e91e 100644 --- a/dds_web/api/user.py +++ b/dds_web/api/user.py @@ -122,7 +122,12 @@ def invite_user(args): auth.current_user().unit.invites.append(new_invite) for project in auth.current_user().unit.projects: if project.is_active: - share_project_private_key(auth.current_user(), new_invite, project) + share_project_private_key( + from_user=auth.current_user(), + to_another=new_invite, + from_user_token=dds_web.security.auth.obtain_current_encrypted_token(), + project=project, + ) else: db.session.add(new_invite) @@ -188,7 +193,12 @@ def add_user_to_project(existing_user, project, role): owner=owner, ) ) - share_project_private_key(auth.current_user(), existing_user, project) + share_project_private_key( + from_user=auth.current_user(), + to_another=existing_user, + from_user_token=dds_web.security.auth.obtain_current_encrypted_token(), + project=project, + ) try: db.session.commit() @@ -457,7 +467,7 @@ def post(self): db.session.commit() except sqlalchemy.exc.SQLAlchemyError as err: db.session.rollback() - raise DatabaseError(message=str(err)) + raise ddserr.DatabaseError(message=str(err)) msg = ( f"The user account {user.username} ({user_email_str}, {user.role}) " f" has been {action}d successfully been by {current_user.name} ({current_user.role})." @@ -542,7 +552,7 @@ def delete_user(user): db.session.commit() except sqlalchemy.exc.SQLAlchemyError as err: db.session.rollback() - raise DatabaseError(message=str(err)) + raise ddserr.DatabaseError(message=str(err)) class RemoveUserAssociation(flask_restful.Resource): @@ -618,7 +628,8 @@ def get(self): return { "message": "Please take this token to /user/second_factor to authenticate with MFA!", "token": encrypted_jwt_token( - username=auth.current_user().username, sensitive_content=None + username=auth.current_user().username, + sensitive_content=flask.request.authorization.get("password"), ), } @@ -633,9 +644,7 @@ def get(self): token_schemas.TokenSchema().load(args) - token_claims = dds_web.security.auth.decrypt_and_verify_token_signature( - flask.request.headers["Authorization"].split()[1] - ) + token_claims = dds_web.security.auth.obtain_current_encrypted_token_claims() return {"token": update_token_with_mfa(token_claims)} diff --git a/dds_web/database/models.py b/dds_web/database/models.py index 447b97fd5..18d625ce8 100644 --- a/dds_web/database/models.py +++ b/dds_web/database/models.py @@ -357,7 +357,6 @@ class User(flask_login.UserMixin, db.Model): hotp_issue_time = db.Column(db.DateTime, unique=False, nullable=True) active = db.Column(db.Boolean) kd_salt = db.Column(db.LargeBinary(32), default=None) - temporary_key = db.Column(db.LargeBinary(32), default=None) nonce = db.Column(db.LargeBinary(12), default=None) public_key = db.Column(db.LargeBinary(300), default=None) private_key = db.Column(db.LargeBinary(300), default=None) @@ -389,8 +388,6 @@ def __init__(self, **kwargs): super(User, self).__init__(**kwargs) if not self.hotp_secret: self.hotp_secret = os.urandom(20) - if not self.public_key or not self.private_key: - generate_user_key_pair(self) def get_id(self): """Get user id - in this case username. Used by flask_login.""" @@ -407,7 +404,14 @@ def password(self, plaintext_password): """Generate the password hash and save in db.""" pw_hasher = argon2.PasswordHasher(hash_len=32) self._password_hash = pw_hasher.hash(plaintext_password) - self.kd_salt = os.urandom(32) + + # User key pair should only be set from here if the password is lost + # and all the keys associated with the user should be cleaned up + # before setting the password. + # This should help the tests for setup as well. + if not self.public_key or not self.private_key: + self.kd_salt = os.urandom(32) + generate_user_key_pair(self, plaintext_password) def verify_password(self, input_password): """Verifies that the specified password matches the encoded password in the database.""" diff --git a/dds_web/development/db_init.py b/dds_web/development/db_init.py index 7753cd0d4..a8be629f2 100644 --- a/dds_web/development/db_init.py +++ b/dds_web/development/db_init.py @@ -17,6 +17,7 @@ generate_project_key_pair, share_project_private_key, ) +from dds_web.security.tokens import encrypted_jwt_token import dds_web.utils #################################################################################################### @@ -154,7 +155,23 @@ def fill_db(): db.session.commit() - share_project_private_key(unituser_1, researchuser_1, project_1) - share_project_private_key(unituser_1, researchuser_2, project_1) + unituser_1_token = encrypted_jwt_token( + username=unituser_1.username, + sensitive_content=password, + ) + + share_project_private_key( + from_user=unituser_1, + to_another=researchuser_1, + from_user_token=unituser_1_token, + project=project_1, + ) + + share_project_private_key( + from_user=unituser_1, + to_another=researchuser_2, + from_user_token=unituser_1_token, + project=project_1, + ) db.session.commit() diff --git a/dds_web/errors.py b/dds_web/errors.py index cbe0be1e4..a838e4919 100644 --- a/dds_web/errors.py +++ b/dds_web/errors.py @@ -10,14 +10,12 @@ # Installed from werkzeug import exceptions import flask -import dds_web -import flask_login import http import json import structlog # Own modules -from dds_web import actions, auth +from dds_web import auth #################################################################################################### # LOGGING ################################################################################ LOGGING # @@ -73,6 +71,50 @@ def __init__(self, encryption_key_char_length): general_logger.error(message) +class TokenMissingError(LoggedHTTPException): + """Errors due to missing token.""" + + code = http.HTTPStatus.BAD_REQUEST + + def __init__(self, message="Token is missing"): + super().__init__(message) + + general_logger.warning(message) + + +class SensitiveContentMissingError(LoggedHTTPException): + """Errors due to missing sensitive content in the encrypted token.""" + + code = http.HTTPStatus.BAD_REQUEST + + def __init__(self, message="Sensitive content is missing in the encrypted token!"): + super().__init__(message) + + general_logger.warning(message) + + +class KeySetupError(LoggedHTTPException): + """Errors due to missing keys.""" + + code = http.HTTPStatus.INTERNAL_SERVER_ERROR + + def __init__(self, message="Keys are not properly setup!"): + super().__init__(message) + + general_logger.warning(message) + + +class KeyOperationError(LoggedHTTPException): + """Errors due to issues in key operations.""" + + code = http.HTTPStatus.INTERNAL_SERVER_ERROR + + def __init__(self, message="A key cannot be processed!"): + super().__init__(message) + + general_logger.warning(message) + + class AuthenticationError(LoggedHTTPException): """Base class for errors due to authentication failure.""" diff --git a/dds_web/security/auth.py b/dds_web/security/auth.py index a73421e53..8401770a4 100644 --- a/dds_web/security/auth.py +++ b/dds_web/security/auth.py @@ -18,9 +18,8 @@ # Own modules from dds_web import basic_auth, auth, mail -from dds_web.errors import AuthenticationError, AccessDeniedError, InviteError +from dds_web.errors import AuthenticationError, AccessDeniedError, InviteError, TokenMissingError from dds_web.database import models -from dds_web.security.project_user_keys import verify_invite_temporary_key import dds_web.utils action_logger = structlog.getLogger("actions") @@ -135,8 +134,11 @@ def matching_email_with_invite(token, email): return claims.get("inv") == email -def verify_invite_key(token): - """Verify token, email, invite and temporary key.""" +def extract_token_invite_key(token): + """Verify token, email and invite. + + Return invite and temporary key. + """ claims = __base_verify_token_for_invite(token=token) # Verify email in token @@ -149,19 +151,29 @@ def verify_invite_key(token): if not invite: raise InviteError(message="Invite could not be found!") - # Verify temporary key from token claims - temporary_key = bytes.fromhex(claims.get("sen_con")) - if verify_invite_temporary_key(invite=invite, temporary_key=temporary_key): - return temporary_key + return invite, bytes.fromhex(claims.get("sen_con")) + + +def obtain_current_encrypted_token(): + try: + return flask.request.headers["Authorization"].split()[1] + except KeyError: + raise TokenMissingError("Encrypted token is required but missing!") + + +def obtain_current_encrypted_token_claims(): + token = obtain_current_encrypted_token() + if token: + return decrypt_and_verify_token_signature(token) @auth.verify_token def verify_token(token): - """Verify token used in token authencation.""" + """Verify token used in token authentication.""" claims = __verify_general_token(token=token) user = __user_from_subject(subject=claims.get("sub")) - return handle_multi_factor_authentication( + return __handle_multi_factor_authentication( user=user, mfa_auth_time_string=claims.get("mfa_auth_time") ) @@ -209,7 +221,7 @@ def __user_from_subject(subject): return user -def handle_multi_factor_authentication(user, mfa_auth_time_string): +def __handle_multi_factor_authentication(user, mfa_auth_time_string): """Verify multifactor authentication time frame.""" if user: if mfa_auth_time_string: @@ -245,8 +257,10 @@ def send_hotp_email(user): return False -def extract_encrypted_token_content(token, username): +def extract_encrypted_token_sensitive_content(token, username): """Extract the sensitive content from inside the encrypted token.""" + if token is None: + raise TokenMissingError(message="There is no token to extract sensitive content from.") content = decrypt_and_verify_token_signature(token=token) if content.get("sub") == username: return content.get("sen_con") diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index 03885e5c1..c83fb35ef 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -1,13 +1,43 @@ """ Code for generating and maintaining project and user related keys """ import os +import argon2 import cryptography.exceptions from cryptography.hazmat.primitives import asymmetric, ciphers, hashes, serialization import flask import gc from dds_web.database import models -from dds_web.errors import KeyNotFoundError +from dds_web.errors import ( + KeyNotFoundError, + KeyOperationError, + KeySetupError, + SensitiveContentMissingError, +) +from dds_web.security.auth import ( + extract_encrypted_token_sensitive_content, + extract_token_invite_key, +) + + +def __derive_key(user, password): + if user.kd_salt is None: + raise KeySetupError(message="User keys are not properly setup!") + + derived_key = argon2.low_level.hash_secret_raw( + secret=password.encode(), + salt=user.kd_salt, + time_cost=2, + memory_cost=flask.current_app.config["ARGON_MEMORY_COST"], + parallelism=8, + hash_len=32, + type=argon2.Type.ID, + ) + + if len(derived_key) != 32: + raise KeySetupError(message="Derived key is not 256 bits long!") + + return derived_key def __get_padding_for_rsa(): @@ -41,43 +71,45 @@ def __decrypt_with_rsa(ciphertext, private_key): def __encrypt_project_private_key(owner, project_private_key): - public_key = serialization.load_der_public_key(owner.public_key) - if isinstance(public_key, asymmetric.rsa.RSAPublicKey): - return __encrypt_with_rsa(project_private_key, public_key) - # TODO: Change exception type - exception = Exception("Public key cannot be loaded for encrypting the project private key!") - flask.current_app.logger.exception(exception) - raise exception - - -def __decrypt_project_private_key(user, encrypted_project_private_key): - user_private_key = serialization.load_der_private_key( - __decrypt_user_private_key(user), password=None - ) - if isinstance(user_private_key, asymmetric.rsa.RSAPrivateKey): - return __decrypt_with_rsa(encrypted_project_private_key, user_private_key) - exception = Exception("User private key cannot be loaded!") - flask.current_app.logger.exception(exception) - raise exception + if owner.public_key: + owner_public_key = serialization.load_der_public_key(owner.public_key) + if isinstance(owner_public_key, asymmetric.rsa.RSAPublicKey): + return __encrypt_with_rsa(project_private_key, owner_public_key) + raise KeyOperationError( + message="User public key cannot be loaded for encrypting the project private key!" + ) + raise KeySetupError(message="User keys are not properly setup!") + +def __decrypt_project_private_key(user, token, encrypted_project_private_key): + private_key_bytes = __decrypt_user_private_key(user, token) + if private_key_bytes: + user_private_key = serialization.load_der_private_key(private_key_bytes, password=None) + if isinstance(user_private_key, asymmetric.rsa.RSAPrivateKey): + return __decrypt_with_rsa(encrypted_project_private_key, user_private_key) + raise KeyOperationError( + message="User private key cannot be loaded for decrypting the project private key!" + ) + raise KeyOperationError(message="User private key cannot be decrypted!") -def obtain_project_private_key(user, project): + +def obtain_project_private_key(user, token, project): project_key = models.ProjectUserKeys.query.filter_by( project_id=project.id, user_id=user.username ).first() if project_key: - return __decrypt_project_private_key(user, project_key.key) + return __decrypt_project_private_key(user, token, project_key.key) raise KeyNotFoundError(project=project.public_id) -def share_project_private_key(from_user, to_another, project): +def share_project_private_key(from_user, to_another, from_user_token, project): if isinstance(to_another, models.Invite): __init_and_append_project_invite_key( - to_another, project, obtain_project_private_key(from_user, project) + to_another, project, obtain_project_private_key(from_user, from_user_token, project) ) else: __init_and_append_project_user_key( - to_another, project, obtain_project_private_key(from_user, project) + to_another, project, obtain_project_private_key(from_user, from_user_token, project) ) @@ -151,36 +183,33 @@ def __owner_identifier(owner): return owner.email if isinstance(owner, models.Invite) else owner.username -def __encrypt_owner_private_key(owner, private_key, temporary_key=None): - if temporary_key is None: - temporary_key = ciphers.aead.AESGCM.generate_key(bit_length=256) +def __encrypt_owner_private_key(owner, private_key, owner_key=None): + key = owner_key + if key is None: + key = ciphers.aead.AESGCM.generate_key(bit_length=256) + nonce, encrypted_key = __encrypt_with_aes( - temporary_key, private_key, aad=b"private key for " + __owner_identifier(owner).encode() + key, private_key, aad=b"private key for " + __owner_identifier(owner).encode() ) owner.nonce = nonce owner.private_key = encrypted_key - return temporary_key + return key -def __encrypt_user_private_key(user, private_key): - user.temporary_key = __encrypt_owner_private_key(user, private_key) +def __decrypt_user_private_key(user, token): + password = extract_encrypted_token_sensitive_content(token, user.username) + if password is None: + raise SensitiveContentMissingError + user_key = __derive_key(user, password) - -def __decrypt_user_private_key(user): - if user.temporary_key and user.private_key and user.nonce: + if user.private_key and user.nonce: return __decrypt_with_aes( - user.temporary_key, + user_key, user.private_key, user.nonce, aad=b"private key for " + user.username.encode(), ) - exception = Exception("User keys are not properly setup!") - flask.current_app.logger.exception(exception) - raise exception - - -def __encrypt_invite_private_key(invite, private_key): - return __encrypt_owner_private_key(invite, private_key) + raise KeySetupError(message="User keys are not properly setup!") def __decrypt_invite_private_key(invite, temporary_key): @@ -194,30 +223,34 @@ def __decrypt_invite_private_key(invite, temporary_key): ) -def transfer_invite_private_key_to_user(invite, temporary_key, user): - private_key_bytes = __decrypt_invite_private_key(invite, temporary_key) - if private_key_bytes and isinstance( - serialization.load_der_private_key(private_key_bytes, password=None), - asymmetric.rsa.RSAPrivateKey, - ): - user.temporary_key = __encrypt_owner_private_key(user, private_key_bytes, temporary_key) - user.public_key = invite.public_key +def verify_and_transfer_invite_to_user(token, user, password): + invite, temporary_key = extract_token_invite_key(token) + private_key_bytes = __verify_invite_temporary_key(invite, temporary_key) + if private_key_bytes: + __transfer_invite_private_key_to_user(invite, private_key_bytes, user, password) del private_key_bytes gc.collect() + return True + return False + + +def __transfer_invite_private_key_to_user(invite, private_key_bytes, user, password): + user_key = __derive_key(user, password) + __encrypt_owner_private_key(user, private_key_bytes, user_key) + user.public_key = invite.public_key + del user_key + gc.collect() -def verify_invite_temporary_key(invite, temporary_key): +def __verify_invite_temporary_key(invite, temporary_key): """Verify the temporary key generated for the specific user invite.""" private_key_bytes = __decrypt_invite_private_key(invite=invite, temporary_key=temporary_key) if private_key_bytes and isinstance( serialization.load_der_private_key(data=private_key_bytes, password=None), asymmetric.rsa.RSAPrivateKey, ): - # Clean up sensitive information - del private_key_bytes - gc.collect() - return True - return False + return private_key_bytes + return None def __generate_rsa_key_pair(owner): @@ -234,16 +267,18 @@ def __generate_rsa_key_pair(owner): return private_key_bytes -def generate_user_key_pair(user): +def generate_user_key_pair(user, password): private_key_bytes = __generate_rsa_key_pair(user) - __encrypt_user_private_key(user, private_key_bytes) + user_key = __derive_key(user, password) + __encrypt_owner_private_key(user, private_key_bytes, user_key) + del user_key del private_key_bytes gc.collect() def generate_invite_key_pair(invite): private_key_bytes = __generate_rsa_key_pair(invite) - temporary_key = __encrypt_invite_private_key(invite, private_key_bytes) + temporary_key = __encrypt_owner_private_key(invite, private_key_bytes) del private_key_bytes gc.collect() return temporary_key diff --git a/dds_web/security/tokens.py b/dds_web/security/tokens.py index 8a1443913..49ef01e81 100644 --- a/dds_web/security/tokens.py +++ b/dds_web/security/tokens.py @@ -57,7 +57,7 @@ def update_token_with_mfa(token_claims): ) return encrypted_jwt_token( username=token_claims.get("sub"), - sensitive_content=None, + sensitive_content=token_claims.get("sen_con"), expires_in=expires_in, additional_claims={"mfa_auth_time": dds_web.utils.current_time().timestamp()}, ) diff --git a/tests/conftest.py b/tests/conftest.py index 159137335..c3d9b11a7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,5 @@ # Standard Library import os -import uuid import unittest.mock import datetime import subprocess @@ -33,6 +32,7 @@ generate_project_key_pair, share_project_private_key, ) +from dds_web.security.tokens import encrypted_jwt_token mysql_root_password = os.getenv("MYSQL_ROOT_PASSWORD") DATABASE_URI_BASE = f"mysql+pymysql://root:{mysql_root_password}@db/DeliverySystemTestBase" @@ -60,9 +60,33 @@ def fill_basic_db(db): db.session.commit() - share_project_private_key(users[2], users[0], projects[0]) - share_project_private_key(users[2], users[1], projects[0]) - share_project_private_key(users[3], users[6], projects[3]) + user2_token = encrypted_jwt_token( + username=users[2].username, + sensitive_content="password", + ) + share_project_private_key( + from_user=users[2], + to_another=users[0], + from_user_token=user2_token, + project=projects[0], + ) + share_project_private_key( + from_user=users[2], + to_another=users[1], + from_user_token=user2_token, + project=projects[0], + ) + + user3_token = encrypted_jwt_token( + username=users[3].username, + sensitive_content="password", + ) + share_project_private_key( + from_user=users[3], + to_another=users[6], + from_user_token=user3_token, + project=projects[3], + ) db.session.commit() diff --git a/tests/test_basic_api.py b/tests/test_basic_api.py index 0ff583f62..8fd4bdf61 100644 --- a/tests/test_basic_api.py +++ b/tests/test_basic_api.py @@ -1,13 +1,11 @@ # IMPORTS ################################################################################ IMPORTS # # Standard library -from cryptography.hazmat.primitives.twofactor.hotp import HOTP import flask import http import datetime # Installed -from jwcrypto import jwk, jws import pytest # Own @@ -16,6 +14,7 @@ from dds_web import db from dds_web.security.auth import decrypt_and_verify_token_signature + # TESTS #################################################################################### TESTS # # Partial Token #################################################################### Partial Token # diff --git a/tests/test_project_user_keys.py b/tests/test_project_user_keys.py index 0ab8b0ff2..4f5ba01ad 100644 --- a/tests/test_project_user_keys.py +++ b/tests/test_project_user_keys.py @@ -3,31 +3,22 @@ from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric.padding import MGF1, OAEP -from cryptography.hazmat.primitives.asymmetric.rsa import RSAPublicKey, RSAPrivateKey -from cryptography.hazmat.primitives.asymmetric.x25519 import X25519PublicKey, X25519PrivateKey -from cryptography.hazmat.primitives.ciphers.aead import AESGCM +from cryptography.hazmat.primitives.asymmetric.rsa import RSAPublicKey +from cryptography.hazmat.primitives.asymmetric.x25519 import X25519PublicKey from cryptography.hazmat.primitives.hashes import SHA256 import dds_web import tests from dds_web.database import models -from dds_web.security.auth import verify_invite_key from dds_web.security.project_user_keys import ( generate_invite_key_pair, share_project_private_key, - transfer_invite_private_key_to_user, + verify_and_transfer_invite_to_user, ) from dds_web.security.tokens import encrypted_jwt_token from tests.test_user_delete import user_from_email -def __aes_decrypt(user): - aesgcm = AESGCM(user.temporary_key) - return aesgcm.decrypt( - user.nonce, user.private_key, b"private key for " + user.username.encode() - ) - - def __padding(): return OAEP( mgf=MGF1(algorithm=SHA256()), @@ -37,16 +28,11 @@ def __padding(): def test_user_key_generation(client): - user = models.User(username="testuser") + user = models.User(username="testuser", password="password") assert user.public_key is not None assert isinstance(serialization.load_der_public_key(user.public_key), RSAPublicKey) - assert user.temporary_key is not None assert user.nonce is not None assert user.private_key is not None - private_key_bytes = __aes_decrypt(user) - assert isinstance( - serialization.load_der_private_key(private_key_bytes, password=None), RSAPrivateKey - ) def test_project_key_generation(client): @@ -65,15 +51,8 @@ def test_project_key_generation(client): number_of_unitusers_with_project_key += 1 assert number_of_unitusers_with_project_key == 3 user = project_user_keys[0].user - assert user.temporary_key is not None assert user.nonce is not None assert user.private_key is not None - user_private_key_bytes = __aes_decrypt(user) - user_private_key = serialization.load_der_private_key(user_private_key_bytes, password=None) - project_private_key_bytes = user_private_key.decrypt(project_user_keys[0].key, __padding()) - assert isinstance( - X25519PrivateKey.from_private_bytes(project_private_key_bytes), X25519PrivateKey - ) def test_project_key_sharing(client): @@ -84,33 +63,16 @@ def test_project_key_sharing(client): project_id=project.id, user_id=researchuser.username ).first() assert project_researchuser_key is not None - assert researchuser.temporary_key is not None assert researchuser.nonce is not None assert researchuser.private_key is not None - researchuser_private_key_bytes = __aes_decrypt(researchuser) - researchuser_private_key = serialization.load_der_private_key( - researchuser_private_key_bytes, password=None - ) - project_private_key_bytes = researchuser_private_key.decrypt( - project_researchuser_key.key, __padding() - ) unituser = models.User.query.filter_by(username="unituser").first() project_unituser_key = models.ProjectUserKeys.query.filter_by( project_id=project.id, user_id=unituser.username ).first() assert project_unituser_key is not None - assert unituser.temporary_key is not None assert unituser.nonce is not None assert unituser.private_key is not None - unituser_private_key_bytes = __aes_decrypt(unituser) - unituser_private_key = serialization.load_der_private_key( - unituser_private_key_bytes, password=None - ) - assert ( - unituser_private_key.decrypt(project_unituser_key.key, __padding()) - == project_private_key_bytes - ) def test_delete_user_deletes_project_user_keys(client): @@ -182,15 +144,24 @@ def test_share_project_keys_via_two_invites(client): # unituser invites a new Unit Personnel invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") temporary_key = generate_invite_key_pair(invite1) - token1 = encrypted_jwt_token( + invite_token1 = encrypted_jwt_token( username="", sensitive_content=temporary_key.hex(), additional_claims={"inv": invite1.email}, ) unituser = models.User.query.filter_by(username="unituser").first() unituser.unit.invites.append(invite1) + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) for project in unituser.unit.projects: - share_project_private_key(unituser, invite1, project) + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=project, + ) dds_web.db.session.commit() # ************************************ @@ -207,7 +178,7 @@ def test_share_project_keys_via_two_invites(client): new_user.emails.append(new_email) new_user.active = True dds_web.db.session.add(new_user) - transfer_invite_private_key_to_user(invite1, verify_invite_key(token1), new_user) + verify_and_transfer_invite_to_user(invite_token1, new_user, common_user_fields["password"]) for project_invite_key in invite1.project_invite_keys: project_user_key = models.ProjectUserKeys( project_id=project_invite_key.project_id, @@ -217,7 +188,6 @@ def test_share_project_keys_via_two_invites(client): dds_web.db.session.add(project_user_key) dds_web.db.session.delete(project_invite_key) - assert temporary_key == new_user.temporary_key assert invite1.nonce != new_user.nonce assert invite1.public_key == new_user.public_key assert invite1.private_key != new_user.private_key @@ -229,15 +199,24 @@ def test_share_project_keys_via_two_invites(client): # new Unit Personnel invites another new Unit Personnel invite2 = models.Invite(email="another_unit_user@mailtrap.io", role="Unit Personnel") - token2 = encrypted_jwt_token( + invite_token2 = encrypted_jwt_token( username="", sensitive_content=generate_invite_key_pair(invite2).hex(), additional_claims={"inv": invite2.email}, ) unituser = models.User.query.filter_by(username="user_not_existing").first() unituser.unit.invites.append(invite2) + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content=common_user_fields["password"], + ) for project in unituser.unit.projects: - share_project_private_key(unituser, invite2, project) + share_project_private_key( + from_user=unituser, + to_another=invite2, + from_user_token=unituser_token, + project=project, + ) dds_web.db.session.commit() project_invite_keys = invite2.project_invite_keys diff --git a/tests/test_token.py b/tests/test_token.py index 2f3b9544c..e6ef41fd2 100644 --- a/tests/test_token.py +++ b/tests/test_token.py @@ -2,10 +2,10 @@ import pytest -from dds_web.errors import AuthenticationError +from dds_web.errors import AuthenticationError, TokenMissingError from dds_web.security.tokens import encrypted_jwt_token, jwt_token from dds_web.security.auth import ( - extract_encrypted_token_content, + extract_encrypted_token_sensitive_content, decrypt_and_verify_token_signature, verify_invite_token, matching_email_with_invite, @@ -17,16 +17,22 @@ def test_encrypted_data_transfer_via_token(client): username = "researchuser" sensitive_content = "sensitive_content" encrypted_token = encrypted_jwt_token(username, sensitive_content) - extracted_content = extract_encrypted_token_content(encrypted_token, username) + extracted_content = extract_encrypted_token_sensitive_content(encrypted_token, username) assert sensitive_content == extracted_content def test_encrypted_data_destined_for_another_user(client): encrypted_token = encrypted_jwt_token("researchuser", "sensitive_content") - extracted_content = extract_encrypted_token_content(encrypted_token, "projectowner") + extracted_content = extract_encrypted_token_sensitive_content(encrypted_token, "projectowner") assert extracted_content is None +def test_extract_encrypted_token_sensitive_content_no_token(client): + with pytest.raises(TokenMissingError) as error: + extract_encrypted_token_sensitive_content(None, "projectowner") + assert "There is no token to extract sensitive content from." in str(error.value) + + def test_encrypted_and_signed_token(client): username = "researchuser" expires_in = datetime.timedelta(minutes=1) diff --git a/tests/test_user_invite_no_project.py b/tests/test_user_invite_no_project.py index 48a32a0d9..17d384164 100644 --- a/tests/test_user_invite_no_project.py +++ b/tests/test_user_invite_no_project.py @@ -165,7 +165,6 @@ def test_successful_registration(registry_form_data, client): user = models.User.query.filter_by(username=registry_form_data["username"]).one_or_none() assert user is not None - assert user.temporary_key is not None assert user.nonce is not None assert user.public_key is not None assert user.private_key is not None From 11028524bfe97c01c5814b4421f9ad218b46abf7 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Wed, 16 Feb 2022 10:58:33 +0100 Subject: [PATCH 2/9] Handle exceptional situation with the sensitive content in the encrypted token, Add tests for invite and encrypted token, and authentication --- dds_web/security/auth.py | 5 +- tests/test_token.py | 98 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/dds_web/security/auth.py b/dds_web/security/auth.py index 8401770a4..214a93b76 100644 --- a/dds_web/security/auth.py +++ b/dds_web/security/auth.py @@ -151,7 +151,10 @@ def extract_token_invite_key(token): if not invite: raise InviteError(message="Invite could not be found!") - return invite, bytes.fromhex(claims.get("sen_con")) + try: + return invite, bytes.fromhex(claims.get("sen_con")) + except ValueError: + raise ValueError("Temporary key is expected be in hexadecimal digits for a byte string.") def obtain_current_encrypted_token(): diff --git a/tests/test_token.py b/tests/test_token.py index e6ef41fd2..ff5a3d224 100644 --- a/tests/test_token.py +++ b/tests/test_token.py @@ -2,7 +2,8 @@ import pytest -from dds_web.errors import AuthenticationError, TokenMissingError +import tests +from dds_web.errors import AuthenticationError, TokenMissingError, InviteError from dds_web.security.tokens import encrypted_jwt_token, jwt_token from dds_web.security.auth import ( extract_encrypted_token_sensitive_content, @@ -10,6 +11,10 @@ verify_invite_token, matching_email_with_invite, verify_token_no_data, + extract_token_invite_key, + obtain_current_encrypted_token, + obtain_current_encrypted_token_claims, + verify_token, ) @@ -148,3 +153,94 @@ def test_verify_token_no_data(client): assert user.username == "unitadmin" user = verify_token_no_data(jwt_token(username="unitadmin")) assert user.username == "unitadmin" + + +def test_extract_token_invite_key_with_wrong_token(client): + with pytest.raises(AuthenticationError) as error: + extract_token_invite_key(encrypted_jwt_token(username="unitadmin", sensitive_content=None)) + assert "Invalid token" in str(error.value) + + +def test_extract_token_invite_key_with_no_invite(client): + with pytest.raises(InviteError) as error: + extract_token_invite_key( + encrypted_jwt_token( + username="", + sensitive_content="bogus", + expires_in=datetime.timedelta(hours=24), + additional_claims={"inv": "bogus.tkek@bogus.com"}, + ) + ) + assert "Invite could not be found!" in str(error.value) + + +def test_extract_token_invite_key_with_wrong_format_for_key(client): + with pytest.raises(ValueError) as error: + extract_token_invite_key( + encrypted_jwt_token( + username="", + sensitive_content="bogus", + expires_in=datetime.timedelta(hours=24), + additional_claims={"inv": "existing_invite_email@mailtrap.io"}, + ) + ) + assert "Temporary key is expected be in hexadecimal digits for a byte string." in str( + error.value + ) + + +def test_extract_token_invite_key_successful(client): + invite, temporary_key = extract_token_invite_key( + encrypted_jwt_token( + username="", + sensitive_content=b"bogus".hex(), + expires_in=datetime.timedelta(hours=24), + additional_claims={"inv": "existing_invite_email@mailtrap.io"}, + ) + ) + assert invite + assert invite.email == "existing_invite_email@mailtrap.io" + assert temporary_key == b"bogus" + + +def test_obtain_current_encrypted_token_fails(client): + with pytest.raises(TokenMissingError) as error: + obtain_current_encrypted_token() + + assert "Encrypted token is required but missing!" in str(error.value) + + +def test_obtain_current_encrypted_token_succeeds(client): + initial_token = tests.UserAuth(tests.USER_CREDENTIALS["unitadmin"]).token(client) + + # Use an endpoint to put the token in the request header. + client.get( + tests.DDSEndpoint.PROJ_PUBLIC, + query_string={"project": "restricted_project_id"}, + headers=initial_token, + ) + + obtained_token = obtain_current_encrypted_token() + assert str(initial_token["Authorization"].split()[1]) == str(obtained_token) + + +def test_obtain_current_encrypted_token_claims(client): + initial_token = tests.UserAuth(tests.USER_CREDENTIALS["unitadmin"]).token(client) + + # Use an endpoint to put the token in the request header. + client.get( + tests.DDSEndpoint.PROJ_PUBLIC, + query_string={"project": "restricted_project_id"}, + headers=initial_token, + ) + + obtained_token_claims = obtain_current_encrypted_token_claims() + assert obtained_token_claims.get("sub") == "unitadmin" + + +def test_expired_encrypted_token(client): + token = encrypted_jwt_token("researchuser", None, expires_in=datetime.timedelta(seconds=-2)) + with pytest.raises(AuthenticationError) as error: + verify_token(token) + + assert "Expired token" in str(error.value) From eab59dd35376e286334218cdaa5466c3283e77ba Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Wed, 16 Feb 2022 13:47:50 +0100 Subject: [PATCH 3/9] Rename ARGON_MEMORY_COST as ARGON_KD_MEMORY_COST to avoid possible collision with the password hasher parameter --- dds_web/config.py | 2 +- dds_web/security/project_user_keys.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dds_web/config.py b/dds_web/config.py index d93b80719..b5052cae7 100644 --- a/dds_web/config.py +++ b/dds_web/config.py @@ -64,7 +64,7 @@ class Config(object): INVITATION_EXPIRES_IN_HOURS = 7 * 24 # 512MiB; at least 4GiB (0x400000) recommended in production - ARGON_MEMORY_COST = os.environ.get("ARGON_MEMORY_COST", 0x80000) + ARGON_KD_MEMORY_COST = os.environ.get("ARGON_KD_MEMORY_COST", 0x80000) SUPERADMIN_USERNAME = os.environ.get("DDS_SUPERADMIN_USERNAME", "superadmin") SUPERADMIN_PASSWORD = os.environ.get("DDS_SUPERADMIN_PASSWORD", "password") diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index c83fb35ef..b7e2565d8 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -28,7 +28,7 @@ def __derive_key(user, password): secret=password.encode(), salt=user.kd_salt, time_cost=2, - memory_cost=flask.current_app.config["ARGON_MEMORY_COST"], + memory_cost=flask.current_app.config["ARGON_KD_MEMORY_COST"], parallelism=8, hash_len=32, type=argon2.Type.ID, From d6758bcadf31b039bc853e25a4d659e7403967bd Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Wed, 16 Feb 2022 14:08:30 +0100 Subject: [PATCH 4/9] Improve handling of exceptions with key setup and operations, and Add tests for them --- dds_web/security/project_user_keys.py | 22 ++-- tests/test_project_user_keys.py | 177 ++++++++++++++++++++++++++ 2 files changed, 189 insertions(+), 10 deletions(-) diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index b7e2565d8..8261423ce 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -71,26 +71,28 @@ def __decrypt_with_rsa(ciphertext, private_key): def __encrypt_project_private_key(owner, project_private_key): - if owner.public_key: + if owner.public_key is None: + raise KeySetupError(message="User keys are not properly setup!") + + try: owner_public_key = serialization.load_der_public_key(owner.public_key) if isinstance(owner_public_key, asymmetric.rsa.RSAPublicKey): return __encrypt_with_rsa(project_private_key, owner_public_key) - raise KeyOperationError( - message="User public key cannot be loaded for encrypting the project private key!" - ) - raise KeySetupError(message="User keys are not properly setup!") + except ValueError: + raise KeyOperationError(message="User public key could not be loaded!") def __decrypt_project_private_key(user, token, encrypted_project_private_key): private_key_bytes = __decrypt_user_private_key(user, token) - if private_key_bytes: + if private_key_bytes is None: + raise KeyOperationError(message="User private key could not be decrypted!") + + try: user_private_key = serialization.load_der_private_key(private_key_bytes, password=None) if isinstance(user_private_key, asymmetric.rsa.RSAPrivateKey): return __decrypt_with_rsa(encrypted_project_private_key, user_private_key) - raise KeyOperationError( - message="User private key cannot be loaded for decrypting the project private key!" - ) - raise KeyOperationError(message="User private key cannot be decrypted!") + except ValueError: + raise KeyOperationError(message="User private key could not be loaded!") def obtain_project_private_key(user, token, project): diff --git a/tests/test_project_user_keys.py b/tests/test_project_user_keys.py index 4f5ba01ad..04eb98e74 100644 --- a/tests/test_project_user_keys.py +++ b/tests/test_project_user_keys.py @@ -1,6 +1,8 @@ import http import json +import uuid +import pytest from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives.asymmetric.padding import MGF1, OAEP from cryptography.hazmat.primitives.asymmetric.rsa import RSAPublicKey @@ -10,12 +12,20 @@ import dds_web import tests from dds_web.database import models +from dds_web.errors import ( + KeySetupError, + KeyOperationError, + KeyNotFoundError, + SensitiveContentMissingError, +) from dds_web.security.project_user_keys import ( generate_invite_key_pair, + generate_user_key_pair, share_project_private_key, verify_and_transfer_invite_to_user, ) from dds_web.security.tokens import encrypted_jwt_token +from dds_web.utils import timestamp from tests.test_user_delete import user_from_email @@ -27,6 +37,173 @@ def __padding(): ) +def test_user_key_setup_error_with_salt(client): + # user is created without a password, so salt will be missing + user = models.User(username="testuser") + with pytest.raises(KeySetupError) as error: + generate_user_key_pair(user, "password") + + assert "User keys are not properly setup!" in str(error.value) + + +def test_user_key_setup_error_with_private_key(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + unituser = models.User.query.filter_by(username="unituser").first() + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) + + # Somehow private key has disappeared + unituser.private_key = None + + with pytest.raises(KeySetupError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "User keys are not properly setup!" in str(error.value) + + +def test_user_key_setup_error_with_nonce(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + unituser = models.User.query.filter_by(username="unituser").first() + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) + + # Somehow nonce has disappeared + unituser.nonce = None + + with pytest.raises(KeySetupError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "User keys are not properly setup!" in str(error.value) + + +def test_user_key_setup_error_with_public_key(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + + # Somehow the key pair for the invite has not taken place or disappeared + + unituser = models.User.query.filter_by(username="unituser").first() + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) + with pytest.raises(KeySetupError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "User keys are not properly setup!" in str(error.value) + + +def test_user_key_operation_error_with_load_user_public_key(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + generate_invite_key_pair(invite1) + unituser = models.User.query.filter_by(username="unituser").first() + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) + + # Somehow the public key of the invite is not the expected public key + invite1.public_key = b"useless_bytes" + + with pytest.raises(KeyOperationError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "User public key could not be loaded!" in str(error.value) + + +def test_user_key_operation_error_with_decrypt_user_private_key(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + unituser = models.User.query.filter_by(username="unituser").first() + + # Somehow a wrong password has ended up in the encrypted token + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="passwor", + ) + with pytest.raises(KeyOperationError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "User private key could not be decrypted!" in str(error.value) + + +def test_sensitive_content_missing_error(client): + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + unituser = models.User.query.filter_by(username="unituser").first() + + # Somehow the password is missing in the encrypted token + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content=None, + ) + with pytest.raises(SensitiveContentMissingError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=unituser.unit.projects[0], + ) + + assert "Sensitive content is missing in the encrypted token!" in str(error.value) + + +def test_user_key_not_found_error_for_project(client): + project_without_keys = models.Project( + public_id="random_project_id", + title="random project_title", + description="This is a random project. ", + pi="PI", + bucket=f"publicproj-{str(timestamp(ts_format='%Y%m%d%H%M%S'))}-{str(uuid.uuid4())}", + ) + + # Somehow the key pair for the project is not created or persisted to the database + + invite1 = models.Invite(email="new_unit_user@mailtrap.io", role="Unit Personnel") + unituser = models.User.query.filter_by(username="unituser").first() + unituser.unit.projects.append(project_without_keys) + dds_web.db.session.commit() + unituser_token = encrypted_jwt_token( + username=unituser.username, + sensitive_content="password", + ) + with pytest.raises(KeyNotFoundError) as error: + share_project_private_key( + from_user=unituser, + to_another=invite1, + from_user_token=unituser_token, + project=project_without_keys, + ) + + assert "Unrecoverable key error. Aborting." in str(error.value) + + def test_user_key_generation(client): user = models.User(username="testuser", password="password") assert user.public_key is not None From ffc69700b0223a0a4e1d824d458361d6027aae30 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Wed, 16 Feb 2022 18:28:24 +0100 Subject: [PATCH 5/9] Implement update of user keys at validated password change, Add test for this and Clean up some unused imports --- dds_web/forms.py | 3 -- dds_web/security/project_user_keys.py | 41 +++++++++++++++---- dds_web/web/user.py | 8 +++- tests/test_project_user_keys.py | 59 +++++++++++++++++++++++++++ 4 files changed, 99 insertions(+), 12 deletions(-) diff --git a/dds_web/forms.py b/dds_web/forms.py index 1c8e09e63..14b38646f 100644 --- a/dds_web/forms.py +++ b/dds_web/forms.py @@ -3,17 +3,14 @@ # IMPORTS ################################################################################ IMPORTS # # Standard library -import re # Installed import flask_wtf import flask_login import wtforms -import marshmallow # Own modules from dds_web import utils -from dds_web.database import models # FORMS #################################################################################### FORMS # diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index 8261423ce..882ba8c79 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -83,7 +83,7 @@ def __encrypt_project_private_key(owner, project_private_key): def __decrypt_project_private_key(user, token, encrypted_project_private_key): - private_key_bytes = __decrypt_user_private_key(user, token) + private_key_bytes = __decrypt_user_private_key_via_token(user, token) if private_key_bytes is None: raise KeyOperationError(message="User private key could not be decrypted!") @@ -198,12 +198,7 @@ def __encrypt_owner_private_key(owner, private_key, owner_key=None): return key -def __decrypt_user_private_key(user, token): - password = extract_encrypted_token_sensitive_content(token, user.username) - if password is None: - raise SensitiveContentMissingError - user_key = __derive_key(user, password) - +def __decrypt_user_private_key(user, user_key): if user.private_key and user.nonce: return __decrypt_with_aes( user_key, @@ -214,6 +209,15 @@ def __decrypt_user_private_key(user, token): raise KeySetupError(message="User keys are not properly setup!") +def __decrypt_user_private_key_via_token(user, token): + password = extract_encrypted_token_sensitive_content(token, user.username) + if password is None: + raise SensitiveContentMissingError + user_key = __derive_key(user, password) + + return __decrypt_user_private_key(user, user_key) + + def __decrypt_invite_private_key(invite, temporary_key): """Decrypt invite private key.""" if temporary_key and invite.private_key and invite.nonce: @@ -225,6 +229,29 @@ def __decrypt_invite_private_key(invite, temporary_key): ) +def update_user_keys_for_password_change(user, current_password, new_password): + """ + Updates the user key (key encryption key) and the encrypted user private key + + :param user: a user object from the models, its password is about to change + :param current_password: the password that is being replaced. It is expected to be validated via its web form. + :param new_password: the password that is replacing the previous one. It is expected to be validated via its web form. + """ + old_user_key = __derive_key(user, current_password) + private_key_bytes = __decrypt_user_private_key(user, old_user_key) + if private_key_bytes is None: + raise KeyOperationError(message="User private key could not be decrypted!") + + user.kd_salt = os.urandom(32) + new_user_key = __derive_key(user, new_password) + __encrypt_owner_private_key(user, private_key_bytes, new_user_key) + + del new_user_key + del old_user_key + del private_key_bytes + gc.collect() + + def verify_and_transfer_invite_to_user(token, user, password): invite, temporary_key = extract_token_invite_key(token) private_key_bytes = __verify_invite_temporary_key(invite, temporary_key) diff --git a/dds_web/web/user.py b/dds_web/web/user.py index 29fc6230e..029419887 100644 --- a/dds_web/web/user.py +++ b/dds_web/web/user.py @@ -15,7 +15,6 @@ import flask_login import itsdangerous import sqlalchemy -import marshmallow # Own Modules from dds_web import forms @@ -27,7 +26,7 @@ from dds_web.api.schemas import user_schemas import dds_web.security from dds_web.api.user import DeleteUser - +from dds_web.security.project_user_keys import update_user_keys_for_password_change auth_blueprint = flask.Blueprint("auth_blueprint", __name__) @@ -368,6 +367,11 @@ def change_password(): if form.validate_on_submit(): # Change password flask_login.current_user.password = form.new_password.data + update_user_keys_for_password_change( + user=flask_login.current_user, + current_password=form.current_password.data, + new_password=form.new_password.data, + ) db.session.commit() flask_login.logout_user() diff --git a/tests/test_project_user_keys.py b/tests/test_project_user_keys.py index 04eb98e74..a26805927 100644 --- a/tests/test_project_user_keys.py +++ b/tests/test_project_user_keys.py @@ -23,6 +23,7 @@ generate_user_key_pair, share_project_private_key, verify_and_transfer_invite_to_user, + update_user_keys_for_password_change, ) from dds_web.security.tokens import encrypted_jwt_token from dds_web.utils import timestamp @@ -409,3 +410,61 @@ def test_share_project_keys_via_two_invites(client): number_of_asserted_projects += 1 assert len(project_invite_keys) == number_of_asserted_projects assert len(project_invite_keys) == 5 + + +def test_update_user_keys_for_password_change(client): + user = models.User(username="randomtestuser", password="password") + + public_key_initial = user.public_key + nonce_initial = user.nonce + private_key_initial = user.private_key + kd_salt_initial = user.kd_salt + + assert public_key_initial is not None + assert nonce_initial is not None + assert private_key_initial is not None + assert kd_salt_initial is not None + + update_user_keys_for_password_change(user, "password", "bogus") + user.password = "bogus" + + public_key_after_password_change = user.public_key + nonce_after_password_change = user.nonce + private_key_after_password_change = user.private_key + kd_salt_after_password_change = user.kd_salt + + assert public_key_after_password_change is not None + assert nonce_after_password_change is not None + assert private_key_after_password_change is not None + assert kd_salt_after_password_change is not None + + assert public_key_after_password_change == public_key_initial + assert nonce_after_password_change != nonce_initial + assert private_key_after_password_change != private_key_initial + assert kd_salt_after_password_change != kd_salt_initial + + # It shouldn't matter whichever comes first between set password + # and update user keys as the password is not stored in database + + user.password = "password" + update_user_keys_for_password_change(user, "bogus", "password") + + public_key_final = user.public_key + nonce_final = user.nonce + private_key_final = user.private_key + kd_salt_final = user.kd_salt + + assert public_key_final is not None + assert nonce_final is not None + assert private_key_final is not None + assert kd_salt_final is not None + + assert public_key_final == public_key_initial + assert nonce_final != nonce_initial + assert private_key_final != private_key_initial + assert kd_salt_final != kd_salt_initial + + assert public_key_after_password_change == public_key_final + assert nonce_after_password_change != nonce_final + assert private_key_after_password_change != private_key_final + assert kd_salt_after_password_change != kd_salt_final From be2d00ad8eedaae79e8d7099a8a562fa19b82943 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Thu, 17 Feb 2022 10:28:39 +0100 Subject: [PATCH 6/9] Add a short description of this work to the changelog --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9b4182e3..f57ae8d6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,5 @@ # Data Delivery System Web / API: Changelog -Please add a _short_ line describing the PR you make, if the PR implements a specific feature or functionality, or refactor. Not needed if you add very small and unnoticable changes. \ No newline at end of file +Please add a _short_ line describing the PR you make, if the PR implements a specific feature or functionality, or refactor. Not needed if you add very small and unnoticable changes. + +## Sprint (2022-02-09 - 2022-02-23) +* Secure operations that require cryptographic keys are protected for each user with the user's password ([#889](https://github.com/ScilifelabDataCentre/dds_web/pull/889)) \ No newline at end of file From 4cf24dcae9b374a8e0bc88c8bc414e9ccdeb109a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Linus=20=C3=96stberg?= Date: Thu, 17 Feb 2022 09:58:43 +0100 Subject: [PATCH 7/9] Add a template for pull requests --- .github/pull_request_template.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..badf771ae --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,6 @@ +Before submitting a pr: +- [ ] Tests passing +- [ ] Black formatting +- [ ] Rebase/merge the `dev` branch +- [ ] Note in the CHANGELOG + From 1b2d0d41013fa021dbc81ab44a2c33f97b8112e2 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Thu, 17 Feb 2022 12:04:31 +0100 Subject: [PATCH 8/9] Changes according to pr comments --- dds_web/security/project_user_keys.py | 14 ++++---- tests/test_project_user_keys.py | 48 +++++++++++++-------------- 2 files changed, 30 insertions(+), 32 deletions(-) diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index 882ba8c79..2d7aa6e2c 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -21,7 +21,7 @@ def __derive_key(user, password): - if user.kd_salt is None: + if not user.kd_salt: raise KeySetupError(message="User keys are not properly setup!") derived_key = argon2.low_level.hash_secret_raw( @@ -71,7 +71,7 @@ def __decrypt_with_rsa(ciphertext, private_key): def __encrypt_project_private_key(owner, project_private_key): - if owner.public_key is None: + if not owner.public_key: raise KeySetupError(message="User keys are not properly setup!") try: @@ -84,7 +84,7 @@ def __encrypt_project_private_key(owner, project_private_key): def __decrypt_project_private_key(user, token, encrypted_project_private_key): private_key_bytes = __decrypt_user_private_key_via_token(user, token) - if private_key_bytes is None: + if not private_key_bytes: raise KeyOperationError(message="User private key could not be decrypted!") try: @@ -186,9 +186,7 @@ def __owner_identifier(owner): def __encrypt_owner_private_key(owner, private_key, owner_key=None): - key = owner_key - if key is None: - key = ciphers.aead.AESGCM.generate_key(bit_length=256) + key = owner_key or ciphers.aead.AESGCM.generate_key(bit_length=256) nonce, encrypted_key = __encrypt_with_aes( key, private_key, aad=b"private key for " + __owner_identifier(owner).encode() @@ -211,7 +209,7 @@ def __decrypt_user_private_key(user, user_key): def __decrypt_user_private_key_via_token(user, token): password = extract_encrypted_token_sensitive_content(token, user.username) - if password is None: + if not password: raise SensitiveContentMissingError user_key = __derive_key(user, password) @@ -239,7 +237,7 @@ def update_user_keys_for_password_change(user, current_password, new_password): """ old_user_key = __derive_key(user, current_password) private_key_bytes = __decrypt_user_private_key(user, old_user_key) - if private_key_bytes is None: + if not private_key_bytes: raise KeyOperationError(message="User private key could not be decrypted!") user.kd_salt = os.urandom(32) diff --git a/tests/test_project_user_keys.py b/tests/test_project_user_keys.py index a26805927..d7cfbbef6 100644 --- a/tests/test_project_user_keys.py +++ b/tests/test_project_user_keys.py @@ -207,16 +207,16 @@ def test_user_key_not_found_error_for_project(client): def test_user_key_generation(client): user = models.User(username="testuser", password="password") - assert user.public_key is not None + assert user.public_key assert isinstance(serialization.load_der_public_key(user.public_key), RSAPublicKey) - assert user.nonce is not None - assert user.private_key is not None + assert user.nonce + assert user.private_key def test_project_key_generation(client): # Setup is done in conftest.py project = models.Project.query.filter_by(public_id="public_project_id").first() - assert project.public_key is not None + assert project.public_key assert isinstance(X25519PublicKey.from_public_bytes(project.public_key), X25519PublicKey) number_of_unitusers_with_project_key = 0 project_user_keys = project.project_user_keys @@ -229,8 +229,8 @@ def test_project_key_generation(client): number_of_unitusers_with_project_key += 1 assert number_of_unitusers_with_project_key == 3 user = project_user_keys[0].user - assert user.nonce is not None - assert user.private_key is not None + assert user.nonce + assert user.private_key def test_project_key_sharing(client): @@ -240,17 +240,17 @@ def test_project_key_sharing(client): project_researchuser_key = models.ProjectUserKeys.query.filter_by( project_id=project.id, user_id=researchuser.username ).first() - assert project_researchuser_key is not None - assert researchuser.nonce is not None - assert researchuser.private_key is not None + assert project_researchuser_key + assert researchuser.nonce + assert researchuser.private_key unituser = models.User.query.filter_by(username="unituser").first() project_unituser_key = models.ProjectUserKeys.query.filter_by( project_id=project.id, user_id=unituser.username ).first() - assert project_unituser_key is not None - assert unituser.nonce is not None - assert unituser.private_key is not None + assert project_unituser_key + assert unituser.nonce + assert unituser.private_key def test_delete_user_deletes_project_user_keys(client): @@ -420,10 +420,10 @@ def test_update_user_keys_for_password_change(client): private_key_initial = user.private_key kd_salt_initial = user.kd_salt - assert public_key_initial is not None - assert nonce_initial is not None - assert private_key_initial is not None - assert kd_salt_initial is not None + assert public_key_initial + assert nonce_initial + assert private_key_initial + assert kd_salt_initial update_user_keys_for_password_change(user, "password", "bogus") user.password = "bogus" @@ -433,10 +433,10 @@ def test_update_user_keys_for_password_change(client): private_key_after_password_change = user.private_key kd_salt_after_password_change = user.kd_salt - assert public_key_after_password_change is not None - assert nonce_after_password_change is not None - assert private_key_after_password_change is not None - assert kd_salt_after_password_change is not None + assert public_key_after_password_change + assert nonce_after_password_change + assert private_key_after_password_change + assert kd_salt_after_password_change assert public_key_after_password_change == public_key_initial assert nonce_after_password_change != nonce_initial @@ -454,10 +454,10 @@ def test_update_user_keys_for_password_change(client): private_key_final = user.private_key kd_salt_final = user.kd_salt - assert public_key_final is not None - assert nonce_final is not None - assert private_key_final is not None - assert kd_salt_final is not None + assert public_key_final + assert nonce_final + assert private_key_final + assert kd_salt_final assert public_key_final == public_key_initial assert nonce_final != nonce_initial From 9c19c3082e511c82a6c5d27dd9d1d256ca324428 Mon Sep 17 00:00:00 2001 From: Volkan Cambazoglu Date: Thu, 17 Feb 2022 15:23:49 +0100 Subject: [PATCH 9/9] Update get project private endpoint according to the new key operations with token, Add tests for it, Rearrange and clarify some key related parameters, Clean up some unused imports --- dds_web/api/project.py | 10 +++++++++- dds_web/security/project_user_keys.py | 14 +++++++++++--- tests/test_project_listing.py | 22 ++++++++++++++++++++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/dds_web/api/project.py b/dds_web/api/project.py index ac3aefaf0..dca60f728 100644 --- a/dds_web/api/project.py +++ b/dds_web/api/project.py @@ -240,7 +240,15 @@ def get(self): flask.current_app.logger.debug("Getting the private key.") return flask.jsonify( - {"private": obtain_project_private_key(auth.current_user(), project).hex().upper()} + { + "private": obtain_project_private_key( + user=auth.current_user(), + project=project, + token=dds_web.security.auth.obtain_current_encrypted_token(), + ) + .hex() + .upper() + } ) diff --git a/dds_web/security/project_user_keys.py b/dds_web/security/project_user_keys.py index 2d7aa6e2c..50153551c 100644 --- a/dds_web/security/project_user_keys.py +++ b/dds_web/security/project_user_keys.py @@ -95,7 +95,7 @@ def __decrypt_project_private_key(user, token, encrypted_project_private_key): raise KeyOperationError(message="User private key could not be loaded!") -def obtain_project_private_key(user, token, project): +def obtain_project_private_key(user, project, token): project_key = models.ProjectUserKeys.query.filter_by( project_id=project.id, user_id=user.username ).first() @@ -107,11 +107,19 @@ def obtain_project_private_key(user, token, project): def share_project_private_key(from_user, to_another, from_user_token, project): if isinstance(to_another, models.Invite): __init_and_append_project_invite_key( - to_another, project, obtain_project_private_key(from_user, from_user_token, project) + invite=to_another, + project=project, + project_private_key=obtain_project_private_key( + user=from_user, project=project, token=from_user_token + ), ) else: __init_and_append_project_user_key( - to_another, project, obtain_project_private_key(from_user, from_user_token, project) + user=to_another, + project=project, + project_private_key=obtain_project_private_key( + user=from_user, project=project, token=from_user_token + ), ) diff --git a/tests/test_project_listing.py b/tests/test_project_listing.py index cb216280d..c99731cf2 100644 --- a/tests/test_project_listing.py +++ b/tests/test_project_listing.py @@ -2,13 +2,11 @@ # Standard library import http -import json import pytest import marshmallow import unittest # Own -from dds_web import db from dds_web.database import models import tests @@ -43,6 +41,26 @@ def test_list_proj_access_granted_ls(client): assert "public_project_id" == list_of_projects[0].get("Project ID") +def test_proj_private_successful(client): + """Successfully get the private key""" + + token = tests.UserAuth(tests.USER_CREDENTIALS["unituser"]).token(client) + response = client.get(tests.DDSEndpoint.PROJ_PRIVATE, query_string=proj_query, headers=token) + assert response.status_code == http.HTTPStatus.OK + response_json = response.json + assert response_json.get("private") + + +def test_proj_private_without_project(client): + """Attempting to get the private key without specifying a project""" + + token = tests.UserAuth(tests.USER_CREDENTIALS["unituser"]).token(client) + with pytest.raises(marshmallow.ValidationError) as error: + client.get(tests.DDSEndpoint.PROJ_PRIVATE, headers=token) + assert "project" in str(error.value) + assert "Missing data for required field." in str(error.value) + + def test_proj_public_no_token(client): """Attempting to get the public key without a token should not work"""