Re: [Perc] AD Evaluation of draft-ietf-perc-double-09

[Richard Barnes <[e-mail]>]

Oh actually, let's not worry about synchronizing now.  I'm OK to send these
to IETF LC separately; I just think they should all be on the same IESG
telechat.

I'll push a new version of this tomorrow, and EKT as soon as I can find a
responsive author.

--Richard

On Mon, Oct 15, 2018 at 7:04 PM Ben Campbell <[e-mail]> wrote:

> Works for me. The framework draft review is in my queue for tomorrow.
>
> Thanks!
>
> Ben.
>
> On Oct 15, 2018, at 5:42 PM, Richard Barnes <[e-mail]> wrote:
>
> I would propose we put forward all three documents together.  (-double,
> -ekt-diet, and -framework)  They'll be more comprehensible to the ADs that
> way.
>
> On Mon, Oct 15, 2018 at 6:40 PM Ben Campbell <[e-mail]> wrote:
>
>> Thanks, the PR looks good to me. Please submit a revision when you are
>> ready.
>>
>> I think that, with the PR applied, it will be ready for IETF LC.  That
>> should probably be done in tandem with the media framework (in my queue to
>> review in the next couple of days. Authors and chairs: do you think we
>> should also coordinate with ekt-diet?
>>
>> Thanks!
>>
>> Ben.
>>
>>
>> On Oct 5, 2018, at 9:48 AM, Richard Barnes <[e-mail]> wrote:
>>
>> Hey Ben,
>>
>> After talking with Cullen, I am now the stuckee for this.  I've filed a
>> PR at:
>>
>> https://github.com/ietf/perc-wg/pull/160
>>
>> Comments inline below.
>>
>> On Tue, Jun 5, 2018 at 12:40 AM Ben Campbell <[e-mail]> wrote:
>>
>>> Hi,
>>>
>>> This is my AD Evaluation of draft-ietf-perc-double-09. Thanks for this
>>> work; I think it is important and on the right track, but suffers from some
>>> readability issues, especially from §7 onwards. I’d like to discuss my
>>> comments prior to IETF last call.
>>>
>>> Thanks!
>>>
>>> Ben.
>>>
>>> —————————————————
>>>
>>> *** Substantive Comments ***
>>>
>>> General: Does it make sense to progress this prior to
>>> draft-ietf-perc-private-media-framework rather than wait and progress them
>>> together? it seems like one really needs to read the framework to
>>> understand at least parts of this draft. (Which would, btw, suggest making
>>> the framework a normative reference.)
>>>
>>
>> I think at this point, -framework has caught up a bit (I understand it's
>> awaiting your review), so this seems OBE.  I agree that it will make easier
>> reading for ADs to read the three docs (this, framework, and EKT) on the
>> same telechat.
>>
>>
>>
>>> §2, Media Distributor: Should this be defined as switch-based? Otherwise
>>> it seems circular.
>>>
>>
>> I've copied down the definition §1, which explicitly says that the MD
>> does not need to interpret or change media content.  Also added a ref to
>> -framework at this point.
>>
>>
>>
>>> §5 and subsections: These sections are confusing in the treatment of
>>> extensions. §5.1 step 3 says to truncate the header to remove any
>>> extensions. Yet other sections of text repeatedly mention the handling of
>>> extensions. I think the document needs a paragraph or two to describe the
>>> handling of RTP extensions at a high level.
>>>
>>
>> I've added an overview of the transform, including thoughts on
>> extensions, to the top of §5.
>>
>>
>>
>>> Along those lines, it’s not entirely clear what is meant by putting “
>>> (12 + 4 * CC bytes)” in parentheses after the guidance to truncate in §5.1.
>>> Is that the amount to be truncated? Amount left after truncation? (Same
>>> question for §5.3)
>>>
>>
>> Clarified that this is the amount left after truncation, i.e., take the
>> first N bytes, in both places.
>>
>>
>>
>>> §5.2: It would be helpful to include a paragraph or two to describe the
>>> impact if multiple MDs modify the same field(s). I can infer that, but it
>>> would be better to be explicit. (IIRC, there is a passing mention in the
>>> security considerations, but it would be better to have more here.)
>>>
>>
>> Added a paragraph.
>>
>>
>>
>>> §8: Can you offer any guidance about selecting 128 vs 256?
>>>
>>
>> I don't really think there's much to say here; it's up to the user of the
>> transform.  Use 128 unless you're paranoid, then use 256?  :)  Note that
>> RFC 7714, which defines the AES-GCM transforms, does not provide similar
>> guidance.
>>
>>
>>
>>> §9:
>>>
>>> - The security considerations mainly summarize the mechanism. I’d like
>>> to see a description of the potential attacks or risks  and how the double
>>> mechanism mitigates them.
>>> - “… this shouldn’t typically impact the strength of e2e integrity given
>>> the fact that there doesn’t exist header extensions defined today that
>>> needs e2e protection.  However, if future specifications define header
>>> extensions that needs e2e integrity protection, the input to inner
>>> transform may be modified to consider including the header.”
>>>
>>> - 2nd paragraph: Why is the HBH key for an outgoing packet only
>>> “typically” different than the key for the incoming packet? Are there
>>> security implications if they are the same?
>>>
>>> - 2nd to last paragraph, last sentence: Please elaborate on those risks?
>>>
>>
>> I agree with your assessment that the existing security considerations
>> were not that useful.  I've replaced them with an analysis that's more
>> focused on what protections are provided with respect to which attackers.
>>
>> The new version also calls out clearly that when a packet is
>> re-encrypted, the outer keys MUST be different.
>>
>>
>>
>>> *** Editorial comments and nits ***
>>>
>>> There are a number of abbreviations that need to be expanded on first
>>> use. Please remember that abbreviations should be expanded both in the
>>> abstract and the body.
>>>
>>> - SRTP
>>> - SSRC
>>> - SEQ
>>> - ROC
>>> - PRF
>>> - PT
>>> - SEQ
>>> - RTX
>>> - RED
>>> - FEC
>>>
>>
>> I've expanded most of these.  I have left SSRC, SEQ, ROC, and PT, since
>> in this context, they are basically opaque labels for RTP header fields.
>>
>>
>>
>>> §2:
>>> - Please use the boilerplate from RFC8174. There are a number of lower
>>> case instances of normative keywords.
>>>
>>
>> Done.
>>
>>
>>> - Please use consistent sentence (or lack of sentence) structure for the
>>> definitions in the bullet list.
>>>
>>
>> Done.
>>
>>
>>> §3,
>>>
>>> - " Generate key and salt values of the length required for the combined
>>> inner (end-to-end) and outer (hop-by-hop) algorithms.”: The document is
>>> inconsistent in whether it describes a single key with inner and outer
>>> halves, or separate inner and outer keys. I realize that this is an
>>> artifact of syntax, but it is likely to confuse a reader new to the topic.
>>>
>>
>> The very next bullet says to glue them together.  Not sure how else to
>> say this.
>>
>>
>>> - Paragraph starting with “Obviously, if the Media Distributor…”:
>>> “Obviously” is a null word in context.
>>>
>>
>> Deleted.
>>
>>
>>> §3.1: This section would be much more approachable if you defined the
>>> terms (such as n, k, and x), rather than forcing readers to look them up in
>>> 3711. Also, the doubled crypto suite IDs are likely to be confusing to the
>>> reader who sees them here without the explanation later in the draft.
>>>
>>
>> Done.
>>
>>
>>> §5.2, step 2: “Change any parts of the RTP packet…” -The MS is limited
>>> to changing the 3 fields defined for the OHB, right, at least as defined in
>>> this draft? Not just anything it wants?
>>>
>>
>> Correct.  Done.
>>
>>
>>> §5.3, last two paragraphs: It’s sort of an odd construction to talk
>>> about “any of the following” with a single entry list..
>>>
>>
>> Revised to clarify.
>>
>>
>>> §7 and subsections: These could use another proofreading pass. In
>>> particular, it is imprecise about inner vs outer encryption, which actors
>>> do what, and has a number of pronouns with ambiguous antecedents. The heavy
>>> use of passive voice obscures the actors in multiple places. I will call
>>> out some specifics, but please do not treat my comments as exhaustive.
>>>
>>
>> Did a revision pass, let me know what you think.
>>
>>
>>> §7: “ The repair mechanism, when used with double, typically operates on
>>> the double encrypted data and encrypts
>>>    them using only the HBH key.” Does “double encrypted” mean “outer
>>> encrypted”? Also, while I recognize “data” as plural, it’s plural in a
>>> non-countable sense; that is, s / them / it.
>>>
>>> §7.1:
>>> - “ … cached payloads MUST be the encrypted packets …” - Inner or outer?
>>> What device enforces this? (Consider active voice.)
>>> - Which element represents “the other side”? Are we talking about an
>>> endpoint or MD? (Please stick to the defined terminology)
>>> - “ When encrypting a retransmission packet, it MUST be encrypted the
>>> packet in repair mode.” - I can’t parse the phrase after the comma. Also,
>>> what device MUST encrypt it?
>>>
>>> §7.2:
>>> - “the primary encoding MAY contain…” - Is that MAY a grant of
>>> permission or a statement of fact?
>>>
>>> - " The sender takes encrypted payload from the cached packets”- Missing
>>> article for encrypted payload? Or should this say “… takes encrypted
>>> packets from the cached payload…”?
>>>
>>> - “ Any header extensions from the primary encoding are copied to the
>>> RTP packet that will carry the RED payload and the other RTP header
>>> information such as SSRC, SEQ, CSRC, etc are set to the same as the primary
>>> payload.  The RED RTP packet is then encrypted in repair mode and sent.”:
>>> This is confusing; parts are copied from the primary encoding and others
>>> are the same as in the primary encoding? Don’t both of those mean the same
>>> thing?
>>>
>>> - “ The receiver decrypts the payload…”: Endpoint or MD? Is this inner
>>> or outer encryption?
>>>
>>> - Last paragraph: Seems like that belongs in the FEC section. Also, does
>>> this document really need to make that sort of recommendation? It seems
>>> like a general statement about RED and FEC that is not specific to double.
>>>
>>>
>>> §7.3:
>>>
>>> - "When using Flex FEC [I-D.ietf-payload-flexible-fec-scheme] with
>>> double, the negotiation of double for the crypto is the out of band
>>> signaling that indicates that the repair packets MUST use the order of
>>> operations of SRTP followed by FEC when encrypting.” - The sentence is hard
>>> to parse. Also, is MUST a normative requirement or a statement of fact? (It
>>> seems odd to signal that you MUST do something).
>>>
>> - “…  data, which is already encrypted, it MUST be encrypted in repair
>>> mode packet.” s/ “, which” / “that”. Also, what actor MUST encrypt the data
>>> in repair mode? (Consider active voice.)
>>>
>>> - “ The algorithm recommend…” s / recommend / recommended
>>>
>>
>> Done.
>>
>>
>>> §7.4:
>>> - s/ "sent with [RFC4733]” / “sent using the mechanism in [RFC4733]”
>>> - “ and the relay can not read it so it cannot be used” - missing comma
>>> between “it” and “so”.
>>>
>>
>> Done.
>>
>>
>>> §8, last paragraph: “ For packets in repair mode, the data they are
>>> caring is often already encrypted further increasing the size.” - I assume
>>> “caring” is not the intended word. Did you mean “carrying”? If so, consider
>>> s / “data they are carrying” / “data they carry”
>>
>>
>> Revised.
>> _______________________________________________
>> Perc mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/perc
>>
>>
>>
>