Add support for public extensions in Reports. #754

jhoyla · 2024-12-31T15:38:22Z

Draft 13 adds mandatory support for public extensions in the ReportMetadata, but doesn't define any (Taskprov is (for now) a private extension). This PR adds support for rejecting unknown extensions, which is all of them.

cjpatton

I believe we're missing one feature: we need to resolve repeated extensions across the public and private extension fields.

crates/daphne/src/error/aborts.rs

crates/daphne/src/messages/mod.rs

crates/daphne/src/protocol/report_init.rs

jhoyla · 2025-01-02T13:05:20Z

I believe we're missing one feature: we need to resolve repeated extensions across the public and private extension fields.

So the spec says:

type. (For the purposes of this check, a private report extension can conflict
with a public report extension.) If so, the Aggregator MUST mark the input share
as invalid with error invalid_message.

Which I take to mean a conflict between the public and private extensions is allowed.
If I add support for taskprov as a public extension do I reject it if it appears in both places?

mendess

not even a nit, just a comment. I don't have a preference but instead of Vec::new() you can write vec![]

cjpatton · 2025-01-02T16:40:28Z

I believe we're missing one feature: we need to resolve repeated extensions across the public and private extension fields.

So the spec says:
type. (For the purposes of this check, a private report extension can conflict
with a public report extension.) If so, the Aggregator MUST mark the input share
as invalid with error invalid_message.
Which I take to mean a conflict between the public and private extensions is allowed. If I add support for taskprov as a public extension do I reject it if it appears in both places?

The intent of the text is: "if the same extension appears twice, then the report MUST be rejected - regardless of what fields the extensions appear in." In particular, if the taskprov extension appears in the public and private fields, then reject.

cjpatton

The code looks largely correct, though I have some suggestions for behavior changes. I think the tests need a bit of work. Also, I have some suggestions for making the code a little cleaner/easier to read.

cjpatton · 2025-01-09T23:33:08Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -533,6 +537,150 @@ async fn leader_upload_taskprov_wrong_version(version: DapVersion) {

 async_test_versions!(leader_upload_taskprov_wrong_version);

+#[tokio::test]


I think the test coverage in the daphne::roles module is high enough that we could just as easily move theses tests there. You can call leader::handle_upload_req() on the mock aggregator, just as you already do in a new test in that module.

Any code path that can be tested without the e2e setup ought to be moved to a unit test, since unit tests are easier to run.

Can we move these tests to the daphne crate?

cjpatton · 2025-01-09T23:35:30Z

crates/daphne/src/error/aborts.rs

+        task_id: &TaskId,
+        unknown_extensions: &[u16],
+    ) -> Result<Self, DapError> {
+        let detail = serde_json::to_string(&unknown_extensions);


Unnecessary reference

Suggested change

let detail = serde_json::to_string(&unknown_extensions);

let detail = serde_json::to_string(unknown_extensions);

Why use json here? We don't expect anyone to parse the detail, you could just use

Suggested change

let detail = serde_json::to_string(&unknown_extensions);

let detail = format!("{unknown_extensions:?}");

which I think will output exactly the same thing in this case

cjpatton · 2025-01-09T23:35:45Z

crates/daphne/src/error/aborts.rs

+                detail: s,
+                task_id: *task_id,
+            }),
+            Err(x) => Err(fatal_error!(err = %x,)),


Suggested change

Err(x) => Err(fatal_error!(err = %x,)),

Err(x) => Err(fatal_error!(err = %x)),

cjpatton · 2025-01-09T23:36:54Z

crates/daphne/src/error/aborts.rs

+        task_id: &TaskId,
+        unknown_extensions: &[u16],
+    ) -> Result<Self, DapError> {
+        let detail = serde_json::to_string(&unknown_extensions);


More Rusty

Suggested change

let detail = serde_json::to_string(&unknown_extensions);

let detail = serde_json::to_string(&unknown_extensions).map_err(|e| fatal_error!(err = e))?;

cjpatton · 2025-01-09T23:41:20Z

crates/daphne/src/protocol/client.rs

+            public_extensions: match version {
+                DapVersion::Draft09 => None,
+                DapVersion::Latest => Some(Vec::new()),
+            },


Since the extensions are replicated between the Leader and Helper, perhaps it makes more sense to include them in the public extensions field rather than in the private extension fields.

If you take this suggestion, let's also rename extensions to public_extensions.

cjpatton · 2025-01-10T00:00:08Z

crates/daphne/src/protocol/mod.rs

+            }
+        );
+    }
+    test_versions! {handle_unknown_public_extensions_in_report}


Note: This creates the test for all versions of DAP, including draft 09.

cjpatton · 2025-01-10T00:04:49Z

crates/daphne/src/roles/leader/mod.rs

