COSE (CBOR Object Signing and Encryption) Receipts
draft-ietf-cose-merkle-tree-proofs-17

Thanks to Charlie Kaufman for their secdir review (his nits still exist in the draft, btw).

Section 4.1, 8.2.2:  I don't love the fact that the registry called 'COSE Verifiable Data Structures' is actually an algorithm registry - super confusing.  Moreover, the description in the registration template in Section 8.2.2 says nothing about algorithms.  In section 4.1, there is a sentence that calls it 'a registry of verifiable data structure algorithms'.  Can we change the name of the registry to that?  

Section 7:  While I think it is probably too early for this, it might be wise to have a post quantum section here warning of an eventual shift in algorithms.  Depending on how long the proofs and receipts are expected to be valid will determine how soon an algorithm migration should be considered.  I would be happy to help write it if that is helpful.  Note:  I would expect that this includes both the hash (currently only SHA-2 256 is registered) and the signatures.

Authors:  A nit...  Will Orie change his contact details before publication?  Seems like it might be better in the long run.

I have no transport-related observations.

Please consider the following comments when revising this document:
---
Introduction:
   COSE Receipts can include proves that a
   document is in a database (proof of inclusion)
- Is this correct: /proves/ ? is this /proofs/?
- or ... /can prove that a.../
---
/increase burden for implementers/
- insert /the/ before /burden/?
---
/which corresponds to the log/
- could be /that corresponds to the log/
---
/It is recommended
   that implementations return a single boolean result for Receipt verification operations.../
- Please check, whether this ought to be a normative RECOMMENDED?
---
and does /merkle tree/ have a capital 'M' or 'M' and 'T'?
---

In Section 2, I don't understand the "introduces a new Section..." language. This reads as if updating an existing RFC to add new sections to it, but the links are to sections in this document. Those sections establish new registries; should this say "a new registry (see Section ...) that contains..."?

The notation used in Section 4.1 ("EC2 keys (1: 2)") probably warrants a bit more explanation. By parallel with the "(TBD_1 : 1)" below, I presume that it should be read "When COSE header parameter 1 is set to the value 2..."?

Section 5.2.1 appears to contain guidance to future VDS definitions, but Section 5 is intended to be a definition of one specific VDS as well as an exemplar of how to do such a definition. That guidance might be better placed elsewhere and let Section 5 focus on this specific VDS. Note also that the recommended behavior (making payloads detatched) is then carried out a few paragraphs later along with a duplicative explanation of why that needs to be done.

As Ralph Merkle (inventor of the Merkle tree) is a person, "Merkle" should be capitalized throughout.

===NITS FOLLOW===

Section 1, "proves" => "proofs"?
Section 4.2, "merkle tree based" => "Merkle-tree-based"
Section 4.3, "this Receipts" should probably be "these Receipts" or simply "Receipts"
Section 4.3, "verifiable data structure specific" => "verifiable-data-structure-specific"
Section 4.3.1, "an IANA registration must be made for each individually supported algorithm" => "a separate IANA registration must be made for each supported algorithm"

Hi Orie, Henk, Antoine, and Cédric, 

Thanks for the effort put into this specification. 

Please find some comments below:

# CDDL is normative

Please move to RFC8610 to be listed a normative.

# Normative language

Overall, the use of the normative language seems inconsistent, especially for the implementation behavior. Examples that triggered my comments are: 

   It is recommended
   that implementations return a single boolean result for Receipt
   verification operations, to reduce the chance of accepting a valid
   signature over an invalid inclusion proof.

Or

   It is recommended to select signature algorithms that share
   cryptographic components with the verifiable data structure used, 

And similar ones.

Also, some clarity about object construction/verification behaviors vs. those related to actual use would be helpful.

# Implementers or users?

CURRENT:
   Implementers should not expect interoperability across
   "verifiable data structures", but they should expect conceptually
   similar properties across the different registered proof types.  For
   example, 2 different merkle tree based verifiable data structures
   might both support proofs of inclusion.

The first sentence in particular smells more relevant for actual use rather than design phase/implementers? Maybe I’m missing the point this text tries to make.

Also, not sure what are the concrete implications of the interoperability mentioned here.

# Why this isn’t a MUST?

CURRENT (4.2):
   Security analysis SHOULD be
   conducted prior to migrating to new structures to ensure the new
   security and privacy assumptions are acceptable for the use case.

# Check

CURRENT (4.3):
   When the receipts header parameter is present, the associated
   verifiable data structure and verifiable data structure proofs MUST
   match entries present in the registries established in this
   specification.

I guess you don’t imply the check should be against the initial values established by this doc, but also future registered values are covered. I suggest to make that more clear and consider this change:

NEW:
   When the receipts header parameter is present, the associated
   verifiable data structure and verifiable data structure proofs MUST
   match entries present in the registries [IANA_URL].

BTW, which entity is required to do the analysis? 

Idem for which entity (user, implementer, verifier, etc.) is targeted by:

   A privacy analysis MUST be
   performed for all mandatory fields in profiles based on this
   specification.

And 

   A security analysis MUST be performed to ensure that the digital
   signature algorithm alg has the appropriate strength to secure
   receipts.

# Each specification?

CURRENT:
   Each specification MUST define how to encode the verifiable data
   ^^^^^^^^^^^^^^^^^^
   structure identifier and its proof types in CBOR.  Each specification
                                                      ^^^^^^^^^^^^^^^^^^
   MUST define how to produce and consume the supported proof types.
   See Section 5 as an example.

Not sure to understand what is referred to by “Each specification” here.

# Payloads

Section 5.2.1 says:

   Specifications are
   encouraged to make payloads detached when possible, forcing
                                       ^^^^^^^^^^^^^^
   validation-time comparison.  

and then

   The payload SHOULD be detached.  

What are the implications if this is not the case?

# IANA Registries & Hidden Required IANA  Actions

CURRENT:
   IANA established the COSE Verifiable Data Structures and COSE
   Verifiable Data Structure Proofs registries under a Specification
   Required policy as described in [RFC8126].

I failed to find these. Can we please have pointers to where those are defined?

Also, Section 4.1 says: 

   This document establishes a registry of verifiable data structure
   algorithms, with the following initial contents:

Idem, Section 4.2 states: 
   This document establishes a registry of verifiable data structure
   algorithms, with the following initial contents:

which seems a hidden IANA actions. Not sure, why these contents are not part of the IANA section, though.

# Nits

## Title: Please expand COSE in the title.

## Abstract should be self-contained

s/RFC 9162/“Certificate Transparency Version 2.0” (RFC 9162)

## Section 2

* s/ore more COSE Receipts/or more COSE Receipts

* s/a new Section 8.2.2/ a new registry (Section 8.2.2)

* s/a new Section 8.2.3/ a new registry (Section 8.2.3)

## Section 4.1

OLD:
   +================+=======+===========================+===========+
   | Name           | Value | Description               | Reference |
   +================+=======+===========================+===========+
   | Reserved       | 0     | Reserved                  | Reserved  |

NEW:
   +================+=======+===========================+===========+
   | Name           | Value | Description               | Reference |
   +================+=======+===========================+===========+
   | Reserved       | 0     | Reserved                  |THIS_DOCUMENT |

## Section 4.2

Please fix the following

CURRENT:
  as EC2 keys (1: 2) keys  

## Section 4.3

* s/This document registered/ This document registers

* Which receipt/receipts are we referring to?

CURRENT:
   to enable this Receipts to be conveyed in the protected and
             ^^^^ ^      ^
   unprotected headers of COSE Objects.

Also, check the singular/plural use.

* 

OLD: The specific structure of COSE Receipts are dependent

NEW: The specific structure of COSE Receipts is dependent 

## Section 5.2

(1) CURRENT:

   Note that [RFC9162] defines that verification MUST fail if leaf-index
   is >= tree-size, and inclusion proofs are defined only for leaf
   nodes.  

May format this as a quote to insist this is not a NEW behavior.

(2) s/verifiers applies/verifier applies  

