Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metadata api: Signed.expires string representation should not contain microseconds #1678

Closed
lukpueh opened this issue Nov 16, 2021 · 3 comments · Fixed by #1712
Closed

metadata api: Signed.expires string representation should not contain microseconds #1678

lukpueh opened this issue Nov 16, 2021 · 3 comments · Fixed by #1712
Assignees
Labels
backlog Issues to address with priority for current development goals
Milestone

Comments

@lukpueh
Copy link
Member

lukpueh commented Nov 16, 2021

Description of issue or feature request:

In signed._common_fields_to_dict we convert the value of the datetime object in Signed.expires to a date-time string.

From the TUF spec:

[...] The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". [...] An example date-time string is "1985-10-21T01:21:00Z".

If the original datetime object contained microseconds, the date-time string representation of Signed.expires will too.

Reproducer:

from datetime import datetime
from tuf.api.metadata import Timestamp, MetaFile
Timestamp(1, "1.0.19", datetime(2021, 11, 16, 12, 0, 0, 123456), MetaFile(1)).to_dict()["expires"]

Current output:

'2021-11-16T12:00:00.123456Z'

Expected behavior:

'2021-11-16T12:00:00Z'

Proposed fix 1:

diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 25f14fe7..cdbd8595 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -480,7 +480,7 @@ class Signed(metaclass=abc.ABCMeta):
             "_type": self._type,
             "version": self.version,
             "spec_version": self.spec_version,
-            "expires": self.expires.isoformat() + "Z",
+            "expires": self.expires.replace(microsecond=0).isoformat() + "Z",
             **self.unrecognized_fields,
         }

Proposed fix 2:

diff --git a/tuf/api/metadata.py b/tuf/api/metadata.py
index 25f14fe7..20365cfe 100644
--- a/tuf/api/metadata.py
+++ b/tuf/api/metadata.py
@@ -480,7 +480,7 @@ class Signed(metaclass=abc.ABCMeta):
             "_type": self._type,
             "version": self.version,
             "spec_version": self.spec_version,
-            "expires": self.expires.isoformat() + "Z",
+            "expires": datetime.strftime(self.expires, "%Y-%m-%dT%H:%M:%SZ"),
             **self.unrecognized_fields,
         }
@jku
Copy link
Member

jku commented Nov 16, 2021

yeah this is definitely not ideal. I suppose there are two options:

  • error out if microseconds are found in serialization (not user friendly but a visible failure)
  • do what you suggest: make sure the serialized content is valid, even modifying the content if needed

In both cases we could also:

  • provide a public setter for expiry (taking a datetime (or even a timedelta from now) as input, flooring microseconds)
  • use the setter in init()

these steps would make it easier for Metadata to not be in a state where it doesn't quite match the form it gets serialized into...

@lukpueh
Copy link
Member Author

lukpueh commented Nov 17, 2021

Good point! I guess, I'm fine either way. Losing (negligible) precision during serialisation doesn't seem like a big deal, and I don't think there is a realistic use case where this leads to unexpected behaviour. But yes, erroring seems safer.

Regardless, I'm all for providing the convenience setter you propose.

OTOH, I'm a little bit reserved about changing the signature of init, i.e. s/expires/expires_in/, because I think the Metadata class should look as much like the spec as possible. But I won't insist here either.

@sechkova sechkova added the backlog Issues to address with priority for current development goals label Nov 24, 2021
@ivanayov
Copy link
Collaborator

ivanayov commented Dec 3, 2021

Can I take this please?

ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 7, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
@sechkova sechkova added this to the Sprint 14 milestone Dec 8, 2021
ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 9, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 10, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 10, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 15, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
ivanayov added a commit to ivanayov/python-tuf that referenced this issue Dec 15, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
sechkova pushed a commit to sechkova/tuf that referenced this issue Dec 21, 2021
This change removes microseconds from expiry in order to fit TUF
specification

Fixes theupdateframework#1678

Signed-off-by: Ivana Atanasova <iyovcheva@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants