Subject: Genart last call review of draft-ietf-lamps-cms-hash-sig-08

From: Dale Worley via Datatracker <[email-masked]>
Date: Wed, 17 Jul 2019 19:38:56 -0700

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]

---

From: Russ Housley <[email-masked]>
Date: Thu, 18 Jul 2019 10:19:26 -0400

Dale:

Thank you for the careful review.

>       proposed standard RFC, with only a few editorial nits.

Good to hear.

> This text seems redundant with the description in the preceding

True.  The intent was to provide the identifiers for the five tree sizes.

Perhaps the two paragraphs should be merged, with the sentence before the list saying:

   ... As a result, the [HASHSIG] specification supports

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

Yes, that reads better.

> is described clearly in the preceding paragraph).

Good catch.  How about:

   ... and the m-byte string associated with the root
   node of the tree (T[1]).

> the output of the hash function."

Okay.  How about:

   n -  The length in bytes of the hash function output. ...

> the left-shift used in ...".

These words were taken directly from Section 4.1 of RFC 8554.  That said, I think you are right that it could be more clear.  How about:

   ls - The number of bits that are left-shifted in the final step of
        the checksum function, which is defined in Section 4.4
        of [HASHSIG].

>   computed over different value depending on whether signed attributes

Fixed.

> attributes are present..."

Okay.  Done.

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

How about:

   ... a message-digest attribute is constructed to contain the
   resulting hash value, and ...

> verb.  Perhaps "the DER encoding is constructed of ...".

How about:

   ... and then the result of DER encoding the set of signed
   attributes, which ...

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

Okay.  There are a lot of commas in this sentence.

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

How about:

   ... then the HSS/LMS signature is computed over the
   DER-encoded output.

> numbered/bulleted list.  (What does the Editor recommend?)

I think the text is accurate.  I wil wait for the RFC Editor to propose a different format if they want to do so.

> s/message-digest attribute/message-digest-attribute/

No.  This is the way that the attributes are talked about in RFC 5652.
(See the indented paragraphs on Page 15 of RFC 5652 as an example.)

>           HSS_LMS_Sign(DER(SignedAttributes))

Thanks again for the careful review.

Russ