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

multiple collaborators per certificate #944

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

hasan7n
Copy link

@hasan7n hasan7n commented Mar 13, 2024

This PR adds the possibility of having multiple collaborator nodes use the same certificate for communication with the aggregator. The goal is to separate the dataset "identity" from its owner identity. We can think of "collaborator name" now as a "dataset name/ID".

cols.yaml's collaborators key is now expected to have one of the following two structures:

  • A dictionary representing a mapping between an authorized collaborator name and the corresponding certified common name. Multiple collaborator names can have the same certified common name.
  • list of strings: as before. In this case, each string will be interpreted as both the collaborator name and its certified common name. (for backward compatibility)

In the code, the Plan object will now have both the authorized_cols property and a new property cn_mapping. I kept the authoized_cols property to minimize changes to the codebase since it is used in many places as a list of strings.

@hasan7n
Copy link
Author

hasan7n commented Mar 13, 2024

possible TODOs:

  1. unit tests
  2. Update fx collaborator certify and generate-cert-request to accept multiple cols per cert.
  3. Director-based flow: I haven't tested this yet. It looks like the list of collaborators is received via a request. Should we add a new optional request param for associated expected CNs?
  4. Experimental code is not updated
  5. documentation

@@ -225,7 +227,7 @@ def valid_collaborator_cn_and_id(self, cert_common_name,
# FIXME: '' instead of None is just for protobuf compatibility.
# Cleaner solution?
if self.single_col_cert_common_name == '':
return (cert_common_name == collaborator_common_name
return (cert_common_name == self.cn_mapping[collaborator_common_name]
Copy link
Author

Choose a reason for hiding this comment

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

TODO: handle error

Copy link
Author

Choose a reason for hiding this comment

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

possible key error

@manuelhsantana
Copy link
Collaborator

Hi @hasan7n,

Great work on enabling multiple collaborators to use a shared certificate. To ensure everything works as expected, could you add unit tests for both the new dictionary mapping in cols.yaml and the backward-compatible list format?

Additionally, could you outline how we might manually test this feature to verify its functionality?

Thanks for your efforts!

Best, Manuel Santana

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.

2 participants