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

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

Dear Paul

Many thanks for reviewing the CMP Updates draft and providing valuable feedback.
Please find below the response of the authors. If you have any feedback, please let us know.
I will try to submit the updates draft as soon as possible.

I am looking forward on the telechat next Thursday :-)

Hendrik

> Von: Paul Wouters via Datatracker <[e-mail]>
> 
> ----------------------------------------------------------------------
> 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.

Also including Carl's comment:
> >
> > [CW] I recognize this is a late comment and that there are no words 
> > to borrow in either RFC6402 or RFC6960, but adding a security 
> > consideration that highlights the need to authorize certificate 
> > requests including these new EKUs (not just id-
> > kp-cmKGA) may be worthwhile. This would mirror the "therefore needs 
> > specific authorization" in the above snip but apply to the act of 
> > requesting delegation via the EKUs.

I rephrased the sentence and remove the 'or'.

New text:
   This is a
   very sensitive service and therefore needs specific
   authorization.  This authorization is with the CA
   certificate itself.  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. Alternatively, the authorization MAY be determined
   through local configuration of the end entity.

In response to Carl's comment I propose adding a security consideration. This 
inserts a new Section after Section 2.24 with the following text. I also propose 
adding a sentence regarding validity periods based on the text in #2.

2.25.  Add Section 8.7 - Authorizing requests for certificates with specific EKUs

   The following subsection addresses the security considerations to follow
   when authorizing requests for certificates containing specific EKUs.

   Insert this section after new Section 8.6:

   8.7.  Authorizing requests for certificates with specific EKUs

   When a CA issues a certificate containing extended key usage extensions as
   defined in Section 4.5, this expresses delegation of an authorization that
   originally is only with the CA certificate itself. Such delegation is a very sensitive
   action in a PKI and therefore special care must be taken when approving such
   certificate requests to ensure that only legitimate entities receive a certificate
   containing such an EKU.

   Certificates containing any of the above EKUs SHOULD NOT use the 'indefinite'
   notAfter date 99991231235959Z. Recommendations on server certificate validity
   by CA-Browser Forum may be used as guidance.

> 
> #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 ?

Rephrased, new text:
   Such validity periods SHOULD
   NOT be used for protection of CMP messages and of centrally generated keys.
   Certificates containing any of the above EKUs SHOULD NOT use the 'indefinite'
   notAfter date 99991231235959Z. Recommendations on server certificate validity by
   CA-Browser Forum may be used as guidance.

I will also add an informative reference for CA-Browser Forum Baseline Requirements.

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

I see your point when I look at datatracker https://datatracker.ietf.org/doc/html/draft-ietf-lamps-cmp-updates#section-2.6 
and I am sorry for such wrong references.
When I look at https://www.ietf.org/archive/id/draft-ietf-lamps-cmp-updates-21.html#name-replace-section-5134-multip
the layout seems OK.
The issue also occurred with the PDF version on datatracker, but it is not there with the 
pdf I produced with xml2rfc.
In the xml document I only used <xref ... /> for references to sections of this document. 
References in the text that are to be inserted in the to-be-patched documents are not 
provided as xref in the xml but laid out like this by the tool chain. I do not know how to 
circumvent this. My hope is that the final version provided by the RFC-Editors will not 
contain such issues. I will definitely check this in Auth48.

If anyone has any guidance what to do in advance to prevent this, please let me know.

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

You are right, if the CA wants to check the POP, it is required to provide the 
original message in the PKIHeader. Generally speaking, providing the original 
message is optional when the POP is set to RAVerified. Using RAVerified", the 
respective RA indicates that it already verified the POP.
I rephrased the example to indicate that in case the CA policy requires to again 
check the POP at CA level, the original message must be provided in the PKIHeader 
by the RA.

New text:
   It MAY
   include the original PKIMessage from the EE in the generalInfo
   field of PKIHeader of a nested message. For example, this is
   REQUIRED to accommodate cases in which the CA wishes to
   check POP or other information on the original EE message.

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

We had to introduce cmp2021 to indicate those cases where the ASN.1 syntax 
changed. These are:
(a) EncryptedKey is used instead of EncryptedValue to facilitate using EnvelopedData
(b) the new hashAlg field is used in CertStatus structures of certConf messages In all 
     other cases new syntax is indicated using OIDs and no new CMP version is required.

The version negotiation defined in RFC4210 is most general and rather 
straightforward but has the drawback that if a client already supports cmp2021 
while a server does not, an extra round trip just for version negotiation is required, 
even though for most messages cmp2000 is still sufficient. For backward compatibility 
while minimizing the need for an extra round trip just for version negotiation, cmp2021 
should only be used in cases where the new ASN.1 syntax is actually needed.

There are cases where the client needs to indicate capability of understanding
cmp2021 also without using the new syntax in the request message to tell the server 
that it is capable understanding the new syntax, e.g., when the clients request the 
server to generate the key pair centrally. 

There are two cases where the client should use cmp2021 in a request message even 
without actually using the new ASN.1 syntax in this request message. This is needed to 
tell the server that it can understand the cmp2021 ASN.1 syntax. These are:
(a) the client requests central key pair generation and expects the server to respond 
     using an EnvelopedData structure for conveying the private key
(b) the client sends a certificate request and expects the server to respond with a 
     certificate encrypted using an EnvelopedData structure (for indirect proof-of-possession).

