Tidy bssl-crypto documentation

This probably needs a few iterations, but fix the stuff I noticed on a
first pass:

- I don't think it's useful to say this is the BoringSSL implementation
  of something. That's all implicit from this crate anyway.

- Rust seems to prefer "Returns ..." rather than "Return ..."

- Algorithm names like "hkdf" or "hmac" should be written "HKDF" or
  "HMAC" when referring to the algorithms. Also BoringSSL is styled
  "BoringSSL" rather than "boringssl".

- Given Rust overall doesn't try to handle allocation failure, let's
  just write that we don't believe in allocation failure once in the
  README rather than marking which functions do and don't panic.

Change-Id: I48501717dd0b063a2fa4106c4c140d76a7ef69a9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/60546
Reviewed-by: Nabil Wadih <nwadih@google.com>
Commit-Queue: Bob Beck <bbe@google.com>
Reviewed-by: Bob Beck <bbe@google.com>

7 files changed, 78 insertions, 59 deletions

diff --git a/rust/bssl-crypto/README.md b/rust/bssl-crypto/README.md
index dff669b..bc7371a 100644
--- a/rust/bssl-crypto/README.md
+++ b/rust/bssl-crypto/README.md
@@ -7,3 +7,5 @@ Then to run all tests:
 ```
 cd rust/bssl-crypto && cargo clippy && cargo deny check && cargo test
 ```
