refactor: replace time::Duration with std::time::Duration in public API#79
Conversation
AkshatM
left a comment
AkshatM
left a comment
There was a problem hiding this comment.
Have some comments, but overall LGTM. Similar to you, I tested the example worker and was pleased to see it working as expected - it directly tests Duration by generating headers in the response containing an expiry time.
Once you make the changes to use unisgned_abs(), I am happy to approve.
| let generation = UtcDateTime::now(); | ||
| let (base_representation, _) = self.parsed.base.into_ascii()?; | ||
| let generation = UtcDateTime::now() - generation; | ||
| let generation_nanos = (UtcDateTime::now() - generation).whole_nanoseconds(); |
There was a problem hiding this comment.
I think you could just do:
| let generation_nanos = (UtcDateTime::now() - generation).whole_nanoseconds(); | |
| let generation_nanos = (UtcDateTime::now() - generation).usigned_abs() |
and realize the same gains without first converting to whole_nanoseconds and recasting to std::time::Duration using Duration::from_nanos.
SignatureTiming (the sole user of generation) is not involved in anything cryptographic. It's just a convenient metric to expose how long each constituent operation took for anyone interested. So it doesn't need a very high bar of safety to begin with.
It is possible to induce negative time in generation (UtcDateTime::now() relies on a wall clock time, rather than a monotonic clock, and someone could certainly reset the wall-clock between measurements), but, seeing as it's not load-bearing, I don't think we need to worry about it.
There was a problem hiding this comment.
Ah, thanks. Valid point. Was doing this on the train and tried to find a method that turns time::Duration into std::time::Duration but must have overlooked that one.
| .map_err(ImplementationError::FailedToVerify) | ||
| .map(|()| SignatureTiming { | ||
| generation, | ||
| verification: UtcDateTime::now() - verification, |
There was a problem hiding this comment.
As in https://github.com/cloudflare/web-bot-auth/pull/79/changes#r3118154134, you could just do
| verification: UtcDateTime::now() - verification, | |
| verification: (UtcDateTime::now() - verification).unsighed_abs(), |
and call it a day.
std::time::Duration has less overhead when using the crate, so it makes more sense to stick to Duration in public APIs. time::Duration was used because the time crate is able to run in wasm32-unknown-unknown on Cloudflare Workers. The exact reason is limited to SystemTime/Instant, used in duration_since. We still use the time crate now to work around the issue of duration_since, but merely as an implementation detail. time::Duration implements Sub<std::time::Duration>
valkum
force-pushed
the
stdtime-in-pub-api
branch
from
f41015c to
261bbab
Compare
- [Implement `std:error:Error` trait for `ImplementationError`](cloudflare#78) - [refactor: replace time::Duration with std::time::Duration in public API](cloudflare#79). The last makes a breaking change to our APIs, and so mandates a minor version bump per our existing convention.
2 participants
Closes #74
This updates the public API surfaced via
web_bot_auth::message_signatures::MessageSigner::generate_signature_headers_contentandweb_bot_auth::message_signatures::SignatureTimingto usestd::time::Durationinstead. Thetimecrate is now an implementation detail.This change makes usage with other time crates easier. You do not need to pull in the
timecrate or convert from a differentDurationintotime::Duration(direct or viastd::time::Duration)I tested this with https://github.com/cloudflare/web-bot-auth/tree/main/examples/signature-agent-card-and-registry and did not find any errors. I am looking forward to better test instructions.
This is a breaking change and requires a minor version bump.