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 exchange shared input/output to support urls #34

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jonmolle
Copy link
Contributor

@jonmolle jonmolle commented Jul 7, 2021

The size of the blobs exchanged in the api steps is likely to be too
large to reasonably support directly passing via the API. On top of
that, we want to avoid having the Kingdom own the storage for some of
these steps. To avoid issues, this swaps the 'shared' values to string
from blob.

  • Only updates the shared input/output map values from blob to string
  • Implementation is done in another repo

This change is Reviewable

The size of the blobs exchanged in the api steps is likely to be too
large to reasonably support directly passing via the API. On top of
that, we want to avoid having the Kingdom own the storage for some of
these steps. To avoid issues, this swaps the 'shared' values to string
from blob.

* Only updates the shared input/output map values from blob to string
* Implementation is done in another step.
@jonmolle jonmolle requested a review from efoxepstein July 7, 2021 17:56
Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @jonmolle)


src/main/proto/wfa/measurement/api/v2alpha/exchange_step.proto, line 62 at r1 (raw file):

  // `shared_inputs` is the subset of that map consisting of labels from
  // `step.shared_input_labels`.
  map<string, string> shared_inputs = 5;

Do we want the URLs themselves to be signed then encrypted so the Kingdom can't access them?


src/main/proto/wfa/measurement/api/v2alpha/exchange_step.proto, line 63 at r1 (raw file):

  // `step.shared_input_labels`.
  map<string, string> shared_inputs = 5;

Should ExchangeStep have a field for outputs?

Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @jonmolle)


src/main/proto/wfa/measurement/api/v2alpha/exchange_step.proto, line 63 at r1 (raw file):

Previously, efoxepstein (Eli Fox-Epstein) wrote…

Should ExchangeStep have a field for outputs?

nevermind!

@jonmolle
Copy link
Contributor Author

Do we want the URLs themselves to be signed then encrypted so the Kingdom can't access them?

If the contents are always encrypted, is there any level of safety this would provide that we're concerned about? There isn't a particular reason they can't be, since the clients are going to handle the actual file transfers here. It would prevent some malicious actions by the kingdom, if that's a consideration.

Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Correct, we want to prevent a malicious Kingdom from even knowing where the files are stored.

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @jonmolle)

The size of the blobs exchanged in the api steps is likely to be too
large to reasonably support directly passing via the API. On top of
that, we want to avoid having the Kingdom own the storage for some of
these steps.

* Only updates the shared input/output map value comments with this
clarification
* Implementation is done in another step.
@jonmolle
Copy link
Contributor Author

Reverted back to bytes to indicate that any value passed through is expected to be encrypted. Only the comments are updated as (so far) this change is invisible to the API

Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @jonmolle)


src/main/proto/wfa/measurement/api/v2alpha/exchange_step.proto, line 54 at r2 (raw file):

  // The values of this map are signed urls pointing to cloud-hosted SignedData.
  // Each SignedData's payload is a consists of concatenated encrypted values of
  // uniform length. The urls themselves are signed and encrypted to prevent

Can you elaborate on the format here? E.g. serialized SignedData protos where the SignedData.data fields, when interpreted as a UTF-8 string, are the URLs, for example.


src/main/proto/wfa/measurement/api/v2alpha/exchange_workflow.proto, line 27 at r2 (raw file):

//
// See docs/panelmatch/example-exchange-workflow.textproto for an example
// ExchangeWorkflow.S

?

jojijac0b pushed a commit that referenced this pull request Apr 27, 2023
* Update LLV2 protos to fit the per computation ElGamalKey design.

1. Two extra stages are added to the protocl, i.e., INITIALIZATION_PHASE and
WAIT_REQUISITIONS_AND_KEY_SET.

2. CONFIRM_REQUISITIONS_PHASE is changed to CONFIRMATION_PHASE. In this
stage, the mill will confirms that
-- all local requisitions are present (it won't read and combine the
actual requisitions and write the result as an output anymore. The actual
requsitions are read later in the Setup phase. This saves storing
another copy of all requistions.)
-- EDP signatures and other duchies signatures are all valid.

3. ToConfirmRequisitionsStageDetails proto will be deprecated. The
requisitionList would be included in the serialized KingdomComputation
in the ComputationDetails.

* Add kingdomComputationDetails to the duchy internal ComputationDetails.

* Add kingdomComputationDetails to the duchy internal ComputationDetails.
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.

None yet

2 participants