Last Call Review of draft-ietf-lamps-cms-sha3-hash-03
review-ietf-lamps-cms-sha3-hash-03-secdir-lc-hardaker-2024-05-03-00

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG. These comments were written primarily for the benefit of the
security area directors. Document editors and WG chairs should treat
these comments just like any other last call comments.

The summary of the review is: ready

In general this is a well written document.  I have some minor
thoughts/nits below.  Note that I'm not a CMS expert, but certainly have
a decent amount of generic background (including ASN.1).

- Section 2: The first paragraph has an odd line break after the
  first sentence.  I'm not sure what formatting has caused this, but it
  is present in both the txt and pdf versions at least.

- Section 2 paragraph 2: it would be helpful to less familiar readers to
  say what the fields were inside of what CMS structure.

- section 2, generally: by specifying exactly the list of fields where
  this applies, it does limit future extensions to the base protocol to
  not add new fields without updating this document as well.  This may
  be by design though.

- section 3.1 and beyond: there is a discrepancy between how OIDs are
  used in some places and the fully qualified OBJECT IDENTIFIERs are
  used in other places.  I note the appendix uses OID everywhere, but
  the copied versions into the sections above.

- Why is sigAlgs and hashAlgs defined in this module but RSAPublicKey is
  imported.  Even though sigAlgs and hashAlgs are simply references,
  wouldn't it be better to import them from somewhere else since I don't
  think you're actually defining them.  (my memory says you can import
  simple OIDs from other places).  My purist had would rather import
  them rather that duplicate the definition, but this is a choice to
  some extent (and yes, there is no actual harm in defining it in
  multiple places or even having two different names assigned to the
  same OID but then you get into proper coding practice and typos and
  fun.)

- section 5.2: suggest changing "If any other value is used for S" to
  "If any value is used for S", since you just said it must be absent if
  there was no label.  And unless "absent" is a value, then the word
  "other" doesn't apply.

- section 5.2: it would be kinda helpful in the paragraph to also state
  that S must be encoded as an OCTET STRING even though it's in the
  definition too.

- section 5.3: you state "an algorithm identifier from section 2 is
  carried in the parameter" but don't state what parameter.  Since this
  is sentences later, I'd suggest adding "KEMRecipientInfo" in front of
  parameter (and if I got the parameter wrong, then you already have one
  failed interpretation!)

- section 6: suggest "...authentication is not provided" ->
  "...authentication is not assured".

- section 6, sentence starting with "An attacker may find"...  The first
  time I read this I had to read it multiple times to pull it all in and
  marked it as "awkward".  Now I seem to understand it better, but I'll
  flag it as "could use some simplification and probably broken in 2" if
  you want to do it to help past-me.

- I find it odd that the appendix doesn't have a letter and only a
  title.  When there is only one, I suppose this makes sense but I
  vaguely normally remember seeing "Appendix A" even when there was only
  a single appendix.  maybe.

-- 
Wes Hardaker
USC/ISI