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

Add poly1305 crate (derived from rust-crypto) #12

Merged
merged 6 commits into from
Aug 15, 2019
Merged

Add poly1305 crate (derived from rust-crypto) #12

merged 6 commits into from
Aug 15, 2019

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Aug 13, 2019

This PR adds an initial poly1305 crate based on the rust-crypto implementation of Poly1305 (which in turn is a translation of poly1305-donna):

https://github.com/DaGenix/rust-crypto/blob/9a4d8f279b643b8ae006e442141571fc7a85a3cf/src/poly1305.rs

I've done the minimal work to adapt it to crypto_mac::Mac and generally fact it more like the rest of RustCrypto/MACs.

Performance is good enough for my purposes, but could be better w\ SIMD acceleration. Benchmark using the crypto-mac crate's bench! macro on a 3.1 GHz Intel Core i7:

running 4 tests
test bench1_10    ... bench:          21 ns/iter (+/- 1) = 476 MB/s
test bench2_100   ... bench:         100 ns/iter (+/- 21) = 1000 MB/s
test bench3_1000  ... bench:         900 ns/iter (+/- 250) = 1111 MB/s
test bench3_10000 ... bench:       8,937 ns/iter (+/- 1,075) = 1118 MB/s

I will throw the code into Godbolt so we can do some inspection of whether or not the implementation appears to be constant-time on select platforms.

This is a verbatim import of rust-crypto's poly1305.rs:

https://github.com/DaGenix/rust-crypto/blob/9a4d8f279b643b8ae006e442141571fc7a85a3cf/src/poly1305.rs

Last commit by Palmer Cox on October 17th, 2015. Commit message:

> Resolve warnings from Rust nightly about unecessary parens in for statements
Runs rustfmt on the original rust-crypto sources
This adds basic crate boilerplate similar to the other RustCrypto/MAC
crates, and also adapts the source code to use the `crypto_mac::Mac`
trait rather than the `rust-crypto` one.

Additionally, it moves the existing Poly1305 tests into the `tests/`
directory, and uses the standard RustCrypto/MACs `bench!` mcaro.

Tests now pass and performance looks reasonable.
Nothing too interesting here, however it seems the original
`rust-crypto` code had a lot of (apparently) unnecessary masking which I
assume it inherited from the original C code.
@tarcieri tarcieri requested a review from newpavlov August 13, 2019 21:17
@tarcieri
Copy link
Member Author

tarcieri commented Aug 13, 2019

@newpavlov well I went ahead and did the thing we talked about not doing originally for now 😅 i.e. impl'd crypto_mac::Mac for the Poly1305 struct directly.

That's mostly to make it a straightforward translation of the original rust-crypto code, but I think it's worth considering whether it's reasonable or safe to treat Poly1305 as a crypto_mac::Mac, or if we need a different-but-similarly-shaped trait for universal hashing functions (from which MACs can be based, when e.g. combined with a cipher).

Perhaps for now it'd be better if the crate had a feature to do Poly1305-AES with the crypto_mac::Mac API, and otherwise had a crypto_mac::Mac-shaped API defined as simply impl Poly1305?

My main interest here is ChaCha20Poly1305 AEAD, so I don't necessarily need Poly1305-AES, but depending on how we handle generic composition it feels like we need some traits to assemble AEADs which use universal hash functions as MACs. This is something we'll have to solve for AES-GCM(-SIV) as well.

@tarcieri
Copy link
Member Author

Here's the code in godbolt. I didn't see anything obviously non-constant time in a quick pass through the generated assembly with -Copt-level=3, but will scrutinize it more:

https://godbolt.org/z/T-7EP_

@tarcieri
Copy link
Member Author

@newpavlov well, I've fixed all of my own nits with this and would say it's ready for review.

I ended up removing the crypto_mac::Mac impl for now. If someone actually desires Poly1305-AES (which seems unlikely, and should probably be gated under a cargo feature), it seems appropriate for that, but not for Poly1305 itself as a one-time authenticator.

As a universal hash function and one-time authenticator, Poly1305
doesn't meet normal expectations of a MAC.

This change removes the `Mac` impl, but otherwise leaves the API looking
very much like a `Mac`.

Additionally, this commit includes a lot of refactoring in an attempt to
clean up the code somewhat.
@tarcieri
Copy link
Member Author

Updated godbolt here: https://godbolt.org/z/V9zoPr

In either case it won't be necessary on Rust 1.32+ as we can use
`u32::from_le_bytes`/`to_le_bytes`, but for now `byteorder` is the more
common solution.
@tarcieri tarcieri merged commit 3893ec1 into master Aug 15, 2019
@tarcieri tarcieri deleted the poly1305 branch August 15, 2019 08:51
@tarcieri
Copy link
Member Author

@newpavlov can you give me access to the poly1305 crate to cut a release of this? (unless you think it needs more work)

@newpavlov
Copy link
Member

@tarcieri
Done! I haven't reviewed code in depth yet, but feel free to publish without waiting on me!

@tarcieri
Copy link
Member Author

@newpavlov thanks! I'd like to circle back at some point and add SIMD support, but this seems like a start

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 this pull request may close these issues.

2 participants