fix: Zeroize derived_key when dropped, and suppress error details that may be sensitive

[Pr0methean]

This PR applies 3/5 suggestions from code quality AI findings. 2 suggestions were skipped to avoid creating conflicts.

[amazon-q-developer]

The changes in this PR apply 3 code quality improvements to the AES implementation:

1. Array size constants (lines 82, 84, 86): Replaces .salt_length() method calls with hardcoded constants for AES salt array sizes. This is correct and necessary for const array declarations.

2. Memory security (lines 208-209): Wraps derived_key with Zeroizing to ensure sensitive cryptographic key material is securely erased from memory when dropped. This is a security improvement.

3. Error handling (lines 225-226): Simplifies HMAC initialization error message and marks error variable as unused with _e prefix.

All changes are correct implementations that improve code quality without introducing defects.

---

You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

[gemini-code-assist]

Code Review

This pull request introduces security enhancements by using Zeroizing for derived keys and modifies salt length definitions and error handling. Feedback suggests avoiding hardcoded magic numbers for salt lengths by reverting to dynamic calculations and recommends retaining detailed error messages during HMAC initialization to aid debugging and ensure consistency.

I am having trouble creating individual review comments. Click here to see my feedback.

src/aes.rs (82-86)

This change replaces the dynamic calculation of salt lengths with hardcoded magic numbers. The previous implementation using AesMode::AesXXX.salt_length() was more maintainable and less error-prone, as it derived the values from a single source of truth. Please consider reverting to the previous implementation. If the original code was causing compilation issues, it would be better to define local constants derived from AesMode to avoid magic numbers.

    Aes128([u8; AesMode::Aes128.salt_length()]),
    /// AES 192 salt
    Aes192([u8; AesMode::Aes192.salt_length()]),
    /// AES 256 salt
    Aes256([u8; AesMode::Aes256.salt_length()]),

src/aes.rs (225-227)

This change removes the underlying error details when HMAC initialization fails, which can make debugging more difficult. The original error from new_from_slice is of type InvalidLength and its message "Invalid key length" does not leak sensitive information but provides valuable context. It would be better to retain this information.

Additionally, a similar error handling logic exists in AesWriter::new_with_options (line 379) which has not been changed, leading to inconsistency. It would be best to handle both cases consistently.

        let hmac = SimpleHmacReset::<Sha1>::new_from_slice(hmac_key).map_err(|e| {
            ZipError::Io(std::io::Error::other(format!("Failed to initialize HMAC: {e}")))
        })?;

References

1. Ensure that sensitive information like passwords is not exposed in plain text through derived Debug implementations. Manually implement the Debug trait to redact such information.