Reviewer: Jürgen Schönwälder
Review result: Ready with Issues
The crypto modules aim at providing a flexible reusable infrastructure
of groupings for modeling cryptographic keys and related concepts. The
flexibility of the definitions provided of course comes with a certain
amount of complexity.
>From a YANG perspective, draft-ietf-netconf-keystore-20.txt is in a
good and close to publish state (a couple of minor issues left). I
also tried to understand what is being modeled here and hence I also
have some questions concerning the concepts modeled and I hope these
are easy to answer/resolve as well.
- I have compiled the YANG modules using yanglint 0.16.105.
- Compared to the other two I-Ds in the batch, this I-D has a more
verbose introduction (appreciated) and it also has a terminology
section (which never hurts). I do not know whether Kent has the
energy to align the I-Ds in their intro style.
- s/in Examples (Section 2.2)./in Section 2.2./
- Is the feature keystore-supported really needed? Does the YANG
library not already provide the information whether a module has
been implemented or just imported to access type and grouping
definitions? OK, I know see that this is used to make definitions
conditional, hence it makes sense. This means that my comment on
truststore-supported in the other review can be ignored, I found
the answer.
- My question concerning two-letter prefixes applies to this I-D as
well.
- In the YANG module, you seem to use Keystore to refer to
/ks:keystore but in the surrounding text you also use just
keystore. I am not sure it is necessary to have the capitalized
version but if people think its necessary, it makes sense to define
the difference and to make sure the proper capitalization is used
throughout the document. (If it is necessary somewhere to be
explicit, I would rather use /ks:keystore but that may be my
subjective preference. In the other I-D review, I used the term
'central truststore', which is not a good term either, the term
'well-known keystore' may be a better alternative.)
- Many of the groupings either symmetric-key-ref or asymmetric-key-ref
and while the groupings seem to offer flexibility to instantiate
other keystores, I have some doubts that this actually works unless
you augment in other reference types. Looking at the
ex-keystore-usage module, I find in there the usage of
ks:symmetric-key-ref and ks:asymmetric-key-ref and they refer to the
well-known keystore, not the one defined in the example module.
YANG's reference mechanism via leafrefs is not really supporting
well what you try to do. I understand the flexibility you want to
achieve but it seems YANG 1.1 does not really support this well
enough. What you would need is a leafref type that can be "anchored"
at different places but we don't really have this...
You hint at this in the definition of the keystore-grouping:
"Grouping definition enables use in other contexts. If ever
done, implementations SHOULD augment new 'case' statements
into local-or-keystore 'choice' statements to supply leafrefs
to the new location.";
It seems the SHOULD really is a 'must' (I do not care about
capitalization); if you do not add your own leafrefs, things will
not work or be majorly surprising.
If I am correct, then there should be stronger warnings upfront that
simply reusing the groupings is not enough and that the example
module is actually an incomplete example...
The same may apply to some of the groupings in the truststore
drafts.
- There is a live discussion concerning the built-in keys, which
obviously applies here as well. Perhaps the conclusion is that what
we have is the best solution. This is just here as a reminder in
case there are changes.
- Section 4 points to keys being compromised 'when in transit' but I
think we also want to protect keys at rest, i.e., sitting in a
backup.
- I am wondering whether key encryption also applies to the related
truststore document.
- Expand RMA in "RMA scenarios" or simply avoid the acronym (its only
used once).
- s/"default-deny-all)/"default-deny-all")/
- Section 4.3 talks about _highly_ restricted access mechanisms and
_highly_ authorized clients and I am usually a bit confused what
_highly_ means. But I am YANG doctor, not a security reviewer. ;-)
- Section 5.2 says:
This module does not define any RPCs, actions, or notifications, and
thus the security consideration for such is not provided here.
Well, the module actually instantiates certificate-expiration
notifications.
- Registrant Contact: should be changed to the IESG.
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
