I've been asked to review this document for the IOTDIR.

TL&DR: Ready with issues.

In general I find the document well written and specified.

Big stuff to look at:

  * Section 10.1 could get a big hair cut (see below).
  * err-msg, err-code, and err-lang demonstrate a bit of a gap in the
    explanation of the theory of operation, both in this draft and in
    RFC 9397 that should be closed prior to publishing.

Caveat: I have *not* checked all of the CDDL in the Appendix.

Details:

Section 4.1.1:

>         The TAM
>         can decide, in any implementation-specific way, whether to
>         include a token in a message.  The initial token value generated
>         by a TAM MUST be produced using a cryptographically secure random
>         source.  Subsequent token values MUST be different for each
>         message the TAM creates.
>
Two problems with this paragraph:

 1. I think you have a general consistency problem with how you want
    tokens to be handled.  Here you say they are optional, but in
    Section 4.2 you say there is at least one case where they are not,
    and then later, you say that their use must be ignored after
    messages with valid signatures have been received.  My suggestion is
    that you add a subsection somewhere above to talk about tokens and
    their use.  This will simplify your text below.
 2. "cryptographically secure random source" requires either a reference
    or a definition.  There are plenty.  Pick one.

Then the next statement:

    2.  Create a COSE header containing the desired set of header
        parameters.

"desired set" seems rather loose.  Please be somewhat more specific, 
again for interoperability's sake.

I'll admit to being somewhat nervous about Table 4 in IANA 
considerations.  Again, if the goal is interoperability, permitting 
other than standards action (except perhaps 1 or 2 bits for 
experiments), seems like a recipe for interoperability failure, given 
these are the protocol primitives.

Section 6: Mapping of TEEP labels

> If future standards require additional messages beyond this
>     range, implementations SHOULD be designed to handle gracefully any
>     unrecognized labels, treating them as unknown optional parameters
>     without failing to process the message.
A great way to assure that gracefulness is to define the behavior (e.g., 
produce an error/ignore/???).

Three other issues: about this section:

  * It seems you are cutting it quite fine by wanting to squeeze in all
    of the parameters into a single uint.  Do you have enough
    operational experience to know that you REALLY have this right? BTW,
    a fine answer would be to simply rev this spec.
  * That said, maybe it makes sense to at least indicate what you would
    want of experiments.
  * I see that these labels are repeated in IANA considerations. Again a
    general preference for all IANA considerations would be to be a bit
    more specific, such as "This document, Section 6".

Section 7.1.1.1 raises some concerns:

> If the TAM does not have
>         permission to update such components (this can happen if
>         different TAMs manage different components in the device), the
>         TAM instead responds with an Update message containing an
>         appropriate err-msg, and err-code set to
>         ERR_ATTESTATION_REQUIRED.

Same with 7.1.2.

I was initially perplexed as to WHY the CDDL would have had err-msg et 
al in Section 4.4 until I read the above, and then I wondered why you 
don't have some sort of parameter flag to simply state in the first 
place that attestation is required.  Having revisited RFC 9397 and 
reread these sections a few times, and perhaps I'm still missing 
something, I think there is a hole in 9397 that you need to close, at 
least minimally, in this document: how are errors processed and by whom?

You don't explain WHY a TAM would generate err-msgs in Section 4.4, and 
it is not made clear to the reader what is supposed to happen with those 
error messages in Section 10.1.  I am also struggling a bit with the 
security concept of providing an error back to the TA if the TA has 
failed in some way its authentication.

Section 10.1:

This section is really quite general.  You're writing a lot of words 
that operators have to do for any number of mechanisms unrelated to 
TEEP.  Moreover, I am unsure that any operator would ever read this 
text, since the draft is primarily written for implementors.  That said:

> Implementations SHOULD support certificate status checks
>        and have clear behavior when certificates are expired or revoked
>        (see err-code behaviors such as ERR_CERTIFICATE_EXPIRED and
>        ERR_BAD_CERTIFICATE).

I agree this is important, but I'm not sure you are doing that SHOULD 
justice.  It would be helpful to have at least an example of what you 
mean.  I write this because operators may be put into quite difficult 
positions with ERR_CERTIFICATE_EXPIRED (there could probably be an 
entire RFC on that), but the general choice is- let the device continue 
to function and notify the operator, or take the device out of service 
and notify the operator. Obviously a TAM may not be the correct element 
to perform these tasks, but it may communicate with something that is.

Also, you might want to mention warnings that might take place 
*prior* to certificate expiry.

On this point:

>     *  Time synchronization: Accurate device time is important for
>        certificate validity checks and for some attestation freshness
>        mechanisms.  Operators SHOULD ensure devices maintain adequate
>        time synchronization.
Of course this is true for certificates, but I don't think you are going 
far enough in two ways:

 1. How accurate is "accurate" and how adequate is "adequate"?
 2. It feels a little like you've buried the lede: this is a strong
    dependency and somehow we don't get to it until Section 10.

Next:

>     *  Transport and deployment-specific concerns: TEEP is transport
>        agnostic; operators MUST ensure the chosen transport provides
>        adequate confidentiality, integrity, authentication, and replay
>        protection.  See the HTTP binding draft
>        [I-D.ietf-teep-otrp-over-http] for transport-specific operational
>        details when that binding is used.

I'm not sure you get to MUSTify operators.  If it's THAT important, then 
MUSTify the implementations in your spec.


Nits:

The diagrams in Section 3 should be labeled.

	The complete CDDL structure is
    shown in Appendix C.

I prefer normative text to not be in appendices, because that matches 
behaviors of other SDOs, but that is me.

In Section 7.2.1:

>     If the QueryRequest is instead determined to contain Evidence, the
>     TEEP Agent passes the Evidence (via some mechanism out of scope of
>     this document) to an attestation Verifier

and

>     The TEEP Agent MAY also use (in any implementation specific way) any
>     SUIT Reports in the QueryRequest in determining whether it trusts the
>     TAM.

Remove the parentheses as they are unneeded.  The test here is to read 
the text aloud to see if the sentence makes sense without parentheses.  
If it does, just remove them.  If it doesn't, then you should rewrite 
the sentence so that it does.

Eliot