Subject: [Rats] Robert Wilton's Discuss on draft-ietf-rats-yang-tpm-charra-16: (with DISCUSS and COMMENT)

From: Robert Wilton via Datatracker <[email-masked]>
Date: Thu, 10 Mar 2022 05:42:30 -0800

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

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

Hi,

Thanks for the document, and apologies for the late discuss ballot.  I have a
few discuss level concerns that I believe should be discussed, and if necessary
addressed before publication:

(1) I would like to understand why some leaves (e.g., TPMS_QUOTE_INFO) are in
all caps rather than following the YANG convention of being lowercase.  Please
see section 4.3.1 of RFC 8407.

(2) I think that the "must" statements for tmp20-hash-algo and tpm12-hash-algo
may not achieving what you want for two reasons:
 (i) They never actually compare against the leaf value that is being set.
 (ii) They require that
 tpm:rats-support-structures/tpm:attester-supported-algos/tpm:tpm12-hash is
 always populated in configuration, whereas the descriptive text is suggesting
 that the supported-algos is optional to provide and you should only be
 checking against it if is has been configured. (iii) It was unclear to me if
 attester-supported-algos is only ever populated via configuration, or if no
 configuration has been provided then the platform will provide default
 operational values?  It is worth nothing that different trees of data (e.g.,
 configuration vs operational) are available depending on whether the must
 statement is being evaluated in the context of a config operation (uses the
 config tree) or an RPC (which uses the state tree).

(3) To fix the YANG error mentioned in Warren's discuss, keystore-ref needs an
if-feature "ks:asymmetric-keys".  I.e., because the leafref points to something
that is conditional on an if-feature, the referring node must also be
conditional on the same if-feature.

(4) I really dislike "last-entry-value", type binary, to act as an input
parameter to specifying the starting point to return the logs from. Since
structured YANG is defined/returned for these log entries, then I think that
defining what this data is in binary is likely to be underspecified.  I would
encourage you to just drop this as an option, given that every log message has
a unique id and that can just as easily be provided, and probably more
efficiently used.

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

Reformatting the yang file (e.g., using pyang) would have slightly helped
reviewing and readability (but the RFC editor will do this).  However, I'm not
sure whether all of the tree diagrams are completely up to date and accurate,
so it might be worth regenerating them after any markups to the YANG files are
have been done to ensure that they are right.

  "This type is used to reference a hardware node.  It is quite
   possible this leafref will eventually point to another YANG
   module's node.";

