Re: [netconf] AD review of draft-ietf-netconf-trust-anchors-20

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Kent,

And replies to your comments on this draft as well are inline ...


From: Kent Watsen <[e-mail]>
Sent: 25 March 2023 16:41
To: Rob Wilton (rwilton) <rwilton=[e-mail]>
Cc: [e-mail]; [e-mail]
Subject: Re: [netconf] AD review of draft-ietf-netconf-trust-anchors-20

Hi Rob,

Thank you for your review!
Please see below for comments.

Kent // author




On Mar 22, 2023, at 5:51 AM, Rob Wilton (rwilton) <rwilton=[e-mail]<mailto:rwilton=[e-mail]>> wrote:

Hi Kent,

Here are my review comments for draft-ietf-netconf-trust-anchors.  This document is also well written, so I would like to thank you, the shepherd, and WG for a high-quality document.

As before, I have a few comments, but will note that I'm not a security expert and hence some of my questions/comments may be due to my security ignorance ...

I really think that the key question about copying the certificates into running, and also how that will play with system datastore.  Further details inline below, but I will also flag this as an email to Netmod.  Hopefully there will be some time to discuss this.

It's such a quagmire!

I've always felt the "<runnig> must always be valid" was a statement regarding the server's view of <running>, not the representation returned to clients.  And in that sense, there is no reason to "copy" nodes from <operational> or <system> into <running>, as already we have "<running> + <system> = <intended>".  This is true for the so called "shared-objects" case (aka. "Inactive-Until-Referenced" in draft-ietf-netmod-system-config), which is the primary (by far) use-case in that draft, in my view.


However, there may be a valid use-case of the client wanting to annotate some system-defined configuration (aka. "Modifying/overriding System Configuration" in draft-ietf-netmod-system-config).   Only in this case do I understand the need to copy system-defined config into <running>.

As you say, best discussed in the NETMOD.





Moderate level comments:

(1) p 27, sec 3.  Support for Built-in Trust Anchors

  In order for the built-in bags of trust anchors and/or their trust
  anchors to be referenced by configuration, they MUST first be copied
  into <running>.

Although I am probably partially guilty of pushing this design, reviewing it now, I think that forcing the entire certificates to be copied into running (even via a new parameter in the system-datastore draft) isn't ideal.  Hence, I will raise a separate thread on Netmod to discuss whether it wouldn't be more pragmatic to relax the constraint that <running> must be valid independently from <intended> (e.g., when we discuss <system>).

Hence, I am thinking whether it might be beneficial to tweak the paragraph above to emphasise the need to copy the trust-anchors is only necessary to make that datastore is valid rather than putting a hard requirement of copying them into <running>.  E.g., perhaps text along the lines of:

  "Any built-in bags of trust anchors and/or their trust anchors that are referenced in a valid configuration datastore MUST be copied into that datastore to satisfy the reference constraints (e.g., leafrefs)."

If you modify this text, then I would also try and remove the hard references to <running> in the paragraphs that follow it.

I agree with the sentiment.  Specifically, given the understanding:
   * for <system> before YANG-next: nodes are always coped into <running>.
   * for <system> after YANG-next: nodes are coped into <running> only when needed.

But maybe we can spin it towards the future, by assuming NMDA will be fully realized someday?


   Built-in bags of trust anchors and/or specific trust anchors, that

   are referenced by configutation (e.g., a "leafref"), MUST be present

   in a datastore in order for the datastore to be valid.  For instance,

   built-in nodes originating in <system> (from

   [I-D.ietf-netmod-system-config]) will (per [RFC8342]) implicitly

   propogate to <intended>, ensuring references to them will be resolved

   in <intended>.

