Conveying a Certificate Signing Request (CSR) in a Secure Zero Touch Provisioning (SZTP) Bootstrapping Request
draft-ietf-netconf-sztp-csr-14

I concur with Zahed's DISCUSS: The media types in the examples should be "application/yang-data+json", I believe.  There's currently "yang.data" instead of "yang-data".

Thank you to Yaron Sheffer for the SECDIR review.

** Section 4.1.1.  

For instance, an NMS controller/orchestrator
   application could periodically prompt the SZTP-client to generate a
   new private key and provide a certificate signing request (CSR) or,
   alternatively, push both the key and an identity certificate to the
   SZTP-client using

I don’t have a sense of the classes of endpoints that would rely on SZTP.  Would it include highly constrained or battery powered devices for which this re-keying would be too expensive?

** Editorial nits:
-- Section 2.1.  s/an new/a new/
-- Section 2.2.  Typo. s/Following are/The following are/
-- Section 4.1.1.  s/forever contain/contain/

A quick note to say thanks to the WG for this document, and also to Dan, Rob, the authors for the OpsDir review thread.

Thanks for addressing my Discuss and comments.

Thank you for the work put into this document.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Mahesh Jethanandani for the shepherd's write-up even if the section about the WG consensus does not give any information about the WG consensus...

I hope that this helps to improve the document,

Regards,

-éric
== COMMENTS ==

-- Section 1.2 --
Just wondering why the terms of SZTP-client/server have to be defined in the document rather than using the previously defined bootstrap server.

-- Section 1.4 --
Even if the base64 content is rather long, why not providing examples in appendix ?

-- Section 2.1 --
I am not a YANG expert, but it seems that the lines "module: ietf-sztp-csr" should only appear once in the tree view ?

In "generate an new asymmetric key", should it rather be a "key pair" ? Of course, being asymmetric requires two keys so this may be implicit.

-- Section 2.2 --
As P10 is not previously defined (guessing it is PKCS#10 though), suggest s/P10-based/PCKS#10-based/

In the example for the algorithm leaves, are the base64 values really too long to be represented in the examples ?

== NITS ==
In section 3, "This module is defines independently" ?

Thanks for this document; it seems useful to have the flexibility to
specify a CSR and get an LDevID during enrollment.

I had a similar concern as Zahed's Discuss point (though I'm not sure
I can really express my concerns very well yet), and will attempt to follow
the discussion thereof.

Section 2.1

Why do we need the "sztp-csr" namespace prefix on the new nodes in the
get-bootstrapping-data example, but not need the "ietf-sztp-csr" prefix
on the nodes in the ietf-restconf/errors example?

Section 2.3

           description
             "Provides the CSR generated by the SZTP-client.

              When present, the SZTP-server SHOULD respond with
              an SZTP onboarding information message containing
              a signed certificate for the conveyed CSR.  The
              SZTP-server MAY alternatively respond with another
              HTTP error containing another 'csr-request', in
              which case the SZTP-client MUST invalidate the
              previously generated CSR.";

I'm curious what is intended by "invalidate the [...] CSR".  My
background on CSR usage hasn't really encountered a sense of "validity"
for them that would need cancellation, since it's not really a important
piece of information in its on right (just a way to ask a CA to do
something).  The interesting bit is whether the CA actually issued
anything, but I'm not sure how much harm a stale CSR would actually
cause.

