Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[POC] Secure channel abstraction #11

Open
wants to merge 31 commits into
base: trusted-aggr-stash
Choose a base branch
from

Conversation

yanndupis
Copy link
Member

Hey @mortendahl @jvmncs ,

Here is an initial draft with the new abstraction for the secure channel. Let me know if it's moving to the right direction.

Thank you!

@yanndupis yanndupis requested review from mortendahl and jvmncs May 8, 2020 02:44
Copy link
Member

@jvmncs jvmncs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's discuss in person, I have some further comments on finding the right abstraction for channels. I'd like to also go over how this might eventually be exposed to the user (taking some pointers from Morten's earlier recommendations)

also note I have not verified the encrypt/decrypt work just yet -- I made sure that the key generation was happening in such a way not to leak secret keys, but otherwise am assuming there haven't been too many significant changes to that. should be worth another look during next round of feedback.

@@ -492,128 +492,38 @@ def validate_executor_placements(cls, executor_placements):
'Unsupported cardinality for placement "{}": {}.'.format(
pl, pl_cardinality))

async def _trusted_aggregator_generate_keys(self):
@classmethod
async def _move(cls, arg, target_executor, parent_executor, channel):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the signature could be _move(self, arg, source_placement, target_placement) or similar. channel should be referencable from self (the Strategy)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, that would make more sense.

@yanndupis
Copy link
Member Author

Hey @mortendahl @jvmncs ,

Quick status on this. I have made good progress but it's not completely done. I still want to add more tests, review the code again, potentially add ChannelGrid as discussed with Jason. Feel free to take a look to validate it's moving to the right direction or if you want provide suggestions. Once I finalize this and it's reviewed, we will move the code to the new repo.

Thank you!

Copy link
Member

@mortendahl mortendahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @yanndupis, I've added a few comments; perhaps it makes sense to do a quick design sync

pass

@abc.abstractmethod
async def receive(self, value, sender_index=None, receiver_index=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is value here? how does it relate to the return value of receive?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value is list of EagerValues and it was returning a FederatingExecutorValue. I changed it to return a list of EagerValues.

I have the same pb with _encrypt_values_on_sender. However not sure yet, how to keep this Federatedtype check (if needed) if I return a list of EagerValues after encryption.

@yanndupis
Copy link
Member Author

Hey @mortendahl , thanks for you! sounds good let's do a quick design sync.

@yanndupis
Copy link
Member Author

Hey guys,

First of all, thank you for the review! I have addressed most of the comment (still have 2-4 for tomorrow).

However, I still have a Channel object per pair of placements. I need to investigate more tomorrow. But it seems we could have a channel per pair of executors. If that's possible, that could simplify the code. For each channel, each executor would have their key pair. Potentially I wouldn't need to use _place_key just need _place. And also wouldn't need this, sender and receiver in send and receive.

I just don't know yet how to structure ChannelGrid and if i am missing potential complications.

Thank you!

@yanndupis
Copy link
Member Author

Hey guys,

I clean up a bit more, added specific tests to test Channel functionalities etc.

I have started to move the code to the tff-aggregations repo: tf-encrypted/federated-aggregations#1.

I will continue to work on this on Monday.

Regarding having Channel per pair of placements vs per pair of executors, after chatting with Jason, we decided to keep per pair of placements in this branch. Still open to discussion if we want to take the other approach but we can do this in the tff-aggregations branch.

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants