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

From: "Eric Voit (evoit)" <[email-masked]>
Date: Thu, 17 Dec 2020 15:10:21 +0000

Thanks Mahesh for the review!    Some updates based on your review below, one 
question** and other thoughts in line...

> -----Original Message-----
> From: Mahesh Jethanandani via Datatracker <[email-masked]>
> Sent: Monday, November 30, 2020 10:08 PM
> To: [email-masked]
> Cc: [email-masked]; [email-masked]
> Subject: Yangdoctors early review of draft-ietf-rats-yang-tpm-charra-03
>
> 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

There are groupings which are used only once in this document, by are used 
again within augmenting YANG
models in other drafts.  See:
draft-birkholz-rats-network-device-subscription, 
ietf-tpm-remote-attestation-stream.yang
which uses groups:
tpm20-attestation, tpm12-attestation, TPM12-hash-algo, bios-event-log, 
ima-event-log, network-equipment-boot-event-log

> or a grouping that
> has only left e.g. tpm-name. Please collapse these grouping where they are
> used.

We can certainly do this.   I thought it would be helpful to unify to a single
place the definition of tpm-name.
**Is there a place in the IETF YANG documentation which suggests that
unifying definitions in this way is not a good practice?

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

done

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

Looks like quotes were needed around the file name.  Fixed.

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

This is a good ask, I will ask another author do help with this as it will 
show a common understanding of the model.

> 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

The is not symmetry in what is returned from the two quotes.   As we are in 
charge of carrying the TCG defined structures, symmetry can only be imposed by 
applying unnecessary complexity on TPM1.2 solutions.

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

It is conceivable that one device might have both types of TPMs.  However one 
TPM cannot call another, as they only react to external requests.

If a device happens to support both, you will know by both features being 
exposed.  And as a client you will have to call both RPCs.  As this will be 
uncommon, it seems a better optimization than having one RPC call.  This RPC 
would have an unnecessarily complex union of input parameters.

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

Made it:
          "A component within this composite device 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.";

Made it:
          "Indicates the compute node measured by this TPM.";

> s/agross/across/

done

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

Looks like one of the copyright statements was duplicated.  It is now removed

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

All canonical order items fixed in PYANG

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

I will do that.

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

This should be covered.  Terms are inherited from the parent document: 
draft-ietf-rats-tpm-based-network-device-attest.   Indicating this a first 
sentence of the Intro section, which is:
   This document is based on the terminology defined in the
   [I-D.ietf-rats-architecture] and uses the operational context defined
   in [I-D.ietf-rats-tpm-based-network-device-attest] as well as the
   interaction model and information elements defined in
   [I-D.birkholz-rats-reference-interaction-model].

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

Reference made.

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

Good point.  Made them 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.

Fixed

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

Don't see any IP addresses used.  Tool parsing error perhaps?

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

Checking the process question with WG chair

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

As this auto-generated, this looks to be a tooling issue.

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

Due to the test being run well after the document was generated.

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

Checking the process question with WG chair

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

This is correct

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

Fixed

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