Ben has already comment on this, but I presume that this description is now
wrong?

  grouping tpm20-hash-algo {
    description
      "The cryptographic algorithm used to hash the TPM2 PCRs.  This
       must be from the list of platform supported options.";
    leaf tpm20-hash-algo {
      type identityref {
        base taa:hash;
      }
      must '/tpm:rats-support-structures/tpm:attester-supported-algos'
         + '/tpm:tpm20-hash' {

As per the discuss, I think that this "must" needs to look something more like:
    (. == <path>) or !<path>)
    where <path> is
    /tpm:rats-support-structures/tpm:attester-supported-algos/tpm:tpm20-hash

I'm not a total expert on XPath, so it might be worth asking on NETMOD or YANG
doctors to confirm that these expressions are correct, and it may depend on
whether or not you expect default operational values to be provided by the
system if it hasn't been configured.

         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.

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

Unclear what is meant by "any in Quotes".  Also "fewer binary" => "fewer binary
digits".

            "Answers the question: is this TPM is a hardware based
             TPM?";

I think that this description could be worded more directly.

grouping ima-event-log {
    description
      "Measurement log created by IMA.";
    list ima-event-entry {
      key event-number;
      description
      "Ordered list of ima event logs by event-number";
       uses ima-event;
    }
  }

Formatting the file would help.

    leaf event-number {
      type uint32;
      description
        "Unique event number of this event";
    }
Is the event-number expected to be monotonically increasing, if so, should that
be stated.  It is allowed to wrap, and if so what is the expected behaviour.  I
also note that event-number is only uint32 for the BIOS logs, compared to
uint64 the other logs, is that intentional?

  leaf-list event-data {
      type uint8;
      description
        "The event data size determined by event-size";
    }
  }

I think that Ben also raised this, but I wasn't sure why you are not just using
a String, or binary here.  If it is binary, then what encoding is being used?

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

I think that Ben has already recommended a better name for this feature (which
I agree with).

       +---x log-retrieval
          +---w input
          |  +---w log-selector* []
          |  |  +---w name*                      string
          |  |  +---w (index-type)?
          |  |  |  +--:(last-entry)
          |  |  |  |  +---w last-entry-value?    binary
          |  |  |  +--:(index)
          |  |  |  |  +---w last-index-number?   uint64
          |  |  |  +--:(timestamp)
          |  |  |     +---w timestamp?           yang:date-and-time
          |  |  +---w log-entry-quantity?        uint16
          |  +---w log-type        identityref

I think that structure would be better if log-type was first, before the
log-selector, but I actually think that it would be cleaner to model this as
three separate RPCs for the three log types, and you can put each RPC under a
different if-feature statement, which means that the choice statement for the
returned data disappears.

I also think that the there should be more text about how multiple entries in
the log-selector, each of which can specify multiple names play together. 
I.e., I presume that for a request, a given name should only turn up once?

Regards,
Rob

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Thu, 10 Mar 2022 18:56:32 +0000

Hi Rob,

Thanks for the comments.

> From Robert Wilton, March 10, 2022 8:43 AM
> 
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/

The purpose of the capital letters is to unequivocally show that the fields
are exactly the same fields as described in TCG TPM standards documents.
This reuse enables easy search for on-line resources such as:
https://dox.ipxe.org/structTPMS__QUOTE__INFO.html

There was commentary around why these specific objects are capitalized
during the previous discussions with YANG doctors.  And at the time they
seemed an acceptable reason for the RFC 8407 divergence.

>  (i) They never actually compare against the leaf value that is being set.

Looking through interactions in 2021 with various YANG doctors, there was
*numerous* requests asking for help/validation with XPATH constraints.
Based on a dearth of suggestions, at times we considered jettisoning the
constraints because of our uncertainty on their efficacy.  But we didn't
jettison since there is good value in such constraints, assuming they are
well-formed.  Could you introduce us to someone who can one-on-one review
these interactively in a phone call?  If you or an XPATH expert has specific
ways to address this, we would *gladly* adopt these improved structures.

>  checking against it if is has been configured. 

Supported-algos is not optional.  I don't see the text which suggests
optional.  Maybe your suggestion is that there should be pre-configured
values for this configuration table?  If not this, can you point to where
this appears as optional?

> (iii) It was unclear to me if
>  operational values?  

This is a reasonable approach if there are no 'attester-supported-algos'
configured.  Would text indication this cover your concern?

> It is worth nothing that different trees of data (e.g.,

Per above, any suggestions on constraints which you believe could be
improved would be welcomed.

> conditional on the same if-feature.

Added.

> a unique id and that can just as easily be provided, and probably more

Makes sense. Just created a GitHub pull request to have it dropped.

Yes.  There was a fix proposed to Ben as follows:

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

My understanding of the XPATH statement is that the constraint just has to
be in the list.  But then again my brain always flushes what I learn about
XPATH as soon as it isn't needed.  Can you facilitate an intro to someone
who can give a definitive answer?

> doctors to confirm that these expressions are correct

We have solicited in the past.  Is it required to ask again on an open
alias?  Per above, my preference is that you could facilitate us meeting
someone whom we could work with in a short meeting to confirm / change the
XPATH.

> and it may depend on

Based on a comment from Francesca, the text now reads:

"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."
 
Also added 'digits' per your request.

> I think that this description could be worded more directly.

Now says:

            "System generated indication of whether this is a
             hardware based TPM.";

> be stated.  It is allowed to wrap,

Now says:

        "Unique event number of this event which monotonically 
         increases.  The maximum event number should not be
         reached, nor is wrapping back to an earlier number
         supported.";

> and if so what is the expected behaviour.  I

Yes, we are matching to other standards venues' formats.  We didn't define
these logs.
 
> a String, or binary here.  If it is binary, then what encoding is being
used?

My response to Ben was:

The object is driven by the TCG:
https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specificat
ion-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.

I.e., this specification is not ours so we can't change it.  We are just
trying to follow what has been put together in another standards venue.
 
> I think that Ben has already recommended a better name for this feature
(which

Renaming the feature to 'multi-tpm' is not hard to do.  But as I pointed out
to Ben, there is a tradeoff in making several trees less readable due to
line wrapping.   If this is required by Ben & you, the change can be made.
Personally I think the shorter one is preferrable.
 
> log-selector

Change made.

> but I actually think that it would be cleaner to model this as

This could be done, and I agree it would be cleaner.  But as the log
structure has been this way v00, and previous YANG Dr comments never
highlighted this, not changing the RPCs at this point is preferred as the
current design works.
 
> I.e., I presume that for a request, a given name should only turn up once?

A leaf-list must have a unique set of 'name' objects.  So that should be ok.
Overall the definition is improved to:

"Only log entries which meet all the selection criteria provided are to be
returned by the RPC output.";
 
Thanks,
Eric

> Regards,
> Rob
> 
> 
> 
> _______________________________________________
> RATS mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rats

---

From: "Rob Wilton (rwilton)" <[email-masked]>
Date: Fri, 18 Mar 2022 11:58:34 +0000

Hi Eric,

Thanks for the quick reply.

I’ve cc’ed Jan because he has kindly agreed to check the XPath expressions.

Please see further comments inline ...

> -----Original Message-----

> From: Eric Voit (evoit) <[email-masked]<mailto:[email-masked]>>

> Sent: 10 March 2022 18:57

> To: Rob Wilton (rwilton) <[email-masked]<mailto:[email-masked]>>; The IESG <[email-masked]<mailto:[email-masked]>>

> Cc: Nancy Cam-Winget (ncamwing) <[email-masked]<mailto:[email-masked]>>; [email-masked]<mailto:[email-masked]>;

> [email-masked]<mailto:[email-masked]>; [email-masked]<mailto:[email-masked]>; draft-ietf-rats-yang-tpm-

> [email-masked]<mailto:[email-masked]>

> Subject: RE: [Rats] Robert Wilton's Discuss on draft-ietf-rats-yang-tpm-

> charra-16: (with DISCUSS and COMMENT)

> > for more information about how to handle DISCUSS and COMMENT

> positions.

> > Thanks for the document, and apologies for the late discuss ballot.  I

> have a

> > few discuss level concerns that I believe should be discussed, and if

> necessary

> > (1) I would like to understand why some leaves (e.g., TPMS_QUOTE_INFO)

> are

> in

> > all caps rather than following the YANG convention of being lowercase.

> Please

> seemed an acceptable reason for the RFC 8407 divergence.

Okay, thanks for the explanation.  I will leave this to discretion of the authors but given that all caps only appears in a couple of places, then my personal opinion would be to make these conform to RFC 8407, and then put the corresponding name in the reference or description.

> > (2) I think that the "must" statements for tmp20-hash-algo and

> tpm12-hash-algo

> well-formed.  Could you introduce us to someone who can one-on-one

> review

> ways to address this, we would *gladly* adopt these improved structures.

I'm trying to find someone to help you.

> >  tpm:rats-support-structures/tpm:attester-supported-algos/tpm:tpm12-

> hash

> is

> >  always populated in configuration, whereas the descriptive text is

> suggesting

> this appears as optional?

Ah, it was the "allows" in the following paragraph that made me think that this was optional, i.e., it could be used if desired, but was not required.  Perhaps tweak the wording?

   container 'attester-supported-algos' - Identifies which TCG hash

   algorithms are available for use on the Attesting platform.  This

   allows an operator to limit algorithms available for use by RPCs to

   just a desired set from the universe of all allowed hash algorithms

   by the TCG.

> >  attester-supported-algos is only ever populated via configuration, or if

> no

> configured.  Would text indication this cover your concern?

Actually, if you are willing for it to always be explicit configured then that might server you better, or at least if you work on that assumption then it will do you no harm.

The key issue is that XPath expressions when validating configuration are only allowed to reference the configuration data tree and not any operational data.  E.g., a config must statement cannot be checking against a value in the operational data tree.

Now, there is an open question as to whether "system configuration" is allowed to be merged in with the running configuration and then be validated or not (https://datatracker.ietf.org/doc/draft-ma-netmod-with-system/).  Depending on which direction that discussion goes that you could regard the server is providing system default values which would be used in place of explicit configuration.  But this discussion will take time to work out, and in the mean time, requiring that it is always configured is simpler, and would seem to work regardless of the outcome of that discussion.

> >  statement is being evaluated in the context of a config operation (uses

> the

> > (3) To fix the YANG error mentioned in Warren's discuss, keystore-ref

> needs an

> > if-feature "ks:asymmetric-keys".  I.e., because the leafref points to

> something

> Added.

Thanks.

> > structured YANG is defined/returned for these log entries, then I think

> that

> > defining what this data is in binary is likely to be underspecified.  I

> would

> > encourage you to just drop this as an option, given that every log message

> has

> Makes sense. Just created a GitHub pull request to have it dropped.

Thanks.

> > reviewing and readability (but the RFC editor will do this).  However, I'm

> not

> > sure whether all of the tree diagrams are completely up to date and

> accurate,

> > so it might be worth regenerating them after any markups to the YANG files

> are

> > Ben has already comment on this, but I presume that this description is

> now

> > As per the discuss, I think that this "must" needs to look something more

> like:

> /tpm:rats-support-structures/tpm:attester-supported-algos/tpm:tpm20-

> hash

> be in the list.

  But then again my brain always flushes what I learn about

> XPATH as soon as it isn't needed.

Er, yes, same 😉

Based on the discussion about system configuration, then actually, I think that this XPath should be:

      must '. = /tpm:rats-support-structures/tpm:attester-supported-algos'

          + '/tpm:tpm20-hash' {

  Can you facilitate an intro to someone

> who can give a definitive answer?

I've asked Jan Lindblad (cc'ed) and he is checking them.

> > I'm not a total expert on XPath, so it might be worth asking on NETMOD or

> > YANG

> someone whom we could work with in a short meeting to confirm / change

> the

> > Unclear what is meant by "any in Quotes".  Also "fewer binary" => "fewer

> binary

> Also added 'digits' per your request.

Okay.

>              hardware based TPM.";

Okay.

> > Is the event-number expected to be monotonically increasing, if so, should

> that

>          supported.";

Okay.

> these logs.

Okay.

> > I think that Ben also raised this, but I wasn't sure why you are not just

> using

> > a String, or binary here.  If it is binary, then what encoding is being

> https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-

> Specificat

> it.

I think that just having event-data as a single leaf of type binary, without the corresponding length field would be a better representation.  I.e., it seems like a reasonable YANG equivalent to the TCG definition, but can leave this to the authors discretion.

> >    attestation. This feature is applicable in cases where multiple line

> cards

> > I think that Ben has already recommended a better name for this feature

> line wrapping.

Just to check, are you aware that pyang have a line wrapping option that may mean that the tree still formats okay without wrapping.

E.g., something like "pyang -f tree --tree-line-length=69 (or perhaps 72) <input_file.yang>"

E.g., for a random file that I have to hand, and limiting it down to 55 chars would convert from

   +-- module* [name]

    |  +-- name                 yang:yang-identifier

    |  +-- revision?            rev:revision-date-or-label

    |  +-- replaces-revision*   rev:revision-date-or-label

    |  +-- namespace?           inet:uri

    |  +-- location*            inet:uri

    |  +-- submodule* [name]

    |     +-- name?       yang:yang-identifier

    |     +-- revision    yang:revision-identifier

    |     +-- location*   inet:uri

to:

    +-- module* [name]

    |  +-- name                 yang:yang-identifier

    |  +-- revision?

    |  |       rev:revision-date-or-label

    |  +-- replaces-revision*

    |  |       rev:revision-date-or-label

    |  +-- namespace?           inet:uri

    |  +-- location*            inet:uri

    |  +-- submodule* [name]

    |     +-- name?       yang:yang-identifier

    |     +-- revision    yang:revision-identifier

    |     +-- location*   inet:uri

   If this is required by Ben & you, the change can be made.

> Personally I think the shorter one is preferrable.

I think that it is confusing because the top level container is tpms and you also have a feature called tpms that effectively has a slightly different meaning.  So, I still believe that a more specific feature name would be beneficial.

> Change made.

Thanks.

> > three separate RPCs for the three log types, and you can put each RPC

> under a

> > different if-feature statement, which means that the choice statement for

> the

> current design works.

Okay.

> > I also think that the there should be more text about how multiple entries

> in

> returned by the RPC output.";

My issue was that I think that this could make a request that contains:

Input

  Log-selector = [

   {

      name = ["foo", "bar"],

      last-entry = 10

   },

   {

      name = "foo",

      last-entry = 20

   }

And it wasn't clear to me how the server is expected to interpret that.

Thanks,

Rob

> > [email-masked]<mailto:[email-masked]>

---

From: "Jan Lindblad (jlindbla)" <[email-masked]>
Date: Fri, 18 Mar 2022 14:01:52 +0000

Rob, Eric,

I notice this mail has a very wide distribution. Since I have not been involved in the thread up to this point, I dare not reduce the distribution, even though I suspect some receivers may not be interested in details of XPath usage in YANG. You have been warned :)

Rob asked me to look at the XPath expressions in module ietf-tpm-remote-attestation.yang.

0) YANG 1.0

My first observation is that the module is written in YANG 1.0 (it does not have the yang-version 1.1; statement). The module ietf-tcg-algs is already YANG 1.1, and it's illegal to import YANG 1.1 modules from YANG 1.0. There are many other good reasons for using 1.1 here too.

First I looked at the 8 when-expressions. They seem to be doing what the authors intend, but I have one strong suggestion and one validation question none the less.

1) Equality test with identity

Equality tests with identities is not very well defined, often causing non-interoperability among implementations. Direct equality test also imposes unnecessary limits on the forward evolution of this module. I'd suggest you replace all equality tests in the when statements with the XPath function derived-from-or-self(). Like this:

uses tpm12-hash-algo {
when "firmware-version = 'taa:tpm12'";
refine "tpm12-hash-algo" {

becomes

uses tpm12-hash-algo {
when "derived-from-or-self(firmware-version, 'taa:tpm12')";
refine "tpm12-hash-algo" {

2) Plenty of hardcoding TPM firmware versions 1.2 and 2.0

Are firmware versions 1.2 and 2.0 the only two versions that will ever be relevant here? If not, you would need to issue new versions of this module to cope with new versions. With some careful redesign, I suspect you could get to a point where you have a stable YANG module that doesn't change even in the presence of new versions. All you'd have to do in that case would be to add a module with some identities and possibly some augments. The current design also leads to a fair amount of duplication.

Then I had a look at the 3 leafrefs.

3) Non-key leafref

This is a reference to a non-key leaf. While this is allowed and works, many implementations will work slower like this. If there is a good reason to do it like this, fine, but I don't see that here, so it feels quite odd. If you have decided about a key for a list, why wouldn't you then use that key to refer to elements in it? Maybe change the name to be the key?

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. Note that an
implementer might include an alternative leafref pointing to a
different YANG module node specifying hardware structures.";
}

4) Single leafref to multi-key instance

Here we have a leafref pointing to a particular certificate name. The problem is that it does not relate to the tpm name, which is another key in the path. This means this is ambiguous. Let me explain.

typedef certificate-name-ref {
type leafref {
path "/tpm:rats-support-structures/tpm:tpms/tpm:tpm"
+ "/tpm:certificates/tpm:certificate/tpm:name";
}
description
"A type which allows identification of a TPM based certificate.";
}

Here's an example of a leaf using this type:

leaf certificate-name {
type certificate-name-ref;
mandatory true;
description
"Identifies a certificate in a keystore.";
}

Let's say this leaf takes the vale "foo". If there are several tpm:s with certificates foo, we have no way of knowing which one it refers to. E.g.

/tpm:rats-support-structures/tpm:tpms/tpm:tpm[tpm:name = '123']/tpm:certificates/tpm:certificate[tpm:name = 'foo']/tpm:name
/tpm:rats-support-structures/tpm:tpms/tpm:tpm[tpm:name = '456']/tpm:certificates/tpm:certificate[tpm:name = 'foo']/tpm:name

In general, you need one leafref for each key in a path in order to construct a proper reference. On top of that, you need to bind the leafrefs together using the deref() function, or (more verbosely) using predicates in the path.

In case you're interested in more details, a longer explanation of this issue can be found in the "Common YANG Mistakes" section, pp 451-454, of the Network Programmability with YANG book. I am certainly happy to explain further as well.

Then I looked at the 6 must statements.

5, 6, 7) Must and When mixed up (line 174 and similar constructs on lines 192 and 938):

leaf tpm20-hash-algo {
type identityref {
base taa:hash;
}
must '/tpm:rats-support-structures/tpm:attester-supported-algos'
+ '/tpm:tpm20-hash' {
error-message "This platform does not support tpm20-hash-algo";
}

While this construct should "work", i.e. generate an error message if a request to use an unsupported algo, it's an odd way to use a must expression. I think this should be turned into a when expression, essentially removing this leaf from the schema if it's irrelevant.

8) Suspect logic in must (line 262):

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

This first part of this expression verifies that there is one tpm with a name leaf equal to all the pcr-index values. The second part, after the and, verifies that there is a tpm20-pcr-bank with a pcr-index value equal to all the pcr-index values. Such pcr-index values are not necessarily in the tpms with with the current pcr-index value, however. As long as all pcr-index values are found in any tpm, the second part will be satisfied. This looks suspect to me.

9) Equality test towards identity in path (line 634 and a similar construct on line 681):

Just as in 1) above, this is not very interoperable. I would suggest the same remedy here as there.