+
+Unlike BoringSSL itself, this crate does not attempt to handle allocation failures. If an allocation fails, functions in this crate will panic.
diff --git a/rust/bssl-crypto/src/aes.rs b/rust/bssl-crypto/src/aes.rs
index e3ec5bd..e5a1607 100644
--- a/rust/bssl-crypto/src/aes.rs
+++ b/rust/bssl-crypto/src/aes.rs
@@ -23,7 +23,7 @@ pub type AesBlock = [u8; BLOCK_SIZE];
 pub struct Aes;
 
 impl Aes {
-    /// Encrypt `block` in place.
+    /// Encrypts `block` in place.
     pub fn encrypt(key: &AesEncryptKey, block: &mut AesBlock) {
         let input = *block;
         // Safety:
@@ -31,7 +31,7 @@ impl Aes {
         unsafe { bssl_sys::AES_encrypt(input.as_ptr(), block.as_mut_ptr(), &key.0) }
     }
 
-    /// Decrypt `block` in place.
+    /// Decrypts `block` in place.
     pub fn decrypt(key: &AesDecryptKey, block: &mut AesBlock) {
         let input = *block;
         // Safety:
diff --git a/rust/bssl-crypto/src/digest.rs b/rust/bssl-crypto/src/digest.rs
index ee26f3e..35b6534 100644
--- a/rust/bssl-crypto/src/digest.rs
+++ b/rust/bssl-crypto/src/digest.rs
@@ -17,11 +17,11 @@ use core::marker::PhantomData;
 
 use crate::ForeignTypeRef;
 
-/// The BoringSSL implemented SHA-256 digest algorithm.
+/// The SHA-256 digest algorithm.
 #[derive(Clone)]
 pub struct Sha256 {}
 
-/// The BoringSSL implemented SHA-512 digest algorithm.
+/// The SHA-512 digest algorithm.
 #[derive(Clone)]
 pub struct Sha512 {}
 
@@ -40,7 +40,7 @@ pub trait Md {
     /// The output size of the hash operation.
     const OUTPUT_SIZE: usize;
 
-    /// Gets a reference to a message digest algorithm to be used by the hkdf implementation.
+    /// Gets a reference to a message digest algorithm to be used by the HKDF implementation.
     fn get_md() -> &'static MdRef;
 }
 
@@ -55,7 +55,7 @@ impl Md for Sha256 {
 }
 
 impl Sha256 {
-    /// Create a new [digest] to compute the SHA256 hash.
+    /// Creates a new [Digest] to compute a SHA-256 hash.
     pub fn new_digest() -> Digest<Self, { Self::OUTPUT_SIZE }> {
         // Note: This cannot be in the trait because using associated constants exprs there
         // requires nightly.
@@ -74,7 +74,7 @@ impl Md for Sha512 {
 }
 
 impl Sha512 {
-    /// Create a new [digest] to compute the SHA512 hash.
+    /// Create a new [Digest] to compute a SHA-512 hash.
     pub fn new_digest() -> Digest<Self, { Self::OUTPUT_SIZE }> {
         // Note: This cannot be in the trait because using associated constants exprs there
         // requires nightly.
@@ -82,12 +82,12 @@ impl Sha512 {
     }
 }
 
-/// A struct for computing the digest
+/// A pending digest operation.
 pub struct Digest<M: Md, const OUTPUT_SIZE: usize>(bssl_sys::EVP_MD_CTX, PhantomData<M>);
 
 impl<M: Md, const OUTPUT_SIZE: usize> Digest<M, OUTPUT_SIZE> {
 
-    /// Create a new Digest from the given `Md` type parameter.
+    /// Creates a new Digest from the given `Md` type parameter.
     ///
     /// Panics:
     /// - If `Md::OUTPUT_SIZE` is not the same as `OUTPUT_SIZE`.
@@ -108,7 +108,7 @@ impl<M: Md, const OUTPUT_SIZE: usize> Digest<M, OUTPUT_SIZE> {
         Self(md_ctx, PhantomData)
     }
 
-    /// Updates this digest computation using the given `data`.
+    /// Hashes the provided input into the current digest operation.
     pub fn update(&mut self, data: &[u8]) {
         // Safety:
         // - `data` is a slice from safe Rust.
@@ -118,7 +118,7 @@ impl<M: Md, const OUTPUT_SIZE: usize> Digest<M, OUTPUT_SIZE> {
         assert_eq!(result, 1, "bssl_sys::EVP_DigestUpdate failed");
     }
 
-    /// Consumes this digest and returns the output digest value.
+    /// Computes the final digest value, consuming the object.
     #[allow(clippy::expect_used)]
     pub fn finalize(mut self) -> [u8; OUTPUT_SIZE] {
         let mut digest_uninit =
diff --git a/rust/bssl-crypto/src/ed25519.rs b/rust/bssl-crypto/src/ed25519.rs
index 1fa5b05..df36507 100644
--- a/rust/bssl-crypto/src/ed25519.rs
+++ b/rust/bssl-crypto/src/ed25519.rs
@@ -26,7 +26,7 @@ pub const SEED_LENGTH: usize =
 /// The length in bytes of an Ed25519 signature.
 pub const SIGNATURE_LENGTH: usize = bssl_sys::ED25519_SIGNATURE_LEN as usize;
 
-// The length in bytes of an Ed25519 keypair. In boringssl the private key is suffixed with the
+// The length in bytes of an Ed25519 keypair. In BoringSSL, the private key is suffixed with the
 // public key, so the keypair length is the same as the private key length.
 const KEYPAIR_LENGTH: usize = bssl_sys::ED25519_PRIVATE_KEY_LEN as usize;
 
diff --git a/rust/bssl-crypto/src/hkdf.rs b/rust/bssl-crypto/src/hkdf.rs
index 66b8d60..d314495 100644
--- a/rust/bssl-crypto/src/hkdf.rs
+++ b/rust/bssl-crypto/src/hkdf.rs
@@ -23,12 +23,12 @@ pub type HkdfSha256 = Hkdf<Sha256>;
 /// Implementation of HKDF-SHA-512
 pub type HkdfSha512 = Hkdf<Sha512>;
 
-/// Error type returned from the hkdf expand operations when the output key material has
+/// Error type returned from the HKDF-Expand operations when the output key material has
 /// an invalid length
 #[derive(Debug)]
 pub struct InvalidLength;
 
-/// Implementation of hkdf operations which are generic over a provided hashing functions. Type
+/// Implementation of HKDF operations which are generic over a provided hashing functions. Type
 /// aliases are provided above for convenience of commonly used hashes
 pub struct Hkdf<M: Md> {
     salt: Option<Vec<u8>>,
@@ -40,7 +40,7 @@ impl<M: Md> Hkdf<M> {
     /// The max length of the output key material used for expanding
     pub const MAX_OUTPUT_LENGTH: usize = M::OUTPUT_SIZE * 255;
 
-    /// Creates a new instance of an hkdf from a salt and key material
+    /// Creates a new instance of HKDF from a salt and key material
     pub fn new(salt: Option<&[u8]>, ikm: &[u8]) -> Self {
         Self {
             salt: salt.map(Vec::from),
@@ -49,9 +49,9 @@ impl<M: Md> Hkdf<M> {
         }
     }
 
-    /// The RFC5869 HKDF-Expand operation. The info argument for the expand is set to
+    /// Computes HKDF-Expand operation from RFC 5869. The info argument for the expand is set to
     /// the concatenation of all the elements of info_components. Returns InvalidLength if the
-    /// output is too large or panics if there is an allocation failure.
+    /// output is too large.
     pub fn expand_multi_info(
         &self,
         info_components: &[&[u8]],
@@ -60,8 +60,7 @@ impl<M: Md> Hkdf<M> {
         self.expand(&info_components.concat(), okm)
     }
 
-    /// The RFC5869 HKDF-Expand operation. Returns InvalidLength if the output is too large, or
-    /// panics if there is an allocation failure
+    /// Computes HKDF-Expand operation from RFC 5869. Returns InvalidLength if the output is too large.
     pub fn expand(&self, info: &[u8], okm: &mut [u8]) -> Result<(), InvalidLength> {
         // extract the salt bytes from the option, or empty slice if option is None
         let salt = self.salt.as_deref().unwrap_or_default();
diff --git a/rust/bssl-crypto/src/hmac.rs b/rust/bssl-crypto/src/hmac.rs
index 7dbf39b..167e92e 100644
--- a/rust/bssl-crypto/src/hmac.rs
+++ b/rust/bssl-crypto/src/hmac.rs
@@ -25,8 +25,7 @@ use core::{
 /// Computes the HMAC-SHA256 of `data` as a one-shot operation.
 ///
 /// Calculates the HMAC of data, using the given `key` and returns the result.
-/// It returns the computed hmac.
-/// Can panic if memory allocation fails in the underlying BoringSSL code.
+/// It returns the computed HMAC.
 pub fn hmac_sha256(key: &[u8], data: &[u8]) -> [u8; 32] {
     hmac::<32, Sha256>(key, data)
 }
@@ -34,100 +33,117 @@ pub fn hmac_sha256(key: &[u8], data: &[u8]) -> [u8; 32] {
 /// Computes the HMAC-SHA512 of `data` as a one-shot operation.
 ///
 /// Calculates the HMAC of data, using the given `key` and returns the result.
-/// It returns the computed hmac.
-/// Can panic if memory allocation fails in the underlying BoringSSL code.
+/// It returns the computed HMAC.
 pub fn hmac_sha512(key: &[u8], data: &[u8]) -> [u8; 64] {
     hmac::<64, Sha512>(key, data)
 }
 
-/// The BoringSSL HMAC-SHA256 implementation. The operations may panic if memory allocation fails
-/// in BoringSSL.
+/// An HMAC-SHA256 operation in progress.
 pub struct HmacSha256(Hmac<32, Sha256>);
 
 impl HmacSha256 {
-    /// Create a new hmac from a fixed size key.
+    /// Creates a new HMAC operation from a fixed-length key.
     pub fn new(key: [u8; 32]) -> Self {
         Self(Hmac::new(key))
     }
 
-    /// Create new hmac value from variable size key.
+    /// Creates a new HMAC operation from a variable-length key.
     pub fn new_from_slice(key: &[u8]) -> Self {
         Self(Hmac::new_from_slice(key))
     }
 
-    /// Update state using the provided data.
+    /// Hashes the provided input into the HMAC operation.
     pub fn update(&mut self, data: &[u8]) {
         self.0.update(data)
     }
 
-    /// Obtain the hmac computation consuming the hmac instance.
+    /// Computes the final HMAC value, consuming the object.
     pub fn finalize(self) -> [u8; 32] {
         self.0.finalize()
     }
 
-    /// Check that the tag value is correct for the processed input.
+    /// Checks that the provided tag value matches the computed HMAC value.
     pub fn verify_slice(self, tag: &[u8]) -> Result<(), MacError> {
         self.0.verify_slice(tag)
     }
 
-    /// Check that the tag value is correct for the processed input.
+    /// Checks that the provided tag value matches the computed HMAC value.
     pub fn verify(self, tag: [u8; 32]) -> Result<(), MacError> {
         self.0.verify(tag)
     }
 
-    /// Check truncated tag correctness using left side bytes of the calculated tag.
+    /// Checks that the provided tag value matches the computed HMAC, truncated to the input tag's
+    /// length.
+    ///
+    /// Truncating an HMAC reduces the security of the construction. Callers must ensure `tag`'s
+    /// length matches the desired HMAC length and security level.
     pub fn verify_truncated_left(self, tag: &[u8]) -> Result<(), MacError> {
         self.0.verify_truncated_left(tag)
     }
 
-    /// Resets the hmac instance to its initial state
+    /// Resets the object to its initial state. The key is retained, but input is discarded.
     pub fn reset(&mut self) {
+        // TODO(davidben): This method is a little odd. The main use case I can imagine is
+        // computing multiple HMACs with the same key, while reusing the input-independent key
+        // setup. However, finalize consumes the object, so you cannot actually reuse the
+        // context afterwards. Moreover, even if you could, that mutates the context, so a
+        // better pattern would be to copy the initial context, or to have an HmacKey type
+        // that one could create contexts out of.
         self.0.reset()
     }
 }
 
-/// The BoringSSL HMAC-SHA512 implementation. The operations may panic if memory allocation fails
-/// in BoringSSL.
+/// An HMAC-SHA512 operation in progress.
 pub struct HmacSha512(Hmac<64, Sha512>);
 
 impl HmacSha512 {
-    /// Create a new hmac from a fixed size key.
+    /// Creates a new HMAC operation from a fixed-size key.
     pub fn new(key: [u8; 64]) -> Self {
         Self(Hmac::new(key))
     }
 
-    /// Create new hmac value from variable size key.
+    /// Creates a new HMAC operation from a variable-length key.
     pub fn new_from_slice(key: &[u8]) -> Self {
         Self(Hmac::new_from_slice(key))
     }
 
-    /// Update state using the provided data.
+    /// Hashes the provided input into the HMAC operation.
     pub fn update(&mut self, data: &[u8]) {
         self.0.update(data)
     }
 
-    /// Obtain the hmac computation consuming the hmac instance.
+    /// Computes the final HMAC value, consuming the object.
     pub fn finalize(self) -> [u8; 64] {
         self.0.finalize()
     }
 
-    /// Check that the tag value is correct for the processed input.
+    /// Checks that the provided tag value matches the computed HMAC value.
     pub fn verify_slice(self, tag: &[u8]) -> Result<(), MacError> {
         self.0.verify_slice(tag)
     }
 
-    /// Check that the tag value is correct for the processed input.
+    /// Checks that the provided tag value matches the computed HMAC value.
     pub fn verify(self, tag: [u8; 64]) -> Result<(), MacError> {
         self.0.verify(tag)
     }
 
-    /// Check truncated tag correctness using left side bytes of the calculated tag.
+    /// Checks that the provided tag value matches the computed HMAC, truncated to the input tag's
+    /// length.
+    ///
+    /// Truncating an HMAC reduces the security of the construction. Callers must ensure `tag`'s
+    /// length matches the desired HMAC length and security level.
     pub fn verify_truncated_left(self, tag: &[u8]) -> Result<(), MacError> {
         self.0.verify_truncated_left(tag)
     }
 
-    /// Resets the hmac instance to its initial state
+    /// Resets the object to its initial state. The key is retained, but input is discarded.
     pub fn reset(&mut self) {
+        // TODO(davidben): This method is a little odd. The main use case I can imagine is
+        // computing multiple HMACs with the same key, while reusing the input-independent key
+        // setup. However, finalize consumes the object, so you cannot actually reuse the
+        // context afterwards. Moreover, even if you could, that mutates the context, so a
+        // better pattern would be to copy the initial context, or to have an HmacKey type
+        // that one could create contexts out of.
         self.0.reset()
     }
 }
@@ -177,12 +193,12 @@ struct Hmac<const N: usize, M: Md> {
 }
 
 impl<const N: usize, M: Md> Hmac<N, M> {
-    /// Infallible HMAC creation from a fixed length key.
+    /// Creates a new HMAC operation from a fixed-length key.
     fn new(key: [u8; N]) -> Self {
         Self::new_from_slice(&key)
     }
 
-    /// Create new hmac value from variable size key. Panics on allocation failure
+    /// Creates a new HMAC operation from a variable-length key.
     fn new_from_slice(key: &[u8]) -> Self {
         // Safety:
         // - HMAC_CTX_new panics if allocation fails
@@ -215,7 +231,7 @@ impl<const N: usize, M: Md> Hmac<N, M> {
         }
     }
 
-    /// Update state using the provided data, can be called repeatedly.
+    /// Hashes the provided input into the HMAC operation.
     fn update(&mut self, data: &[u8]) {
         let result = unsafe {
             // Safety: HMAC_Update will always return 1, in case it doesnt we panic
@@ -224,7 +240,7 @@ impl<const N: usize, M: Md> Hmac<N, M> {
         assert_eq!(result, 1, "failure in bssl_sys::HMAC_Update");
     }
 
-    /// Obtain the hmac computation consuming the hmac instance.
+    /// Computes the final HMAC value, consuming the object.
     fn finalize(self) -> [u8; N] {
         let mut buf = [0_u8; N];
         let mut size: c_uint = 0;
@@ -238,13 +254,12 @@ impl<const N: usize, M: Md> Hmac<N, M> {
         buf
     }
 
-    /// Check that the tag value is correct for the processed input.
+    /// Checks that the provided tag value matches the computed HMAC value.
     fn verify(self, tag: [u8; N]) -> Result<(), MacError> {
         self.verify_slice(&tag)
     }
 
-    /// Check truncated tag correctness using all bytes
-    /// of calculated tag.
+    /// Checks that the provided tag value matches the computed HMAC value.
     ///
     /// Returns `Error` if `tag` is not valid or not equal in length
     /// to MAC's output.
@@ -253,10 +268,13 @@ impl<const N: usize, M: Md> Hmac<N, M> {
         self.verify_truncated_left(tag)
     }
 
-    /// Check truncated tag correctness using left side bytes
-    /// (i.e. `tag[..n]`) of calculated tag.
+    /// Checks that the provided tag value matches the computed HMAC, truncated to the input tag's
+    /// length.
     ///
     /// Returns `Error` if `tag` is not valid or empty.
+    ///
+    /// Truncating an HMAC reduces the security of the construction. Callers must ensure `tag`'s
+    /// length matches the desired HMAC length and security level.
     fn verify_truncated_left(self, tag: &[u8]) -> Result<(), MacError> {
         let len = tag.len();
         if len == 0 || len > N {
diff --git a/rust/bssl-crypto/src/lib.rs b/rust/bssl-crypto/src/lib.rs
index 2c80ae8..ce9500b 100644
--- a/rust/bssl-crypto/src/lib.rs
+++ b/rust/bssl-crypto/src/lib.rs
@@ -22,26 +22,26 @@
     clippy::expect_used
 )]
 
-//! Rust boringssl binding
+//! Rust BoringSSL bindings
 
 extern crate core;
 
-/// BoringSSL implemented plain aes operations.
+/// AES block operations.
 pub mod aes;
 
-/// BoringSSL implemented hash functions.
+/// Hash functions.
 pub mod digest;
 
-/// BoringSSL implemented Ed25519 operations.
+/// Ed25519, a signature scheme.
 pub mod ed25519;
 
-/// BoringSSL implemented hkdf operations.
+/// HKDF, a hash-based key derivation function.
 pub mod hkdf;
 
-/// BoringSSL implemented hmac operations.
+/// HMAC, a hash-based message authentication code.
 pub mod hmac;
 
-/// BoringSSL implemented cryptographically secure pseudo-random number generation.
+/// Random number generation.
 pub mod rand;
 
 #[cfg(test)]