Implement draft-ietf-ppm-dap-taskprov-01. #3509
Conversation
branlwyd
force-pushed
the
bran/taskprov
branch
from
082348a to
2d349e8
Compare
|
Part of #3436. |
| taskprov::VdafConfig::Prio3Sum { | ||
| max_measurement: _max_measurement, | ||
| } => Ok(Self::Prio3Sum { | ||
| bits: 32, // TODO(#3436): plumb through max_measurement once it's available |
There was a problem hiding this comment.
This will certainly need to change, but at the time I'm writing this, the change to libprio-rs to specify max_measurement instead of bits for Prio3Sum is still pending. I'll update this once the libprio-rs change lands.
| @@ -795,7 +777,8 @@ impl<C: Clock> Aggregator<C> { | |||
| return Err(Error::UnauthorizedRequest(*task_id)); | |||
| } | |||
|
|
|||
| if self.clock.now() > *task_config.task_end() { | |||
| let task_end = task_config.task_start().add(task_config.task_duration())?; | |||
| if self.clock.now() > task_end { | |||
There was a problem hiding this comment.
I think we should be checking report timestamps against task_start + task_duration. The spec requirement is "Aggregators MUST reject reports that have timestamps later than the end time, and MAY choose to opt out of the task if task_duration is too long." However, that means we can't take care of this check in this function, and instead would have to handle it separately in the aggregation endpoint and upload endpoint (if we ever implement it).
There was a problem hiding this comment.
This code is implementing the requirement that "A protocol participant MUST opt out if ... [t]he task has ended."[1] I think using the server's clock, rather than the timestamp from a given report, is appropriate for this specific check.
You're correct that we should also be checking each report's timestamp against the end (and start) of the task, and we MUST reject reports that don't fall into the acceptable time interval. Those checks are implemented in handle_upload_generic.
Interestingly, I don't think the equivalent Helper-side timestamp check has ever been implemented, though we do implement checking that the report isn't from too far in the future. I filed #3524 for this, arguing that it is not mandatory. Still, it's a small change, so I'll address it if I have time.
[1] https://www.ietf.org/archive/id/draft-ietf-ppm-dap-taskprov-01.html#name-opting-into-a-task
branlwyd
force-pushed
the
bran/taskprov
branch
from
2d349e8 to
1efa7b4
Compare
No description provided.