Subject: Re: [COSE] Benjamin Kaduk's Yes on draft-ietf-cose-x509-07: (with COMMENT)

From: Ivaylo Petrov <[email-masked]>
Date: Sun, 13 Dec 2020 23:10:43 +0100

Hello Ben,

Thank you for your review and apologies for the delay. Please find below my
answers.

The latest version of the draft is available here
https://cose-wg.github.io/X509/draft-ietf-cose-x509.html. There are some
technical issues that currently prevent me from uploading it to
datatracker, but that should be resolved very soon.
Best regards,
Ivaylo

On Wed, Oct 21, 2020 at 7:58 PM Benjamin Kaduk via Datatracker <
[email-masked]> wrote:

> Benjamin Kaduk has entered the following ballot position for
> draft-ietf-cose-x509-07: Yes
>
> 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/iesg/statement/discuss-criteria.html
> for more information about IESG DISCUSS and COMMENT positions.
>
>
> The document, along with other ballot positions, can be found here:
> https://datatracker.ietf.org/doc/draft-ietf-cose-x509/
>
>
>
> ----------------------------------------------------------------------
> COMMENT:
> ----------------------------------------------------------------------
>
> It's still hard to believe we've lost Jim.  Thank you for taking up the
> mantle to finish this work.
>

[IP]: Indeed, such a great loss... I hope I will be able to do justice to
this work.

As Roman noted already, the secdir review had some good points (and
> perhaps also some that do not require changes to the text), which
> deserves a response.
>
> [The missing examples in the linked github repo have already been noted a
> few times so I have nothing further to say about it.]
>
> It's interesting that JOSE does not have an 'x5bag' parameter, though I
> don't think that there is a need for this document to do anything in that
> regard.
>
> Abstract
>
>    The CBOR Signing And Encrypted Message (COSE) structure uses
>    references to keys in general.  For some algorithms, additional
>    properties are defined which carry parts of keys as needed.  The COSE
>
> (nit) I'm not sure that "parts of keys" covers all cases; the boundary
> between a "key" and various attributes or parameters associated with it
> can be fuzzy at times.  Perhaps something like "parameters relating to
> keys" would be more encompassing?
>

[IP]: Applied the suggested change.

> Section 2
>
>    required certificate evaluation and processing.  It is also
>    reasonable that a constrained device would have the hash of a
>    certificate associated with a public key and be configured use a
>    public key for that thumbprint, but without performing the
>    certificate evaluation or even having the entire certificate.
>
> This implies that the constrained device is not doing the required
> revocation checking, which probably merits some form of caveat (a
> management system that monitors revocation and updates the device if
> needed?).  This is particularly poingnant given the following paragraphs
> admonition that "[c]ertificates [...] MUST still be validated".
>

[IP]: I have added text to clarify this. You can find it in commit bd066fa
<https://github.com/cose-wg/X509/commit/bd066fa4175a5a673ee04213de2f1224a7db1442>.
Please let me know if further clarifications could be useful.

   a chain length of one as well as longer chains.  If the application
>    cannot establish trust in the certificate, that certificate cannot be
>    used.
>
> The secdir review already noted some potential issues with the blanket
> "cannot be used"; another possible rewording would be "the public key
> contained in the certificate cannot be used for cryptographic
> operations".
>

[IP]: For now I put your wording, but could you share what parts of the
certificate it might make sense to use in that case.

   x5chain:  This header parameter contains an ordered array of X.509
>       certificates.  The certificates are to be ordered starting with
>       the certificate containing the end-entity key followed by the
>       certificate which signed it and so on.  There is no requirement
>       for the entire chain to be present in the element if there is
>       reason to believe that the relying party already has, or can
>       locate the missing certificates.  This means that the relying
>
> Are the missing certificates required to be contiguous and contain the
> root?  That is, is it okay to leave a gap in the middle of the chain?
>

[IP]: My reading of the text is that it would not make sense to have a gap
in the middle. My understanding is that each certificate in the chain after
the first one should sign the one that came before it.

   [still x5chain]
>       This header parameter allows for a single X.509 certificate or a
>       chain of X.509 certificates to be carried in the message.
>
> It's slightly surprising that the "chain" structure allows for a single
> certificate not in an array container; it might be intuitively more
> simple to just always use the array encoding for a chain, even a
> one-element chain.  But I'm sure there's some reason to allow it, too,
> just not one that I thought of right away.
>

[IP]: I can only see this as saving us 1 byte, but probably there are other
reasons as well.

   x5t:  This header parameter provides the ability to identify an X.509
>       certificate by a hash value.  The attribute is an array of two
>
> I suggest using the word "thumbprint" somewhere to motivate the 't' in
> "x5t".
>

[IP]: Done.

Also, we may want to make a pass to check for consistent usage of
> "attribute", "parameter", etc. -- I think this is the first time we say
> "the attribute is".
>

[IP]: Apart from `protected attribute bucket` I am not sure if there should
be any other usage of attribute or they should all be changed to header
parameter. I have already changed some of them and I will think some more
for the others.

      For interoperability, applications which use this header parameter
