Remove microseconds from metadata API Signed.expires

[ivanayov]

This change removes microseconds from expiry in order to fit TUF
specification

Fixes #1678

• The code follows the Code Style Guidelines
• [n/a] Tests have been added for the bug fix or new feature
• [n/a] Docs have been added for the bug fix or new feature

[coveralls]

Pull Request Test Coverage Report for Build 1563947545

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 5 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+1.0%) to 98.563%

---

Files with Coverage Reduction	New Missed Lines	%
tuf/api/metadata.py	5	98.12%

Totals
Change from base Build 1563115608:	1.0%
Covered Lines:	3936
Relevant Lines:	3963

---

💛 - Coveralls

[jku]

We'll want think this through for all the different cases: deserialization, users calling the api , serialization... it's a little detail but could make life difficult in the future

• I'd want to see docstrings for public API at least
• is it correct that we accept non-spec-compliant dates on deserialize?
• is it correct that we change the date to compliant on deserialize?

[ivanayov]

@jku

is it correct that we accept non-spec-compliant dates on deserialize?

It think that spec-compliant should mean spec-compliant with no exception, so if metadata is not in the correct form, we should throw an error.

is it correct that we change the date to compliant on deserialize?

I think it's fine, when it comes to preventing a bug, coming not from the metadata, but from the implementation (and in this case - python specific) as we guarantee that it won't break the deserialisation. But we should guarantee that the metadata is correct at the same time.
May be I'd need to add a check, which is now missing.

[lukpueh]

I agree with @jku that we need to be careful here. This is what I think about the different cases:

• deserialization:
  The deserializer shouldn't accept non-spec compliant metadata, regardless of its importance. If we start modifying the semantics of inputs during deserialization, we'll likely end up with determinism problems (goal: data == serialize(deserialize(data))

• Object API:
  Ideally, we don't accept non-sepc compliant inputs here either. But in reality this is hard to enforce, because properties can be set in different ways. So I think it is easier and okay to only enforce spec compliance on the de/serialization boundaries.

• Serialization:
  Under above assumptions, i.e. that we don't accept non-spec compliant metadata in deserialization, and that some properties that were modified during object life-time contain additional (unneeded) information, certain transformations should be fine, e.g. removing unnecessary microseconds. We need to find a balance between expected behaviour and usability.

[jku]

I think I lean in the same direction (EDIT: I wrote this before seeing lukas' last message :) )

• spec defines the date format exactly: we should only accept that in deserialization?
• constructing a new object should probably accept microseconds and just remove them
• deserialization and constructing new object currently use same code path (in the constructor)

so possibly

• from_dict() should be strict, and should fail if format is not exactly as defined in spec
• but the constructor should use set_expiry() as your PR does: so constructing new object does not fail on microseconds ...

I'm not 100% sure of this yet, but that's my thinking...

[lukpueh]

I agree with the overall direction...

from_dict() should be strict, and should fail if format is not exactly as defined in spec

This makes sense, and we might need to revise existing data validation in from_dict/__init__-methods, because I saw that some is performed in one and some in the other.

but the constructor should use set_expiry() as your PR does: so constructing new object does not fail on microseconds

I guess we can do that, but we need to to be careful that there's no unexpected behavior, above all upon serialization, if the expires field is modified outside of the constructor

[jku]

but we need to to be careful that there's no unexpected behavior, above all upon serialization, if the expires field is modified outside of the constructor

Yes, and should consider buggy code as well (obviously we'd never write bugs but just theoretically speaking): we should ensure we rather fail than persist metadata that is invalid. #1696 has some discussion on this

[ivanayov]

In the latest update I stick to:

• __ini__() uses set_expiry(), so that we guarantee microseconds are not added on runtime
• a format verification for "expires" in _common_fields_from_dict()
• no format changes (which I pushed earlier) in _common_fields_to_dict()

[kairoaraujo]

To test your code (lazily), I used the tests/metadata_api_tests/test_metadata_serialization.py and I added an invalid expire date entry to this data set

python-tuf/tests/test_metadata_serialization.py

Line 169 in d3b877b

invalid_roots: utils.DataSet = {

        "invalid expire date": '{"_type": "root", "spec_version": "1.0.0", "version": 1, \
            "expires": "2021-11-16T12:00:00.123456Z", "consistent_snapshot": false, \
            "keys": { \
                "keyid1" : {"keytype": "rsa", "scheme": "rsassa-pss-sha256", "keyval": {"public": "foo"}}, \
                "keyid2" : {"keytype": "ed25519", "scheme": "ed25519", "keyval": {"public": "bar"}}}, \
            "roles": { \
                "root": {"keyids": ["keyid1"], "threshold": 1}, \
                "timestamp": {"keyids": ["keyid2"], "threshold": 1}, \
                "targets": {"keyids": ["keyid1"], "threshold": 1}, \
                "snapshot": {"keyids": ["keyid2"], "threshold": 1}} \
            }',

It gave me the expected raise:

  File "/Users/kdearaujo/devel/TUF/python-tuf/tuf/api/metadata.py", line 476, in _common_fields_from_dict
    raise exceptions.InvalidConfigurationError(
tuf.exceptions.InvalidConfigurationError: 
            Role root should have "expires" metadata of the format 
            "yyyy-mm-ddThh:mm:ssZ" but got 2021-11-16T12:00:00.123456Z instead

But I am not sure if the right to a place to add the test or even we want to test it there. I noticed that we don't test these custom exceptions in tests_metadata_serialization.py

Maybe some maintainer could comment :)

[lukpueh]

Thanks for working on this @ivanayov. I think it's a fair approach for the first two use cases discussed above. We still need to figure out what to do for the 3rd use case, that is do we allow serialisation of invalid metadata. But maybe we don't have to do this here. Would you mind adding a test for what you have so far?

[ivanayov]

@lukpueh @kairoaraujo Just pushed an update with latest comments.

[jku]

I left some comments, I think the core of this PR is the new API (set_expiry()) which allows both construction of new metadata and modifications to existing expiry to be done safely: I think this is the correct direction but new API needs to to be polished (include API documentation and so on).

The big question is, do we need to solve the related #1727 at the same time (at least on paper)?

[jku]

We still need to figure out what to do for the 3rd use case, that is do we allow serialisation of invalid metadata

Martin opened #1696 about this relevant question in general: maybe we can continue there (the issue is big enough to maybe have a chat as well, to discuss some possible ways to deal with it)

[jku]

oh a new idea: since one of the problems here is that our expiry is a datetime but most datetimes are not valid expiries (because microseconds), how about we hide the variable and make expiry a property (where the setter is Ivanas function) -- that would effectively prevent a user from accidentally setting signed.expiry to an invalid value: something like signed.expiry = utcnow() + relativedelta(day=3) would now work correctly because it would use the property setter.

[jku]

This feels like the correct solution, thank you.

• API user just provides a datetime, we make sure it's valid (removing microseconds)
• the Metadata object always matches the serialized output (since the microseconds are never stored in the object)
• type "enforcement" is about as good as it gets with python (static checks will catch wrong types and we use the datetime API in the setter so even without static checks things will break on type mismatches)

Looks good to me, leaving open for @lukpueh to see as well

[lukpueh]

Seconding Jussi's assessment. Thanks for the clean fix, @ivanayov!