Re: [netconf] Benjamin Kaduk's No Objection on draft-ietf-netconf-sztp-csr-12: (with COMMENT)

[Kent Watsen <[e-mail]>]

Hi Ben,

Thank you for your thoughtful review.  Responses to your comments are below.

Also, diffs are here: https://github.com/netconf-wg/sztp-csr/commit/ee5b28d35ec26c895ce4691f85affd1487d51f3e <https://github.com/netconf-wg/sztp-csr/commit/ee5b28d35ec26c895ce4691f85affd1487d51f3e>.

Best regards,
Kent (and Russ and Sean)


> On Dec 14, 2021, at 7:09 PM, Benjamin Kaduk via Datatracker <[e-mail]> wrote:
> 
> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-netconf-sztp-csr-12: No Objection
> 
> 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://www.ietf.org/blog/handling-iesg-ballot-positions/
> for more information about how to handle DISCUSS and COMMENT positions.
> 
> 
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-netconf-sztp-csr/
> 
> 
> 
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
> 
> 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.

Zahed’s discuss is ongoing, but I think we’re just an edit away now.


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

You write “example” but I think you mean “tree diagrams”.  The tree-diagram at the top of page 7 is stitched together via some scripts.  Technically, the parent node, "error-info”, is of type “yang:anydata”, which is a YANG for “anything, no data model needed, can go here”, which means what is there now isn’t wrong.  That said, your observation is valid, the dichotomy is stark.  I have updated the scripts to prefix all those nodes the same as in the earlier tree diagram.  Since you cannot see what it looks like from the GitHub diff, the new rendered view follows:

   module: ietf-restconf
     +--ro errors
        +--ro error* []
           +--ro error-type       enumeration
           +--ro error-tag        string
           +--ro error-app-tag?   string
           +--ro error-path?      instance-identifier
           +--ro error-message?   string
           +--ro error-info
              +--ro sztp-csr:csr-request
                 +--ro sztp-csr:key-generation!
                 |  +--ro sztp-csr:selected-algorithm
                 |     +--ro sztp-csr:algorithm-identifier    binary
                 +--ro sztp-csr:csr-generation
                 |  +--ro sztp-csr:selected-format
                 |     +--ro sztp-csr:format-identifier    identityref
                 +--ro sztp-csr:cert-req-info?    ct:csr-info




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

The intention of “invalidate” was to, e.g., delete the private key that may have been generated specifically for the previous CSR.  Thinking about this now, the authors are of the opinion that deleting the private key is likely all that any SZTP-client could ever do.  That being the case, we suggest:

OLD: the SZTP-client MUST invalidate the previously generated CSR.
NEW: the SZTP-client MUST delete any key generated for the previously generated CSR.

Makes sense?



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

The authors think that “profiled” is a better word than “constrained”, as so suggest:

   description
     "Indicates that the ZTP-client supports generating
      requests using a profiled version of the PKIMessage
      that MUST contain a PKIHeader followed by a PKIBody
      containing only the p10cr structure defined in RFC 4210.";

Thoughts?



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

After a little discussion, the authors are unable to understand a scenario where it makes sense that a ZTP-client would disregard and/or decorate/extend the CertificationRequestInfo structure from the ZTP-server.  That being the case, we recommend s/SHOULD/MUST/ and s/MAY/will/ as follows:

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

Better?



>           leaf cmc-csr {
>             type binary;
>             description
>               "A constrained version of the 'Full PKI Request'
>                message defined in RFC 5272, encoded using ASN.1
>                distinguished encoding rules (DER), as specified
>                in ITU-T X.690.
> 
> (Same comment about "constrained version" as above.)


The authors again think that “profiled” is a better word than “constrained”, as so suggest:

    description
            "A profiled version of the 'Full PKI Request'
             message defined in RFC 5272, encoded using ASN.1
             distinguished encoding rules (DER), as specified
             in ITU-T X.690.

Thoughts?



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

The authors propose:

OLD: and no otherMsgSequence element. 
NEW: and  no controlSequence, reqSequence, or otherMsgSequence elements. 

Good?


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

Yes, the authors propose:

OLD: […]

NEW:   PKIData contains one cmsSequence element and no
            controlSequence, reqSequence, or otherMsgSequence
            elements. […]


Good?



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


Yes.  Fixed.


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

Also now a "SHOULD” here.



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

Silly authors, we forgot the apple pie  ;)

We added:

   For the various CSR formats, when using PKCS#10, the security
   considerations in [RFC2986] apply, when using CMP, the security
   considerations in [RFC4210] apply and, when using CMC, the security
   considerations in [RFC5272] apply.



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

Fix applied - thanks!


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

Hmmm, if we were to mention the “nonce” parameter at all, it would only be to say that it has *nothing* to do with the CSR mechanism presented in this document…or, at least it doesn’t have any relation currently.  To be clear, the “nonce” parameter is defined in RFC 8572 (SZTP) as follows:

      leaf nonce {
        type binary {
          length "16..32";
        }
        description
          "This optional input parameter enables a device to
           communicate to the bootstrap server a nonce value.
           This may be especially useful for devices lacking
           an accurate clock, as then the bootstrap server
           can dynamically obtain from the manufacturer a
           voucher with the nonce value in it, as described
           in RFC 8366.";
        reference
          "RFC 8366:
             A Voucher Artifact for Bootstrapping Protocols";
      }

Thoughts?



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

Good point.  We appended the following to the end of the paragraph:

	Protection of the private-key information is vital to public-key
	cryptography.  Disclosure of the private-key material to another
	entity can lead to masquerades.

Better?


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

The authors propose replacing the paragraph above with the following:

     When the bootstrapping device's manufacturer-generated private key
     (e.g., the IDevID key) is reused for the CSR, proof-of-origin is
     verified by validating the IDevID certification path and ensuring that
     the CSR uses the same key pair.

     When a fresh asymmetric key is used with the CMP or CMC formats, the
     authentication is part of the protocols, which could employ either
     the manufacturer-generated private key or a shared secret.  In addition,
     CMP and CMC support processing by a RA before the request is passed
     to the CA, which allows for more robust handling of errors.

Thoughts?



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

The authors believe this is best addressed with more “apple pie” in the top-level of Section 4 (Security Considerations):

   For the various authentication mechanisms, when using TLS-level
   authentication, the security considerations in [RFC8446] apply and,
   when using HTTP-level authentication, the security considerations in
   [RFC7235] apply.

Would this satisfy your discuss?



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

We put the “apple pie” into the top-level of Section 4.


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

Done.


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

Done.


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

Not fixed, given:

1.4.  Conventions

   Various examples used in this document use a placeholder value for
   binary data that has been base64 encoded (e.g., "BASE64VALUE=").
   This placeholder value is used as real base64 encoded structures are
   often many lines long and hence distracting to the example being
   presented.


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

Not fixed, since the document refs RFC 2986 for the CSR structure, so having a forward ref to RFC 5280 doesn’t seem right to us.  Thoughts?


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

Good catch.  Fixed.


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

Not fixed, due to complexity and that this is just a “nit”.  More detail, one author wrote:

"We have gone back and forth on the text a couple of times. I am not sure I won’t just revert to what we had before.  I will note that if we make a change here we have to change it 6 different times, 3X for CMC and 3X for CMP. The phrasing was specifically done this way and amended along the way to make it clear which key is doing what.  We got three cases two are asymmetric-based and one is symmetric-based. We needed to differentiate between the 1st two, and that convoluted text is how we did it."


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

Done. used “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/.

All done.


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

Fix applied.


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

Done.



Thanks again!
Kent (and Russ and Sean)