[yang-doctors] Yangdoctors last call review of draft-ietf-netconf-crypto-types-18

[Jürgen Schönwälder via Datatracker <[e-mail]>]

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.