Actually, given that the "system-config" draft is now adopted (it didn't even exist when that text was first written), this draft could more broadly replace "built-in" with "system-defined" (no need for another term, right?) and then delete the next three paragraphs (as the mechanics should be defined in the system-config draft):
[Rob Wilton (rwilton)]
Thinking about this over the last couple of days, I wonder if it is better just to say less, and only say (note fixed typo for 'configutation') :


   Built-in bags of trust anchors and/or specific trust anchors, that

   are referenced by configuration (e.g., a "leafref"), MUST be present

   in a datastore in order for the datastore to be valid.

I think that this paragraph may also be worth keeping:


   Built-in bags and/or their trust anchors MAY be copied into other

   parts of the configuration but, by doing so, they lose their

   association to the built-in entries and any assurances afforded by

   knowing they are/were built-in.

But then removing the other two paragraphs, as you suggest.



===START: these 3 paragraphs could be removed===



   The built-in bags and/or their trust anchors MUST be copied into

   <running> using the same "key" values if it is desired for the server

   to maintain/update them (e.g., a software update may update a bag of

   trusted public CA certificates used for TLS-client connections).



   Built-in bags and/or their trust anchors MAY be copied into other

   parts of the configuration but, by doing so, they lose their

   association to the built-in entries and any assurances afforded by

   knowing they are/were built-in.



   The built-in bags and/or their trust anchors are immutable by

   configuration operations.  Servers MUST ignore attempts to modify any

   aspect of built-in bags and/or their trust anchors from <running>.


===STOP: these 3 paragraphs could be removed===


Thoughts?




(2) p 28, sec 3.  Support for Built-in Trust Anchors

  The built-in bags and/or their trust anchors are immutable by
  configuration operations.  Servers MUST ignore attempts to modify any
  aspect of built-in bags and/or their trust anchors from <running>.

Is this really MUST ignore, or MUST reject?  I.e., Should they be rejecting any configuration attempt to modify the built-in bags and/or their trust anchors?  I think that this behaviour is consistent with the system datastore draft, but we should check.

Reject sounds more proper.  That said, this is one of the paragraphs I suggest removing, punting such mechanics to the system draft (and its continuations).
[Rob Wilton (rwilton)]
Yes, I agree to removing this.

But I guess that you also have the issue that if the trust anchors are completely copied into running and the trust store is subsequently updated out of band, then the certificates in the configuration differ from what is in trust-store (and hence system), and this is presumably an example of when they should be ignored.

My current expectation is that the system datastore will probably say that the running configuration always overrides the configuration in the system datastore, but this starts to look like a counter example, which potentially means that it may require special handling.

This is where the "immutable-flag" draft comes into play, so that it's clear when not allowed, and it shouldn't be allowed here.  If running wishes to use a different trust-anchor, then it should define a new trust-anchor and point to it, instead of hacking the built-in trust-anchor.



Finally, are they still immutable even if copied into other parts of the configuration?  Presumably not, since at the point they are copied they lose their association with the built-in trust anchors.

No and true.  But then again, as soon as they're copied somewhere else in the config, they lose all history/association of having originated from a built-in, and therefore there's no need for the immutability to continue.




(3) p 32, sec 6.2.  Informative References

  [I-D.ietf-netconf-trust-anchors]
             Watsen, K., "A YANG Data Model for a Truststore", Work in
             Progress, Internet-Draft, draft-ietf-netconf-trust-
             anchors-19, 19 October 2022,
             <https://datatracker.ietf.org/doc/html/draft-ietf-netconf-
             trust-anchors-19<https://datatracker.ietf.org/doc/html/draft-ietf-netconf-%0b             trust-anchors-19>>.

The same comment as for the other drafts about a document not referencing itself also applies here.

Please see my response to your similar comment in the crypto-types draft review regarding the Editor's Note.
[Rob Wilton (rwilton)]
Ack.  I think that the same solution is needed here, i.e., slightly clearer instructions to the RFC editor.





Minor level comments:

(4) p 3, sec 1.  Introduction

     Provide groupings that enable raw public keys and certificates to
     be configured locally or as references truststore instances.
     Enable the truststore to be instantiated in other data models, in
     addition to or in lieu of the central truststore instance.

Would it be helpful to have a brief sentence in this document, and perhaps the keystore document that highlights the main differences between keystore and truststore.  Alternatively, you could reference another document that defines these.


Yes, it would be helpful, but folks can also read the abstracts of both docs, yes?

Also, all the first-page results from googling "truststore vs keystore" are accurate, AFAICT, suggesting that these are reasonably well-known concepts.
[Rob Wilton (rwilton)]
Okay, I'll leave it to your choice as to whether to add anything here.




(5) p 6, sec 2.1.3.1.  The "local-or-truststore-certs-grouping" Grouping

    grouping local-or-truststore-certs-grouping:
      +-- (local-or-truststore)
         +--:(local) {local-definitions-supported}?
         |  +-- local-definition
         |     +-- certificate* [name]
         |        +-- name?                            string
         |        +---u ct:trust-anchor-cert-grouping
         +--:(truststore) {central-truststore-supported,certificates}?
            +-- truststore-reference?   ts:certificate-bag-ref

I wonder whether "inline" would be a slightly better choice than "local-definition", but presumably this has been previously considered.  I.e., you don't need to change unless you want to.  If you do change, then obviously the public-keys grouping below would similarly change.

Actually, this label has never been discussed.  To be honest, "local-definition" never struck me as ideal either.  "Inline-definition" seems better.   I just made the switch, in "ietf-keystore" too, and throughout  the suite of drafts.  Thank goodness for scripts!  ;)
[Rob Wilton (rwilton)]
Thanks.