Therefore, the version handling of cmp2021 is a bit complex. Following your guidance we 
managed to simplify the text of Section 7 dramatically. We finally only extended the 
second paragraph and added a note. For ease of reading I add the complete text of Section 7.
Sections 7.1 stays unchanged.

New text:
7.  Version Negotiation

   This section defines the version negotiation used to support older
   protocols between client and servers.

   If a client knows the protocol version(s) supported by the server
   (e.g., from a previous PKIMessage exchange or via some out-of-band
   means), then it MUST send a PKIMessage with the highest version
   supported by both it and the server.  If a client does not know what
   version(s) the server supports, then it MUST send a PKIMessage using
   the highest version it supports, with the following exception. Version
   cmp2021 SHOULD only be used if cmp2021 syntax is needed for the
   request being sent or for the expected response.

   Note: Using cmp2000 as the default pvno is done to avoid extra message
   exchanges for version negotiation and to foster compatibility with 
   cmp2000 implementations. Version cmp2021 syntax is only needed if a
   message exchange uses hashAlg (in CertStatus) or EnvelopedData.

   If a server receives a message with a version that it supports, then
   the version of the response message MUST be the same as the received
   version.  If a server receives a message with a version higher or
   lower than it supports, then it MUST send back an ErrorMsg with the
   unsupportedVersion bit set (in the failureInfo field of the
   pKIStatusInfo).  If the received version is higher than the highest
   supported version, then the version in the error message MUST be the
   highest version the server supports; if the received version is lower
   than the lowest supported version then the version in the error
   message MUST be the lowest version the server supports.

   If a client gets back an ErrorMsgContent with the unsupportedVersion
   bit set and a version it supports, then it MAY retry the request with
   that version.

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

I will rephrase the text in Section 4 IANA Consideration and copy the instructions provided in Section 2.25 and Section 3.4.
I will also remove the todos from the draft.

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

Thank you for the clarification. I will remove this sentence from 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.

I like this proposal and change to the literal text 'this RFC' or 'this document' in 
full text sections. In the ASN.1 modules the replacement with the RFC number is 
still needed because the may be used also outside the document.

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

This text portion mainly originates from RFC4210, but I see your point.

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

> 
>       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 ?

This text in not part of the Section 7 anymore.

> 
>    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?)

I am worried about both, the number of times the private key of the CA signing 
certificate is used as well as exposing it to other SW systems, e.g., for CMP 
protection or TLS handshake.

New text:
   A CA should not reuse its certificate signing key for other purposes
   such as protecting CMP responses and TLS connections. This way, exposure
  to other parts of the system  and the number of uses of this particularly
  critical key is reduced to a minimum.

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

Thank you for this proposal.

New text:
   a CA
   certificate for use as a trust anchor, 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)

RFC4211 defines EncryptedKey already introduced the flexibility for using EnvelopedData 
instead of EncryptedValue. By some reason, it was not used in RFC4210 back then.
If you dislike the referring to RFC4211 we could also refer to RFC4210 and [TBDx] instead.

New text:
   -- *  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 RFC4210 and [TBDx].

> 
>        -- 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 ?

I will remove this as SHA-1 is not listed as recommend hash algorithm in CMP Algorithms.

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

Thank you for this hint.

New text:
[RFC Editor: This appendix must be deleted in the final version of the document.]

> 
> 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?

Thank you for this suggestion. I will rephrase the title of Section A.1 and Section A.2

New section titles:
A.1. Update to RFC4210 - 1988 ASN.1 Module 
A.2. Update to RFC5912 - 2002 ASN.1 Module

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

Agree.

New text:
   Such delegation may also be
   expressed by other means, e.g., explicit configuration.

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

Right.

New text:
   Note: RFC 6402 section 2.10 [RFC6402] specifies OIDs for a CMC CA and
   a CMC RA.  As the functionality of a CA and RA is not specific to
   using CMC or CMP as the certificate management protocol, theses EKUs
   are re-used by CMP.

> 
>    5.1.3.4.  Multiple Protection
> 
> multiple proction what? I guess Multiple Proction in a message (or in a
> request) ?

This is the original title as of RFC4210. I think renaming it is not required as 
this section is about adding additional protections to a CMP message.
If no one opposes, I would keep this wording.

> 
>    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?

You are right. This does not makes sense here.

New text:
   To indicate support for EnvelopedData the pvno cmp2021 has been
   introduced.

> 
>    Note: To indicate support for EnvelopedData the pvno cmp2021 is
>    introduced by this document.
> 
> Similar issue with "this document".

New text:
   Note: To indicate support for EnvelopedData the pvno cmp2021 has
   been introduced.  Details on the usage of different pvno values are
   described in Section 7.

> 
>    Moreover
> 
> Maybe use "additionally" instead of Moreover?

I will update the four places 'moreover' occurs with 'additionally'.

> 
>     see CVE-2008-0166 [CVE-2008-0166];
> 
> Maybe just use the link instead of doubling the name in these references

I will do so.

> 
> cannot be measure -> cannot be measured

Thanks, done.

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

Thank you for this proposal

New text:
   the security of the
   shared secret information should exceed the security strength of each
   individual key pair.

> 
>     using pollReq and pollReq messages
> 
> That second pollReq should be pollRep.

Thank you for spotting this :-)

New text:
    using pollReq and pollRep messages

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

Thanks for this proposal:

New text:
    No security considerations updates of RFC 6712 [RFC6712] were required.