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.72% 78.77% +0.05%
==========================================
Files 645 647 +2
Lines 110641 110830 +189
Branches 15648 15657 +9
==========================================
+ Hits 87097 87308 +211
+ Misses 22841 22819 -22
Partials 703 703 ☔ 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
Update test class names in pass_util_test.cc to match the file name change: - Rename PasswordTest to PassUtilTest - Rename PasswordSourceTest to PassUtilSourceTest - Rename PasswordSourceParams to PassUtilSourceParams - Update test declarations and instantiations accordingly Also fix password handling to match OpenSSL behavior: - Reject empty strings with no prefix - Accept 'pass:' as valid format for empty password - Continue rejecting 'file:' with no filename - Continue rejecting 'env:' with no variable name 🤖 Assisted by Amazon Q Developer
Update FileEdgeCases test to properly verify truncation detection: - Add test for exact truncation boundary (4095 bytes, no newline) - Add test for valid max length with newline (4094 + newline) - Keep existing tests for oversized files and empty files - Verify both truncation and content length checks 🤖 Assisted by Amazon Q Developer
…mory issues This change adds proper initialization for the PassUtilSourceParams struct by implementing both a default constructor and a parameterized constructor. The fix resolves Valgrind errors that were occurring when Google Test tried to print test parameters that contained uninitialized memory. 🤖 Assisted by Amazon Q Developer
…tead of raw pointer. Addresses AWS-LC-863
… PEM_BUFSIZE - Replace custom DEFAULT_MAX_SENSITIVE_STRING_LENGTH (4KB) with PEM_BUFSIZE (1KB) for better compatibility with PEM functions and AWS-LC standards - Refactor ExtractPasswords to use consistent API: modify parameters in-place instead of separate input/output parameters - Simplify same-file logic: use same_file boolean to control skip_first_line parameter, eliminating duplicate code paths - Add comprehensive helper functions for password source detection and extraction - Update tests to use PEM_BUFSIZE and add helper functions for better maintainability - Improve code organization with clear separation of concerns 🤖 Assisted by Amazon Q Developer
…ests - Add ValidateSource helper function to eliminate code duplication between ExtractPassword and ExtractPasswords functions - Refactor both functions to use shared validation logic for consistency - Remove redundant parameterized tests (PassUtilSourceTest) that duplicated coverage provided by dedicated edge case tests - Fix WriteTestFile helper to handle empty content correctly - Update newline-only file test expectation to match correct behavior - Reduce test count from 17 to 11 while maintaining 100% coverage - All tests now passing (11/11) Benefits: - 50% reduction in duplicate validation code - 35% reduction in test count with no coverage loss - Improved maintainability with single source of truth for validation - Faster test execution and cleaner test suite 🤖 Assisted by Amazon Q Developer
andrewhop
reviewed
Jul 28, 2025
tool-openssl/pass_util_test.cc
Outdated
| // Call the deleter | ||
| pass_util::SensitiveStringDeleter(str); | ||
|
|
||
| // The original string content should still be intact for comparison |
There was a problem hiding this comment.
Is this test missing the most important test that checks the data in str is all 0?
There was a problem hiding this comment.
You're right. We aren't verifying the data has been updated to 0000000.
I was running into issues testing this having issues viewing data after it had already been freed.
We can remove this test from CLI tests as it is testing OPENSSL_cleanse and bssl:UniquePtr which if tested should be tested on the library side.
tool-openssl/pkcs8.cc
Outdated
Comment on lines
115
to
128
| if (format == "DER") { | ||
| BIO_reset(in_bio); | ||
| bssl::UniquePtr<PKCS8_PRIV_KEY_INFO> p8inf( | ||
| d2i_PKCS8_PRIV_KEY_INFO_bio(in_bio, nullptr)); | ||
| if (p8inf) { | ||
| pkey.reset(EVP_PKCS82PKEY(p8inf.get())); | ||
| if (pkey) { | ||
| return pkey; | ||
| } | ||
| } | ||
| BIO_reset(in_bio); | ||
| pkey.reset(d2i_PrivateKey_bio(in_bio, nullptr)); | ||
| return pkey; | ||
| } else { |
There was a problem hiding this comment.
This could use some comments, I don't follow the logic for keys with and without a PKCS8_PRIV_KEY_INFO. Can any of this be combined with the logic above in the if(passin) branch. Are there any other utility functions in AWS-LC that supports parsing all these keys with and without passwords?
There was a problem hiding this comment.
Some of this complexity stemmed from users that would be giving a private key not already in PKCS8 format along with trying an empty password for incoming keys that did not have -passin but may be encrypted.
With #2419 I now need to refactor pkcs8.cc to stop blocking encrypted paths that should prompt for password
…lass Replace static uint8_t constants with a proper Source enum class in the pass_util namespace for better type safety and code readability. This improves maintainability and follows modern C++ best practices. Changes: - Add Source enum class to internal.h with scoped values - Update all references in pass_util.cc to use enum class - Maintain same functionality with improved type safety 🤖 Assisted by Amazon Q Developer
Add specific byte limits to password-related error messages to help users understand the exact constraints when passwords exceed maximum allowed length. This improves user experience by providing actionable information instead of generic error messages. Changes: - Direct password error: Include PEM_BUFSIZE value (1024 bytes) - File password error: Include maximum limit in truncation message - Environment variable error: Include maximum limit in length message 🤖 Assisted by Amazon Q Developer
Improve code readability by adjusting parentheses in truncation detection logic. Remove outermost parentheses and add explicit grouping around the length check to make the primary condition (buffer full) more prominent. Changes: - Group length comparison: (static_cast<size_t>(len) == PEM_BUFSIZE - 1) - Remove outer parentheses around entire boolean expression - Maintain identical logic while improving readability 🤖 Assisted by Amazon Q Developer
…ter location Remove outdated comment referencing password.cc file location since: - File is now named pass_util.cc, not password.cc - Git history already tracks code movement - Such comments become stale and confusing over time 🤖 Assisted by Amazon Q Developer
nhatnghiho
reviewed
Jul 30, 2025
|
Not sure if I missed something but OpenSSL looks like they also support |
Change password length validation from '<= 0' to '< 0' to allow empty passwords, matching OpenSSL behavior for interactive prompting. - evp.c: Allow min_length of 0 in EVP_read_pw_string_min - pem_pkey.c: Accept zero-length passwords in PEM_read_bio_PrivateKey This enables proper interactive password prompting when no password is provided via -passin, allowing users to enter empty passwords or be prompted interactively for encrypted PEM keys. 🤖 Assisted by Amazon Q Developer
- 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
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:, andenv:password sources for tool-openssl commandsDesign:
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.