Skip to content

Conversation

@nhatnghiho
Copy link
Contributor

Issues:

Resolves #CryptoAlg-3380

Description of changes:

Implement the following options for x509 cli command:

  • sha256
  • config
  • key
  • passin
  • extensions
  • outform

Testing:

Unit tests added, including missing coverage for some existing x509 options.

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.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 11. Check the log or trigger a new build to see more.

@codecov-commenter
Copy link

codecov-commenter commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 35.71429% with 756 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.43%. Comparing base (981c79b) to head (e236319).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
tool-openssl/req_test.cc 31.13% 374 Missing and 2 partials ⚠️
tool-openssl/pkcs8_test.cc 17.10% 126 Missing ⚠️
tool-openssl/req.cc 59.37% 117 Missing ⚠️
tool-openssl/test_util.cc 4.44% 84 Missing and 2 partials ⚠️
tool-openssl/pkeyutl_test.cc 44.00% 42 Missing ⚠️
tool-openssl/pkeyutl.cc 64.00% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2775    +/-   ##
========================================
  Coverage   78.42%   78.43%            
========================================
  Files         683      683            
  Lines      116286   117174   +888     
  Branches    16402    16478    +76     
========================================
+ Hits        91198    91900   +702     
- Misses      24212    24391   +179     
- Partials      876      883     +7     

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

@nhatnghiho nhatnghiho force-pushed the req-cli branch 3 times, most recently from b504f7e to 6a32876 Compare October 30, 2025 01:51
@nhatnghiho nhatnghiho marked this pull request as ready for review October 30, 2025 01:52
@nhatnghiho nhatnghiho requested a review from a team as a code owner October 30, 2025 01:52
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

Copy link
Contributor

@justsmth justsmth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov shows most of req.cc as not being covered -- the logic seems to be mostly covered by comparison tests, but apparently not by unit tests. We should have unit tests to provide better coverage and possibly hit a few edge cases that aren't easily covered by comparisons.

@justsmth
Copy link
Contributor

justsmth commented Nov 6, 2025

Also a couple of the Openssl comparison tests are failing for this:

[ RUN      ] ReqComparisonTest.DigestSelectionFromConfig
Writing private key to awslcTestTmpFileWWLGLo
Generating a RSA private key
..........................................................................................................................................................+++++
..................................................+++++
writing new private key to 'awslcTestTmpFile0XuYdf'
-----
unable to find 'distinguished_name' in config
problems making Certificate Request
140514192817024:error:0E06D06C:configuration file routines:NCONF_get_string:no value:crypto/conf/conf_lib.c:273:group=req name=distinguished_name
/home/runner/work/aws-lc/aws-lc/tool-openssl/req_test.cc:550: Failure
Expected equality of these values:
  ExecuteCommand(openssl_command)
    Which is: 256
  0
[  FAILED  ] ReqComparisonTest.DigestSelectionFromConfig (747 ms)
[ RUN      ] ReqComparisonTest.GenerateProtectedPrivateKey
AWS-LC command: /home/runner/work/aws-lc/aws-lc/test_build_dir/tool-openssl/openssl req -new -config awslcTestTmpFiledvLFlt -key awslcTestTmpFileOXNEGz -passout pass:testpassword -keyout awslcTestTmpFilep2rXBR -out awslcTestTmpFileBxyzmy -subj "/CN=encrypted-key.example.com"
OpenSSL command: /home/runner/work/aws-lc/openssl-scratch/libcrypto_install_dir/openssl-OpenSSL_1_1_1-stable/bin/openssl req -new -config awslcTestTmpFiledvLFlt -key awslcTestTmpFileOXNEGz -passout pass:testpassword -keyout awslcTestTmpFile9S9dHU -out awslcTestTmpFilee5UCiH -subj "/CN=encrypted-key.example.com"
Writing private key to awslcTestTmpFilep2rXBR
/home/runner/work/aws-lc/aws-lc/tool-openssl/req_test.cc:681: Failure
Value of: openssl_key_content.find( "-----BEGIN ENCRYPTED PRIVATE KEY-----") != std::string::npos
  Actual: false
Expected: true
OpenSSL private key should be encrypted PKCS#8
/home/runner/work/aws-lc/aws-lc/tool-openssl/req_test.cc:697: Failure
Value of: openssl_key
  Actual: false
Expected: true
Failed to load OpenSSL private key
89335319809600:error:0900006e:PEM routines:OPENSSL_internal:NO_START_LINE:/home/runner/work/aws-lc/aws-lc/crypto/pem/pem_lib.c:635:Expecting: ANY PRIVATE KEY
[  FAILED  ] ReqComparisonTest.GenerateProtectedPrivateKey (250 ms)

@justsmth justsmth force-pushed the req-cli branch 3 times, most recently from 7727746 to 5d2bc29 Compare November 10, 2025 14:18
justsmth
justsmth previously approved these changes Nov 10, 2025
@WillChilds-Klein WillChilds-Klein enabled auto-merge (squash) November 10, 2025 22:57
@WillChilds-Klein WillChilds-Klein merged commit a3c40ea into aws:main Nov 11, 2025
382 of 390 checks passed
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.

4 participants