Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support by sfarestam-iproov · Pull Request #3286 · aws/aws-lc

sfarestam-iproov · 2026-06-03T14:58:55Z

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 is ec_group_set_a_mont() for setting an arbitrary Montgomery-form a coefficient, since Brainpool curves have a ≠ -3 (unlike NIST curves) and a ≠ 0 (unlike secp256k1). The existing ec_GFp_mont_dbl() already handles this case correctly via its a_is_minus3 == 0 code path.

NIDs (925, 927, 929, 931, 933) and OIDs were already registered in nid.h and obj_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:

FIPS boundary: The code is in crypto/fipsmodule/ec/. If adding curves inside the FIPS module is a concern for recertification, the implementation can be moved to crypto/ec_extra/. Please advise on the preferred placement.
Arbitrary a coefficient: Added a small helper ec_group_set_a_mont() (6 lines) to copy a Montgomery-form a value. This is the only new function beyond the existing curve template pattern.
make_tables.go changes: Added a curveWithA wrapper type and writeCurveDataWithA() function to emit MontA constants for curves where a is not -3 or 0. The standard elliptic.CurveParams in Go doesn't carry an A field (it assumes a = -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-trip i2d_ECPKParameters_bio / d2i_ECPKParameters_bio for all 5 curves.
GetNamedCurve: dispatches Brainpool curve names for file-based test vectors.
Scalar base multiplication test vectors are not included in this PR (generating them requires a Go Brainpool library); they can be added as a follow-up.

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

github-actions · 2026-06-03T15:08:11Z

+    // Sign a digest.
+    std::vector<uint8_t> sig(ECDSA_size(key.get()));
+    unsigned sig_len;
+    ASSERT_TRUE(


warning: variable 'sig_len' is not initialized [cppcoreguidelines-init-variables]

crypto/fipsmodule/ec/ec_test.cc:2829:

- len; + len = 0;

Fixed — initialized to 0. Thanks.

codecov-commenter · 2026-06-09T13:23:02Z

Codecov Report

❌ Patch coverage is 92.89617% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.43%. Comparing base (8cbf357) to head (a6b406d).

Files with missing lines	Patch %	Lines
crypto/fipsmodule/ec/ec_test.cc	78.72%	10 Missing ⚠️
crypto/fipsmodule/evp/evp.c	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3286      +/-   ##
==========================================
+ Coverage   78.22%   78.43%   +0.21%     
==========================================
  Files         695      696       +1     
  Lines      124281   124465     +184     
  Branches    17268    17280      +12     
==========================================
+ Hits        97214    97626     +412     
+ Misses      26144    25915     -229     
- Partials      923      924       +1

☔ View full report in Codecov by Harness.
📢 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.

nebeid · 2026-06-22T19:09:06Z

Thank you @sfarestam-iproov for this contribution.

FIPS boundary placement — I'd lean toward moving this to crypto/ec_extra/ as you offered.
Known-answer test vectors — Unless I'm missing them, I don't see any KATs. The current tests (BrainpoolKeygenSignVerify, ECPKParmatersBio) are round-trip/self-consistency checks, which would pass even if the domain parameters were subtly off as long as they're internally consistent. Since hardcoded curve parameters are the core of this change, it'd be good to validate p/a/b/G/n and the generated tables against independent reference vectors (RFC 5639, Wycheproof, or another library). I'd suggest including these in this PR rather than as a follow-up.

sfarestam-iproov · 2026-06-23T13:15:32Z

Thank you for the review @nebeid — both changes are now addressed:

1. FIPS boundary placement (commit 4ee8115): Moved all 5 Brainpool DEFINE_METHOD_FUNCTION blocks from crypto/fipsmodule/ec/ec.c to crypto/ec_extra/ec_brainpool.c. The small helper functions (ec_group_init_static_mont, ec_group_set_a_mont) are duplicated in the new file rather than exposed via internal.h, to avoid widening the FIPS module's internal API surface.

2. Known-answer test vectors (commit 193a2a2): Added two independent validation layers:

BrainpoolDomainParamsRFC5639 (ec_test.cc): Extracts p/a/b/Gx/Gy/n from each curve's EC_GROUP (converting out of Montgomery form) and compares against the hex reference values from RFC 5639 Sections 3.2–3.7. This catches subtly wrong precomputed constants that self-consistency tests would miss.
WycheproofECDSABrainpool (evp_test.cc): Runs Wycheproof ECDSA verify vectors for all 5 curves (~22k lines of test vectors sourced from C2SP/wycheproof), validating that the precomputed multiplication tables produce correct point arithmetic. Follows the existing WycheproofECDSAsecp256k1 pattern.

…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)

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.

