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. , ? Same for container names . 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 ..// 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: * - 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: - 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.