Certificate Management Protocol (CMP) Updates
draft-ietf-lamps-cmp-updates-23

Thank you for the work on this document.

I have a few minor comments, hopefully easy to fix; answers are appreciated.

Francesca

1. -----

   previous PKI management operation).  PKIProtection will contain a MAC
   value and the protectionAlg MAY be one of the options described in
   CMP Algorithms [I-D.ietf-lamps-cmp-algorithms].  The PasswordBasedMac

FP: I think the correct term here is MUST rather than MAY, otherwise this seem to imply that the protectionAlg can be something different as well.

2. -----

   Note: In case several EC curves are supported, several id-ecPublicKey
   elements need to be given, one per named curve.

FP: I could not find id-ecPublicKey in RFC 4210, could you give more context where this element is defined?

3. -----

Section 2.25 and 3.4 - IANA considerations

FP: Given that Section 4 does now a full update of the IANA considerations (as a result from Paul's comment, which I believe was a necessary improvement), it seems to me as Section 2.25 and 3.4 have become useless. I suggest to just remove those to avoid the redundancy (and the risk for future updates that will modify one section but not the other).

4. -----

   [RFC4210].  This document redirects to the new algorithm profile as
   specified in Appendix A.1 of CMP Algorithms
   [I-D.ietf-lamps-cmp-algorithms].

...

   For specifications of algorithm identifiers and respective
   conventions for conforming implementations, please refer to CMP
   Algorithms Appendix A.1 [I-D.ietf-lamps-cmp-algorithms].

FP: There is no Appendix A.1 of [I-D.ietf-lamps-cmp-algorithms]. Did you mean Section 7?

5. -----

FP: Nits reports the following: 

  == Unused Reference: 'RFC2510' is defined on line 1580, but no explicit
     reference was found in the text

RFC 2510 does appear in the document, but only in the section header, I would suggest adding the reference in the text as well.

I continue to regret the chosen format but given that (a) taken in context there’s a need for publication as an RFC sooner rather than later, and (b) the WG has committed to promptly produce a clean version, I’m changing my ABSTAIN to NOOBJ. 

I’m a little concerned that there’s some risk the subsequent clean version will receive more intensive review and feedback than the WG might otherwise have expected based on the premise it only represents the output of applying the patches specified in this document (which will have been approved) over the base document. I think there’s a decent chance that reviewers (including me) who haven’t seen fit to expend the effort to do a thorough review of this version, may do so with the planned version. Just a heads-up.

My DISCUSS items have been resolved, thanks for the updates to the document.
A copy of DISCUSS and Comments is included below:

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://www.iana.org/assignments/cmp) 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.


Comments:


   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".

[  2022-06-29 -- edit to simply reiterate that I really don't like to "format" of this, but that I think don't want to block publication or cause the WG more work for what presumably feels like "moving the goalposts" ] 
I think understand why the WG did this - from Russ' shepherd writeup it seems that this path was chosen when the changes were not quite expected to be this numerous / complex and that is snowballed.

There has previously been advice / evidence that 1: taking an existing RFC and making a -bis runs the risk of re-litigating all of the existing text and the vagaries of the sitting IESG and 2: people kvetch if you just change a few things and publish it as a new document - it looks like you are trying to take credit for other people's work, it can be hard to tell what actually changed, etc.

And so, even though I don't really like the document / really don't like the document, I feel that the authors / WG was following advice that they had received, and so I'm, with misgivings, balloting No Objection instead of Abstain or DISCUSS. I'll note that it's easier for me to take this somewhat sanctimonious moral high ground when there are other people holding the Abstains / Discusses -- I don't actually know what I would ballot if they were not holding them...

Anyway, thank you very much to the authors, WG, shepherd and OpsDir reviewer for the hard work on the document -- it is clear that there has been lots of work, all done with the best of intentions.

I share the concerns from my fellow ADs. This is a long diff/change document, which not nice to read and can lead to confusion. I ,however, see this as a short term fix and want to believe the implementer's and consumers of this doc have given their consent to publish the document like it is . So I am balloting no objection.

That said, this document need to describe the motivation to this publication. so why updates and why not a -bis? to bring us on the same page. The shepherd's write up hints the plan and reason but I would suggest to add those reasoning and plans in the document itself.

With the recent addition of milestones for the LAMPS WG to produce bis versions for the various I-Ds updated by this document, I have decided to move to a "No Objection" ballot.

Thanks for addressing my DISCUSS about downgrade attacks.

I support Paul's DISCUSS.

(2.13) IIUC, this section should also require the cmp2021 version.

I found it quite confusing that this document is specifying two different versions, with version 3 diffs scattered throughout the draft. It would be much cleaner to have a clean definition of the revised v2, and then a new section that defines v3 with the small diff from v2.

I support Paul's discuss.

Hopefully, given all the hard work that has been done, it shouldn't be too hard to respin -bis versions of the RFCs instead of publishing this document.

Regards,
Rob