Subject: [MLS] PSK Injection, Group recovery, Re-Init, Sub-group Branching (#336)

From: Richard Barnes <[email-masked]>
Date: Wed, 17 Jun 2020 18:54:16 -0400

Hey all,

Sorry for the late review on this PR, I have finally gotten a chance to
really dig into it, and ... I have some concerns.  My current thinking is
consistent with my earlier comments that the general thrust of this PR is
OK.  But I think the actual approach needs to change a fair bit.

As its title implies, this PR is trying to do several things at once, which
seem to me to be fairly separable:

0. Update the key schedule so that PSKs are injected at the right point and
multiple PSKs are supported
1. Signal that a PSK should get injected into a given epoch
2. Request injection of PSK at join / in Welcome
3. Re-initialize the group with different parameters
4. Re-sync group members that have lost state

For the most part, I think these are worthwhile goals, but they should be
accomplished in different ways than the PR proposes.  Detailed thoughts by
objective:

# 0. Update the key schedule so that PSKs are injected at the right point
and multiple PSKs are supported

This part of the PR is fine, but I think is ultimately done a bit more
elegantly with the n-PRF stuff in #337.

# 1. Signal that a PSK should get injected into a given epoch
# 2. Request injection of PSK at join / in Welcome

(These go together)

The PR proposes (heh) an EPSK proposal or an Add proposal, but then
requires that only one EPSK be used.  I actually don't the proposal for
including PSKs in Adds actually works.  The joiner would have to know all
of them plus the EPSK, and there's not even any syntax in the PR to tell
the joiner which ones to include.

I would propose that instead we add an extensions field to the Commit
message, then add a Commit extension that specifies the PSK ID for the PSK
to be added.  That extension can then also go in the Welcome message
corresponding to the commit, so that joiners know to add the PSK.

This change actually doesn't make a difference in terms of what ends up in
the transcript -- either way, only the PSK ID selected by the committer
goes in the transcript.  It seems better not to deliberately have proposals
floating around that never get included in the transcript.

# 3. Re-initialize the group with different parameters

The PR proposes another proposal type for reinitialization, but it's really
under-specified what you do to re-init.  For example, what goes in the
ratchet tree?  Clearly you need a brand new tree, since the one you have
might be tied to the wrong ciphersuite.  What you really need is something
like a Welcome message.

And that's how I think we should actually arrange things.  Instead of
having the parent group do something to spawn, have the initiator of the
child send a Welcome that has a pointer back to the parent.  So we have
another Welcome extension (`parent_group`, say) that has a pointer to the
parent group by group ID and epoch, and instructs the joiner to take a
secret from that group/epoch's key schedule and add it into the new group's
key schedule.

# 4. Re-sync group members that have lost state

We should not do these.  At the January interim, we decided not to drop the
resync issue (#93) given that there were some subtleties depending on
exactly what state had been lost, and that it could be cleanly done in a
separate spec.

# Conclusions

Given the above, I would propose we break this into separate issues to be
closed with individual, smaller PRs.  Assuming that (0) will get addressed
by the n-PRF PR, that leaves us with:

#XXX Add Extensions to the Commit message
#XXX Signal use of external PSKs in Commit/Welcome (=> extension to
Commit/Welcome messages)
#XXX Allow Welcome to signal the inclusion of information from a prior
group (=> Welcome extension)

Happy to write those up, and contribute to PRs.  Sorry again for the slow
progress here.

--Richard

---

From: "Hale, Britta (CIV)" <[email-masked]>
Date: Thu, 18 Jun 2020 01:07:35 +0000

Hi Richard,

Thank you for the comments and careful read of the PR.

For #1/#2: I am amicable to the idea of adding an extension field to the commit message. As a general comment, we may want to consider if there are other types of commit extensions worth adding or, alternatively, how the field may be abused (e.g. treated as AD by an application).
It terms of objection to the current form though, your argument is not clear. PSKIds should uniquely identify PSKs, and a joiner (or anyone) should be able to identify the correct PSK given the PSKId, if they are in possession of it. PSKs must only be used in an Add if the joiner is a past group member (line 2055) and therefore has the correct PSKId/group_id/psk_epoch, although this is nullified if we make the change discussed in #4. In that case, the issue for Adds goes away.
The PR already allows for a PSK in the Welcome message.

For #3: Currently, in the PR, there is a recovery_secret derived per-epoch. When a member wants to re-initialize, they issue a Re-Init proposal. They may then re-initialize the group by sending a Welcome message (see lines 1923, 1948, and 2349) with the PSKId for the parent group/epoch and a psktype that indicates that they are re-initializing the group with the stated PSKId. So this seems to already be what you describe below as “have the initiator of the child send a Welcome that has a pointer back to the parent”. However, you are right that more detail can be added; in particular a requirement should be added to terminate the parent group and that the terminating member/re-init proposer must also create the new group.

For #4: There was a discussion at the first virtual interim in late April (Interim #11-2020) wherein some expressed a desire for this feature. Namely, instead of re-syncing the entire group, there were use cases for re-syncing the odd member when necessary. Its inclusion in the PR arises from that later request. I am favorable to removing it since, from security standpoint, it is not ideal. However, the motivation for this was pushed from the implementation standpoint. Consequently, now is a good time for anyone to speak up who wants to push for this.

One item: you made point of specifically requesting – a few times – at the virtual interims #11 and #12 in April/May that this PSK PR and as many other key schedule-related PRs be combined into a single one. Raphael made a similar request under git issue #325. In fact, the authors of this PR had pushed back on that insistence to limit the current PR down to the items it currently covers, instead of merging in other key schedule changes as well. Are you saying that you have changed your mind?
We can, of course, accommodate a revised preference if needed.

Britta

From: MLS <[email-masked]> on behalf of Richard Barnes <[email-masked]>
Date: Wednesday, June 17, 2020 at 3:54 PM
To: Messaging Layer Security WG <[email-masked]>
Subject: [MLS] PSK Injection, Group recovery, Re-Init, Sub-group Branching (#336)

Hey all,

Sorry for the late review on this PR, I have finally gotten a chance to really dig into it, and ... I have some concerns.  My current thinking is consistent with my earlier comments that the general thrust of this PR is OK.  But I think the actual approach needs to change a fair bit.

As its title implies, this PR is trying to do several things at once, which seem to me to be fairly separable:

0. Update the key schedule so that PSKs are injected at the right point and multiple PSKs are supported

For the most part, I think these are worthwhile goals, but they should be accomplished in different ways than the PR proposes.  Detailed thoughts by objective:

# 0. Update the key schedule so that PSKs are injected at the right point and multiple PSKs are supported

This part of the PR is fine, but I think is ultimately done a bit more elegantly with the n-PRF stuff in #337.

(These go together)

The PR proposes (heh) an EPSK proposal or an Add proposal, but then requires that only one EPSK be used.  I actually don't the proposal for including PSKs in Adds actually works.  The joiner would have to know all of them plus the EPSK, and there's not even any syntax in the PR to tell the joiner which ones to include.

I would propose that instead we add an extensions field to the Commit message, then add a Commit extension that specifies the PSK ID for the PSK to be added.  That extension can then also go in the Welcome message corresponding to the commit, so that joiners know to add the PSK.

This change actually doesn't make a difference in terms of what ends up in the transcript -- either way, only the PSK ID selected by the committer goes in the transcript.  It seems better not to deliberately have proposals floating around that never get included in the transcript.

The PR proposes another proposal type for reinitialization, but it's really under-specified what you do to re-init.  For example, what goes in the ratchet tree?  Clearly you need a brand new tree, since the one you have might be tied to the wrong ciphersuite.  What you really need is something like a Welcome message.

And that's how I think we should actually arrange things.  Instead of having the parent group do something to spawn, have the initiator of the child send a Welcome that has a pointer back to the parent.  So we have another Welcome extension (`parent_group`, say) that has a pointer to the parent group by group ID and epoch, and instructs the joiner to take a secret from that group/epoch's key schedule and add it into the new group's key schedule.

We should not do these.  At the January interim, we decided not to drop the resync issue (#93) given that there were some subtleties depending on exactly what state had been lost, and that it could be cleanly done in a separate spec.

# Conclusions

Given the above, I would propose we break this into separate issues to be closed with individual, smaller PRs.  Assuming that (0) will get addressed by the n-PRF PR, that leaves us with:

#XXX Add Extensions to the Commit message
#XXX Signal use of external PSKs in Commit/Welcome (=> extension to Commit/Welcome messages)
#XXX Allow Welcome to signal the inclusion of information from a prior group (=> Welcome extension)

Happy to write those up, and contribute to PRs.  Sorry again for the slow progress here.

--Richard

---

From: Konrad Kohbrok <[email-masked]>
Date: Thu, 18 Jun 2020 17:29:25 +0300

Hi,

Just a small addition to Britta's response:

For #3: We already mandate that the parent group must not be used anymore if a
Re-Init proposal is included in a commit in lines 2341-2342 of the PR (although
"to send messages" might as well be removed). We could also get rid of the
proposal and simply have the intent to re-initialize be implicit in sending a
Welcome message with a corresponding PSK Id. The idea behind the Proposal was to
make that intent explicit such that the intent, as well as the target
ciphersuite and MLS version are all entered into the transcript and thus agreed
upon. Even though the group is terminated after the proposal is received, this
agreement can be verified by using the authentication key technique that Britta
proposed, i.e. each party can check that their authentication key corresponds to
the one the committer uploaded.

I'm also ok with splitting it into multiple individual PRs if that makes it
easier to review and discuss.

Cheers,
Konrad

On 18.06.20 04:07, Hale, Britta (CIV) wrote:
> For #1/#2: I am amicable to the idea of adding an extension field to the commit
> message. As a general comment, we may want to consider if there are other types
> of commit extensions worth adding or, alternatively, how the field may be abused
> (e.g. treated as AD by an application).
> 
> It terms of objection to the current form though, your argument is not clear.
> PSKIds should uniquely identify PSKs, and a joiner (or anyone) should be able to
> identify the correct PSK given the PSKId, if they are in possession of it. PSKs
> must only be used in an Add if the joiner is a past group member (line 2055) and
> therefore has the correct PSKId/group_id/psk_epoch, although this is nullified
> if we make the change discussed in #4. In that case, the issue for Adds goes away.
> 
> For #3: Currently, in the PR, there is a recovery_secret derived per-epoch. When
> a member wants to re-initialize, they issue a Re-Init proposal. They may then
> re-initialize the group by sending a Welcome message (see lines 1923, 1948, and
> 2349) with the PSKId for the parent group/epoch and a psktype that indicates
> that they are re-initializing the group with the stated PSKId. So this seems to
> already be what you describe below as “have the initiator of the child send a
> Welcome that has a pointer back to the parent”. However, you are right that more
> detail can be added; in particular a requirement should be added to terminate
> the parent group and that the terminating member/re-init proposer must also
> create the new group.
> 
>  
> 
> For #4: There was a discussion at the first virtual interim in late April
> (Interim #11-2020) wherein some expressed a desire for this feature. Namely,
> instead of re-syncing the entire group, there were use cases for re-syncing the
> odd member when necessary. Its inclusion in the PR arises from that later
> request. I am favorable to removing it since, from security standpoint, it is
> not ideal. However, the motivation for this was pushed from the implementation
> standpoint. Consequently, now is a good time for anyone to speak up who wants to
> push for this.
> 
>  
> 
>  
> 
> One item: you made point of specifically requesting – a few times – at the
> virtual interims #11 and #12 in April/May that this PSK PR and as many other key
> schedule-related PRs be combined into a single one. Raphael made a similar
> request under git issue #325. In fact, the authors of this PR had pushed back on
> that insistence to limit the current PR down to the items it currently covers,
> instead of merging in other key schedule changes as well. Are you saying that
> you have changed your mind?
> 
> *From: *MLS <[email-masked]> on behalf of Richard Barnes <[email-masked]>
> *Date: *Wednesday, June 17, 2020 at 3:54 PM
> *To: *Messaging Layer Security WG <[email-masked]>
> *Subject: *[MLS] PSK Injection, Group recovery, Re-Init, Sub-group Branching (#336)
> 
>  
> 
> Hey all,
> 
> Sorry for the late review on this PR, I have finally gotten a chance to really
> dig into it, and ... I have some concerns.  My current thinking is consistent
> with my earlier comments that the general thrust of this PR is OK.  But I think
> the actual approach needs to change a fair bit.
> 
> The PR proposes (heh) an EPSK proposal or an Add proposal, but then requires
> that only one EPSK be used.  I actually don't the proposal for including PSKs in
> Adds actually works.  The joiner would have to know all of them plus the EPSK,
> and there's not even any syntax in the PR to tell the joiner which ones to include.
> 
> I would propose that instead we add an extensions field to the Commit message,
> then add a Commit extension that specifies the PSK ID for the PSK to be added. 
> That extension can then also go in the Welcome message corresponding to the
> commit, so that joiners know to add the PSK.
> 
> This change actually doesn't make a difference in terms of what ends up in the
> transcript -- either way, only the PSK ID selected by the committer goes in the
> transcript.  It seems better not to deliberately have proposals floating around
> that never get included in the transcript.
> 
> 
> under-specified what you do to re-init.  For example, what goes in the ratchet
> tree?  Clearly you need a brand new tree, since the one you have might be tied
> to the wrong ciphersuite.  What you really need is something like a Welcome message.
> 
> And that's how I think we should actually arrange things.  Instead of having the
> parent group do something to spawn, have the initiator of the child send a
> Welcome that has a pointer back to the parent.  So we have another Welcome
> extension (`parent_group`, say) that has a pointer to the parent group by group
> ID and epoch, and instructs the joiner to take a secret from that group/epoch's
> key schedule and add it into the new group's key schedule.
> 
> 
> 
> resync issue (#93) given that there were some subtleties depending on exactly
> what state had been lost, and that it could be cleanly done in a separate spec.
> 
> 
> # Conclusions
> 
> Given the above, I would propose we break this into separate issues to be closed
> with individual, smaller PRs.  Assuming that (0) will get addressed by the n-PRF
> PR, that leaves us with:
> 
> _______________________________________________
> MLS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/mls
>