Skip to content

Conversation

jruizaranguren
Copy link

@jruizaranguren jruizaranguren commented Aug 25, 2025

This merge request:

  • Removes the dependency constraint cbor2>=5.4.0,<5.5.0, which can cause dependency‑resolution conflicts and block downstream projects from receiving security updates.
  • Updates cbor2 to the latest release. This surfaced a change in date/time handling that affects pyMDOC-CBOR.

Impact:

  • Only one test failed after the update: pymdoccbor/tests/test_08_mdoc_cbor.py.
  • A targeted workaround has been applied so the date assertion passes.

Next steps:

  • Release a new version of the library without cbor2 pin
  • Perform a thorough review of date/time handling in both code and tests, considering the behavior described in the cbor2 documentation: https://cbor2.readthedocs.io/en/latest/usage.html#date-time-handling
    • Decide on a canonical representation (naive vs. timezone‑aware, UTC normalization).
    • Define parsing/serialization policy and CBOR tag handling.
    • Add tests that cover timezones, DST, and round‑trip encoding/decoding.

cert_path: str | None = None,
revocation: dict | None = None,
status: dict | None = None
status: dict | None = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
status: dict | None = None,
status: dict | None = None

encoding=serialization.Encoding.Raw,
format=serialization.PublicFormat.Raw
)
format=serialization.PublicFormat.Raw,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
format=serialization.PublicFormat.Raw,
format=serialization.PublicFormat.Raw

"big"
)
(public_key.public_numbers().e.bit_length() + 7) // 8, "big"
),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
),
)

validity=validity,
revocation=revocation,
cert_info=self.cert_info
cert_info=self.cert_info,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cert_info=self.cert_info,
cert_info=self.cert_info

validity=validity,
revocation=revocation,
cert_info=self.cert_info
cert_info=self.cert_info,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cert_info=self.cert_info,
cert_info=self.cert_info

mso = msoi.sign(
doctype=doctype,
device_key=devicekeyinfo,
valid_from=datetime.now(timezone.utc),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
valid_from=datetime.now(timezone.utc),
valid_from=datetime.now(timezone.utc)

for ns, dgst in msoi.disclosure_map.items()
},
"issuerAuth": cbor2.decoder.loads(mso_cbor),
"issuerAuth": cbor2.loads(mso_cbor),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"issuerAuth": cbor2.loads(mso_cbor),
"issuerAuth": cbor2.loads(mso_cbor)

@peppelinux
Copy link
Member

we may have security updates in 5.4.x and breaking changes in 5.5.0, this is why we decided to specify the version of a dependency using a known minor release

This surfaced a change in date/time handling that affects pyMDOC-CBOR

therefore we should move our deps up to cbor2 55., and removing support for 5.4

we wil lcontinue the review, I am interested in having in this current PR also these ones:

  • Decide on a canonical representation (naive vs. timezone‑aware, UTC normalization).
  • Define parsing/serialization policy and CBOR tag handling.
  • Add tests that cover timezones, DST, and round‑trip encoding/decoding.

I'd wait for any of your proposal for the impl of the previous tasks, therefore I will engage @PascalDR in the final review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants