Add AES CTR, CBC, GCM #35

jonas-lj · 2022-09-09T11:46:23Z

Added support for AES with 128 and 256 bit keys using either CTR, CBC, GCM with PKCS#7 padding. All modes are backed by crates from RustCrypto (https://github.com/RustCrypto/block-modes).

kchalkias · 2022-09-09T17:34:13Z

src/tests/aes_tests.rs

+};
+use rand::{rngs::StdRng, SeedableRng};
+
+#[test]


can we also add wycheproof tests for completeness?

There doesn't seem to be wycheproof tests for any of the modes I've added so far. There's only tests for authenticated encryption, but I think it would be a good idea to add those along with some wycheproof tests.

kchalkias

Another general comment is if we did some digging and comparison of the various available AES implementations, as one of the things we want to advertise is faster crypto. The goal of this repo is to pick the fastest possible (but safe) Rust impl and wrap or augment them.

jonas-lj · 2022-09-09T20:10:08Z

I did some research on it, and there doesn't seem to be a lot of options. The RustCrypto modes seem to be by far the most used and best supported. However, I did look at https://github.com/keepsimple1/libaes which has a benchmark showing that it's about 3-4 times faster than RustCrypto for CBC. I tried it with the most recent version of RustCrypto, and the benchmark shows the same, so we could use this lib at least for the modes it supports.

kchalkias · 2022-09-10T23:02:43Z

src/traits.rs

+///
+pub trait Cipher {
+    /// Encrypt `plaintext` and write result to `buffer`
+    fn encrypt_b2b(&self, plaintext: &[u8], buffer: &mut [u8]) -> Result<(), Error>;


Wondering if we should rename these to just encrypt and decrypt

kchalkias · 2022-09-10T23:06:20Z

Cargo.toml

+aes = "0.8.1"
+ctr = "0.9.1"
+cbc = "0.1.2"
+cfb-mode = "0.8.1"


I think we could retire cfb for now, and replace it with GCM which is the most popular mode for authenticated encryption (and for which there is a wycheproof test).
All in all, we can start with CTR, CBC, GCM for which we know of Sui applications already

kchalkias · 2022-09-12T16:43:15Z

src/aes.rs

+/// Aes128 in GCM mode (Authenticated)
+///
+pub struct Aes128Gcm<'a, NonceSize: ArrayLength<u8>> {
+    pub iv: &'a GenericArray<u8, NonceSize>,


qq: how about having a similar iv field type (bytearray), to have consistency with the other schemes?

kchalkias · 2022-09-12T16:44:08Z

src/tests/aes_tests.rs

+}
+
+#[test]
+fn wycheproof_test() {


kchalkias · 2022-09-13T11:44:33Z

src/traits.rs

+///
+pub trait Cipher {
+    /// Encrypt `plaintext` and write result to `buffer` using the given IV
+    fn encrypt(&self, iv: &[u8], plaintext: &[u8], buffer: &mut [u8]) -> Result<(), Error>;


Wondering if we should hide this from the end-user buffer: &mut [u8]), by experiencer it's very common to make mistakes here or be inconvenient, ie by providing wrong size input buffers, even the fact to be prepared to have an existing buffer already. Wdyt?

That makes sense. I just copied the interface from rustcrypto, assuming that was the common way to do it in rust. I think we should return a new array with the result then.

kchalkias · 2022-09-13T11:51:05Z

src/aes.rs

+{
+    fn encrypt(
+        &self,
+        iv: &[u8],


A few questions:

do we already check iv size mismatch?

we handle the AES Key as a special struct, what about the IV? maybe for consistency we should as well. Not strongly opinionated on that.

For all of the encrypt/decrypt and their authenticated variants, let's have some tests on input [u8] sizes mismatch.

kchalkias · 2022-09-13T11:52:53Z

src/aes.rs

+///
+#[serde_as]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct Key<const N: usize> {


Similarly to what we do for other schemes (i.e., for ed25519), let's rename this to AesKey to avoid any future readability confusion against future apis ie HMAC keys etc.

kchalkias · 2022-09-13T11:57:29Z

src/aes.rs

+#[serde_as]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct Key<const N: usize> {
+    // This is needed to derive Serialize and Deserialize since serde does not support derive for arrays


cc: @huitseeker to sign off the idiomatic way

Yep, this is good.

huitseeker

Just food for thought: there may be a way to express a size floor on Key<N> by requiring a MinSlice<_; 16> or some such appropriate projection to a slice of a minimum length (ref)
In the old days, before const generics, we used to do it with something like https://crates.io/crates/generic-bytes

huitseeker · 2022-09-13T14:31:08Z

src/aes.rs

+#[serde_as]
+#[derive(Debug, Clone, Serialize, Deserialize)]
+pub struct Key<const N: usize> {
+    // This is needed to derive Serialize and Deserialize since serde does not support derive for arrays


Yep, this is good.

kchalkias

I guess the only remaining feature is IV to be in its own struct, right?

…r ciphers

kchalkias

Awesome! LGTM

kchalkias

Thanks for merging! We generally prefer to squash and merge to avoid inflating our commit log and have all main commits in a stable state just in case we ever need to cherry pick, go back to specific commits etc.

jonas-lj · 2022-09-16T11:35:58Z

Thanks for merging! We generally prefer to squash and merge to avoid inflating our commit log and have all main commits in a stable state just in case we ever need to cherry pick, go back to specific commits etc.

That makes sense. I guess it doesn't make sense to revert the merge, because the commits will still be in the log, but I'll make sure to do that next time.

kchalkias

Yes, we're good with this PR merge, no need to revert.

Jonas Lindstrøm added 7 commits September 8, 2022 14:08

AES128 in counter mode added

9539b6f

Implement serialize and deserialize for aes keys

b4b0da6

Add CBC with Pkcs7 padding

4454f7a

Add doc and a negative test

0ddcb44

Add CFB mode and refactor tests

e98c01a

Implement as_ref for Key

e06fe9b

Align interfaces

1ba04f6

jonas-lj requested review from huitseeker, kchalkias, joyqvq and punwai as code owners September 9, 2022 11:46

Jonas Lindstrøm added 5 commits September 9, 2022 13:52

Remove unnecessary into's

4a454ee

Format code

30fec8d

More unnecessary into's

8884947

copy_key feature shouldn't be set

7f39d4f

Add cargo.lock

2513102

kchalkias reviewed Sep 9, 2022

View reviewed changes

kchalkias reviewed Sep 10, 2022

View reviewed changes

Jonas Lindstrøm and others added 5 commits September 12, 2022 10:40

Add AES-GCM

353a291

Add wycheproof tests for AES-GCM

4b57416

Merge branch 'main' into aes

cacc79d

Clean up

a5cd518

Merge branch 'aes' of github.com:MystenLabs/fastcrypto into aes

2f80dbe

kchalkias reviewed Sep 12, 2022

View reviewed changes

jonas-lj marked this pull request as draft September 12, 2022 21:21

Align interfaces

c71a466

kchalkias reviewed Sep 13, 2022

View reviewed changes

kchalkias changed the title ~~Add AES~~ Add AES CTR, CBC, GCM Sep 13, 2022

kchalkias reviewed Sep 13, 2022

View reviewed changes

huitseeker reviewed Sep 13, 2022

View reviewed changes

Jonas Lindstrøm added 2 commits September 14, 2022 13:33

Return array instead of using a buffer

e338f15

Add AES192 in CRT mode

7370f1f

kchalkias reviewed Sep 14, 2022

View reviewed changes

Jonas Lindstrøm and others added 10 commits September 15, 2022 08:56

IV's defined in struct

a5047e6

Impl serialize/deserialize for nonces

bb002d2

Remove unused lifetime

e910e35

Merge branch 'main' into aes

e34ffdc

Reorder dependencies and fmt

f263581

Use typenum constants for both keys and nonces

81dcf15

Add serialization tests and clean up

9b9cbe3

Doc

de89e91

Add doc to public structs and traits and rename generic nonce type fo…

b5d9f13

…r ciphers

Format

a418b5e

jonas-lj marked this pull request as ready for review September 15, 2022 09:48

kchalkias approved these changes Sep 15, 2022

View reviewed changes

jonas-lj merged commit 41f5010 into main Sep 16, 2022

jonas-lj deleted the aes branch September 16, 2022 07:19

kchalkias reviewed Sep 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AES CTR, CBC, GCM #35

Add AES CTR, CBC, GCM #35

jonas-lj commented Sep 9, 2022 •

edited by kchalkias

kchalkias Sep 9, 2022

jonas-lj Sep 9, 2022

kchalkias left a comment •

edited

jonas-lj commented Sep 9, 2022

kchalkias Sep 10, 2022

kchalkias Sep 10, 2022

kchalkias Sep 12, 2022

kchalkias Sep 12, 2022

kchalkias Sep 13, 2022

jonas-lj Sep 13, 2022

kchalkias Sep 13, 2022

kchalkias Sep 13, 2022

kchalkias Sep 13, 2022

huitseeker Sep 13, 2022

huitseeker left a comment

huitseeker Sep 13, 2022

kchalkias left a comment

kchalkias left a comment

kchalkias left a comment

jonas-lj commented Sep 16, 2022

kchalkias left a comment

Add AES CTR, CBC, GCM #35

Add AES CTR, CBC, GCM #35

Conversation

jonas-lj commented Sep 9, 2022 • edited by kchalkias

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchalkias left a comment • edited

Choose a reason for hiding this comment

jonas-lj commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

kchalkias left a comment

Choose a reason for hiding this comment

jonas-lj commented Sep 16, 2022

kchalkias left a comment

Choose a reason for hiding this comment

jonas-lj commented Sep 9, 2022 •

edited by kchalkias

kchalkias left a comment •

edited