From ab7850bbdcded8b0e36cb27a2468f55d1910c218 Mon Sep 17 00:00:00 2001 From: drebs Date: Tue, 12 Aug 2014 11:06:23 -0300 Subject: Fix bits from pullreq review. --- client/src/leap/soledad/client/__init__.py | 2 +- client/src/leap/soledad/client/crypto.py | 19 +++++--- client/src/leap/soledad/client/mp_safe_db.py | 2 +- client/src/leap/soledad/client/secrets.py | 69 +++++++++++++++++----------- client/src/leap/soledad/client/sqlcipher.py | 51 ++++++++++++-------- client/src/leap/soledad/client/target.py | 7 +-- 6 files changed, 90 insertions(+), 60 deletions(-) (limited to 'client/src') diff --git a/client/src/leap/soledad/client/__init__.py b/client/src/leap/soledad/client/__init__.py index 0b72be27..c76e4a4a 100644 --- a/client/src/leap/soledad/client/__init__.py +++ b/client/src/leap/soledad/client/__init__.py @@ -755,7 +755,7 @@ class Soledad(object): @property def remote_storage_secret(self): """ - Return the secret used for encryption of remotelly stored data. + Return the secret used for encryption of remotely stored data. """ return self._secrets.remote_storage_secret diff --git a/client/src/leap/soledad/client/crypto.py b/client/src/leap/soledad/client/crypto.py index a24f2053..5e3760b3 100644 --- a/client/src/leap/soledad/client/crypto.py +++ b/client/src/leap/soledad/client/crypto.py @@ -863,7 +863,7 @@ class SyncDecrypterPool(SyncEncryptDecryptPool): :param encrypted: If not None, only return documents with encrypted field equal to given parameter. - :type encrypted: bool + :type encrypted: bool or None :return: list of doc_id, rev, generation, gen, trans_id :rtype: list @@ -878,16 +878,23 @@ class SyncDecrypterPool(SyncEncryptDecryptPool): def get_insertable_docs_by_gen(self): """ - Return a list of documents ready to be inserted. + Return a list of non-encrypted documents ready to be inserted. """ + # here, we compare the list of all available docs with the list of + # decrypted docs and find the longest common prefix between these two + # lists. Note that the order of lists fetch matters: if instead we + # first fetch the list of decrypted docs and then the list of all + # docs, then some document might have been decrypted between these two + # calls, and if it is just the right doc then it might not be caught + # by the next loop. all_docs = self.get_docs_by_generation() decrypted_docs = self.get_docs_by_generation(encrypted=False) insertable = [] for doc_id, rev, _, gen, trans_id, encrypted in all_docs: try: - next_decrypted = decrypted_docs.next() - if doc_id == next_decrypted[0]: - content = next_decrypted[2] + next_doc_id, _, next_content, _, _, _ = decrypted_docs.next() + if doc_id == next_doc_id: + content = next_content insertable.append((doc_id, rev, content, gen, trans_id)) else: break @@ -901,7 +908,7 @@ class SyncDecrypterPool(SyncEncryptDecryptPool): :param encrypted: If not None, return count of documents with encrypted field equal to given parameter. - :type encrypted: bool + :type encrypted: bool or None :return: The count of documents. :rtype: int diff --git a/client/src/leap/soledad/client/mp_safe_db.py b/client/src/leap/soledad/client/mp_safe_db.py index 2c6b7e24..780b7153 100644 --- a/client/src/leap/soledad/client/mp_safe_db.py +++ b/client/src/leap/soledad/client/mp_safe_db.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -# crypto.py +# mp_safe_db.py # Copyright (C) 2014 LEAP # # This program is free software: you can redistribute it and/or modify diff --git a/client/src/leap/soledad/client/secrets.py b/client/src/leap/soledad/client/secrets.py index 55580692..b1c22371 100644 --- a/client/src/leap/soledad/client/secrets.py +++ b/client/src/leap/soledad/client/secrets.py @@ -69,21 +69,28 @@ logger = logging.getLogger(name=__name__) # Exceptions # -class NoStorageSecret(Exception): + +class SecretsException(Exception): + """ + Generic exception type raised by this module. + """ + + +class NoStorageSecret(SecretsException): """ Raised when trying to use a storage secret but none is available. """ pass -class PassphraseTooShort(Exception): +class PassphraseTooShort(SecretsException): """ Raised when trying to change the passphrase but the provided passphrase is too short. """ -class BootstrapSequenceError(Exception): +class BootstrapSequenceError(SecretsException): """ Raised when an attempt to generate a secret and store it in a recovery document on server failed. @@ -107,34 +114,35 @@ class SoledadSecrets(object): LOCAL_STORAGE_SECRET_LENGTH = 512 """ - The length of the secret used to derive a passphrase for the SQLCipher - database. + The length, in bytes, of the secret used to derive a passphrase for the + SQLCipher database. """ REMOTE_STORAGE_SECRET_LENGTH = 512 """ - The length of the secret used to derive an encryption key and a MAC auth - key for remote storage. + The length, in bytes, of the secret used to derive an encryption key and a + MAC auth key for remote storage. """ SALT_LENGTH = 64 """ - The length of the salt used to derive the key for the storage secret - encryption. + The length, in bytes, of the salt used to derive the key for the storage + secret encryption. """ GEN_SECRET_LENGTH = LOCAL_STORAGE_SECRET_LENGTH \ + REMOTE_STORAGE_SECRET_LENGTH \ + SALT_LENGTH # for sync db """ - The length of the secret to be generated. This includes local and remote - secrets, and the salt for deriving the sync db secret. + The length, in bytes, of the secret to be generated. This includes local + and remote secrets, and the salt for deriving the sync db secret. """ MINIMUM_PASSPHRASE_LENGTH = 6 """ - The minimum length for a passphrase. The passphrase length is only checked - when the user changes her passphrase, not when she instantiates Soledad. + The minimum length, in bytes, for a passphrase. The passphrase length is + only checked when the user changes her passphrase, not when she + instantiates Soledad. """ IV_SEPARATOR = ":" @@ -288,7 +296,7 @@ class SoledadSecrets(object): self._secrets[self._secret_id] += new_piece enlarged = True # store and save in shared db if needed - if mac is False or enlarged is True: + if not mac or enlarged: self._store_secrets() self._put_secrets_in_shared_db() @@ -443,13 +451,17 @@ class SoledadSecrets(object): raise WrongMac('Could not authenticate recovery document\'s ' 'contents.') # include secrets in the secret pool. - secrets = 0 + secret_count = 0 for secret_id, encrypted_secret in data[self.STORAGE_SECRETS_KEY].items(): if secret_id not in self._secrets: - secrets += 1 - self._secrets[secret_id] = \ - self._decrypt_storage_secret(encrypted_secret) - return secrets, mac + try: + self._secrets[secret_id] = \ + self._decrypt_storage_secret(encrypted_secret) + secret_count += 1 + except SecretsException as e: + logger.error("Failed to decrypt storage secret: %s" + % str(e)) + return secret_count, mac def _get_secrets_from_shared_db(self): """ @@ -512,10 +524,13 @@ class SoledadSecrets(object): :return: The decrypted storage secret. :rtype: str + + :raise SecretsException: Raised in case the decryption of the storage + secret fails for some reason. """ # calculate the encryption key if encrypted_secret_dict[self.KDF_KEY] != self.KDF_SCRYPT: - raise Exception("Unknown KDF in stored secret.") + raise SecretsException("Unknown KDF in stored secret.") key = scrypt.hash( self._passphrase_as_string(), # the salt is stored base64 encoded @@ -524,16 +539,16 @@ class SoledadSecrets(object): buflen=32, # we need a key with 256 bits (32 bytes). ) if encrypted_secret_dict[self.KDF_LENGTH_KEY] != len(key): - raise Exception("Wrong length of decryption key.") + raise SecretsException("Wrong length of decryption key.") if encrypted_secret_dict[self.CIPHER_KEY] != self.CIPHER_AES256: - raise Exception("Unknown cipher in stored secret.") + raise SecretsException("Unknown cipher in stored secret.") # recover the initial value and ciphertext iv, ciphertext = encrypted_secret_dict[self.SECRET_KEY].split( self.IV_SEPARATOR, 1) ciphertext = binascii.a2b_base64(ciphertext) decrypted_secret = self._crypto.decrypt_sym(ciphertext, key, iv=iv) if encrypted_secret_dict[self.LENGTH_KEY] != len(decrypted_secret): - raise Exception("Wrong length of decrypted secret.") + raise SecretsException("Wrong length of decrypted secret.") return decrypted_secret def _encrypt_storage_secret(self, decrypted_secret): @@ -729,8 +744,8 @@ class SoledadSecrets(object): :rtype: str """ return scrypt.hash( - self._get_local_storage_secret(), # the password - self._get_local_storage_salt(), # the salt + password=self._get_local_storage_secret(), + salt=self._get_local_storage_salt(), buflen=32, # we need a key with 256 bits (32 bytes) ) @@ -755,7 +770,7 @@ class SoledadSecrets(object): :rtype: str """ return scrypt.hash( - self._get_local_storage_secret(), # the password - self._get_sync_db_salt(), # the salt + password=self._get_local_storage_secret(), + salt=self._get_sync_db_salt(), buflen=32, # we need a key with 256 bits (32 bytes) ) diff --git a/client/src/leap/soledad/client/sqlcipher.py b/client/src/leap/soledad/client/sqlcipher.py index a7ddab24..b7de2fba 100644 --- a/client/src/leap/soledad/client/sqlcipher.py +++ b/client/src/leap/soledad/client/sqlcipher.py @@ -63,6 +63,7 @@ from leap.soledad.client.target import SoledadSyncTarget from leap.soledad.client.target import PendingReceivedDocsSyncError from leap.soledad.client.sync import SoledadSynchronizer from leap.soledad.client.mp_safe_db import MPSafeSQLiteDB +from leap.soledad.common import soledad_assert from leap.soledad.common.document import SoledadDocument @@ -262,13 +263,16 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase): self._crypto = crypto # define sync-db attrs + self._sqlcipher_file = sqlcipher_file + self._sync_db_key = sync_db_key self._sync_db = None self._sync_db_write_lock = None self._sync_enc_pool = None - self._init_sync_db(sqlcipher_file, sync_db_key) + self.sync_queue = None if self.defer_encryption: # initialize sync db + self._init_sync_db() # initialize syncing queue encryption pool self._sync_enc_pool = SyncEncrypterPool( self._crypto, self._sync_db, self._sync_db_write_lock) @@ -471,6 +475,8 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase): res = None # the following context manager blocks until the syncing lock can be # acquired. + if defer_decryption: + self._init_sync_db() with self.syncer(url, creds=creds) as syncer: # XXX could mark the critical section here... try: @@ -564,28 +570,27 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase): 'ALTER TABLE document ' 'ADD COLUMN syncable BOOL NOT NULL DEFAULT TRUE') - def _init_sync_db(self, sqlcipher_file, sync_db_password): + def _init_sync_db(self): """ Initialize the Symmetrically-Encrypted document to be synced database, and the queue to communicate with subprocess workers. - - :param sqlcipher_file: The path for the SQLCipher file. - :type sqlcipher_file: str """ - sync_db_path = None - if sqlcipher_file != ":memory:": - sync_db_path = "%s-sync" % sqlcipher_file - else: - sync_db_path = ":memory:" - self._sync_db = MPSafeSQLiteDB(sync_db_path) - # protect the sync db with a password - if sync_db_password is not None: - self._set_crypto_pragmas( - self._sync_db, sync_db_password, True, - 'aes-256-cbc', 4000, 1024) - self._sync_db_write_lock = threading.Lock() - self._create_sync_db_tables() - self.sync_queue = multiprocessing.Queue() + if self._sync_db is None: + soledad_assert(self._sync_db_key is not None) + sync_db_path = None + if self._sqlcipher_file != ":memory:": + sync_db_path = "%s-sync" % self._sqlcipher_file + else: + sync_db_path = ":memory:" + self._sync_db = MPSafeSQLiteDB(sync_db_path) + # protect the sync db with a password + if self._sync_db_key is not None: + self._set_crypto_pragmas( + self._sync_db, self._sync_db_key, False, + 'aes-256-cbc', 4000, 1024) + self._sync_db_write_lock = threading.Lock() + self._create_sync_db_tables() + self.sync_queue = multiprocessing.Queue() def _create_sync_db_tables(self): """ @@ -1106,24 +1111,30 @@ class SQLCipherDatabase(sqlite_backend.SQLitePartialExpandDatabase): """ Close db_handle and close syncer. """ - logger.debug("Sqlcipher backend: closing") + if logger is not None: # logger might be none if called from __del__ + logger.debug("Sqlcipher backend: closing") # stop the sync watcher for deferred encryption if self._sync_watcher is not None: self._sync_watcher.stop() self._sync_watcher.shutdown() + self._sync_watcher = None # close all open syncers for url in self._syncers: _, syncer = self._syncers[url] syncer.close() + self._syncers = [] # stop the encryption pool if self._sync_enc_pool is not None: self._sync_enc_pool.close() + self._sync_enc_pool = None # close the actual database if self._db_handle is not None: self._db_handle.close() + self._db_handle = None # close the sync database if self._sync_db is not None: self._sync_db.close() + self._sync_db = None # close the sync queue if self.sync_queue is not None: self.sync_queue.close() diff --git a/client/src/leap/soledad/client/target.py b/client/src/leap/soledad/client/target.py index 1cb02856..ae2010a6 100644 --- a/client/src/leap/soledad/client/target.py +++ b/client/src/leap/soledad/client/target.py @@ -807,12 +807,9 @@ class SoledadSyncTarget(HTTPSyncTarget, TokenBasedAuth): self._sync_db = sync_db self._sync_db_write_lock = sync_db_write_lock - def _setup_sync_decr_pool(self, last_known_generation): + def _setup_sync_decr_pool(self): """ Set up the SyncDecrypterPool for deferred decryption. - - :param last_known_generation: Target's last known generation. - :type last_known_generation: int """ if self._sync_decr_pool is None: # initialize syncing queue decryption pool @@ -1133,7 +1130,7 @@ class SoledadSyncTarget(HTTPSyncTarget, TokenBasedAuth): if defer_decryption and self._sync_db is not None: self._sync_exchange_lock.acquire() - self._setup_sync_decr_pool(last_known_generation) + self._setup_sync_decr_pool() self._setup_sync_watcher() self._defer_decryption = True else: -- cgit v1.2.3