*ring* crypto backend (and other feedback)

[djc]

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, maybe Arc would be too confusing? I would suggest ArChain 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 of io::Write, which preserves the notion that the outcome is valid UTF-8. Maybe good to do that here, too? 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.
• 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.

I can file separate issues for these if you prefer.

[mdecimus]

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.

[djc]

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.