Implement ecparam CLI tool #2718

kingstjo · 2025-09-26T23:36:31Z

Issues:

Addresses #CryptoAlg-3381

Description of changes:

This PR implements a comprehensive ecparam CLI tool for AWS-LC, providing OpenSSL-compatible elliptic curve parameter and key generation functionality.

Current behavior: AWS-LC lacked an ecparam command-line tool, limiting users' ability to generate EC parameters and keys via CLI.

New behavior: Users can now generate elliptic curve parameters and private keys using the openssl ecparam command with full OpenSSL compatibility.

Key features implemented:

EC parameter generation in PEM and DER formats for all supported curves
EC private key generation with configurable point compression (compressed/uncompressed)
Multiple output formats (PEM, DER) with proper format validation
Comprehensive error handling with helpful error messages and supported curve listings
OpenSSL CLI compatibility - generated keys work seamlessly with OpenSSL tools

Supported curves: secp224r1 (P-224), prime256v1 (P-256), secp384r1 (P-384), secp521r1 (P-521), secp256k1

Call-outs:

Error handling: Comprehensive validation with user-friendly error messages and curve listings
Cross-compatibility: Generated keys are validated to work with standard OpenSSL CLI tools

Testing:

Comprehensive test suite with 21 Google Tests:

Basic functionality tests (5 tests):
- Parameter generation with cryptographic validation using PEM_read_bio_ECPKParameters
- Key generation with EC key validation using PEM_read_bio_PrivateKey
- Point compression format validation using EC_KEY_get_conv_form
- DER format validation using d2i_ECPKParameters_bio
Error handling tests (3 tests):
- Invalid curve names, conversion forms, and output formats
OpenSSL compatibility tests (13 tests):
- Dynamic curve testing: Automatically tests all 5 supported curves using EC_get_builtin_curves()
- Cross-compatibility validation: Verifies OpenSSL CLI can read AWS-LC generated keys
- Format compatibility: Tests PEM, DER, compressed, and uncompressed formats
- Parameterized testing: Uses Google Test parameterization for maintainable test coverage

Testing approach:

Real-world compatibility: Tests that AWS-LC keys work with openssl pkey command
Comprehensive coverage: All supported curves, formats, and options tested
Future-proof: Dynamic curve detection ensures tests adapt to new curves automatically

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.

…el APIs - Use bssl::UniquePtr for automatic memory management - Use high-level EVP APIs (EVP_PKEY_keygen) instead of low-level EC APIs - Fix test argument parsing to exclude command name - All 19 tests passing including OpenSSL compatibility tests - Add ecparam to CMakeLists.txt, internal.h, and tool.cc

- Replace all ScopedFILE usage with bssl::UniquePtr<BIO> - Use BIO_new_file instead of fopen for better OpenSSL integration - Update PEM_read_PrivateKey to PEM_read_bio_PrivateKey - Update d2i_PrivateKey_fp to d2i_PrivateKey_bio - Replace file size checking with BIO operations for DER tests - Add openssl/bio.h header include - All 19 ecparam tests pass with BIO implementation

…put helper - Replace all manual system() calls in comparison tests with RunCommandsAndCompareOutput() - Eliminates code duplication and improves error handling - Makes ecparam_test.cc consistent with other CLI tool test patterns (x509_test.cc, pkey_test.cc) - All 19 ecparam tests continue to pass - Follows standard AWS-LC CLI testing pattern: direct calls for basic tests, helper for comparisons

- Replace FORMAT_PEM/FORMAT_DER defines with OutputFormat enum class - Replace point_conversion_form_t with PointConversionForm enum class - Add ParseOutputFormat() and ParsePointConversionForm() helper functions - Add test constants for better maintainability - Improve type safety and code readability - All 19 ecparam tests continue to pass

- Add ERR_print_errors_fp() at err label for centralized OpenSSL error reporting - Add specific error messages for all OpenSSL API failures: - Key generation context creation and initialization - EC key generation and extraction - Private key and parameter writing in PEM/DER formats - Provides user-friendly messages + technical OpenSSL details - Superior error UX compared to OpenSSL's cryptic/verbose messages - All 19 ecparam tests continue to pass

- Replace complex EVP_PKEY_CTX setup with direct EC_KEY_new_by_curve_name() - Use EC_KEY_generate_key() instead of multi-step EVP key generation - Reduce code from 25 lines to 8 lines (68% reduction) - Eliminate unnecessary EVP_PKEY wrapper and extraction step - Fewer error points and clearer error messages - Same functionality - all 19 ecparam tests pass - Better performance and maintainability

Add ValidateECCurve helper function to: - Validate curve names using both short names and NIST names - Provide user-friendly error messages - List supported curves with descriptions - Maintain clean separation of concerns All tests passing including OpenSSL compatibility tests.

