Re: [lamps] AD review of draft-ietf-lamps-rfc7030est-clarify-05

[Roman Danyliw <[e-mail]>]

Hi Michael!

Thanks for making all of these edits.  More commentary is inline, but the bottom line is it looks ready for IETF LC.

> -----Original Message-----
> From: Spasm <[e-mail]> On Behalf Of Michael Richardson
> Sent: Sunday, June 14, 2020 2:08 PM
> To: Roman Danyliw <[e-mail]>; LAMPS WG <[e-mail]>
> Cc: max pritikin <[e-mail]>
> Subject: Re: [lamps] AD review of draft-ietf-lamps-rfc7030est-clarify-05
> 
> 
> WG and Max, I have changed the section 3 (in this document) edits for the
> RFC7030 section 4 edits to Replace/New.
> 
> I could change the 4.5.2 change to be more explicit about what has changed.
> I found that section very difficult to read in rfc7030, so I prefer to have the
> changes applied in the update.
> 
> Roman Danyliw <[e-mail]> wrote:
>     > I conducted an AD review of draft-ietf-lamps-rfc7030est-clarify-05.
>     > Thanks for the work on this document to address issues that had arisen
>     > in RFC7030.  Below is my feedback:
> 
> Thank you.
> 
>     > (1) From idnits:
>     > == The 'Updates: ' line in the draft header should list only the _numbers_
>     > of the RFCs which will be updated by this document (if approved); it
>     > should not include the word 'RFC' in the list.
> 
> Fixed.  kramdown puts whatever I put in the header into the output.
> I had "RFC7030"
> 
>     > This is likely an artifact of the document build chain you are using,
>     > but can you please recompile it to spit out the following s/Updates:
>     > RFC7030/Updates: 7030/
> 
> 
>     > (2) From idnits:
>     > == Unused Reference: 'RFC8179' is defined on line 377, but no explicit
>     > reference was found in the text
> 
> That's BCP14.  I see it in the BCP14 paragraph.
> 
>     > == Unused Reference: 'X681' is defined on line 385, but no explicit
>     > reference was found in the text
> 
>     > == Unused Reference: 'X682' is defined on line 389, but no explicit
>     > reference was found in the text
> 
> I believe that I got a request to please include them all.
> The reference is:
>    This annex provides the normative ASN.1 definitions for the
>    structures described in this specification using ASN.1 as defined in
>    [X680] through [X683].
> 
> I will write them all out.
>
>     > (3) Section 1.  Editorial.
>     > "[RFC7030] defines the Enrollment over Secure Transport, or EST protocol.
>     > This specification defines a number  ..."
> 
>     > Because of the paragraph break, I initially read "This specification
>     > defines a ..." to mean this document when you really mean RFC7030.
> 
> Changed to:
> 
> -{{RFC7030}} defines the Enrollment over Secure Transport, or EST protocol.
> -
> -This specification defines a number of HTTP end points for certificate
> enrollment and management.
> +Enrollment over Secure Transport (EST) is defined in {{RFC7030}}.
> +The EST specification defines a number of HTTP end points for certificate
> enrollment and management.
>
>     > (4) Section 1.  Per "Changes to [RFC7030] to bring it inline with
>     > typical HTTP processing  risk changes the on-wire protocol in a way
>     > that is not backwards  compatible.  However, reports ..."
> 
>     > -- I had difficulty parsing this first sentence due to the use of
>     > "changes" twice.
> 
>     > -- Calling something "typical HTTP processing" begs for a reference
>     > describing that baseline behavior.
> 
>     > Maybe saying something like:
> 
>     > Any updates to [RFC7030] to bring it inline with HTTP processing per
> [RFC7231] risks changing the on-wire protocol in a way that is not backwards
> compatible.
> 
> Done.
>
>     > (5) Section 1.  Editorial.  There is no canonical set of implementers
>     > (so the definite article shouldn't suggest that).  s/However, reports
>     > from the implementers .../However, reports from implementers/
> 
> done.
> 
>     > (6) Section 1.  Typo.  s/extranous/extraneous/
> 
> fixed.

Thanks for all of the edits noted above.

>     > (7) Section 1.  Editorial. Per "This document deals with errata numbers
>     > [errata4384], [errata5107], [errata5108], and [errata5904]", this is
>     > helpful to point out that these errata are getting addressed.  They are
>     > even cited as normative references.  However, by my read, the text only
>     > every explains how [errata5108] is being addressed.  The other three
>     > are not mentioned in the text at all.  Please make the text more
>     > explicit on which changes are addressing which errata.
> 
> I have listed in the introduction how each of the four errata are closed.
> 
>     > (8) Section 3, 4, and 5.  I found it confusing that each section used a
>     > different patch style.  Is there a reason to do that?
> 
> I see your point, but I think that we used the right style each time.
>
>     > ** Section 3 summarizes the behavior of 4.1.3, 4.3.1, 4.3.2, 4.4.2 and
>     > 4.5.2 without providing specific references to exact text, and then
>     > provides a general updated behavior (again without providing exact text
>     > updates)
> 
> Yes. I don't think that patching the text of each of those would make
> it easier to read.   We could cross-off the text of 4.1.3 if that would be
> easier.
> 
>     > ** Section 4.  Provides an outright new blob of text to drop into a
>     > section with high redundancy to the original
> 
> The CSR Attribute text would have the base64 edit applied to it as well.
> I personally thought that we might have edited that text a bit more to clarify
> how CSRs are made.
> 
>     > ** Section 5.  Uses an OLD vs. NEW convention.
> 
>     > Minimally, since the style of Section 4 and 5 are similar and are
>     > precise, I'd recommend making Section 3 look like one of these style
>     > (i.e., being clear on exactly what text is being updated/deprecated).
> 
> okay, I'll change section 3.  Please see next email/diff.

The new Section 3.2 is much clearer.  Thanks for putting it in.

Consider all of my other comments about patch style as resolved.  The nesting of the changes per the addressed issue works for me.

>     > (a) Section 1.  "However, reports from the implementers suggest that
>     > many implementations do not send the Content-Transfer-Encoding, and
>     > many of them ignore it.  The consequence is that simply deprecating the
>     > header would remain compatible with current implementations." - this
>     > commentary suggests that dropping the header would cause no issues if
>     > that approach is taken.
> 
> That's what we have done.
> 
>     > (b) Section 3.  "This document updates [RFC7030] to require the POST
>     > request and payload response of all endpoints use Base64 encoding as
>     > specified in Section 4 of [RFC4648]." - this updated guidance doesn't
>     > explicitly rule out the use of the header
> 
> Correct, current implementations do not set the header, and ignore it upon
> receipt.  If there are lurking implementations that did not response that do
> include the header, then they will continue to operate.  If someone ran into
> such an implementation that required the header, then it could work around by
> continuing to include it.
> 
>     > (c) Section 3. "This format is to be used regardless of any
>     > Content-Transfer-Encoding header, and any value in such a header MUST
>     > be ignored." - this doesn't say anything about sending the header, only
>     > that it should be ignored on processing; this seems important for
>     > backwards compatibility.
> 
> Yes.  Liberal in what you accept.
> Not, no need to send it.
> 
>     > Why drop the header from one section per (d) but not from the others
>     > per (e)?  Under what circumstances should a header be sent -
>     > specifically, if (c) says the header should be dropped, why do we still
>     > send it per (e)?
> 
> I don't think we do, and maybe your request for explicit patches is waranteed.

Fixed as noted above.

>     > (10) Section 3.1. Per "This specification clarifies that despite
>     > [RFC2616], that white space including CR, LF, spaces (ASCII 32) and,
>     > tabs (ASCII 9) should be tolerated by receivers", is there a reason
>     > that there isn't a normative SHOULD here?
> 
> Agreed. SHOULD now.

Thanks.

>     > (11) Section 4.
>     > "Section 4.5.2 of [RFC7030] is to be replaced with the following text:
>     > 4.1.  CSR Attributes Response"
> 
>     > ** Editorial. I suspect there is a tool chain rendering issue.  The
>     > section header to drop into RFC7030 needs to read "4.5.2 CSR Attributes
>     > Response" (not 4.1 which is the next section of this document).
>     > Editorial alternative would be to drop the header entirely or following
>     > convention used in Sections 5.1 and 5.2.
> 
> Yeah, I'll render it without the markup.

Thanks.

>     > ** Editorial. It might also be valuable to explain what is getting
>     > changed instead of just providing an updated blob.
> 
> I'll defer to the WG here.
> 
>     > (12) Section 4. Editorial.  For consistency with RFC7090, consider
> s/[X609]/[X.609]/.
> 
> Okay.

Thanks.

>     > (13) Section 5.  The updates here drop the normative MUST to a "must"
>     > regarding the "response data must ...".  The Errata text explains that
>     > "Note that the MUST was reduced to a must because no content-type is
>     > specified" but I'm still not following why we lost the normative MUST.
>     > What am I missing?
> 
> Editorial mistake

Thanks.

Regards,
Roman

> --
> Michael Richardson <[e-mail]>, Sandelman Software Works  -=
> IPv6 IoT consulting =-