Re: [MLS] Éric Vyncke's No Objection on draft-ietf-mls-protocol-17: (with COMMENT)

Richard Barnes <rlb@ipv.sx> Thu, 02 February 2023 16:46 UTC

Return-Path: <rlb@ipv.sx>
X-Original-To: mls@ietfa.amsl.com
Delivered-To: mls@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 7E39BC1575CC for <mls@ietfa.amsl.com>; Thu, 2 Feb 2023 08:46:34 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.894
X-Spam-Level:
X-Spam-Status: No, score=-1.894 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=ipv-sx.20210112.gappssmtp.com
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id rZKvGfIBvpKk for <mls@ietfa.amsl.com>; Thu, 2 Feb 2023 08:46:30 -0800 (PST)
Received: from mail-wm1-x334.google.com (mail-wm1-x334.google.com [IPv6:2a00:1450:4864:20::334]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 5372BC14EB19 for <mls@ietf.org>; Thu, 2 Feb 2023 08:46:29 -0800 (PST)
Received: by mail-wm1-x334.google.com with SMTP id d4-20020a05600c3ac400b003db1de2aef0so1904312wms.2 for <mls@ietf.org>; Thu, 02 Feb 2023 08:46:29 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipv-sx.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=jxCSHD/EqARdV9FTBNk1ALZ28G+kkWzmVeAHTfVAMBI=; b=NtB3emme97r3p1fbDYlk9A04iAuTo4roRs7zta7WfptzZzhGyaOfODz1ucZo4OwPSw nx0QwCgifpNipTo8hyGQvn5td+OhlYrksv4kn09L1wOpukkr3RMVjSZdHd+E2rGVim27 9iV7wc3XCzRLaINQwrwi0pZVcdXMgionWbfYzRZBEeTDftmfqDUCp2K+Bhk2JCymBFkx tfjJpPsOsCMtfN06WJC7f0CuXAAb3KbJD5lTZxVAFStmB2JBxBSuPf2OeOGGZW6rgiR0 PoKNve3YzUG+hsr1U5IZPoJldTdwrxwZQoK6SYvHUCYeBalAioXgBfghpwyVicO9H3bW mxnA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=jxCSHD/EqARdV9FTBNk1ALZ28G+kkWzmVeAHTfVAMBI=; b=6lpVj4fiF5iRCGxLA3vrXPWIsNPxnBCqHhxsHUrKlegH70MA+lsDHHREMBcgs0nXnG 9ShXDGmig3NKUg21cnG9pqUyUXf7U4yF02WGuWfqvlfkQh3C68Is1pKsN3pe8kvu6gNu lqzdX+/SmvZKvZJ1drBTbyPLBB3VxupcWssEpuw0f3r7F/5SJbFgdoGf3rLsHDrdn3Gi uBOWsaI49O6FYFUydDoRrfP5J20Ib/RqTvzgkXvjPJx3bgd2QhXJxbZmOCS4Ols++OAG y0ZpV0nq7bkxmqCTYMewuEKw1/fOSmSI3Qp9vm7p3YIW9bz6QDJVossofhNsNmTebeyp M+rw==
X-Gm-Message-State: AO0yUKWFD4qyso4jxV5s+0EG02KeZVUyTFDhrE24VTD1P0b8IvAFkAUe rBwySUmz9GbsdiqBz2G+nD5eCRosZBpQnKLGD8WEtA==
X-Google-Smtp-Source: AK7set9p9yn11AFxXIFRDFZMk5zAxV37V9PhgMriSoYL/Pw7etLqc3Msv/+Y72hc0/EsAUPtYVx17OOoPTGhwiOlJ54=
X-Received: by 2002:a05:600c:1e0a:b0:3dc:5300:90ab with SMTP id ay10-20020a05600c1e0a00b003dc530090abmr265263wmb.16.1675356387060; Thu, 02 Feb 2023 08:46:27 -0800 (PST)
MIME-Version: 1.0
References: <167532900162.58055.17525341252308658581@ietfa.amsl.com>
In-Reply-To: <167532900162.58055.17525341252308658581@ietfa.amsl.com>
From: Richard Barnes <rlb@ipv.sx>
Date: Thu, 02 Feb 2023 11:46:15 -0500
Message-ID: <CAL02cgQ3J_oOwrp0Q44982gQFqdKyGFTkuqXSCcxEAew=T_9hw@mail.gmail.com>
To: Éric Vyncke <evyncke@cisco.com>
Cc: The IESG <iesg@ietf.org>, mls@ietf.org
Content-Type: multipart/alternative; boundary="00000000000052bbbe05f3ba4e92"
Archived-At: <https://mailarchive.ietf.org/arch/msg/mls/ih4BsY1V-w7HgBZ_vB8iuMb4vGA>
Subject: Re: [MLS] Éric Vyncke's No Objection on draft-ietf-mls-protocol-17: (with COMMENT)
X-BeenThere: mls@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Messaging Layer Security <mls.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/mls>, <mailto:mls-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mls/>
List-Post: <mailto:mls@ietf.org>
List-Help: <mailto:mls-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/mls>, <mailto:mls-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 02 Feb 2023 16:46:34 -0000

Hi Éric,

Thanks for the review.  Some comments inline below.  I have filed the
following PR:

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

On Thu, Feb 2, 2023 at 4:10 AM Éric Vyncke via Datatracker <noreply@ietf.org>
wrote:

> ### Intended Status
>
> I second Lars' point about the intended status. As the IETF Last Call
> clearly
> indicated 'proposed standard', a revised I-D will be enough IMHO.
>

Yep, this is covered by the PR for Lars' comments:
https://github.com/mlswg/mls-protocol/pull/848


> ### Implementation of complex protocol
>
> Based on my affiliation, I obviously know about one implementation (unsure
> whether it is a project or it is deployed). It would be nice to know
> whether
> there are other implementations of this protocol, which looks quite
> complex to
> implement.
>

There are two services running draft versions of MLS in production, Webex
and RingCentral.  Wire, Wickr, and Matrix have all been active contributors
to the protocol, so one might presume they are looking at using it.  We are
actively working on interop testing among five actively-developed
implementations (#mls-interop on ietf.slack.com in case anyone is
interested).

Some links in case you're interested:

List of implementations:
https://github.com/mlswg/mls-implementations/blob/main/implementation_list.md
Matrix's scalability testing: http://arewemlsyet.com/
Webex whitepaper on MLS-based E2E encryption:
https://www.cisco.com/c/en/us/solutions/collateral/collaboration/white-paper-c11-744553.html



> ### Ratchet
>
> As the whole concept of MLS relies on "ratchet trees", an early short
> introduction to them would have made reading easier. The reader has to wait
> until section 4 to get some explanations.
>

Section 4 is the section immediately following the protocol overview, it's
not that far away!  Section 3.1 provides a high-level description of a
ratchet tree, "A _ratchet tree_ that represents the membership of the
group, providing group members a way to authenticate each other and
efficiently encrypt messages to subsets of the group."   I think that's the
right level for the protocol overview, so I would propose keeping this
as-is.



> ### Section 1.1
>
> Suggestion: swap the introduction of AD and DS as it sounds more logical
> timewise.
>

Well, you can't authenticate a credential with the AS until you receive it
from the DS, but OK :)