leaf-list certificate-name {
if-feature "tpm:mtpm";
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.";
}

Happy to elaborate or check suggested fixes.

Best Regards,
/jan

On 18 Mar 2022, at 12:58, Rob Wilton (rwilton) <[email-masked]<mailto:[email-masked]>> wrote:

Hi Eric,

Thanks.

Thanks.

Er, yes, same 😉

Okay.

Okay.

Okay.

Okay.

   +-- module* [name]

to:

    +-- module* [name]

Thanks.

Okay.

Input
  Log-selector = [
   {
      name = ["foo", "bar"],
      last-entry = 10
   },
   {
      name = "foo",
      last-entry = 20
   }

Thanks,
Rob

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Fri, 18 Mar 2022 19:00:06 +0000

Hi Jan,

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

> From: Jan Lindblad, March 18, 2022 10:02 AM
>
> Rob, Eric,
>
> I notice this mail has a very wide distribution. Since I have not been 
> involved in
> the thread up to this point, I dare not reduce the distribution, even though 
> I
> suspect some receivers may not be interested in details of XPath usage in 
> YANG.
> You have been warned :)
>
> My first observation is that the module is written in YANG 1.0 (it does not 
> have
> the yang-version 1.1; statement). The module ietf-tcg-algs is already YANG 
> 1.1,
> and it's illegal to import YANG 1.1 modules from YANG 1.0. There are many
> other good reasons for using 1.1 here too.

