Encoding of header parameters varies depending on order of keys in the dict

[achamayou]

I stumbled (quite badly!) across this while passing application-specific header parameters. I am trying to pass Python str values, and instead of being encoded as tstr, they come out of as bstr. A look at to_cose_header() explains why, they are being explicitly encoded that way. I want to propose a patch for this, so I started with trying a small repro:

        protected = {COSEHeaders.KID: b"01", COSEHeaders.ALG: COSEAlgs.ES256, "content type": "application/cwt"}

        ctx = COSE.new(alg_auto_inclusion=True)
        encoded4 = ctx.encode_and_sign(b"Hello world!", cose_key, protected=protected)
        phdr, _, payload = recipient.decode_with_headers(encoded4, pub_key)
        assert payload == b"Hello world!"

        assert phdr[COSEHeaders.ALG] == COSEAlgs.ES256
        assert phdr["content type"] == "application/cwt"

But that works. How?

This does not work:

        # Different order than 781, but surely equivalent?
        protected = {"content type": "application/cwt", COSEHeaders.KID: b"01", COSEHeaders.ALG: COSEAlgs.ES256}

        with pytest.raises(ValueError) as err:
            # Raises ValueError: Unsupported or unknown COSE header parameter: 4.
            # Very surprising!
            encoded5 = ctx.encode_and_sign(b"Hello world!", cose_key, protected=protected)

            phdr, _, payload = recipient.decode_with_headers(encoded5, pub_key)
            assert payload == b"Hello world!"

            assert phdr[COSEHeaders.ALG] == COSEAlgs.ES256
            assert phdr["content type"] == "application/cwt"

It turns out that to_cose_header() does this:

if len(data) == 0 or not isinstance(list(data.keys())[0], str):

If the first key in the keys of the header dict is not a string, the code assumes the dict has already been encoded, and lets it through unmodified. Huh. On the one hand, this is great, now I have a way to get my tstr through. On the other hand... yikes!

Repro available on: https://github.com/achamayou/python-cwt/tree/encoding_repro

pytest -k test_cose_usage_examples_cose_signature

I can think of a few solutions, most of which involve letting the user handle header pre-processing. Another one may be to add a specific type for the built-in header parameters, instead of using string mappings. That way the logic could be: int and str keys are let through unprocessed all the way to cbor2, but only keys of COSEHeaderKey type are mapped to int. Collisions probably throw an exception?

I will think about this over the weekend, but I wanted to write this up while it is fresh. I should also say that the library is lovely, and a pleasure to use so far!

[dajiaji]

@achamayou thank you for filing the issue!

I’m sorry for taking up your valuable time with my poor code.

I hadn’t considered the case where key types are mixed at all…
I’ve long wanted to do a major refactor of this library, but I haven’t been able to find the time. Pull requests are more than welcome, so please don’t hesitate to send one.

[achamayou]

I think there are two main possible directions to resolve this problem, and allow users to exploit the full range of COSE header possibilities, while maintaining convenient usage of common values.

Option A is a breaking change for the library, and would require a major release, but seems preferable to me:

Remove calls to to_cose_header() from

python-cwt/cwt/cose.py

Line 590 in 40fe8e0

u = to_cose_header(unprotected)

(and a couple of similar locations).

Users who rely on string shortcuts, such as "alg", either need to:

1. Switch to the enum equivalent (COSEHeaders.ALG)
2. Wrap their header dict in a to_cose_header() call
3. Use a per-key lookup that the library can provide (from cwt.utils import hp; ...protected={hp("alg")})

Option B (#636) is backwards compatible, and does not require a major release, but complicates the library and the calling path for users who need tstr header parameters:

Instead of this check: https://github.com/dajiaji/python-cwt/blob/main/cwt/utils.py#L174

to_cose_header() instead checks for a wrapper type, let's say EncodedHeader, defined as:

class EncodedHeader:
  def __init__(self, header):
    self.header = header

The same early-out logic applies, and skips the mapping of string values. This means callers that want to use tstr must do this:

sign(...protected=EncodedHeader({"foo": ..., COSEHeaders.ALG: ...})

@dajiaji if you could let me know your preference, I'd be happy to post the corresponding PR.

[achamayou]

After experimenting with #636, I can see that Option B as originally envisaged is not backwards compatible, because a lot of usage of the library relies on this exception:

not isinstance(list(data.keys())[0], str)

In fact all usage that sets alg_auto_inclusion=True or kid_auto_inclusion=True tends to go that way, because it sets integer alg/kid.

So I don't think there is a way to preserve compatibility without keeping that exception unfortunately.

[achamayou]

I believe this is addressed by #636 now.