[lamps] Genart last call review of draft-ietf-lamps-cms-hash-sig-08

[Dale Worley via Datatracker <[e-mail]>]

Reviewer: Dale Worley
Review result: Ready

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-lamps-cms-hash-sig-08
Reviewer:  Dale R. Worley
Review Date:  2019-07-17
IETF LC End Date:  2019-08-01
IESG Telechat date:  not known

Summary:

       This draft is in great shape and ready for publication as a
       proposed standard RFC, with only a few editorial nits.

Nits/editorial comments: 

2.2.  Leighton-Micali Signature (LMS)

   The [HASHSIG] specification supports five tree sizes:

      LMS_SHA256_M32_H5;
      LMS_SHA256_M32_H10;
      LMS_SHA256_M32_H15;
      LMS_SHA256_M32_H20; and
      LMS_SHA256_M32_H25.

This text seems redundant with the description in the preceding
paragraph.

   The LMS public key is the string consists of four elements: the

Perhaps "An LMS public key consists of ...".

      u32str(lms_algorithm_type) || u32str(otstype) || I || T[1]

The notation "T[1]" seems to be undefined (although the intended value
is described clearly in the preceding paragraph).

2.3.  Leighton-Micali One-time Signature Algorithm (LM-OTS)

      n -  The number of bytes associated with the hash function.
           [HASHSIG] supports only SHA-256 [SHS], with n=32.

"associated" seems to me to be vague.  Perhaps "The length in bytes of
the output of the hash function."

      ls - The number of left-shift bits used in the checksum function,
           which is defined in Section 4.4 of [HASHSIG].

"The number of left-shift bits" is not quite right.  Perhaps "The
number of bits of left-shifting used in ..." or "The amount/size of
the left-shift used in ...".

5.  Signed-data Conventions

This paragraph has to be a number of minor wording issues, which I
have described interline:

   As specified in [CMS], the digital signature is produced from the
   message digest and the signer's private key.  The signature is
   computed over different value depending on whether signed attributes

s/value/values/

   are absent or present.  When signed attributes are absent, the
   HSS/LMS signature is computed over the content.  When signed

It might help the reader to put a paragraph break before "When signed
attributes are present..."

   attributes are present, a hash is computed over the content using the
   same hash function that is used in the HSS/LMS tree, and then a
   message-digest attribute is constructed with the resulting hash

I would replace "with" with "containing" or "whose value is"

   value, and then DER encode the set of signed attributes, which MUST

For parallelism, this clause should start with a subject and a passive
verb.  Perhaps "the DER encoding is constructed of ...".

   include a content-type attribute and a message-digest attribute, and

It might be clearer if the clause "which MUST ... attribute" was put
in parentheses.

   then the HSS/LMS signature is computed over the output of the DER-
   encode operation.  In summary:

You can probably change "the output of the DER-encode operation" with
"the DER encoding".

The paragraph contains four clauses joined by three successive "and
then".  You probably want to change that, perhaps breaking it out as a
numbered/bulleted list.  (What does the Editor recommend?)

And in this computation:

      IF (signed attributes are absent)
      THEN HSS_LMS_Sign(content)
      ELSE message-digest attribute = Hash(content);

I think you want to add a hyphen:
s/message-digest attribute/message-digest-attribute/

           HSS_LMS_Sign(DER(SignedAttributes))

[END]