Subject: [Netconf] mbj review of draft-ietf-netconf-restconf-server-model-09

From: Martin Bjorklund <[email-masked]>
Date: Tue, 29 Mar 2016 21:25:56 +0200 (CEST)

Hi,

Here is my review of draft-ietf-netconf-restconf-server-model-09.

o  Section 3

  The picture should s/<augments>/<uses>/g

o  Section 4

  It would be usefule with a reference to the term IDevID in this
  document.

o  Section 4.1

  OLD:

   o  A configurable list of lists of trust anchor certificates.  This
      enables the server to have use-specific trust anchors.  For

  NEW:

   o  A configurable list of trust anchor certificates.  This
      enables the server to have use-specific trust anchors.  For

  ... but what does "use-specific" mean?

o  Section 4.1

  The text says:

   o  An RPC to request the server to load a new private key.

  When I first read this, I didn't understand what this meant.  See
  more below.

o  Section 4.1

  I think the "semi-configurable" design has some issues.  You have
  defined some actions that actually modifies the configuration of the
  system.  It is not clear which config datastore is modified.
  Presumably running.  Interactions with locking and access control
  are not described.  Also, the resulting configuration is not
  complete - i.e., you cannot do <copy-config> to save/restore a
  backup.  This is fine, since you really don't want to expose the
  private keys in the config backup.  But some discussion is needed
  around this subject.  What happens if I generate a private key with
  your action, backup that config and then restore it?  What happens
  with config that has references to such a key?

  One way to avoid that the actions modify the configuration could be
  to move them into the private-key list.  One drawback is that two
  operations are needed in order to create a (usable) key - first
  create the config in running, then call the action.

  Another option might be to model the keys as config false data.
  This also solves the problem that some keys (in TPM for example)
  are not deletable.

o  Section 4.1.3

  OLD:

  import ietf-yang-types {     // RFC 6991
    prefix yang;
  }

  NEW:

  import ietf-yang-types {
    prefix yang;
    reference "RFC 6991: Common YANG Data Types";
  }

  (same for the other modules)

o  Section 4.1.3

  I think the "algorithms" typedef should be called "algorithm".  But
  actually, it should probably have a more descriptive name
  "algorithm" is rather generic...

  Also, this is an fixed enumeration.  Section 5 says: 

    In additional
    algorithms are needed, they MAY be augmented in by another module,

  [note: s/In/If/]

  You can't really augment in additional enums.  You'd have to augment
  an additional leaf of some other type.

  What happens to the "algorithm" leaf when I load a key with your
  action?  Will other algorithms be rejected?

  If you keep the enums, a reference statement for each enum would be
  useful.

o  Section 4.2.1

  I was confused by the containers that the RFC editor are supposed to
  remove.  Then I realized they exist due to a deficiency in pyang...
  So I fixed pyang; you can now pass --tree-print-groupings (only in
  1.1 branch).

  I don't have a solution for the long leafrefs though.  But in any
  case, I think the current long lines make the tree picture very
  difficult to grasp.  It would be better to manually tweak the output
  before posting the next draft - unless we can come up with some
  clever way to handle this in pyang.

o  Section 4.2.3

  You have a leaf:

            leaf certificate {
              if-feature ssh-x509-certs;
              type leafref {
                path "/kc:keychain/kc:private-keys/kc:private-key/"
                     + "kc:certificate-chains/kc:certificate-chain/"
                     + "kc:certificate";
              }

   This is a leafref that will actually contain the entire binary
   certificate.  This may be a bit awkward...  Unless you really meant
   kc:name as the last part of the path?  In either case, the
   description statement probably needs an update.

   Also, the path crosses two lists, which in general means that that
   the leafref may refer to multiple leafs.  But in this case it might
   be ok. 

o  Section 4.2.3

      leaf address {
        type inet:ip-address;
        description
         "The IP address of the interface to listen on. The SSH
          server will listen on all interfaces if no value is
          specified.";
      }

  What about "0.0.0.0" and "::"?  So a missing 'address' leaf should
  mean any ipv4 and ipv6 address on all interfaces?

      leaf port {
        type inet:port-number;
        mandatory true;  // will a default augmented in work?
        description
         "The local port number on this interface the SSH server
          listens on.";
      }

  I suggest you remove the mandatory statement, and then use refine to
  set the default port when the grouping is used; OR use refine to set
  mandatory true if no reasonable default exists.

o  Section 4.4.1

  You have:

       +--rw listen {(ssh-listen or tls-listen)}?

  Hmm, the design allows for other protocols than ssl and tls (just
  augment new cases).  But this if-feature statement essentially
  REQUIRE an implementation to _also_ support ssh or tls.  Is that
  intentional?

o  Section 5

  ietf-system-keychain vs. ietf-routing-key-chain

  Is it "keychain" or "key-chain"?

o  General remark.

  Unless it is too much of a burden, I think it would make sense to
  move the generic tls and ssh grouping models (and keychain) into a
  separate draft.   It might also be useful with corresponding
  groupings for ssh/tls clients (which you almost already have).

/martin

---

From: Kent Watsen <[email-masked]>
Date: Tue, 29 Mar 2016 20:47:05 +0000

Hi Martin,

Thanks for your speedy review.  A quick read through your comments, I get the impression that you’re generally happy and, with exception of the splitting to draft into parts (which I plan to raise as an open issue next week), it seems mostly fit-and-finish, for which I’m grateful.

BTW, the routing module’s name is "ietf-routing-key-chain" - I don’t know why they didn’t choose "ietf-routing-keychain”, but that’s what they did.

I will follow-up with more specific responses later.

Thanks again,
Kent

On 3/29/16, 3:25 PM, "Netconf on behalf of Martin Bjorklund" <[email-masked] on behalf of [email-masked]> wrote:

>_______________________________________________
>Netconf mailing list
>[email-masked]
>https://www.ietf.org/mailman/listinfo/netconf