A YANG Data Model for Challenge-Response-based Remote Attestation Procedures using TPMs
draft-ietf-rats-yang-tpm-charra-21

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"

The document has 8 authors, which exceeds the recommended author limit. I
assume the sponsoring AD has agreed that this is appropriate?

DOWNREF from this Standards Track doc to [netequip-boot-log]. 
[netequip-boot-log] seems to be a Linux kernel README, which I have
a hard time accepting as a non-IETF standards document.

DOWNREF from this Standards Track doc to [yang-parameters]. This could become
and informational reference, since the defining RFC6020 is normatively cited.

DOWNREF from this Standards Track doc to [xml-registry]. This could become
and informational reference, since the defining RFC3688 is normatively cited.

Thanks to Roni Even for their General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/LVJgDXJ4Bwr2DuJ2ytGxONSTSQ8).

-------------------------------------------------------------------------------
All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Document still refers to the "Simplified BSD License", which was corrected in
the TLP on September 21, 2021. It should instead refer to the "Revised BSD
License".

"RATS ", paragraph 1, nit:
> cument defines YANG RPCs and a small number of configuration nodes required
>                              ^^^^^^^^^^^^^^^^^
Specify a number, remove phrase, use "a few", or use "some".

Section 2. , paragraph 2, nit:
> tion 2.1.2.3 with prefix 'taa'. Additionally references are made to [RFC8032
>                                 ^^^^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Additionally".

Section 2.1.1.5. , paragraph 19, nit:
> n Quotes returned from the TPM. Additionally if more bytes are sent, the non
>                                 ^^^^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Additionally".

Section 2.1.1.5. , paragraph 34, nit:
> By comparing this information to the what has previously been validated, it
>                                  ^^^^^^^^
Did you mean "what"?

Section 2.1.1.5. , paragraph 35, nit:
> an unsigned PCR value is actually that that within a quote. If there is a dif
>                                   ^^^^^^^^^
Possible typo: you repeated a word.

Section 2.1.1.5. , paragraph 36, nit:
> ing ima-event { description "Defines an hash log extend event for IMA measur
>                                      ^^
Use "a" instead of "an" if the following word doesn't start with a vowel sound,
e.g. "a sentence", "a university".

Section 2.1.1.5. , paragraph 43, nit:
> type binary; description "Content of an log event which matches 1:1 with a u
>                                      ^^
Use "a" instead of "an" if the following word doesn't start with a vowel sound,
e.g. "a sentence", "a university".

Section 2.1.1.5. , paragraph 43, nit:
> ned within the log. Log entries subsequent to this will be passed to the requ
>                                 ^^^^^^^^^^^^^
Consider using "after".

Section 2.1.1.5. , paragraph 43, nit:
> e beginning of the log. Entries subsequent to this will be passed to the requ
>                                 ^^^^^^^^^^^^^
Consider using "after".

Section 2.1.1.5. , paragraph 43, nit:
> extraction. The next log entry subsequent to this timestamp is to be sent.";
>                                ^^^^^^^^^^^^^
Consider using "after".

Section 2.1.1.5. , paragraph 44, nit:
> ue 0; description "The TPM currently is currently running normally and is re
>                            ^^^^^^^^^^^^^^^^^^^^^^
Adverb repetition.

Section 2.1.1.5. , paragraph 48, nit:
> escription "This module defines a identities for asymmetric algorithms. Copy
>                                 ^^^^^^^^^^^^
The plural noun "identities" cannot be used with the article "a". Did you mean
"a identity" or "identities"?

Section 2.1.1.5. , paragraph 50, nit:
> ficient cryptographic protection. However it is still useful for hash algori
>                                   ^^^^^^^
A comma may be missing after the conjunctive/linking adverb "However".

Section 2.1.2.3. , paragraph 5, nit:
> ietf-tpm-remote-attestation.yang. However the full definition of Table 3 of
>                                   ^^^^^^^
A comma may be missing after the conjunctive/linking adverb "However".

Section 2.1.2.3. , paragraph 13, nit:
> as 'rw', it is system generated. Therefore it should not be possible for an
>                                  ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Therefore".

Section 2.1.2.3. , paragraph 18, nit:
> ulnerabilities on those systems. Therefore RPCs should be protected by NACM
>                                  ^^^^^^^^^
A comma may be missing after the conjunctive/linking adverb "Therefore".

These URLs in the document can probably be converted to HTTPS:
 * http://trustedcomputinggroup.org/resource/tcg-algorithm-registry/TCG-_Algorithm_Registry_r1p32_pub

I will echo Lars' concern about the number of authors on this document.  The guideline is five; is there a good reason we need to list eight here?

I also concur with Francesca's points about "at this moment", etc. (specifically, her points 3, 7, and 10).

Discuss level issues resolved.

3 points to close on before final approval:
 - I still need to close one of the issues with Eric regarding the config handling (just to ensure that the draft is clear).

 - I would also like to check the YANG warnings that are being flagged to understand if they are tooling or model issues which need to be fixed.

 - I would also like to see an ack from Jan if he hasn't already done so after his review, but can chase if needed.

Thank you.

Thanks for this document. 

I have also noticed similar reference anomalies in this document like Benjamin kaduk did. For example - I could not find "ietf-basic-remote-attestation" module. Hence supporting his discuss. 

Don't we need to resolve the 2 YANG validation errors?

I know that there is still a fair amount of editing effort underway, and
so am not concerned that some of my comments from the -16 remain
unaddressed.  I will try to repeat them here along with a handful of new
comments, while posting an updated ballot position with the primary goal
of removing my DISCUSS.

Many thanks for adding the appendices, they really help lay out how the
pieces fit together in a way that the external references used in the
-16 couldn't.

With regards to my previous discuss point (3), it seems that [ima-log]
still points ot the TCG "canonical event log" document with no mention
of Linux IMA.
Also, the string "netequip-boot-log" still appears in a YANG description
(where it can't be an xml2rfc reference) pointing to the linux IMA docs;
presumably we'd want it to point to Appendix B of [this RFC].

       leaf event-number {
         type uint32;
         description
           "Unique event number of this event which monotonically
            increases. [...]

I think we should say "within a given even log", maybe at the end of
this sentence.

       leaf-list event-data {
         type binary;
         description
           "The event data size determined by event-size. For more
            see ";

I think there was a botched edit here.

     grouping ima-event {
       description
         "Defines an hash log extend event for IMA measurements";
       reference
         "ima-log:
          https://www.trustedcomputinggroup.org/wp-content/uploads/
          TCG_IWG_CEL_v1_r0p30_13feb2021.pdf  Section 4.3";

Is section 4.3 the best section to reference?  I only see specifics
about,
e.g., the hash algorithm being encoded as a string later on, circa
§5.1.6.

       leaf filename-hint {
         type string;
         description
           "File that was measured";

Is this just the file name, a full path, either, ...?

     identity TPM_ALG_SYMCIPHER {
       if-feature "tpm20";
       base tpm20;
       base symmetric;
       description
         "Object type for a symmetric block cipher";

Thanks for adding "base symmetric".  Please confirm that "base
object_type" is not needed (as I thought I saw it in the TCG doc).

NITS

Section 1

   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.

"rolling hash of"

  Appendix B.  IMA for Network Equipment Boot Logs        
       
  Network equipment can generally implement similar IMA-protected   
  functions to generate measurements (Claims) about the boot process of      
  a device and enable corresponding remote attestation.  Network 
  Equipment Boot Logs combine the measurement and logging of boot    
  components and operating system components (executables and files)  
  into a single log file in identical IMA format.      

"single log file in a format identical to the IMA format"