Implement more options for req CLI #2775

nhatnghiho · 2025-10-29T13:13:25Z

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.

github-actions

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.

tool-openssl/req.cc

tool-openssl/req_test.cc

codecov-commenter · 2025-10-29T13:48:28Z

Codecov Report

❌ Patch coverage is 35.73243% with 759 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.43%. Comparing base (93f9f73) to head (5d2bc29).

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.86%	116 Missing ⚠️
tool-openssl/test_util.cc	4.25%	88 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%   +0.01%     
==========================================
  Files         683      683              
  Lines      116286   117177     +891     
  Branches    16401    16477      +76     
==========================================
+ Hits        91197    91911     +714     
- Misses      24215    24381     +166     
- Partials      874      885      +11

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

github-actions

clang-tidy made some suggestions

tool-openssl/req_test.cc

github-actions

clang-tidy made some suggestions

tool-openssl/test_util.cc

tool-openssl/req.cc

…ional tests

justsmth

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.

tool-openssl/req.cc

justsmth · 2025-11-06T14:02:28Z

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 · 2025-11-10T13:43:39Z

tool-openssl/req_test.cc

+TEST_F(ReqTest, DefaultKeyoutPath) {
+  // Test that key is written to privkey.pem when -keyout is not specified


In some CI setups this test might be run in parallel, which creates a race condition since the "privkey.pem" created by one run could be deleted or overwritten by another.

WillChilds-Klein · 2025-11-10T18:07:57Z

tool-openssl/test_util.cc

+  // Check if keys are RSA type
+  if (EVP_PKEY_id(key1) != EVP_PKEY_RSA) {
+    std::cout << "AWS-LC key is not an RSA key" << std::endl;
+    return false;
+  }
+
+  if (EVP_PKEY_id(key2) != EVP_PKEY_RSA) {
+    std::cout << "OpenSSL key is not an RSA key" << std::endl;
+    return false;
+  }
+


aren't these unreachable due to L497?

WillChilds-Klein · 2025-11-10T18:10:55Z

tool-openssl/test_util.cc

+  // Check if keys are RSA type
+  if (EVP_PKEY_id(key1) != EVP_PKEY_RSA) {
+    std::cout << "AWS-LC key is not an RSA key" << std::endl;
+    return false;
+  }
+
+  if (EVP_PKEY_id(key2) != EVP_PKEY_RSA) {
+    std::cout << "OpenSSL key is not an RSA key" << std::endl;
+    return false;
+  }


also unreachable?

WillChilds-Klein · 2025-11-10T18:19:36Z

tool-openssl/test_util.cc

  X509_NAME *name2 = X509_REQ_get_subject_name(csr2);
-  if (X509_NAME_cmp(name1, name2) != 0) {
+
+  if (!name1 || !name2) {


don't you still need to call X509_NAME_cmp on the subject names?

WillChilds-Klein · 2025-11-10T18:23:46Z

tool-openssl/pkeyutl.cc

    bssl::UniquePtr<BIO> output_bio;
    if (out_path.empty()) {
-      output_bio.reset(BIO_new_fp(stdout, BIO_CLOSE));
+      output_bio.reset(BIO_new_fp(stdout, BIO_NOCLOSE));


here and elsewhere, why change this flag?

WillChilds-Klein · 2025-11-10T20:00:14Z

tool-openssl/req.cc

  // Parse keyspec
  if (OPENSSL_strncasecmp(keyspec, "rsa:", 4) == 0) {
    char *endptr = NULL;
+    errno = 0;


hm, it's not generally a good practice to set errno directly in the application layer

The <errno.h> header file defines the integer variable errno,
which is set by system calls and some library functions in the
event of an error to indicate what went wrong.

https://man7.org/linux/man-pages/man3/errno.3.html

why do we do so here?

WillChilds-Klein · 2025-11-10T20:05:48Z

tool-openssl/req.cc

+        "basicConstraints=critical,CA:true\n";
+
+    // Create a BIO for the config
+    bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(default_config, -1));


do we have an upper bound on the buffer size here? negative length is not encouraged for new usages

aws-lc/include/openssl/bio.h

Lines 461 to 463 in 981c79b

// If |len| is negative, then |buf| is treated as a NUL-terminated string, but

// don't depend on this in new code.

OPENSSL_EXPORT BIO *BIO_new_mem_buf(const void *buf, ossl_ssize_t len);

github-actions bot reviewed Oct 29, 2025

View reviewed changes

tool-openssl/req_test.cc Outdated Show resolved Hide resolved

nhatnghiho force-pushed the req-cli branch 2 times, most recently from ced2f39 to b504f7e Compare October 30, 2025 01:50

Implement more options for req cli

6a32876

nhatnghiho force-pushed the req-cli branch from b504f7e to 6a32876 Compare October 30, 2025 01:51

nhatnghiho marked this pull request as ready for review October 30, 2025 01:52

nhatnghiho requested a review from a team as a code owner October 30, 2025 01:52

github-actions bot reviewed Oct 30, 2025

View reviewed changes

tool-openssl/test_util.cc Show resolved Hide resolved

tool-openssl/test_util.cc Show resolved Hide resolved

justsmth reviewed Oct 30, 2025

View reviewed changes

nhatnghiho and others added 3 commits November 5, 2025 02:04

Incorporate feedback

f82cd36

Implement -passout to enable key encryption with cipher and add addit…

3eaeb13

…ional tests

Merge branch 'main' into req-cli

b6e1884

justsmth reviewed Nov 5, 2025

View reviewed changes

tool-openssl/req.cc Outdated Show resolved Hide resolved

nhatnghiho added 4 commits November 8, 2025 00:34

Force fail if -newkey is not RSA

1dd7b62

Fix failing tests and add unit tests

9f04f4a

Fix stdout bio error

0446518

Merge remote-tracking branch 'upstream/main' into req-cli

1220426

nhatnghiho force-pushed the req-cli branch from 2cd2db5 to 1220426 Compare November 8, 2025 02:47

Fix FIPS test failures

d89e588

justsmth reviewed Nov 10, 2025

View reviewed changes

justsmth force-pushed the req-cli branch 2 times, most recently from c577d68 to 7727746 Compare November 10, 2025 14:13

Fix race condition

5d2bc29

justsmth force-pushed the req-cli branch from 7727746 to 5d2bc29 Compare November 10, 2025 14:18

justsmth approved these changes Nov 10, 2025

View reviewed changes

WillChilds-Klein reviewed Nov 10, 2025

View reviewed changes

		TEST_F(ReqTest, DefaultKeyoutPath) {
		// Test that key is written to privkey.pem when -keyout is not specified

	// If \|len\| is negative, then \|buf\| is treated as a NUL-terminated string, but
	// don't depend on this in new code.
	OPENSSL_EXPORT BIO BIO_new_mem_buf(const void buf, ossl_ssize_t len);

Implement more options for req CLI #2775

Are you sure you want to change the base?

Implement more options for req CLI #2775

Conversation

nhatnghiho commented Oct 29, 2025

Issues:

Description of changes:

Testing:

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

justsmth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justsmth commented Nov 6, 2025

Uh oh!

justsmth Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

WillChilds-Klein Nov 10, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Oct 29, 2025 •

edited

Loading