rust: reconsider dependency on `time` crate in public APIs

[valkum]

The time crate had multiple issues in the past and thus was replaced by other crates in numerous codebases.
Now there is chrono with at least the same amount of downloads in the last 90 days and jiff as a rather new crate.

Until the Rust ecosystem consolidates on one major crate again, the best way is to support at least the two big ones or stick to std::time.
time is currently only used in MessageSigner::generate_signature_headers_content to get the value for created and expires.

created can simply be replaced by (this is taken from time::UtcDateTime::now()):

match system_time.duration_since(SystemTime::UNIX_EPOCH) {
            Ok(duration) => Self::UNIX_EPOCH + duration,
            Err(err) => Self::UNIX_EPOCH - err.duration(),
        }

jiff goes so far and says it would be ok to panic if the local time is too far in the past or future. But that completely depends on whether this crate should be panic-free or not.

The expires argument for MessageSigner::generate_signature_headers_content should just be std::time::Duration. I don't think using time::Duration has any benefit here. The major difference is that time::duration can be negative, which does not make sense for the expires argument, at least if the docs of MessageSigner::generate_signature_headers_content are taken as granted.

This will keep the API surface clean of any particular date & time crate for now.

[AkshatM]

The primary reason for using time instead of std::time (which we originally used) was support in Cloudflare Workers: std::time (more accurately Instant, which duration_since is a member of) is not supported by wasm32-unknown-unknown, which (I presumed) was the build target used by Cloudflare Workers[0]. The official page for crates supported by Workers lists time as a preferred citizen.

This way, we'd have support in both ordinary POSIX and WASM environments, which made sense for a library for HTTP interactions (and lets users interchange with the Typescript implementation as well).

Switching to another time library is valid, and revisiting the decision to not use std::time is fine as well. After all, there is experimental support for wasm32-wasi in Workers, which would mean we could use std::time after all. I am reluctant to do that without learning more about whether this is a standard architecture targeted by users first.

That being said, I would be interested to learn more about the issues behind time first before considering a switch. As far as I can see, the main issue had been Rust 1.80's changes to type inference rules, which affected crates that relied on older versions of time. It doesn't invalidate the use of time itself. If you could provide that, would be excellent.

But that completely depends on whether this crate should be panic-free or not.

Personally, I would not want this crate to panic. It seems entirely unnecessary for a library to do so.

[0] How I discovered this wasn't supported is somewhat unpleasant: rustc will compile std::time code fine, but will emit a runtime panic stating that time is not supported when code actually uses it. Thus, it's the sort of bug that's not discoverable until you use it.

[valkum]

Thanks for your reply. I did not take Cloudflare workers into account.
My dislike of the time crate mostly comes from https://rustsec.org/advisories/RUSTSEC-2020-0071.html. I just refreshed my memory on it, and I guess I was wrong with discrediting the time crate.

I still think it might make sense to add support for chrono as well. As far as I can tell (I have not tested compatibility with wasm32-unknown-unknown and Cloudflare Workers) chrono should be working with the wasmbind feature enabled.

I am wondering if providing a more generic way could work and make the API more usable across different codebases. Without tying in chrono at all.
For example, ::time::Duration is only used in MessageSigner::generate_signature_headers_content to add to the result of ::time::UtcDateTime::now().

let created = UtcDateTime::now();
let expiry = created + expires;

::time::UtcDateTime does implement Add<std::time::Duration>[0] so MessageSigner::generate_signature_headers_content could just take std::time::Duration instead of ::time::Duration. This would make the usage of ::time an implementation detail.

I guess your duration_since example comes from a different place, I assume SignatureTiming? I think this could also use std::time::Duration in it's public API by just using ::time as an implementation detail.

[0]: Checked the actual usage, and if I didn't miss it, it will just call std::Duration::as_secs()

[AkshatM]

I welcome PRs that can let us use std::time, provided it can run in both POSIX and WASM environments. I'm also open to a generic API, where people can bring their own time library. My only expectation is that the examples need to be updated and tested accordingly. I do not think adding support for both chrono and time takes precedence over those options.

Feel free to use the Cloudflare Worker example as a testbed / verification case for the wasm32-unknown-unknown target.

[valkum]

Do you have proper code guidelines for Rust at Cloudflare or are they project-based? Looking at all those use declarations (mixed declarations from std and crate), it urges me to hand in a PR that cleans those up as well, but I wanted to check with your first.

[AkshatM]

Per-project, and, for this crate in particular, I am satisfied with how cargo fmt reorders crate declaration.

If you wish to add a cleanup on top, feel free and I can review / accept it.