Re: [netconf] Yangdoctors last call review of draft-ietf-netconf-crypto-types-18

[Kent Watsen <[e-mail]>]

Thank you for your review, Juergen!

Below are responses to your comments.

PS: this response took longer because I had to triage with Security Experts on a couple points.

Kent


> 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/

Fixed.


> - 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?

I find using tree-diagram like notation for features, identities, and typedefs helpful, especially in showing the “if-feature” constraints and hierarchical relationships between items.  I recommend adding these enhancements to RFC 8340.  It would be helpful if someone could whip up an I-D so these documents could reference the I-D instead of having the "The diagram above uses syntax that is similar to but not defined in [RFC8340]” language.

That said, I also used a "rfc8340-like" diagram to list all the groupings in a module.  It seems like the consistent thing to do, but I can’t imagine the RFC 8340 being updated to support that output, so I replaced that diagram, with a normal "xml2rfc" list, in the entire suite of drafts.


> - "Identities use to specify uncommon formats are enabled [...]" is
>  this correct or should it be "used to specify”?

Fixed.


> - 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 […]

Fixed, but see below...


>  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?

Not only was the solution unclear, but it was rather upside-down in terms of how to encode encrypted values.  Please see the “diffs” to get a sense for how the solution was changed.


> - 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?

No.  Encryption was initially for just keys, but since expanded to support passwords.


>  ...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?

Correct.


> 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.

Yes, especially since the original definition the WG added support for encrypted passwords…so the grouping name “encrypted-key-value-grouping” is now a misnomer.  I just s/encrypted-key-value-grouping/encrypted-value-grouping/g to resolve this naming issue.  This better aligns the naming, as you say, as now the "encrypted-value-grouping” contains a node called "encrypted-value”.


>  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.

+1


>  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.

You’re correct but, looking at the updated text (see diffs), the word “ancestor” was removed, so no action to take now.


>  Hm. Did I reverse engineer this correctly?

You did well, but it was unfortunate that the draft wasn’t clearer.  I do hope the current version is better.


> - 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

Yes, the format for encrypted passwords was hardcoded (which was not a great solution).  

The new solution resolves this by treating encrypted passwords just like any other encrypted value.  Specifically:

      case encrypted-password {
        if-feature password-encryption;
        container encrypted-password {
          description
            "A container for the encrypted password value.";
          uses encrypted-value-grouping;
        }
      }





> .- 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.

Yes, always “cleartext”.   I just replaced “plaintext” with “cleartext” everywhere.


> - The example in section 2.2 is helpful and much appreciated.

I now even more so as its been extended to show a cleartext/encrypted password as well.  :)


> - s/confugration/configuration/

Fixed.


> - 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?

Yeah, this part was a mess.  Please see the latest.


> - 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...

This is not an issue, as follows:

1) When the encrypting key is symmetric, a random IV should be used, thus identical inputs will produce different outputs.
2) When the encrypting key is asymmetric, a random symmetric key is generated, thus identical inputs will produce different outputs.


> - 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.

This text was mostly rewritten in, hopefully, a more exact manner…


> - 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.)

Again, new text abounds.  To point, there are no more hardcoded formats, all formats are identity-based now.


> - 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, ...

It currently a "SHOULD” because some implementations may assume that clients always configure perfect values, and so it becomes squishy as to if they MUST.   If a server fails to validate the base64 contents (which are outside YANG validation) as they’re being applied, then the server may run into nasty exceptions when needing to use the value at runtime.  A nice system will decode the base64 values and verify that they contain valid values during the commit process.

Make sense?  - leave as SHOULD okay?


> - 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.";
>     }

Fixed!


> - 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 all cases how the algorithm is
>  determined.

Exactly right, the OctetString is not bound to an algorithm (because the algorithm-identifier effort collapsed), which is now plainly stated in the identity’s “description” statement:

       How the associated algorithm is known is outside the
       scope of this module.  This statement also applies when
       the octet string has been encrypted.";

That said, when the octet string is used to create an EncryptedData structure, the structure encodes an algorithm identifier (e.g., aes128-cbc).  The “aes128” part is unimportant, as already there is the “encrytped-by” node that points to the octet string that, per above, the application needs to know is an AES-128 key.  The important part, though, is the identification of the blocking mode (e.g., “cbc”) and/or padding (e.g., pkcs5).

If applications have issue with limitations that come with the OctetString type, they are welcomed to use the OneSymmetricKey structure, which self-identifies the keys algorithm.


> - 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.

True, but I instead removed the anecdotal comments altogether.


> 
> - s/is was unclear/it was unclear/

Fixed!


> - 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.

Fixed, in the entire suite of drafts.



Thank you again for your review!

Kent