Re: [Rats] Benjamin Kaduk's Discuss on draft-ietf-rats-yang-tpm-charra-16: (with DISCUSS and COMMENT)

[Henk Birkholz <[e-mail]>]

Thanks Eric,
hi Ben!

there is no perfect choice here.

I hear your point that an "Linux IMA document" is not normative. Yes.

While that is technically correct, corresponding normative references 
are actually not better. We could inject a normative reference, such as 
the documents found at:

> https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2p0-v1p05p_r14_pub.pdf

or

> https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClient_PFP_r1p05_v23_pub.pdf

Would it be possible to retain IMA kernel reference as a informative 
reference in this case? Effectively, I'd find that more useful to 
implementers. Or maybe both? Maybe you could provide us with some 
guidance here.

Viele Grüße,

Henk

On 09.03.22 21:02, Eric Voit (evoit) wrote:
> Hi Benjamin,
> 
> Thanks for the comments.  Tweaks in line.  Everything was covered 
> excepting one ***.     Each fixed/updated area in the document 
> corresponds to a pull request in:
> 
> https://github.com/ietf-rats-wg/basic-yang-module 
> <https://github.com/ietf-rats-wg/basic-yang-module>
> 
> It is not possible to post a new draft yet since we are close to the 
> IETF, so hopefully this will be sufficient for now.
> 
>  > From: Benjamin Kaduk, March 9, 2022 12:58 AM
> 
>  >
> 
>  > Benjamin Kaduk has entered the following ballot position for
> 
>  > draft-ietf-rats-yang-tpm-charra-16: Discuss
> 
>  >
> 
>  > 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- 
> <https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/>
> 
>> ballot-positions/ 
> <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/ 
> <https://datatracker.ietf.org/doc/draft-ietf-rats-yang-tpm-charra/>
> 
>  >
> 
>  >
> 
>  >
> 
>  > ----------------------------------------------------------------------
> 
>  > DISCUSS:
> 
>  > ----------------------------------------------------------------------
> 
>  >
> 
>  > (1) Section 2 references the "ietf-basic-remote-attestation YANG module",
> 
>  > but that appears to be an older name for what got renamed to the
> 
>  > "ietf-tpm-remote-attestation" module at time of WG adoption.
> 
> Good catch.  Fixed.
> 
>  > (2) There are some issues with references, either pointing to the wrong
> 
>  > document or to a draft version of a specification when there is a final
> 
>  > version available.
> 
>  >
> 
>  > (2.1) The YANG reference stanza for "feature ima" (in the
> 
>  > ietf-tpm-remote-attestation module) still refers to a draft version 
> of the
> 
>  > Canonical Event Log specification.
> 
> Fixed.  Updated to the just approved version.
> 
>  > (2.2) The YANG reference stanza for "grouping ima-event" (in the same
> 
>  > module) does the same.
> 
> Fixed.  Updated to the just approved version.
> 
>  > (2.3) The PDF link in the description of "feature tpm20" (in the
> 
>  > ietf-tcg-algs YANG module) appears to point to a draft version of the
> 
>  > specification.  Since the link is to a 2011 version, I trust that the
> 
>  > final version is already available, and the fix is just to update the
> 
>  > link.
> 
> Fixed the link to the latest 2019 approved version.  Also there was a 
> new version update everywhere the TPM2.0 Architecture was referenced.  
> They were updated as well.
> 
>  > (2.4) The PDF link in the descriptions of all three "enum" values of
> 
>  > "leaf type" in "container certificates" in the 
> ietf-tpm-remote-attestation
> 
>  > module points to a draft version of the TCG specification.
> 
> Fixed.  All now point to the official Oct 2021 version:
> 
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-2p0-Keys-for-Device-Identity-and-Attestation_v1_r12_pub10082021.pdf 
> <https://trustedcomputinggroup.org/wp-content/uploads/TPM-2p0-Keys-for-Device-Identity-and-Attestation_v1_r12_pub10082021.pdf>
> 
>  > (2.5) The PDF link in the description of "feature tpm20" in the
> 
>  > ietf-tcg-algs YANG module points to a draft version of the TCG
> 
>  > specification.
> 
> Duplicate of comment (2.3), fixed.
> 
>  > (2.6) The description for "identity TPM_ALG_HMAC" in the ietf-tcg-algs
> 
>  > YANG module points to RFC 2014 as one reference, but HMAC is RFC 2104.
> 
> Fixed.   Eight years ago my fingers were programmed that way I guess.
> 
>  > (2.7) The reference for [TPM2.0-Arch] links to a draft version of the
> 
>  > TCG specification.
> 
> Fixed, per above.
> 
>  > (2.8) The reference for [TPM2.0-Key] links to a draft version of the TCG
> 
>  > specification.
> 
> Fixed, per above
> 
>  > (3) Some of the references don't seem to match up with their slugs --
> 
>  > e.g., [ima-log] seems to point to the TCB "canonical event log 
> format" and
> 
>  > [netequip-boot-log] (and the YANG reference stanza for "feature
> 
>  > netequip_boot") points to what looks like a Linux IMA document.  
> While the
> 
>  > description in "grouping network-equipment-boot-event-log" does give some
> 
>  > indication that [netequip-boot-log] is intended to be very similar to the
> 
>  > IMA format, I don't think we can reasonably just directly reference the
> 
>  > IMA spec and call it [netequip-boot-log] without some additional
> 
>  > clarification either in the reference entry or where it is used.
> 
> *** Defer to Henk who has a few questions here.
> 
>  > ----------------------------------------------------------------------
> 
>  > COMMENT:
> 
>  > ----------------------------------------------------------------------
> 
>  >
> 
>  > Section 2.1.1.1
> 
>  >
> 
>  >    *  'TPMs': Indicates that multiple TPMs on the device can support
> 
>  >       remote attestation.  This feature is applicable in cases where
> 
>  >       multiple line cards are present, each with its own TPM.
> 
>  >
> 
>  > It sounds like this is intending to emphasize "TPMs" as opposed to "TPM"
> 
>  > singular; making the plural 's' have semantic value seems to risk
> 
>  > confusion, and perhaps a "multi-tpm" feature name would be less
> 
>  > risk-prone.
> 
> The longer feature name would make almost all of trees contained in the 
> figures line wrap.  Currently none of the lines word wrap.   While the 
> change could be made, it doesn't seem unreasonable to instead optimize 
> for easier readability in the figures.
> 
>  > Section 2.1.1.5
> 
>  >
> 
>  >    Container 'rats-support-structures':  This houses the set of
> 
>  >       information relating to a device's TPM(s).
> 
>  >
> 
>  >    Container 'tpms':  Provides configuration and operational details for
> 
>  >       each supported TPM, including the tpm-firmware-version, PCRs which
> 
>  >
> 
>  > I believe that in the current YANG module, "tpms" is a child of
> 
>  > "rats-support-structures" (along with "compute-nodes" and
> 
>  > "attester-supported-algos"), which does not really seem very commensurate
> 
>  > with the prose summary here.
> 
> Changed the description of Container 'rats-support-structures' to:
> 
> This houses the set of information relating to remote attestation for a 
> device.  This includes specific device TPM(s), the compute nodes (such 
> as line cards) on which the TPM(s) reside, and the algorithms supported 
> across the platform.
> 
>  >    +--rw tpms
> 
>  >       +--rw tpm* [name]
> 
>  >       [...]
> 
>  >          +--rw firmware-version    identityref
> 
>  >          +--rw tpm12-hash-algo?    identityref
> 
>  >          +--rw tpm12-pcrs*         pcr
> 
>  >          +--rw tpm20-pcr-bank* [tpm20-hash-algo]
> 
>  >
> 
>  > It's fairly surprising that the firmware-version is writable, and I'm not
> 
>  > really sure what it means to have the set of pcrs be writable, either.
> 
> This is an artifact of YANG.  If you make the information read only, 
> then these nodes are not available to be used as constraints within the 
> configuration datastore.  There were several discussions on this last 
> year including:
> 
> https://mailarchive.ietf.org/arch/msg/rats/JgNtYx0LPWlL8E0yEXkK9ZW_0CU/ 
> <https://mailarchive.ietf.org/arch/msg/rats/JgNtYx0LPWlL8E0yEXkK9ZW_0CU/>
> 
> And during the YANG Doctor review:
> 
> https://mailarchive.ietf.org/arch/msg/rats/xzcjDbMIObdyCbUyWqsWCODJ34c/ 
> <https://mailarchive.ietf.org/arch/msg/rats/xzcjDbMIObdyCbUyWqsWCODJ34c/>
> 
> The alternative is to simply not do the  consistency check, which would 
> drive errors.
> 
>  > Section 2.1.1.6
> 
>  >
> 
>  >      typedef pcr {
> 
>  >        type uint8 {
> 
>  >          range "0..31";
> 
>  >        }
> 
>  >        description
> 
>  >          "Valid index number for a PCR.  At this point 0-31 is viable.";
> 
>  >
> 
>  > That's a fairly surprising description to see in a Proposed Standard.
> 
>  > If we are not sure that the range restriction will always be valid, 
> should
> 
>  > we be enforcing it as part of the module?
> 
> Custom TPM implementations can have additional PCRs.  We did not want to 
> have the model restrict such flexibility.
> 
>  >      typedef compute-node-ref {
> 
>  >        type leafref {
> 
>  >          path "/tpm:rats-support-structures/tpm:compute-nodes"
> 
>  >             + "/tpm:compute-node/tpm:node-name";
> 
>  >        }
> 
>  >        description
> 
>  >          "This type is used to reference a hardware node.  It is quite
> 
>  >           possible this leafref will eventually point to another YANG
> 
>  >           module's node.";
> 
>  >
> 
>  > Similarly here, I might expect the statement to be a caution to the 
> reader
> 
>  > that they need to handle the case where it points to another YANG 
> module's
> 
>  > node, rather than "it is quite possible" that such an eventuality might
> 
>  > occur.
> 
> Changed the text to:
> 
>        "This type is used to reference a hardware node.  Note that an
> 
>         implementer might include an alternative leafref pointing to a
> 
>         different YANG module node specifying hardware structures.";
> 
>  >      grouping tpm20-hash-algo {
> 
>  >      [...]
> 
>  >          default "taa:TPM_ALG_SHA256";
> 
>  >          description
> 
>  >            "The hash scheme that is used to hash a TPM1.2 PCR. This
> 
>  >             must be one of those supported by a platform.";
> 
>  >
> 
>  > Is this description with "TPM1.2" really correct in the "tpm20-hash-algo"
> 
>  > grouping?
> 
> Fixed.  (It looks like a previous fix here got dropped somehow.)
> 
>  >          default "taa:TPM_ALG_SHA1";
> 
>  >          description
> 
>  >            "The hash scheme that is used to hash a TPM1.2 PCR. This
> 
>  >             MUST be one of those supported by a platform.  This assumes
> 
>  >             that an algorithm other than SHA1 can be supported on some
> 
>  >             TPM1.2 cryptoprocessor variant.";
> 
>  >
> 
>  > What exactly assumes that?  Use of a non-default value, perhaps?
> 
> SHA1 is the hash default for TPM1.2.  But at a global level, SHA1 is 
> deprecated for signatures.
> 
> https://csrc.nist.gov/News/2006/NIST-Comments-on-Cryptanalytic-Attacks-on-SHA-1 
> <https://csrc.nist.gov/News/2006/NIST-Comments-on-Cryptanalytic-Attacks-on-SHA-1> 
> 
> 
> It seems wise to require alternative algorithms so that users do not get 
> confused.
> 
>  >      grouping nonce {
> 
>  >        description
> 
>  >          "A random number intended to be used once to show freshness
> 
>  >           and to allow the detection of replay attacks.";
> 
>  >
> 
>  > "Show freshness" makes sense and seems intuitive.  Detecting replay
> 
>  > attacks seems like it would require some additional infrastructure (a
> 
>  > "replay cache", perhaps, ideally with some time-based aging out), about
> 
>  > which we say nothing.  Is this replay protection expected to be part of
> 
>  > the TPM chip itself, or on the composite device hosting the YANG server,
> 
>  > or somewhere else entirely?  It seems a bit disingenuous to suggest that
> 
>  > such functionality is possible while providing no pointers at all for how
> 
>  > to achieve it.
> 
> There have been a number of discussions on this
> 
> https://mailarchive.ietf.org/arch/msg/rats/KvUkqzBuisozqKfKoAzbRtbvV_k/ 
> <https://mailarchive.ietf.org/arch/msg/rats/KvUkqzBuisozqKfKoAzbRtbvV_k/>
> 
> We didn't want to tie the nonce down to a specific size, as the proper size
> 
> is a function of how many quotes per second can be generated from a physical
> 
> TPM1.2, TPM2, or perhaps more speedy custom hardware that support the TPM
> 
> API.  To help clarify, the description of the nonce in the YANG model now
> 
> defines specific predictable behavior for padding/trimming as the nonce is
> 
> sent into a specific TPM type:
> 
>  >      grouping tpm12-pcr-selection {
> 
>  >        description
> 
>  >          "A Verifier can request one or more PCR values using its
> 
>  >           individually created Attestation Key Certificate (AC).
> 
>  >           The corresponding selection filter is represented in this
> 
>  >           grouping.
> 
>  >           Requesting a PCR value that is not in scope of the AC used,
> 
>  >           detailed exposure via error msg should be avoided.";
> 
>  >
> 
>  > This last sentence seems to be missing a few words that are vital to the
> 
>  > meaning.
> 
> Deleted the last sentence.  It was trying (and failing) to describe a 
> fairly obvious error case which is not core the grouping definition.
> 
>  >      grouping tpm12-pcr-selection {
> 
>  >        [...]
> 
>  >        leaf-list pcr-index {
> 
>  >          [...]
> 
>  >          description
> 
>  >            "The numbers/indexes of the PCRs. At the moment this is 
> limited
> 
>  >             to 32.   In addition, any selection of PCRs MUST verify that
> 
>  >
> 
>  > (A similar comment as above regarding "at the moment" and future-proofing
> 
>  > what goes into a Proposed Standard.)
> 
> 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.
> 
>  >      grouping tpm20-pcr-selection {
> 
>  >        [...]
> 
>  >        list tpm20-pcr-selection {
> 
>  >          unique "tpm20-hash-algo";
> 
>  >
> 
>  > I confess to being only a YANG amateur, but am not really sure why the
> 
>  > "unique" qualifier is preferable to the "key" one in this case (and
> 
>  > several others later on, but I'll just mention this one).
> 
> https://datatracker.ietf.org/doc/html/rfc7950#section-7.8.3 
> <https://datatracker.ietf.org/doc/html/rfc7950#section-7.8.3>
> 
>  >      grouping tpm-name {
> 
>  >        description
> 
>  >          "A unique TPM on a device.";
> 
>  >        leaf name {
> 
>  >          type string;
> 
>  >          description
> 
>  >            "Unique system generated name for a TPM on a device.";
> 
>  >
> 
>  > If it's system-generated would that imply config false?
> 
> Same issue as above.  Making it config=false means it cannot be used for 
> constraining configuration data.  Many discussions on this are in the 
> archives.  More reading is in RFC8342, Network Management Datastore 
> Architecture (NMDA).
> 
>  >        uses node-uptime;
> 
>  >        list unsigned-pcr-values {
> 
>  >          description
> 
>  >            [...]
> 
>  >             significant processing.  There should never be a result where
> 
>  >             an unsigned PCR value is actually that that within a quote.
> 
>  >             If there is a difference, a signed result which has been
> 
>  >             verified from retrieved logs is considered definitive.";
> 
>  >
> 
>  > I'm not sure I understand this part -- is there a missing negation in the
> 
>  > first sentence?
> 
> Tweaked the definition to:
> 
>           significant processing.  Note that there should never be a
> 
>           result where an unsigned PCR value differs from what may be
> 
>           reconstructed from the within the PCR quote and the event logs.
> 
>           If there is a difference, a signed result which has been
> 
>           verified from retrieved logs is considered definitive.";
> 
>  >      grouping boot-event-log {
> 
>  >        description
> 
>  >          "Defines a specific instance of an event log entry
> 
>  >           and corresponding to the information used to
> 
>  >           extend the PCR";
> 
>  >        leaf event-number {
> 
>  >          type uint32;
> 
>  >          description
> 
>  >            "Unique event number of this event";
> 
>  >
> 
>  > Should we mention what scope the uniqueness is within?  (At only 32 bits
> 
>  > it cannot be globally unique.)
> 
> Text refined to:
> 
>          "Identifies a unique event which extended a PCR.
> 
>          Uniqueness is since last boot.";
> 
> With 32 bits, there are billions of events recordable.  Tracking more 
> than this since boot is unlikely.
> 
>  >        leaf pcr-index {
> 
>  >          type pcr;
> 
>  >          description
> 
>  >            "Defines the PCR index that this event extended";
> 
>  >        }
> 
>  >
> 
>  > Why is this not mandatory?
> 
> Making it mandatory would restrict informational events which could also 
> be placed into the log. Eliminating this possibility could be 
> unnecessarily restrictive as PCR reconstruction would only occur on 
> entries with a pcr-index.
> 
>  >        leaf-list event-data {
> 
>  >          type uint8;
> 
>  >          description
> 
>  >            "The event data size determined by event-size";
> 
>  >
> 
>  > Is there some missing punctuation or something here?  Otherwise the focus
> 
>  > on size seems strange, given that there is a separate "leaf event-size".
> 
>  > (Also, why is a leaf-list of uint8 better than "type binary"?)
> 
> The object is driven by the TCG:
> 
> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf 
> <https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf> 
> 
> 
> Section 5.1 provides the object definitions.   In multiple notes later 
> in section 5, this particular object has the note:
> 
> "this is a memory copy of EventSize bytes".  All the examples I see fill 
> this with "0x00, 0x00, 0x00, 0x00"
> 
> So mostly this object data will be ignored as it will not be used for 
> PCR validation.  But as it arrive as part of the TCG EFI log format, we 
> include it.
> 
>  >      grouping ima-event {
> 
>  >        description
> 
>  >          "Defines an hash log extend event for IMA measurements";
> 
>  >        reference
> 
>  >          "ima-log:
> 
>  > https://www.trustedcomputinggroup.org/wp-content/uploads/ 
> <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.
> 
> 5.1.6 also works.  Changed to that.
> 
>  >        leaf filename-hint {
> 
>  >          type string;
> 
>  >          description
> 
>  >            "File that was measured";
> 
>  >
> 
>  > Is this just the file name, a full path, either, ...?
> 
> As each vendor might do this their own way, we didn't want to specify how.
> 
>  >        leaf signature {
> 
>  >          type binary;
> 
>  >          description
> 
>  >            "The file signature";
> 
>  >
> 
>  > Both this description and the reference for ima-event seem pretty sparse
> 
>  > on what the signature actually covers.
> 
> Now says "Digital file signature which provides a fingerprint for the 
> file being measured."
> 
>  >      rpc tpm20-challenge-response-attestation {
> 
>  >        if-feature "taa:tpm20";
> 
>  >        description
> 
>  >          "This RPC accepts the input for TSS TPM 2.0 commands of the
> 
>  >           managed device. ComponentIndex from the hardware manager YANG
> 
>  >           module to refer to dedicated TPM in composite devices,
> 
>  >           e.g. smart NICs, is still a TODO.";
> 
>  >
> 
>  > I would prefer to not see "is still a TODO" in a Proposed Standard.
> 
>  > "Out of the scope of this specification" is seen much more often (though
> 
>  > in some sense it conveys much the same meaning...)
> 
> Made "is not is covered".
> 
>  >        container compute-nodes {
> 
>  >          [...]
> 
>  >          list compute-node {
> 
>  >            key "node-id";
> 
>  >
> 
>  > If we're going to use "leaf node-name" for leafrefs, does that require a
> 
>  > uniqueness constraint on "node-name" here?
> 
> Yes.  Good catch.   Added unique "node-name";
> 
>  >        container tpms {
> 
>  >          [...]
> 
>  >            leaf path {
> 
>  >              type string;
> 
>  >              config false;
> 
>  >              description
> 
>  >                "Path to a unique TPM on a device.  This can change across
> 
>  >                 reboots.";
> 
>  >
> 
>  > Is this a ... device-tree path?  Surely it is not a filesystem path,
> 
>  > right?
> 
> Yes a device tree path.  Made it "Device path".
> 
>  >            leaf firmware-version {
> 
>  >              type identityref {
> 
>  >                base taa:cryptoprocessor;
> 
>  >              }
> 
>  >              mandatory true;
> 
>  >              description
> 
>  >                "Identifies the cryptoprocessor API set supported.  This
> 
>  >                 is automatically configured by the device and should not
> 
>  >                 be changed.";
> 
>  >
> 
>  > (Per my previous comment on the tree diagram, "should not be changed"
> 
>  > strongly implies that it should be "config false" to me.  When might that
> 
>  > not be the case?)
> 
> See earlier discussions.
> 
>  >        container attester-supported-algos {
> 
>  >          description
> 
>  >            "Identifies which TPM algorithms are available for use on an
> 
>  >             attesting platform.";
> 
>  >          [...]
> 
>  >
> 
>  > Just to confirm I'm reading this properly: the lists herein are not 
> scoped
> 
>  > to specific TPM instances, but are rather global to the entire composite
> 
>  > device; the "when" constraints just say that each leaf-list is only
> 
>  > present if there are any TPMs of the corresponding TPM version 
> present but
> 
>  > do not otherwise limit the scope of applicability for the algorithms in
> 
>  > question?  That seems rather surprising, especially if one considers a
> 
>  > scenario where there are multiple line cards on a given device and one
> 
>  > could be hot-replaced to a newer model with a TPM that supports more
> 
>  > algorithms.  But perhaps I'm missing something.
> 
> This interpretation is correct.  In surveying the vendors of the 
> devices, nobody wanted to mix and match algorithms for a platform.  Only 
> when the full platform supported it, would they be advertised.  Doing so 
> simplifies the management interfaces.
> 
>  > Section 2.1.2.2
> 
>  >
> 
>  >    3.  Specific algorithm types: Each algorithm type defines what
> 
>  >        cryptographic functions may be supported, and on which type of
> 
>  >        API specification.  It is not required that an implementation of
> 
>  >        a specific TPM will support all algorithm types.  The contents of
> 
>  >        each specific algorithm mirrors what is in Table 3 of
> 
>  >        [TCG-Algos].
> 
>  >
> 
>  > This phrasing implies a strong sense of consistency between the two
> 
>  > documents.  I did not attempt to perform a rigorous verification of each
> 
>  > element between the two documents, but I hope someone has done so.
> 
> Henk did a scrub to make sure I copied everything verbatim.
> 
>  > Section 2.1.2.3
> 
>  >
> 
>  >      contact
> 
>  >        "WG Web:   <https://datatracker.ietf.org/wg/rats/ 
> <https://datatracker.ietf.org/wg/rats/>>
> 
>  >         WG List:  <mailto:[e-mail] <mailto:[e-mail]>>
> 
>  >         Author:   Eric Voit <mailto:[e-mail] 
> <mailto:[e-mail]>>";
> 
>  >
> 
>  > Just to confirm: it's correct for Eric to be the only listed author for
> 
>  > this module (vs ietf-tpm-remote-attestation, that includes what 
> appears to
> 
>  > be the whole author list of the I-D)?
> 
> I don't have a problem with it ;-).   Note: while *lots* of people 
> contributed initial objects and structures for their companies and did 
> reviews, I did the vast majority of the synthesis.  This included a 
> couple full rewrites after earlier authors departed.
> 
> That said, I really don’t really care if other authors are listed.  
> Anyone who wants just needs to speak up and say so.  Or add their name 
> to a GitHub pull before it is too late.
> 
>  >      identity signing {
> 
>  >        description
> 
>  >          "A TCG recognized signing algorithm";
> 
>  >        reference
> 
>  >          "TCG-Algos:TCG Algorithm Registry Rev1.32  Table 2";
> 
>  >
> 
>  > I understand a desire to remain consistent with what the TCG says, but
> 
>  > would we consider mentioning that this also includes symmetric crypto
> 
>  > "signing" operations?
> 
> To minimize any chance of perceived divergence, prefer to say exactly 
> what TCG says in the referenced document.
> 
>  >      identity method {
> 
>  >        description
> 
>  >          "A TCG recognized method such as a mask generation function.";
> 
>  >
> 
>  > Perhaps we want "cryptographic method" here, in light of the way the TCG
> 
>  > spec frames things.
> 
> By using their text, reduces the chance for perceived divergence.
> 
>  >      identity TPM_ALG_XOR {
> 
>  >        if-feature "tpm12 or tpm20";
> 
>  >        base tpm12;
> 
>  >        base tpm20;
> 
>  >        base hash;
> 
>  >        base symmetric;
> 
>  >
> 
>  > I didn't see in the TCG Algorithm Registry Rev1.32 doc that indicated 
> that
> 
>  > the "XOR encryption algorithm" derived from a hash.  Please confirm that
> 
>  > "base hash" is correct.
> 
> H in the TCG table means "hash algorithm that compresses input data to a 
> digest value or indicates a method that uses a hash".  And "H" appears 
> in the Table next to TPM_ALG_XOR.
> 
>  >      identity TPM_ALG_SYMCIPHER {
> 
>  >        if-feature "tpm20";
> 
>  >        base tpm20;
> 
>  >        description
> 
>  >          "Object type for a symmetric block cipher";
> 
>  >        reference
> 
>  >          "TCG-Algos:TCG Algorithm Registry Rev1.32  Table 3 and
> 
>  >           TCG TPM 2.0 library specification. ALG_ID: 0x0025";
> 
>  >
> 
>  > Is the lack of "base symmetric" correct?  (The algorithm registry doc
> 
>  > seems to list is as an "object" as well as symmetric.)
> 
> Looks like this got dropped somewhere.  Thanks for the catch!
> 
>  > Also, I guess this is a tolerable place to ask why the "AES" alg is
> 
>  > specific to TPM 1.2.  I found mention of (e.g.) "AES128" in at least one
> 
>  > TPM 2.0 spec, so presumably I'm just missing how those algorithms are
> 
>  > represented in the TPM 2.0 model (and why symmetric ciphers are treated
> 
>  > differently than the different output lengths of the SHA2 and SHA3 family
> 
>  > of hash functions, which do get individual algorithm identities in this
> 
>  > YAND module).  Perhaps they use this TPM_ALG_SYMCIPHER!
> 
> AES is supported as a symmetric algorithm on TPM2.   But symmetric keys 
> are not use with specific PCR signatures or hashing extensions driven 
> from outside a router.  So putting a "Base" statement would make the 
> algorithm appear available when in fact it is not used in this context.
> 
>  > Section 4
> 
>  >
> 
>  > The YANG security considerations template has a section about readable
> 
>  > data nodes that are considered sensitive.  Does that apply to any of the
> 
>  > nodes in the modules defined by this document?
> 
> The colon after the following sentence is supposed to indicate these 
> specific vulnerabilities.
> 
>  >    Container '/rats-support-structures/attester-supported-algos':  'tpm1
> 
>  >       2-asymmetric-signing', 'tpm12-hash', 'tpm20-asymmetric-signing',
> 
>  >       and 'tpm20-hash'.  All could be populated with algorithms that are
> 
>  >       not supported by the underlying physical TPM installed by the
> 
>  >       equipment vendor.
> 
>  >
> 
>  > We should say what the impact of that configuration would be at runtime.
> 
> Added: "A vendor should restrict the ability to configure unsupported 
> algorithms."  In this case there shouldn't be the ability to configure 
> these.
> 
>  >    Container: '/rats-support-structures/tpms':  'name': Although shown
> 
>  >       as 'rw', it is system generated.  Therefore it should not be
> 
>  >
> 
>  > Why is it shown as 'rw', then?
> 
> See note above.  YANG isn't always pretty.
> 
>  > Also, separately, if we're going to keep 'firmware-version' as writable,
> 
>  > there seems to be some consideration there, as telling a TPM 2.0-only 
> chip
> 
>  > to use TPM 1.2 is going to be a DoS.
> 
> It should not be possible, this is a YANG artifact.
> 
>  >       'certificates': It is possible to provision a certificate which
> 
>  >       does not correspond to an Attestation Identity Key (AIK) within
> 
>  >       the TPM 1.2, or an Attestation Key (AK) within the TPM 2.0
> 
>  >       respectively.
> 
>  >
> 
>  > Could we have a sentence about the runtime impact of such a
> 
>  > misconfiguration?
> 
> Added: "In such a case, calls to an RPC requesting this specific 
> certificate could result in either no response or a response for an 
> unexpected TPM."
> 
>  >    RPC 'tpm12-challenge-response-attestation':  It must be verified that
> 
>  >    [...]
> 
>  >    RPC 'tpm20-challenge-response-attestation':  It must be verified that
> 
>  >
> 
>  > Must be verified by whom?  In order to achieve what property?
> 
> Now says:
> 
> RPC 'tpm12-challenge-response-attestation'
> 
> The receiver of the RPC response must verify that the certificate is for 
> an active AIK, i.e., the certificate has been confirmed by a third party 
> as being able to support Attestation on the targeted TPM
> 
> RPC 'tpm20-challenge-response-attestation'
> 
> The receiver of the RPC response must verify that the certificate is for 
> an active AK, i.e., the private key confirmation of the quote signature 
> within the RPC response has been confirmed by a third party to belong to 
> an entity legitimately able to perform Attestation on the targeted TPM 2.0.
> 
>  > Section 6.1
> 
>  >
> 
>  >    [ISO-IEC-9797-2]
> 
>  >               "Message Authentication Codes (MACs) - ISO/IEC
> 
>  >               9797-2:2011", n.d.,
> 
>  >               <https://www.iso.org/standard/51618.html 
> <https://www.iso.org/standard/51618.html>>.
> 
>  >
> 
>  > Following the link indicates that there's a 2021 revision available; 
> do we
> 
>  > need the older version specifically?
> 
> As the TCG table is from before that standard was released, it seems 
> safer to leave the older reference (as getting/verifiying the new 
> version would require buying it.)
> 
>  > NITS
> 
>  >
> 
>  > Section 2.1.1.1
> 
>  >
> 
>  >    *  'TPMs': Indicates that multiple TPMs on the device can support
> 
>  >       remote attestation.  This feature is applicable in cases where
> 
>  >       multiple line cards are present, each with its own TPM.
> 
>  >
> 
>  > An "e.g." or "for example" seems appropriate here.
> 
> I don't know of another example, so the current text looks good.
> 
>  > Section 2.1.1.6
> 
>  >
> 
>  >          description
> 
>  >            "Name of one or more unique TPMs on a device.  If this object
> 
>  >             exists, a selection should pull only the objects related to
> 
>  >             these TPM(s).  If it does not exist, all qualifying TPMs that
> 
>  >             are 'hardware-based' equals true on the device are 
> selected.";
> 
>  >
> 
>  > I think s/are/have/ would make sense.
> 
> Updated.
> 
>  >      grouping tpm20-attestation {
> 
>  >        description
> 
>  >          "Contains an instance of TPM2 style signed cryptoprocessor
> 
>  >           measurements.  It is supplemented by unsigned Attester
> 
>  >           information.";
> 
>  >        leaf TPMS_QUOTE_INFO {
> 
>  >          type binary;
> 
>  >          mandatory true;
> 
>  >          description
> 
>  >            "A hash of the latest PCR values (and the hash algorithm used)
> 
>  >             which have been returned from a Verifier for the selected 
> PCRs
> 
>  >             and Hash Algorithms.";
> 
>  >          reference
> 
>  >            "TPM2.0-Structures:
> 
>  > https://www.trustedcomputinggroup.org/wp-content/uploads/ 
> <https://www.trustedcomputinggroup.org/wp-content/uploads/>
> 
>  >             TPM-Rev-2.0-Part-2-Structures-01.38.pdf  Section 10.12.1";
> 
>  >
> 
>  > The reference appeears to include two sections marked 10.12.1(!), though
> 
>  > clearly we do not want the one entitled "introduction".
> 
> :-) Hopefully the reader can go down a couple lines.
> 
>  >        leaf filedata-hash {
> 
>  >          type binary;
> 
>  >          description
> 
>  >            "Hash of filedata";
> 
>  >
> 
>  > It is perhaps banal, but mentioning that the interpretation is controlled
> 
>  > by the filedata-hash-algorithm might be useful.
> 
> Now says:  "Hash of filedata as updated based upon the 
> filedata-hash-algorithm."
> 
>  >      rpc tpm12-challenge-response-attestation {
> 
>  >        [...]
> 
>  >        input {
> 
>  >          container tpm12-attestation-challenge {
> 
>  >            [...]
> 
>  >            leaf-list certificate-name {
> 
>  >              if-feature "tpm:tpms";
> 
>  >              type certificate-name-ref;
> 
>  >              must "/tpm:rats-support-structures/tpm:tpms"
> 
>  >                 + "/tpm:tpm[tpm:firmware-version='taa:tpm12']"
> 
>  >                 + "/tpm:certificates/"
> 
>  >                 + "/tpm:certificate[name=current()]" {
> 
>  >                error-message "Not an available TPM1.2 AIK certificate.";
> 
>  >
> 
>  > This is the first instance of "AIK" in the document (it does not 
> appear in
> 
>  > the prose until the security considerations).  Perhaps it should get
> 
>  > mentioned in the Introduction as another term inported from TPM2.0-key?
> 
> Good idea.  Added.
> 
>  >      rpc tpm20-challenge-response-attestation {
> 
>  >        if-feature "taa:tpm20";
> 
>  >        description
> 
>  >          "This RPC accepts the input for TSS TPM 2.0 commands of the
> 
>  >           managed device. ComponentIndex from the hardware manager YANG
> 
>  >           module to refer to dedicated TPM in composite devices,
> 
>  >           e.g. smart NICs, is still a TODO.";
> 
>  >
> 
>  > The second sentence seems to be missing a verb.
> 
> Now says: "is used to refer"
> 
>  >        output {
> 
>  >          list tpm20-attestation-response {
> 
>  >            unique "certificate-name";
> 
>  >            description
> 
>  >              "The binary output of TPM2b_Quote in one TPM chip of the
> 
>  >               node which identified by node-id. [...]
> 
>  >
> 
>  > Does "chip" imply hardware chip, which is perhaps more specific than the
> 
>  > introductory materials indicated to be the scope of applicability?
> 
> Removed chip.  Now says "from one TPM of the"
> 
>  >      container rats-support-structures {
> 
>  >        container compute-nodes {
> 
>  >          if-feature "tpm:tpms";
> 
>  >          description
> 
>  >            "Holds the set device subsystems/components in this composite
> 
>  >             device that support TPM operations.";
> 
>  >
> 
>  > "set of"
> 
> Updated
> 
>  >            leaf status {
> 
>  >              type enumeration {
> 
>  >                enum operational {
> 
>  >                  value 0;
> 
>  >                  description
> 
>  >                    "The TPM currently is currently running normally and
> 
>  >                     is ready to accept and process TPM quotes.";
> 
>  >                  reference
> 
>  >                    "TPM2.0-Arch:
> 
>  >                     TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> 
>  >                     Section 12";
> 
>  >
> 
>  > Huh, why is this not an https URI?
> 
> Updated
> 
>  >            container certificates {
> 
>  >              description
> 
>  >                "The TPM's certificates, including EK certificates
> 
>  >                 and AK certificates.";
> 
>  >
> 
>  > While we're expanded LAK and IAK previously, we haven't expanded just 
> "AK"
> 
>  > itself yet.
> 
> Expanded to Attestation Key.
> 
>  > Section 2.1.2.3
> 
>  >
> 
>  >      organization
> 
>  >        "IETF RATS Working Group";
> 
>  >
> 
>  > Do we want to use the same spelling as in the ietf-tpm-remote-attestation
> 
>  > module?
> 
> Made:
> 
> "IETF RATS (Remote ATtestation procedureS) Working Group";
> 
>  >      identity tpm12 {
> 
>  >        if-feature "tpm12";
> 
>  >        base cryptoprocessor;
> 
>  >        description
> 
>  >          "Supportable by a TPM1.2.";
> 
>  >        reference
> 
>  >          "TPM1.2-Structures:
> 
>  > https://trustedcomputinggroup.org/wp-content/uploads/ 
> <https://trustedcomputinggroup.org/wp-content/uploads/>
> 
>  >           TPM-Main-Part-2-TPM-Structures_v1.2_rev116_01032011.pdf
> 
>  >           TPM_ALGORITHM_ID values, page 18";
> 
>  >
> 
>  > We referenced it as section 4.8 (vs page 18) for the YANG "feature"
> 
>  > stanza.
> 
> Updated
> 
>  >      identity tpm20 {
> 
>  >        if-feature "tpm20";
> 
>  >        base cryptoprocessor;
> 
>  >        description
> 
>  >          "Supportable by a TPM2.";
> 
>  >        reference
> 
>  >          "TPM2.0-Structures:
> 
>  > https://trustedcomputinggroup.org/wp-content/uploads/ 
> <https://trustedcomputinggroup.org/wp-content/uploads/>
> 
>  >           TPM-Rev-2.0-Part-2-Structures-01.38.pdf
> 
>  >           The TCG Algorithm Registry. Table 9";
> 
>  >
> 
>  > Is table 9 the right reference?
> 
> Removed "The TCG Algorithm Registry. Table 9"
> 
>  >      identity TPM_ALG_SM4 {
> 
>  >        if-feature "tpm20";
> 
>  >        base tpm20;
> 
>  >        base symmetric;
> 
>  >        description
> 
>  >          "SM4 symmetric block cipher";
> 
>  >        reference
> 
>  >          "TCG-Algos:TCG Algorithm Registry Rev1.32  Table 3.
> 
>  >          ALG_ID: 0x0013";
> 
>  >
> 
>  > Isn't SM4 an ISO standard now as well?
> 
> Wiki says no,
> 
> https://en.wikipedia.org/wiki/SM4_(cipher) 
> <https://en.wikipedia.org/wiki/SM4_(cipher)>
> 
> but a web search says it happened in the middle of last year.
> 
> https://www.iso.org/standard/81564.html 
> <https://www.iso.org/standard/81564.html>
> 
> Leaving current text as is, since I don't want to buy the document.
> 
>  >      identity TPM_ALG_RSASSA {
> 
>  >        if-feature "tpm20";
> 
>  >        base tpm20;
> 
>  >        base asymmetric;
> 
>  >        base signing;
> 
>  >        description
> 
>  >          "Signature algorithm defined in section 8.2 (RSASSAPKCS1-v1_5)";
> 
>  >        reference
> 
>  >          "TCG-Algos:TCG Algorithm Registry Rev1.32  Table 3 and
> 
>  >          RFC 8017.  ALG_ID: 0x0014";
> 
>  >      }
> 
>  >
> 
>  >      identity TPM_ALG_RSAES {
> 
>  >        if-feature "tpm20";
> 
>  >        base tpm20;
> 
>  >        base asymmetric;
> 
>  >        base encryption_mode;
> 
>  >        description
> 
>  >          "Signature algorithm defined in section 7.2 (RSAES-PKCS1-v1_5)";
> 
>  >
> 
>  > I think you need to say what document the section numbers are from, since
> 
>  > two references are listed in the reference stanza.
> 
> Done. Put RFC 8017 at the front of the description
> 
>  >      identity TPM_ALG_ECDH {
> 
>  >        if-feature "tpm20";
> 
>  >        base tpm20;
> 
>  >        base asymmetric;
> 
>  >        base method;
> 
>  >        description
> 
>  >          "Secret sharing using ECC";
> 
>  >        reference
> 
>  >          "TCG-Algos:TCG Algorithm Registry Rev1.32  Table 3 and
> 
>  >           NIST SP800-56A. ALG_ID: 0x0019";
> 
>  >
> 
>  > RFC 7748 is specific to the so-called "CFRG curves"; is it really the
> 
>  > right reference for generic ECDH?
> 
> This is what the TCG table says.  Nevertheless removed 7748.
> 
> Thanks again,
> 
> Eric
>