Skip to content

Conversation

@kingstjo
Copy link
Contributor

Issues:

Addresses CryptoAlg-3387
Addresses CryptoAlg-3383

Description of changes:

This PR introduces a centralized password handling approach:

  1. New Password Handling Implementation:
    • Created password.cc for centralized password functionality
    • Implemented HandlePassOptions for unified passin/passout processing
    • Added optimization for same password source usage
    • Implemented SensitiveStringDeleter for secure memory cleanup
    • Updated pkcs8.cc to use the new centralized functions

Call-outs:

  • The implementation follows OpenSSL's app_passwd approach
  • Memory safety is prioritized with proper cleanup and null checks
  • No changes to existing password handling behavior in pkcs8.cc

Testing:

  • Created password_test.cc with test coverage for:
    • ExtractPassword with various sources (direct, file, env)
    • HandlePassOptions with both passin and passout
    • HandlePassOptions with only passin or passout
    • Same source optimization validation
    • Memory safety and cleanup verification
    • SensitiveStringDeleter validation
  • Test design follows Google Test best practices:
    • Uses EXPECT_TRUE with proper null checks
    • Enables full test execution to show all failures
    • Includes comprehensive error case coverage
  • Cross-validation with existing pkcs8.cc functionality
  • All tests pass

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

kingstjo and others added 4 commits July 16, 2025 21:17
…ptions

Implement a centralized password handling approach similar to OpenSSL's app_passwd function:
- Create a new password.cc file with password handling functionality
- Implement HandlePassOptions function to process both passin and passout options
- Optimize for the case where the same password source is used for both
- Move SensitiveStringDeleter to password.cc
- Update pkcs8.cc to use the new function
- Improve documentation in internal.h

🤖 Assisted by Amazon Q Developer
Add password_test.cc with tests for:
- ExtractPassword with various sources (direct, file, env)
- HandlePassOptions with both passin and passout
- HandlePassOptions with only passin or passout
- HandlePassOptions with same source optimization
- SensitiveStringDeleter memory clearing
- Memory safety with HandlePassOptions

🤖 Assisted by Amazon Q Developer
Add a new FIPS Mode section to BUILDING.md with build instructions and
platform limitations. Update README.md to clarify that static FIPS
builds are only supported on Linux platforms. This documentation helps
users understand FIPS support in AWS-LC.

### Issues:
n/a

### Description of changes: 

While trying to use Nix on darwin-aarch64, I realized the FIPS build
instructions are somewhat lacking.

### Call-outs:

Yes, one could just read the CMakeLists.txt file.

### Testing:
How is this change tested (unit tests, fuzz tests, etc.)?
Documentation...

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

---------

Co-authored-by: Daryl Martin <[email protected]>
Replace ASSERT_TRUE with EXPECT_TRUE and add proper null checks to:
- Prevent test termination on first failure
- Show all test failures in a single run
- Add defensive null pointer checks
- Follow testing best practices for assertions

🤖 Assisted by Amazon Q Developer
@kingstjo kingstjo requested a review from a team as a code owner July 17, 2025 18:31
@kingstjo
Copy link
Contributor Author

Closing in favor of #2555 which has a cleaner commit history without unrelated changes.

@kingstjo kingstjo closed this Jul 17, 2025
@kingstjo kingstjo deleted the password-handling-common branch July 17, 2025 18:38
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 85.46512% with 25 lines in your changes missing coverage. Please review.

Project coverage is 78.76%. Comparing base (7374766) to head (352564b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
tool-openssl/password.cc 82.55% 15 Missing ⚠️
tool-openssl/password_test.cc 88.09% 0 Missing and 10 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2554      +/-   ##
==========================================
+ Coverage   78.69%   78.76%   +0.06%     
==========================================
  Files         644      647       +3     
  Lines      110480   110730     +250     
  Branches    15638    15658      +20     
==========================================
+ Hits        86944    87214     +270     
+ Misses      22836    22806      -30     
- Partials      700      710      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants