-
Notifications
You must be signed in to change notification settings - Fork 191
Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support#3286
Open
sfarestam-iproov wants to merge 2 commits into
Open
Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support #3286sfarestam-iproov wants to merge 2 commits into
sfarestam-iproov wants to merge 2 commits into
Conversation
@sfarestam-iproov
sfarestam-iproov
requested a review
from a team
as a code owner
June 3, 2026 14:58
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 14:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:00 — with
GitHub Actions
Error
Contributor
There was a problem hiding this 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
// Sign a digest.
std::vector<uint8_t> sig(ECDSA_size(key.get()));
unsigned sig_len;
ASSERT_TRUE(
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'sig_len' is not initialized [cppcoreguidelines-init-variables]
crypto/fipsmodule/ec/ec_test.cc:2829:
- len; + len = 0;
Author
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — initialized to 0. Thanks.
...1, brainpoolP512r1 EC group support Add all five Brainpool r1 prime curves from RFC 5639 using the generic Montgomery arithmetic path (EC_GFp_mont_method), following the same pattern as secp256k1. No hand-optimized assembly is needed. The only structural addition beyond the secp256k1 template is ec_group_set_a_mont() for setting an arbitrary Montgomery-form a coefficient (Brainpool curves have a != -3 and a != 0). The existing ec_GFp_mont_dbl() already handles this case correctly. NIDs were already registered in nid.h. This change adds the EC_GROUP definitions, the precomputed Montgomery constants (via make_tables.go), the NID-to-group switch cases, and the public API functions. Domain parameters sourced from: - brainpoolP224r1: RFC 5639, Section 3.3 - brainpoolP256r1: RFC 5639, Section 3.4 - brainpoolP320r1: RFC 5639, Section 3.5 - brainpoolP384r1: RFC 5639, Section 3.6 - brainpoolP512r1: RFC 5639, Section 3.7 OIDs from: RFC 5639, Section 4.1 These curves are needed for: - ICAO 9303 ePassport certificate processing (30+ countries) - BSI TR-03116-4 compliance (German federal regulation) - pyca/cryptography Brainpool support (blocked on aws-lc, see aws#2939) Resolves aws#2939 (EC primitive support; TLS negotiation can follow separately)
@sfarestam-iproov
sfarestam-iproov
force-pushed
the
add-brainpool-curves
branch
from
June 3, 2026 15:58
b17795a to
6f99c57
Compare
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 3, 2026 15:59 — with
GitHub Actions
Error
The kAllGroups array in ec_asn1.c is used by EC_KEY_parse_curve_name and EC_get_builtin_curves to look up curves by OID. Without the Brainpool entries, i2d/d2i_ECPKParameters_bio round-trips fail with EC_R_UNKNOWN_GROUP.
@sfarestam-iproov
sfarestam-iproov
had a problem deploying
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Error
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
@sfarestam-iproov
sfarestam-iproov
requested a deployment
to
manual-approval
June 9, 2026 11:10 — with
GitHub Actions
Waiting
codecov-commenter
commented
Jun 9, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #3286 +/- ## ========================================== - Coverage 78.17% 78.15% -0.02% ========================================== Files 689 689 Lines 123733 123872 +139 Branches 17197 17204 +7 ========================================== + Hits 96724 96817 +93 - Misses 26091 26136 +45 - Partials 918 919 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Issues:
Resolves #2939
Description of changes:
AWS-LC currently does not support Brainpool elliptic curves. This PR adds EC group support for the five Brainpool r1 prime curves defined in RFC 5639: brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, and brainpoolP512r1.
The implementation follows the same pattern as secp256k1 — generic Montgomery arithmetic via
EC_GFp_mont_method(), with no hand-optimized assembly. The only structural addition isec_group_set_a_mont()for setting an arbitrary Montgomery-formacoefficient, since Brainpool curves havea ≠ -3(unlike NIST curves) anda ≠ 0(unlike secp256k1). The existingec_GFp_mont_dbl()already handles this case correctly via itsa_is_minus3 == 0code path.NIDs (925, 927, 929, 931, 933) and OIDs were already registered in
nid.handobj_dat.h. All domain parameters are sourced from RFC 5639 Sections 3.3–3.7, with OIDs from Section 4.1.This PR covers EC primitive support (key generation, ECDSA, ECDH). TLS negotiation support (RFC 7027 / RFC 8734) can follow in a separate PR.
Motivation: These curves are required by ICAO Doc 9303 (ePassport standard, 30+ countries), BSI TR-03116-4 (German federal regulation), and are blocking the pyca/cryptography project from accepting Brainpool improvements (pyca/cryptography#14905).
Call-outs:
crypto/fipsmodule/ec/. If adding curves inside the FIPS module is a concern for recertification, the implementation can be moved tocrypto/ec_extra/. Please advise on the preferred placement.acoefficient: Added a small helperec_group_set_a_mont()(6 lines) to copy a Montgomery-formavalue. This is the only new function beyond the existing curve template pattern.make_tables.gochanges: Added acurveWithAwrapper type andwriteCurveDataWithA()function to emitMontAconstants for curves whereais not -3 or 0. The standardelliptic.CurveParamsin Go doesn't carry anAfield (it assumesa = -3).Testing:
BrainpoolKeygenSignVerify: generates a key, signs a digest with ECDSA, verifies the signature, and checks that a corrupted signature is rejected — for all 5 curves.ECPKParmatersBio: round-tripi2d_ECPKParameters_bio/d2i_ECPKParameters_biofor all 5 curves.GetNamedCurve: dispatches Brainpool curve names for file-based test vectors.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.