Last Call Review of draft-ietf-rats-yang-tpm-charra-07
review-ietf-rats-yang-tpm-charra-07-yangdoctors-lc-jethanandani-2021-05-09-00

Status:

This review is looking at the draft from a YANG perspective. Since -03 version
of the draft, which I did an early review on, this document has come a long
way. With that said, I have marked it as On the Right Track, because of some of
the TODO items that the authors have left in the model, the fact that the IANA
section is not filled out, and some of the discussion in this review.

Summary:

This document defines a YANG RPC and a minimal datastore required to retrieve
attestation evidence about integrity measurements from a device following the
operational context defined in [I-D.ietf-rats-tpm-based-network-device-attest].

General:

Please run 'pyang -f yang' on the model for it to reformat the model per IETF
style and to fix some of the indentation and quoting issues in the model.

The draft references an acronym PCR, and several others, but does not define or
reference it. Suggest that a Terminology Section be added to the draft that
either defines acronyms used in the draft or references them if they have been
defined in another draft. It would also help to understand some of the terms
being used in the draft. For example, what is Attestation Key, and how is
different from Attestation Key Certificate (AC), or Attestation Key Cert, and
Initial Attestation key (inconsistent use of capitalization) Certificate type
or Local Attestation Key Certificate type. Same for Attester.

Why reference features with angle brackets, e.g. <TPMs>, <certificate-name>?
Same for container names <rats-support-structure>. Please replace all angle
brackets with " or ' quotes. Note, NETCONF operations use angle brackets, so
having attribute names with angle brackets is confusing.

Please remove underscores and uppercase characters from all identifiers. See
Section 4.3.1 of RFC 8407 for naming conventions for identifiers.

I see tpm, and TPM being used inconsistently. Suggest that all references to
TPM be replaced with tpm.

Also in general attribute names do not need to be prefixed with the parent
name. E.g.

   +--rw tpms
      +--rw tpm* [tpm-name]
         +--rw tpm-name                string
         +--ro hardware-based?         boolean
         +--ro tpm-physical-index?     int32 {ietfhw:entity-mib}?
         +--ro tpm-path?               string
         +--ro compute-node            compute-node-ref {tpm:TPMs}?
         +--ro tpm-manufacturer?       string
         +--rw tpm-firmware-version    identityref

could easily be replaced by:

   +--rw tpms
      +--rw tpm* [tpm-name]
         +--rw name                    string
         +--ro hardware-based?         boolean
         +--ro physical-index?         int32 {ietfhw:entity-mib}?
         +--ro path?                   string
         +--ro compute-node            compute-node-ref {tpm:TPMs}?
         +--ro manufacturer?           string
         +--rw firmware-version        identityref

and

         +--rw certificates
            +--rw certificate* [certificate-name]
               +--rw certificate-name            string
               +--rw certificate-keystore-ref?   ->
               /ks:keystore/asymmetric-keys/asymmetric-key/certificates/certificate/name
               +--rw certificate-type?           enumeration

could easily be replaced by:

         +--rw certificates
            +--rw certificate* [certificate-name]
               +--rw name                        string
               +--rw keystore-ref?               ->
               /ks:keystore/asymmetric-keys/asymmetric-key/certificates/certificate/name
               +--rw type?                       enumeration

What is cryptographic-strength algorithm? This is the first and only reference
to a term that has not been explained before.

s/independent from the/independent of the/

Have the models in the draft been validated with pyang and yanglint? I tried to
debug the following issue, but the number of groupings, and groupings within
groupings made for difficult reading of the model.