Section 3.2

     identity cmp-csr {
       base certificate-request-format;
       description
         "Indicates that the ZTP-client supports generating
          requests using a constrained version of the PKIMessage
          containing a p10cr structure defined in RFC 4210.";

I'd probably spend a few more words to indicate the nature of the
constraints that cause us to say "constrained version".
(Likewise for the cmc-csr.)

            Enables the ZTP-server to provide a fully-populated
            CertificationRequestInfo structure that the ZTP-client
            only needs to sign in order to generate the complete
            'CertificationRequest' structure to send to ZTP-server
            in its next 'get-bootstrapping-data' request message.

            When provided, the ZTP-client SHOULD use this structure
            to generate its CSR; failure to do so MAY result in a
            400 Bad Request response containing another 'csr-request'
            structure.

This guidance seems to risk running afoul of the first rule of PKIs:
know what you sign.  While I understand that the context for this
protocol exchange is a relatively "dumb" device getting onboarded into
the owner's deployment, and that the owner may in fact require some
things in the LDevID that were not coded into the device by its
implementors, I don't think we should stand mute on the security
considerations of asking for a certificate that contains
attributes/restrictions/etc. that are not understood.

           leaf cmc-csr {
             type binary;
             description
               "A constrained version of the 'Full PKI Request'
                message defined in RFC 5272, encoded using ASN.1

(Same comment about "constrained version" as above.)

                For asymmetric key-based origin authentication based on
                the initial device identity certificate's private key
                that signs the encapsulated CSR signed by the local
                device identity certificate's private key, the PKIData
                contains one cmsSequence element and no
                otherMsgSequence element.  The cmsSequence is the

This part says nothing about whether there is a reqSequence element in
the toplevel PKIData...

                TaggedContentInfo and it includes a bodyPartID element
                and a contentInfo.  The contentInfo is a SignedData
                encapsulating a PKIData with one reqSequence element
                and no cmsSequence or otherMsgSequence elements. The
                reqSequence is the TaggedRequest and it is the tcr
                CHOICE. The tcr is the TaggedCertificationRequest and
                it a bodyPartId and the certificateRequest elements.
                [...]

... since this reqSequence seems to refer to the PKIData inside the
SignedData in the contentInfo in the cmsSequence.  Should we say
anything about the presence/absence of reqSequence in the toplevel
PKIData (since we do in the other two cases)?

         case cmp-csr {
           leaf cmp-csr {
             type binary;
             description
               [...]
                For asymmetric key-based origin authentication of a
                CSR based on the initial device identity certificate's
                private key for the associated initial device identity
                certificate's public key, PKIMessages contains one
                PKIMessage with the header and body elements, no
                protection element, and should contain the extraCerts
                element. [...]

Is this a "should" or a "SHOULD"?

                For asymmetric key-based origin authentication based on
                the initial device identity certificate's private key
                that signs the encapsulated CSR signed by the local
                device identity certificate's private key, PKIMessages
                contains one PKIMessage with the header, body, and
                protection elements, and should contain the extraCerts
                element. [...]

(ditto)

Section 4

I'd consider also mentioning the CMC, CMP, and PKCS#10 security
considerations as being applicable to the relevant CSR choices.

Section 4.1.1

   The security of this private key is essential in order to ensure the
   associated identity certificate can be used as a root of trust.

"root of trust" is something of a loaded term (see the ongoing
discussion in the RATS WG for differing interpretations of what it
means).  I'd suggest using a different phrasing, like "can be used to
authenticate the device it is issued to".

Section 4.1.3

Should we mention the "nonce" parameter to get-bootstrapping-data
here?

   When a public/private key pair associated with the manufacturer-
   generated identity certificate (e.g., IDevID) is used for the
   request, there may not be confirmation to the SZTP-client that the
   response has not been replayed; however, the worst case result is a
   lost certificate that is associated to the private key known only to
   the SZTP-client.

That's only the worse-case result if we assume that the private key is
not compromised.  We might want to reiterate that assumption and/or
mention the scope of consequences for the case where the private key is
compromised.

Section 4.2.2

   When a new asymmetric key is used, with the CMP or CMC formats, the
   parent ASN.1 structure of the CSR provides origin authentication
   using either the manufacturer-generated private key or a shared
   secret.  In this way the proof-of-possession of the CSR is directly
   linked to the proof-or-origin provided by the parent ASN.1 structure.

I think this section would be a good place to talk about how in the "raw
PKCS#10" case, there needs to be a level of trust in the bootstrapping
server roughly analogous to how an RA would be trusted, in that they are
the only entity that has verified the client identity and the
bootstrapping server's assertion of the client identity is a key step in
the certificate issuance process.

Such discussion might also contrast use of the IDevID at the TLS layer
with HTTP-layer authentication of the client.

Section 4.4

Though our "identity" and "grouping" statements (as noted) to not appear
in any protocol-accessible nodes as-is, we might still feel empowered to
discuss any considerations that would apply when they actually are
instantiated in other modules.  However, it seems that there would not
be anything new to say other than what's in RFCs 2986, 4210, and 5272,
and if we take my suggestion to reference them from the toplevel
section 4, there would be no need to do so again here.

NITS

Abstract, Introduction

I'd consider clarifying that when we "extend" the get-bootstrapping-data
RPC to include a CSR, that's as the input to the RPC, not the output.
(I initially misread it when reading the abstract.)

Section 2.2

You may want to use more-recent dates than 2015 for the examples.

I suggest using different "BASE64VALUE="s for the different public
keys in the two-asymmetric-key example.

Section 3.2

             leaf-list algorithm-identifier {
               type binary;
               min-elements 1;
               description
                 "An AlgorithmIdentifier, as defined in RFC 2986,
                  encoded using ASN.1 distinguished encoding rules
                  (DER), as specified in ITU-T X.690.";

At risk of excessive pedanticism, I see RFC 2986 discuss the
parametrized AlgorithmIdentifier{}, which compares it to the
unparameterized AlgorithmIdentifier in the ensuing discussion.
AlgorithmIdentifier is (also) defined in, e.g., RFC 5280 if we didn't
want to get into the parameterization here.  (Any change made here
should be made throughout, of course.)

     grouping csr-grouping {
       description
         "Enables a ZTP-client to convey a certificate signing
          request, using the encoding format selected by a
          ZTP-server's 'csr-request' response to the ZTP-client's
          previously sent 'get-bootstrapping-data' request
          containing the 'csr-support' node.";

(nit) Since we're (now) in just the ztp-types module, we can't
necessarily assume there will be a "get-bootstrapping-data" request
involved.

                For asymmetric key-based origin authentication of a
                CSR based on the initial device identity certificate's
                private key for the associated identity certificate's
                public key, the PKIData contains one reqSequence

(nit) I think the "associated identity certificate's public key"
phrasing here is a bit confusing.  I think that we're trying to cover
the case where the key from the IDevID is reused for the LDevID, so
we're using the LDevID private key to authenticate the CSR and it's
sigining "it's own" public key", but in the context of this YANG
grouping we've mostly lost the context of the "associated identity
certificate".
(A similar comment applies to the CMP case.)

                elements. The reqSequence is the TaggedRequest and it
                is the tcr CHOICE. The tcr is the

(nit) should we have another word after "CHOICE", like "arm" or
"branch"?

                TaggedCertificationRequest and it a bodyPartId and the

(nit) also, I'd maybe s/the/a/ for these two "the <foo> is the <bar>"
constructions since they seem to just be describing the type of a given
ASN.1 element.
Also, s/it/it is/.

Section 4.1.1

   private keys.  For instance, an NMS controller/orchestrator
   application could periodically prompt the SZTP-client to generate a
   new private key and provide a certificate signing request (CSR) or,
   alternatively, push both the key and an identity certificate to the
   SZTP-client using, e.g., a PKCS #12 [RFC7292].  [...]

maybe "a PKCS #12 message"?

Section 4.1.5

   The CMP and CMC certificate request formats defined in this document
   support origin authentication.  A raw PKCS#10 does not support origin
   authentication.

"PKCS#10 CSR"

Reference [I-D.ietf-netconf-crypto-types] from this Proposed Standard to
draft-ietf-netconf-crypto-types of unknown standards level. That should be
fixed in the datatracker for draft-ietf-netconf-crypto-types...

Document still refers to the "Simplified BSD License", which was corrected in
the TLP on September 21, 2021. It should instead refer to the "Revised BSD
License".

Thanks to Meral Shirazipour for their General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/4XKQuQMRJ3xEca_Le-NpYcF-RYI).

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 4.1.1. , paragraph 5, nit:
-    private key and associated identity certificates and reexecution of
+    private key and associated identity certificates and re-execution of
+                                                           +

Section 2.1. , paragraph 5, nit:
> ver that it supports the ability the generate CSRs. This parameter conveys if
>                                  ^^^^^^^^^^^^
After "the", the verb "generate" doesn't fit. Is "generate" spelled correctly?
If "generate" is the first word in a compound adjective, use a hyphen between
the two words. Using the verb "generate" as a noun may be non-standard.

Section 2.1. , paragraph 5, nit:
>  the SZTP-client is able to generate an new asymmetric key and, if so, which
>                                      ^^
Use "a" instead of "an" if the following word doesn't start with a vowel sound,
e.g. "a sentence", "a university".

Section 3.2. , paragraph 20, nit:
>  the TaggedCertificationRequest and it a bodyPartId and the certificateReque
>                                     ^^^^
A verb may be missing between "it" and "a", or a word may be misspelled.

Section 3.2. , paragraph 20, nit:
>  the TaggedCertificationRequest and it a bodyPartId and the certificateReque
>                                     ^^^^
A verb may be missing between "it" and "a", or a word may be misspelled.

Section 3.2. , paragraph 21, nit:
>  the TaggedCertificationRequest and it a bodyPartId and the certificateReque
>                                     ^^^^
A verb may be missing between "it" and "a", or a word may be misspelled.

Section 4.1.5. , paragraph 7, nit:
> in Wu. Contributors Special thanks goes to David von Oheimb and Hendrik Broc
>                                    ^^^^
It seems that the correct verb form here is "go".

Document references draft-ietf-netmod-factory-default, but that has been
published as RFC8808.

Document references draft-ietf-netconf-keystore-22, but -23 is the latest
available revision.

Document references draft-ietf-netconf-trust-anchors-15, but -16 is the latest
available revision.

These URLs point to tools.ietf.org, which is being deprecated:
 * http://tools.ietf.org/wg/netconf

These URLs in the document did not return content:
 * http://standards.ieee.org/findstds/standard/802.1AR-2018.html