Skip to content

Remove iso8601 dependency #1065

Closed
#1176
Closed
Remove iso8601 dependency#1065
#1176
Labels
clientRelated to the client (updater) implementationenhancementrepositoryRelated to the repository implementation
Milestone
Foundations

Description

@jku
jku
opened on Jul 3, 2020

I've been going through the tuf dependency chain with an eye on integrating tuf with pip: The issue with pip is that it's a package manager so needs to vendor everything it needs -- so being conservative with dependencies is a good idea. The good news is that tuf does not have many direct or indirect dependencies that would be a problem (good work!).

The one that possibly sticks out is "iso8601". The module is currently used in two places:

  • repository_tool: Metadata.expiration property getter uses it to return a datetime object
  • updater.py: Updater::_ensure_not_expired()

I'm mostly interested in that last one. it's used to compare the expiration stamp to current time and to format the error message:

    expires_datetime = iso8601.parse_date(expires)
    expires_timestamp = tuf.formats.datetime_to_unix_timestamp(expires_datetime)

    if expires_timestamp < current_time:
      message = 'Metadata '+repr(metadata_rolename)+' expired on ' + \
        expires_datetime.ctime() + ' (UTC).'
      logger.error(message)

      raise tuf.exceptions.ExpiredMetadataError(message)

I'm not familiar with date handling in python so my question is: Is this dependency valid or could this code be replaced with something that did not depend on iso8601?

