Subject: [Last-Call] Yangdoctors last call review of draft-ietf-rats-yang-tpm-charra-07

From: Mahesh Jethanandani via Datatracker <[email-masked]>
Date: Sun, 09 May 2021 09:35:17 -0700

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.

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

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 ../<where ever the dependent models are>/
[email-masked] 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:

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

   RPC: <tpm12-challenge-response-attestation> - 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.

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Tue, 11 May 2021 19:25:55 +0000

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
>
> 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
> operational context defined in 
> [I-D.ietf-rats-tpm-based-network-device-attest].
>
> General:
>

Done

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

> having attribute names with angle brackets is confusing.

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

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

> 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

> key/certificates/certificate/name

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.

> $ yanglint -p ../<where ever the dependent models are>/ ietf-tpm-remote-
> [email-masked] err : Invalid value "TPM_ALG_SHA256" in
> "TPM20-hash-algo" element.
> (/ietf-tpm-remote-attestation:TPM12-hash-algo) err : Identity "TPM_ALG_SHA1"
> (/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

>              "/tpm:tpm[tpm:tpm-firmware-version='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.

> 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

Fixed

>                    "Type of this certificate";

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

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

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

>   Checking nits according to 
> https://www.ietf.org/id-info/1id-guidelines.txt:

Not sure what this refers to.

>      'I-D.ietf-rats-tpm-based-network-device-attest')

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

>      Run idnits with the --verbose option for more detailed information

Latest results below

[email-masked]
XYM Extraction
No warnings or errors
Pyang Validation
 /var/yang/all_modules/[email-masked]:493: error: keyword 
"length" not in canonical order (see RFC 6020, Section 12)
Pyang Output
No warnings or errors
Confdc Output
[email-masked]:645: warning: The 'must' 
expression should have a tailf:dependency. If is doesn't, it will be checked 
for every commit.
[email-masked]: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

[email-masked]
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