Cheers,
Med

Thank you to Linda Dunbar for the GENART review.

** Section 2.  Editorial.
     Correspondingly, this
      document introduces a new Section 8.2.2 that registers the
      integers used to identify verifiable data structures.

What does it mean to introduce a “new Section 8.2.2” per the write-up of TBD_1?  Maybe:

NEW
Correspondingly, this document introduces a new registry (see Section 8.2.2) that registers the identifiers used to identify verifiable data structures.
Same recommendation for TBD_2.

** Section 4.2.  Table 2.  Shouldn’t the values provided in the reference column also include an RFC in addition to the section numbers?

** Section 4.2
   Security analysis SHOULD be
   conducted prior to migrating to new structures to ensure the new
   security and privacy assumptions are acceptable for the use case

-- The situation where one is migrating between new structures isn’t sufficiently described

-- When would security analysis NOT be appropriate (as a SHOULD is used)?  Perhaps it might be better to use a lower case “should”.

** Section 5.1
5.1.  Verifiable Data Structure

   The integer identifier for this Verifiable Data Structure is 1.  The
   string identifier for this Verifiable Data Structure is
   "RFC9162_SHA256".  See Table 1.  See [RFC9162], 2.1.1.  Definition of
   the Merkle Tree, for a complete description of this verifiable data
   structure.

Should the text explicitly say (what is obvious from string identifier), that RFC9162_SHA256 is a Merkle Tree where SHA256 is used as the hash algorithm?

** Section 5.2.
   The term leaf-index is used for alignment with the use established in
   [RFC9162]
   Note that [RFC9162] defines that verification MUST fail if leaf-index
   is >= tree-size, and inclusion proofs are defined only for leaf
   nodes.  The identifying index of a leaf node is relative to all nodes
   in the tree size for which the proof was obtained.

Perhaps be clear that the term is “leaf_index” in Section 2.1.3.1 of RFC9162.

** Section 7
   See the security considerations section of:
   *  [RFC9162]
   *  [RFC9053]

-- Is this text saying that the security considerations of these two RFCs apply?

-- The Security Considerations of RFC9162 seems to have significant guidance related to CT.  Is there only a particular part of RFC9162’s Security Considerations which applies?

** Section 7.1
   A security analysis MUST be performed to ensure that the digital
   signature algorithm alg has the appropriate strength to secure
   receipts.

Is “MUST” the appropriate guidance here since an interoperable way to do “security analysis” on the appropriate strength isn’t clear?

# Éric Vyncke, INT AD, comments for draft-ietf-cose-merkle-tree-proofs-14
CC @evyncke

Thank you for the work put into this document. While the introduction is easy to read, the rest of the document is less easy though.

Please find below some non-blocking COMMENT points/nits (replies would be appreciated even if only for my own education).

Special thanks to Ivaylo Petrov for the shepherd's write-up including the WG consensus *and* the justification of the intended status.

I hope that this review helps to improve the document,

Regards,

-éric


## COMMENTS (non-blocking)

### Section 4.1

For a non-expert reader, ` EC2 keys (1: 2)` is basically meaningless especially `(1: 2)`.

### Section 4.2

`Security analysis SHOULD be conducted` why not a "MUST" and it a "SHOULD", then explain why and when the "SHOULD" can be bypassed.

### Section 4.3

Should it be "TBD_0" in `This document registered a new COSE Header Parameter receipts (394)` ? Also in Figure 1.

Is Figure 2 just an example or normative ? If example (as I guess), then clearly label it as 'example' in the caption and in the leading text.

### Section 5.2.1

Please replace 395 & co with TBC_* (not repeating this further).

### Section 6

Sometimes it is better to state the obvious and state that the privacy considerations of the 2 RFCs also apply to this document.

### Section 9

It is obviously up to the authors, but should there be a section about "Contributors" rather than writing `for their contributions (some of which substantial)` ?

## NITS (non-blocking / cosmetic)

### Capitalization of Merkle

There is at least one `merkle tree`, i.e., missing the capitalisation...