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

From: Benjamin Kaduk <[email-masked]>
Date: Wed, 20 Jan 2021 23:12:10 -0800

Hi Ivo,

This document being back on the IESG agenda caused me to notice that I was
delinquent in responding to you.  Sorry about that, though I think there is
not much left to actually do.

I looked over the changes (now available in the -08) and they all look
good.  Just a few points inline, mostly where you had asked for
clarification...

On Sun, Dec 13, 2020 at 11:10:43PM +0100, Ivaylo Petrov wrote:
> 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.

It has been a while since I thought about this, so my memory is hazy.  I
think one potential case would be to log some information about the
certificate for diagnostics, but I don't remember what else I had in mind.

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

Your understanding is correct.  I was not very functional (nor was my a/v
setup) during that interim, but IIRC we did conclude that this document
should not say anything about countersignatures.

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

Yes, this was relating to x5t, and your explanation enlightened me that I
was wrong.

Thanks,

Ben

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