Brainpool curves are not FIPS-approved, so move their DEFINE_METHOD_FUNCTION definitions from crypto/fipsmodule/ec/ec.c to crypto/ec_extra/ec_brainpool.c. The helper functions ec_group_init_static_mont and ec_group_set_a_mont are duplicated (rather than exposed via internal.h) to avoid widening the FIPS module's internal API surface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Add two independent validation layers: 1. BrainpoolDomainParamsRFC5639 (ec_test.cc): extracts p/a/b/G/n from each curve's EC_GROUP and compares against RFC 5639 reference hex values. This catches subtly wrong Montgomery constants in builtin_curves.h that self-consistency tests would miss. 2. WycheproofECDSABrainpool (evp_test.cc): runs Wycheproof ECDSA verify vectors for all 5 curves (~22k lines of test vectors from C2SP/wycheproof), validating that the precomputed tables produce correct point arithmetic results. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

DEFINE_METHOD_FUNCTION (from delocate.h) relies on delocated BSS storage within bcm.o, so it cannot be used outside crypto/fipsmodule/. Replace with an equivalent CRYPTO_once-based lazy-init macro that works in both FIPS and non-FIPS builds. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Replace the 10 per-curve case arms in EC_GROUP_new_by_curve_name and _mutable with a single default fallthrough to ec_group_new_by_curve_name_nonfips(), defined in ec_extra/ec_brainpool.c. This keeps all Brainpool dispatch logic outside the FIPS boundary and ensures bcm.o grows by a fixed amount (one call site) rather than per-curve. Follows the evp_pkey_meth_find + non_fips_pkey_evp_methods pattern. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Run generate_build_files.py to include the 5 new Wycheproof ECDSA Brainpool vector files in the pre-generated test data archive used by no-Go CI toolchains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The EVP_PKEY_{get,set}1_tls_encodedpoint functions had a hardcoded allowlist of only four NIST curves. The underlying point encoding (EC_POINT_oct2point, EC_KEY_key2buf) is curve-agnostic, so this restriction was unnecessarily narrow. Add all remaining built-in curves (secp256k1 and the five Brainpool curves) to fix strongswan ECDH interoperability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Extend the existing EVP_PKEY_{get,set}1_tls_encodedpoint round-trip test to cover secp256k1 (Wycheproof vector) and brainpoolP256r1 (RFC 7027 Section A.1), pinning the curve allowlist added in the previous commit and exercising ECDH derivation for both curves. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

… macro - ec_asn1.c: "pull in all four" -> "pull in all ten" (now 10 curves) - ec_brainpool.c: align backslash continuations to column 79 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sfarestam-iproov · 2026-08-11T09:08:47Z

@manastasova Both nits addressed in e82b181.

The PR currently has a merge conflict against main — it's in generated-src/crypto_test_data.cc.tar.bz2 (binary file, both sides modified). Resolving it requires a rebase onto main followed by regenerating generated-src.

Since @justsmth already rebased the branch once, happy to defer to you on whether you'd like to handle this rebase or if I should do it on my end.

Resolve binary conflict in generated-src/crypto_test_data.cc.tar.bz2 by regenerating via util/generate_build_files.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

sfarestam-iproov · 2026-08-11T15:48:41Z

Resolved the merge conflict — merged upstream main and regenerated generated-src/crypto_test_data.cc.tar.bz2. PR is mergeable again. Both approvals were dismissed by the push, so re-review is needed when you get a chance.

sfarestam-iproov requested a review from a team as a code owner June 3, 2026 14:58

