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 and not starting with 'example-' Extracting 'ietf-tpm-remote-attestation' WARNING: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 1917: misplaced ERROR: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 1967 - YANG module 'ietf-tcg-algs' with no and not starting with 'example-' Extracting 'ietf-tcg-algs' WARNING: 'draft-ietf-rats-yang-tpm-charra-03.txt', Line 2824: misplaced 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 (--).