Skip to content

Conversation

@maxtropets
Copy link
Collaborator

@maxtropets maxtropets commented Nov 3, 2025

Resolves #6673

  • Break RSA->EC inheritance
  • Rename PublicKey -> ECPublicKey, KeyPair -> ECKeyPair, etc
  • No more common public key, ccf::crypto::KeyVariant provided for explicit handing of different key types

✅ Long Test passed
☑️ AB perf: no regression

Mainly involved changes in RSA, other survived with no functional changes, the interface surface remains the same (except naming).

This's how it looks for RSA (and others, with probably extra non-exported files for historical reasons)

    ccf::crypto::RSAPublicKey          ccf::crypto::RSAKeyPair
               |                                  |
               |                                  |
               v                                  |
    RSAPublicKey_OpenSSL                          |
               |                                  |
               |                                  v
               └─────────────────────→  RSAKeyPair_OpenSSL
                                         (private_openssl)

@maxtropets maxtropets self-assigned this Nov 3, 2025
@maxtropets maxtropets force-pushed the f/openssl-refactor-interface branch from 2f07a97 to 751f3e5 Compare November 4, 2025 10:40
@maxtropets maxtropets force-pushed the f/openssl-refactor-interface branch from 751f3e5 to a7a4c47 Compare November 4, 2025 11:42
@maxtropets maxtropets added run-long-test Run Long Test job bench-ab labels Nov 4, 2025
@maxtropets maxtropets changed the title [DRAFT] Pandora box, do not open Revisit CCF crypto interfaces (mainly RSA/EC keys) Nov 4, 2025
@maxtropets maxtropets changed the title Revisit CCF crypto interfaces (mainly RSA/EC keys) Revisit CCF crypto interfaces (mostlyRSA/EC keys) Nov 4, 2025
@maxtropets maxtropets changed the title Revisit CCF crypto interfaces (mostlyRSA/EC keys) Revisit CCF crypto interfaces (mostly RSA/EC keys) Nov 4, 2025
@maxtropets maxtropets marked this pull request as ready for review November 4, 2025 15:23
@maxtropets maxtropets requested a review from a team as a code owner November 4, 2025 15:23
Copilot AI review requested due to automatic review settings November 4, 2025 15:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the crypto API to explicitly separate EC (Elliptic Curve) and RSA key types, improving type safety and API clarity. The main changes include renaming PublicKey to ECPublicKey and KeyPair to ECKeyPair, removing problematic inheritance between RSA and EC key types, and introducing a KeyVariant type for handling different key types explicitly.

  • Renamed core EC crypto types: PublicKeyECPublicKey, KeyPairECKeyPair
  • Separated RSA and EC key type hierarchies to eliminate error-prone inheritance
  • Introduced KeyVariant template for explicit handling of different key types
  • Updated RSA API to use explicit padding parameters and unified method names

Reviewed Changes

Copilot reviewed 75 out of 75 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
include/ccf/crypto/key_variant.h New file introducing KeyVariant template for handling different key types
include/ccf/crypto/ec_key_pair.h Renamed from key_pair.h, now specifically for EC keys
include/ccf/crypto/ec_public_key.h Renamed PublicKey class to ECPublicKey
include/ccf/crypto/rsa_key_pair.h Updated RSA API to inherit from RSAPublicKey, unified method names
include/ccf/crypto/rsa_public_key.h Added RSAPadding enum, removed inheritance, unified verify API
include/ccf/crypto/verifier.h Updated to use KeyVariant for storing different key types
src/crypto/verifier.cpp Implemented verify methods with KeyVariant handling
src/crypto/openssl/ec_key_pair.* Renamed from key_pair.*, implementation for EC keys
src/crypto/openssl/ec_public_key.* Renamed from public_key.*, implementation for EC public keys
src/crypto/openssl/rsa_public_key.* Refactored to remove inheritance, unified verify API with padding
src/crypto/openssl/rsa_key_pair.* Updated to inherit from RSAPublicKey_OpenSSL
src/crypto/key_pair.cpp Deleted, moved to ec_key_pair.cpp
src/crypto/rsa_key_pair.cpp Deleted, moved into openssl/rsa_key_pair.cpp
src/endpoints/authentication/jwt_auth.cpp Updated to use ECPublicKey and new RSA verify API
src/js/extensions/ccf/crypto.cpp Updated to use make_ec_public_key and unified JWK methods
doc/build_apps/crypto.rst Updated documentation to reflect new type names
CHANGELOG.md Added entry documenting the breaking API changes

maxtropets and others added 2 commits November 4, 2025 15:29
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@maxtropets maxtropets removed run-long-test Run Long Test job bench-ab labels Nov 5, 2025
@maxtropets maxtropets force-pushed the f/openssl-refactor-interface branch from 4b97eb3 to b66413f Compare November 5, 2025 12:58
@maxtropets maxtropets added run-long-test Run Long Test job and removed run-long-test Run Long Test job labels Nov 5, 2025
@maxtropets maxtropets added the run-long-test Run Long Test job label Nov 11, 2025
@maxtropets maxtropets removed the run-long-test Run Long Test job label Nov 12, 2025
Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
@achamayou achamayou enabled auto-merge (squash) November 13, 2025 14:10
@maxtropets maxtropets disabled auto-merge November 13, 2025 14:15
@maxtropets maxtropets enabled auto-merge (squash) November 13, 2025 14:29
@maxtropets maxtropets merged commit a94b745 into microsoft:main Nov 13, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix RSAPublicKey hierarchy and hidden virtual functions

3 participants