Made v1.1

> First I looked at the 8 when-expressions. They seem to be doing what the
> authors intend, but I have one strong suggestion and one validation question
> none the less.
>
> Equality tests with identities is not very well defined, often causing non-
> interoperability among implementations. Direct equality test also imposes
> unnecessary limits on the forward evolution of this module. I'd suggest you
> replace all equality tests in the when statements with the XPath function
> derived-from-or-self(). Like this:
>
>         uses tpm12-hash-algo {

Happy to help with forward-longing compatibility.  I made the three equality 
test changes requested.

> Are firmware versions 1.2 and 2.0 the only two versions that will ever be
> relevant here? If not, you would need to issue new versions of this module 
> to
> cope with new versions. With some careful redesign, I suspect you could get 
> to
> a point where you have a stable YANG module that doesn't change even in the
> presence of new versions. All you'd have to do in that case would be to add 
> a
> module with some identities and possibly some augments. The current design
> also leads to a fair amount of duplication.

Yes, these are the only two versions of interest.  New versions are not 
expected.

> This is a reference to a non-key leaf. While this is allowed and works, many
> implementations will work slower like this. If there is a good reason to do 
> it like
> this, fine, but I don't see that here, so it feels quite odd. If you have 
> decided
> about a key for a list, why wouldn't you then use that key to refer to 
> elements in
> it? Maybe change the name to be the key?
>

The functionality supported is the same, with better performance.  So change 
incorporated.

> Here we have a leafref pointing to a particular certificate name. The 
> problem is
> that it does not relate to the tpm name, which is another key in the path. 
> This
> means this is ambiguous. Let me explain.
>
> Let's say this leaf takes the vale "foo". If there are several tpm:s with 
> certificates
> foo, we have no way of knowing which one it refers to. E.g.
>
> /tpm:rats-support-structures/tpm:tpms/tpm:tpm[tpm:name =
> '123']/tpm:certificates/tpm:certificate[tpm:name = 'foo']/tpm:name
> /tpm:rats-support-structures/tpm:tpms/tpm:tpm[tpm:name =
> '456']/tpm:certificates/tpm:certificate[tpm:name = 'foo']/tpm:name
>
> In general, you need one leafref for each key in a path in order to 
> construct a
> proper reference. On top of that, you need to bind the leafrefs together 
> using
> the deref() function, or (more verbosely) using predicates in the path.
>
> In case you're interested in more details, a longer explanation of this 
> issue can
> be found in the "Common YANG Mistakes" section, pp 451-454, of the Network
> Programmability with YANG book. I am certainly happy to explain further as
> well.

A certificate cannot repeat across different TPMs, so the design is 
intentional.   There is text within the draft about the container says:

      Of note are the certificates
      which are associated with that TPM.  As a certificate is
      associated with a particular TPM attestation key, knowledge of the
      certificate allows a specific TPM to be identified.

I.e., is a design constraint that the remote device *not* the identifiers of 
specific TPM when making a request.  I.e., we don't want to force the device 
doing the query from needing to know which specific TPM which hosts a 
certificate when the original query is made.

> 5, 6, 7) Must and When mixed up (line 174 and similar constructs on lines 
> 192
> and 938):
>
>     leaf tpm20-hash-algo {
>       type identityref {
>         base taa:hash;
>       }
> While this construct should "work", i.e. generate an error message if a 
> request
> to use an unsupported algo, it's an odd way to use a must expression. I 
> think this
> should be turned into a when expression, essentially removing this leaf from 
> the
> schema if it's irrelevant.

I did attempt a when before.  And I just tried a 'when' statement again:

          leaf tpm20-hash-algo {
            when "derived-from(/tpm:rats-support-structures"
               + "/tpm:attester-supported-algos/tpm:tpm20-hash";
            type identityref {
              base taa:hash;
            }

Pyang gives me the error "the statement 'when' cannot be given for a key".

As the current statement works, can we just keep it?

> This first part of this expression verifies that there is one tpm with a 
> name leaf
> equal to all the pcr-index values. The second part, after the and, verifies 
> that
> there is a tpm20-pcr-bank with a pcr-index value equal to all the pcr-index
> values. Such pcr-index values are not necessarily in the tpms with with the
> current pcr-index value, however. As long as all pcr-index values are found 
> in any
> tpm, the second part will be satisfied. This looks suspect to me.

I think you are suggesting the following...

       leaf-list pcr-index {
         type pcr;
         must '/tpm:rats-support-structures/tpm:tpms'
            + '/tpm:tpm[name = current()] '
            + '/tpm:tpm20-pcr-bank[pcr-index = current()]' {
           error-message "Acquiring this PCR index is not supported";
         }

I made the change.  Let me know if you prefer something different.

> 9) Equality test towards identity in path (line 634 and a similar construct 
> on line
> 681):
>
> Just as in 1) above, this is not very interoperable. I would suggest the 
> same
> remedy here as there.
>

derived-from-or-self requires two arguments.  And the certificate-name is not 
an identity.  If it was an identity I would try something like:

          must "derived-from-or-self(current(), "
             + "/tpm:rats-support-structures"
             + "/tpm:tpms/tpm:tpm[tpm:firmware-version='taa:tpm20']"
             + "/tpm:certificates/"
             + "/tpm:certificate[name=current()])" {

But there is not.  So is there an issue with just keeping what is currently 
there?

          must "/tpm:rats-support-structures/tpm:tpms"
             + "/tpm:tpm[tpm:firmware-version='taa:tpm20']"
             + "/tpm:certificates/"
             + "/tpm:certificate[name=current()]" {

As most people don't care about XPATH minutia, lets come to conclusion 
off-thread (with Rob there too).   We can then reply-all with the final 
disposition.

Eric

---

From: "Eric Voit (evoit)" <[email-masked]>
Date: Fri, 18 Mar 2022 20:03:13 +0000

Hi Rob,

More thoughts in-line...

From: Rob Wilton, March 18, 2022 7:59 AM

Hi Eric,

> From: Eric Voit (evoit) <[email-masked] <mailto:[email-masked]> >

> To: Rob Wilton (rwilton) <[email-masked] <mailto:[email-masked]> >; The IESG <[email-masked] <mailto:[email-masked]> >

> Cc: Nancy Cam-Winget (ncamwing) <[email-masked] <mailto:[email-masked]> >; [email-masked] <mailto:[email-masked]> ;

> [email-masked] <mailto:[email-masked]> ; [email-masked] <mailto:[email-masked]> ; draft-ietf-rats-yang-tpm-

> [email-masked] <mailto:[email-masked]> 

<eric> We have had this discussion a few times, and the preference is to keep the capitals.

<eric> Thanks, Jan has made some good suggestions.

   by the TCG.

<eric> Changed "This allows an operator to limit" to: "An operator will use this information to limit"

<eric> This discussion is exactly what we were thinking about.  And this is a long term discussion we would rather not wait upon to close this document.   

As forcing an explicit configuration does take away implementation flexibility, I would prefer that it remain undefined for now.  This would allow vendors to make their own choices.    If you require this to be explicitly configured, we will accede.  But it is not preferred.

<snipped away closed items>

<snipped away closed items>

Er, yes, same 😉

          + '/tpm:tpm20-hash' {

<eric> I made the change you requested above.  

      must '. = /tpm:rats-support-structures'

         + '/tpm:attester-supported-algos/tpm:tpm20-hash' {

      }

and

      must '. = /tpm:rats-support-structures'

         + '/tpm:attester-supported-algos/tpm:tpm12-hash' {

        error-message "This platform does not support tpm12-hash-algo";

      }

The pyang and yanglint approve.

<snipped away closed items>

That is exactly what Ben asked as well.  It is now shown this way in the recently posted v17.

<eric> Yes I am aware, and use this.

   +-- module* [name]

    |  +-- location*            inet:uri

    |  +-- submodule* [name]

    |     +-- location*   inet:uri

to:

    +-- module* [name]

    |  +-- revision?

    |  +-- replaces-revision*

    |  +-- location*            inet:uri

    |  +-- submodule* [name]

    |     +-- location*   inet:uri

<eric> the feature name has been changed to mtpm in the posted v17.

<snipped away closed items>

Input

  Log-selector = [

   {

      name = ["foo", "bar"],

      last-entry = 10

   },

   {

      name = "foo",

      last-entry = 20

   }

<eric>  The current text for leaf-list name says:

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

To the end of that statement I just added a sentence to the Pull request:  

"When this selection criteria is provided, it will be considered as a logical AND with any other selection criteria provided."

Thanks,

Eric

Thanks,

Rob

> > [email-masked] <mailto:[email-masked]>