Re: [Rats] Yangdoctors last call review of draft-ietf-rats-yang-tpm-charra-07

["Eric Voit (evoit)" <[e-mail]>]

Hi Mahesh,

Thanks for the detailed review.   Thoughts in-line, and attached is a full 
version of the models which embeds all the changes I made below.

> From: Mahesh Jethanandani, May 9, 2021 12:35 PM
>
> Reviewer: Mahesh Jethanandani
> Review result: On the Right Track
>
> 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.

Done

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

The initial introduction paragraph does generically reference terminology from 
draft-ietf-rats-architecture and 
draft-ietf-rats-tpm-based-network-device-attest.   But more explicit 
references could be helpful.  More on what is included is detailed below:

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

I have added the following text to the Intro:

"Specific terms imported from {{TPM2.0-Key}} and used in this document 
include: Endorsement Key (EK), Initial Attestation key (IAK), Local 
Attestation Key (LAK)."   Where {{TPM2.0-Key}} points to:
https://trustedcomputinggroup.org/wp-content/uploads/TCG_IWG_DevID_v1r2_02dec2020.pdf

In addition, I have changed all 'Cert' to 'Certificate" within the model.  I 
also have installed a specific reference to each of the enums in the model:

                enum endorsement-certificate {
                  value 0;
                  description
                    "Endorsement Key (EK) Certificate type.";
                  reference
                    "https://trustedcomputinggroup.org/wp-content/
                     uploads/TCG_IWG_DevID_v1r2_02dec2020.pdf
                     Section 3.11";
                }
                enum initial-attestation-certificate {
                  value 1;
                  description
                    "Initial Attestation key (IAK) Certificate type.";
                  reference
                    "https://trustedcomputinggroup.org/wp-content/
                     uploads/TCG_IWG_DevID_v1r2_02dec2020.pdf
                     Section 3.2";
                }
                enum local-attestation-certificate {
                  value 2;
                  description
                    "Local Attestation Key (LAK) Certificate type.";
                  reference
                    "https://trustedcomputinggroup.org/wp-content/
                     uploads/TCG_IWG_DevID_v1r2_02dec2020.pdf
                     Section 3.2";
                }

Note that this reference document points to {{TPM2.0-Key}}, and this document 
was formally approved by TCG at the end of April 2021.

> Same for Attester.

I have now explicitly referenced the Attester from the 
draft-ietf-rats-architecture.

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

Throughout the entire document, all angle brackets have been converted to 
single quote characters.

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

*Partially done*.  Now the only time we use upper case and underscores is when 
there is a direct 1:1 mapping between a specific TCG defined object, and a 
charra object.  By showing this name equivalence, it is felt that this would 
make the definition and domain inheritance explicit.

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

I have downshifted all the TPM used in YANG object names, with the *exception* 
of where there is a 1:1 mapping to specific TCG defined objects

> 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

All the prefixing reductions suggested in the three trees above have been 
made.

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

I have rewritten the definition of "nonce" to:

      description
        "A cryptographically generated random number which should
         not be predictable prior to its issuance from a random
         number generation function. The random number MUST be
         derived from an entropy source external to the Attester.";


> s/independent from the/independent of the/

Fixed with above rewording.

> 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-
> [e-mail] 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.
>

Interesting.    Both of the identities 'TPM_ALG_SHA256' and 'TPM_ALG_SHA1' 
have dual if-features set.  I.e., the YANG contains the line:
    if-feature "tpm12 or tpm20";
in the model  ietf-tcg-algs.yang

It looks like this an error in yanglint.   The error doesn't appear when I run 
the yanglint on https://yangcatalog.org/yangvalidator/validator

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

Yes, this is redundant.  I have removed the 'if-feature' in the four places 
where this recurs.

*Separate topic on YANGLINT*

What I do get from yanglint is:

warn: Incompatible types of operands "name" and "pcr-index" for comparison.
warn: Previous warning generated by XPath subexpression[118] "name = 
current()] an".
warn: Incompatible types of operands "name" and "pcr-index" for comparison.
warn: Previous warning generated by XPath subexpression[118] "name = 
current()] an".

These warnings suggest that the compound XPATH constraint which you suggested 
has a type mis-match?    I thought two booleans were the supposed to generated 
within:

      must '/tpm:rats-support-structures/tpm:tpms'
         + '/tpm:tpm[name = current()] and '
         + '/tpm:rats-support-structures/tpm:tpms'
         + '/tpm:tpm[tpm12-pcrs = current()]' {
        error-message "Acquiring this PCR index is not supported";
      }

Any suggestions would be appreciated here.

> Change Copyright year in the model from 2020 to 2021.

Done.

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

Fixed

> Could this description statement be improved?
>
>                  description
>                    "Type of this certificate";

Changed to : Function supported by this certificate from within the TPM.

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

This was driven by suggestions from Juergen as the tpm-name is used to 
validate configuration.  Therefore it must be shown as 'rw' although it is 
really read only.   As a result of you question, I added text on this:

"Although shown as 'rw', it is system generated.  Therefore it should not be 
possible for an operator to add or remove a TPM from the configuration."


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

Changed text to: "Need to verify that the certificate is provided is able to 
support Attestation on the targeted TPM."


> 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

Not sure what this refers to.

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

As this are auto-generated references, I won't worry about them here.


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

Latest results below

[e-mail]
XYM Extraction
No warnings or errors
Pyang Validation
 /var/yang/all_modules/[e-mail]:493: error: keyword 
"length" not in canonical order (see RFC 6020, Section 12)
Pyang Output
No warnings or errors
Confdc Output
[e-mail]:645: warning: The 'must' 
expression should have a tailf:dependency. If is doesn't, it will be checked 
for every commit.
[e-mail]:692: warning: The 'must' 
expression should have a tailf:dependency. If is doesn't, it will be checked 
for every commit.
yanglint Validation
warn: Incompatible types of operands "name" and "pcr-index" for comparison.
warn: Previous warning generated by XPath subexpression[118] "name = 
current()] an".
warn: Incompatible types of operands "name" and "pcr-index" for comparison.
warn: Previous warning generated by XPath subexpression[118] "name = 
current()] an".
yangdump-pro Validation
No warnings or errors


[e-mail]
XYM Extraction
No warnings or errors
Pyang Validation
No warnings or errors
Pyang Output
No warnings or errors
Confdc Output
No warnings or errors
yanglint Validation
No warnings or errors
yangdump-pro Validation
No warnings or errors

validator version: 1.1.0, xym version: 0.5, pyang version: 2.4.0, confdc 
version: confd-7.5, yanglint version: yanglint SO 1.10.7, yangdump-pro 
version: yangdump-pro 20.10-6

Thanks again,
Eric