Repository: cloudflare/daphne
Pull request #655 (2024-08-22 16:49:57)

Populate ProblemDetails::instance
- Add task id to DapAbort::UnrecognizedTask
- Add report id to DapAbort::ReportToLate
- Make task id mandatory in DapAbort::InvalidMessage
- Make ProblemDetails::instance mandatory per RFC9457
- Merge DapError impl blocks


mendess: > 🔥 🚒 

Do you think this is a good implementation of the instance field? I opted for what felt easier + useful. At the same time I feel like no will ever make use of this
cjpatton: > > 🔥 🚒
> 
> Do you think this is a good implementation of the instance field? I opted for what felt easier + useful. At the same time I feel like no will ever make use of this

Hmm, actually the instance should be a URI: https://datatracker.ietf.org/doc/html/rfc7807#section-3.1
mendess: > > > 🔥 🚒
> > 
> > 
> > Do you think this is a good implementation of the instance field? I opted for what felt easier + useful. At the same time I feel like no will ever make use of this
> 
> Hmm, actually the instance should be a URI: https://datatracker.ietf.org/doc/html/rfc7807#section-3.1

that RFC has been obsoleted by 9457 and in that one it allows for a opaque string with meaning for the server: https://datatracker.ietf.org/doc/html/rfc9457#section-3.1.5-3

Maybe I misunderstood and it should always be a URI. If it must be a URI what URI would I even use? 
cjpatton: > that RFC has been obsoleted by 9457 and in that one it allows for a opaque string with meaning for the server: https://datatracker.ietf.org/doc/html/rfc9457#section-3.1.5-3
> 
> Maybe I misunderstood and it should always be a URI. If it must be a URI what URI would I even use?

Nice catch,, that's the correct reference. My read of this text is that it wants a proper URI that an HTTP client can get in order to get more info about the error. But you're right that it doesn't have to be "dereferencable", which I take to mean if I try "GET /some-instance" I may not get a response.

I'd suggest reformatting the strings so that they could be interpreted as relative URIs.

Patch:
diff --git a/crates/daphne-server/src/roles/aggregator.rs b/crates/daphne-server/src/roles/aggregator.rs
index 00aad48..f748dee 100644
--- a/crates/daphne-server/src/roles/aggregator.rs
+++ b/crates/daphne-server/src/roles/aggregator.rs
@@ -88,7 +88,9 @@ impl DapAggregator<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
 
         let durable = self.durable();
         let mut requests = Vec::new();
@@ -122,7 +124,9 @@ impl DapAggregator<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
 
         let durable = self.durable();
         let mut requests = Vec::new();
@@ -322,7 +326,9 @@ impl DapAggregator<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
 
         // Check whether the request overlaps with previous requests. This is done by
         // checking the AggregateStore and seeing whether it requests for aggregate
