more editorial changes #454

uhoreg · 2021-01-21T23:21:53Z

add the missing authenticated_data field to MLSPlaintextCommitContent
the sections for the Commit sender and Commit receiver seem to have gotten out of sync. I tried to sync them up (but let me know if I messed something up)
fix a typo

Bren2010 · 2021-01-25T18:12:07Z

draft-ietf-mls-protocol.md

-  in the initial GroupContext object.  The `leaf_key_package` for this
-  UpdatePath must have a `parent_hash` extension.
+* If populating the `path` field: Create an UpdatePath using the new tree and
+  the provisional GroupContext. Any new member (from an add proposal) MUST be


You say use the provisional GroupContext, but the original text says to use the original GroupContext

The "A member of the group applies a Commit message by taking the following steps:" section says

If the path value is populated: Process the path value using the new ratchet tree and the provisional GroupContext, to update the ratchet tree and generate the commit_secret:

So this change is to make the two sections consistent

I agree it looks a bit inconsistent here indeed. I think this part is however the correct one and what you quote is confusing and should be changed instead.
The reason for my thinking is that the provisional group context is not known at this point in time and therefore the existing one should be used.

If I'm reading the mlspp sources right, it looks like it uses the provisional GroupContext for this: https://github.com/cisco/mlspp/blob/main/src/state.cpp#L347-L359

The reason for my thinking is that the provisional group context is not known at this point in time and therefore the existing one should be used.

The provisional group context is created two steps before this step.

I personally don't care which one is used, but they should be consistent. For now, I'm going to follow what mlspp does, but if the consensus is that the original text here is correct, then I'm happy to change it.

I think "provisional" is the right answer here. The idea of the GroupContext used with TreeKEM is to capture the tree to which the secrets are encapsulated. That tree is the one in the provisional GroupContext, to which the proposals have been applied.

Bren2010 · 2021-01-25T18:13:27Z

draft-ietf-mls-protocol.md

+* Construct an MLSPlaintext object containing the Commit object. Sign the
+  MLSPlaintext using the current epoch's GroupContext as context. Use the
+  `confirmation_key` for the new epoch to compute the `confirmation_tag` value
+  in the MLSPlaintext.


Again, mixing epochs seems bad

This is how it works in the "A member of the group applies a Commit message by taking the following steps:" section. It verifies the signature before doing any modifications, and uses the confirmation_key from the new epoch.

draft-ietf-mls-protocol.md

raphaelrobert · 2021-03-02T14:34:27Z

draft-ietf-mls-protocol.md

-  in the initial GroupContext object.  The `leaf_key_package` for this
-  UpdatePath must have a `parent_hash` extension.
+* If populating the `path` field: Create an UpdatePath using the new tree and
+  the provisional GroupContext. Any new member (from an add proposal) MUST be


I agree it looks a bit inconsistent here indeed. I think this part is however the correct one and what you quote is confusing and should be changed instead.
The reason for my thinking is that the provisional group context is not known at this point in time and therefore the existing one should be used.

raphaelrobert · 2021-03-02T14:44:04Z

draft-ietf-mls-protocol.md

-  the `confirmation_tag` value in the MLSPlaintext.
+* Use the `commit_secret`, the `psk_secret`, the provisional GroupContext, and
+  the init secret from the previous epoch to compute the epoch secret and
+  derived secrets for the new epoch.


This paragraph explains how the key schedule should be computed, but it omits quite a few steps (joiner secret, welcome secret, etc.) and the values are in the wrong chronologic order.

This was basically a copy-and-paste from the other section. I can update both sections to be more accurate.

I think I've addressed this comment

raphaelrobert · 2021-03-02T14:47:53Z

draft-ietf-mls-protocol.md

-* If the `path` value is populated: Process the `path` value using the ratchet
-  tree the provisional GroupContext, to update the ratchet tree and generate the
+* If the `path` value is populated: Process the `path` value using the new ratchet
+  tree and the provisional GroupContext, to update the ratchet tree and generate the


See comment from above. I think this part is indeed wrong and the broken syntax might be a hint that it's the result of a copy & pasta job gone wrong.

This should be "provisional" in both cases -- the tree after applying proposals but before applying the UpdatePath.

