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

[Benjamin Kaduk <[e-mail]>]

Hi Kent (and Russ and Sean),

Comments inline.

On Tue, Dec 21, 2021 at 05:55:13PM +0000, Kent Watsen wrote:
> 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:

You're right, I must have meant "tree diagrams"; sorry about that.
Thanks for the explanation and updates!

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

Yes, that makes sense.  [comment about "overkill" removed since we use the
single-use property for replay detection]

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

That seems to flow better to me and not leave me looking for more content
somewhere else.  So, looks like a clear improvement.

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

Those changes are okay in terms of the protocol operations.
I think there are probably still some security considerations to document,
in that the ZTP-client is trusting the ZTP-server to tell it what to sign
(in the CSR).  The ZTP client trusts the ZTP server for equally sensitive
things already, so this seems to  mostly be a documentation issue: "the ZTP
server has full control over the contents of the CSR that the ZTP client
generates (except for the actual private key), which can result in the
client receiving a certificate with permissions or restrictions that it
does not recognize.  The ZTP server is already a trusted part of the
ecosystem and it is natural to extend that trust by relying on it to give
the ZTP client a CSR template that will be appropriate for its
implementation and role in the network."

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

Sounds good.  It looks like the "constrained" phrasing is also present in
the identity cmc-csr; presumably we should make an analogous change there
and anywhere else relevant.

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

Yup.

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

I don't think we mention controlSequence anywhere else at present (though
it is mentioned in the "NEW" text above).  I don't think the omission
seemed noteworthy when I was doing my original review, so my primary
consideration here is just that we give it consistent treatment everywhere
(in terms of whether or not we mention it).

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

I did see (after I originally wrote the remark) that the "nonce" is not new
in this document...but since nonces are a common way to prevent replay
attacks, I figured I would still ask the question.
The 8572 discussion covers the nonce going into the voucher to
detect/prevent replay.  I suppose in principle that one could also have a
setup that puts the nonce into the certificate as well, but I don't expect
that to be common.  So my thinking is that we should consider putting in
some text along the lines of "though the get-bootstrapping-data RPC does
contain a 'nonce' parameter, it is defined by RFC 8572 for use in
preventing replay of a voucher; that mechanism is unrelated to detecting
replay of the certificate that can be returned by the
get-bootstrapping-data RPC".

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

Yes

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

That's good text to have.  I think it leaves unaddressed my desire to
document that the PKCS#10 format does not provide this property, and the
implication that if PKCS#10 is used with a new asymmetric key, there is an
additional requirement on the SZTP server to be trusted infrastructure in
the PKI (as well as the existing requirement to be trusted in the
deployment/operations sense).  Unless we propose to forbid the use of
PKCS#10 with new asymmetric keys?

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

This is fine "apple pie" text to have, though not quite the direction I was
thinking.  That said, looking at it again, I'm no longer sure that the
direction I was thinking has much to say -- basically it's just that using
the IDevID for TLS provides proof of origin as being from the device while
HTTP-level authentication does not ... but it's only proof to the SZTP
server, which (per the previous bit) would need to be trusted to properly
relay that information into the PKI.  So it's not terribly useful
information to add.

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

I am not going to insist on excessive pedanticism -- leaving it alone is
probably fine.

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

I think my comment can be distilled to: "there is no context in this
paragraph to clearly tie down what the 'associated identity certificate'
is."  A practitioner skilled in the art that has read the entire document
will figure it out, but in terms of quality of writing it's not great.
(There are plenty of examples of worse writing in the RFC Series, though,
so don't stress out about 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)

Thank you, too!

-Ben