draft-ietf-netconf-keystore-02 - s/YANG data module/YANG module/ or s/YANG data module/YANG model/ - Find a better phrase for 'hold onto'. Does the module allow configuration of trusted private keys and certificates? If so, say so. - What is 'extends special consideration'? - What is a 'semi-configurable' list? - s/is MAY/MAY/ (does this may need to be allcaps?) - s/an optional attributes/optional attributes/ - the module seems to use actions but the surrounding text talks about rpcs - does the generate-private-key really take a key length as a parameter as promised in section 2? - section 2.2. talks about 'examples above' but there are none 'above' - Have the examples been validated? Is rsa1024 a valid identityref value? - There are a number of binary fields where you show explanatory text in the examples instead of encoded data. Is this always clear? ... - I am not sure I understand trusted-host-keys. On systems that have multiple user accounts, you usually have a per account list of trusted host keys in addition to a global system wide list. Perhaps make it clear that this models the global known hosts list only? - Is it helpful that some examples use XML / NETCONF while others use JSON / RESTCONF or is this possibly confusing? If you include JSON examples, do you not need to reference RFC 7951? - s/(c) 2014/(c) 2017/ - reference statement of the revision statement seems to have the wrong title - Should the document title be aligned with other YANG module definitions, so it is easier to spot it? - Since crypto algorithms come and go, is it reasonable to have the identities defined in a rather static RFC? Would an IANA maintained identity module perhaps make more sense? - Oh, I now see that the key length is embedded in the algorithm identity. Perhaps clarify this earlier, see my confused comments above. Anyway, I think this makes the need for an IANA maintained module even stronger. - "If additional algorithms are needed, they MAY be augmented in by another module" seems confusing; I think all that is needed is to define a new identity, right? If so, there is no augmentation in the YANG sense. - The description of the keystore container talks about passwords and I am not sure this is correct. I am also not sure the terms 'active material' and 'passive material' really help. - The description of keys says "A list of keys maintained by the keystore.". I think it would be more helpful to say "A list of public-private key pairs maintained by the keystore." Every list element seems to have two keys. - What about the comment in the definition of algorithm-identifier? Looks like an open issue or something that should be resolved before publishing this document. - Add a reference clause to the definition of private-key? Do we really need the INACCESSIBLE enum and the union? There is another open question right below the private-key definition... - Add a reference clause to the definition of public-key? - Why do certificate names have to be unique across all keys? - Since the certs of a key do not contain a signature, where are signed certificates stored or are they outside the scope of the model? - All references to RFCs and I-Ds in description clauses must resolve to regular citations somewhere and references in the references section. - Is there any text needed to explain what happens if actions can't be carried out, e.g., the algorithm identity is not supported? Can a client actually learn which algorithm identities are supported or is the approach simply trial and error? - I am a bit concerned about the two list names 'trusted-certificates' and 'trusted certificate' which mean very different things and can be potentially confused. (The same for 'trusted-host-keys' and 'trusted-host-key'.) [No, I do not have a good naming proposal that I like, perhaps some brainstorming helps.] - I am not sure what 'trusted certificate' here really means. What you are modeling is a set of sets of certificates. What such a set means depends on where this list is used. I am not sure what it means for a certificate in such a set to be 'trusted'. - What about the // rename to 'data' comment? I like it, renaming to list certificate { key name; leaf name; leaf data; } seems less confusing (and it is even shorter). Perhaps something along this line would be better (not sure I like certificate-set) +--rw certificate-set* [name] | +--rw name string | +--rw description? string | +--rw certificate* [name] | +--rw name string | +--rw data? binary - Why is the certificate (data) leaf in trusted-certificate not mandatory? Does it make sense to have list elements where there is only a name? - I am wondering whether it is really a good idea to handle both X509 keys and certificates as well as some SSH host keys in this document. Perhaps it is better to really split things. Also think about devices that only do SSH or only to TLS. - I am not sure I fully understood the call home statement in the description of trusted-host-key. This probably needs more or better explanation. - Are SSH implementation encoding keys in ASN.1/DER? I doubt this very much. Perhaps this SSH stuff should be moved to a separate I-D since I believe this needs more work. - You cite RFC 6020 (YANG 1) but you use actions which were only added in YANG 1.1. I think you should only cite YANG 1.1 (RFC 7950). - The prefix is 'ks' and not 'kc' as stated in the IANA considerations section. - Security considerations talk about 'RPC operations' but the data model actually uses actions. Perhaps simply talk about 'operations'.