> For DS, at this point in the document, readers can only guess what
> `KeyPackage`
> and `Proposal` are. Suggest not to use the capitalised words but simply
> their
> lowercase equivalent.
>

I added a note that these terms are defined below.


> ### Section 2.1.2
>
> In `encoded on the remaining bits, in network byte order` I find weird to
> use
> *byte* order for *bits*. Should it rather be *MSB first* ? Or even
> suppress it
> as all other bit encoding in IETF document implicitly used this notation ?
>

Adjusted this to say that the overall value should be in network byte order.


> Does this compressed encoding add value in a world with long crypto
> material ?
> (just curious here)
>

The value is partly compactness, and partly that you don't have to track
what size header you're supposed to use for which vector (you just always
use a varint).  We added this after we found a lot of bugs of the latter
form in earlier rounds of interop testing.


`up to 2^30 bytes` is about 1 Gbytes, this is huge and for sure will not
> often
> be used (written by the guy supporting 128-bit addresses). Feel free to
> ignore
> this comment of course ;-)
>

It's not an outlandish worry.  The largest objects in MLS in practice today
are Welcome messages that include the ratchet tree, because each leaf of
the tree carries a certificate chain.  In Webex today, this means the tree
is on the order of a few KB per client, which brings you to the 1GB bound
at a few hundred thousand clients, the same order of magnitude as the
scaling bounds Matrix is looking to support.

