Improve verify and x509 CLI compatibility - #3548
Merged
Merged
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
|
🔒 Security Review — View Report Please review before merging. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3548 +/- ##
==========================================
+ Coverage 78.24% 78.35% +0.11%
==========================================
Files 700 700
Lines 126282 126759 +477
Branches 17440 17501 +61
==========================================
+ Hits 98810 99327 +517
+ Misses 26594 26543 -51
- Partials 878 889 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
justsmth
force-pushed
the
openssl-verify-x509-compat
branch
from
September 18, 2026 16:44
1fe0699 to
07ed70a
Compare
justsmth
requested review from
WillChilds-Klein,
samuel40791765 and
skmcgrail
and removed request for
samuel40791765
September 18, 2026 17:56
skmcgrail
previously approved these changes
Sep 18, 2026
justsmth
enabled auto-merge (squash)
September 18, 2026 21:42
gtest has no printer for the VerifyTrustCase parameter struct, so it falls
back to hex-dumping the object's raw bytes when naming each TEST_P instance.
The struct has four bytes of tail padding that the aggregate initializers
never write, and Valgrind reports the snprintf("%02X") over them as a use of
uninitialised values, failing every valgrind shard that runs the suite.
Define PrintTo so gtest prints the fields instead, which also makes the
parameterized test names readable.
The file loop printed the error queue after each failed check() so the next iteration's ERR_clear_error() would not discard it, while the stdin path relied on tool.cc printing the queue after exit. The two paths were equivalent in effect but looked asymmetric. Move the per-input reporting into check() itself, matching the structure of OpenSSL's verify: clear the queue, run the check, and print any errors on failure. Both input paths now share it and the caller loop returns to a one-liner.
justsmth
force-pushed
the
openssl-verify-x509-compat
branch
from
September 21, 2026 16:16
5317370 to
b351fb8
Compare
Pre-C++11 glibc only defines the PRI* macros for C++ when __STDC_FORMAT_MACROS is set, so the powerpc-old-glibc build failed with "expected ')' before 'PRIx32'". Format the hash as unsigned long instead, which is what tool-openssl/verify_test.cc already does for the same value.
createTempDirPath creates its directory under /tmp, which an Android app process cannot write to, so the test fails on every Device Farm device and takes the rest of the android-omnibus matrix down with it via fail-fast. Skip it on Android as the other temp-file tests already do.
WillChilds-Klein
approved these changes
Sep 21, 2026
Comment on lines
+208
to
212
| bssl::UniquePtr<BIO> input(BIO_new_fp(stdin, BIO_NOCLOSE)); | ||
| if (!input) { | ||
| return 0; | ||
| } | ||
| cert.reset(PEM_read_bio_X509(input.get(), nullptr, nullptr, nullptr)); |
Contributor
There was a problem hiding this comment.
nit: looks like this case is not covered by tests
skmcgrail
approved these changes
Sep 21, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Context and motivation
Scripts written against OpenSSL's
verifyandx509break on the AWS-LC tool:verifyrequires-CAfileand lacks-CApath/-purpose/-no-CAfile/-no-CApath/-verbose;x509 -subjectprintsCN = valueinstead ofCN=value, defeating the commonsed 's/.*CN=//'idiom;-nameoptis rejected; and-textprints the CT SCT-list extension as a raw OID. Separately,X509_LOOKUP_hash_dirsplits directory lists on:on Windows, mangling drive letters.Description of changes
verify:-CAfileis optional and falls back to the default trust store (SSL_CERT_FILE/SSL_CERT_DIR). Adds-CApath,-no-CAfile,-no-CApath,-purpose, and-verbose(no-op). Explicit paths replace their defaults;-no-*disables a default only.-purposealso selects the matching trust type, soTRUSTED CERTIFICATEtrust/reject attributes are honored as in OpenSSL. Exit codes follow OpenSSL: 1 for setup errors (including a non-directory-CApath, checked up front sinceX509_LOOKUP_add_dirdefers filesystem access), 2 for any load/verify failure. Per-input errors print as they occur.x509:-subjectdefaults toCN=value. New-nameoptaccepts the presetscompat,oneline,RFC2253,multiline; when given it also applies to-text's Issuer/Subject lines.-nameopt onelinerestores the old-subjectformat.NID_ct_precert_scts(1.3.6.1.4.1.11129.2.4.2) so-textlabels it "CT Precertificate SCTs". Name only; no SCT parsing. Generated files regenerated viacrypto/obj/objects.go.by_dir.c: use;as the list separator on Windows, matching OpenSSL and the existingX509_LOOKUP_add_dirheader comment.test_util.h:ExecuteCommandre-quotes commands on Windows socmd.exedoesn't strip the outer quotes.Testing
New tests:
ObjTest.CTPrecertSCTs;X509Test.HashDir*;VerifyChainTest.*andVerifyTrustTest.*(generated root/intermediate/leaf chain covering-CApath,-purpose, trust/reject attributes,-no-*with env-var defaults, exit codes, per-input errors, plus OpenSSL comparisons);X509Test.Subject*/Text*/Nameopt*(exact output per preset checked against OpenSSL 3.6.4, thesedCN idiom on a multi-CN/multi-valued-RDN subject, control-char escaping, SCT label vs. an unregistered OID).Affected
crypto_test/tool_openssl_testfilters pass on macOS (vs OpenSSL 3.6.4), Ubuntu 24.04 x86-64 (vs 3.0.13, also under Valgrind), and Windows Server 2022/MSVC 19.40 (vs 3.6.3).Review considerations
-nameoptaccepts only the four presets, not OpenSSL's comma-separated flags (oneline,-esc_msbnow exits 1). This is the one existing command line that could newly fail.-subjectoutput changes fromCN = valuetoCN=value, matching OpenSSL 3.x. Default-textoutput is unchanged (stillX509_print), so it does not pick up OpenSSL 3.x's UTF-8 rendering; switching it is a possible follow-up.1.3entry swap inkNIDsInOIDOrder(obj_dat.h) is regeneration noise fromobjects.go's unstable sort over two identical OIDs, not a change here.NID_/SN_/LN_/OBJ_ct_precert_sctsdefines.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.