subscriber-01: Implement draft-wang-ppm-dap-taskprov-02 #2067
Conversation
inahga
force-pushed
the
inahga/sub01-taskprov-02
branch
from
0f6c66f to
220febd
Compare
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let aggregation_param = dummy_vdaf::AggregationParam(0); | ||
| let vdaf = Prio3Aes128Count::new_aes128_count(2).unwrap(); |
There was a problem hiding this comment.
I'm unhappy about using Prio3 as the test VDAF as that requires us to discard tests that exercise varying aggregation parameters (unless I'm mistaken?).
Unsure if it's really worth addressing in this PR. We can either adapt the taskprov messages to accept dummy_vdaf when compiled under tests, or I can try harder to use Poplar1 (the API is quite different in libprio-rs 0.10 compared to the latest).
There was a problem hiding this comment.
So far as we know right now, this deployment will only ever use Prio3 family VDAFs, in which case only exercising single-round, aggregation-parameter-less VDAFs is acceptable. I think it's not worth putting in more effort here until we understand this deployment's requirements better.
There was a problem hiding this comment.
I'm skeptical of the value of investing more work into this branch without more clarity on the requirements here. Thus far we've been making the guesses and divining what to do here from reading tea leaves, and it's already led to wasted effort (going to the taskprov that puts the config in a header and now back to per-report-extensions). Or in the context of this PR, we could simplify it somewhat if we knew which of the two query modes the subscriber is going to use exclusively.
That said, this particular piece of work is already done and this accomplishes what it sets out to do, so I think we can take this change.
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let aggregation_param = dummy_vdaf::AggregationParam(0); | ||
| let vdaf = Prio3Aes128Count::new_aes128_count(2).unwrap(); |
There was a problem hiding this comment.
So far as we know right now, this deployment will only ever use Prio3 family VDAFs, in which case only exercising single-round, aggregation-parameter-less VDAFs is acceptable. I think it's not worth putting in more effort here until we understand this deployment's requirements better.
| /// Validate and authorize a taskprov request. Returns values necessary for determining whether | ||
| /// we can opt into the task. This function might return an opt-out error for conditions that | ||
| /// are relevant for all DAP workflows (e.g. task expiration). | ||
| #[tracing::instrument(skip(self, aggregator_auth_token), err)] |
There was a problem hiding this comment.
Do we want to keep this tracing span?
There was a problem hiding this comment.
Good catch, I think we should. I originally kept it out since logging on unauthorized requests might end up being just noise, but it in the early stages of integration testing it should aid in debugging.
Co-authored-by: Tim Geoghegan <[email protected]>
|
Agreed that we ought to freeze changes to this branch after this PR until we have more clarity on requirements (or better yet, direct time with their engineering). |
Adopts draft-wang-ppm-dap-taskprov-02 for the subscriber-01 branch.
The primary difference between this revision and the latest is that the taskprov configuration is expressed in the extension field of each report share. A helper must parse the extension in each AggregateInitRequest and use it to provision the task.
The implementation of this is unclean. Due to how AggregateInitRequest is structured, we need to know the query type ahead of time. We can't decode it unless we know the query type, but we won't know the query type until we decode it. To work around this, decode by trial.
To simplify implementation I made all helper requests use the main aggregator auth token established in #1976. Leader requests are unmodified, but are effectively inert/useless now. We can consider removing leader endpoints in a future PR.
The first commit contains the operative logic, the remaining commits address tests.
Tests underwent considerable modification. The aggregate init harness needed to be adapted to provide the taskprov extension and aggregator authentication token. I accepted the large amount of change on the basis that we will likely just want to revert this PR to adopt taskprov-04+ again.
I disabled all integration tests as this branch can no longer service non-taskprov leader->helper requests.