Centralize password handling tool-openssl #2555
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
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
- Add Windows-specific environment variable handling - Enhance test robustness with defensive programming - Improve error case handling and reporting - Maintain memory safety in password operations 🤖 Assisted by Amazon Q Developer
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2555 +/- ##
==========================================
+ Coverage 78.81% 78.89% +0.08%
==========================================
Files 667 669 +2
Lines 113982 114316 +334
Branches 16028 16060 +32
==========================================
+ Hits 89835 90191 +356
+ Misses 23374 23351 -23
- Partials 773 774 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…eter - Fix memory safety issue in password test validation - Store string content before deletion for comparison - Avoid accessing freed memory during validation - Update test to properly verify password handling 🤖 Assisted by Amazon Q Developer
- Add parameterized tests for password sources and options - Replace BIO with ScopedFILE for file operations - Add descriptive error messages to assertions - Consolidate similar test cases - Improve test organization and readability 🤖 Assisted by Amazon Q
…ssword-handling-clean
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
smittals2
reviewed
Jul 18, 2025
- Rename password.cc to pass_util.cc to better reflect its purpose - Rename password_test.cc to pass_util_test.cc for consistency - Update CMakeLists.txt with new file names - No functional changes 🤖 Assisted by Amazon Q
Change ExtractPassword function to modify the source string in-place instead of returning a new string. This simplifies the API while maintaining security properties through proper memory cleanup. - Update function signature to return bool - Add comprehensive documentation - Improve error handling with specific messages - Update test cases for new behavior 🤖 Assisted by Amazon Q
- Rename namespace to match file naming convention - Update namespace references in all files - Update BORINGSSL_MAKE_DELETER reference - Improve namespace description comment 🤖 Assisted by Amazon Q
- Rename read_private_key to read_private_der for clarity - Remove PEM handling from function (handled directly by caller) - Eliminate format parameter and branching logic - Reduce function complexity from ~40 to ~25 lines (37% reduction) - Maintain all functionality while improving code clarity - Fix default behavior to output unencrypted PKCS#8 per OpenSSL docs - Enable automatic password prompting for encryption 🤖 Assisted by Amazon Q Developer
Remove self-explanatory comments that merely restate test names: - Comments like 'Test edge cases for file-based passwords' when test is named 'FileEdgeCases' - Comments like 'Make a copy of the string content' for obvious operations - Comments like 'Verify we have the password' for basic assertions Keep valuable comments that provide context, explain complex logic, or document OpenSSL compatibility requirements. All 114 tests continue to pass after cleanup. 🤖 Assisted by Amazon Q Developer
…calls The pkcs8 tool should use ExtractPasswords() to handle both -passin and -passout together, which properly supports the same-file case where both passwords are read from the same file (line 1 for input, line 2 for output). Before: Called ExtractPassword() twice separately - Could not handle same-file case correctly - Both passwords would read from line 1 (incorrect) - Missing validation coordination between passin/passout After: Call ExtractPasswords() once - Properly handles same-file case (line 1 + line 2) - Validates both passwords can be extracted successfully - Matches OpenSSL behavior for: -passin file:pass.txt -passout file:pass.txt All 11 pass_util tests continue to pass. 🤖 Assisted by Amazon Q Developer
Replace manual deleter call with actual smart pointer scope testing: Before: - Manually called pass_util::SensitiveStringDeleter(str) - Only tested deleter function directly - Didn't test UniquePtr integration After: - Tests real usage pattern with bssl::UniquePtr<std::string> - Verifies deleter is called when smart pointer goes out of scope - Tests OPENSSL_cleanse functionality to ensure memory clearing works - More concise and focused test This addresses Comment #2237993238 by testing that memory clearing actually works, while using the same code path as production usage. All 11 pass_util tests continue to pass. 🤖 Assisted by Amazon Q Developer
Integrate upstream's ParseOrderedKeyValueArguments system with our centralized pass_util::ExtractPasswords approach to preserve same-file password support while adopting the new argument parsing framework. Key changes: - Use ParseOrderedKeyValueArguments instead of ParseKeyValueArguments - Keep pass_util::ExtractPasswords for same-file password handling - Adapt v2_cipher/v2_prf validation to new argument system - Preserve reference-based validate_bio_size (addresses AWS-LC-863) 🤖 Assisted by Amazon Q Developer
Match upstream indentation style to reduce reviewer noise while preserving functional improvements: - Keep reference-based validate_bio_size() (addresses AWS-LC-863) - Keep centralized pass_util::ExtractPasswords() for same-file support - Use upstream's 4-space indentation and spacing conventions 🤖 Assisted by Amazon Q Developer
- Add comprehensive CRLF, CR, and mixed line ending tests to FileEdgeCases - Add same-file CRLF testing to ExtractPasswordsSameFile - Test embedded vs trailing carriage return handling - Verify cross-platform compatibility for Windows/Mac/Unix files - All tests pass, addressing reviewer feedback on Windows CR behavior Assisted by Amazon Q
- Support using stdin for both -passin and -passout options - Read first line for input password, second line for output password - Matches OpenSSL behavior for dual stdin usage - Refactor ExtractPasswordFromFile to ExtractPasswordFromStream - Use lambda to eliminate code duplication - Update documentation to reflect stdin support - All tests pass, backward compatibility maintained
- Add fd:N password source support (Unix only, excluded on Windows) - Refactor ExtractPasswordFromStream to use pass_util::Source enum - Support dual fd usage (same fd for both passin and passout) - Read first line for passin, second line for passout from same fd - Use BIO_new_fd directly without buffering (AWS-LC supports BIO_gets on fd) - Update documentation to include fd: option - Maintain backward compatibility with existing functionality - All tests pass, manual testing confirms fd functionality works
- Replace ternary operator with explicit if-else for fprintf calls - Resolves C4774 warning treated as error on Windows CI - Maintains same functionality with MSVC-compatible format strings
|
Can we add some tests for stdin and fd options as well? |
- Add FdExtraction test for valid fd, invalid fd (-1), and non-numeric fd - Add StdinExtraction test for single password from stdin - Add StdinExtractPasswords test for dual passwords from stdin - Fix write() return value checking for CI compatibility
- Wrap StdinExtraction and StdinExtractPasswords tests with #ifndef _WIN32 - pipe(), dup(), dup2(), STDIN_FILENO are Unix-only functions - Tests will only run on Unix platforms where stdin redirection is supported
smittals2
reviewed
Sep 9, 2025
smittals2
reviewed
Sep 9, 2025
smittals2
reviewed
Sep 9, 2025
smittals2
previously approved these changes
Sep 9, 2025
- Remove inaccurate static cipher list (included non-working des-cbc) - Add dynamic validation using PKCS5_pbe2_encrypt_init() - Use RAII with bssl::ScopedCBB and bssl::ScopedEVP_CIPHER_CTX - Improve error messages with specific cipher names - Automatically stays in sync with kCipherOIDs[] array - All existing tests pass
- Add Windows headers for CreatePipe, SetStdHandle APIs - Implement Windows stdin test using same pattern as Unix version - All tests pass, no functional changes to existing code - Windows version ready for use if needed
- Wrap StdinExtractPasswords test in platform guards - Add Windows implementation for both stdin tests - Fix DWORD vs int comparison with static_cast
- Remove internal header dependency that caused linking issues - Remove upfront PKCS#8 cipher compatibility validation - Let EVP_get_cipherbyname() and encryption functions handle invalid ciphers - Fix incorrect logic that rejected all cipher inputs - All 31 PKCS8 and PassUtil tests now pass - Matches OpenSSL behavior: simple validation, deferred error handling
- Add FlushFileBuffers() call before closing write pipe - Ensures data is flushed to read pipe before test proceeds - Should fix Windows CI stdin test failures
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issues:
Addresses CryptoAlg-3387
Addresses CryptoAlg-3383
Addresses AWS-LC-863
Description of changes:
This PR creates new centralized password handling utility for AWS-LC tool-openssl commands:
New Password Utility Implementation:
pass_util.ccandpass_util.hproviding unified password functionality for tool-opensslExtractPasswordfor single password extraction from various sourcesExtractPasswordsfor dual password extraction with same-file supportValidateSourcehelper for consistent validation logicSensitiveStringDeleterfor secure memory cleanupPEM_BUFSIZEfor consistent buffer sizingAPI Features:
pass:,file:,env:,stdin, andfdpassword sources for tool-openssl commandsfdis not supported for WindowsDesign:
Call-outs:
validate_bio_sizeparameter to take reference over pointerTesting:
pass_util_test.ccwith full coverage:ExtractPasswordwith various sources (direct, file, env) and edge casesExtractPasswordswith different files, same file, and mixed sourcesSensitiveStringDeletervalidationBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.