Subject: Re: [netconf] draft-ietf-netconf-keystore-09.txt

From: Kent Watsen <[email-masked]>
Date: Thu, 2 May 2019 02:41:42 +0000

Hi Nick,

You are correct that something is amiss, but I'd like to discuss the fix more.

Your proposal looks (squinting my eyes) right (have you tested it?), but I'm wondering if there isn't a lower-level and potentially more useful fix...

In particular, note that 'ks:asymmetric-key-certificate-ref' points to a certificate in the the keystore that comes from the 'uses ct:asymmetric-key-pair-with-certs-grouping' statement.   The idea behind this grouping is that, whenever a certificate is configured, whether in the keystore or not, that the 'asymmetric-key-pair-grouping 3-tuple (alg, pub key, private key) is also configured.   This suggests that ct:asymmetric-key-pair-with-certs-grouping may be missing a 'must' statement or, more likely (due to the fact that the 3-tuple may only exist in <operational>), its 'description' statement is missing that precondition in its text.

Thoughts?

Kent // contributor

> On Apr 30, 2019, at 11:39 AM, Nick Hancock <[email-masked]> wrote:
> 
> Hi Kent,
> 
> I have just noticed a issue with the leaf 'keystore-reference' used 
> in the grouping 'local-or-keystore-end-entity-cert-with-key-grouping' 
> in ietf-keystore.
> 
> This leafref uses the typedef 'asymmetric-key-certificate-ref', but, 
> unless I am missing something, this alone will not work, because a 
> predicate for the list 'asymmetric-key' is missing. 
> 
> I would expect something like the following:
> 
> case keystore {
>  if-feature "keystore-supported";
>  leaf asymmetric-key-name {
>    type ks:asymmetric-key-ref;
>      description
>        "A reference to an asymmetric key that exists in
>         the keystore. "; 
>  }
>  leaf certificate-name {
>    type leafref {
>      path 
>        "/ks:keystore"
>        + "/ks:asymmetric-keys"
>        + "/ks:asymmetric-key"
>        + "[ks:name=current()/../" 
>        + "asymmetric-key-name]" 
>        + "/ks:certificates" 
>        + "/ks:certificate/ks:name";
>    }
>    description
>     "A reference to a specific certificate associated 
>      with the given private key, stored in the keystore.";   
>  }
> }
> 
> Regards
> Nick
>

---

From: Nick Hancock <[email-masked]>
Date: Fri, 3 May 2019 13:55:03 +0000

Hi Kent, 

> -----Original Message-----
> From: Kent Watsen <[email-masked]>
> Sent: 02 May 2019 04:42
> To: Nick Hancock <[email-masked]>
> Cc: [email-masked]
> Subject: Re: [netconf] draft-ietf-netconf-keystore-09.txt
> 
> 
> Hi Nick,
> 
> Your proposal looks (squinting my eyes) right (have you tested it?), but I'm
> wondering if there isn't a lower-level and potentially more useful fix...

No, I haven't tested it. However, we have implemented similar YANG to 
access an entry of a list that itself is within a list in the same 
way successfully in the past.

> 
> In particular, note that 'ks:asymmetric-key-certificate-ref' points to a
> certificate in the the keystore that comes from the 'uses ct:asymmetric-key-
> pair-with-certs-grouping' statement.   The idea behind this grouping is that,
> whenever a certificate is configured, whether in the keystore or not, that the
> 'asymmetric-key-pair-grouping 3-tuple (alg, pub key, private key) is also
> configured.   This suggests that ct:asymmetric-key-pair-with-certs-grouping
> may be missing a 'must' statement or, more likely (due to the fact that the 3-
> tuple may only exist in <operational>), its 'description' statement is missing
> that precondition in its text.
> 
> Thoughts?

Given that a client has the required permissions, the current model 
allows a client to create an entry in the list 'asymmetric-key' 
without the 3-tuple leafs 'algorithm', 'public-key' and 'private-key' 
as allowed via the must statement on the list. At this point there 
would be no 3-tuple in <operational> either. Defining the 3-tuple in 
<intended> or <operational> would be a second step, such as invoking 
the action 'generate-hidden-key'. Since there is, as you indicated, 
no 'must' statement relevant to the 'certificate' list, the model 
also allows a client to configure a certificate (chain) on an entry 
in the list 'asymmetric-key' without the 3-tuple in either <intended> 
or <operational>. The asymmetric key entry would not be able to be 
used to establish a TLS session due to the missing 3-tuple, of course.