(6) p 6, sec 2.1.3.1.  The "local-or-truststore-certs-grouping" Grouping

         +--:(truststore) {central-truststore-supported,certificates}?
            +-- truststore-reference?   ts:certificate-bag-ref

truststore-reference isn't really to a truststore, but to a bag of certs within the truststore, hence also wondering whether 'truststore-certs' or 'truststore-certificates' (and 'truststore-keys' for below) might be clearer than truststore-reference.

It is a bit of a misnomer, but the intent wasn't so much to label the thing pointed to, so much as to label where it came from.  Specifically, the solution doesn't exclude the possibility that the consuming modules might augment in their own location for where certificates can be found.  Indeed, my own product defines a local truststore (i.e., "uses truststore-grouping") and then augments-in alternate "case" statements.  To this end "central-truststore-reference" would be more apt, though I resigned to the idea that, unqualified, "truststore" refers to *the* central truststore and, hence, my product's uses "local-truststore" for its case-statements.  If we wanted to fully expand the label, it would be  "central-truststore-certificate-bag-reference", anything shorter leaves something off, unless using acronyms, e.g., "central-ts-cart-bag-ref", which actually looks okay to me.  Thoughts?
[Rob Wilton (rwilton)]
Based on your explanation, I think that what you have is best.  Please check whether expanding the leaf description in the YANG modules, or the "comments" in section 2.1.3.1 to indicate that this is referencing the central truststore.




(7) p 7, sec 2.1.3.2.  The "local-or-truststore-public-keys-grouping" Grouping

    grouping local-or-truststore-public-keys-grouping:
      +-- (local-or-truststore)
         +--:(local) {local-definitions-supported}?
         |  +-- local-definition
         |     +-- public-key* [name]
         |        +-- name?                     string
         |        +---u ct:public-key-grouping
         +--:(truststore) {central-truststore-supported,public-keys}?
            +-- truststore-reference?   ts:public-key-bag-ref

Given the definition, I presume that it is not common to either want to reference more than one truststore, or to use a mix of truststores and locally defined certificates.  Besides it is only a grouping, so if the need ever arose then it could always be defined inline, or as another grouping.

Correct, in use, only one "case" is ever needed at a time.  Seems okay for now.
[Rob Wilton (rwilton)]
Okay.



(8) p 11, sec 2.2.1.  A Truststore Instance

      <!-- End Entity Certs for Authenticating Servers -->
      <certificate-bag>
        <name>trusted-server-ee-certs</name>
        <description>
          Specific end-entity certificates used to authenticate server
          certificates.  A server certificate is authenticated if its
          end-entity certificate is an exact match to one of these
          certificates.
        </description>
        <certificate>
          <name>My Application #1</name>
          <cert-data>BASE64VALUE=</cert-data>
        </certificate>
        <certificate>
          <name>My Application #2</name>
          <cert-data>BASE64VALUE=</cert-data>
        </certificate>
      </certificate-bag>

Are these effectively for pinned certificates?  If so, a quick web search suggests that they may no longer be recommended because the downsides outweigh the benefits.  If correct, should we add a warning to the description of the example, or not include them in the example?

Yes, and EE-certs come with a mix of plusses and minuses.   I'm not opposed to adding a warning, if correct, and thus wonder if we should ask a Security Area person?
[Rob Wilton (rwilton)]
Let's leave this for now, and we can flag it to the SEC ADs during IESG review.  To be honest, I would expect them to review these drafts very carefully anyway ...





(9) p 26, sec 3.  Support for Built-in Trust Anchors

  As built-in trust anchors are provided by the server, they are
  present in <operational> (and <system> [I-D.ma-netmod-with-system],
  if used).

The reference to system can now be updated to ietf-netmod-with-system.

Already done, in my local repo...
[Rob Wilton (rwilton)]
Thanks.




(10) p 28, sec 3.  Support for Built-in Trust Anchors

  In order for the built-in bags of trust anchors and/or their trust
  anchors to be referenced by configuration, they MUST first be copied
  into <running>.
  The built-in bags and/or their trust anchors MUST be copied into
  <running> using the same "key" values if it is desired for the server
  to maintain/update them (e.g., a software update may update a bag of
  trusted public CA certificates used for TLS-client connections).

I didn't find this text to be entirely clear that the update mechanism is out of band, and not being done by the client trying to update the certificates in running configuration with different values.

True, the text could be clarified.  That said, in a response above, I propose to remove "mechanics" from this draft (including this paragraph), deferring to the "system-config" draft to define.
[Rob Wilton (rwilton)]
I basically agree that we should remove 'mechanics' for this draft, and hopefully reach consensus on how to handle this in the system datastore draft.

PS: IDK if this "update" issue is covered by "system-config" either.



(11) p 28, sec 3.  Support for Built-in Trust Anchors

        <certificate>
          <name>Public Root CA Cert 3</name>
          <cert-data>BASE64VALUE=</cert-data>
        </certificate>
      </certificate-bag>

Is it required to copy the cert-data into running, or is coping the name sufficient?  Checking the YANG, even with the current system-datastore, I think that it would be necessary to copy the cert-data as well.

It is necessary to copy, since the leaf is "mandatory true".
[Rob Wilton (rwilton)]
We don't know really where the discussion regarding system-configuration validation will end up (e.g., can you have an incomplete configuration in <running> that is implicitly valid because only <intended> is validated and that includes configuration from <system> making it valid?)  But, even if we do allow this, then it would still be nice, for operators who want to use it that way, to allow the configuration in <running> to be referentially complete and valid.  To this end, I wonder whether making the cert-data mandatory true (via the refine statement in the truststore-grouping) is not worth it.  Without the mandatory true it would allow the certificate to be referenced by name only without requiring the certificate data to be present in running.

At run time, if the certificate isn't present, then could that just be treated operationally similar to be it being an invalid certificate?


Nit level comments:

(12) p 18, sec 2.2.3.  The "Local or Truststore" Groupings

    description
      "This module illustrates notable groupings defined in
       the 'ietf-truststore' module.";

Suggest "This example module ..."

Fixed.  (In the keystore draft too)
[Rob Wilton (rwilton)]
Thanks.

Regards,
Rob



Regards,
Rob


Kent // author