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-crypto-types-18.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.
- s/ietf-crypt-types/ietf-crypto-types/
- Does it make sense to have a new notation like
| The diagram above uses syntax that is similar to but not
| defined in [RFC8340].
or shall this simply become regular text?
- "Identities use to specify uncommon formats are enabled [...]" is
this correct or should it be "used to specify"?
- Second bullet 2.1.4.1:
Perhaps change
[...] specified by the "format" identity Section 2.1.2 associated [...]
to
[...] specified by the "format" identity (see Section 2.1.2) associated [...]
What is 'encoded in the format specified by the "format" identity' I
assume this refers to the key value that is encrypted and stored in
encrypted-value, no?
- Same bullet in 2.1.4.1:
The "encrypted-value" node is the key, encrypted by the key
referenced by the "encrypted-by" node, encoded in the format
specified by the "format" identity Section 2.1.2 associated with
the ancestor node using this grouping.
Do I understand correctly that the encrypted-value always holds an
encrypted key? If so, a better name for the leaf would perhaps have
been encrypted-key and for the grouping encrypted-key-grouping. I
assume you did not pick this name since you wanted to use
encrypted-key for other purposes? This makes sense and may have led
to encrypted-key-value-grouping, in the sense of an encrypted
'key-value'? For the definition of the grouping, does it actually
matter that the value encrypted is a key? Or would it make sense to
rename the grouping to encrypted-value-grouping and then the second
bullet becomes:
The "encrypted-value" node holds the value, encrypted by the key
referenced by the "encrypted-by" node, encoded in the format
specified by the "format" identity (see Section 2.1.2) associated with
the ancestor node using this grouping.
Looking at the definition of encrypted-key-value-grouping, it seems
that the name encrypted-value-grouping would actually be more
appropriate, there is nothing in the definition that says that the
value must be a key for something.
And is it "the" ancestor node or "an" ancestor node? Perhaps the
possible confusion here is that ancestor node may mean different
things, schema tree versus data tree. What about this?
The "encrypted-value" node holds the value, encrypted by the key
referenced by the "encrypted-by" node, encoded in the format
specified by the "format" identity (see Section 2.1.2) associated with
the ancestor data node of the data node using this grouping.
Hm. Did I reverse engineer this correctly?
- In 2.1.4.2 last item, where do I see that the encrypted-password
node is encoded using the CMS EnvelopedData structure? There is no
"format" identity here, so this is hard coded to always have the CMS
EnvelopedData structure (or is it CMS EncryptedData structure, see
below)? OK, if I lookup the definition, it seems to use a hardcoded
format.
- Always use cleartext instead plain-text? I understand that some
people make a distinction between plain text, plaintext, and
cleartext. Does this document do that? I do not think so, and if so,
we may reduce confusion by picking just one term. On the other hand,
if there is a distinction made, then the document should perhaps be
explicit about it somewhere and define how these terms are used.
- The example in section 2.2 is helpful and much appreciated.
- s/confugration/configuration/
- The encrypted-password description says that it uses a" CMS
EnvelopedData structure, per Section 8 in RFC 5652, encoded using
ASN.1 distinguished encoding rules (DER)". However, section 8 of RFC
5652 defined EncryptedData and not EnvelopedData. So what is the
intention here?
- Following up on the previous point, it seems that the EncryptedData
format does not introduce a salt, i.e., if the same password is used
multiple times and they are all protected by the same key, then this
is visible since the encrypted formats will all be the same as well.
Is this something to be concerned about? Well, this is not really
a yang-doctor question...
- The encrypted-one-symmetric-key-format again refers to the
EnvelopedData per Section 8 in RFC 5652 but section 8 only defines
EncryptedData. So either the type name is wrong or the reference is
wrong.
- The encrypted-password container description refers to the
EnvelopedData and either the reference to section 8 is wrong or it
should be EncryptedData (see above). Is it OK to have a fixed
encrypted password format? Perhaps it is, I am just checking. (The
password leaf in RFC 7317 uses the ianach:crypt-hash format, i.e.,
it is somewhat extensible but then this module may target different
use cases as it deals with encrypted passwords instead of hashed
passwords.)
- I wonder why this is a SHOULD and not a MUST:
"Identifies the symmetric key's format. Implementations
SHOULD ensure that the incoming symmetric key value is
encoded in the specified format.";
This statement shows up several times when the key-format identities
are used. I wonder what this means to an implementer. If I receive a
key format (say one-symmetric-key-format) and a corresponding blob
of data, do I have to decode this to see whether the format works
out? If later the key-format is changed to something else (lets say
octet-string-key-format), do I reject such a change or is it OK as
long as the binary data I have would be a valid value given the new
format? Perhaps this is implementation specific? Well, since we deal
with keys, ...
- The definitions of the features symmetric-key-encryption and
private-key-encryption have copy and paste text from the definition
of the feature password-encryption. I think they need to be changed
to:
feature symmetric-key-encryption {
description
"Indicates that the server supports encryption of
symmetric keys.";
}
feature private-key-encryption {
description
"Indicates that the server supports encryption of
private keys.";
}
- If I have a 'octet-string-key-format' key and I use it to create an
encrypted key. How do I know which crypto algorithm is used? I have
not digged into it, but do the various CMS structures provide this
information somewhere? In the case of asymmetric keys, the format
gives a hint whether I am dealing with RSA keys or EC keys. For
symmetric keys, I am not sure in call cases how the algorithm is
determined.
- The asymmetric-key-pair-with-certs-grouping descriptions talk about
'IDevID' and 'LDevID' and 'TPM-protected asymmetric keys'. This
seems to call for additional references to explain what these are.
- s/is was unclear/it was unclear/
- RFC 8407 suggests that the XML registry Registrant Contact is:
Registrant Contact: The IESG.
This does not seem to be handled in a consistent manner if I look at
recent RFCs but I think the idea was to give the responsibility to
the IESG, assuming the IESG is a more stable entity than WGs.
--
last-call mailing list
last-call@xxxxxxxx
https://www.ietf.org/mailman/listinfo/last-call
