Hi Christer, Sorry, just now seeing this. Responses inline. PR here:
https://github.com/ietf/perc-wg/pull/165 --RLB On Sat, Oct 20, 2018 at 6:06 PM Christer Holmberg < [email protected]> wrote: > Reviewer: Christer Holmberg > Review result: Ready with Issues > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-perc-srtp-ekt-diet-09 > Reviewer: Christer Holmberg > Review Date: 2018-10-20 > IETF LC End Date: 2018-11-01 > IESG Telechat date: Not scheduled for a telechat > > Summary: The document is well written, and easy to read. But, I think some > things still need to be clarified. I also have some editorial modification > suggestions. > > Major issues: > -------------- > > Q1_1: > > The text in section 1 says that EKT removes the need for co-ordinating > SSRCs, > and that an endpoint can choose whatever value it wants. > > However, I can't find any explanation on how EKT removes that need. > The answer is toward the end of the introduction: "EKT does not control the manner in which the SSRC is generated..." -- it just makes the distribution of security information easier. I extended that paragraph to clarify. > Q1_3: > > The text in section 1 says that EKT is not intended to replace key > establishment mechanisms, but to "be used in conjunction". > > However, there is no description on how that works in conjunction with > existing > mechanisms (e.g., together with SDP Offer/Answer), and whether existing > mechanisms need to be modified in order to work in conjunction with EKT. > > Section 5.3 does say that the SDP O/A exchange is not affected. If that is > true, then you need to describe how SSRC values etc signalled in SDP > relate to > values signalled using EKT, what happens if there are mismatches etc. > This document only defines one such augmentation -- the integration with DTLS-SRTP. I extended the relevant part of the introduction to say this. > Minor issues: > -------------- > > Q1_2: > > The text in section 1 says: > > "However, there are several cases in which conventional signaling > systems > cannot easily provide all of the coordination required." > > I think some examples would be useful. > Disagree. > Q1_4: > > The text in section 1 says that providing SSRCs etc using signaling systems > cause layer violations. > > Is this layer violation described somewhere? If so, I think a reference > would > be useful. > I just deleted this sentence. It wasn't adding much. > Q4-2_1: > > The text in section 4.2 says: > > "All of the received EKT parameter sets SHOULD be stored by all of the > participants in an SRTP session, for use in processing inbound SRTP > and SRTCP traffic." > > What is the reason for SHOULD, instead of MUST? What happens if an endpoint > does NOT store the EKT parameter sets? > I think the SHOULD here is to allow, e.g., cache management. Clarified the consequences. > Q4-2-1_2: > > The text in section 4.2.1 says: > > "Outbound packets SHOULD continue to use the old SRTP Master Key for > 250 ms after sending any new key." > > What is the reason for SHOULD, instead of MUST? > I don't think this should be a MUST. For example, how accurately are you going to measure compliance? If the change-over is 10ms short 10% of the time, is the endpoint out of compliance. Plus, there's minimal interop impact; this just helps things go smoothly. > Q4-3_2: > > The text in section 4.3 says: > > "Section 4.2.1 recommends that SRTP senders continue using an old key > for some time after sending a new key in an EKT tag." > > The text in section 4.2.1 contains a SHOULD, so it is more than a > recommendation. > According to RFC 2119, a SHOULD is precisely a recommendation: """ 3. SHOULD This word, or the adjective "RECOMMENDED", mean that there may exist valid reasons in particular circumstances to ignore a particular item, but the full implications must be understood and carefully weighed before choosing a different course. """ https://tools.ietf.org/html/rfc2119#section-3 > Nits/editorial comments: > -------------------------- > > Q1_5: > > I think Section 1 should also indicate that EKT is an DTLS extension, > similar > to the Abstract. Otherwise, when you say that EKT is a way to distribute > information, it is unclear why EKT doesn't violate layers. > Done. > I think that Section 2 (Overview) could be combined with Section 1 > (Introduction). Introduction sections in RFCs typically provide both > background, problem statement and an overview of the mechanism - and > sometimes > also the document structure. > Disagree. > Q4_1: > > The text in section 4.1 says: > > "EKT MUST NOT be used in conjunction with SRTP's MKI (Master Key > Identifier) or with SRTP's <From, To> [RFC3711], as those SRTP > features duplicate some of the functions of EKT. Senders MUST NOT > include MKI when using EKT. Receivers SHOULD simply ignore any MKI > field received if EKT is in use." > > I suggest to put this text in a dedicated Applicability section. > Disagree. > Q4-1_1: > > The text in section 4.1 says: > > "Together, these data elements are called an EKT parameter set. Each > distinct EKT parameter set that is used MUST be associated with a > distinct SPI value to avoid ambiguity." > > I suggest to defined "EKT parameter set" in the same way as the other > terminology. I.e., > > "EKT parameter set: The parameters indicated by the SPI" > > ....or something like that. > There's not a terminology section, so I didn't do exactly what you say. But I pulled this out into a separate section so that it doesn't interrupt the flow, and clarified that the SPI-params association is set up by the DTLS-SRTP extension. Q4-2-1_1: > > For the section names of sections 4.2.1 and 4.2.2, I suggest to talk about > sending/transmitting and receiving instead of inbound and outbound. > Disagree > Q4-3_1: > > In section 4.3, I think the name of the section ("Implementation Notes") > is a > little confusing. Also, is there a reason why the text is not in section > 4.2.1 > and/or 4.2.2? > Just folded this into section 4.2.2. > Q4-4_1: > > Sections 4.4 and 4.4.1 have the same section names. Please change one of > them > (e.g., change 4.4.1 to "Default Cipher"). > Changed to "AES Key Wrap" > Q4-6_1: > > I think the text in section 4.6 should be placed in the Applicability > section I > suggested earlier (Q4_1). > Merged it into section 4. > Q5_1: > > In section 5, I suggest to change the section name to "DTLS-SRTP > Considerations". > Disagree. > > > _______________________________________________ > Perc mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/perc >
_______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