Activity

  1. jku commented on Jul 3, 2020

    @jku
    jku
    on Jul 3, 2020
    MemberAuthor

    The spec says

    Metadata date-time data follows the ISO 8601 standard. The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". Time is always in UTC, and the "Z" time zone designator is attached to indicate a zero UTC offset.

    My naive understanding is that because of the zero offset requirement this might be easy to parse with standard library functions.

  2. jku commented on Jul 3, 2020

    @jku
    jku
    on Jul 3, 2020
    MemberAuthor

    That said, now that I've looked at iso8601 module itself... It's tiny, literally 215 lines of code. So vendoring it might not be a problem.

  3. joshuagl commented on Jul 3, 2020

    @joshuagl
    joshuagl
    on Jul 3, 2020
    Member

    #1060 makes use of dateutil. A quick browse of their docs made me think we could probably replace iso8601 with functionality in dateutil.

  4. joshuagl commented on Jul 3, 2020

    @joshuagl
    joshuagl
    on Jul 3, 2020
    Member

    The spec says

    Metadata date-time data follows the ISO 8601 standard. The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". Time is always in UTC, and the "Z" time zone designator is attached to indicate a zero UTC offset.

    My naive understanding is that because of the zero offset requirement this might be easy to parse with standard library functions.

    oh, that would be even better!

  5. joshuagl
    added
    enhancement
    good first issueBite-sized items for first time contributors
    up for grabs
    on Jul 3, 2020

  6. jku commented on Jul 3, 2020

    @jku
    jku
    on Jul 3, 2020
    MemberAuthor

    #1060 makes use of dateutil. A quick browse of their docs made me think we could probably replace iso8601 with functionality in dateutil.

    For the pip use case dateutil would be worse than iso8601 as it's much larger -- but I guess #1060 wouldn't necessarily mean updater changes?

    Oh and for reference: securesystemslib does use dateutil but AFAICS only in one place in gpg code

  7. joshuagl commented on Jul 3, 2020

    @joshuagl
    joshuagl
    on Jul 3, 2020
    Member

    #1060 makes use of dateutil. A quick browse of their docs made me think we could probably replace iso8601 with functionality in dateutil.

    For the pip use case dateutil would be worse than iso8601 as it's much larger -- but I guess #1060 wouldn't necessarily mean updater changes?

    In the short term, no. However, my current thought process is that the work in #1060 could become a low-level abstraction around TUF metadata that we build client/updater and repository APIs on top of. Therefore I'd like to make/keep those APIs as minimal (with as minimal dependencies) as possible so that they can be comfortably vendored into pip.

    Note to self (or whoever picks up this issue): look at the use of dateutil in tuf/api and figure out if we can achieve the same with the standard library. Specifically in tuf/api/metadata we're currently using dateutil.relativedelta to cleanly specify a number of days/months/years by which to bump metadata expiration.

    Oh and for reference: securesystemslib does use dateutil but AFAICS only in one place in gpg code

    💯 Thanks for looking into this.

  8. joshuagl
    modified the milestones: Next release – PEP 458 integration, on Jul 7, 2020

  9. joshuagl commented on Aug 24, 2020

    @joshuagl
    joshuagl
    on Aug 24, 2020
    Member

    The dateutil usage in securesystemslib does not appear to be necessary, so I created a PR to remove it secure-systems-lab/securesystemslib#268

  10. joshuagl
    removed this from the milestone on Sep 8, 2020
  11. joshuagl
    removed
    good first issueBite-sized items for first time contributors
    on Sep 10, 2020

  12. 2 remaining items

  13. joshuagl
    added
    clientRelated to the client (updater) implementation
    repositoryRelated to the repository implementation
    on Sep 10, 2020
  14. joshuagl
    changed the title [-]Updater: review iso8601 dependency[/-] [+]Review iso8601 dependency[/+] on Sep 10, 2020

  15. joshuagl commented on Sep 10, 2020

    @joshuagl
    joshuagl
    on Sep 10, 2020
    Member

    Our new metadata model (#1112) avoids using iso8601, which we'll build upon for both the client and repository code during the course of our refactor

  16. lukpueh commented on Sep 10, 2020

    @lukpueh
    lukpueh
    on Sep 10, 2020
    Member

    @joshuagl suggests to create a helper for the iso8601 replacing call

    datetime.strptime(exires, "%Y-%m-%dT%H:%M:%SZ").replace(tzinfo=None)
  17. lukpueh
    added a commit that references this issue on Sep 10, 2020

    Remove iso8601 dependency from simple metadata api

    f106435
  18. MVrachev
    added a commit that references this issue on Sep 17, 2020

    Remove iso8601 dependency from simple metadata api

    3accf60

  19. jku commented on Oct 13, 2020

    @jku
    jku
    on Oct 13, 2020
    MemberAuthor

    I've just reviewed the iso8601 test suite and have come to the conclusion that using the iso8601 module is in fact not compliant with the (spirit of the) tuf spec: current implementation allows for timestamps that I would not expect to work after reading the spec, such as "2006-10-20T15:34:56.123+02:30".

    The spec currently says:

    Metadata date-time data follows the ISO 8601 standard. The expected format of the combined date and time string is "YYYY-MM-DDTHH:MM:SSZ". Time is always in UTC, and the "Z" time zone designator is attached to indicate a zero UTC offset. An example date-time string is "1985-10-21T01:21:00Z".

    I think the paragraph is fairly clear but first sentence could be better: we want to support only this very specific format of the ISO8601 not the whole iso8601 spec.

    As for the implementation, I think the helper idea is good. I don't think the replace() call in the example is needed: the strptime format used does not allow for a timezone at all.

  20. jku commented on Oct 13, 2020

    @jku
    jku
    on Oct 13, 2020
    MemberAuthor

    Adding one more thing: the Securesystemslib schema for "iso8601 datetime string" is very strict: datetime can definitely parse that.

    I think the review is over... but since there's good discussion here I'm not closing: I'll just rename this to "Remove iso8601 dependency"

  21. jku
    changed the title [-]Review iso8601 dependency[/-] [+]Remove iso8601 dependency[/+] on Oct 13, 2020
  22. jku
    added a commit that references this issue on Oct 13, 2020

    Remove iso8601 dependency

    62b2d57

  23. trishankatdatadog commented on Oct 13, 2020

    @trishankatdatadog
    trishankatdatadog
    on Oct 13, 2020
    Contributor

    I think the paragraph is fairly clear but first sentence could be better: we want to support only this very specific format of the ISO8601 not the whole iso8601 spec.

    Great observation. Please send a PR to the spec 🙂

  24. jku
    added 2 commits that reference this issue on Oct 13, 2020

    Remove iso8601 dependency

    95bed5e

    Remove iso8601 dependency

    2f69986
  25. lukpueh
    closed this as completedin #1176on Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    clientRelated to the client (updater) implementationenhancementrepositoryRelated to the repository implementation

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions