Re: [lamps] AD review of draft-ietf-lamps-cmp-algorithms-07

[Roman Danyliw <[e-mail]>]

Hi Hendrik!

> -----Original Message-----
> From: Brockhaus, Hendrik <[e-mail]>
> Sent: Friday, October 29, 2021 8:31 AM
> To: Roman Danyliw <[e-mail]>
> Cc: LAMPS WG <[e-mail]>
> Subject: AW: AD review of draft-ietf-lamps-cmp-algorithms-07
> 
> Roman
> 
> Many thanks for your feedback.
> 
> I am confused regarding your comments on field names vs. type names.
> If you look for example at the definition of KeyAgreeRecipientInfo (RFC 5652
> Section 6.2.2):
>       KeyAgreeRecipientInfo ::= SEQUENCE {
>         version CMSVersion,  -- always set to 3
>         originator [0] EXPLICIT OriginatorIdentifierOrKey,
>         ukm [1] EXPLICIT UserKeyingMaterial OPTIONAL,
>         keyEncryptionAlgorithm KeyEncryptionAlgorithmIdentifier,
>         recipientEncryptedKeys RecipientEncryptedKeys } My reading is that
> keyEncryptionAlgorithm is the field name and
> KeyEncryptionAlgorithmIdentifierits the type. Am I wrong?

You should be confused, because I'm wrong.  Sorry about that.  In that syntax of "<type> <type> <type> <field name>" that is used in the document, I translated it in my head as "<field name> <field name> ... <type>".  Please ignore all the various places where I called this out incorrectly in my earlier feedback.

> Please find further comments below inline.
> Feedback on you comment regarding Section 7 and Section 9 will come next
> week.
> 
> > -----Ursprüngliche Nachricht-----
> > Von: Spasm <[e-mail]> Im Auftrag von Roman Danyliw
> >
> > Hi!
> >
> > I did an AD review of draft-ietf-lamps-cmp-algorithms-07 with my
> > comments below.  Thanks for this work to evolve CMP.
> >
> > ** Section 2, 3, etc.  Editorial. When listing the places where digest
> > and signature algorithm identifies are found, consider if enumerating
> > them as a bulleted list might be easier to read.  For example:
> >
> > OLD
> >    Digest algorithm identifiers are located in the hashAlg field of
> >    OOBCertHash, the owf field of Challenge, PBMParameter, CertStatus,
> >    and DHBMParameter, and the digestAlgorithms field of SignedData and
> >    the digestAlgorithm field of SignerInfo.
> >
> > NEW
> > Digest algorithm identifiers are located in:
> > * hashAlg field of OOBCertHash
> > * owf field of Challenge, PBMParameter, CertStatus, and DHBMParameter
> > * digestAlgorithms field of SignedData
> > * digestAlgorithm field of SignerInfo.
> 
> I like this proposal and will update the draft accordingly. I also change the
> paragraph on digest value locations accordingly. I did the same in other
> sections, where appropriate.

Thanks.

> >
> > ** Section 2.
> > Note: Specific conventions are needed for CertStatus content in
> >    certConf messages when confirming certificates where the
> >    AlgorithmIdentifier of the certificate signature does not clearly
> >    imply a specific hash algorithm.  In such cases the hash algorithm to
> >    use to build certHash should be specified, e.g., as done in
> >    Section 2.1 and Section 2.2 for certificates signed using EdDSA.
> >
> > I'm confirming my understanding of the text.  Is the specific
> > convention being described here the second sentence (i.e., explicitly
> > specifying the hash algorithm)?  I ask because "conventions" was
> > plural suggesting that there was more than one.
> 
> Good point. Doth this proposal solved your issue?
> 
> Old:
>    Note: Specific conventions are needed for CertStatus content in
>    certConf messages when confirming certificates where the
>    AlgorithmIdentifier of the certificate signature does not clearly
>    imply a specific hash algorithm.  In such cases the hash algorithm to
>    use to build certHash should be specified, e.g., as done in
>    Section 3.3 for certificates signed using EdDSA.
> 
> New:
>    Note: When confirming certificates where the AlgorithmIdentifier
>    of the certificate signature does not clearly imply a specific hash
>    algorithm and no specific hash algorithm is defined for usage with
>    this signature algorithm, as done in Section 3.3 for EdDSA, the hash
>    algorithm needs to indicated in the hashAlgs field of the certConf
>    message as specified in CMP Updates Section 2.9
>    [I-D.ietf-lamps-cmp-updates].