sfarestam-iproov had a problem deploying to manual-approval June 3, 2026 14:59 — with GitHub Actions Error

sfarestam-iproov had a problem deploying to manual-approval June 3, 2026 15:00 — with GitHub Actions Error

sfarestam-iproov mentioned this pull request Jun 3, 2026

Feature Request: Support for Brainpool Curves (RFC 5639) in TLS 1.2 and TLS 1.3 #2939

Open

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

sfarestam-iproov force-pushed the add-brainpool-curves branch from b17795a to 6f99c57 Compare June 3, 2026 15:58

sfarestam-iproov had a problem deploying to manual-approval June 3, 2026 15:59 — with GitHub Actions Error

justsmth requested review from dkostic and nebeid June 3, 2026 16:10

sfarestam-iproov had a problem deploying to manual-approval June 9, 2026 11:10 — with GitHub Actions Error

sfarestam-iproov had a problem deploying to manual-approval June 25, 2026 19:24 — with GitHub Actions Error

nebeid had a problem deploying to manual-approval June 25, 2026 21:30 — with GitHub Actions Error

nebeid had a problem deploying to manual-approval June 25, 2026 21:30 — with GitHub Actions Failure

nebeid previously approved these changes Jun 26, 2026

View reviewed changes

sfarestam-iproov and others added 9 commits July 27, 2026 09:19

Fix Brainpool curve ASN.1 serialization by adding to kAllGroups

d8caac3

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.

Regenerate generated-src for Brainpool test vectors

03bf94f

Run generate_build_files.py to include the 5 new Wycheproof ECDSA Brainpool vector files in the pre-generated test data archive used by no-Go CI toolchains. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

dougch mentioned this pull request Jul 31, 2026

ci: pin openssh required job #3387

Merged

3 tasks

manastasova reviewed Aug 10, 2026

View reviewed changes

Comment thread crypto/ec_extra/ec_asn1.c Outdated

Comment thread crypto/ec_extra/ec_brainpool.c

Fix nits: update kAllGroups comment count and align DEFINE_CURVE_DATA…

e82b181

… macro - ec_asn1.c: "pull in all four" -> "pull in all ten" (now 10 curves) - ec_brainpool.c: align backslash continuations to column 79 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

manastasova previously approved these changes Aug 11, 2026

View reviewed changes

Merge upstream main and regenerate generated-src

a6b406d

Resolve binary conflict in generated-src/crypto_test_data.cc.tar.bz2 by regenerating via util/generate_build_files.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

manastasova approved these changes Aug 11, 2026

View reviewed changes

nebeid approved these changes Aug 11, 2026

View reviewed changes

This was referenced Aug 12, 2026

Support explicit EC parameters for Brainpool curves pyca/cryptography#14905

Open

Add brainpoolP320r1 elliptic curve pyca/cryptography#14906

Open

justsmth mentioned this pull request Aug 12, 2026

Add brainpool EC_group symbols; make public API symbol registration self-service #3423

Merged

Navigation Menu

Uh oh!

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support - #3286

Add brainpoolP224r1, brainpoolP256r1, brainpoolP320r1, brainpoolP384r1, brainpoolP512r1 EC group support#3286
justsmth merged 11 commits into
aws:mainfrom
sfarestam-iproov:add-brainpool-curves

sfarestam-iproov commented Jun 3, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot Jun 3, 2026

Uh oh!

sfarestam-iproov Jun 3, 2026

Uh oh!

codecov-commenter commented Jun 9, 2026 •

edited

Loading

Uh oh!

nebeid commented Jun 22, 2026

Uh oh!

sfarestam-iproov commented Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

sfarestam-iproov commented Aug 11, 2026

Uh oh!

sfarestam-iproov commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

sfarestam-iproov commented Jun 3, 2026

Issues:

Description of changes:

Call-outs:

Testing:

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

sfarestam-iproov Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nebeid commented Jun 22, 2026

Uh oh!

sfarestam-iproov commented Jun 23, 2026

Uh oh!

Uh oh!

Uh oh!

sfarestam-iproov commented Aug 11, 2026

Uh oh!

sfarestam-iproov commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jun 9, 2026 •

edited

Loading