Re: [lamps] Paul Wouters' Discuss on draft-ietf-lamps-cmp-updates-21: (with DISCUSS and COMMENT)

["Brockhaus, Hendrik" <[e-mail]>]

Paul

Many thanks for your detailed review and your comments. Currently I am on vacation and will provide feedback after my return on June 20th.

Hendrik
________________________________
Von: Paul Wouters via Datatracker <[e-mail]>
Gesendet: Friday, June 10, 2022 8:42:45 PM
An: The IESG <[e-mail]>
Cc: [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>; [e-mail] <[e-mail]>
Betreff: Paul Wouters' Discuss on draft-ietf-lamps-cmp-updates-21: (with DISCUSS and COMMENT)

Paul Wouters has entered the following ballot position for
draft-ietf-lamps-cmp-updates-21: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)


Please refer to https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ietf.org%2Fabout%2Fgroups%2Fiesg%2Fstatements%2Fhandling-ballot-positions%2F&amp;data=05%7C01%7Chendrik.brockhaus%40siemens.com%7C82d56eb761c0488dd79f08da4b110393%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637904833709102651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=CAjQmh3nWmgTPMVcb3EICuQcw%2BlggelRo6f9EKGMf3o%3D&amp;reserved=0
for more information about how to handle DISCUSS and COMMENT positions.


The document, along with other ballot positions, can be found here:
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdatatracker.ietf.org%2Fdoc%2Fdraft-ietf-lamps-cmp-updates%2F&amp;data=05%7C01%7Chendrik.brockhaus%40siemens.com%7C82d56eb761c0488dd79f08da4b110393%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637904833709102651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=OGPjLGaIHUn8edJ3yUJvg5xppea9%2FjS1HkM7OnYrX6U%3D&amp;reserved=0



----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

As a reviewer, and therefor I suspect also implementors, needing to read
current + old and then compare it to new is very confusing. If this is for a
few paragraphs I can see the point but throughout the entire long document? It
prevented me from doing a full review.

The document also “updates” the IANA Considerations which is not a real process
we have. We only have new IANA Considerations and I don’t think we should tell
IANA to decode their instructions based on a diff with another rfc.

Please tell me how this document would not be simply better if the diffing and
replacing is done for the reader by obsoleting the old documents and creating
one new clear readable document? If the WG could not do this, how can we expect
an implementer to do this ?

This deliverable might have been good for the WG for tracking purposes but I
don’t think it works as an RFC for the intended target audience.

UPDATE: I've completed my review of -21:

#1:
             This is a
             very sensitive service and therefore needs specific
             authorization.  This authorization is with the CA
             certificate itself.  Alternatively, the CA MAY delegate the
             authorization by placing the id-kp-cmKGA extended key usage
             in the certificate used to authenticate the origin of the
             generated private key or the delegation MAY be determined
             through local configuration of the end entity.

These two MAYs are related, you MUST do one or the other. The text as it
can be interpreted to not perform either MAYs.

#2

   Such validity periods SHOULD
   NOT be used for protection of CMP messages and key generation.
   Certificates containing one of the above EKUs SHOULD NOT use
   indefinite expiration date.

This leaves a rather unspecified part on the implementer. What time period
is too much? Clearly something between a few seconds and indefinite, but what
is it? Can this document make a recommendation ?

#3

Throughout the document, Section references for the to-be-patched RFC are
turned into links for this RFC, eg in the text "Replace Section 5.1.3.4 -
Multiple Protection" in Section 2.6 where the section title has a bad link but
the section body has the right link. Please verify all of these references and
fix where needed.

#4

      It MAY
      include the original PKIMessage from the EE in the generalInfo
      field of PKIHeader of a nested message (to accommodate, for
      example, cases in which the CA wishes to check POP or other
      information on the original EE message).

If a CA wishes to do so, it would REQUIRE this original PKIMessage. Would it
not be better to say "It MUST include the original PKIMessage" ? It seems also
generally better to send the originals along with the modification so that the
next step can (optionally!) authenticate the previous step. Otherwise, there is
a lot of implied trust that should be modeled in the Security Considerations.

#5

In Section 2.20, it talks abot updating 4210's Section 7. It suggests removing
the first 3 paragraphs with replacement text. However, the text removed
describes the behaviour in a version agnostic way that I think is more clear
than the replacement text.

      If the client does not accept EnvelopedData, but EncryptedValue,
      then it MUST use cmp2000.

Why not cmp1999? Because EncryptedValue is valid for cmp1999 RFC2510 as well?
Are we assuming cmp1999 is completely dead and no longer deployed?

In general I would clarify section 2.20 better. More clearly subdivide client
and server, and leave a version of the text in Section 7 before section 7.1
intact.

Also, it seems the into in the original Section 7 really covers the protocol
behaviour. I am not sure why there are subsections with specific version
numbers in 4210 nor do I understand why this has to be patched to an even more
elaborate versioning, and mentioning EncryptedValue vs EncryptedEnvelope. It
seems the section 7 overview covers all behaviour already.

#6
Section 3.4 "patches" the IANA Considerations. I'd rather we didn't do this and
add a clear new IANA Considerations section with clear complete instructions to
IANA as to what changes to make, but I understand perhaps why to do this from a
readability point of view. But at the very least leave a note to the RFC Editor
to confirm all IANA Actions for this document are summarized in this document's
IANA Considerations.

    < TBD: The temporary registration of cmp URI suffix must be updated
   from provisional to permanent. >

