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

API responses should include time-zone information, be valid ISO8061/RFC339 #6257

Open
3 of 4 tasks
gonzalo-bulnes opened this issue Feb 9, 2022 · 3 comments · May be fixed by #6413
Open
3 of 4 tasks

API responses should include time-zone information, be valid ISO8061/RFC339 #6257

gonzalo-bulnes opened this issue Feb 9, 2022 · 3 comments · May be fixed by #6413

Comments

@gonzalo-bulnes
Copy link
Contributor

gonzalo-bulnes commented Feb 9, 2022

Description

A recent change aimed at including time zone information caused the API to respond with date strings in an invalid format. As a workaround, the time zone information was removed from the API responses.
For the sake of focus, I'll keep the details on how the issue happened in the comments section below.

While that is an effective workaround, it does result in more fragile communication between the SecureDrop server and potential clients.
Instead of communicating via date strings that do not include time zone information (but are ambiguous because of that), we should resume communicating via date strings that do include time zone information (i.e. that are unambiguous) and make sure that we:

  1. use a valid date format (like we used to do, and can still do within existing constraints)
  2. use UTC where possible for inter-service communication (like we used to do and have no reason to stop doing)

Current Behavior

The API responses contain date strings with no time zone information, e.g. 2022-02-09T05:22:12.165773
This time could be legitimately interpreted as 5 in the morning in Australia, or 5 in the morning in London.

To make things even less reliable, default behaviors vary: for example RFC3339 specifies that time zone information shall always be provided [Section 4.3] [Section 5.6], while ISO8061 specifies that the local time zone could be assumed. 🌦️ Because of that, stripping time zone information is not a reliable long-term solution.

Note: The ISO documents are behind a pay wall, please bear with the Wikipedia links! 🐻

Expected Behavior

The API responses contain date strings in the following format 2022-02-09T05:22:12.165773+00:00.

  • 🍏 This string is valid when interpreted by both the RFC3339 and ISO8061 standards.
  • 🍏 This string is supported by the ISO8061 parser included in the datetime package (which is rather limited).

Why this format specifically?

  1. The date strings contained in API responses should be unambiguous. And several formats can achieve that, for example 2022-02-09T05:22:12.165773Z or 2022-02-09T05:22:12.165773+00:00 or 2022-02-09T15:52:12.165773+10:30. All these examples describe the exact same moment, around 5 in the morning UTC.

  2. For machine to machine communication, there is no need to communicate a preference for any given local time representation, and UTC could (should?) be favored. (Local times are mostly useful for people, with the noteworthy exception of people troubleshooting logs, who benefit from consistent logging! UTC is a good "neutral ground" for that.) So 2022-02-09T05:22:12.165773Z or 2022-02-09T05:22:12.165773+00:00 are preferred.

  3. Different standards interpret a zero time offset sightly differently. In RFC3339, +00:00 expresses a preference for UTC and -00:00 expresses the absence of preference. The negative zero offset -00:00 is simply not valid in the context of ISO8061. The +00:00 notation is valid in both standards, and expresses a preference for UTC. (So far, the +00:00 and Z variants would be equally good, hold that thought.)

  4. Additionally, the datetime parser only supports a limited subset of ISO8061 strings. I particular, 2022-02-09T05:22:12.165773Z cannot be parsed by the datetime.fromisoformat(date_string) function. So 2022-02-09T05:22:12.165773+00:00 is a more convenient choice.
    Because using existing datetime methods ensures consitency and could prevent similar bugs in the future, the +00:00-flavored format seems like a better choice.

Summary of usage

from datetime import datetime, timezone

# Datetime object MUST be created with time zone information:
d = datetime.now(timezone.utc)
d
# datetime.datetime(2022, 2, 9, 5, 9, 19, 592689, tzinfo=datetime.timezone.utc)

# Serialization:
date_string = d.isoformat()
# '2022-02-09T05:09:49.423016+00:00'

# This date_string is good to send to other services, across the network etc.

# Deserialization:
d = datetime.fromisoformat(date_string)
d
# datetime.datetime(2022, 2, 9, 5, 9, 19, 592689, tzinfo=datetime.timezone.utc)

Comments

Timeline

I was able to trace the sequence of events that lead to this situation:

  1. In 2018, a UTC time started being sent in the API response. Date-times were handled without timezone information internally, and UTC was specified during serialization. (c324abe) On the SDK side, those were de-serialized in a consistent manner.
    The hard-coding of the time-zone information very likely responded to the limitations of the datetime package (e.g. reference), the code did internally assumed UTC and all ambiguity was cleared in communications. ✔️

  2. A few months ago, time zones started being handled internally. (1633566#diff-0814e8ec8e8bcae2e28501f8d8397a3199fbff361b132552d597e7e9696e88baR118-R122) A side-effect of that was that the hard-coded UTC Z became redundant, and caused the API to respond with invalid (ISO 8061) date strings. IMHO that was an easy side-effect to miss, and hopefully we don't hard-code time format handling in the future 🙂
    At that point, the SDK couldn't handle the invalid strings. 🐛 Journalist API datetime format has changed, breaking client login. #6255 and Add support for an ISO 8601 timezone compliant token expiration date-timestamp securedrop-sdk#171 (Note: I don't think that issue suggests the best solution, because Z is less well supported than +00:00 in Python context, see above.)

  3. Today a workaround was applied, Update API to return datetimes without offset field. #6256 which brings us here.

I hope that makes sense : )

Proposed plan

  • Open this issue describing the situation in details.
  • Open a PR to ensure that the SecureDrop API responds with valid ISO8061/RFC3339 strings, which use the +00:00 variant to ensure we let datetime do the formatting and hopefully prevent future inconsistencies.
  • Open a PR to ensure that the SDK supports the time-zone information (+00:00 in practice) in the responses it gets from the API and allows the SDK to handle some of the formats (valid or invalid) we've used in the past: with time zone (Z-flavored, +00:00-flavored), with twice the info +00:00Z, any other? Ensure support for common date string formats securedrop-sdk#172
  • Upgrade the Client with the new SDK.

Urgency

This is not urgent, since a workaround is in place.
The SDK should be updated first, then the Client should use it (not a release blocker) and finally the servers could be released. The SDK will accept inputs generously, so the server release doesn't need to be coordinated with the release of the Client as long as it happens last.

CC: @creviera

@gonzalo-bulnes
Copy link
Contributor Author

Additional argument for favoring isoformat/fromisoformat over strftime manual formatting: https://github.com/freedomofpress/securedrop/pull/6256/files#r802309565

@sssoleileraaa
Copy link
Contributor

securedrop-client 0.7.0 release is complete so the only task left in the Proposed plan section in the issue description is:

Open a PR to ensure that the SecureDrop API responds with valid ISO8061/RFC3339 strings, which use the +00:00 variant to ensure we let datetime do the formatting and hopefully prevent future inconsistencies.

@gonzalo-bulnes
Copy link
Contributor Author

We'll have to decide how long we want to keep backwards-compatibility re: deserializing invalid date strings in the SDK, and some day clean up that code. Thinking aloud: we may want to do that when/if we start supporting other time zones than UTC.

@gonzalo-bulnes gonzalo-bulnes linked a pull request Apr 27, 2022 that will close this issue
5 tasks
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 a pull request may close this issue.

2 participants