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

allow a data provider to specify a different certificate used in signing a direct result #171

Merged
merged 7 commits into from
Nov 6, 2023

Conversation

stevenwarejones
Copy link
Contributor

@stevenwarejones stevenwarejones commented Sep 12, 2023

A Data Provider may not want to use the same certificate that was noted in the Requisition for various reasons. This update allows a Data Provider to specify a different certificate to verify a Measurement Result.

@wfa-reviewable
Copy link

This change is Reviewable

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Thanks for this! I didn't realize we weren't already taking in the certificate here and were instead using the one associated with the encryption public key.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/requisitions_service.proto line 141 at r2 (raw file):

  // Resource name of a `Certificate` belonging to the `DataProvider` that
  // produced the result. If unspecified, the certificate from the
  // data_provider_certificate from the `Requisition` will be used.

nit: I assume you didn't mean to repeat the phrase

Suggestion:

  // produced the result. If unspecified, the `data_provider_certificate`
  // from the `Requisition` will be used.

@SanjayVas
Copy link
Member

See #188

Copy link
Contributor Author

@stevenwarejones stevenwarejones 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: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/proto/wfa/measurement/api/v2alpha/requisitions_service.proto line 143 at r3 (raw file):

  // `data_provider_certificate` from the `Requisition` will be used.
  string certificate = 4
      [(google.api.resource_reference).type = "halo.wfanet.org/Certificate"];

based on PR187, I should change this to required I'm thinking @SanjayVas

Copy link
Contributor

@Marco-Premier Marco-Premier 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 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)

Copy link
Member

@SanjayVas SanjayVas 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 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)


src/main/proto/wfa/measurement/api/v2alpha/requisitions_service.proto line 143 at r3 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

based on PR187, I should change this to required I'm thinking @SanjayVas

The current (informal) API change policy has been to avoid breaking EDP requisition fulfillment in a single release. I don't think we can safely require this until after 0.5.0. Noting that it may be required in the future would be good for now.

@stevenwarejones stevenwarejones force-pushed the stevenwarejones_separate_fulfill_certificate branch from 305e08a to 170eb74 Compare November 6, 2023 16:08
Copy link
Contributor

@Marco-Premier Marco-Premier 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 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

@stevenwarejones stevenwarejones merged commit 693def0 into main Nov 6, 2023
3 checks passed
@stevenwarejones stevenwarejones deleted the stevenwarejones_separate_fulfill_certificate branch November 6, 2023 16:19
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.

4 participants