Add AES encrypt/decrypt example#492
Conversation
|
Please note there are two comments in the code that we should probably discuss. First is exposing the AES block size from the AesKeyBits enum. The second is if we should add pkcs7 padding to the crate to prevent people from "rolling their own". |
ionut-arm
left a comment
There was a problem hiding this comment.
Thank you for the effort of putting these examples together!
| // REVIEW NOTE: Tss-esapi likely should expose these as constants from AesKeyBits::Aes128 | ||
| // to prevent ambiguity! | ||
| const AES_BLOCK_SIZE: usize = 16; |
There was a problem hiding this comment.
I agree that we need to expose this constant from somewhere in the code (and I'd welcome it as a separate commit in this PR!), but I disagree it should have anything to do with AesKeyBits because the block size for AES is not tied to key size.
Given that this would be an addition to what the spec provides, the abstraction module would probably be the best place to have it, perhaps as a method on this. But I wouldn't be opposed to also adding it as a method on SymmetricCipherParameters, for example. We just need to figure out what that method should return for Xor and Null, and document them.
See comment below.
There was a problem hiding this comment.
Wait what? I thought that block size was tied to the AESKeyBit size? IE AES-128 requires a 128bit key and a 16 byte block, where AES-256 requires a 256 bit key and a 32 byte block? I'm happy to be wrong, but all my other experience with openssl etc has the above assumptions enshrined.
There was a problem hiding this comment.
See NIST FIPS 197, paragraph three of section 1, first paragraph of section 3.1 ... Rijndael accepts different block sizes, but AES does not.
I'm not that great at digging through OpenSSL docs, the best I found was this.
There was a problem hiding this comment.
Ahhh yes, it's related to cipher selection not key. Maybe it should be on https://docs.rs/tss-esapi/latest/tss_esapi/interface_types/algorithm/enum.SymmetricMode.html or https://docs.rs/tss-esapi/latest/tss_esapi/interface_types/algorithm/enum.SymmetricAlgorithm.html 🤔
AES-GCM makes it tricky since it's block size is 1, but I think the way we have the parameters laid out we need the SymmetricMode + Key to really know what the block size is.
There was a problem hiding this comment.
I'm not actually sure if in the context of TPMs the same applies. I've skimmed a bit through the spec, couldn't find much, I'll need to test it.
| // REVIEW NOTE: Should we added PKCS7 padding as a function to MaxBuffer to prevent | ||
| // people needing to "roll their own"? |
There was a problem hiding this comment.
Yeah, agree, that would be good to have! But again, this seems like an abstraction kind of thing, perhaps it could go in the Cipher module - something like PaddedBuffer? We can then reference these new handy features in the documentation for the encryption/decryption methods, and, of course, in this example.
Actually, it strikes me that we could (maybe even should) offload these details to the cipher RustCrypto traits. For example if we allow Block, or a vector of Blocks, to be converted to MaxBuffer, then padding is free for us, and we can easily get the block size. This could be a first step towards tighter integration with those traits.
What do you think?
There was a problem hiding this comment.
Yeah, I think that's a good idea. But I suspect that is it's own PR?
Still even so, this example has highlighted a few places the api we have can be better :)
There was a problem hiding this comment.
Having that as its own PR makes sense! And we can update this example when that's pushed.
There was a problem hiding this comment.
I'm not even sure we'd need that since https://docs.rs/block-padding/0.3.2/block_padding/struct.Pkcs7.html already has .as_slice and we have https://docs.rs/tss-esapi/latest/tss_esapi/structures/struct.MaxBuffer.html#impl-TryFrom%3C%26%5Bu8%5D%3E-for-MaxBuffer
So we could just swap the padding process here.
There was a problem hiding this comment.
Actually, reading that API it seems to be really limited and complex.
We would need to take the array of bytes, then iterate over it as chunks exact with https://doc.rust-lang.org/std/slice/struct.ChunksExact.html to match the block size.
We need this because we need to check the remainder case. If the remainder is empty, we need to append another block because pkcs7 always pads to avoid ambiguity. So that means we can't just look at the vec and grow it if needed, we have to clone the full input everytime because blocks are effectively an array.
And then a vec of generic array can't do to_slice, so we have to iterate again to make a new vec that we can try_from for max_buffer.
So unless I'm really overlooking something obvious, this seems like a really rough api to want to interface with.
afdff20 to
ad678ed
Compare
|
Have you forgot to sign the commits? |
Very likely :) But it wasn't ready to merge yet either. |
ad678ed to
32a3338
Compare
|
Okay, I've rebased an updated to latest main. I think that pkcs7/padding is "out of scope" for this crate, and I'm going to make an example of exposing the TPM via rust-crypto interfaces so we can inherit their padding options. I'll hopefully have this example "converted" shortly as a POC. |
|
This has finally come up again so I should be looping back to improve this shortly :) |
|
rebase and try again? |
ce6dea3 to
dbb4fc0
Compare
|
Rebased, thanks! |
This adds an example of how to use AES-128-CBC encryption and decryption with a TPM. Signed-off-by: William Brown <william.brown@suse.com>
dbb4fc0 to
a986b26
Compare
wiktor-k
left a comment
There was a problem hiding this comment.
This is a well-done example 👌
I'm wondering though whether the RustCrypto trait implementation should be moved to the main crate. It seems quite useful and generic so instead of folks copying that part of the code they could use our rustcrypto feature and get these traits...
Pinging our RustCrypto guru @baloo 🐻
| .load(primary.key_handle, enc_private.clone(), public.clone()) | ||
| .unwrap(); | ||
|
|
||
| let decrypt = true; |
There was a problem hiding this comment.
I'm very happy that you've made this explicit and I'm wondering how whether we should be providing encrypt_2 and decrypt_2 as two separate functions just for the sake of users 😅
That's of course a totally different subject, nothing to do here, carry on! 🚀
| }) | ||
| .unwrap(); | ||
|
|
||
| /* |
There was a problem hiding this comment.
I think this entire section can be removed 🤔
| &mut self, | ||
| f: impl cipher::BlockClosure<BlockSize = Self::BlockSize>, | ||
| ) { | ||
| // No errors? I'll raise an issue. |
There was a problem hiding this comment.
I have the same issue with RustCrypto traits and some of them started getting error capabilities 😅 Is there a backing ticket already?
There was a problem hiding this comment.
I don't know if those traits are really meant for being used with a secret abstracted away. They are designed with local computation where you have access to the cipher state.
| panic!("Should not be empty"); | ||
| } | ||
|
|
||
| const AES_BLOCK_SIZE: usize = 16; |
There was a problem hiding this comment.
| const AES_BLOCK_SIZE: usize = 16; | |
| const AES_BLOCK_SIZE: usize = aes::Aes128::BlockSize::USIZE; |
https://docs.rs/aes/latest/aes/struct.Aes128.html#associatedtype.BlockSize
Will need use cipher::BlockSizeUser and cipher::Unsigned if not already in scope.
(absolute nitpick)
Sure, but make a new PR. |
This adds an example of how to use AES-128-CBC encryption and decryption with a TPM.