@@ -351,7 +357,9 @@ impl DapAggregator<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
         let version = task_config.as_ref().version;
 
         let agg_span = task_config.batch_span_for_sel(&BatchSelector::FixedSizeByBatchId {
@@ -466,7 +474,9 @@ impl HpkeProvider for crate::App {
         let version = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?
             .version;
 
         Ok(self
@@ -495,7 +505,7 @@ impl HpkeDecrypter for crate::App {
             .get_task_config_for(task_id)
             .await?
             .as_ref()
-            .ok_or(DapAbort::UnrecognizedTask)?
+            .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?
             .version;
         self.kv()
             .peek::<kv::prefix::HpkeReceiverConfigSet, _, _>(
diff --git a/crates/daphne-server/src/roles/leader.rs b/crates/daphne-server/src/roles/leader.rs
index cbef0d2..9cda289 100644
--- a/crates/daphne-server/src/roles/leader.rs
+++ b/crates/daphne-server/src/roles/leader.rs
@@ -49,7 +49,7 @@ impl DapLeader<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapAbort::UnrecognizedTask)?;
+            .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
 
         self.test_leader_state
             .lock()
@@ -61,7 +61,9 @@ impl DapLeader<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
 
         self.test_leader_state
             .lock()
@@ -79,7 +81,7 @@ impl DapLeader<DaphneAuth> for crate::App {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapAbort::UnrecognizedTask)?;
+            .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
 
         self.test_leader_state.lock().await.init_collect_job(
             task_id,
diff --git a/crates/daphne/src/error/aborts.rs b/crates/daphne/src/error/aborts.rs
index 200bd3b..b223167 100644
--- a/crates/daphne/src/error/aborts.rs
+++ b/crates/daphne/src/error/aborts.rs
@@ -5,7 +5,7 @@
 
 use crate::{
     fatal_error,
-    messages::{AggregationJobId, TaskId, TransitionFailure},
+    messages::{AggregationJobId, ReportId, TaskId, TransitionFailure},
     DapError, DapMediaType, DapRequest, DapVersion,
 };
 use hex::FromHexError;
@@ -18,7 +18,7 @@ use super::FatalDapError;
 // The display implementation of this error is used for metrics, as such, it can't be changed to
 // include field values
 /// DAP aborts.
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, PartialEq, Eq, thiserror::Error)]
 pub enum DapAbort {
     /// Bad request. Sent in response to an HTTP request that couldn't be handled preoprly.
     #[error("bad request")]
@@ -65,7 +65,7 @@ pub enum DapAbort {
     /// Report too late. Sent in response to an upload request for a task that is known to have
     /// expired.
     #[error("reportTooLate")]
-    ReportTooLate,
+    ReportTooLate { report_id: ReportId },
 
     /// Round mismatch. The aggregators disagree on the current round of the VDAF preparation protocol.
     /// This abort occurs during the aggregation sub-protocol.
@@ -90,14 +90,11 @@ pub enum DapAbort {
 
     /// Invalid message. Sent in response to a malformed or unexpected message.
     #[error("invalidMessage")]
-    InvalidMessage {
-        detail: String,
-        task_id: Option<TaskId>,
-    },
+    InvalidMessage { detail: String, task_id: TaskId },
 
     /// Unrecognized DAP task. Sent in response to a request indicating an unrecognized task ID.
     #[error("unrecognizedTask")]
-    UnrecognizedTask,
+    UnrecognizedTask { task_id: TaskId },
 }
 
 impl DapAbort {
@@ -105,27 +102,51 @@ impl DapAbort {
     /// request was targeted and `task_id` is the associated `TaskID`.
     pub fn into_problem_details(self) -> ProblemDetails {
         let (title, typ) = self.title_and_type();
-        let (task_id, detail, agg_job_id) = match self {
+        let to_instance = |params| {
+            format!(
+                "/problem-details/{}?{params}",
+                typ.as_deref().unwrap_or("badrequest"),
+            )
+        };
+        let (task_id, detail, agg_job_id, instance) = match self {
             Self::BatchInvalid { detail, task_id }
             | Self::InvalidTask { detail, task_id }
             | Self::BatchMismatch { detail, task_id }
             | Self::BatchOverlap { detail, task_id }
             | Self::InvalidBatchSize { detail, task_id }
             | Self::QueryMismatch { detail, task_id }
-            | Self::UnauthorizedRequest { detail, task_id } => (Some(task_id), Some(detail), None),
+            | Self::UnauthorizedRequest { detail, task_id }
+            | Self::InvalidMessage { detail, task_id } => (
+                Some(task_id),
+                Some(detail),
+                None,
+                to_instance(format_args!("task_id={task_id}")),
+            ),
             Self::MissingTaskId => (
                 None,
                 Some("A task ID must be specified in the query parameter of the request.".into()),
                 None,
+                to_instance(format_args!("task_id=none")),
             ),
             Self::BadRequest(detail) | Self::ReportRejected { detail } => {
-                (None, Some(detail), None)
+                (
+                    None,
+                    Some(detail),
+                    None,
+                    // TODO: make it possible to pass the rejected report id here.
+                    to_instance(format_args!("report_id=not-implemented")),
+                )
             }
             Self::RoundMismatch {
                 detail,
                 task_id,
                 agg_job_id,
-            } => (Some(task_id), Some(detail), Some(agg_job_id)),
+            } => (
+                Some(task_id),
+                Some(detail),
+                Some(agg_job_id),
+                to_instance(format_args!("task_id={task_id}&agg_job_id={agg_job_id}")),
+            ),
             Self::UnrecognizedAggregationJob {
                 task_id,
                 agg_job_id,
@@ -133,9 +154,20 @@ impl DapAbort {
                 Some(task_id),
                 Some("The request indicates an aggregation job that does not exist.".into()),
                 Some(agg_job_id),
+                to_instance(format_args!("agg_job_id={agg_job_id}")),
+            ),
+            Self::ReportTooLate { report_id } => (
+                None,
+                Some("one of the reports' timestamp was too late".into()),
+                None,
+                to_instance(format_args!("report_id={report_id}")),
+            ),
+            Self::UnrecognizedTask { task_id } => (
+                Some(task_id),
+                None,
+                None,
+                to_instance(format_args!("task_id={task_id}")),
             ),
-            Self::InvalidMessage { detail, task_id } => (task_id, Some(detail), None),
-            Self::ReportTooLate | Self::UnrecognizedTask => (None, None, None),
         };
 
         ProblemDetails {
@@ -143,7 +175,7 @@ impl DapAbort {
             title: title.to_string(),
             task_id,
             agg_job_id,
-            instance: None, // TODO interop: Implement as specified.
+            instance,
             detail,
         }
     }
@@ -254,7 +286,7 @@ impl DapAbort {
                 Some(self.to_string()),
             ),
             Self::ReportRejected { .. } => ("Report rejected", Some(self.to_string())),
-            Self::ReportTooLate => (
+            Self::ReportTooLate { .. } => (
                 "The requested task expires after report timestamp",
                 Some(self.to_string()),
             ),
@@ -265,7 +297,7 @@ impl DapAbort {
                 ("Unrecognized aggregation job", Some(self.to_string()))
             }
             Self::InvalidMessage { .. } => ("Malformed or invalid message", Some(self.to_string())),
-            Self::UnrecognizedTask => (
+            Self::UnrecognizedTask { .. } => (
                 "Task indicated by request is not recognized",
                 Some(self.to_string()),
             ),
@@ -280,17 +312,17 @@ impl DapAbort {
 }
 
 impl DapAbort {
-    pub fn from_codec_error<Id: Into<Option<TaskId>>>(e: CodecError, task_id: Id) -> Self {
+    pub fn from_codec_error(e: CodecError, task_id: TaskId) -> Self {
         Self::InvalidMessage {
             detail: format!("codec error: {e}"),
-            task_id: task_id.into(),
+            task_id,
         }
     }
 
     pub fn from_hex_error(e: FromHexError, task_id: TaskId) -> Self {
         Self::InvalidMessage {
             detail: format!("invalid hexadecimal string {e:?}"),
-            task_id: Some(task_id),
+            task_id,
         }
     }
 }
@@ -318,9 +350,79 @@ pub struct ProblemDetails {
     )]
     pub(crate) agg_job_id: Option<AggregationJobId>,
 
-    #[serde(skip_serializing_if = "Option::is_none")]
-    pub(crate) instance: Option<String>,
+    pub(crate) instance: String,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     pub detail: Option<String>,
 }
+
+#[cfg(test)]
+mod test {
+    use crate::messages::{AggregationJobId, ReportId, TaskId};
+
+    use super::{DapAbort, ProblemDetails};
+
+    #[test]
+    fn assert_that_instance_fields_are_url_safe() {
+        let detail = String::from("detail");
+        let task_id = TaskId(std::array::from_fn(|i| i.try_into().unwrap()));
+        let report_id = ReportId(std::array::from_fn(|i| i.try_into().unwrap()));
+        let agg_job_id = AggregationJobId(std::array::from_fn(|i| i.try_into().unwrap()));
+        let errors = [
+            DapAbort::BatchInvalid {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::BatchMismatch {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::BatchOverlap {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::InvalidBatchSize {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::InvalidTask {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::MissingTaskId,
+            DapAbort::QueryMismatch {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::ReportRejected {
+                detail: detail.clone(),
+            },
+            DapAbort::ReportTooLate { report_id },
+            DapAbort::RoundMismatch {
+                detail: detail.clone(),
+                task_id,
+                agg_job_id,
+            },
+            DapAbort::UnauthorizedRequest {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::UnrecognizedAggregationJob {
+                task_id,
+                agg_job_id,
+            },
+            DapAbort::InvalidMessage {
+                detail: detail.clone(),
+                task_id,
+            },
+            DapAbort::UnrecognizedTask { task_id },
+            DapAbort::BadRequest("bad-request".into()),
+        ];
+
+        for e in errors {
+            let ProblemDetails { instance, .. } = e.into_problem_details();
+            let instance_url = url::Url::parse(&format!("https://some-host.com{instance}"));
+            assert!(instance_url.is_ok(), "{instance:?} is not url safe");
+        }
+    }
+}
diff --git a/crates/daphne/src/error/mod.rs b/crates/daphne/src/error/mod.rs
index 33a9ecc..c510a14 100644
--- a/crates/daphne/src/error/mod.rs
+++ b/crates/daphne/src/error/mod.rs
@@ -12,7 +12,7 @@ use prio::codec::CodecError;
 use self::aborts::ProblemDetails;
 
 /// DAP errors.
-#[derive(Debug, thiserror::Error)]
+#[derive(Debug, PartialEq, Eq, thiserror::Error)]
 pub enum DapError {
     /// Fatal error. If this triggers an abort, then treat this as an internal error.
     ///
@@ -42,7 +42,7 @@ impl DapError {
             title: "Internal server error".into(),
             agg_job_id: None,
             task_id: None,
-            instance: None,
+            instance: "/problem-details/internal-server-error".into(),
             detail: None,
         }
     }
@@ -53,16 +53,7 @@ impl DapError {
             "encountered fatal error during encoding: {e}"
         )))
     }
-}
-
-impl FatalDapError {
-    #[doc(hidden)]
-    pub fn __use_the_macro(s: String) -> Self {
-        FatalDapError(s)
-    }
-}
 
-impl DapError {
     pub(crate) fn from_vdaf(e: VdafError) -> Self {
         match e {
             VdafError::Codec(..) | VdafError::Vdaf(..) => {
@@ -91,6 +82,13 @@ impl Debug for FatalDapError {
     }
 }
 
+impl FatalDapError {
+    #[doc(hidden)]
+    pub fn __use_the_macro(s: String) -> Self {
+        FatalDapError(s)
+    }
+}
+
 /// This macro is to be used when constructing fatal errors ([`DapError::Fatal`]).
 ///
 /// It follows the exact same syntax as
diff --git a/crates/daphne/src/protocol/aggregator.rs b/crates/daphne/src/protocol/aggregator.rs
index dfa0655..69c0eee 100644
--- a/crates/daphne/src/protocol/aggregator.rs
+++ b/crates/daphne/src/protocol/aggregator.rs
@@ -551,7 +551,7 @@ impl DapTaskConfig {
                             "report ID {} appears twice in the same aggregation job",
                             prep_init.report_share.report_metadata.id.to_base64url()
                         ),
-                        task_id: Some(*task_id),
+                        task_id: *task_id,
                     }
                     .into());
                 }
@@ -713,7 +713,7 @@ impl DapTaskConfig {
                     agg_job_resp.transitions.len(),
                     state.seq.len(),
                 ),
-                task_id: Some(*task_id),
+                task_id: *task_id,
             }
             .into());
         }
@@ -726,7 +726,7 @@ impl DapTaskConfig {
                         "report ID {} appears out of order in aggregation job response",
                         helper.report_id.to_base64url()
                     ),
-                    task_id: Some(*task_id),
+                    task_id: *task_id,
                 }
                 .into());
             }
@@ -743,7 +743,7 @@ impl DapTaskConfig {
                         // inevitable.
                         return Err(DapAbort::InvalidMessage {
                             detail: "The Helper's AggregationJobResp is invalid, but it may have already committed its state change. A batch mismatch is inevitable.".to_string(),
-                            task_id: Some(*task_id),
+                            task_id: *task_id,
                         }.into());
                     };
 
diff --git a/crates/daphne/src/protocol/collector.rs b/crates/daphne/src/protocol/collector.rs
index 02fbc21..d96fbea 100644
--- a/crates/daphne/src/protocol/collector.rs
+++ b/crates/daphne/src/protocol/collector.rs
@@ -92,6 +92,6 @@ impl VdafConfig {
             #[cfg(feature = "experimental")]
             Self::Pine(pine) => pine.unshard(num_measurements, agg_shares),
         }
-        .map_err(DapError::from_vdaf)
+        .map_err(|e| fatal_error!(err = ?e, "failed to unshard agg_shares"))
     }
 }
diff --git a/crates/daphne/src/roles/aggregator.rs b/crates/daphne/src/roles/aggregator.rs
index 0c1cb2b..edc4d39 100644
--- a/crates/daphne/src/roles/aggregator.rs
+++ b/crates/daphne/src/roles/aggregator.rs
@@ -183,7 +183,7 @@ where
         let task_config = aggregator
             .get_task_config_for(&task_id)
             .await?
-            .ok_or(DapAbort::UnrecognizedTask)?;
+            .ok_or(DapAbort::UnrecognizedTask { task_id })?;
 
         // Check whether the DAP version in the request matches the task config.
         if task_config.as_ref().version != req.version {
diff --git a/crates/daphne/src/roles/helper.rs b/crates/daphne/src/roles/helper.rs
index 6b081f7..74835bd 100644
--- a/crates/daphne/src/roles/helper.rs
+++ b/crates/daphne/src/roles/helper.rs
@@ -48,7 +48,7 @@ pub async fn handle_agg_job_init_req<'req, S: Sync, A: DapHelper<S>>(
     let wrapped_task_config = aggregator
         .get_task_config_for(task_id)
         .await?
-        .ok_or(DapAbort::UnrecognizedTask)?;
+        .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
     let task_config = wrapped_task_config.as_ref();
 
     if let Some(reason) = aggregator.unauthorized_reason(task_config, req).await? {
@@ -153,7 +153,7 @@ pub async fn handle_agg_share_req<'req, S: Sync, A: DapHelper<S>>(
     let wrapped_task_config = aggregator
         .get_task_config_for(req.task_id()?)
         .await?
-        .ok_or(DapAbort::UnrecognizedTask)?;
+        .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
     let task_config = wrapped_task_config.as_ref();
 
     if let Some(reason) = aggregator.unauthorized_reason(task_config, req).await? {
@@ -269,7 +269,7 @@ fn check_part_batch(
         // TODO spec: Define this behavior.
         return Err(DapAbort::InvalidMessage {
             detail: "invalid aggregation parameter".into(),
-            task_id: Some(*task_id),
+            task_id: *task_id,
         });
     }
 
diff --git a/crates/daphne/src/roles/leader/in_memory_leader.rs b/crates/daphne/src/roles/leader/in_memory_leader.rs
index 02d686a..32baaab 100644
--- a/crates/daphne/src/roles/leader/in_memory_leader.rs
+++ b/crates/daphne/src/roles/leader/in_memory_leader.rs
@@ -88,7 +88,9 @@ impl InMemoryLeaderState {
         }
 
         let Some(per_task) = self.per_task.get(task_id) else {
-            return Err(DapError::Abort(DapAbort::UnrecognizedTask));
+            return Err(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }));
         };
 
         per_task
@@ -193,7 +195,9 @@ impl InMemoryLeaderState {
                 .cloned()
                 .unwrap_or(DapCollectionJob::Unknown))
         } else {
-            Err(DapError::Abort(DapAbort::UnrecognizedTask))
+            Err(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))
         }
     }
 
diff --git a/crates/daphne/src/roles/leader/mod.rs b/crates/daphne/src/roles/leader/mod.rs
index 0a7c63c..78e184a 100644
--- a/crates/daphne/src/roles/leader/mod.rs
+++ b/crates/daphne/src/roles/leader/mod.rs
@@ -199,7 +199,7 @@ pub async fn handle_upload_req<S: Sync, A: DapLeader<S>>(
     let task_config = aggregator
         .get_task_config_for(task_id)
         .await?
-        .ok_or(DapAbort::UnrecognizedTask)?;
+        .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
 
     // Check whether the DAP version in the request matches the task config.
     if task_config.as_ref().version != req.version {
@@ -212,7 +212,7 @@ pub async fn handle_upload_req<S: Sync, A: DapLeader<S>>(
                 "expected exactly two encrypted input shares; got {}",
                 report.encrypted_input_shares.len()
             ),
-            task_id: Some(*task_id),
+            task_id: *task_id,
         }
         .into());
     }
@@ -230,7 +230,10 @@ pub async fn handle_upload_req<S: Sync, A: DapLeader<S>>(
 
     // Check that the task has not expired.
     if report.report_metadata.time >= task_config.as_ref().not_after {
-        return Err(DapAbort::ReportTooLate.into());
+        return Err(DapAbort::ReportTooLate {
+            report_id: report.report_metadata.id,
+        }
+        .into());
     }
     if report.report_metadata.time
         < task_config.as_ref().not_before - task_config.as_ref().time_precision
@@ -268,10 +271,11 @@ pub async fn handle_coll_job_req<S: Sync, A: DapLeader<S>>(
         resolve_taskprov(aggregator, task_id, req, &global_config).await?;
     }
 
+    let task_id = req.task_id()?;
     let wrapped_task_config = aggregator
-        .get_task_config_for(req.task_id()?)
+        .get_task_config_for(task_id)
         .await?
-        .ok_or(DapAbort::UnrecognizedTask)?;
+        .ok_or(DapAbort::UnrecognizedTask { task_id: *task_id })?;
     let task_config = wrapped_task_config.as_ref();
 
     if let Some(reason) = aggregator.unauthorized_reason(task_config, req).await? {
@@ -568,7 +572,7 @@ pub async fn process<S: Sync, A: DapLeader<S>>(
                     let task_config = aggregator
                         .get_task_config_for(&task_id)
                         .await?
-                        .ok_or(DapAbort::UnrecognizedTask)?;
+                        .ok_or(DapAbort::UnrecognizedTask { task_id })?;
 
                     if reports.is_empty() {
                         return Ok(0);
@@ -609,7 +613,7 @@ pub async fn process<S: Sync, A: DapLeader<S>>(
                 let task_config = aggregator
                     .get_task_config_for(&task_id)
                     .await?
-                    .ok_or(DapAbort::UnrecognizedTask)?;
+                    .ok_or(DapAbort::UnrecognizedTask { task_id })?;
 
                 tracing::debug!("RUNNING run_collect_job FOR TID {task_id} AND {coll_job_id} AND {batch_sel:?} AND {agg_param:?} AND {host}");
                 let collected = run_coll_job(
diff --git a/crates/daphne/src/roles/mod.rs b/crates/daphne/src/roles/mod.rs
index 84d6613..3570c18 100644
--- a/crates/daphne/src/roles/mod.rs
+++ b/crates/daphne/src/roles/mod.rs
@@ -32,7 +32,7 @@ async fn check_batch<S: Sync>(
         // TODO spec: Define this behavior.
         return Err(DapAbort::InvalidMessage {
             detail: "invalid aggregation parameter".into(),
-            task_id: Some(*task_id),
+            task_id: *task_id,
         }
         .into());
     }
@@ -749,9 +749,11 @@ mod test {
             ..Default::default()
         };
 
-        assert_matches!(
-            aggregator::handle_hpke_config_req(&*t.leader, &req, Some(task_id)).await,
-            Err(DapError::Abort(DapAbort::UnrecognizedTask))
+        assert_eq!(
+            aggregator::handle_hpke_config_req(&*t.leader, &req, Some(task_id))
+                .await
+                .unwrap_err(),
+            DapError::Abort(DapAbort::UnrecognizedTask { task_id })
         );
     }
 
@@ -1064,9 +1066,11 @@ mod test {
         };
 
         // Expect failure due to invalid task ID in report.
-        assert_matches!(
+        assert_eq!(
             leader::handle_upload_req(&*t.leader, &req).await,
-            Err(DapError::Abort(DapAbort::UnrecognizedTask))
+            Err(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: TaskId([0; 32])
+            }))
         );
     }
 
@@ -1088,11 +1092,13 @@ mod test {
             ..Default::default()
         };
 
-        assert_matches!(
+        assert_eq!(
             leader::handle_upload_req(&*t.leader, &req)
                 .await
                 .unwrap_err(),
-            DapError::Abort(DapAbort::ReportTooLate)
+            DapError::Abort(DapAbort::ReportTooLate {
+                report_id: report.report_metadata.id
+            })
         );
     }
 
diff --git a/crates/daphne/src/taskprov.rs b/crates/daphne/src/taskprov.rs
index c5de596..4190a87 100644
--- a/crates/daphne/src/taskprov.rs
+++ b/crates/daphne/src/taskprov.rs
@@ -139,7 +139,7 @@ fn get_taskprov_task_config<S>(
 
     if compute_task_id(taskprov_data.as_ref()) != *task_id {
         // Return unrecognizedTask following section 5.1 of the taskprov draft.
-        return Err(DapAbort::UnrecognizedTask);
+        return Err(DapAbort::UnrecognizedTask { task_id: *task_id });
     }
 
     // Return unrecognizedMessage if parsing fails following section 5.1 of the taskprov draft.
diff --git a/crates/daphne/src/vdaf/mod.rs b/crates/daphne/src/vdaf/mod.rs
index 338feff..71b7043 100644
--- a/crates/daphne/src/vdaf/mod.rs
+++ b/crates/daphne/src/vdaf/mod.rs
@@ -14,7 +14,6 @@ pub(crate) mod prio3;
 #[cfg(feature = "experimental")]
 use crate::pine::vdaf::PinePrepState;
 use crate::{
-    error::DapAbort,
     fatal_error,
     vdaf::{prio2::prio2_decode_prep_state, prio3::prio3_decode_prep_state},
     DapError,
@@ -389,23 +388,19 @@ impl VdafConfig {
     }
 
     /// Parse a verification key from raw bytes.
-    pub fn get_decoded_verify_key(&self, bytes: &[u8]) -> Result<VdafVerifyKey, DapError> {
+    pub fn get_decoded_verify_key(&self, bytes: &[u8]) -> Result<VdafVerifyKey, CodecError> {
         match self {
             Self::Prio3(Prio3Config::SumVecField64MultiproofHmacSha256Aes128 { .. })
-            | Self::Prio2 { .. } => {
-                Ok(VdafVerifyKey::L32(<[u8; 32]>::try_from(bytes).map_err(
-                    |e| DapAbort::from_codec_error(CodecError::Other(Box::new(e)), None),
-                )?))
-            }
-            Self::Prio3(..) => Ok(VdafVerifyKey::L16(<[u8; 16]>::try_from(bytes).map_err(
-                |e| DapAbort::from_codec_error(CodecError::Other(Box::new(e)), None),
-            )?)),
+            | Self::Prio2 { .. } => Ok(VdafVerifyKey::L32(
+                <[u8; 32]>::try_from(bytes).map_err(|e| CodecError::Other(Box::new(e)))?,
+            )),
+            Self::Prio3(..) => Ok(VdafVerifyKey::L16(
+                <[u8; 16]>::try_from(bytes).map_err(|e| CodecError::Other(Box::new(e)))?,
+            )),
             #[cfg(feature = "experimental")]
-            Self::Mastic { .. } | Self::Pine(..) => {
-                Ok(VdafVerifyKey::L16(<[u8; 16]>::try_from(bytes).map_err(
-                    |e| DapAbort::from_codec_error(CodecError::Other(Box::new(e)), None),
-                )?))
-            }
+            Self::Mastic { .. } | Self::Pine(..) => Ok(VdafVerifyKey::L16(
+                <[u8; 16]>::try_from(bytes).map_err(|e| CodecError::Other(Box::new(e)))?,
+            )),
         }
     }
 


Test patch:
diff --git a/crates/daphne/src/testing/mod.rs b/crates/daphne/src/testing/mod.rs
index f94ea7b..95de8d9 100644
--- a/crates/daphne/src/testing/mod.rs
+++ b/crates/daphne/src/testing/mod.rs
@@ -872,7 +872,9 @@ impl DapAggregator<BearerToken> for InMemoryAggregator {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
         let mut agg_store = self
             .agg_store
             .lock()
@@ -891,7 +893,9 @@ impl DapAggregator<BearerToken> for InMemoryAggregator {
         let task_config = self
             .get_task_config_for(task_id)
             .await?
-            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask))?;
+            .ok_or(DapError::Abort(DapAbort::UnrecognizedTask {
+                task_id: *task_id,
+            }))?;
 
         let aggregated = {
             let mut agg_store = self