Add brainpool EC_group symbols; make public API symbol registration self-service - #3423
Merged
Merged
Conversation
The EC_group_brainpoolP*r1 functions added in aws#3286 were never added to the libcrypto symbol registry, so "local: *;" in crypto/libcrypto.map demoted all five to local symbols in ENABLE_DIST_PKG builds. They are compiled into libcrypto, but applications cannot link against them: nm -D --defined-only libcrypto-awslc.so | grep brainpool # nothing nm libcrypto-awslc.so | grep brainpool # t EC_group_... Add them to the open AWS_LC_1.0 node and regenerate the version script, matching how EVP_AEAD_CTX_copy (aws#3332) and CRYPTO_cts128_* (aws#3308) were registered. This clears two checks that are currently red on main: the dist-pkg-install-tests-* silent-drop test and the libcrypto symbol check (baseline).
Registering new API required hand-editing the symbol registry, which the docs
tell contributors not to do. The command documented in tests/ci/README.md was
./util/update_symbol_version.sh AWS_LC_1.0
which always failed with "AWS_LC_1.0 already exists in the registry", because the
script could only open a new version node. There was no supported way to do what
every symbol addition so far has actually done: add to the node that is already
open. A contributor who hit the baseline check and followed the documented
remediation reached a dead end.
- update_symbol_version.sh now takes --current to add to the newest node, or a
version to open a new one. Exactly one is required, so the node is always an
explicit choice and never a silent default. Passing a node that already exists
points at --current rather than dead-ending.
- Re-sort the registry under LC_ALL=C. The committed registries are in byte
order, matching what generate_version_script emits; re-sorting under a UTF-8
locale rewrote the entire file and buried the real change.
- The dist-pkg silent-drop failure printed no remediation at all. It and the
baseline check now both name the command and explain the consequence: the
symbols are compiled in but cannot be linked against.
- Document that new API goes into the open node, that opening a node closes the
previous one and is a release-level decision, and describe the built-library
check, which was previously undocumented.
Contributor
|
🔒 Security Review — View Report Please review before merging. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3423 +/- ##
=======================================
Coverage 78.23% 78.24%
=======================================
Files 696 696
Lines 124462 124462
Branches 17275 17278 +3
=======================================
+ Hits 97371 97383 +12
+ Misses 26167 26157 -10
+ Partials 924 922 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
WillChilds-Klein
approved these changes
Aug 12, 2026
Comment on lines
+190
to
+193
| # The re-sort runs under LC_ALL=C so the registry stays in byte order. That is | ||
| # the order generate_version_script emits symbols in, and the order the | ||
| # committed registries are already in; sorting under a UTF-8 locale instead | ||
| # would silently rewrite the whole file and bury the real change in a |
Contributor
There was a problem hiding this comment.
nice. thanks for figuring this out!
WillChilds-Klein
enabled auto-merge (squash)
August 12, 2026 19:27
dougch
approved these changes
Aug 12, 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/Motivation
The five
EC_group_brainpoolP*r1()functions added in #3286 were never registered.OPENSSL_EXPORTsymbols that are not added to the symbol registry are compiled into shared libraries but hidden by the version script, preventing applications from linking against them.Description of changes:
This PR registers the Brainpool EC group APIs in
AWS_LC_1.0and makes the registration path when updating the current node explicit with./util/update_symbol_version.sh --current.The symbol-versioning tool now distinguishes adding API to the current open node from opening a new node, rejects accidental reuse of an existing version node, and uses a deterministic byte-order sort. Documentation and CI failure messages now explain the impact of unregistered symbols and how to remediate them.
Call-outs:
Opening a new symbol-version node is now documented and treated as a release-level decision. Most API additions should use
--current; providing a version creates a new node and closes the current one by convention.Testing:
git diff --checkBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.