Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
tgeoghegan opened this issue Jun 14, 2023 · 13 comments · Fixed by #566
Closed

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

tgeoghegan opened this issue Jun 14, 2023 · 13 comments · Fixed by #566
Assignees
Labels
editorial The issue raised is purely editorial (i.e., doesn't impact implementations). ietf120

Comments

@tgeoghegan
Copy link
Collaborator

tgeoghegan commented Jun 14, 2023

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
Copy link
Contributor

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
Copy link
Collaborator

+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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

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 cjpatton added editorial The issue raised is purely editorial (i.e., doesn't impact implementations). ietf118 and removed draft-06 labels Oct 24, 2023
@cjpatton
Copy link
Collaborator

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
Copy link

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

    case reject:
      ReportId report_id;
      ReportShareError report_share_error;

@cjpatton
Copy link
Collaborator

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

@cjpatton
Copy link
Collaborator

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
Copy link
Collaborator

cjpatton commented Nov 7, 2023

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
Copy link
Collaborator Author

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.

@cjpatton cjpatton removed the ietf118 label Jun 4, 2024
@cjpatton cjpatton assigned cjpatton and unassigned chris-wood Jun 26, 2024
@cjpatton cjpatton assigned tgeoghegan and unassigned cjpatton Jun 26, 2024
@tgeoghegan tgeoghegan added editorial The issue raised is purely editorial (i.e., doesn't impact implementations). and removed editorial The issue raised is purely editorial (i.e., doesn't impact implementations). labels Jun 26, 2024
tgeoghegan added a commit that referenced this issue Jul 6, 2024
Explains in detail how the syntax from RFC 8446 section 3.7 is
repurposed for concise descriptions of structure variants. Additionally,
the section on message encoding is moved to "Protocol Definition" to
make the document flow better.

Closes #472
@tgeoghegan
Copy link
Collaborator Author

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

  • I don't think either of the extensions from MLS are useful for us: there's nowhere we would use Optional<T> (or at least nowhere that I think is more clear than what we have now), and I don't think the variable length vector encoding is worthwhile for us.
  • I don't think there's a strong enough case for rewriting everything into RFC 9000 syntax. Proponents of that approach are welcome to draft a PR, though.

@cjpatton
Copy link
Collaborator

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

tgeoghegan added a commit that referenced this issue Jul 27, 2024
Explains in detail how the syntax from RFC 8446 section 3.7 is
repurposed for concise descriptions of structure variants. Additionally,
the section on message encoding is moved to "Protocol Definition" to
make the document flow better.

Closes #472
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editorial The issue raised is purely editorial (i.e., doesn't impact implementations). ietf120
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants