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

Update Porter endpoint for retrieval #2768

Merged
merged 30 commits into from Sep 19, 2021

Conversation

derekpierre
Copy link
Member

@derekpierre derekpierre commented Aug 11, 2021

Type of PR:

  • Bugfix
  • Feature
  • Documentation
  • Other

Required reviews:

  • 1
  • 2
  • 3

What this does:
Changes the Porter endpoint intended for part Bob retrieval process to use the updated retrieval protocol in #2730.

Issues fixed/closed:
Based over #2730 .
Related to #2714 , #2751 .

Questions for Reviewers:

  1. Any issues with defining __eq__:
  2. Serialization format for retrieval kits:

@derekpierre derekpierre added API Bob 👨‍💼 Effects the "Bob" development area labels Aug 11, 2021
@derekpierre derekpierre self-assigned this Aug 11, 2021
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 11, 2021
@derekpierre
Copy link
Member Author

@fjarri, fyi, I used a dirty hack regarding decrypted treasure maps for now to be able to properly test Porter retrieve - see 511091f.

derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 23, 2021
@derekpierre derekpierre mentioned this pull request Aug 23, 2021
@derekpierre derekpierre changed the title [WIP] Update Porter endpoint for retrieval Update Porter endpoint for retrieval Aug 23, 2021
@derekpierre derekpierre marked this pull request as ready for review August 23, 2021 19:50
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 24, 2021
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 24, 2021
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 30, 2021
derekpierre added a commit to derekpierre/nucypher that referenced this pull request Aug 30, 2021
nucypher/policy/maps.py Show resolved Hide resolved
@@ -311,3 +312,6 @@ def from_bytes(cls, data: bytes):
result = cls(hrac, public_signature, message_kit, blockchain_signature=blockchain_signature)
result._public_verify()
return result

def __eq__(self, other):
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to double-check, any concerns with this i.e. adding an equality function i.e. for EncryptedTreasureMap

Copy link
Member

Choose a reason for hiding this comment

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

No concerns from me.

@derekpierre derekpierre added this to Review in progress in Porter Aug 31, 2021
@derekpierre derekpierre added this to the Porter v1 (MVP) milestone Sep 1, 2021
…instead of bytes.

Minor update to Porter docs for retrieval endpoint.
…ld to CapsuleFrag.

Improve retreive_cfrags test to ensure that the cfrags are valid and can successfully be used by Bob to decrypt the data.
…s - it should be an unencrypted treasure map.
…a list of message kits since the respective Python API was modified to accept a list of message kits.

Cleaned up Bob's retrieve_and_decrypt CLI code to account for updated required options - there was code that was based on some options not being required
Adjusted associated tests.
Copy link
Contributor

@fjarri fjarri left a comment

Choose a reason for hiding this comment

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

Approved with some minor comments.

alice_verifying_key = PublicKey.from_bytes(alice_verifying_key)
message_kit = MessageKit.from_bytes(message_kit) # TODO #846: May raise UnknownOpenSSLError and InvalidTag.

if isinstance(encrypted_treasure_map, bytes):
Copy link
Member

Choose a reason for hiding this comment

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

I suppose the thinking here is that deserialization must be handled by the caller?

Copy link
Member Author

Choose a reason for hiding this comment

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

The EncryptedTreasureMap field used in the RetrieveAndDecrypt Schema, automatically handles the deserialization - https://github.com/derekpierre/nucypher/blob/porter-retrieval/nucypher/characters/control/specifications/fields/treasuremap.py#L27.

@@ -311,3 +312,6 @@ def from_bytes(cls, data: bytes):
result = cls(hrac, public_signature, message_kit, blockchain_signature=blockchain_signature)
result._public_verify()
return result

def __eq__(self, other):
Copy link
Member

Choose a reason for hiding this comment

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

No concerns from me.

tests/utils/policy.py Show resolved Hide resolved
@@ -39,12 +40,19 @@ def __init__(self, implementer=None, *args, **kwargs):
super().__init__(*args, **kwargs)

@classmethod
def connect_cli(cls, action):
def connect_cli(cls, action, exclude: Optional[Set[str]] = None):
Copy link
Member

Choose a reason for hiding this comment

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

Where is this kwarg being called? The idea here is to provide an alternative method for CLI option exclusion?

Copy link
Member Author

Choose a reason for hiding this comment

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

The general idea is that the schema can still be used for populating CLI options, but allow for some CLI options to be modified.

More specifically, it is used here - https://github.com/derekpierre/nucypher/blob/porter-retrieval/nucypher/cli/commands/bob.py#L347 with the nucypher bob retrieve-and-decrypt CLI command. In this case, the retrieve-and-decrypt command from the CLI can be used without requiring an alice_verifying_key to be specified via --alice-verifying-key option which is required by the schema. An Alice card can be specified from the CLI via --alice and the verifying key obtained from the Card API.

In this case --alice-verifying-key needs to be optional and not required. Either Alice's verifying key is provided directly via --alice-verifying-key or indirectly via --alice.

@KPrasch KPrasch merged commit 5582a9f into nucypher:main Sep 19, 2021
Porter automation moved this from Review in progress to Done Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bob 👨‍💼 Effects the "Bob" development area
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants