diff --git a/aggregator/src/aggregator.rs b/aggregator/src/aggregator.rs index 0d991ec6e..8475a9f37 100644 --- a/aggregator/src/aggregator.rs +++ b/aggregator/src/aggregator.rs @@ -391,58 +391,64 @@ impl Aggregator { task_id_base64: Option<&[u8]>, ) -> Result<(Vec, Option), Error> { // Retrieve the appropriate HPKE config list. - let hpke_config_list = - if self.cfg.taskprov_config.enabled || self.cfg.require_global_hpke_keys { - // If we're running in taskprov mode or requiring global keys, unconditionally - // provide the global keys and ignore the task_id parameter. - let configs = self.global_hpke_keypairs.configs(); - if configs.is_empty() { - return Err(Error::Internal( - "this server is missing its global HPKE config".into(), - )); - } else { - HpkeConfigList::new(configs.to_vec()) - } + let hpke_config_list = if self.cfg.taskprov_config.enabled + || self.cfg.require_global_hpke_keys + { + // If we're running in taskprov mode or requiring global keys, unconditionally + // provide the global keys and ignore the task_id parameter. + let configs = self.global_hpke_keypairs.configs(); + if configs.is_empty() { + return Err(Error::Internal( + "this server is missing its global HPKE config".into(), + )); } else { - // Otherwise, try to get the task-specific key. - match task_id_base64 { - Some(task_id_base64) => { - let task_id_bytes = URL_SAFE_NO_PAD - .decode(task_id_base64) - .map_err(|_| Error::InvalidMessage(None, "task_id"))?; - let task_id = TaskId::get_decoded(&task_id_bytes) - .map_err(|_| Error::InvalidMessage(None, "task_id"))?; - let task_aggregator = self - .task_aggregators - .get(&task_id) - .await? - .ok_or(Error::UnrecognizedTask(task_id))?; - - match task_aggregator.handle_hpke_config() { - Some(hpke_config_list) => hpke_config_list, - // Assuming something hasn't gone horribly wrong with the database, this - // should only happen in the case where the system has been moved from taskprov - // mode to non-taskprov mode. Thus there's still taskprov tasks in the database. - // This isn't a supported use case, so the operator needs to delete these tasks - // or move the system back into taskprov mode. - None => { - return Err(Error::Internal("task has no HPKE configs".to_string())) - } + HpkeConfigList::new(configs.to_vec()) + } + } else { + // Otherwise, try to get the task-specific key. + match task_id_base64 { + Some(task_id_base64) => { + let task_id_bytes = URL_SAFE_NO_PAD.decode(task_id_base64).map_err(|_| { + Error::InvalidMessage(None, "task_id parameter is not valid base64url") + })?; + let task_id = TaskId::get_decoded(&task_id_bytes).map_err(|_| { + Error::InvalidMessage( + None, + "task_id parameter is not of the correct length", + ) + })?; + let task_aggregator = self + .task_aggregators + .get(&task_id) + .await? + .ok_or(Error::UnrecognizedTask(task_id))?; + + match task_aggregator.handle_hpke_config() { + Some(hpke_config_list) => hpke_config_list, + // Assuming something hasn't gone horribly wrong with the database, this + // should only happen in the case where the system has been moved from + // taskprov mode to non-taskprov mode. Thus there's still taskprov tasks + // in the database. This isn't a supported use case, so the operator + // needs to delete these tasks or move the system back into taskprov + // mode. + None => { + return Err(Error::Internal("task has no HPKE configs".to_string())) } } - // No task ID present, try to fall back to a global config. - None => { - let configs = self.global_hpke_keypairs.configs(); - if configs.is_empty() { - // This server isn't configured to provide global HPKE keys, the client - // should have given us a task ID. - return Err(Error::MissingTaskId); - } else { - HpkeConfigList::new(configs.to_vec()) - } + } + // No task ID present, try to fall back to a global config. + None => { + let configs = self.global_hpke_keypairs.configs(); + if configs.is_empty() { + // This server isn't configured to provide global HPKE keys, the client + // should have given us a task ID. + return Err(Error::MissingTaskId); + } else { + HpkeConfigList::new(configs.to_vec()) } } - }; + } + }; // Encode & (if configured to do so) sign the HPKE config list. let encoded_hpke_config_list = hpke_config_list diff --git a/aggregator/src/aggregator/aggregate_init_tests.rs b/aggregator/src/aggregator/aggregate_init_tests.rs index ccc47437c..1f8e193aa 100644 --- a/aggregator/src/aggregator/aggregate_init_tests.rs +++ b/aggregator/src/aggregator/aggregate_init_tests.rs @@ -669,6 +669,7 @@ async fn aggregation_job_init_wrong_query() { "status": StatusCode::BAD_REQUEST.as_u16(), "type": "urn:ietf:params:ppm:dap:error:invalidMessage", "title": "The message type for a response was incorrect or the payload was malformed.", + "detail": "Could not decode a message" }), ); } diff --git a/aggregator/src/aggregator/aggregation_job_continue.rs b/aggregator/src/aggregator/aggregation_job_continue.rs index 7fd1270fe..8334713cc 100644 --- a/aggregator/src/aggregator/aggregation_job_continue.rs +++ b/aggregator/src/aggregator/aggregation_job_continue.rs @@ -362,6 +362,7 @@ pub mod test_util { want_status: Status, want_error_type: &str, want_error_title: &str, + want_error_detail: Option<&str>, want_aggregation_job_id: Option<&AggregationJobId>, ) { let mut test_conn = post_aggregation_job_expecting_status( @@ -380,10 +381,14 @@ pub mod test_util { "taskid": format!("{}", task.id()), }); + let map = assert_matches!(expected_problem_details, serde_json::Value::Object(ref mut map) => map); + if let Some(job_id) = want_aggregation_job_id { - assert_matches!(expected_problem_details, serde_json::Value::Object(ref mut map) => { - map.insert("aggregation_job_id".into(), format!("{job_id}").into()); - }); + map.insert("aggregation_job_id".into(), format!("{job_id}").into()); + } + + if let Some(detail) = want_error_detail { + map.insert("detail".into(), detail.into()); } assert_eq!( @@ -640,6 +645,7 @@ mod tests { "urn:ietf:params:ppm:dap:error:unrecognizedTask", "An endpoint received a message with an unknown task ID.", None, + None, ) .await; } @@ -701,6 +707,7 @@ mod tests { Status::BadRequest, "urn:ietf:params:ppm:dap:error:invalidMessage", "The message type for a response was incorrect or the payload was malformed.", + Some("aggregation job cannot be advanced to step 0"), None, ) .await; @@ -874,6 +881,7 @@ mod tests { "urn:ietf:params:ppm:dap:error:stepMismatch", "The leader and helper are not on the same step of VDAF preparation.", None, + None, ) .await; } @@ -898,6 +906,7 @@ mod tests { "urn:ietf:params:ppm:dap:error:stepMismatch", "The leader and helper are not on the same step of VDAF preparation.", None, + None, ) .await; } @@ -986,6 +995,7 @@ mod tests { Status::Gone, "https://docs.divviup.org/references/janus-errors#aggregation-job-deleted", "The aggregation job has been deleted.", + None, Some(&test_case.aggregation_job_id), ) .await; diff --git a/aggregator/src/aggregator/http_handlers.rs b/aggregator/src/aggregator/http_handlers.rs index 0128af572..eb9f7f1ab 100644 --- a/aggregator/src/aggregator/http_handlers.rs +++ b/aggregator/src/aggregator/http_handlers.rs @@ -51,7 +51,10 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { let conn = match error { Error::InvalidConfiguration(_) => conn.with_status(Status::InternalServerError), Error::MessageDecode(_) => { - conn.with_problem_document(&ProblemDocument::new_dap(DapProblemType::InvalidMessage)) + conn.with_problem_document( + &ProblemDocument::new_dap(DapProblemType::InvalidMessage) + .with_detail("Could not decode a message"), + ) } Error::MessageEncode(_) => conn.with_status(Status::InternalServerError), Error::ReportRejected(rejection) => match rejection.reason() { @@ -69,8 +72,9 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { .with_detail(rejection.reason().detail()), ), }, - Error::InvalidMessage(task_id, _) => { + Error::InvalidMessage(task_id, detail) => { let mut doc = ProblemDocument::new_dap(DapProblemType::InvalidMessage); + doc = doc.with_detail(detail); if let Some(task_id) = task_id { doc = doc.with_task_id(task_id); } @@ -148,7 +152,9 @@ async fn run_error_handler(error: &Error, mut conn: Conn) -> Conn { | Error::TaskParameters(_) => conn.with_status(Status::InternalServerError), Error::AggregateShareRequestRejected(_, _) => conn.with_status(Status::BadRequest), Error::EmptyAggregation(task_id) => conn.with_problem_document( - &ProblemDocument::new_dap(DapProblemType::InvalidMessage).with_task_id(task_id), + &ProblemDocument::new_dap(DapProblemType::InvalidMessage) + .with_task_id(task_id) + .with_detail("An empty aggregation with no report shares was attempted"), ), Error::ForbiddenMutation { .. } => conn.with_status(Status::Conflict), Error::BadRequest(_) => conn.with_status(Status::BadRequest), diff --git a/aggregator/src/aggregator/http_handlers/tests/aggregation_job_continue.rs b/aggregator/src/aggregator/http_handlers/tests/aggregation_job_continue.rs index ee75e27c1..faf5d09dc 100644 --- a/aggregator/src/aggregator/http_handlers/tests/aggregation_job_continue.rs +++ b/aggregator/src/aggregator/http_handlers/tests/aggregation_job_continue.rs @@ -1494,6 +1494,7 @@ async fn aggregate_continue_unexpected_transition() { Status::BadRequest, "urn:ietf:params:ppm:dap:error:invalidMessage", "The message type for a response was incorrect or the payload was malformed.", + Some("leader sent unexpected, duplicate, or out-of-order prepare steps"), None, ) .await; @@ -1672,6 +1673,7 @@ async fn aggregate_continue_out_of_order_transition() { Status::BadRequest, "urn:ietf:params:ppm:dap:error:invalidMessage", "The message type for a response was incorrect or the payload was malformed.", + Some("leader sent unexpected, duplicate, or out-of-order prepare steps"), None, ) .await; @@ -1766,6 +1768,7 @@ async fn aggregate_continue_for_non_waiting_aggregation() { Status::BadRequest, "urn:ietf:params:ppm:dap:error:invalidMessage", "The message type for a response was incorrect or the payload was malformed.", + Some("leader sent prepare step for non-WAITING report aggregation"), None, ) .await; diff --git a/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs b/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs index 6b3ad38f2..39d41b3ea 100644 --- a/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs +++ b/aggregator/src/aggregator/http_handlers/tests/aggregation_job_init.rs @@ -971,6 +971,7 @@ async fn aggregate_init_duplicated_report_id() { "status": want_status, "type": "urn:ietf:params:ppm:dap:error:invalidMessage", "title": "The message type for a response was incorrect or the payload was malformed.", + "detail": "aggregate request contains duplicate report IDs", "taskid": format!("{}", task.id()), }) ); diff --git a/aggregator/src/aggregator/http_handlers/tests/collection_job.rs b/aggregator/src/aggregator/http_handlers/tests/collection_job.rs index c9502d0ce..68ab95af2 100644 --- a/aggregator/src/aggregator/http_handlers/tests/collection_job.rs +++ b/aggregator/src/aggregator/http_handlers/tests/collection_job.rs @@ -131,6 +131,7 @@ async fn collection_job_put_request_invalid_aggregation_parameter() { "status": Status::BadRequest as u16, "type": "urn:ietf:params:ppm:dap:error:invalidMessage", "title": "The message type for a response was incorrect or the payload was malformed.", + "detail": "Could not decode a message", }) ); } diff --git a/aggregator/src/aggregator/taskprov_tests.rs b/aggregator/src/aggregator/taskprov_tests.rs index 85564b958..f7a54f74c 100644 --- a/aggregator/src/aggregator/taskprov_tests.rs +++ b/aggregator/src/aggregator/taskprov_tests.rs @@ -701,6 +701,7 @@ async fn taskprov_opt_out_mismatched_task_id() { "status": Status::BadRequest as u16, "type": "urn:ietf:params:ppm:dap:error:invalidMessage", "title": "The message type for a response was incorrect or the payload was malformed.", + "detail": "derived taskprov task ID does not match task config", "taskid": format!("{}", test.task_id), }) );