Clean up Hacl-rs integration and add Poly1305 and ChaCha20Poly1305 #703
Conversation
- drop portable_hacl feature - don't export hacl modules when hacl feature is set - (on some) add an expose-hacl feature to export hacl module
This commit adds inlines: - plain inline inside hacl-rs code (sometimes) - inline(always) for functions that call hacl-rs code This way our wrapper functions get inlined away and the caller just calls the hacl-rs function, and inside the hacl-rs function there are few calls to internal functions (where it makes sense).
Sounds good to me. |
There was a problem hiding this comment.
Generally lgtm with a couple nits.
The main question I have is around when hacl internals are exposed. The basic design should be that only a libcrux API is exposed (backed by some implementation), and we only expose something backend (hacl) specific if that's required by another backend (hacl) implementation, guarded by some fatures. You started doing that. But there still seem to be a bunch of exports of hacl specific modules. Or maybe it's just the naming that needs to change.
| @@ -6,8 +6,8 @@ pub use libcrux_hacl_rs::curve25519_51 as hacl; | |||
| #[cfg(feature = "hacl")] | |||
| mod impl_hacl; | |||
|
|
|||
| #[cfg(feature = "portable_hacl")] | |||
| pub use impl_hacl::HaclCurve25519 as Impl; | |||
| #[cfg(feature = "hacl")] | |||
There was a problem hiding this comment.
Do we need this in any other libcrux crate? If yes, we should start using a different feature for it. If not, let's not expose it at all.
| @@ -23,21 +23,11 @@ pub struct Error; | |||
|
|
|||
| /// This trait is implemented by the backing implementations. | |||
| /// Only used for implementation agility. | |||
| trait Curve25519 { | |||
| pub trait Curve25519 { | |||
There was a problem hiding this comment.
Do we actually need this public? If I recall correctly, this was only used internally to ensure different implementations have the same interface.
| @@ -11,5 +11,5 @@ pub mod hacl { | |||
| #[cfg(feature = "hacl")] | |||
| mod impl_hacl; | |||
|
|
|||
| #[cfg(feature = "portable_hacl")] | |||
| #[cfg(feature = "hacl")] | |||
There was a problem hiding this comment.
Same comment about features and exposing as for x25519.
| pub fn store32_le(bytes: &mut [u8], x: u32) { | ||
| bytes[0..4].copy_from_slice(&u32::to_le_bytes(x)) | ||
| } | ||
|
|
||
| #[inline] |
There was a problem hiding this comment.
Did you check if this makes a difference for performance?
| @@ -13,6 +13,7 @@ pub mod hacl; | |||
| #[cfg(feature = "hacl")] | |||
| mod impl_hacl; | |||
|
|
|||
| #[cfg(feature = "hacl")] | |||
There was a problem hiding this comment.
Same comment on exposing and features.
| use libcrux_macros as krml; | ||
|
|
||
| #[inline] | ||
| fn double_round_32(st: &mut [u32]) { |
There was a problem hiding this comment.
All these functions in here are unused according to the compiler.
| #[cfg(feature = "hacl")] | ||
| mod impl_hacl; | ||
|
|
||
| #[cfg(feature = "hacl")] |
There was a problem hiding this comment.
Same comment on features and exposing hacl internals.
|
|
||
| // ensure destination slice has just the right length | ||
| let ctxt_len = ptxt.len() + TAG_LEN; | ||
| let ctxtu32 = &mut ctxt[..ctxt_len]; |
| @@ -0,0 +1,112 @@ | |||
| use crate::{AeadError, MacError, KEY_LEN, NONCE_LEN, TAG_LEN}; | |||
There was a problem hiding this comment.
Unused import MacError.
| #[cfg(feature = "hacl")] | ||
| mod impl_hacl; | ||
|
|
||
| #[cfg(feature = "hacl")] |
There was a problem hiding this comment.
Same comment on features and exposing.
This PR addresses the following cleanup issues:
It also adds Poly1305 (MAC) and ChaChaPoly1305 (AEAD), which means we also make progress on this issue:
I am currently stuck on implementing the existing AEAD API with the new code, because that uses a single
ctxt_ptxtfunction argument to encrypt and does in-place encryption. The API exposed by hacl-rs uses separate arguments, and we can't put the argument in both because that would result in a shared mutable reference. I suggest we leave the old API be for now and address this once we have a better idea of what the final API should be.Opinions @franziskuskiefer @jschneider-bensch?
TODOs left:
Fixes #696
Fixes #673
Fixes #675