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

[Richard Barnes <[e-mail]>]

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
>
>
>