Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Upgrade signatory -> 0.15; yubihsm -> 0.28; use chacha20poly1305 crate #366

Merged
merged 1 commit into from
Oct 21, 2019

Conversation

tarcieri
Copy link
Contributor

This commit updates signatory and yubihsm to the latest versions.

This requires either upgrading ring to 0.16, or replacing it:

informalsystems/tendermint-rs#11

This commit has opted to replace it with the (i.e. my) chacha20poly1305 crate:

https://github.com/RustCrypto/AEADs/tree/master/chacha20poly1305

While ring is a great crate with excellent cryptographic implementations, the inability to link two versions of ring-asm is a major impediment to doing upgrades, as it requires updating all ring-dependent dependencies at once, or not at all, which so far has been a major blocker and has been stalling KMS development for awhile. As we're only using it for ChaCha20Poly1305, it seems like a rather heavyweight dependency just to serve that one purpose.

This commit updates `signatory` and `yubihsm` to the latest versions.

This requires either upgrading *ring* to 0.16, or replacing it:

informalsystems/tendermint-rs#11

This commit has opted to replace it with the (i.e. my)
`chacha20poly1305` crate:

https://github.com/RustCrypto/AEADs/tree/master/chacha20poly1305

While *ring* is a great crate with excellent cryptographic
implementations, the inability to link two versions of `ring-asm` is a
major impediment to doing upgrades, as it requires updating all
*ring*-dependent dependencies at once, or not at all, which so far has
been a major blocker and has been stalling KMS development for awhile.
As we're only using it for ChaCha20Poly1305, it seems like a rather
heavyweight dependency just to serve that one purpose.
@tarcieri
Copy link
Contributor Author

A quick note on performance: the chacha20poly1305 crate is much slower (over 4X) than ring. However, there's upstream work on the chacha20 crate to add SSE2 as well as potentially AVX2 acceleration, which should bring the performance more in line with ring's implementation:

RustCrypto/stream-ciphers#61

That said, we're nowhere close to performance bound on symmetric encryption speed, so I don't think it really matters.

PrivateKey::Ed25519(pk) => pk.to_seed(),
PrivateKey::Ed25519(pk) => {
// TODO(tarcieri): upgrade Signatory version
ed25519::Seed::from_bytes(pk.to_seed().as_secret_slice()).unwrap()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since tendermint-rs v0.10 is still linking signatory v0.12, it necessitates these gross thunks.

However, I think it should be pretty easy to update it and cut another release now that the ring and Secret Connection code will be out-of-the-way.

.map_err(|_| ErrorKind::Crypto)?
.len()
} else {
let mut in_out = ciphertext.to_vec();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liamsi I was a bit curious what case you're trying to handle here. It seems like it only helps in the event that the out buffer is up to the tag length smaller than the input ciphertext, but any shorter than that and the code below will still panic when it slices out out-of-bounds

@tarcieri tarcieri merged commit 443d19e into master Oct 21, 2019
@tarcieri tarcieri deleted the signatory-0-15-yubihsm-0-28-chacha20poly1305-crate branch October 21, 2019 18:03
@tarcieri tarcieri mentioned this pull request Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants