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

[Roman Danyliw <[e-mail]>]

Hi!

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: 

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

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

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

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

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

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

(6) Section 1.  Typo.  s/extranous/extraneous/

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

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

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

** Section 4.  Provides an outright new blob of text to drop into a section with high redundancy to the original

** 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). 

(9) Section 3.  The updated guidance on handling the "Content-Transfer-Encoding" header isn't clear to me.  My confused is likely related to comment (8).  The following sections/text seem to contain related guidance/commentary:

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

(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

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

(d) Section 4.  The text removes the reference to "Content-Transfer-Encoding" in a patch to Section 4.5.2 of [RFC7030]

(e) [RFC7030] Sections 4.1.3 (CA Certificates Response, /cacerts), 4.3.1/4.3.2 (Full CMC, /fullcmc), and 4.4.2 (Server-Side Key Generation) still contain a reference to Content-Transfer-Encoding even after  applying my read on how this document "patches" RFC7030.  For example:

** [RFC7030] Section 4.1.3. "The Simple PKI Response is sent with a Content-Transfer-Encoding of "base64" [RFC2045]."

** [RFC7030] Section 4.3.1/4.3.2. "The body of the message is the binary value of the encoding of the XXXX with a Content-Transfer-Encoding of "base64" [RFC2045]."

** An "application/pkcs8" part consists of the base64-encoded DER-encoded [X.690] PrivateKeyInfo with a Content-Transfer-Encoding of "base64" [RFC2045].

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

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

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

** Editorial. It might also be valuable to explain what is getting changed instead of just providing an updated blob.

** Editorial. Given that the changes are confined to paragraph 2 and 3, why reproduce the rest of the content here - i.e., there is over a page worth of text that isn't changed but is reproduced here.  Other updates, described in section 5, only reproduced the updated paragraphs.

(12) Section 4. Editorial.  For consistency with RFC7090, consider s/[X609]/[X.609]/.

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

(14) Section 8.  Typo. s/doucment/document/

(15) Section 8.  Typo. s/doducment/document/

Regards,
Roman