ietf-wg-ppm / draft-ietf-ppm-dap

This document describes the Distributed Aggregation Protocol (DAP) being developed by the PPM working group at IETF.
Other
46 stars 22 forks source link

Document extensions to and/or deviations from RFC 8446 presentation language #472

Closed tgeoghegan closed 2 months ago

tgeoghegan commented 1 year ago

In #393, we define a union with an enum discriminant roughly like:

enum {
  continue(0),
  finished(1)
  reject(2),
  (255)
} PrepareStepState;

struct {
  PrepareStepState prepare_step_state;
  select (PrepareStep.prepare_step_state) {
    case continue:
      ReportId report_id;
      opaque payload<0..2^32-1>;
    case finished:
      ReportId report_id;
    case reject:
      ReportId report_id;
      ReportShareError report_share_error;
  };
} PrepareStep;

Then later in protocol text, we define partial constructions of values like:

struct {
  PrepareStepState prepare_step_state = 2; /* reject */
  ReportId report_id;
  ReportShareError report_share_error;
} PrepareStep;

This notation is clear and certainly is more compact than a paragraph explaining to implementers what enum variant to use, but it conflicts with section 3.7 of RFC 8446, which tells us that this is the notation for defining structs that contain fields whose values are fixed (I gather this was useful in TLS 1.3 for backward compatibility with older TLS versions).

We can resolve this by adding some text to "Conventions and Definitions", where we introduce the usage of RFC 8446 presentation language, explaining our extensions and deviations. There may also be some prior art in other TLS WG documents we can reference, but I haven't found it.

wangshan commented 1 year ago

Alternatively, can we use a different syntax to describe object, like the pseudo code used to describe HPKE and VDAF interaction. For e.g.

  return PrepareResp(report_id: ReportID, prepare_resp_state: PrepareRespState = reject, prepare_error: PrepareError);
cjpatton commented 1 year ago

+1 to clarifying, and I like @wangshan's suggestion here. VDAF is just Python3, so to match it we'd do something like

PrepareResp(report_id, prepare_resp_type=reject, prepare_error=vdaf_prep_error)
simon-friedberger commented 1 year ago

A Python constructor call could potentially do a lot of things, though. The original phrasing that just nails down a few bytes of the message is more clear about the simplicity.

tgeoghegan commented 1 year ago

We have not ever used Python notation in DAP, and I don't think we should introduce it just for this. It'd be much easier to document a couple extensions to/deviations from RFC 8446 presentation language.

wangshan commented 1 year ago

I don't think we have to use full python syntax either, but the section in question is talking about instantiation of structs, instead of defining a struct type. I feel pseudo code communicates that well, since the purpose is to let people know what the message contains in different conditions, rather than how to encode/decode the message. If we introduce deviations, don't we have to introduce new syntax to explain why in this case it's not a fixed value but an assignment?

cjpatton commented 11 months ago

Are there other deviations from TLS-syntax that we're aware of in the spec? cc/ @wangshan who has spent some time on this before.

suman-ganta commented 11 months ago

This kind of multi fields are not part of the spec either.

    case reject:
      ReportId report_id;
      ReportShareError report_share_error;
cjpatton commented 11 months ago

Ahh, yup, I think @suman-ganta could be right! Thanks, I'll add this to my slides for IETF 118.

cjpatton commented 11 months ago

I didn't find an example of this here, but I did in VDAF: https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-vdaf#section-5.8-6

cjpatton commented 11 months ago

At IETF 118 people were mostly inclined to clarify in the draft any discrepancies with TLS-syntax as they arise. @chris-wood offered an alternative option, which is to adopt QUIC-representation language, which may be expressive enough for our purposes. We decided to look at a PR and make a decision then if the wire change is worth the effort.

tgeoghegan commented 4 months ago

RFC 9420 2.1 has some prior art on documenting extensions to TLS PR. And actually we should check whether we want to adopt their optional and variable length vector notation.

tgeoghegan commented 2 months ago

556 addresses this to my satisfaction. To answer some specific points from above:

cjpatton commented 2 months ago

Decision at IETF 120 is to merge @tgeoghegan's PR.