Repository: cloudflare/daphne
Pull request #281 (2023-04-12 22:17:18)

Add problem details for "batchInvalid" abort
Partially addresses #123.


Patch:
diff --git a/daphne/src/lib.rs b/daphne/src/lib.rs
index d5ae400..b57b096 100644
--- a/daphne/src/lib.rs
+++ b/daphne/src/lib.rs
@@ -125,18 +125,16 @@ impl From<VdafError> for DapError {
 #[derive(Debug, thiserror::Error)]
 pub enum DapAbort {
     /// Bad request. Sent in response to an HTTP request that couldn't be handled preoprly.
-    //
-    // TODO spec: Decide whether to specify this.
     #[error("badRequest")]
     BadRequest(String),
 
     /// Invalid batch. Sent in response to a CollectReq or AggregateShareReq.
     #[error("batchInvalid")]
-    BatchInvalid,
+    BatchInvalid { detail: String, task_id: TaskId },
 
     /// Batch mismatch. Sent in response to an AggregateShareReq.
     #[error("batchMismatch")]
-    BatchMismatch,
+    BatchMismatch { detail: String, task_id: TaskId },
 
     /// Batch overlap. Sent in response to an CollectReq for which the Leader detects the same
     /// Collector requesting an aggregate share which it has collected in the past.
@@ -144,7 +142,7 @@ pub enum DapAbort {
     BatchOverlap,
 
     /// Internal error.
-    #[error("{0}")]
+    #[error("internalError")]
     Internal(#[source] Box<dyn std::error::Error + 'static + Send + Sync>),
 
     /// Invalid batch size (either too small or too large). Sent in response to a CollectReq or
@@ -152,9 +150,10 @@ pub enum DapAbort {
     #[error("invalidBatchSize")]
     InvalidBatchSize,
 
-    /// Invalid DAP task.  Sent when a server opts out of a taskprov task configuration.
+    /// draft-wang-ppm-dap-taskprov-02: Invalid DAP task. Sent when a server opts out of a
+    /// taskprov task configuration.
     #[error("invalidTask")]
-    InvalidTask,
+    InvalidTask { detail: String, task_id: TaskId },
 
     /// Request with missing task ID.
     #[error("missingTaskID")]
@@ -213,13 +212,15 @@ pub enum DapAbort {
 impl DapAbort {
     /// Construct a problem details JSON object for this abort. `url` is the URL to which the
     /// request was targeted and `task_id` is the associated TaskID.
-    pub fn to_problem_details(&self) -> ProblemDetails {
-        let (typ, detail) = match self {
-            Self::BatchInvalid
-            | Self::BatchMismatch
-            | Self::BatchOverlap
+    pub fn into_problem_details(self) -> ProblemDetails {
+        let typ = format!("urn:ietf:params:ppm:dap:error:{}", self);
+        let title = self.title().map(ToString::to_string);
+        let (task_id, detail) = match self {
+            Self::BatchInvalid { detail, task_id }
+            | Self::InvalidTask { detail, task_id }
+            | Self::BatchMismatch { detail, task_id } => (Some(task_id), Some(detail)),
+            Self::BatchOverlap
             | Self::InvalidBatchSize
-            | Self::InvalidTask
             | Self::QueryMismatch
             | Self::RoundMismatch
             | Self::MissingTaskId
@@ -230,14 +231,15 @@ impl DapAbort {
             | Self::UnrecognizedAggregationJob
             | Self::UnrecognizedHpkeConfig
             | Self::UnrecognizedMessage
-            | Self::UnrecognizedTask => (self.to_string(), None),
-            Self::BadRequest(s) => ("badRequest".to_string(), Some(s.clone())),
-            Self::Internal(e) => ("internalError".to_string(), Some(e.to_string())),
+            | Self::UnrecognizedTask => (None, None),
+            Self::BadRequest(s) => (None, Some(s)),
+            Self::Internal(e) => (None, Some(e.to_string())),
         };
 
         ProblemDetails {
-            typ: format!("urn:ietf:params:ppm:dap:error:{typ}"),
-            taskid: None,   // TODO interop: Implement as specified.
+            typ,
+            title,
+            task_id: task_id.map(|id| id.to_base64url()),
             instance: None, // TODO interop: Implement as specified.
             detail,
         }
@@ -269,6 +271,31 @@ impl DapAbort {
     pub(crate) fn version_unknown() -> Self {
         DapAbort::BadRequest("DAP version of request is not recognized".into())
     }
+
+    fn title(&self) -> Option<&'static str> {
+        match self {
+            Self::BatchInvalid { .. } => Some("Batch boundary check failed"),
+            Self::BatchMismatch { .. } => {
+                Some("Aggregators disagree on the set of reports in the batch")
+            }
+            Self::BatchOverlap => None,
+            Self::InvalidBatchSize => None,
+            Self::InvalidTask { .. } => Some("Opted out of Taskprov task"),
+            Self::QueryMismatch => None,
+            Self::RoundMismatch => None,
+            Self::MissingTaskId => None,
+            Self::ReplayedReport => None,
+            Self::ReportTooLate => None,
+            Self::StaleReport => None,
+            Self::UnauthorizedRequest => None,
+            Self::UnrecognizedAggregationJob => None,
+            Self::UnrecognizedHpkeConfig => None,
+            Self::UnrecognizedMessage => None,
+            Self::UnrecognizedTask => None,
+            Self::BadRequest(..) => None,
+            Self::Internal(..) => None,
+        }
+    }
 }
 
 impl From<DapError> for DapAbort {
@@ -302,13 +329,13 @@ impl From<TransitionFailure> for DapAbort {
 pub struct ProblemDetails {
     #[serde(rename = "type")]
     pub typ: String,
-
     #[serde(skip_serializing_if = "Option::is_none")]
-    pub(crate) taskid: Option<String>,
-
+    pub title: Option<String>,
+    #[serde(rename = "taskid")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub(crate) task_id: Option<String>,
     #[serde(skip_serializing_if = "Option::is_none")]
     pub(crate) instance: Option<String>,
-
     #[serde(skip_serializing_if = "Option::is_none")]
     pub detail: Option<String>,
 }
diff --git a/daphne/src/roles.rs b/daphne/src/roles.rs
index e5258f6..cabdeb7 100644
--- a/daphne/src/roles.rs
+++ b/daphne/src/roles.rs
@@ -55,9 +55,13 @@ where
 
     /// Decide whether to opt-in or out-out of a task provisioned via taskprov.
     ///
-    /// Returning `Ok(true)` opts in, returning `Ok(false)` opts out, and any error is
-    /// also an opt out, but that error code is used instead of InvalidTask.
-    fn taskprov_opt_in_decision(&self, task_config: &DapTaskConfig) -> Result<bool, DapError>;
+    /// If the return value is `None`, then the decision is to opt-in. If the return value is
+    /// `Some(reason)`, then the decision is to opt-out; `reason` conveys details about how the
+    /// decision was rached (e.g.., the minimum batch size is too smal).
+    fn taskprov_opt_out_reason(
+        &self,
+        task_config: &DapTaskConfig,
+    ) -> Result<Option<String>, DapError>;
 
     /// Look up the DAP task configuration for the given task ID.
     ///
@@ -1062,11 +1066,18 @@ where
             .get_agg_share(task_id, &agg_share_req.batch_sel)
             .await?;
 
-        // Check that we have aggreagted the same set of reports as the leader.
+        // Check that we have aggreagted the same set of reports as the Leader.
         if agg_share_req.report_count != agg_share.report_count
             || !constant_time_eq(&agg_share_req.checksum, &agg_share.checksum)
         {
-            return Err(DapAbort::BatchMismatch);
+            return Err(DapAbort::BatchMismatch{
+                detail: format!("Either the report count or checksum does not match: the Leader computed {} and {}; the Helper computed {} and {}.",
+                    agg_share_req.report_count,
+                    hex::encode(agg_share_req.checksum),
+                    agg_share.report_count,
+                    hex::encode(agg_share.checksum)),
+                task_id: task_id.clone(),
+            });
         }
 
         // Check the batch size.
@@ -1151,7 +1162,10 @@ where
                 || batch_interval.duration % task_config.time_precision != 0
                 || batch_interval.duration < task_config.time_precision
             {
-                return Err(DapAbort::BatchInvalid);
+                return Err(DapAbort::BatchInvalid {
+                    detail: format!("The queried batch interval ({batch_interval:?}) is too small or its boundaries are misaligned. The time precision for this task is {}s.", task_config.time_precision),
+                    task_id: task_id.clone(),
+                });
             }
 
             if batch_interval.duration > global_config.max_batch_duration {
@@ -1178,7 +1192,13 @@ where
             //
             // Consider removing this callback once we resolve DAP issue #342.
             if !agg.batch_exists(task_id, batch_id).await? {
-                return Err(DapAbort::BatchInvalid);
+                return Err(DapAbort::BatchInvalid {
+                    detail: format!(
+                        "The queried batch ({}) does not exist.",
+                        batch_id.to_base64url()
+                    ),
+                    task_id: task_id.clone(),
+                });
             }
         }
         _ => return Err(DapAbort::QueryMismatch),
diff --git a/daphne_worker/src/dap.rs b/daphne_worker/src/dap.rs
index bfaf256..4e19fe5 100644
--- a/daphne_worker/src/dap.rs
+++ b/daphne_worker/src/dap.rs
@@ -357,12 +357,12 @@ where
         &self.config().global
     }
 
-    fn taskprov_opt_in_decision(
+    fn taskprov_opt_out_reason(
         &self,
         _task_config: &DapTaskConfig,
-    ) -> std::result::Result<bool, DapError> {
+    ) -> std::result::Result<Option<String>, DapError> {
         // For now we always opt-in.
-        Ok(true)
+        Ok(None)
     }
 
     /// Get an existing task (whether an ordinary task or a previously created
@@ -415,8 +415,11 @@ where
             )?;
 
             // This is the opt-in / opt-out decision point.
-            if !self.taskprov_opt_in_decision(&task_config)? {
-                return Err(DapError::Abort(daphne::DapAbort::InvalidTask));
+            if let Some(reason) = self.taskprov_opt_out_reason(&task_config)? {
+                return Err(DapError::Abort(daphne::DapAbort::InvalidTask {
+                    detail: reason,
+                    task_id: task_id.into_owned(),
+                }));
             }
 
             // Write the leader bearer token to the KV.  We do this so authorize_with_bearer_token()
diff --git a/daphne_worker/src/lib.rs b/daphne_worker/src/lib.rs
index c411d7b..5f42119 100644
--- a/daphne_worker/src/lib.rs
+++ b/daphne_worker/src/lib.rs
@@ -650,7 +650,7 @@ fn abort(e: DapAbort) -> Result<Response> {
     } else {
         400
     };
-    let problem_details = e.to_problem_details();
+    let problem_details = e.into_problem_details();
     error!(
         "request aborted: {}",
         serde_json::to_string(&problem_details)?


Test patch:
diff --git a/daphne/src/roles_test.rs b/daphne/src/roles_test.rs
index 779c479..a7976f9 100644
--- a/daphne/src/roles_test.rs
+++ b/daphne/src/roles_test.rs
@@ -809,7 +809,7 @@ async fn http_post_aggregate_share_invalid_batch_sel(version: DapVersion) {
         .await;
     assert_matches!(
         t.helper.http_post_aggregate_share(&req).await.unwrap_err(),
-        DapAbort::BatchInvalid
+        DapAbort::BatchInvalid { .. }
     );
 }
 
@@ -1501,7 +1501,7 @@ async fn http_post_collect_invalid_query(version: DapVersion) {
         .await;
     assert_matches!(
         t.leader.http_post_collect(&req).await.unwrap_err(),
-        DapAbort::BatchInvalid
+        DapAbort::BatchInvalid { .. }
     );
 }
 
diff --git a/daphne/src/testing.rs b/daphne/src/testing.rs
index 5d79cc5..a94c455 100644
--- a/daphne/src/testing.rs
+++ b/daphne/src/testing.rs
@@ -341,8 +341,12 @@ where
         &self.global_config
     }
 
-    fn taskprov_opt_in_decision(&self, _task_config: &DapTaskConfig) -> Result<bool, DapError> {
-        Ok(true)
+    fn taskprov_opt_out_reason(
+        &self,
+        _task_config: &DapTaskConfig,
+    ) -> Result<Option<String>, DapError> {
+        // Always opt-in.
+        Ok(None)
     }
 
     async fn get_task_config_considering_taskprov(
@@ -376,8 +380,11 @@ where
                 let mut tasks = self.tasks.lock().expect("tasks: lock failed");
                 if tasks.get(task_id.as_ref()).is_none() {
                     // Decide whether to opt-in to the task.
-                    if !self.taskprov_opt_in_decision(&task_config)? {
-                        return Err(DapError::Abort(DapAbort::InvalidTask));
+                    if let Some(reason) = self.taskprov_opt_out_reason(&task_config)? {
+                        return Err(DapError::Abort(DapAbort::InvalidTask {
+                            detail: reason,
+                            task_id: task_id.into_owned(),
+                        }));
                     }
 
                     tasks