Split LeafNode from KeyPackage

[bifurcation]

Depends on #573

Fixes #539

This PR implements the suggestion from @TWal in #539, taking into account the discussion on the issue and on the call last week. There are a bunch of small changes (e.g., changing a lot of KeyPackageRef to LeafNodeRef), but the critical change is the following split of KeyPackage into LeafNode and KeyPackage:

struct {
    opaque group_id<0..255>;
    opaque parent_hash<0..255>;
} MembershipInfo;

struct {
    HPKEPublicKey public_key;
    Credential credential;
    Capabilities capabilities;
    optional<MembershipInfo> membership;
    Extension extensions<8..2^32-1>;
    // SignWithLabel(., "LeafNodeTBS", LeafNodeTBS)
    opaque signature<0..2^16-1>;
} LeafNode;

struct {
    ProtocolVersion version;
    CipherSuite cipher_suite;
    HPKEPublicKey init_key;
    Lifetime lifetime;
    LeafNode leaf_node;
    Extension extensions<8..2^32-1>;
    // SignWithLabel(., "KeyPackageTBS", KeyPackageTBS)
    opaque signature<0..2^16-1>;
} KeyPackage;

This does a few things we had discussed:

• KeyPackage is the thing you pre-publish; LeafNode is what goes in the tree
• Sign the LeafNode and the KeyPackage separately, nesting the LeafNode inside the signed KeyPackage information
• Sign the group_id when possible. (This PR takes the chattiest approach, replicating the group_id in each leaf.)
• Promote mandatory extensions to fields

Add continues to refer to KeyPackage. Update and Commit use LeafNode directly.

I did a walk through the document grepping for KeyPackage, and I think I found all the instances that needed to be changed. But some review to make sure I didn't miss anything would be appreciated.

[kkohbrok]

I haven't done a full review yet, but one thing to note is that if a member commits an Update proposal by another member, there should be a group_id, but there can't be a parent_hash. One way to tackle that issue would be to remove the notion of an update proposal entirely such that key material can only be updated via commit. I have no data to back it up, of course, but I doubt that there are going to be many cases where clients will issue update proposals without actually committing them themselves.

[bifurcation]

@kkohbrok I think this is already taken into account:

• A LeafNode sent in an Update proposal MUST have this field set. The
  group_id MUST match the value set for the overall group, and the
  parent_hash field MUST be empty.

Maybe it would be clearer to say parent_hash MUST be the zero-length octet string. Or make group_id and parent_hash two independent optionals (this is a bit messy with the TLS syntax, but doable). I kind of liked the MembershipInfo approach because it gave you a clean signal of whether the LeafNode was a prepublished one or not.

[bifurcation]

After the virtual interim today, I did a little thinking on how to structure these objects to take into account @TWal's point about having the signature be lifetime-constrained in the prepublished case. I think we all agree on which fields should appear in which cases:

Item	KeyPackage	Pre-pub LeafNode	Update LeafNode	Commit LeafNode
version	X
cipher_suite	X
init_key (for Welcome)	X
lifetime	X	X
public_key (for TreeKEM)		X	X	X
credential	X	X	X	X
capabilities	X	X	X	X
group_id			X	X
parent_hash				X
extensions	X	X	X	X

(Where the Credential/Capabilities in KeyPackage might be indirect, via containing a pre-pub LeafNode.) So it's mainly a question of how we lay this out syntactically. A couple of options follow. I have not split the Update and Commit cases; they would differ only in whether the parent_hash field has a value.

Option 1: Variant field within leaf node

struct {
    HPKEPublicKey public_key;
    Credential credential;
    Capabilities capabilities;

    LeafNodeSource leaf_node_source;
    select (leaf_node_source) {
        case pre_published: 
            Lifetime lifetime;
        
        case member:
            opaque parent_hash<0..255>;   // zero-length octet string if update
    }
    
    // SignWithLabel(., "LeafNodeTBS", LeafNodeTBS)
    opaque signature<0..2^16-1>;
} LeafNode;

// LeafNodeTBS adds group_id in the `leaf_node_source = member` case
// KeyPackage wraps LeafNode as in current PR

Option 2: Two types of leaf node

struct {
    ProtocolVersion version;
    CipherSuite cipher_suite;
    HPKEPublicKey init_key;
    Lifetime lifetime;

    HPKEPublicKey public_key;
    Credential credential;
    Capabilities capabilities;

    Extension extensions<8..2^32-1>;
    // SignWithLabel(., "KeyPackageTBS", KeyPackageTBS)
    opaque signature<0..2^16-1>;
} KeyPackage;

struct {
    HPKEPublicKey public_key;
    Credential credential;
    Capabilities capabilities;

    opaque parent_hash<0..255>;
    
    Extension extensions<8..2^32-1>;
    // SignWithLabel(., "LeafNodeContentTBS", LeafNodeContentTBS)
    opaque signature<0..2^16-1>;
} LeafNodeContent;

// KeyPackageTBS just has fields above signature
// LeafNodeContentTBS adds group_id

struct {
    LeafNodeSource leaf_node_source;
    select (leaf_node_type) {
        case pre_published:
            KeyPackage key_package;

        case member:
            LeafNodeContent leaf_node_content;
    }
} LeafNode;

Opinion below this line

Personally, I feel pretty strongly that Option 1 will be easier to work with. Option 2 basically means that there are three types of tree node, and two different algorithms for verifying leaf signatures. (!) That seems like a bigger distinction than is merited here, given that the two types of leaf have mostly the same content.

