*ring* crypto backend (and other feedback)
djc opened this issue · 2 comments
Hi, this crate is looking great, thanks for open sourcing! I was wondering, would you be open to maintaining alternative implementations for the required crypto primitives from ring, using an opt-in Cargo feature? In projects where rustls is already required, this saves on dependencies; it also looks like ed25519-dalek pulls in an obsolete version of rand.
Other minor points of feedback in random order:
- I noticed that there are no rustfmt or clippy checks in the CI config even though the project is fmt/clippy clean, probably good to add?
- Also acronyms in type names are not matching the recommended spelling from RFC 430 -- I can send a PR for this if you like. A potential sticking point is what
ARC
should be called, maybeArc
would be too confusing? I would suggestArChain
plus some docs, maybe? is_within_pct()
could probably use some comments on what it's trying to achieve and how.- In the DKIM crate I was working on that I'm considering replacing with this, I was writing out signatures etc into a
fmt::Write
instead ofio::Write
, which preserves the notion that the outcome is valid UTF-8. Maybe good to do that here, too? Also I would recommend not importingstd::io::Write
directly to avoid confusion between the two, I usually importstd::fmt
/std::io
and then useio::Write
/fmt::Write
. - The
fmt::Display
impl forError
is inconsistent about using.
at the end of strings -- I usually prefer to avoid them since it's easier/faster to add them later in surrounding context rather than the other way around.
I can file separate issues for these if you prefer.
Hi Dirkjan,
Thanks for the feedback, I have just fixed most of the issues you listed in the latest commit. My comments follow,
I was wondering, would you be open to maintaining alternative implementations for the required crypto primitives from ring, using an opt-in Cargo feature? In projects where rustls is already required, this saves on dependencies; it also looks like ed25519-dalek pulls in an obsolete version of rand.
Sure, if you send a PR for this I'll gladly merge it.
I noticed that there are no rustfmt or clippy checks in the CI config even though the project is fmt/clippy clean, probably good to add?
Fixed.
Also acronyms in type names are not matching the recommended spelling from RFC 430 -- I can send a PR for this if you like. A potential sticking point is what ARC should be called, maybe Arc would be too confusing? I would suggest ArChain plus some docs, maybe?
Fixed. I renamed ARC
to ArcSet
since it represents a set of three Arc headers.
is_within_pct() could probably use some comments on what it's trying to achieve and how.
Fixed. I added a brief explanation to this function as well as to verify_*
. I also need to add comments everywhere in the code but I'll do that once I finish the core components of the mail server.
I was writing out signatures etc into a fmt::Write instead of io::Write, which preserves the notion that the outcome is valid UTF-8. Maybe good to do that here, too?
In this case it won't be possible to use fmt::Write
because the Signature::write
function also needs to be able to write to the Sha256/Sha1 hashers (see here).
In addition to that I used io::Write
in order to be able sign e-mail messages that use other encodings (but I admit this is rare nowadays) or non-UTF8 MIME parts. Even though the DKIM signature will always be ASCII, the writer needs to be able to support any non-UTF8 content that might follow the signature.
Also I would recommend not importing std::io::Write directly to avoid confusion between the two, I usually import std::fmt/std::io and then use io::Write/fmt::Write.
Fixed.
The fmt::Display impl for Error is inconsistent about using . at the end of strings -- I usually prefer to avoid them since it's easier/faster to add them later in surrounding context rather than the other way around.
Fixed.
Great stuff! I'll work on the ring backend in the near future. I briefly looked at the fmt::Write
vs io::Write
thing you looked to but haven't quite figured out, will look into it in more detail soon.