Is this a valid use case? Possibly. I could argue that a client may 
wish to 'park' certificate chains in the keystore without necessary 
immediately using them on the server or even just pre-provision them 
and then provide the actual public key later maybe through some other 
process based on an operator's security policies? It makes no sense 
to reference a certificate in the keystore that is missing the 
3-tuple as the server identity of a NETCONF server or client identity 
of a NETCONF client as it cannot be used, so the question is whether 
it would make more sense to constrain the actual reference to 
reference certificates that have 3-tuple, if this is at all possible 
considering that the 3-tuple may only be in <operational>?

During analysis of deploying the keystore and truststore in our 
implementation, I have made some further observations that may be of 
interest:

1. There is nothing in the model to prevent a client configuring a 
   certificate to an asymmetric key that is not applicable to that 
   asymmetric key or even a certificate chain in the keystore or 
   truststore that is incompatible to the required type 
   ('end-entity-cert-cms' or 'trust-anchor-cert-cms' respectively) 
   or is this to be rejected by the implementation, in which case 
   this behavior is not described in the YANG?

2. I am not convinced whether the use of the prefix 'pinned-' on the 
   containers and lists in ietf-trust-anchors is correct. The 
   certificates are available in the truststore for pinning, yes, but 
   does not the 'pinning' actually take place when the certificates 
   are configured for client authentication on a server or server 
   authentication on a client? So I would argue that a certificate 
   may be in the trust store, but not necessarily pinned, so the name 
   may be misleading or am I missing something? Thus, I would have 
   expected the containers and lists to be without the 'pinned-' 
   prefix within the truststore itself. The 'pinned-' prefix being 
   only on the references, which is the case in the NETCONF client 
   and server models.

3. As an initial step we are analyzing the standalone use of the 
   keystore in truststore modules and were considering some 
   proprietary notifications that would include the certificate 
   (chain) received during TLS session establishment, but could not 
   'use' the groupings in ietf-crypt-types because they include 
   actions and notifications, which are not allowed in notifications. 
   Maybe it would be advantageous to provide basic groupings without 
   actions and notifications for use in proprietary RPCs and 
   notifications and provide refined versions of these groupings that 
   include the actions and notifications? Obviously this would mean 
   even more groupings, but wouldn't this increase their reusability?

Regards
Nick

> 
> Kent // contributor
> 
> 
> 
> 
> On Apr 30, 2019, at 11:39 AM, Nick Hancock
> <mailto:[email-masked]> wrote:
> 
> Hi Kent,
>

---

From: Kent Watsen <[email-masked]>
Date: Tue, 7 May 2019 22:24:22 +0000

Hi Nick,

> considering that the 3-tuple may only be in <operational>?

If you look at my "wide-screen needed" email from last Friday, you'll see that I'm trying to make a case that these 3-tuple nodes should be "mandatory true".   If that discussion falls apart, then we can revisit your proposal here.

BTW, please jump into that discussion, as comments from implementations are highly valued.

> implementation, I have made some further observations that may be of 

I see you're calling it "truststore", which I take as a confirmation for the rename proposal sent out last Thursday.

>   this behavior is not described in the YANG?

Such incompatibilities must not be allowed.  As neither issue can be enforced via YANG validation constraints, the best we can do is add comments to the description statements.  

For the latter case, I believe that both the 'end-entity-cert-cms' or 'trust-anchor-cert-cms' description statements have a sufficient number of MUST statements. - agreed?

For the former, how about this:

@@ -1614,7 +1624,9 @@ module ietf-crypto-types {
 
   grouping asymmetric-key-pair-with-cert-grouping {
     description
-      "A private/public key pair and an associated certificate.";
+      "A private/public key pair and an associated certificate.
+       Implementations SHOULD assert that certificates contain
+       the matching public key.";
 
     uses asymmetric-key-pair-grouping;
     uses end-entity-cert-grouping;
@@ -1692,7 +1704,9 @@ module ietf-crypto-types {
 
   grouping asymmetric-key-pair-with-certs-grouping {
     description
-      "A private/public key pair and associated certificates.";
+      "A private/public key pair and associated certificates.
+       Implementations SHOULD assert that certificates contain
+       the matching public key.";
     uses asymmetric-key-pair-grouping;
     container certificates {

>   only on the references, which is the case in the NETCONF client 

Agreed, especially with renaming the module and top-level container node to "truststore" (what else would a truststore contain, right?).  I just all the "pinned" prefixes in my local copy (s/pinned.//g)

>   even more groupings, but wouldn't this increase their reusability?

I don't understand "standalone use of the keystore in truststore modules" - maybe you meant "and"?

I know what you mean.   I recently wanted to use some groupings from another module but couldn't for a similar reason, and hence had a similar copy/paste yuck experience.

If I understand you correctly, you're saying that tooling such as `pyang` or `yanglint` is complaining because your notification "uses" the crypto-type grouping, which itself defines a notification - yes?

RFC 7950 Section 7.16 says"

   A notification MUST NOT be defined within an rpc, action, or another
   notification, i.e., a notification node MUST NOT have an rpc, action,
   or a notification node as one of its ancestors in the schema tree.
   For example, this means that it is an error if a grouping that
   contains a notification somewhere in its node hierarchy is used in an
   rpc definition.

Ouch, that's rather specific.  I was hoping there might be some wiggle room allowing for "inner" notifications to just be ignored.  That sounds like a better solution, don't you think, perhaps a YANG-Next candidate?

You're right in guessing we're reluctant to create more groupings unnecessarily and, in this case, these groupings are very small - just a single "leaf" or "leaf-list", so the yuck isn't all that much.  Copy/paste might be reasonable in this case, no?

Kent // contributor

---

From: Nick Hancock <[email-masked]>
Date: Fri, 10 May 2019 15:38:43 +0000

Hi Kent, 

> Sent: 08 May 2019 00:24
> If you look at my "wide-screen needed" email from last Friday, you'll see that I'm
> trying to make a case that these 3-tuple nodes should be "mandatory true".   If
> that discussion falls apart, then we can revisit your proposal here.
> 
> Such incompatibilities must not be allowed.  As neither issue can be enforced via
> YANG validation constraints, the best we can do is add comments to the
> description statements.
> 
> For the latter case, I believe that both the 'end-entity-cert-cms' or 'trust-anchor-
> cert-cms' description statements have a sufficient number of MUST statements.
> - agreed?
> 

Agreed.

>      uses asymmetric-key-pair-grouping;

Yes, but I believe that these SHOULD statements should also be 
included on the description statement of 'cert' leafs as well as they 
may get overlooked, if they are only to be found in the description 
statement on the groupings.

> Agreed, especially with renaming the module and top-level container node to
> "truststore" (what else would a truststore contain, right?).  I just all the "pinned"
> prefixes in my local copy (s/pinned.//g)
> 
> 
> 
> 

Yes, I meant 'and'.

> 
> I know what you mean.   I recently wanted to use some groupings from another
> module but couldn't for a similar reason, and hence had a similar copy/paste
> yuck experience.
> 
> If I understand you correctly, you're saying that tooling such as `pyang` or
> `yanglint` is complaining because your notification "uses" the crypto-type
> grouping, which itself defines a notification - yes?

Yes, that is correct.

> Ouch, that's rather specific.  I was hoping there might be some wiggle room
> allowing for "inner" notifications to just be ignored.  That sounds like a better
> solution, don't you think, perhaps a YANG-Next candidate?
> 

It would be a possible solution, but that is something for the future though.

> You're right in guessing we're reluctant to create more groupings unnecessarily
> and, in this case, these groupings are very small - just a single "leaf" or "leaf-
> list", so the yuck isn't all that much.  Copy/paste might be reasonable in this
> case, no?
> 

Yes, copy/paste is always an option, but this also has its drawbacks, 
including having multiple duplicate definitions within the YANG model; 
the opportunity to modify the copy, intentionally or unintentionally, 
such as to change the names of leafs; but most importantly copying and 
pasting would mean that copy will not follow any changes made to 
the original grouping, such as corrections and the addition of new 
nodes in newer revisions, meaning additional maintenance and possible 
discrepancies across YANG models going forward.  
And this is why I am a keen user of groupings to ensure consistency 
across models, even though it makes the YANG modules even harder to 
read. If something is the same then it should always be the same 
wherever is appears in the model and using a grouping does just that.

Regards
Nick

---

From: Kent Watsen <[email-masked]>
Date: Sat, 11 May 2019 16:19:12 +0000

> statement on the groupings.

Good idea.  I just added this as well:

  grouping end-entity-cert-grouping {
     description
       "An end entity certificate, and a notification for when
-       it is about to (or already has) expire.";
+       it is about to (or already has) expire.  Implementations
+       SHOULD assert that, where used, the end entity certificate
+       contains the expected public key.";

> It would be a possible solution, but that is something for the future though.

True, but we do what we can to make the future better  :)
https://github.com/netmod-wg/yang-next/issues/84

> wherever is appears in the model and using a grouping does just that.

Okay, let's see what we can do.  Firstly, please see my response just now in the other thread where I'm suggesting the remove the `action` statements entirely (via an idea similar to "crypt-hash").   If accepted, then we only have to create "data-only" groupings for those groupings that containing `notification` statements. 

Kent // contributor