Review from Sofía Celi

[beurdouche]

Dear, list,

Thank you so much for all your efforts into developing MLS.

After re-reading the architecture and protocol drafts, I have some
comments. Please, excuse me if I got anything wrong. Apologies as well
if this have been already discussed.

- While it is clear on the architecture draft that 'Client' refers to a
device that a user has, it is unclear on the protocol draft and might
generate confusion, specifically, by the definition:

"""
Client:
: An agent that uses this protocol to establish shared cryptographic
 state with other clients.  A client is defined by the
 cryptographic keys it holds.
"""

Might be good to clarify here the relationship with devices and users,
as this is something often enforced by messaging protocols, such as XMPP.

- It is unclear if any member can 'Add' another one. In the case of
'Remove', it is clear, as:

"""
Members are removed from the group in a similar way. Any member of the
group can send a _Remove_ proposal followed by a _Commit_ message, which
adds new entropy to the group state that's known to all except for the
removed member.
Note that this does not necessarily imply that any member is actually
allowed to evict other members; groups can enforce access control
policies on top of these basic mechanism.
"""

It might be good to add the same note to the 'Add' mechanism, as some
protocols might enforce only some members to be able to add others (an
admin, for example). I think this is clearer on the architecture draft.

- While there is a deletion schedule section, it should perhaps be
enforced on several sections where deletion is critical: reuse of
KeyPackages, key/nonce pair reuse, etc. It will be good to specially
refer where these values should be safely deleted, even in regars that,
on some occasions, key/nonce pairs, for example, should be kept in
memory for delayed messages.

- It will be good to enforce a time, as part of an structure to which
retention occurs:

"""
For usability, MLS clients might be required to keep the AEAD key
and nonce for a certain amount of time to retain the ability to decrypt
delayed or out of order messages, possibly still in transit while a
decryption is being done.
"""

This time, I think should be enforced by the application but could be
part of an struct and periodically be checked. After the times passes,
the values should be deleted.

- I'm not really sure of this section:

"""
Decryption is done by decrypting the metadata, then the message, and
then verifying the content signature.
"""

Wouldn't it be better to first verify the signature and then decrypt the
message? I can imagine that, as it is written, an application can show
the messages to the end users without verifying the identity (and I
assume integrity, as well).

- What happens if a proposal or message is invalid?

On these sections:

"""
An invited 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.  A client receiving a _Welcome_
message SHOULD verify whether it is a newly created group, and if so,
SHOULD verify that the above process was followed by reconstructing the
_Add_ and _Commit_ messages, and verifying that the resulting transcript
hashes and epoch secret match those found in the Welcome message.

and

The sender of a _Commit_ MUST include all valid proposals that it has
received during the current epoch. Invalid proposals include, for
example, proposals with an invalid signature or proposals that are
semantically invalid, such as an _Add_ when the sender does not have the
application-level permission to add new users.
"""

What will be the correct behaviour when failure? Should the messages be
ignored or ask to be resent? I assumed ignored; but might be worth noting.


- Will it be better to use a MAC key?
On the section:

"""
To prevent counter manipulation by the server, the counter's integrity
can be ensured by including the counter in a signed message envelope.
"""

Might be better to somehow derive a MAC key and attach it to the counter...

- Security level:

On the section:

"""
requirements, clients can choose between two security levels (roughly
128-bit and 256-bit). Within the security levels clients can choose
between faster
"""

As I see that ed448 is used, it is worth noting that is security level
is of 224-bit, as stated over: https://tools.ietf.org/html/rfc8032#section-1

I'll keep on reviewing some other parts in the future in more detail.

I submitted two pull request with some grammar fixes as well and can put
these thoughts on issues on the repository.

Thanks!

[beurdouche]

The Arch specific parts have been handled, modulo the security considerations that are gonna be updated as we discuss as a group.