IANA will do this when the document goes from draft to RFC. So this comment can
safely be removed.

   < TBD: A new protocol registry group "Certificate Management Protocol
   (CMP)" (at https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.iana.org%2Fassignments%2Fcmp&amp;data=05%7C01%7Chendrik.brockhaus%40siemens.com%7C82d56eb761c0488dd79f08da4b110393%7C38ae3bcd95794fd4addab42e1495d55a%7C1%7C0%7C637904833709102651%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=ZWPWJjKMrxeNsAKR8FepXTymrv4pQohGYwnQBo%2FYzNw%3D&amp;reserved=0) and an initial entry
   'p' must be registered. >

Same here.

Section 4 IANA Considerations should contain a copy of the "patch" instructions
as a clear instruction to IANA so they can make the changes without them
needing to "patch" the old RFC to obtain the instructions. Even if this sounds
redundant in this document.


----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

   As a next step, the LAMPS Working Group will consider providing
   RFC4210bis and RFC6712bis documents in order to offer the reader
   self-contained updated documents for CMP.

I don't think these kind of "instructions to the WG or IETF" should appear
in the document. But also, I would really like to see a commitment from the WG
that these bis documents will be created. But again, such a response should not
be stated within the document.

   The following updates are made in [thisRFC]:

I assume thisRFC refers to the current document being reviewed. But if that
text is replaced it would still be confusing. Maybe use: "[thisRFC] (this RFC)"
where [thisRFC] is replaced for the actual RFC and "(this RFC)" is literal text.

   a PKI management entity such
   as an RA MAY forward that message adding its own protection (which
   MAY be a MAC or a signature, depending on the information and
   certificates shared between the RA and the CA)

More confusing use of MAY. Was the intension to say "which MUST be a MAC or a
signature" ? If I am wrong, it perhaps need to say "MAY be a MAC or a signature
or nothing". But that seems unlikely to me. The first MAY also wouldn't need
all caps.

      A client not supporting cmp2021 will not be able to handle this
      situation and will fail or reject the certificate.

I don't understand the "or" here. Doesn't it fail because it rejects the
certificate? Can it reject a certificate and not fail ?

   When a CA acts as a CMP endpoint, it should not use the same private
   key for issuing certificates and for protecting CMP responses, to
   reduce the number of usages of the key to the minimum required.

What is "the key". It makes me read "CA" as the "CA certificate/key pair" but
since there can be multiple keys, I should read "CA" as the Certificate Agency
in general, not the single CA cert. Perhaps use "to reduce the different type
of usages of a particular key to a minimum" ? (Are we worried about different
types of usage, or actual just number of signing operations?)

   a CA
   certificate for use as a trust anchor, it MUST properly authenticate
   the message sender without already trusting any of the CA
   certificates given in the message.

If the EE already trusts CA X, and CA X is included in the message, the EE is
suddenly not allowed to use its preconfigured CA? Maybe rewrite to say "it MUSt
properly authenticate the message sender with existing trust anchors without
requiring new trust anchors included in the message".

   -- *  that the contents of "thisMessage" MUST be encoded either as
   -- *  "EnvelopedData" or "EncryptedValue" (only for backward
   -- *  compatibility) and then wrapped in a BIT STRING.  This
   -- *  allows the necessary conveyance and protection of the
   -- *  private key while maintaining bits-on-the-wire compatibility
   -- *  with RFC 4211 [RFC4211].

As EnvelopedData is only added in "this document" I think the last line
should be modified to say: with RFC 4211 [RFC4211] and [thisRFC]
(note, using TBDx instead of thisRFC might be helpfull to the RFC Editor)

       -- AlgId for a One-Way Function (SHA-1 recommended)

Can the part about recommending SHA-1 be removed, or does this document
really want to state SHA-1 is still recommended. I guess this might be
okay since it is describing the 1988 ASN.1 Module ?

   Note: This appendix will be deleted in the final version of the
   document.

Please rewrite as an instruction to the RFC Editor and put in [ square braces ]
for clarity.

Nits:

It updates  RFC 4210, RFC 5912, and RFC 6712, but skimming the ToC one can only
find a section on updates for 4210 and for 6712. It turns out updates to 5912
are in Appendix B. Perhaps change the title so this becomes more obvious in the
ToC?

   Such delegation MAY also be
   expressed by other means, e.g., explicit configuration.

I don't think that MAY needs to be in all caps? It is not part of the protocol
to implement, but a directive to a human operator.

   these OIDs MAY be re-used.

Same here, it explains why the authors of the RFC did this, so this MAY seems
silly at best

   5.1.3.4.  Multiple Protection

multiple proction what? I guess Multiple Proction in a message (or in a
request) ?

   To indicate support for EnvelopedData the pvno cmp2021 is introduced
   by this document.

The use of "this document" is super confusing due to this being a "patch
document". Use "has been introduced" instead?

   Note: To indicate support for EnvelopedData the pvno cmp2021 is
   introduced by this document.

Similar issue with "this document".

   Moreover

Maybe use "additionally" instead of Moreover?

    see CVE-2008-0166 [CVE-2008-0166];

Maybe just use the link instead of doubling the name in these references

cannot be measure -> cannot be measured

    the security of the
   shared secret information should exceed the security strength of each
   key pair.

I would say "of each individual key pair", so people are not confused in
thinking they might need to add up all the key strength bits.

    using pollReq and pollReq messages

That second pollReq should be pollRep.

    No changes are made to the existing security considerations of RFC 6712
    [RFC6712].

I think what is meant is "no security considerations updates of RFC 6712 were
required".