[yang-doctors] Yangdoctors early review of draft-ietf-rats-yang-tpm-charra-03

[Mahesh Jethanandani via Datatracker <[e-mail]>]

Reviewer: Mahesh Jethanandani
Review result: On the Right Track

Status:

I am not an expert in Trusted Platform Module (TPM). If my understanding of
TPM, feel free to educate me and everyone else. This review is looking at the
draft from a YANG perspective. With that said, I have marked it as On the Right
Track, because of some of the points discussed below.

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:

The YANG modules uses groupings that are used only once or a grouping that has
only left e.g. tpm-name. Please collapse these grouping where they are used.

And there are groupings like TPM20-asymmetric-signing-algo and
TPM12-asymmetric-signing-algo that are not used. Please remove any unused
groupings.

The draft has problems with extraction of the modules. See message below.

 ERROR: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 373 - YANG module
 'ietf-tpm-remote-attestation' with no <CODE BEGINS> and not starting with
 'example-'

Extracting 'ietf-tpm-remote-attestation'
   WARNING: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 1917: misplaced
   <CODE ENDS> ERROR: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 1967 -
   YANG module 'ietf-tcg-algs' with no <CODE BEGINS> and not starting with
   'example-'

Extracting 'ietf-tcg-algs'
   WARNING: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 2824: misplaced
   <CODE ENDS>

The draft lacks examples, and therefore there is no way to validate the
model(s). Please add an example.

Is there a reason for TPM 1.2 and TPM 2.0 to not have symmetry in the model
definition? See the portion of the tree diagram below.

       |     +--rw TPM12-hash-algo?        identityref
       |     +--rw TPM12-pcrs*             pcr
       |     +--rw tpm20-pcr-bank* [TPM20-hash-algo]
       |     |  +--rw TPM20-hash-algo    identityref
       |     |  +--rw pcr-index*         tpm:pcr

Is it possible for a TPM 1.2 to call a TPM 2.0 RPC and vice-versa? I understand
there is a feature statement, but if I understand from one of the authors, a
device can conceivably support both TPM 1.2 and TPM 2.0.

Nits:

This description statement can be simplified:
        description
          "A components in this composite device that RATS which
          supports TPM operations.";

This sentence construct does not sound right:

          description
            "When there is more that one TPM, this indicates for which
            compute node this TPM services.";

s/agross/across/

Comments:

The module has plenty of errors as reported by pyang.

ietf-tpm-remote-attestation.yang:65: warning: RFC 8407: 3.1: The IETF Trust
Copyright statement seems to be missing (see pyang --ietf-help for details).
ietf-tpm-remote-attestation.yang:144: error: keyword "must" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:148: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:155: error:
keyword "default" not in canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:164: error: keyword "must" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:168: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:175: error:
keyword "default" not in canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:185: error: keyword "must" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:188: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:194: error:
keyword "default" not in canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:203: error: keyword "must" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:206: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:214: error:
keyword "default" not in canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:599: error: keyword "mandatory" not in
canonical order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:600: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:900: error:
keyword "must" not in canonical order, expected "type" (see RFC 6020, Section
12) ietf-tpm-remote-attestation.yang:903: error: keyword "type" not in
canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:945: error: keyword "must" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:948: error: keyword "type" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:1178: error:
keyword "must" not in canonical order, expected "type" (see RFC 6020, Section
12) ietf-tpm-remote-attestation.yang:1181: error: keyword "type" not in
canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:1278: error: keyword "when" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:1278: error: keyword "when" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:1288: error:
keyword "when" not in canonical order, expected "type" (see RFC 6020, Section
12) ietf-tpm-remote-attestation.yang:1288: error: keyword "when" not in
canonical order (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:1298: error: keyword "when" not in canonical
order, expected "type" (see RFC 6020, Section 12)
ietf-tpm-remote-attestation.yang:1298: error: keyword "when" not in canonical
order (see RFC 6020, Section 12) ietf-tpm-remote-attestation.yang:1308: error:
keyword "when" not in canonical order, expected "type" (see RFC 6020, Section
12) ietf-tpm-remote-attestation.yang:1308: error: keyword "when" not in
canonical order (see RFC 6020, Section 12)

Having 3 pages of a tree diagram is helpful in an appendix. Maybe an abridged
diagram with explanation for the different sections of the tree diagram would
be more helpful. Suggest that --tree-depth and --tree-path options be used in
pyang to reduce the size of the tree and to break it up into smaller pieces
that can then be explained.

A terminology and acronym section would be nice for folks who are not familiar
with all the terms and acronyms used in the document. If terms are defined in
some other document, a statement to that effect would be nice.

Section 2.2.1

A reference is made to ietf-tcp-algs.yang but is missing a reference (to this
document).

Section 2.2.1.1

Is an implementation supposed to support all the types of attestation? If not,
should each of them be features?

A idnits run of the draft reveals a few issues. Please address them.

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

  ** The abstract seems to contain references
     ([I-D.ietf-rats-tpm-based-network-device-attest]), which it shouldn't.
     Please replace those with straight textual mentions of the documents in
     question.

  == There are 1 instance of lines with non-RFC6890-compliant IPv4 addresses
     in the document.  If these are example addresses, they should be changed.

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

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

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

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

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

  -- The document date (September 30, 2020) is 61 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 147, but not
     defined

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

  -- Unexpected draft version: The latest known version of
     draft-ietf-netconf-keystore is -19, but you're referring to -20.

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

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

  == Outdated reference: A later version (-05) exists of
     draft-ietf-rats-tpm-based-network-device-attest-04

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

  -- Obsolete informational reference (is this intentional?): RFC 5246
     (Obsoleted by RFC 8446)

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