Skip to content
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

Rust docs noise_sv2 #1111

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
46 changes: 46 additions & 0 deletions protocols/v2/noise-sv2/src/aed_cipher.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need doc cmts on:

  • impl AeadCipher for ChaCha20Poly1305
  • impl AeadCipher for Aes256Gcm

Copy link
Collaborator

Choose a reason for hiding this comment

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

here @Fi3 said #1111 (comment)

I usually do not documents methods on trait impl but only one trait definition

I did some research and indeed, when rendered on the browser, Rust Docs trickle down the doc comments from the trait definition to trait implementations.

But if we're aiming for completeness, it would be good to have them nevertheless.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? If you want to go against best practice you should have a good reasons. The reason here is that if you have 2 trait impl that do very different things you should not have the trait at all. If you really need the trait and you still need to documents a specific thing of a particular implementation (not our case imo) is better to just document it in the trait definition, that at the end is what the user will most likely see

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I added a test doc comments left right above the impl AeadCipher for ChaCha20Poly1305. It renders in the UI like:

Screenshot 2024-09-12 at 10 31 40 AM

I disagree with @Fi3 that we should never have doc comments on specific implementations, however we should be judicious to make sure that when we add them it is not redundant.

I think in the case, it is obvious what the implementations are doing here. Adding doc comments that say /// AEAD implementation for ChaCha20Poly1305. is redundant in this case.

So, while I take back my initial suggestions of adding documentation here, I do think there may be exceptions of adding doc comments to on other implementations in the future. Perhaps more so in the roles crates to give clarity on why specific traits are implemented on specific structs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

the fact is that if we need to add comment is very likley that using a trait is not the right thing to do. Ofc if we have to use a trait and we need also a comment for a very particular case we will add it

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree. The documentation should belong to the trait itself, not the implementation. Again reiterating, that there may be exceptions to this rule, however this is not an exception case.

Original file line number Diff line number Diff line change
@@ -1,16 +1,60 @@
//! # AEAD Cipher Trait
//!
//! Defines the [`AeadCipher`] trait which standardizes the interface for Authenticated Encryption
//! with Associated Data (AEAD) ciphers used within the Noise protocol implementation. The trait is
//! implemented for two common cipher, [`ChaCha20Poly1305`] and [`Aes256Gcm`], providing encryption
//! and decryption functionality with authenticated data.
//!
//! ## Overview
//!
//! AEAD ciphers provide both confidentiality and integrity by encrypting data and creating an
//! authentication tag in a single step. The integrity of additional associated data (AAD) can also
//! be verified without including it in the encrypted output.
//!
//! ## Usage
//!
//! The [`AeadCipher`] trait is used by the [`crate::handshake::HandshakeOp`] trait to perform
//! cryptographic operations during the Noise protocol handshake, ensuring secure communication
//! between parties.

use aes_gcm::Aes256Gcm;
use chacha20poly1305::{aead::Buffer, AeadInPlace, ChaCha20Poly1305, ChaChaPoly1305, KeyInit};

/// Defines the interface for AEAD ciphers.
///
/// The [`AeadCipher`] trait provides a standard interface for initializing AEAD ciphers, and for
/// performing encryption and decryption operations with additional AAD. This trait is implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

What is AAD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Authenticated Associated data

/// by either the [`ChaCha20Poly1305`] or [`Aes256Gcm`] specific cipher types, allowing them to be
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// by either the [`ChaCha20Poly1305`] or [`Aes256Gcm`] specific cipher types, allowing them to be
/// by both the ChaCha20Poly1305 and Aes256Gcm, allowing them to be

I think both of them implement the trait right? also I dont think we should link to crates outside of our project, unless maybe if its top of the file documentation or something else that is more general

/// used interchangeably in cryptographic protocols. It is utilized by the
/// [`crate::handshake::HandshakeOp`] trait to secure the handshake process.
///
/// The `T: Buffer` represents the data buffer to be encrypted or decrypted. The buffer must
Copy link
Contributor

Choose a reason for hiding this comment

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

This is decrypt/encrypt functions specific and not in trait level, so maybe it should be moved(duplicated) above each function