- Fixed RunCommandsAndCompareOutput parameter mismatch by creating RunAndCompareCommands helper - Added missing namespace closing brace - Improved key generation tests to validate OpenSSL CLI compatibility with AWS-LC generated keys - Simplified tests to focus on CLI-to-CLI interoperability rather than mixed API/CLI approaches - All 19 tests now pass with proper cross-compatibility validation

- Replaced duplicate curve comparison tests with EcparamCurveComparisonTest parameterized test - Consolidated key generation tests into EcparamKeyGenComparisonTest parameterized test - Improved test maintainability and reduced code duplication - All 19 tests still pass with cleaner, more organized structure - Easy to add new curves and key generation scenarios

- Replace basic file existence checks with proper cryptographic validation - EcparamToolBasicTest: Validate EC parameters using PEM_read_bio_ECPKParameters - EcparamToolGenkeyTest: Validate generated EC private keys and curve correctness - EcparamToolConvFormTest: Verify compressed point format using EC_KEY_get_conv_form - EcparamToolOutformTest: Validate DER format using d2i_ECPKParameters_bio - All 19 tests pass with stronger cryptographic correctness validation

- Replace hardcoded curve list with GetSupportedCurves() function - Use EC_get_builtin_curves() API to dynamically discover all supported curves - Automatically generate test names from curve names with proper formatting - Increased test coverage from 3 to 5 curves (secp224r1, prime256v1, secp384r1, secp521r1, secp256k1) - Tests now stay in sync with actual AWS-LC supported curves - All 21 tests pass with comprehensive curve coverage

- Create OpenSSLComparisonTestBase to eliminate SetUp/TearDown duplication - Remove unused tool_output_str and openssl_output_str variables - Simplify GetSupportedCurves() string manipulation using std::transform - Fix inconsistent error messages for environment variables - Add missing includes for algorithm and cctype headers - Reduce code duplication by ~30 lines while maintaining all functionality - All 21 tests pass with cleaner, more maintainable code structure

- Add clear section headers for better code organization - Remove duplicate function and struct definitions - Group helper functions and structures at the top - Improve logical flow: Basic tests → Error tests → Comparison tests - Add descriptive comments for each test section - All 21 tests pass with cleaner, more maintainable structure

codecov-commenter · 2025-09-27T00:10:17Z

Codecov Report

❌ Patch coverage is 72.01835% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.81%. Comparing base (84d06ec) to head (5f31209).

Files with missing lines	Patch %	Lines
tool-openssl/ecparam_test.cc	70.19%	30 Missing and 1 partial ⚠️
tool-openssl/ecparam.cc	73.68%	30 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2718      +/-   ##
==========================================
- Coverage   78.82%   78.81%   -0.01%     
==========================================
  Files         667      669       +2     
  Lines      114077   114297     +220     
  Branches    16062    16096      +34     
==========================================
+ Hits        89920    90086     +166     
- Misses      23382    23433      +51     
- Partials      775      778       +3

☔ 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.

smittals2 · 2025-10-01T00:50:38Z

tool-openssl/ecparam.cc

