Hi Juergen,
Thanks for your review. Below are my responses to your comments.
PS: why is ietf@xxxxxxxx in the CC?
Kent
--
Reviewer: Jürgen Schönwälder
Review result: Not Ready
draft-ietf-netconf-keystore-02
- s/YANG data module/YANG module/ or s/YANG data module/YANG model/
<KENT> Fixed.
- Find a better phrase for 'hold onto'. Does the module allow
configuration of trusted private keys and certificates? If so, say
so.
<KENT> Done.
- What is 'extends special consideration'?
<KENT> reworded.
- What is a 'semi-configurable' list?
<KENT> fixed (per Martin's comment earlier)
- s/is MAY/MAY/ (does this may need to be allcaps?)
<KENT> previously fixed and case-lowered.
- s/an optional attributes/optional attributes/
<KENT> fixed.
- the module seems to use actions but the surrounding text talks about rpcs
<KENT> fixed.
- does the generate-private-key really take a key length as a parameter
as promised in section 2?
<KENT> no it doesn't (anymore). fixed.
- section 2.2. talks about 'examples above' but there are none 'above'
<KENT> fixed.
- Have the examples been validated? Is rsa1024 a valid identityref
value?
<KENT> Just did, found no issues.
- There are a number of binary fields where you show explanatory text
in the examples instead of encoded data. Is this always clear?
<private-key>...</private-key> <!-- elipsis placeholder for a base64 encoded key -->
<KENT> This issue has been raised by others as well. I used to
have full base64 values, but it made the examples very difficult
to read. Hmmm, I like your suggestion, but the resulting line
is much longer than allowed. I could line-wrap, but the result
looks worse. Any other suggestions?
- 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?
<KENT> it's a list of lists, so there can be many different
sets a host-keys defined, with each application/use pointing
to its own entry. The ietf-ssh-client module's grouping has
a leafref to a specific entry. FWIW, this grouping is
used by ietf-netconf-client module. At the moment, there
no other uses of this grouping. I think that you may be
getting confused between the key the server presents (the
host-key) versus the key user presents.
.
- 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?
<KENT> I thought there was a criticism a while back that
drafts should show balance in their examples? In the
meanwhile, I reworded the sentence to look more like the
other two examples and, in doing so, removed the "JSON
encoding" string altogether.
- s/(c) 2014/(c) 2017/
<KENT> fixed, in all my drafts.
- reference statement of the revision statement seems to have the
wrong title
<KENT> fixed.
- Should the document title be aligned with other YANG module
definitions, so it is easier to spot it?
<KENT> I don't understand, what do you mean?
- 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?
<KENT> Unsure, but I wouldn't suspect needing to make that
kind of update for a long time. If we were to define a
module for algorithm identities, it might lead to us wanting
to define every algorithm (not just public-key algs), which
could take some time...
- 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.
<KENT> yes, but your comment before was valid. I don't see why
this makes a stronger case though...
- "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.
<KENT> Fixed.
- 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.
<KENT> agreed. Fixed.
- 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.
<KENT> Fixed.
- 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.
<KENT> comment removed per Martin's review.
- 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...
<KENT> added.
- Add a reference clause to the definition of public-key?
<KENT> added.
- Why do certificate names have to be unique across all keys?
<KENT> because otherwise leafrefs might be ambiguous, right?
- 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?
<KENT> I'm confused, certificates are signed structures already...
- All references to RFCs and I-Ds in description clauses must resolve
to regular citations somewhere and references in the references
section.
<KENT> Fixed.
- 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?
<KENT> If an action fails, then standard NC/RC error is returned.
There is no support to learn which algorithms a server supports,
but by supporting this module, the server must support all of
these algorithms.
- 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.]
<KENT> okay, let's keep this one open.
- 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'.
<KENT> yes, the certs may be used by different modules for
different use-cases, but they'd always be trusted. E.g.,
the certs preinstalled by a browser...
- 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
<KENT> "set" doesn't sound too bad - dunno. Regarding moving
"data", see the tail-end of the email I sent on July 11.
- 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?
<KENT> I was supposed to be mandatory. Fixed.
- 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.
<KENT> I believe that it is correct for the keystore to
contain all these things, and I think it's most readable
for them all to be defined in this document. What do
others think?
- 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.
<KENT> it was dumb, I removed the whole paragraph.
- 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.
<KENT> Good point. Just now realizing that ietf-system's
'authorized-key' has a workable definition. I just switched
to using it.
- 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).
<KENT> Fixed.
- The prefix is 'ks' and not 'kc' as stated in the IANA considerations
section.
<KENT> holdover from when draft was called "keychain". Fixed.
- Security considerations talk about 'RPC operations' but the data
model actually uses actions. Perhaps simply talk about 'operations'.
<KENT> Fixed. (Benoit should update his template as well)