-
Notifications
You must be signed in to change notification settings - Fork 226
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
Support 192 and 256-bit keys for AES #1316
Conversation
Not a huge fan of just panicking on invalid key length, as there's no way for an application to recover from that. I guess without @bjoernQ what do you think? |
Not exactly nice, true. Fallible would be better or maybe we define an Enum for the keysizes, like pub enum Key {
Key16([u8;16),
#[cfg(any(feature = "esp32", feature = "esp32s2")]
Key24([u8;24),
Key32([u8;32),
} Also don't we define cfg-symbols for the chip? Like |
Thanks for the input @bjoernQ. Not quite sure what is best right now, I think making it fallible or the key size enum are both probably fine solutions. So, no strong opinions now, will sleep on it. |
Can we decide on a path forward here? |
I've suggested a solution, so has Bjoern. I'm not going to be too picky about what we decide on as long as it doesn't just panic. If nobody else has any input then we should probably just make it fallible and we can revisit it at some other time. |
I'm fine with making it fallible for now. I think the Key enum would be a better solution if we don't end up in having a hell lot of key lengths. But for the sake to get this forward - let's make it fallible |
I think, as |
Sure, that's fine too. A fallible API is probably simpler, though. |
Let's go with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - the key-enum is really the nicer solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for making those changes, and thanks for sticking it out through all these review comments!
Review comments have been addressed
closes #1234