/// implement the [`Buffer`] trait, which provides necessary operations for in-place encryption and
/// decryption.
pub trait AeadCipher {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Fi3 whats the goal behind this trait and then implementing it for ChaChaPoly1305 if the ChaCha.. already have some AEAD traits implemented for it https://docs.rs/chacha20poly1305/latest/src/chacha20poly1305/lib.rs.html#241 which is used in our implementation without any changes https://github.com/stratum-mining/stratum/pull/1111/files#diff-af2c2f10de68656c4249abc945c50c903ccfcd35bf169ce3a4779f1c4e5245e1L33?

/// Creates a new instance of the cipher from a 32-byte key.
///
/// Initializes the AEAD cipher with the provided key (`k`), preparing it for
/// encryption and decryption operations.
fn from_key(k: [u8; 32]) -> Self;

/// Encrypts the data in place using the provided 12-byte `nonce` and AAD (`ad`).
///
/// Performs authenticated encryption on the provided mutable data buffer (`data`), modifying
/// it in place to contain the ciphertext. The encryption is performed using the provided nonce
/// and AAD, which ensures that the data has not been tampered with during transit.
fn encrypt<T: Buffer>(
&mut self,
nonce: &[u8; 12],
ad: &[u8],
data: &mut T,
) -> Result<(), aes_gcm::Error>;

/// Decrypts the data in place using the provided 12-byte nonce (`n`) and AAD (`ad`).
///
/// Performs authenticated decryption on the provided mutable data buffer, modifying it in
/// place to contain the plaintext. The decryption is performed using the provided nonce and
/// AAD, ensuring that the data has not been tampered with during transit.
fn decrypt<T: Buffer>(
&mut self,
nonce: &[u8; 12],
Expand Down Expand Up @@ -47,6 +91,7 @@ impl AeadCipher for Aes256Gcm {
fn from_key(k: [u8; 32]) -> Self {
Aes256Gcm::new(&k.into())
}

fn encrypt<T: Buffer>(
&mut self,
nonce: &[u8; 12],
Expand All @@ -55,6 +100,7 @@ impl AeadCipher for Aes256Gcm {
) -> Result<(), aes_gcm::Error> {
self.encrypt_in_place(nonce.into(), ad, data)
}

fn decrypt<T: Buffer>(
&mut self,
nonce: &[u8; 12],
Expand Down
113 changes: 112 additions & 1 deletion protocols/v2/noise-sv2/src/cipher_state.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested rust docs at top of file:

//! # AEAD Cipher Management
//!
//! The `CipherState` trait manages the state and operations of Authenticated Encryption with
//! Associated Data (AEAD) ciphers within cryptographic protocols.
//!
//! ## Overview
//!
//! Details of key management, nonce generation, and encryption/decryption are abstracted away,
//! ensuring the underlying cryptographic details are consistently handled across different cipher
//! implementations.
//!
//! The module also includes the `GenericCipher` enum, which allows for the use of different AEAD
//! cipher implementations in a generic manner.
//!
//! ## Usage
//!
//! The `CipherState` trait is used by the `HandshakeOp` trait to handle the stateful encryption
//! and decryption tasks required during the Noise protocol handshake. By implementing
//! `CipherState`, handshake operations securely manage cryptographic material and perform
//! necessary transformations on messages exchanged between the initiator and responder.
//!
//! The `Initiator` and `Responder` structs use `GenericCipher` instances (`c1` and `c2`) to
//! perform symmetric encryption and decryption once the Noise handshake is complete. These
//! ciphers, initialized and managed through the `CipherState` trait, ensure that ongoing
//! communication remains confidential and authenticated.
//!
//! ### Example
//!
//! In this example, the `CipherState` trait and the `GenericCipher` enum are used to perform
//! encryption and decryption operations. The example shows how to initialize a cipher, encrypt a
//! message, and then decrypt it back to the original plaintext.
//!
//! ```rust
//! use chacha20poly1305::aead::{AeadInPlace, Buffer, generic_array::GenericArray};
//! use chacha20poly1305::ChaCha20Poly1305;
//! use aes_gcm::Aes256Gcm;
//! use cipher_state::{Cipher, CipherState, GenericCipher};
//!
//! fn main() -> Result<(), aes_gcm::Error> {
//!     // Example key and plaintext message (normally generated securely)
//!     let key = [0u8; 32]; // A 32-byte key
//!     let mut plaintext = b"secret message".to_vec();
//!
//!     // Initialize a ChaCha20Poly1305 cipher with the key
//!     let cipher = ChaCha20Poly1305::new(&GenericArray::clone_from_slice(&key));
//!
//!     // Create a CipherState instance using the key and cipher
//!     let mut cipher_state = Cipher::from_key_and_cipher(key, cipher);
//!
//!     // Encrypt the plaintext in place, modifying it to become the ciphertext
//!     cipher_state.encrypt_with_ad(b"", &mut plaintext)?;
//!     println!("Encrypted ciphertext: {:?}", plaintext);
//!
//!     // Decrypt the ciphertext back to the original plaintext
//!     cipher_state.decrypt_with_ad(b"", &mut plaintext)?;
//!     println!("Decrypted message: {:?}", String::from_utf8(plaintext).unwrap());
//!
//!     // Use the GenericCipher enum to perform similar operations
//!     let mut generic_cipher = GenericCipher::ChaCha20Poly1305(cipher_state);
//!
//!     // Encrypt a new message using the GenericCipher
//!     let mut new_plaintext = b"another secret message".to_vec();
//!     generic_cipher.encrypt(&mut new_plaintext)?;
//!     println!("GenericCipher encrypted ciphertext: {:?}", new_plaintext);
//!
//!     // Decrypt the message back to the original plaintext
//!     generic_cipher.decrypt(&mut new_plaintext)?;
//!     println!("GenericCipher decrypted message: {:?}", String::from_utf8(new_plaintext).unwrap());
//!
//!     Ok(())
//! }
//! ```

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Fi3 I cannot find any instance of the use of the into_aesg and into_chacha methods in the CipherState<Cipher_: AeadCipher> implementation. Should these be removed? If they need to stay, can you help me understand when/how they should be used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is another into_aesg method in the impl GenericCipher block. It has the [allow(dead_code)] decorator, should this be removed?

Original file line number Diff line number Diff line change
@@ -1,19 +1,79 @@
//! # AEAD Cipher Management
//!
//! The [`CipherState`] trait manages the state and operations of Authenticated Encryption with
//! Associated Data (AEAD) ciphers within cryptographic protocols.
//!
//! ## Overview
//!
//! Details of key management, nonce generation, and encryption/decryption are abstracted away,
//! ensuring the underlying cryptographic details are consistently handled across different cipher
//! implementations.
//!
//! The module also includes the [`GenericCipher`] enum, which allows for the use of different AEAD
//! cipher implementations in a generic manner.
//!
//! ## Usage
//!
//! The [`CipherState`] trait is used by the [`crate::handshake::HandshakeOp`] trait to handle the
//! stateful encryption and decryption tasks required during the Noise protocol handshake. By
//! implementing [`CipherState`], handshake operations securely manage cryptographic material and
//! perform necessary transformations on messages exchanged between the initiator and responder.
//!
//! The [`crate::Initiator`] and [`crate::Responder`] structs use [`GenericCipher`] instances (`c1`
//! and `c2`) to perform symmetric encryption and decryption once the Noise handshake is complete.
//! These ciphers, initialized and managed through the [`CipherState`] trait, ensure that ongoing
//! communication remains confidential and authenticated.

use std::ptr;

use crate::aed_cipher::AeadCipher;
use aes_gcm::Aes256Gcm;
use chacha20poly1305::{aead::Buffer, ChaCha20Poly1305};

/// Manages the state and operations of an AEAD cipher.
///
/// Provides methods for managing the cryptographic state of an AEAD cipher, including the
/// encryption key (`k`), nonce (`n`), and the cipher instance itself. It also provides methods for
/// encrypting and decrypting data with additional associated data (AAD) using the managed cipher.
///
/// [`CipherState`] is typically implemented by structs that need to manage cipher state as part of
/// a the Noise protocol handshake.
pub trait CipherState<Cipher_: AeadCipher>
where
Self: Sized,
{
/// Retrieves a mutable reference to the 32-byte encryption key (`k`).
fn get_k(&mut self) -> &mut Option<[u8; 32]>;

/// Sets the 32-byte encryption key to the optionally provided value (`k`).
///
/// Allows the encryption key to be explicitly set, typically after it has been derived or
/// initialized during the handshake process. If `None`, the encryption key is unset.
fn set_k(&mut self, k: Option<[u8; 32]>);

/// Retrieves the current nonce (`n`) used for encryption.
///
/// The nonce is a counter that is incremented with each encryption/decryption operations to
/// ensure that each encryption operation with the same key produces a unique ciphertext.
fn get_n(&self) -> u64;

/// Sets the nonce (`n`) to the provided value.
///
/// Allows the nonce to be explicitly set, typically after it has been initialized, incremented
/// during the encryption process, or reset.
fn set_n(&mut self, n: u64);

/// Retrieves a mutable reference to the optional cipher instance.
///
/// Provides access to the underlying AEAD cipher instance used for encryption and decryption
/// operations.
fn get_cipher(&mut self) -> &mut Option<Cipher_>;

/// Converts the current 64-bit nonce value (`n`) to a 12-byte array.
///
/// Converts the 64-bit nonce value to a 12-byte array suitable for use with AEAD ciphers,
/// which typically expect a 96-bit (12-byte) nonce. The result is a correctly formatted nonce
/// for use in encryption and decryption operations.
fn nonce_to_bytes(&self) -> [u8; 12] {
let mut res = [0u8; 12];
let n = self.get_n();
Expand All @@ -37,6 +97,11 @@ where
Some(Cipher::from_cipher(c))
}

/// Encrypts the provided `data` in place using the cipher and AAD (`ad`).
///
/// Performs authenticated encryption on the provided `data` buffer, modifying it in place to
/// contain the ciphertext. The encryption is performed using the current nonce and the AAD.
/// The nonce is incremented after each successful encryption.
fn encrypt_with_ad<T: Buffer>(
&mut self,
ad: &[u8],
Expand All @@ -58,6 +123,11 @@ where
}
}

/// Decrypts the data in place using the cipher and AAD (`ad`).
///
/// Performs authenticated decryption on the provided `data` buffer, modifying it in place to
/// contain the plaintext. The decryption is performed using the current nonce and the provided
/// AAD. The nonce is incremented after each successful decryption.
fn decrypt_with_ad<T: Buffer>(
&mut self,
ad: &[u8],
Expand All @@ -80,6 +150,17 @@ where
}
}

/// An enum representing a generic cipher state.
///
/// [`GenericCipher`] abstracts over different AEAD cipher implementations, allowing for flexible
/// use of either [`ChaCha20Poly1305`] or [`Aes256Gcm`]. It provides methods for encryption,
/// decryption, and key erasure.
///
/// A generic cipher that can be either [`ChaCha20Poly1305`] or [`Aes256Gcm`].
///
/// The [`GenericCipher`] enum allows for flexibility in choosing between different AEAD ciphers.
/// It provides methods for encrypting, decrypting, and securely erasing the encryption key,
/// ensuring that sensitive cryptographic material is handled correctly.
#[allow(clippy::large_enum_variant)]
pub enum GenericCipher {
ChaCha20Poly1305(Cipher<ChaCha20Poly1305>),
Expand All @@ -88,24 +169,45 @@ pub enum GenericCipher {
}
Shourya742 marked this conversation as resolved.
Show resolved Hide resolved

impl Drop for GenericCipher {
/// Securely erases the encryption key when the [`GenericCipher`] is dropped.
///
/// Ensures that the encryption key is securely erased from memory when the [`GenericCipher`]
/// instance is dropped, preventing any potential leakage of sensitive cryptographic material.
fn drop(&mut self) {
self.erase_k();
}
}

impl GenericCipher {
/// Encrypts the data (`msg`) in place using the underlying cipher.
///
/// Performs authenticated encryption on the provided data buffer, modifying it in place to
/// contain the ciphertext. The encryption is performed using the current nonce and an empty
/// additional associated data (AAD) buffer.
pub fn encrypt<T: Buffer>(&mut self, msg: &mut T) -> Result<(), aes_gcm::Error> {
Shourya742 marked this conversation as resolved.
Show resolved Hide resolved
match self {
GenericCipher::ChaCha20Poly1305(c) => c.encrypt_with_ad(&[], msg),
GenericCipher::Aes256Gcm(c) => c.encrypt_with_ad(&[], msg),
}
}

/// Decrypts the data (`msg`) in place using the underlying cipher.
///
/// Performs authenticated decryption on the provided data buffer, modifying it in place to
/// contain the plaintext. The decryption is performed using the current nonce and an empty
/// additional associated data (AAD) buffer.
pub fn decrypt<T: Buffer>(&mut self, msg: &mut T) -> Result<(), aes_gcm::Error> {
Shourya742 marked this conversation as resolved.
Show resolved Hide resolved
match self {
GenericCipher::ChaCha20Poly1305(c) => c.decrypt_with_ad(&[], msg),
GenericCipher::Aes256Gcm(c) => c.decrypt_with_ad(&[], msg),
}
}

/// Securely erases the encryption key (`k`) from memory.
///
/// Overwrites the encryption key stored within the [`GenericCipher`] with zeros and sets it to
/// `None`, ensuring that the key cannot be recovered after the [`GenericCipher`] is dropped or
/// no longer needed.
pub fn erase_k(&mut self) {
match self {
GenericCipher::ChaCha20Poly1305(c) => {
Expand Down Expand Up @@ -180,9 +282,19 @@ impl CipherState<Aes256Gcm> for GenericCipher {
}
}

/// Represents the state of an AEAD cipher, including the optional 32-byte encryption key (`k`),
/// nonce (`n`), and optional cipher instance (`cipher`).
///
/// Manages the cryptographic state required to perform AEAD encryption and decryption operations.
/// It stores the optional encryption key, the nonce, and the optional cipher instance itself. The
/// [`CipherState`] trait is implemented to provide a consistent interface for managing cipher
/// state across different AEAD ciphers.
pub struct Cipher<C: AeadCipher> {
/// Optional 32-byte encryption key.
k: Option<[u8; 32]>,
Shourya742 marked this conversation as resolved.
Show resolved Hide resolved
/// Nonce value.
n: u64,
/// Optional cipher instance.
cipher: Option<C>,
}

Expand All @@ -191,7 +303,6 @@ pub struct Cipher<C: AeadCipher> {
// anymore it
//impl<C: AeadCipher> !Sync for Cipher<C> {}
Shourya742 marked this conversation as resolved.
Show resolved Hide resolved
//impl<C: AeadCipher> !Copy for Cipher<C> {}

impl<C: AeadCipher> Cipher<C> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should add rust docs on this impl:

/// Implementation of the `Cipher` struct for managing AEAD ciphers in cryptographic protocols.
///
/// Provides methods for initializing and managing the state of an AEAD cipher, including key
/// management, nonce handling, and encryption/decryption operations. The `Cipher` struct is
/// designed to be used in scenarios where secure, stateful encryption and decryption are required,
/// such as during a Noise protocol handshake.
///
/// This implementation provides the necessary methods for the `CipherState` trait used by
/// higher-level abstractions like `HandshakeOp` to manage cryptographic operations.

@Fi3, can you add some color to these existing comments:

// Make sure that Cipher is not sync so we do not need to worry about what other memory accessor see
// after that we zeroize k is send cause if we send it the original thread can not access
// anymore it
//impl<C: AeadCipher> !Sync for Cipher<C> {}
//impl<C: AeadCipher> !Copy for Cipher<C> {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @Fi3

/// Internal use only, we need k for handshake
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure what "Internal use only, we need k for handshake" means (@Fi3 do you know), but I think we should remove that comment and add:

Suggested change
/// Internal use only, we need k for handshake
/// Creates a new `Cipher` instance from the provided encryption key (`k`) and cipher (`c`).
///
/// Initializes the `Cipher` struct with a given encryption key and cipher instance, preparing
/// it for use in encryption and decryption operations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the from_cipher method below, I do not know what the existing comment "At the end of the handshake we return a cipher with hidden key" means (@Fi3 do you know?), but I think we should remove it and add:

    /// Creates a new `Cipher` instance from the provided cipher (`c`), without an initial key.
    ///
    /// Initializes the `Cipher` struct with the provided cipher instance without an encryption
    /// key. The key can be set later using the `set_k` method.

pub fn from_key_and_cipher(k: [u8; 32], c: C) -> Self {
Expand Down
29 changes: 29 additions & 0 deletions protocols/v2/noise-sv2/src/error.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that there are several error variants here that I cannot find uses of in the codebase. I did my best to make comments for them for the sake completeness and listed them in this comment in our #1130 tech debt tracker issue.

Original file line number Diff line number Diff line change
@@ -1,18 +1,47 @@
//! # Noise Sv2 Error Handling
//!
//! [`Error`] defines the set of possible errors that may arise during the Noise protocol
//! handshake, cipher selection, and encryption/decryption processes.

use aes_gcm::Error as AesGcm;

/// Noise protocol error handling.
#[derive(Debug, PartialEq, Eq)]
pub enum Error {
/// The handshake has not been completed when a finalization step is executed.
HandshakeNotFinalized,

/// Error on an empty cipher list is provided where one is required.
CipherListMustBeNonEmpty,

/// Error on unsupported ciphers.
UnsupportedCiphers(Vec<u8>),

/// Provided cipher list is invalid or malformed.
InvalidCipherList(Vec<u8>),

/// Chosen cipher is invalid or unsupported.
InvalidCipherChosed(Vec<u8>),

/// Wraps AES-GCM errors during encryption/decryption.
AesGcm(AesGcm),

/// Cipher is in an invalid state during encryption/decryption operations.
InvalidCipherState,

/// Provided certificate is invalid or cannot be verified.
InvalidCertificate([u8; 74]),

/// A raw public key is invalid or cannot be parsed.
InvalidRawPublicKey,

/// A raw private key is invalid or cannot be parsed.
InvalidRawPrivateKey,

/// An incoming handshake message is expected but not received.
ExpectedIncomingHandshakeMessage,

/// A message has an incorrect or unexpected length.
InvalidMessageLength,
}

Expand Down
Loading
Loading