Subject: [Rats] Francesca Palombini's No Objection on draft-ietf-rats-yang-tpm-charra-16: (with COMMENT)

From: Francesca Palombini via Datatracker <[email-masked]>
Date: Wed, 09 Mar 2022 11:02:04 -0800

Francesca Palombini has entered the following ballot position for
draft-ietf-rats-yang-tpm-charra-16: No Objection

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ 
for more information about how to handle DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-rats-yang-tpm-charra/

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Thank you for the work on this document

Thank you to Nancy Cam-Winget for noting in the shepherd write up that the YANG
errors in the DT validation are a bug of the tool - however since the shepherd
write up was last updated 6 months ago and the last YANG Doctors review was 9
versions ago, I'd like a confirmation from Roman and/or the authors that the
bug is still actual and the two YANG validation errors are not relevant.

I have some minor comments below. I'll appreciate answers to them and I hope
they help improve the document.

Francesca

1. -----

FP: Please expand TPM on first use.

2. -----

(_TPM Quote_ primitive operation)

FP: it is unclear where this terminology comes from ( _TPM Quote_ operation or
primitive operation, but also _TPM PCR Extend_)

3. -----

 At this point 0-31 is viable.

FP: I do not think "at this point" is a valid way to put that ranges could be
expanded. It makes me wonder how and what's the motivation for the current
restriction, as well as the process behind a possible expansion. If the
community sees a need for a future range expansion, why not describe that here.

4. -----

          It is quite
          possible this leafref will eventually point to another YANG
          module's node.

FP: would this YANG module be updated in that case? I think that if this text
stays, it needs to be more detailed.

5. -----

            with leading zeros any in Quotes returned from the TPM.

FP: the end of this sentence does not parse for me.

6. -----

            Note that a nonce sent into a TPM will typically be 160 or 256
            binary digits long.  (This is 20 or 32 bytes.) So if fewer
            binary are sent, this nonce object will be padded
            with leading zeros any in Quotes returned from the TPM.
            Additionally if more bytes are sent, the nonce will be trimmed
            to the most significant binary digits.";

FP: How is the length decided if the nonce is shorter than 20 bytes long? Will
it always be padded to 20 bytes or can it be padded to 32 bytes? Maybe adding a
sentence about how the length is established can be useful here.

7. -----

           "The numbers/indexes of the PCRs. At the moment this is limited
            to 32.   In addition, any selection of PCRs MUST verify that

FP: Same comment than 3. "at the moment"

8. -----

     'unsigned-pcr-values'.   By comparing this information to the
            what has previously been validated, it is possible for a

FP: s/to the what/to what

9. -----

         an unsigned PCR value is actually that that within a quote.

FP: s/that that/that

10. -----

          e.g. smart NICs, is still a TODO.";

FP: Also the same comment as 3. - "still a TODO" seems like the wrong
terminology to mean something out of scope or to be extended later.

11. -----

                   "Content of an log event which matches 1:1 with a

FP: s/an/a

12. -----

       "This module defines a identities for asymmetric algorithms.

FP: remove "a"

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Wed, 9 Mar 2022 23:36:15 +0000

Hi Francesca,

Thanks for the review.  Thoughts in-line...

> From: Francesca Palombini, March 9, 2022 2:02 PM
>
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this 
> introductory
> paragraph, however.)
>
>

Yes, the YANG errors have persisted due to tooling.

> FP: Please expand TPM on first use.

TPM is expanded with an "s" in the introduction.  Do your prefer if we remove 
the "s"?

> primitive operation, but also _TPM PCR Extend_)

I have made it more generic since the specific commands are listed in later in 
the document.  Updated the intro via a GitHub pull request in a way which 
covers the concepts generically.  So now the text is saying:

"One TPM, or multiple TPMs in the case of a Composite Device, are required in 
order to use the YANG module defined in this document. Each TPM is used as a 
root of trust for storage (RTS) in order to store system security measurement 
Evidence.  And each TPM is used as a root of trust for reporting (RTR) in 
order to retrieve attestation Evidence.  This is done by using a YANG RPC to 
request a quote which exposes a rolling hash the security measurements held 
internally within the TPM."

> community sees a need for a future range expansion, why not describe that

I have created a GitHub Pull request with the following:

"Valid index number for a PCR.  A {{TPM2.0}} compliant PCR index extends from 
0-31.  Custom TPM implementations can have additional PCRs.  An implementation 
may deviate away the range constraint should a platform wish to support more 
PCRs.";

> stays, it needs to be more detailed.

What will happen is vendors will augment this IETF models with other leafrefs 
to non IETF YANG structures.  And the IETF leafrefs will go unpopulated.  As 
IETF models are not dominant, this is a common practice.

> FP: the end of this sentence does not parse for me.

Made it:

So if fewer binary are sent, this nonce object will always be padded up to 20 
or 32 bytes with leading zeros when that nonce value is returned as part of 
response generated by the TPM.

> sentence about how the length is established can be useful here.

The "always" in my previous response hopefully answers part of the question. 
Optimal nonce lengths has much variation by chip (and how frequently quotes 
can be generated), so it is better not to take a position in this document.

> FP: Same comment than 3. "at the moment"

Removed "at the moment this is limited to 32", as this not necessary to point 
out within this specific definition.  It is better to just leave up to the 
definition of the PCR range.

> FP: s/to the what/to what

Fixed

>          an unsigned PCR value is actually that that within a quote.

Fixed

> terminology to mean something out of scope or to be extended later.

Made "is not is covered".

>                    "Content of an log event which matches 1:1 with a

Fixed

>        "This module defines a identities for asymmetric algorithms.

Done.

Thanks!
Eric