So we have some headroom, but it's not totally impossible that there will
be cases that will hit the top of the 4-byte bound.  If that happens,
though, there are some work-arounds that don't require changing the varint
format, or an extension could activate the 8-byte form.



> ### Section 3.2
>
> Where was "leaf secret" in `Updating the leaf secret of a member` defined ?
>

Thanks, this is a relic of some older terminology.  I have reworded.



> Which is the relationship between the directory/service provider and AS/DS
> ?
>

SP is more old terminology.  The directory is a service provided by the DS,
as discussed in the architecture.  Reworded to clarify.


> As it seems that the group channel is part of DS, then let's make it clear
> in
> the figure ?
>

Fair point.  I added an indication to the first figure.


> `Once A has updated its state, the new member has processed the Welcome,
> and
> any other group members have processed the Commit,` should this rather be
> `Once
> A has updated its state and the new member has processed the Welcome and
> any
> other group members have processed the Commit,` i.e., no Oxford comma as
> the
> "once" applies to all three conditions ? Bear with me as I am not an
> English
> speaker.
>

I think the Oxford comma is called for, but I will let the RFC Editor be
the arbiter on this.



> `a rate that overwhelms the transport` is transport really the only
> bottleneck
> ? I would assume that crypto processing could be another one (but could be
> wrong). While is seems really smart and nice on the 3 parties example, I
> wonder
> how it could scale to thousands of nodes (per -architecture) with frequent
> join/leave by members.
>

It's a fair point, but those sorts of things should be more readily visible
to in testing, whereas congestion might only appear in the wild with other
applications


> ### Section 3.3
>
> Suggest to explicitly refer to figure numbers and not only by "next
> figure" (or
> even nothing in the text pointing to the figure).
>

Fixed.


> ### Section 4
>
> Some justification of the log(N) would be nice for the reader (or an
> external
> reference), does it come from the binary tree ? Does the amount of
> subgroups
> influence the number of crypto operations ?
>

Yep, it's the tree structure.  Basically, you encrypt to the key at the top
of each subtree that does not include the member being excluded.  There are
log(N) of these, and they collectively cover the N-1 remaining
participants.  Added a note.


> ### Section 4.1.1
>
> To be honest, I failed to understand the example because "unmerged" is
> explained later in the text. No suggestion though...
>

Yeah, it's tough to put all this in a linear sequence.  There's a forward
reference, though, so I'm going to leave it as-is.


> ### Section 4.2
>
> s/all the intermediate nodes they're below/all the intermediate nodes that
> are
> below/ ?
>

"all the intermediate nodes above them".



> ### Section 5.1.3
>
> Probably due to my lack of knowledge about "TLS syntax", but I find
> imposing a
> syntax `label = "MLS 1.0 " + Label;` on an *opaque* field a little weird...
>

"Opaque" here just means "vector of bytes".



> ### Section 11
>
> `The creator ... verify that the chosen version and ciphersuite is the best
> option supported by all members.`, but how can the creator do that when
> creating the group ?
>

When creating the group, the creator has a KeyPackage for each other client
that they are adding to the group.  Each KeyPackage has a `capabilities`
field that expresses what parameters the client supports.



> ## NITS
>
> ### Figure numbers
>
> There are a lot of nice and useful figures in the I-D, may I suggest to
> add a
> label/reference to all of them ?
>

I've tried to add titles to all of the `aasvg` figures.  Not the pseudocode
/ TLS syntax.



> ### Section 2
>
> s/shared cryptographic state/shared cryptographic states/ ?
>

They're all holding on to the same literal bytes, so I think singular is
appropriate.


### Section 4.1
>
> Suggest either remove the last paragraph (about implementations) or move it
> after the 1st paragraph of 4.1.1 (where the *index* is introduced).
>

I think this is OK as-is.  The "index" referred to in that paragraph is an
index into the array representation, which is different from any indices
elsewhere.



> ### Section 8.4
>
> s/Groups which already have an out-of-band mechanism/Groups that already
> have
> an out-of-band mechanism/ ?
>

Done.

Thanks,
--Richard