mlswg / mls-protocol

MLS protocol
https://messaginglayersecurity.rocks
Other
234 stars 60 forks source link

Minor clarifications for AUTH48 #878

Closed bifurcation closed 1 year ago

bifurcation commented 1 year ago

This PR includes clarifications on a few minor points that were found during interoperability testing.

  1. The prose description of the info parameter to HPKE conflicts with the pseudocode. This PR deletes the conflicting prose.
  2. When LeafNode.extensions contains GREASE extensions, those extensions have to be reflected in LeafNode.capabilities.extensions, or else the leaf note is invalid. This PR adds a cautionary note to this effect.
  3. 875 allows GREASE extensions in LeafNode.extensions, but did not add LN to the IANA registrations for the GREASE extension values. This PR adds that notation.

We plan to hold this PR open until AUTH48 begins, then apply the changes in AUTH48.

dconnolly commented 1 year ago

2. When LeafNode.extensions contains GREASE extensions, those extensions have to be reflected in LeafNode.capabilities.extensions, or else the leaf note is invalid. This PR adds a cautionary note to this effect.

Not blocking these changes but does this indicate duplication amongst LeafNode? It's probably too late to change but

bifurcation commented 1 year ago

@dconnolly - It's a good question, but the duplication is small. The only thing that's duplicated is the 2-byte extension type value. In addition to sending the GREASE extension with type 0xXXXX, you also need to say "I support extensions of type 0xXXXX".

dconnolly commented 1 year ago

@dconnolly - It's a good question, but the duplication is small. The only thing that's duplicated is the 2-byte extension type value. In addition to sending the GREASE extension with type 0xXXXX, you also need to say "I support extensions of type 0xXXXX".

Got it, that makes sense and is as you say small. Thanks!

bifurcation commented 1 year ago

While responding to @dconnolly, though, I got to looking at the relevant text, and suspect we might have yet another minor issue:

Proposal and extension types defined in this document are considered "default" and thus need not be listed [...] Extensions that appear in the extensions field of a LeafNode MUST be included in the extensions field of the capabilities field [...]

That second sentence reads to me like extensions types must appear in capabilities even if they are default types. That seems incorrect to me. Propose clarifying to "Extensions of non-default types that appear in the extensions field..." You would still have to GREASE consistently, but it would save a few bytes for default extension. But maybe adding an exception to the logic isn't worth it. @mulmarta @Bren2010 wdyt?

mulmarta commented 1 year ago

@bifurcation I agree that the clarification you propose reflects what I always understood. Even more clear may be something like "Non-default types of extensions that appear in the extensions field of a LeafNode..." -- this indicates that types, not full extensions are copied. (FWIW the impact is quite small, because the only default leaf node extension is application id.)

Bren2010 commented 1 year ago

I agree it's inconsistent, though I don't have a strong opinion on which way we go. This sentence also stands out:

The capabilities field indicates what protocol versions, ciphersuites, extensions, credential types, and non-default proposal types are supported by a client.

(Extensions isn't covered by "non-default")

kkohbrok commented 1 year ago

The PR looks good to me. I support the solution approach proposed by @mulmarta. The inconsistency pointed out by @Bren2010 we can probably get rid of by rewriting as follows:

The capabilities field indicates what protocol versions, ciphersuites, credential types, and non-default proposal and extension types are supported by a client.

bifurcation commented 1 year ago

Thanks @mulmarta @Bren2010 @kkohbrok. I updated the relevant sentences to clarify, and I also removed the paragraph about GREASE-ing consistently. Under the theory that since GREASE extensions are defined in this document, they are "default", and thus don't have to be listed in capabilities.extensions (though they MAY be listed).

franziskuskiefer commented 1 year ago

Under the theory that since GREASE extensions are defined in this document, they are "default", and thus don't have to be listed in capabilities.extensions (though they MAY be listed).

We ran into this issue interopping between OpenMLS and mlspp because OpenMLS was checking for the GREASE values. But I agree that it is more consistent when GREASE values don't need to be listed as capabilities (since they're "default").

raphaelrobert commented 1 year ago

Under the theory that since GREASE extensions are defined in this document, they are "default", and thus don't have to be listed in capabilities.extensions (though they MAY be listed).

Wouldn't that defeat the purpose of GREASE values? If they are considered "default", it means they need to be hard-coded on the receiving side. Isn't that what should be avoided?

DavidSchinazi commented 1 year ago

(Hi! GREASE enthusiast here) I agree with @raphaelrobert. For GREASE codepoints to work as intended, they need to behave exactly like an unknown optional extension. It would be best to specify that the clause Proposal and extension types defined in this document are considered "default" does not apply to GREASE values.

franziskuskiefer commented 1 year ago

I generally with the GREASE sentiment. But any more exceptions here make this really hard to get right. I'd be in favour of dropping the notion of "default" extensions all together instead.

bifurcation commented 1 year ago

I'm inclined to agree, but it just seems like bloat to have to list, e.g., the ratchet tree extension. I don't think we should consider support for those extensions optional, so they would all appear there. What if instead we were explicit that implementations are REQUIRED to support extension types 0x0001-0x0005, and thus these are considered default?

The following proposal types and extension types MUST be supported by implementations. They are considered "default" and MAY be omitted from the corresponding fields in a Capabilities object.

  • [proposal types 0x0001-0x0007]
  • [extension types 0x0001-0x0005]

... and then we'll need to re-add the "consistent GREASE" paragraph.

bifurcation commented 1 year ago

In the latest commit:

I think this reflects all the feedback received so far, so I will plan to merge in the next 24hr or so unless there are objections.