Re: [MLS] Two poorly defined aspects of the spec

[Richard Barnes <[e-mail]>]

I filed #388 to fix (1) and (1a).

https://github.com/mlswg/mls-protocol/pull/388

Not sure what to do with your observations about Welcome.  If you have
thoughts about how to clarify, please send a PR!

--Richard


On Tue, Aug 11, 2020 at 9:47 AM Cornelissen Eric <[e-mail]>
wrote:

> Hi Richard,
>
>
> 1. In that case I would agree that clarifying the indices is the way to
> go. I personally think just adding *interim_[0] = 0* to the current
> definition would suffice and be clearer than your suggestion here, but
> that's just personal taste.
>
>
> 2. The "secrets" array makes a ton of sense.
>
>
> If the welcome is ignored when the verification fails, then the fact that
> this is not strictly true seems to make it impossible to continue with a
> group after all but one person left (unfortunately, the paragraph doesn't
> specify what should happen if the verification fails). This is not a
> cryptographic problem, but I'd imagine this being confusing for end users
> *if* the application layer doesn't deal with it properly.
>
>
> However, as I just pointed out in #385
> <https://github.com/mlswg/mls-protocol/pull/385>, it seems that we (I)
> made it impossible for joiners to verify group creation at all.
>
>
>
> Regards,
>
> Eric
>
> ------------------------------
> *From:* Richard Barnes <[e-mail]>
> *Sent:* Monday, August 10, 2020 5:22:47 PM
> *To:* Cornelissen Eric
> *Cc:* [e-mail]
> *Subject:* Re: [MLS] Two poorly defined aspects of the spec
>
> Hi Eric,
>
> Thanks for the analysis here.  More good finds.
>
> 1. I think the right answer is a little of both.  If you initialize a
> one-member group, before any Commit messages have been sent, then it should
> be the zero-length octet string.  But then as soon as the group is used for
> something, it is updated.  Maybe the way to clarify this is to fix the
> indices.  If we have the interim hash belong to the new epoch, then we
> could have something like the following:
>
> interim_[0] = 0
> confirmed_[n] = Hash(interim_[n] || MLSPlaintextCommitContent_[n]);
> interim_[n+1] = Hash(confirmed_[n] || MLSPlaintextCommitAuthData_[n]);
>
>
> 1.a. BTW, this raises another issue: I had thought we had removed direct
> dependencies on hash functions when we move from invoking HKDF to invoking
> the KDF from HPKE.  But this points out that we still have raw hash
> invocations.  I filed an issue for this:
>
> https://github.com/mlswg/mls-protocol/issues/386
>
> FWIW, I think the latter course there is the right answer, i.e., to keep
> the raw hash invocations and clarify that the ciphersuite specifies a hash
> function.
>
>
> 2. It seems like the right reference here is probably to the "secrets"
> array in the Welcome message.  There's one entry there for each member
> being added, so if len(secrets) = len(group_info.tree.leaves) - 1, then
> there was only one member in the group to start with.  It's not strictly
> true to say that this is a new creation, since one commit could remove all
> but one of the members of the group and re-add a new set of members.  But
> this distinction doesn't really seem salient.
>
>
>
> On Thu, Aug 6, 2020 at 6:02 AM Cornelissen Eric <[e-mail]>
> wrote:
>
>> Hi All,
>>
>>
>> I have two questions about things that are unclear to me in the current
>> state of the spec as they are undefined/poorly defined.
>>
>>
>>
>> 1. What is the intended value of *interim_transcript_hash_**[0]*?
>>
>> From the current draft I cannot definitively (i.e. with 100% certainty)
>> say whether the value of the first *interim_transcript_hash *should be:
>>
>> a. "the zero-length octet string" following the last line of Group state
>> section
>> <https://github.com/mlswg/mls-protocol/blob/39455d2ea5e8fb42e8f0f0624bddd8c56675da0e/draft-ietf-mls-protocol.md#group-state>
>> <https://github.com/mlswg/mls-protocol/blob/master/draft-ietf-mls-protocol.md#group-state>,
>> or
>> b. a value derived based on the MLSPlaintextCommitAutData for the first
>> commit concatenated with 0:
>>     interim_transcript_hash_[0]
>>         = Hash(confirmed_transcript_hash_[0] ||
>> MLSPlaintextCommitAuthData_[0])
>>         = Hash(0 || MLSPlaintextCommitAuthData_[0])
>>
>> The draft strongly suggests to me that it is the former, but the latter
>> seems to more accurately reflect the statement "The
>> *confirmed_transcript_hash* field contains a running hash over the
>> messages that led to this state."
>>
>> In the case that *a* is correct I would suggest updating the definition
>> of *interim_transcript_hash_**[n]* to:
>>
>>     interim_transcript_hash_[0] = 0
>>     interim_transcript_hash_[n] =
>>         Hash(confirmed_transcript_hash_[n] ||
>>             MLSPlaintextCommitAuthData_[n]);
>>
>> In the case that *b* is correct I would suggest adding a "warning" that
>> says something along the lines of "*as a placeholder value" *to the
>> place(s) where the draft says that the *interim_transcript_hash* of a
>> group is initialized to 0.
>>
>>
>> 2. What is the `members` array mentioned in the Group creation section
>> <https://github.com/mlswg/mls-protocol/blob/39455d2ea5e8fb42e8f0f0624bddd8c56675da0e/draft-ietf-mls-protocol.md#group-creation>
>> ?
>>
>> In that section it is stated that "A new member receiving a Welcome
>> message can recognize group creation if the number of entries in the
>> `members` array is equal to the number of leaves in the tree minus one."
>> First of all, it is not clear what this `members` array is. It seems this
>> refers to a field of the now-removed Init struct. Interestingly this struct
>> was removed in the very same PR that added the above sentence (see #239
>> <https://github.com/mlswg/mls-protocol/pull/239>). But I wasn't able to
>> really figure out what the draft-09 equivalent of it should be.
>>
>> In addition, I'm pretty sure this technique would no longer work to
>> detect group creation (but that would depend on how the sentence should be
>> updated).
>>
>>
>> Regards,
>> Eric
>> _______________________________________________
>> MLS mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/mls
>>
>