Add public report extensions. #632
Conversation
|
Closes #620. +cc @martinthomson |
draft-ietf-ppm-dap.md
Outdated
| @@ -1157,6 +1157,7 @@ struct { | |||
| struct { | |||
| ReportMetadata report_metadata; | |||
| opaque public_share<0..2^32-1>; | |||
| Extension public_extensions<0..2^32-1>; | |||
There was a problem hiding this comment.
This change would be smaller if you added the extensions to ReportMetadata. Any reason why you chose not to?
There was a problem hiding this comment.
I should have added. 4 gigabytes is an awful lot. Do I have to start sharpening my varint axe? There's a lot of overhead in this protocol that would be trivially cut with a single slice.
There was a problem hiding this comment.
+1, let's move this to the report metadata. This will also help with code movement in daphne.
There was a problem hiding this comment.
Sure, no objection to phrasing things that way.
There was a problem hiding this comment.
I should have added. 4 gigabytes is an awful lot. Do I have to start sharpening my varint axe? There's a lot of overhead in this protocol that would be trivially cut with a single slice.
Using varints for length specifiers is not (AFAICT) supported by TLS syntax we currently use; we'd probably extend our syntax somehow to specify this. I'd need to do a little digging to see what varint formats are currently specified/standardized; a very quick glance does reveal anything. (Of course, if we can't lean on prior work we could certainly specify something ourselves.)
edit: if we do want to discuss use of varints for field lengths, let's do so outside this PR. It is very likely we'd apply this technique to field lengths across many messages used by DAP, not only for the extension list.
There was a problem hiding this comment.
Oh, I should also add: I'm in favor of a 2-byte prefix, for consistency with the private extensions.
draft-ietf-ppm-dap.md
Outdated
| @@ -1157,6 +1157,7 @@ struct { | |||
| struct { | |||
| ReportMetadata report_metadata; | |||
| opaque public_share<0..2^32-1>; | |||
| Extension public_extensions<0..2^32-1>; | |||
There was a problem hiding this comment.
+1, let's move this to the report metadata. This will also help with code movement in daphne.
branlwyd
force-pushed
the
bran/public-report-extensions
branch
from
4ca44d1 to
d93d076
Compare
The existing (private) report extension mechanism is renamed to "private extensions." A few notes: * Public extensions are always the same for both aggregators; the public extensions are included in the input share AAD to ensure that the Leader can't modify these while passing them along to the Helper. * Public & private extensions share the same "namespace", i.e. they use the same set of extension type codepoints, and duplicating an extension in both the public & private extensions is considered invalid.
branlwyd
force-pushed
the
bran/public-report-extensions
branch
from
156a286 to
b6682fc
Compare
|
Rebased & squashed. |
The existing (private) report extension mechanism is renamed to "private extensions."
A few notes: