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-trust-anchors-13.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.
- The prefix 'ts' is rather short, the set of two letter prefixes is
rather small limited. This comment also applies to the other
documents, crypto-types uses 'ct. Perhaps this is not a problem
since collisions can be handled but if we go for rather short
prefixes, we will have to exercise the collision resolution. (I see
that 'ct' has already been used by ietf-complex-types, RFC 6095.) A
possible alternative could be to use sec-ct, sec-ts, sec-ks, ...).
RFC 8407 provides the following guidelines (Section 4.2):
Prefix values SHOULD be short but are also likely to be unique.
Prefix values SHOULD NOT conflict with known modules that have been
previously published.
Well, having short and terse prefixes is actually nice and enhancing
programmer readability.
- s/is defines a truststore/defines a truststore/
- s/to be grouped references/to be grouped and referenced/
- In 2.2.1, I was not sure what CA certificates are and what EE
certificates are. I then tried to guess EE = end entity cert, but
this does not explain CA since the term used in crypto types is
trust anchor cert. The description in the XML clarified that my
guess was kind of correct. Perhaps explain upfront what these
acronyms mean? Or perhaps the acronyms can be avoided by simply
spelling things out? They do not appear to be used frequently.
- s/<!-- Entity Certs/<!-- End Entity Certs/
- s/(Section 2.1.3.2), groupings/(Section 2.1.3.2) groupings/
- Is the feature truststore-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?
- In the YANG module, you seem to use Truststore to refer to
/ts:truststore but in the surrounding text you also use just
truststore. 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 /ts:truststore but that may be my
subjective preference. Well, I started to use 'central truststore'
below, not sure this is better as the term also would benefit from
being defined.)
- Does it make sense to be explicit about the fact that the cert/key
references are all weak references in the sense that they can exist
without the corresponding cert/key being present? In other words, in
order to safely delete a cert/key, one would have to check that
there are no dangling references left (which is difficult in case
references are scattered over multiple (proprietary) data models).
For YANG savvy people this may be obvious since there is no
require-instance statement in the leafref type definition but not
every implementer may recall this - and it would be good to document
that using weak references was an explicit design decision and not
an oversight.
- Does it make sense to spell out that using the grouping in other
YANG modules requires to define additional reference types since the
ones provided by this modules only work for the central truststore
store? And this as a consequence may require multiple leafs to refer
to keys that exist in different truststores, i.e., having multiple
truststores is possible but comes with a cost.
- Would it make sense to add to all three documents an objectives
section that summarizes the design objectives? For this module,
a starting point might be this:
2.1. Objectives
The design of the truststore data model was guided by the following
design objectives:
- provide a central truststore for storing keys or certificates
- provide support for storing named bags of keys or certificates
- provide types that can be used to reference keys or certificates
stored in the central truststore
- protect the truststore using suitable access control definitions
- provide groupings that support locally configured keys or
certificates or references to key or certificate bags in the
central truststore
- provide groupings that can be used to instantiate truststores
in other data models (but this requires to introduce additional
types to reference keys or certificates)
I do not know whether this list is complete but I find it usually
helpful to have the design objectives spelled out.
- Section 3 talks about populating <running> with built-in trust
anchors.
In order for the built-in trust anchors to be referenced by
configuration, the referenced certificates MUST first be copied into
<running>. The certificates SHOULD be copied into <running> using
the same "key" values, so that the server can bind them to the built-
in entries.
Is the idea that this copy operation is an explicit management
operation or can implementations populate <running> with this
data automatically?
- Section 4.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
