[WIP]: Secure Aggregation cleanup #1420
Conversation
Signed-off-by: Pant, Akshay <[email protected]>
…or to helper class Signed-off-by: Pant, Akshay <[email protected]>
Signed-off-by: Pant, Akshay <[email protected]>
Signed-off-by: Pant, Akshay <[email protected]>
There was a problem hiding this comment.
Thanks @theakshaypant, I believe these changes are a move in the right direction! A couple of comments from my initial read-through:
| except Exception as error: | ||
| raise Exception( | ||
| "'pycryptodome' not installed." | ||
| f"This package is necessary when secure aggregation is enabled: {error}" | ||
| ) |
There was a problem hiding this comment.
How do we know this general Exception is caused by a missing pycryptodome dependency?
| # Fetch private mask from tensor db if not already fetched. | ||
| if not private_mask: | ||
| private_mask = tensor_db.get_tensor_from_cache( | ||
| TensorKey("private_mask", collaborator_name, -1, False, ("secagg",)) | ||
| )[0] | ||
| # Fetch shared mask from tensor db if not alreday fetched. | ||
| if not shared_mask: | ||
| shared_mask = tensor_db.get_tensor_from_cache( | ||
| TensorKey("shared_mask", collaborator_name, -1, False, ("secagg",)) | ||
| )[0] |
There was a problem hiding this comment.
IMHO it's not great to have a dependency on the TensorDB in a module dedicated to cryptography. Could you consider moving the code for retrieving the private and shared masks back to the collaborator module?
| @@ -12,6 +12,7 @@ TaskRunner API | |||
| ------------------------------------- | |||
|
|
|||
| OpenFL treats SecAgg as a core security feature and can be enabled for any experiment by simply modifying the plan. | |||
| **NOTE**: `pycryptodome <https://pypi.org/project/pycryptodome/>`_ is a required dependency that must be installed on the participant nodes before starting the experiment. | |||
There was a problem hiding this comment.
Please add limitations as well i.e Straggler policies will not work with secure aggregation. Also resiliency or restart of aggregator or collaborator during experiment may cause issues.
|
@theakshaypant I haven't completed a full review yet (as this is still WIP), but it's clearly moving in the right direction. I have a question about openfl/openfl/utilities/secagg/crypto.py Lines 14 to 15 in a208651 for ciphertext and mask generation. Instead could you use the cryptography package primitives that we already import? https://cryptography.io/en/latest/hazmat/primitives/
That would eliminate the need for the package detection code and make user's lives a bit easier |
Summary
Perform cleanup of secure aggregation code in openfl based on the checklist below.
Type of Change (Mandatory)
Specify the type of change being made.
Description (Mandatory)
pycryptodomefrom openfl dependencies.openfl.utilities.secagg.setupTesting
TODO