+    ) {
+        (Some(extensions), DapVersion::Latest) => {
+            let mut unknown_extensions = Vec::<u16>::new();
+            if crate::protocol::no_duplicates(extensions.iter()).is_err() {


And use crate::protocol;. Also, we should rename no_duplicates to something more distinctive.

Suggested change

if crate::protocol::no_duplicates(extensions.iter()).is_err() {

if protocol::no_duplicates(extensions.iter()).is_err() {

crates/daphne/src/roles/leader/mod.rs

cjpatton · 2025-01-10T00:10:44Z

crates/daphne/src/roles/leader/mod.rs

+        (Some(_), DapVersion::Draft09) => {
+            return Err(DapError::Abort(DapAbort::version_mismatch(
+                DapVersion::Draft09,
+                DapVersion::Latest,
+            )))
+        }
+        (None, DapVersion::Latest) => {
+            return Err(DapError::Abort(DapAbort::version_mismatch(
+                DapVersion::Latest,
+                DapVersion::Draft09,
+            )))
+        }


I'm not sure either of these arms could ever be reached, assuming the code that sets public_extensions is correct. If I were you, I would use the public_extension.is_some() to deduce which version was negotiated. Tnen we could replace this big ugly match with:

if let Some(public_extensions) = &report.report_metadata.public_extensions { ... // check for duplicates, unorecgonized values, and so on }

cjpatton · 2025-01-10T00:14:38Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -1348,6 +1496,116 @@ async fn leader_selected() {
    .unwrap();
 }

+#[tokio::test]
+async fn leader_collect_taskprov_repeated_abort() {


What does this new test do?

It checks that the Leader aborts if the taskprov extension appears in both the public and private extensions.

I fixed a bug in it too.

Can we move this to the unit tests in the daphne crate. or does exercise a behavior that's not implemented by InMemoryAggregator?

cjpatton

Looks pretty good. Let's chat in our 1:1 about the necessity of the e2e tests.

cjpatton · 2025-01-14T15:24:14Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -533,6 +537,150 @@ async fn leader_upload_taskprov_wrong_version(version: DapVersion) {

 async_test_versions!(leader_upload_taskprov_wrong_version);

+#[tokio::test]


Can we move these tests to the daphne crate?

cjpatton · 2025-01-14T15:28:28Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -1348,6 +1496,116 @@ async fn leader_selected() {
    .unwrap();
 }

+#[tokio::test]
+async fn leader_collect_taskprov_repeated_abort() {


Can we move this to the unit tests in the daphne crate. or does exercise a behavior that's not implemented by InMemoryAggregator?

cjpatton · 2025-01-14T15:32:40Z

crates/daphne/src/messages/mod.rs

+            (DapVersion::Latest, Some(extensions)) => {
+                encode_u16_items(bytes, version, extensions.as_slice())?;
+            }
+            _ => return Err(CodecError::UnexpectedValue),


We usually use this for encoding. How about his:

Suggested change

_ => return Err(CodecError::UnexpectedValue),

_ => return Err(CodecError::Other("encountered incorrectly set public extensions".into()))s

cjpatton · 2025-01-14T15:38:05Z

crates/daphne/src/protocol/client.rs

+                return Err(DapError::ReportError(
+                    crate::messages::ReportError::InvalidMessage,
+                ))


I would actually go with a fatal error here

Suggested change

return Err(DapError::ReportError(

crate::messages::ReportError::InvalidMessage,

))

return Err(fatal_error!(

err = format!("public extensions not set correctly for {version:?}")

));

cjpatton · 2025-01-14T15:39:39Z

crates/daphne/src/protocol/client.rs

+                    crate::messages::ReportError::InvalidMessage,
+                ))
+            }
+            _ => (),


nit: you can avoid having to specify this arm using if let:F

if let (Some(_), DapVersion::Draft09) | (None, DapVersion::Latest) = (&public_extensions, version) { ... }

cjpatton · 2025-01-14T15:44:50Z

crates/daphne/src/roles/leader/mod.rs

@@ -223,6 +226,34 @@ pub async fn handle_upload_req<A: DapLeader>(
        .into());
    }

+    if let Some(public_extensions) = &report.report_metadata.public_extensions {
+        // We can be sure at this point that the ReportMetadata is well formed (as
+        // the decoding / error checking happens in the extractor).


Suggested change

// the decoding / error checking happens in the extractor).

// the decoding / error checking happens in the extractor.

cjpatton · 2025-01-14T15:45:45Z

crates/daphne/src/roles/leader/mod.rs

+        if protocol::check_no_duplicates(public_extensions.iter()).is_err() {
+            return Err(DapError::Abort(DapAbort::InvalidMessage {
+                detail: "Repeated public extension".into(),
+                task_id,
+            }));
+        };


IF the metdata "well-formed" (see above comment), then do we need to check for repeats here?

cjpatton · 2025-01-14T16:40:07Z

crates/daphne-server/tests/e2e/e2e.rs

+    let mut report = task_config
+        .vdaf
+        .produce_report(
+            &hpke_config_list,
+            t.now + 1,
+            &task_id,
+            DapMeasurement::U32Vec(vec![1; 10]),
+            version,
+        )
+        .unwrap();
+    report.report_metadata.public_extensions = Some(vec![Extension::Taskprov, Extension::Taskprov]);


Overwriting exetensions causes an HPKE decryption failure, which isn't what we intend to test here. Modify produce_report() to take in public extensions and set them as you've set them here.

cjpatton · 2025-01-14T16:40:36Z

crates/daphne-server/tests/e2e/e2e.rs

+    report.report_metadata.public_extensions = Some(vec![
+        Extension::Taskprov,
+        Extension::NotImplemented {
+            typ: 3,
+            payload: b"ignore".to_vec(),
+        },
+    ]);


Same thing here: don't override

cjpatton · 2025-01-14T16:40:57Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -533,6 +549,150 @@ async fn leader_upload_taskprov_wrong_version(version: DapVersion) {

 async_test_versions!(leader_upload_taskprov_wrong_version);

+#[tokio::test]
+async fn leader_upload_taskprov_public() {


Remove this test (already covered by unit tests).

cjpatton · 2025-01-14T16:42:41Z

crates/daphne-server/tests/e2e/e2e.rs

@@ -1348,6 +1508,116 @@ async fn leader_selected() {
    .unwrap();
 }

+#[tokio::test]
+async fn leader_collect_taskprov_repeated_abort() {


Make sure there's a unit test for this and delete.

cjpatton · 2025-01-14T16:43:28Z

crates/daphne/src/roles/mod.rs

+        let mut report = t.gen_test_report(task_id).await;
+        report.report_metadata.public_extensions = Some(vec![Extension::NotImplemented {
+            typ: 0x01,
+            payload: vec![0x01],
+        }]);


Don't override, as this causes hpke decryption to fail. Here and below.

jhoyla requested review from armfazh, bhalleycf, cjpatton, fisherdarling, cbranch, lbaquerofierro, mendess and RageKnify as code owners December 31, 2024 15:38

cjpatton reviewed Jan 1, 2025

View reviewed changes

mendess reviewed Jan 2, 2025

View reviewed changes

jhoyla force-pushed the jhoyla/public-extensions branch 4 times, most recently from eda9b3c to 5078b99 Compare January 8, 2025 12:41

jhoyla requested review from mendess and cjpatton January 8, 2025 12:49

mendess approved these changes Jan 8, 2025

View reviewed changes

cjpatton requested changes Jan 10, 2025

View reviewed changes

jhoyla requested review from cjpatton and mendess January 14, 2025 14:33

jhoyla force-pushed the jhoyla/public-extensions branch from 2728fcd to 28ff330 Compare January 14, 2025 14:42

jhoyla added 2 commits January 14, 2025 14:44

Add support for public extensions in Reports.

35297ad

Address comments

0287417

jhoyla force-pushed the jhoyla/public-extensions branch from 28ff330 to 0287417 Compare January 14, 2025 14:46

cjpatton reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for public extensions in Reports. #754

Add support for public extensions in Reports. #754

jhoyla commented Dec 31, 2024 •

edited

Loading

cjpatton left a comment

jhoyla commented Jan 2, 2025 •

edited

Loading

mendess left a comment

cjpatton commented Jan 2, 2025

cjpatton left a comment

cjpatton Jan 9, 2025

cjpatton Jan 14, 2025

cjpatton Jan 9, 2025

mendess Jan 10, 2025

cjpatton Jan 9, 2025

cjpatton Jan 9, 2025

cjpatton Jan 9, 2025

cjpatton Jan 10, 2025

cjpatton Jan 10, 2025

cjpatton Jan 10, 2025

cjpatton Jan 10, 2025

jhoyla Jan 14, 2025

jhoyla Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton left a comment

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

cjpatton Jan 14, 2025

		@@ -533,6 +537,150 @@ async fn leader_upload_taskprov_wrong_version(version: DapVersion) {

		async_test_versions!(leader_upload_taskprov_wrong_version);

		#[tokio::test]

	let detail = serde_json::to_string(&unknown_extensions);
	let detail = serde_json::to_string(unknown_extensions);

	let detail = serde_json::to_string(&unknown_extensions);
	let detail = format!("{unknown_extensions:?}");

	Err(x) => Err(fatal_error!(err = %x,)),
	Err(x) => Err(fatal_error!(err = %x)),

	if crate::protocol::no_duplicates(extensions.iter()).is_err() {
	if protocol::no_duplicates(extensions.iter()).is_err() {

	_ => return Err(CodecError::UnexpectedValue),
	_ => return Err(CodecError::Other("encountered incorrectly set public extensions".into()))s

	// the decoding / error checking happens in the extractor).
	// the decoding / error checking happens in the extractor.

Add support for public extensions in Reports. #754

Are you sure you want to change the base?

Add support for public extensions in Reports. #754

Conversation

jhoyla commented Dec 31, 2024 • edited Loading

cjpatton left a comment

Choose a reason for hiding this comment

jhoyla commented Jan 2, 2025 • edited Loading

mendess left a comment

Choose a reason for hiding this comment

cjpatton commented Jan 2, 2025

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjpatton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhoyla commented Dec 31, 2024 •

edited

Loading

jhoyla commented Jan 2, 2025 •

edited

Loading