Looks good.  Thanks.

> >
> > ** Section 3.  Editorial.  s/Section 7.2,RFC/Section 7.2, RFC/
> 
> Yes, thanks
> 
> >
> > ** Section 3.1.  Editorial.
> > OLD
> > The algorithm identifiers for RSASAA-PSS signatures used with SHA2
> >    message digest algorithms is identified by the following OID:
> >
> > NEW
> > The algorithm identifier for RSASAA-PSS signatures used with SHA2
> >    message digest algorithms is identified by the following OID:
> 
> I will update the draft accordingly
> 
> >
> > ** Section 3.3.  Editorial. s/signature algorithm Ed25519
> > MUST/signature algorithm Ed25519 that MUST/
> 
> Yes, thanks
> 
> >
> > ** Section 4.1.
> >
> > Key agreement algorithm identifiers are located in the EnvelopedData
> >    RecipientInfos KeyAgreeRecipientInfo keyEncryptionAlgorithm fields.
> >
> > -- "keyEncryptionAlgorithm" is the type, the field name is
> > "KeyEncryptionAlgorithmIdentifier" per Section 6.2.1 of RFC5652.  It
> > should likely read "... KeyAgreeRecipientInfo
> KeyEncryptionAlgorithmIdentifier".
> 
> Section 4.1 addresses key agreement. Therefore RFC 5652 Section 6.2.2 applies.
> 
> RFC 5652 Section 6.2.2 defines:
>       KeyAgreeRecipientInfo ::= SEQUENCE {
>         version CMSVersion,  -- always set to 3
>         originator [0] EXPLICIT OriginatorIdentifierOrKey,
>         ukm [1] EXPLICIT UserKeyingMaterial OPTIONAL,
>         keyEncryptionAlgorithm KeyEncryptionAlgorithmIdentifier,
>         recipientEncryptedKeys RecipientEncryptedKeys }
> 
> Isn't keyEncryptionAlgorithm the field name?
> 
> >
> > -- why is "fields" plural? Isn't the cardinality here a single field?
> 
> Yes, it should be "field".
> 
> >
> > -- I'm mentioning it here but it applies to the other uses, using
> > spaces to convey the nested CMS structure is confusing.
> 
> I propose to use '-' instead of a space in case such structure is required. While
> switching to the bulleted lists I removed the nested structure.

Above looks fine.  Thanks.

> >
> > ** Section 4.1. Checking my understanding, is it accurate that both
> > the key agreement algorithm identifiers and the key encryption
> > algorithm identifiers are going to the same place in EnvelopedData?
> 
> Thanks for spotting this. This points at a confusion on my side. This needs to be
> changed in Lightweight CMP Profile Section 4.1.6.1 as well.
> 
> Looking at RFC 5753 Section 3.1.1 I understand now, that the key agreement
> algorithm OID is located in keyEncryptionAlgorithm and the key-wrap algorithm
> OID is located in the parameters field of the key agreement algorithm OID, e.g.,
> a PARAMS field (see RFC 5753 A.3).
> 
> RFC 5753 Section 3.1.1
>    -  keyEncryptionAlgorithm MUST contain the object identifier of the
>       key-encryption algorithm, which in this case is a key agreement
>       algorithm (see Section 7.1.4).  The parameters field contains
>       KeyWrapAlgorithm.  The KeyWrapAlgorithm is the algorithm
>       identifier that indicates the symmetric encryption algorithm used
>       to encrypt the content-encryption key (CEK) with the key-
>       encryption key (KEK) and any associated parameters (see Section
>       7.1.5).  Algorithm requirements are found in Section 8.
> 
> RFC 5753 Appendix A.2
> kaa-dhSinglePass-stdDH-sha224kdf-scheme KEY-AGREE ::= {
>   IDENTIFIER dhSinglePass-stdDH-sha224kdf-scheme
>   PARAMS TYPE KeyWrapAlgorithm ARE required
>   UKM -- TYPE unencoded data -- ARE preferredPresent
>   SMIME-CAPS cap-kaa-dhSinglePass-stdDH-sha224kdf-scheme
> }