>       MUST support the hash algorithm 'SHA-256', but can use other hash
>       algorithms.
>
> In light of the following notes, perhaps we should add another sentence
> along the lines of "This requirement allows for different
> implementations to be configured to use an interoperable algorithm, but
> does not preclude the use (by prior agreement) of other algorithms."
>

[IP]: I like this wording and I added it as you suggested it.

   x5u:  This header parameter provides the ability to identify an X.509
>    [...]
>       As this header parameter implies a trust relationship, the
>       attribute MUST be in the protected attribute bucket.
>
> In light of the secdir reviewer's comments, perhaps "a trust
> relationship between the party generating the x5u parameter and the
> party hosting the referred-to resource" would help?
>

[IP]: Done.

      The URI provided MUST provide integrity protection and server
>       authentication.  For example, an HTTP or CoAP GET request to
>       retrieve a certificate MUST use TLS [RFC8446] or DTLS
>       [I-D.ietf-tls-dtls13].  If the certificate does not chain to an
>       existing trust anchor, the certificate MUST NOT be trusted unless
>
> I think we should probably clarify that it is the "received" or
> "retrieved" certificate (as opposed to the certificate used to
> authenticate the (D)TLS connection used to dereference the URI).
>

[IP]: That is a good clarification indeed. Done.

>       the server is configured as trusted to provide new trust anchors.
>       In particular, self-signed certificates MUST NOT be trusted
>       without an out-of-band confirmation.

I agree with the secdir reviewer that some rewording/clarification is in
> order here.

[IP]: I reformulated it as

          If the retrieved certificate does not chain to an existing trust
anchor,
          the certificate MUST NOT be trusted unless the server is
configured
          as trusted to provide new trust anchors or if an out-of-band
confirmation
          can be received for trusting the retrieved certificate.

Please let me know if you think this is clear enough.

>          +---------+-------+---------------+---------------------+
>          | x5u     | TBD2  | uri           | URI pointing to an  |
>          |         |       |               | X.509 certificate   |
>          +---------+-------+---------------+---------------------+
>
> I agree with Roman that a more explicit statement of where the uri type
> is defined (or inline definition) is needed.
>

[IP]: I added a reference to RFC 3986 and a mention that the CBOR type of
this field is text string.

   The header parameters are used in the following locations:
>    [...]
>
> I guess draft-ietf-cose-countersign is on the hook for specifying
> anything needed about the X.509-related parameter usage?  It looks like
> COSE_Countersignature reuses the COSE_Signature sturcture, but I would
> not (naively) expect that to also inherit the ability to use these
> parameters.  COSE_Countersignature0 does not have any place to stow
> metadata, so I guess these parameters are not useful at all there.
>

[IP]: Is my understanding correct that you are wondering if we should do
anything about COSE_Countersignature in this document or should we only add
some text to draft-ietf-cose-countersign? I think we can discuss this
during the interim on 16 Dec 2020.

In Table 2, I think all the "AES192KW" and "AES256KW" should be "A192KW"
> and "A256KW", respectively.
>

[IP]: Fixed.

> Section 5
>
> I think we need to explicitly pull in (by reference) the security
> considerations from RFC 3986, arguably with specific call-out to the
> "reliability and consistency" portions.
>

[IP]: I added it.

> We might also mention that path validation is an important part of
> establishing trust in a certificate and point to RFC 5280.  I don't feel
> a huge need to also mention the possibility of constructing alternative
> chains, though we could do that as well if desired.
>

[IP]: Done.

We should probably also have some pro-forma mention of the strength of
> the hash used for a certificate thumbprint being a factor in the
> security of the system.
>

[IP]: Is this related to x5t? My understanding is that the algorithm does
not affect the security of the system as it is only used to select a
certificate that is already on the system. Even if a
different thumbprint is provided, could that lead to some attack? I tried
to add a sentence about this. Please let me know if you believe the text
needs to be improved or if you meant something else.

> Section 6.2
>
> I feel like RFC 8152 (or the bis) ought to be present as a normative
> reference; you cannot implement these new parameters outside the context
> of a broader COSE implementation.
>

[IP]: I agree. For now I made RFC 8152 as the normative reference as I
don't believe it is worth blocking this document due to missref.

---

From: Laurence Lundblade <[email-masked]>
Date: Mon, 14 Dec 2020 12:53:36 -0800

> On Dec 13, 2020, at 2:10 PM, Ivaylo Petrov <[email-masked]> wrote:
> 
> 
> [IP]: My reading of the text is that it would not make sense to have a gap in the middle. My understanding is that each certificate in the chain after the first one should sign the one that came before it.

I agree with Ivaylo on this. JWS is clear about this for the equivalent x5c parameter: https://tools.ietf.org/html/rfc7515#section-4.1.6

> [IP]: I can only see this as saving us 1 byte, but probably there are other reasons as well.

Seems like it would be less code if it was always an array and more inline with JWS.

> party hosting the referred-to resource" would help?

Not sure where everyone’s thinking is about replicating section 6 from JWS <https://tools.ietf.org/html/rfc7515#section-6>. As you know I’ve mentioned this before and I think it is necessary.

LL