-
Notifications
You must be signed in to change notification settings - Fork 57
anti-replay requirements for level 1 #354
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
base: main
Are you sure you want to change the base?
Conversation
anti-replay requirements -- just the basic ones from w3c#43 not including partitioning and requerying. I have a separate branch with addition of associated data in the AEAD to partition reports but requires more changes and probably can wait till level 2 as @csharrison mentioned on the issue.
|
Hi @bmcase I would prefer we just combine this PR with the one which actually modifies the encryption logic in the spec (i.e. https://w3c.github.io/attribution/#encrypt-dap). Do you mind adding that? |
|
Hi @csharrison , sure thing. I added to this PR adding topLevelSite to the reportMetadata where we encryption logic is defined in the spec. |
api.bs
Outdated
|
|
||
| 1. Let |reportMetadata| be encoded DAP [`ReportMetadata`](https://datatracker.ietf.org/doc/html/draft-ietf-ppm-dap-15#section-4.5.2) | ||
| generated from |reportID|, |time|, and |extensions|. | ||
| generated from |reportID|, |topLevelSite|, |time|, and |extensions|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't match the other section, which asks for the origin of the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. updated the other section to site which also makes it consistent with the privacy unit section https://w3c.github.io/attribution/#dp-unit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But "site of the caller" vs. topLevelSite is a different concept. Which one should it be? I believe this one should be site and not topLevelSite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, it should be the site of the caller. Updated to define a |conversionCaller| like is done in the Common Impression Matching Logic section https://w3c.github.io/attribution/#logic-matching
This comment was marked as spam.
This comment was marked as spam.
also makes it consistent with the privacy unit section https://w3c.github.io/attribution/#dp-unit
martinthomson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but I don't think that this is right.
| * The [=site=] that invoked the {{Attribution/measureConversion()}} API. | ||
| This is the [=intermediary site=] if the API was called from a cross-site frame, | ||
| or the [=conversion site=] otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relates to the changes you added to the algorithms, but I don't think that this is correct.
We charge budget to the top-level site. Therefore the aggregation service needs to attribute individual reports to the same entity. Otherwise, we create the possibility that reports from multiple sites (and therefore multiple budgets) are aggregated together and noise is not correctly applied.
Concretely, if I have reports from site A and site B, then there are two budgets. I would expect two lots of noise as well. This approach would allow an intermediary to combine reports from sites into a single aggregation that includes just one lot of noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should discuss this next week. Here is my opinion:
- The conversion caller should be in the associated data and we should fail decryption in the helpers if the callers don't match. This prevents report-theft from being a problem.
- I don't think the top level site needs to be in the associated data. I don't think a single lot of noise is problematic for the guarantees (esp. if safety limits are in place), but @bmcase can correct me if I'm wrong. In fact, I think we wanted to support cross-advertiser queries right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discussed with @csharrison; we think we should include the following
-
the caller of the API (either an intermediary (like measurement provider for the advertiser) or the advertiser themself) and for this use the full origin. This can be used to make sure only this origin can query the report to the Helpers (and prevent DOS on reports being queried by some other party).
-
the eTLD+1 of the conversion site. This corresponds to the grain of the DP budgeting in Level 1 and prevents aggregation of reports from different advertisers which spent different budgets. (This can be handled different in Level 2 when we do support cross-advertiser aggregation and there ensure we bind to whichever budget is being deducted from).
anti-replay requirements -- just the basic ones from #43 not including partitioning and requerying.
I have a separate branch with addition of associated data in the AEAD to partition reports but requires more changes and probably can wait till level 2 as @csharrison mentioned on the issue.
Preview | Diff