+  // Parse output format
+  if (!outform.empty()) {
+    if (isStringUpperCaseEqual(outform, "DER") || isStringUpperCaseEqual(outform, "PEM")) {
+      output_format = ParseOutputFormat(outform);


this is feeling a bit redundant. we end up validating here, and then again in ParseOutputFormat, and then anyway you have to do if else checks in code to see if its DER or PEM

smittals2 · 2025-10-01T00:54:05Z

tool-openssl/ecparam.cc

+    }
+
+    // Set point conversion form on the key
+    EC_KEY_set_conv_form(eckey.get(), static_cast<point_conversion_form_t>(point_form));


why do we need a cast here, can we just handle this in the enum type? maybe define as an enum and not enum class

smittals2 · 2025-10-01T00:56:30Z

tool-openssl/ecparam.cc

+  return PointConversionForm::UNCOMPRESSED; // default
+}
+
+static bool ValidateECCurve(const std::string& curve_name, int* out_nid = nullptr, bool print_supported = true) {


minor nit, this static func is called once in this file, so do we need default vals or the print_supported param at all

+1: YAGNI -- I say just remove the parameter if you're only using the default for it.

smittals2 · 2025-10-01T00:59:00Z

tool-openssl/ecparam_test.cc

+// -------------------- Helper Functions and Structures ------------------------
+
+// Base class for OpenSSL comparison tests
+class OpenSSLComparisonTestBase : public ::testing::Test {


Do we need the complexity of all of these class hierarchies? can we just do one class that handles all openssl comparison tests?

justsmth · 2025-10-01T11:23:49Z

tool-openssl/ecparam.cc

+enum class PointConversionForm {
+  UNCOMPRESSED = POINT_CONVERSION_UNCOMPRESSED,
+  COMPRESSED = POINT_CONVERSION_COMPRESSED
+};


NP: You could just use (or alias) the existing enum: https://github.com/aws/aws-lc/blob/main/include/openssl/ec.h#L81-L96

justsmth · 2025-10-01T11:25:09Z

tool-openssl/ecparam.cc

+static OutputFormat ParseOutputFormat(const std::string &format_str) {
+  if (isStringUpperCaseEqual(format_str, "DER")) {
+    return OutputFormat::DER;
+  } else if (isStringUpperCaseEqual(format_str, "PEM")) {
+    return OutputFormat::PEM;
+  }
+  // Invalid format will be handled by caller
+  return OutputFormat::PEM; // default
+}


NP: There's no need to compare it to "PEM", you're going to return OutputFormat::PEM regardless.

However if this produces an error message when the string doesn't match, then the comparison might be useful.

justsmth · 2025-10-01T11:26:05Z

tool-openssl/ecparam.cc

+static PointConversionForm ParsePointConversionForm(const std::string &form_str) {
+  if (form_str == "compressed") {
+    return PointConversionForm::COMPRESSED;
+  } else if (form_str == "uncompressed") {
+    return PointConversionForm::UNCOMPRESSED;
+  }
+  // Invalid form will be handled by caller
+  return PointConversionForm::UNCOMPRESSED; // default
+}


NP: There's no need to compare it to "uncompressed", you're going to return PointConversionForm::UNCOMPRESSED regardless.

justsmth · 2025-10-01T11:29:12Z

tool-openssl/ecparam.cc

+  return PointConversionForm::UNCOMPRESSED; // default
+}
+
+static bool ValidateECCurve(const std::string& curve_name, int* out_nid = nullptr, bool print_supported = true) {


+1: YAGNI -- I say just remove the parameter if you're only using the default for it.

justsmth · 2025-10-01T11:35:17Z

tool-openssl/ecparam.cc

+  if (!outform.empty()) {
+    if (isStringUpperCaseEqual(outform, "DER") || isStringUpperCaseEqual(outform, "PEM")) {
+      output_format = ParseOutputFormat(outform);
+    } else {
+      fprintf(stderr, "Invalid output format: %s\n", outform.c_str());
+      goto err;
+    }
+  }


Delete ParseOutputFormat -- you're doing all the comparisons here anyways.

justsmth · 2025-10-01T11:36:13Z

tool-openssl/ecparam.cc

+    if (conv_form == "compressed" || conv_form == "uncompressed") {
+      point_form = ParsePointConversionForm(conv_form);
+    } else {
+      fprintf(stderr, "Invalid point conversion form: %s\n", conv_form.c_str());
+      goto err;
+    }


Delete ParsePointConversionForm -- you're doing all of the comparisons here anyways.

justsmth · 2025-10-01T11:41:17Z

tool-openssl/ecparam.cc

+}
+
+static bool ValidateECCurve(const std::string& curve_name, int* out_nid = nullptr, bool print_supported = true) {
+    int nid = OBJ_sn2nid(curve_name.c_str());


I think it would be better to call EC_GROUP_new_by_curve_name below with this nid to "validate" that it's an EC curve. The function can then provide the caller with the group.

justsmth · 2025-10-01T11:46:46Z

tool-openssl/ecparam.cc

+  } else {
+    // Output parameters
+    if (!noout) {


NP: You could make this an } else if (!noout) {

kingstjo added 13 commits September 26, 2025 16:17

kingstjo requested a review from a team as a code owner September 26, 2025 23:36

justsmth requested review from justsmth and smittals2 September 30, 2025 20:21

smittals2 reviewed Oct 1, 2025

View reviewed changes

justsmth reviewed Oct 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Implement ecparam CLI tool #2718

Implement ecparam CLI tool #2718

kingstjo commented Sep 26, 2025

Uh oh!

codecov-commenter commented Sep 27, 2025

Uh oh!

smittals2 Oct 1, 2025

Uh oh!

smittals2 Oct 1, 2025

Uh oh!

smittals2 Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

smittals2 Oct 1, 2025 •

edited

Loading

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

justsmth Oct 1, 2025

Uh oh!

Uh oh!

Implement ecparam CLI tool #2718

Are you sure you want to change the base?

Implement ecparam CLI tool #2718

Conversation

kingstjo commented Sep 26, 2025

Issues:

Description of changes:

Call-outs:

Testing:

Uh oh!

codecov-commenter commented Sep 27, 2025

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smittals2 Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smittals2 Oct 1, 2025 •

edited

Loading