Uhm, yes. Obviously it can't be the "new" ratchet tree, since this is the step that creates the "new" ratchet tree. Not sure why I put "new" there.

Oh, it's because we've only defined "old", "provisional", and "new" for GroupContext, and not for the ratchet tree. I'll adjust the wording to fix that up.

uhoreg · 2021-03-26T22:32:28Z

draft-ietf-mls-protocol.md

+* Use the `init_secret` from the previous epoch, the `commit_secret` and the
+  `psk_secret` as defined in the previous steps, and the new GroupContext to
+  compute the new `joiner_secret`, `welcome_secret`, `epoch_secret`, and
+  derived secrets for the new epoch.


Alternatively, I can say "Use the ... and the new GroupContext to advance the key schedule." if we want to be less verbose.

uhoreg · 2021-03-26T22:34:45Z

draft-ietf-mls-protocol.md


-* Update the tree in the provisional state by applying the direct path


It looks like DirectPath was renamed to UpdatePath, and the UpdatePath is already applied in a previous step (as part of the step where it's actually created), so this looks to me like an obsolete leftover.

uhoreg · 2021-03-26T22:35:50Z

draft-ietf-mls-protocol.md

+  MLSPlaintext using the old GroupContext as context. Use the
+  `confirmation_key` for the new epoch to compute the `confirmation_tag` value,
+  and the `membership_key` for the old epoch to compute the `membership_tag`
+  value in the MLSPlaintext.


I'm not entirely sure why it takes the confirmation_key from the new epoch, but the membership_key from the old epoch, but that's what the current mlspp code does.

Oh, actually, I do know, I think. The confirmation_tag allows the other group members to check that they derived the same tree/secrets as the sender. The membership_tag allows other group members to ensure that it was sent by a group member before processing the message. I think this is mentioned in other parts of the document, but I can reiterate here if necessary.

uhoreg · 2021-03-26T22:37:38Z

I've tried to align with the current mlspp code for how to process commits. If the current mlspp code is incorrect, or if I'm reading it incorrectly, I'm more than happy to fix my changes to reflect the correct behaviour.

seanturner · 2021-05-26T15:38:25Z

@bifurcation @raphaelrobert took action item to review editorial changes.

draft-ietf-mls-protocol.md

bifurcation · 2021-08-10T23:43:32Z

draft-ietf-mls-protocol.md

-  in the initial GroupContext object.  The `leaf_key_package` for this
-  UpdatePath must have a `parent_hash` extension.
+* If populating the `path` field: Create an UpdatePath using the new tree and
+  the provisional GroupContext. Any new member (from an add proposal) MUST be


I think "provisional" is the right answer here. The idea of the GroupContext used with TreeKEM is to capture the tree to which the secrets are encapsulated. That tree is the one in the provisional GroupContext, to which the proposals have been applied.

bifurcation · 2021-08-10T23:46:31Z

draft-ietf-mls-protocol.md

-* If the `path` value is populated: Process the `path` value using the ratchet
-  tree the provisional GroupContext, to update the ratchet tree and generate the
+* If the `path` value is populated: Process the `path` value using the new ratchet
+  tree and the provisional GroupContext, to update the ratchet tree and generate the


This should be "provisional" in both cases -- the tree after applying proposals but before applying the UpdatePath.

Co-authored-by: Richard Barnes <rlb@ipv.sx>

and construct the new GroupContext in the right place

uhoreg · 2021-08-12T03:49:16Z

I think I've addressed all the comments. I also realized that the calculation of the new GroupContext was in the wrong spot (it has to be done after the MLSPlaintext is created (sans confirmation_tag), and then the confirmed transcript hash is created, then the new GroupContext, then the secrets, then the confirmation tag -- that's what mlspp does AFAICT), so I've fixed that up as well.

bifurcation · 2021-08-19T21:07:36Z

@raphaelrobert I think if you're OK with this, we're clear to merge.

raphaelrobert

One more comment about the 3 ratcheting trees

raphaelrobert · 2021-10-01T15:13:54Z

draft-ietf-mls-protocol.md

+   GroupContext uses the epoch number for the new epoch, and the old confirmed
+   transcript hash.  This is used when creating the UpdatePath, if the
+   UpdatePath is needed.
+3. "New" refers to the ratchet tree and GroupContext constructed after applying


I'm still struggling with the fact that there are 3 ratcheting trees. In my mind there are only 2:

The ratcheting tree before applying proposals and the (potential) UpdatePath

The ratcheting tree after applying proposals and the (potential) UpdatePath

The ratcheting trees described here as "provisional" and "new" are in fact identical. It's just that they exist at different times. I think it would be good to make that clear, i.e. a "provisional" ratcheting tree becomes a "new" ratcheting tree when a Commit is applied.

The "provisional" ratchet tree is the tree after applying the proposals, but before applying the UpdatePath. We could probably do without a "provisional" ratchet tree, but I think it helps with clarity to give it a name since applying the proposals and applying the UpdatePath are different steps.

Discussed at 20211004 MLS interim.

Fine, that makes sense to me!

seanturner · 2021-10-04T14:18:43Z

As discussed at interim. @raphaelrobert review. If good, then merge.

raphaelrobert

Ready to be merged from my side.

uhoreg added 3 commits January 21, 2021 17:46

add missing field

9baa0af

make the Commit creator and Commit receiver sections agree more

94d4558

remove an extra word

1b4b1c6

Bren2010 reviewed Jan 25, 2021

View reviewed changes

raphaelrobert requested changes Mar 2, 2021

View reviewed changes

uhoreg added 2 commits March 24, 2021 19:02

Merge branch 'master' into editorial-changes-2021-01-21

c7319e5

sync with mlspp

93ff197

uhoreg commented Mar 26, 2021

View reviewed changes

Merge branch 'main' into editorial-changes-2021-01-21

2f4e8a6

bifurcation reviewed Aug 10, 2021

View reviewed changes

uhoreg and others added 2 commits August 11, 2021 14:48

Update draft-ietf-mls-protocol.md

03a7617

Co-authored-by: Richard Barnes <rlb@ipv.sx>

use old/provisional/new to refer to ratchet trees

a352fa1

and construct the new GroupContext in the right place

bifurcation approved these changes Aug 19, 2021

View reviewed changes

raphaelrobert requested changes Oct 1, 2021

View reviewed changes

raphaelrobert approved these changes Oct 4, 2021

View reviewed changes

raphaelrobert merged commit cb45ce3 into mlswg:main Oct 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more editorial changes #454

more editorial changes #454

uhoreg commented Jan 21, 2021

Bren2010 Jan 25, 2021

uhoreg Jan 25, 2021

raphaelrobert Mar 2, 2021

uhoreg Mar 26, 2021

bifurcation Aug 10, 2021

Bren2010 Jan 25, 2021

uhoreg Jan 25, 2021

raphaelrobert Mar 2, 2021

raphaelrobert Mar 2, 2021

uhoreg Mar 26, 2021

uhoreg Aug 11, 2021

raphaelrobert Mar 2, 2021

bifurcation Aug 10, 2021

uhoreg Aug 11, 2021

uhoreg Aug 11, 2021

uhoreg Mar 26, 2021

uhoreg Mar 26, 2021

uhoreg Mar 26, 2021

uhoreg Mar 27, 2021

uhoreg commented Mar 26, 2021

seanturner commented May 26, 2021

bifurcation Aug 10, 2021

bifurcation Aug 10, 2021

uhoreg commented Aug 12, 2021

bifurcation commented Aug 19, 2021

raphaelrobert left a comment

raphaelrobert Oct 1, 2021

uhoreg Oct 4, 2021

seanturner Oct 4, 2021

raphaelrobert Oct 4, 2021

seanturner commented Oct 4, 2021

raphaelrobert left a comment


		* Update the tree in the provisional state by applying the direct path

more editorial changes #454

more editorial changes #454

Conversation

uhoreg commented Jan 21, 2021

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

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

uhoreg commented Mar 26, 2021

seanturner commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uhoreg commented Aug 12, 2021

bifurcation commented Aug 19, 2021

raphaelrobert 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

seanturner commented Oct 4, 2021

raphaelrobert left a comment

Choose a reason for hiding this comment