Glad we caught that.

> >
> > ** Section 4.1.
> >    Wrapped content-encryption keys are located in the EnvelopedData
> >    RecipientInfos KeyAgreeRecipientInfo RecipientEncryptedKeys
> >    encryptedKey field.
> >
> > -- "encryptedKey" is also a type.  It should likely be "EncryptedKey"
> > per Section
> > 6.2.2 of RFC5652.
> 
> RFC 5652 Section 6.2.2 defines
>       RecipientEncryptedKey ::= SEQUENCE {
>         rid KeyAgreeRecipientIdentifier,
>         encryptedKey EncryptedKey }
> 
> Isn't encryptedKey the field name?
> 
> >
> > -- My ASN.1 reading always needs help.  Should it be "...
> > RecipientEncryptedKeys RecipientEncryptedKey EncryptedKey" or
> > "RecipientEncryptedKeys EncryptedKey"?  Same thing would apply to
> > "RecipientInfos" and "RecipientInfo".
> 
> I propose updating Section 4.1 as follows:
> Old:
>    Key agreement algorithm identifiers are located in the EnvelopedData
>    RecipientInfos KeyAgreeRecipientInfo keyEncryptionAlgorithm fields.
> 
>    Key encryption algorithm identifiers are located in the EnvelopedData
>    RecipientInfos KeyAgreeRecipientInfo keyEncryptionAlgorithm field.
> 
>    Wrapped content-encryption keys are located in the EnvelopedData
>    RecipientInfos KeyAgreeRecipientInfo RecipientEncryptedKeys
>    encryptedKey field.
> 
> New:
> Key agreement algorithm identifiers are located in:
> * keyEncryptionAlgorithm field of KeyAgreeRecipientInfo
> 
> Key wrap algorithm identifiers are located in:
> * KeyWrapAlgorithm parameters within keyEncryptionAlgorithm field of
> KeyAgreeRecipientInfo
> 
> Wrapped content-encryption keys are located in:
> * encryptedKey field of RecipientEncryptedKeys

Looks good.  Thanks.

> >
> > ** Section 4.2.
> > Key transport algorithm identifiers are located in the EnvelopedData
> >    RecipientInfos KeyTransRecipientInfo keyEncryptionAlgorithm field.
> >
> > Similar to Section 4.1, keyEncryptionAlgorithm is the type.  It likely
> > should be KeyEncryptionAlgorithmIdentifier per Section 6.2.1 of
> > RFC5652
> 
> RFC 5652 Section 6.2.1 defines:
>       KeyTransRecipientInfo ::= SEQUENCE {
>         version CMSVersion,  -- always set to 0 or 2
>         rid RecipientIdentifier,
>         keyEncryptionAlgorithm KeyEncryptionAlgorithmIdentifier,
>         encryptedKey EncryptedKey }
> 
> Isn't keyEncryptionAlgorithm the field name?
> 
> >
> > ** Section 4.2
> >    Key transport encrypted content-encryption keys are located in the
> >    EnvelopedData RecipientInfos KeyTransRecipientInfo encryptedKey
> >    Field.
> >
> > Similar to Section 4.1, encryptedKey is the type.  It likely should be
> > EncryptedKey per Section 6.2.1 of RFC5652.
> 
> RFC 5652 Section 6.2.1 defines:
>       KeyTransRecipientInfo ::= SEQUENCE {
>         version CMSVersion,  -- always set to 0 or 2
>         rid RecipientIdentifier,
>         keyEncryptionAlgorithm KeyEncryptionAlgorithmIdentifier,
>         encryptedKey EncryptedKey }
> 
> Isn't encryptedKey the field name?
> 
> >
> > ** Section 4.3.  As before,
> > s/keyEncryptionAlgorithm/KeyEncryptionAlgorithmIdentifier/
> 
> See above
> 
> >
> > ** Section 4.3.  As before, s/encryptedKey/EncryptedKey/
> 
> See above
> 
> >
> > ** Section 4.4. As before,
> > s/keyDerivationAlgorithm/KeyDerivationAlgorithmIdentifier/
> 
> RFC 5652 Section 6.2.4 defines:
>       PasswordRecipientInfo ::= SEQUENCE {
>         version CMSVersion,   -- Always set to 0
>         keyDerivationAlgorithm [0] KeyDerivationAlgorithmIdentifier
>                                      OPTIONAL,
>         keyEncryptionAlgorithm KeyEncryptionAlgorithmIdentifier,
>         encryptedKey EncryptedKey }
> 
> Isn't keyDerivationAlgorithm the field name?
> 
> >
> > ** Section 5.  Editorial.  s/Section 7,RFC/Section 7, RFC/
> 
> Yes, thanks
> 
> >
> > ** Section 5.  As before, s/ contentEncryptionAlgorithm
> > /ContentEncryptionAlgorithmIdentifier/
> >
> > ** Section 6. Editorial.  s/Section 7,RFC/Section 7, RFC/
> 
> Yes, thanks
> 
> >
> > ** Section 6.1.1. Editorial. s/andAlgorithm/and Algorithm/
> 
> Yes, thanks