Having selects here is helpful, though. (Once again select > optional!) In addition to avoiding the chattiness of replicating the group_id everywhere (since now there's a clear signal of when to inject the group_id as context), it provides a simple way to check that a LeafNode has the right contents for a given context in the protocol.

In either case, we will need to be clear about how the lifetime field should be interpreted in a pre-published LeafNode. I got the impression from the call today that there was agreement that the limitation here was purely advisory, i.e., that we would not require that a client reject as invalid a leaf with an expired lifetime. I'm OK with that, but it does seem like it reduces the utility of including the lifetime in the leaf node.

[TWal]

I think I also prefer the first option!
This refactor looks great, it makes things about leaf content a lot more clear. I also like the new consistency between the names LeafNode and ParentNode.

I think we should advise to reject expired LeafNode when adding them to the tree (and also check that they have the same lifetime as the corresponding KeyPackage?)
However when joining a group, we should not reject a tree containing expired leaf nodes (as they might have been added when they were not expired)

[bifurcation]

@TWal - Thanks for the feedback.

With regard to when expired LeafNodes should be rejected -- With the scheme you propose, why does the Lifetime need to be in the LeafNode instead of the KeyPackage? Since the KeyPackage is in the Add, having the lifetime there would still allow it to be validated at add time. And if you're not going to do anything with the lifetime after add time, why do you need it to still be in the tree?

[bifurcation]

The latest commit updates the PR to match Option 1. It still seems awkward to have the Lifetime in the LeafNode.

[kkohbrok]

I think I also prefer the first option.

On lifetimes: Lifetimes are the only thing that allow us to get some degree of PCS for pre-published key material. If Alice is compromised and Bob won't check lifetimes if he is invited into a group that includes Alice, then the adversary can impersonate Alice towards Bob indefinitely after compromise: He creates a group using Alice's KeyPackage and invites Bob.

Of course there are a few caveats to this attack:

• the credential could have its own lifetime
• a higher-level signature key could have been rotated in the meantime, leading to Bob's check with the AS to fail
• the credential could be revoked (with a mandatory stapling scheme that implements a lifetime)
• depending on how the application handles "duplicated" groups, this might or might not work if Bob already has a 1-to-1 group with Alice
  • although the adversary could just include another party in the group, weakening the attack slightly

[raphaelrobert]

Given the two options, I also prefer option 1. That being said, it still bothers me that we now have two signatures instead of one, which doubles the creation time for large groups.

[bifurcation]

@raphaelrobert note that Option 2 does avoid double signature. But in my experience, the slow thing about MLS in large groups is not so much the cryptography (signature or otherwise), but the moving around of the large tree.

[bifurcation]

@kkohbrok - To refine your analysis a bit -- the interesting case here is when the Alice's HPKE private key is compromised, but not her signature key. If her signature key is compromised, then all bets are off; the attacker can impersonate Alice in any way, until the AS invalidates the signature key (e.g., via credential expiry/revocation).

If we take on that constraint, then your attack actually doesn't work. The attacker can't create sign a GroupInfo message that Bob will accept.

The best the attacker could do is create a group with the attacker, Alice, and Bob, such that if Bob removes the attacker, the attacker still has access to the group via knowledge of Alice's HPKE private key (since Bob's UpdatePath will TreeKEM-encrypt to Alice's compromised HPKE key). And here the attacker is pretty limited, since they can't send any MLSPlaintext/MLSCiphertext signatures.

But at that point, it seems like you're in the same posture for pre-published keys as for keys in the group. Any leaf signed by Alice can be used in that attack in the same way.

So it seems like the answer for PCS in any case is "Update/Commit"?

[kkohbrok]

@bifurcation you're right, the attack doesn't quite work the way I thought it would.

It all depends a bit on what the AS does.

• If the app naively checks the signature based on the key package (credential) provided in the Welcome, the attack has a chance. Although I admit that's quite an oversight on part of the AS/app, the lifetime would be useful here and add some robustness.
• Similarly, if the app uses per-group credentials that don't expire, the attack still works. If we had lifetimes, the client could recover by updating all groups and cycling the key packages that were uploaded to the server. This is again assuming the AS doesn't invalidate credentials after a while.

Good point also about the adversary being able to just pluck key packages from any given group at the time of compromise. Those would have to be fitted with lifetimes as well to make this work.

I still like lifetimes as a conservative, defense-in-depth approach in case the AS is very minimal or just not very good. I think as a general rule, it's good to attach an expiration date to key material that is not single-use.

That being said, I understand that due to the asynchronicity in many use-cases it's going to be hard to enforce in a meaningful way that doesn't impede usability too much.

[bifurcation]

Discussion at virtual interim:

• @raphaelrobert points out that a Commit+Add might be processed some time after it is created, so the lifetime might have expired
• @Bren2010 notes that a lifetime in the leaf node at least provides a signal as to how old the leaf is (notBefore being more useful than notAfter here, since it roughly corresponds to creation time)
• TODO(@bifurcation): Implement the following, then merge:
  • Leave lifetime in LeafNode
  • MUST verify lifetime when creating an Add from a prepublished KP
  • SHOULD verify lifetime when receive an Add (modulo time skew between send and receive)
  • SHOULD NOT enforice lifetime when receiving a tree
• It's also possible to put a "last updated epoch" in the leaf alongside the group ID, useful?
  • Doesn't obviously correspond to time, use cases not obvious
  • No action for now