$ yanglint -p ../<where ever the dependent models are>/
ietf-tpm-remote-attestation@2021-03-16.yang err : Invalid value
"TPM_ALG_SHA256" in "TPM20-hash-algo" element.
(/ietf-tpm-remote-attestation:TPM20-hash-algo) err : Identity "TPM_ALG_SHA256"
is disabled by its if-feature condition.
(/ietf-tpm-remote-attestation:TPM20-hash-algo) err : Invalid value
"TPM_ALG_SHA1" in "TPM12-hash-algo" element.
(/ietf-tpm-remote-attestation:TPM12-hash-algo) err : Identity "TPM_ALG_SHA1" is
disabled by its if-feature condition.
(/ietf-tpm-remote-attestation:TPM12-hash-algo) err : Module
"ietf-tpm-remote-attestation" parsing failed.

Is both a when and an if-feature statement needed here? Isn't the setting of
'tpm-firware-version' to tpm12 not already indicate which version is being
supported?

      leaf-list tpm12-asymmetric-signing {
        when "../../tpm:tpms" +
             "/tpm:tpm[tpm:tpm-firmware-version='taa:tpm12']";
        if-feature "taa:TPM12";

Change Copyright year in the model from 2020 to 2021.

Nits:

s/Not a platform supported TPM20-hash-algo/This platform does not support
TPM20-hash-algo/ s/Not a platform supported TPM12-hash-algo/This platform does
not support TPM12-hash-algo/ s/available for use the Attesting
platform/available for use by the Attesting platform/

Could this description statement be improved?

                 description
                   "Type of this certificate";

Comments:

Security Considerations section:

Please separate out nodes that are read/write from those are read only or are
related to RPC.

Not clear on why the following is a security issue:

   *  <tpm-name> - Although shown as 'rw', it is system generated

For that matter, why is the following a security issue and not something that
is highlighted in the model itself?

   RPC: <tpm12-challenge-response-attestation> - Need to verify that the
   certificate is for an active AIK.

A idnits run of the draft continues to reveal a few issues. Some of them are
issues with the tool itself, i.e. outdated references, but others need
attention.

    Checking boilerplate required by RFC 5378 and the IETF Trust (see
  https://trustee.ietf.org/license-info):
  ---------------------------------------------------------------------------

     No issues found here.

  Checking nits according to https://www.ietf.org/id-info/1id-guidelines.txt:
  ---------------------------------------------------------------------------

     No issues found here.

  Checking nits according to https://www.ietf.org/id-info/checklist :
  ---------------------------------------------------------------------------

  ** There are 40 instances of too long lines in the document, the longest
     one being 53 characters in excess of 72.

  Miscellaneous warnings:
  ---------------------------------------------------------------------------

  == Line 180 has weird spacing: '...r-index    pcr...'

  == Line 243 has weird spacing: '...te-name    cer...'

  == Line 275 has weird spacing: '...-number    uin...'

  == Line 332 has weird spacing: '...version    ide...'

  == Line 336 has weird spacing: '...sh-algo    ide...'

  -- The document date (April 2021) is 24 days in the past.  Is this
     intentional?

  Checking references for intended status: Proposed Standard
  ---------------------------------------------------------------------------

     (See RFCs 3967 and 4897 for information about using normative
     references to lower-maturity documents in RFCs)

  == Missing Reference: 'TPM20-hash-algo' is mentioned on line 335, but not
     defined

  == Outdated reference: A later version (-12) exists of
     draft-ietf-rats-architecture-11

  ** Downref: Normative reference to an Informational draft:
     draft-ietf-rats-architecture (ref. 'I-D.ietf-rats-architecture')

  == Outdated reference: A later version (-02) exists of
     draft-ietf-rats-reference-interaction-models-01

  ** Downref: Normative reference to an Informational draft:
     draft-ietf-rats-reference-interaction-models (ref.
     'I-D.ietf-rats-reference-interaction-models')

  ** Downref: Normative reference to an Informational draft:
     draft-ietf-rats-tpm-based-network-device-attest (ref.
     'I-D.ietf-rats-tpm-based-network-device-attest')

  -- Possible downref: Non-RFC (?) normative reference: ref. 'TCG-Algos'

     Summary: 4 errors (**), 0 flaws (~~), 8 warnings (==), 2 comments (--).

     Run idnits with the --verbose option for more detailed information
     about the items above.