Thanks for those changes above.

Thanks,
Roman

> >
> > ** Section 7.  I would have benefit from a bit more clarity on the
> > direction provided in this section.
> >
> > -- "The following criteria will help implementers choose appropriate
> > algorithms for managing certificates"
> >
> > The bulleted list seems like only part of the criteria.  As a
> > non-exhaustive list, additional considerations might be the
> > capabilities of the end-point (e.g., does the target deployment have
> > hardware acceleration for some, are some algorithms simply out of
> > reach due to available compute or expected software support); or perhaps
> local policy or compliance requirements?
> >
> > -- "The cryptographic strength of the algorithm with the lowest security."
> >
> > It seems like some words are missing here.  Is the intent here to
> > guide the selection towards the algorithm that provides the minimum
> > amount of strength relative to the needed security requirements?
> >
> > ** Section 7.1.  The text in this section seems it should explicit say
> > that it is updating RFC4210.
> >
> > OLD
> > The following table contains definitions of algorithms used within
> >
> > NEW
> > The following table updates the definitions of algorithms used within
> >
> > ** Section 7.1.  If an algorithm is listed in the "Change from 4210"
> > column, should it be considered the equivalent of being added to the
> > "other" column (per Appendix D.2 of RFC4210?  If so, please be explicit about
> that.
> >
> > ** Section 7.1.  Table 1.
> >
> > -- What does it mean for PasswordBasedMac to be in both the Mandatory
> > and
> > Change-from-4210 column for the MSG_MAC_ALG row?  It was already the
> > MTI in RFC4210.  Same question and situation with D-H for the
> PROT_ENC_ALG row.
> >
> > -- Should 3-DES be considered "other" in MSG_MAC_ALG and
> PROT_SYM_ALG?
> >
> > -- Editorial.  In PROT_SYM_ALG and SYM_PENC_ALG, the "3-DES
> > (3-key-EDE), CBC Mode" should read "3-DES (3-key-EDE, CBC mode)" per the
> text in RFC4210.
> >
> > ** Section 9.  Can a reference be added for the theoretical weakness
> > in PasswordBasedMac?
> >
> > ** Section 9.  Editorially, the first paragraph ("RFC 4210 Appendix
> > D.2 ...) and the two paragraphs of "In Section 7 ..." and "To keep the
> > list ...", seem to be saying the same thing.  Recommend a merge.
> >
> > ** Section 9.
> > In such systems the weakened algorithms should
> >    be disabled from further use.
> >
> > Can this document make stronger recommendations about deprecating
> > certain algorithms such as 3-DES SHA-1 with normative language rather
> > than keeping it in the "other" category in the profiles?
> >
> > ** Normative References.  [I-D.ietf-lamps-lightweight-cmp-profile]
> > should be a normative reference given the use of normative language
> > around it in Section 7.2.
> 
> I will move the reference up
> 
> 
> Hendrik