Refactor pgp_key_material_t to C++. #2250

ni4 · 2024-07-16T11:11:17Z

To avoid anonymous opaque unions and bring more flexibility to the codebase.

src/librepgp/stream-dump.cpp

+ !obj_add_mpi_json(jso, "g", eg.g(), ctx.dump_mpi) ||
+ !obj_add_mpi_json(jso, "y", eg.y(), ctx.dump_mpi)) {
+ return false; // LCOV_EXCL_LINE
 }
- break;
+ return true;
+ }
 case PGP_PKA_ECDSA:
 case PGP_PKA_EDDSA:
- case PGP_PKA_SM2: {
- const ec_curve_desc_t *cdesc = get_curve_desc(key.material.ec.curve);
- if (!obj_add_mpi_json(material, "p", key.material.ec.p, ctx->dump_mpi)) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
- }
- if (!json_add(material, "curve", cdesc ? cdesc->pgp_name : "unknown")) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
- }
- break;
- }
+ case PGP_PKA_SM2:
 case PGP_PKA_ECDH: {
- const ec_curve_desc_t *cdesc = get_curve_desc(key.material.ec.curve);
- if (!obj_add_mpi_json(material, "p", key.material.ec.p, ctx->dump_mpi)) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
+ auto &ec = dynamic_cast<const pgp::ECKeyMaterial &>(*material);
+ auto cdesc = get_curve_desc(ec.curve());
+ /* Common EC fields */
+ if (!obj_add_mpi_json(jso, "p", ec.p(), ctx.dump_mpi)) {
+ return false; // LCOV_EXCL_LINE
 }
- if (!json_add(material, "curve", cdesc ? cdesc->pgp_name : "unknown")) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
+ if (!json_add(jso, "curve", cdesc ? cdesc->pgp_name : "unknown")) {
+ return false; // LCOV_EXCL_LINE
 }
- if (!obj_add_intstr_json(
- material, "hash algorithm", key.material.ec.kdf_hash_alg, hash_alg_map)) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
+ if (material->alg() != PGP_PKA_ECDH) {
+ return true;
+ }
+ /* ECDH-only fields */
+ auto &ecdh = dynamic_cast<const pgp::ECDHKeyMaterial &>(*material);
+ if (!obj_add_intstr_json(jso, "hash algorithm", ecdh.kdf_hash_alg(), hash_alg_map)) {
+ return false; // LCOV_EXCL_LINE
 }
 if (!obj_add_intstr_json(
- material, "key wrap algorithm", key.material.ec.key_wrap_alg, symm_alg_map)) {
- return RNP_ERROR_OUT_OF_MEMORY; // LCOV_EXCL_LINE
+ jso, "key wrap algorithm", ecdh.key_wrap_alg(), symm_alg_map)) {
+ return false; // LCOV_EXCL_LINE
 }
- break;
+ return true;
 }
 #if defined(ENABLE_CRYPTO_REFRESH)
 case PGP_PKA_ED25519:
 case PGP_PKA_X25519:
 /* TODO */
- break;
+ return true;
 #endif
 #if defined(ENABLE_PQC)
 case PGP_PKA_KYBER768_X25519:
 FALLTHROUGH_STATEMENT;
 // TODO add case PGP_PKA_KYBER1024_X448: FALLTHROUGH_STATEMENT;
 case PGP_PKA_KYBER768_P256:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_KYBER1024_P384:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_KYBER768_BP256:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_KYBER1024_BP384:
 // TODO
- break;
+ return true;
 case PGP_PKA_DILITHIUM3_ED25519:
 FALLTHROUGH_STATEMENT;
 // TODO: add case PGP_PKA_DILITHIUM5_ED448: FALLTHROUGH_STATEMENT;
 case PGP_PKA_DILITHIUM3_P256:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_DILITHIUM5_P384:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_DILITHIUM3_BP256:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_DILITHIUM5_BP384:
 /* TODO */
- break;
+ return true;
 case PGP_PKA_SPHINCSPLUS_SHA2:
 FALLTHROUGH_STATEMENT;
 case PGP_PKA_SPHINCSPLUS_SHAKE:
 /* TODO */
- break;
+ return true;
 #endif
 default:
- break;
+ return false;
+ }


src/lib/key_material.cpp

src/librekey/key_store_g10.cpp

codecov · 2024-07-22T13:57:45Z

Codecov Report

Attention: Patch coverage is 83.64706% with 139 lines in your changes missing coverage. Please review.

Project coverage is 83.75%. Comparing base (e3662a7) to head (878e89b).
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/lib/key_material.cpp	81.40%	93 Missing ⚠️
src/lib/rnp.cpp	50.00%	16 Missing ⚠️
src/librekey/key_store_g10.cpp	86.66%	12 Missing ⚠️
src/librepgp/stream-dump.cpp	94.04%	5 Missing ⚠️
src/lib/key_material.hpp	84.61%	4 Missing ⚠️
src/lib/pgp-key.cpp	83.33%	4 Missing ⚠️
src/lib/crypto.cpp	60.00%	2 Missing ⚠️
src/librepgp/stream-key.cpp	88.23%	2 Missing ⚠️
src/lib/crypto/signatures.cpp	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2250      +/-   ##
==========================================
+ Coverage   83.58%   83.75%   +0.17%     
==========================================
  Files         107      113       +6     
  Lines       23163    23246      +83     
==========================================
+ Hits        19360    19470     +110     
+ Misses       3803     3776      -27

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/tests/key-protect.cpp

ni4 · 2024-07-26T13:39:27Z

@falko-strenzke @TJ-91 Possibly you'd want to take a look at this PR as it changes some of the PQC code.

falko-strenzke

A nice move towards more object orientation 👍 I left some comments.

falko-strenzke · 2024-07-29T06:35:20Z

src/lib/key_material.cpp

+}
+
+bool
+KeyMaterial::equals(const KeyMaterial &value) noexcept


I think this function should be const as well.

falko-strenzke · 2024-07-29T06:42:44Z

src/lib/key_material.hpp

+ void set_validity(const pgp_validity_t &val);
+ void reset_validity();
+ bool valid() const;
+ virtual bool equals(const KeyMaterial &value) noexcept;


Function hould be const

falko-strenzke · 2024-07-29T06:43:29Z

src/lib/key_material.hpp

+ virtual void clear_secret();
+ virtual bool parse(pgp_packet_body_t &pkt) noexcept = 0;
+ virtual bool parse_secret(pgp_packet_body_t &pkt) noexcept = 0;
+ virtual void write(pgp_packet_body_t &pkt) = 0;


Function should be `const` ?

falko-strenzke · 2024-07-29T06:44:26Z

src/lib/key_material.hpp

+ virtual void write(pgp_packet_body_t &pkt) = 0;
+ virtual void write_secret(pgp_packet_body_t &pkt) = 0;
+ virtual bool generate(const rnp_keygen_crypto_params_t &params);
+ virtual rnp_result_t encrypt(rnp::SecurityContext & ctx,


All crypto functions (en- decrypt, sign, verify) should be const, shouldn't they?
One exception could indeed be sign(), as for a stateful hash-based signature scheme the key would actually change during signing. However, stateful hash-based schemes have not been standardized for OpenPGP and most likely never will be.

Thanks for pointing at all this! Updated code with const, will push once addressed other comments.

falko-strenzke · 2024-07-29T06:54:46Z

src/lib/key_material.hpp

+#endif
+
+#if defined(ENABLE_PQC)
+class KyberKeyMaterial : public KeyMaterial {


Clearly, this class should be named to reflect the algorithm, for instance something like "KyberWithECDH". Otherwise, if pure Kyber/ML-KEM is ever added, there will be confusion and naming conflicts.

It might also be a good chance here to start introducing the NIST names, i.e., name the class "MLKEMWithECDH". We plan to rename the algorithms to the NIST names throughout the code-base as well once we reach a state where all PQC-related PRs merged.

Would MLKEMECDHKeyMaterial look good to you? Okay, it doesn't look good at all with 9 letters at start, but would it look comparable well as MLKEMWithECDHKeyMaterial? Just don't want to have too long names.

If you want to save the "with" I suggest to use MlkemEcdhKeyMaterial.

falko-strenzke · 2024-07-29T06:56:01Z

src/lib/key_material.hpp

+ const pgp_kyber_ecdh_composite_private_key_t &priv() const noexcept;
+};
+
+class DilithiumKeyMaterial : public KeyMaterial {


As mentioned for Kyber, also this class should be name "DilithiumWithECC" or similar.

Would DilithiumECCKeyMaterial look good to you?

But if you choose my suggestion for ML-KEM above, then it might be more consistent to use DilithiumEccKeyMaterial.

falko-strenzke · 2024-07-29T06:56:56Z

src/lib/key_material.hpp

+ const pgp_dilithium_exdsa_composite_private_key_t &priv() const noexcept;
+};
+
+class SphincsPlusKeyMaterial : public KeyMaterial {


Might want to call it "SLHDSAKeyMaterial".

Agree on this, thanks!

Also here, for a consistent camel case naming it could rather be SlhdsaKeyMaterial also.

@falko-strenzke Done, thanks for the suggestions!

ni4 · 2024-08-03T13:22:27Z

@ronaldtse @desvxx Ping for review.

ronaldtse

Thank you @ni4 !

github-advanced-security bot found potential problems Jul 16, 2024

View reviewed changes

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch from 55b0225 to 79bde7e Compare July 22, 2024 12:07

github-advanced-security bot found potential problems Jul 22, 2024

View reviewed changes

src/librekey/key_store_g10.cpp Fixed Show fixed Hide fixed

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch from 79bde7e to ac46b51 Compare July 22, 2024 13:40

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch 6 times, most recently from 6270dd6 to c5f6c60 Compare July 25, 2024 09:33

github-advanced-security bot found potential problems Jul 25, 2024

View reviewed changes

src/tests/key-protect.cpp Fixed Show fixed Hide fixed

src/tests/key-protect.cpp Fixed Show fixed Hide fixed

src/tests/key-protect.cpp Fixed Show fixed Hide fixed

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch 4 times, most recently from 36b9735 to f0d7691 Compare July 25, 2024 13:29

github-advanced-security bot found potential problems Jul 25, 2024

View reviewed changes

src/tests/key-protect.cpp Fixed Show fixed Hide fixed

src/tests/key-protect.cpp Fixed Show fixed Hide fixed

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch from 487f57e to bc71992 Compare July 26, 2024 12:48

ni4 marked this pull request as ready for review July 26, 2024 13:37

ni4 requested review from ronaldtse, antonsviridenko and desvxx July 26, 2024 13:38

antonsviridenko approved these changes Jul 28, 2024

View reviewed changes

falko-strenzke reviewed Jul 29, 2024

View reviewed changes

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch from bc71992 to e43ffcf Compare July 30, 2024 09:29

ni4 requested a review from falko-strenzke July 30, 2024 09:29

ni4 added 2 commits August 22, 2024 18:33

Add pgp::KeyMaterial classes.

3b1788b

Use pgp::KeyMaterial instead of pgp_key_material_t.

f222110

ni4 added 6 commits August 22, 2024 18:33

Fix bunch of compiler warnings.

36f34d2

Update g10 fuzzer with linking to Botan library.

fc355ba

Enable RNP logging to be able to troubleshoot Windows CI runner.

40d3b49

Remove code which may cause segfault from the tests.

962e9db

Update constness where applicable.

48f94da

Rename PQC classes.

878e89b

ni4 force-pushed the ni4-refactor-pgp_key_material_t branch from e43ffcf to 878e89b Compare August 22, 2024 15:33

ni4 requested review from maxirmx and falko-strenzke and removed request for falko-strenzke August 22, 2024 18:24

ronaldtse approved these changes Aug 24, 2024

View reviewed changes

ronaldtse merged commit 1aa98ed into main Aug 24, 2024
123 checks passed

ronaldtse deleted the ni4-refactor-pgp_key_material_t branch August 24, 2024 02:47

falko-strenzke mentioned this pull request Aug 26, 2024

Rebase our PRs to RNP upstream main pqc-thunderbird/rnp#78

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor pgp_key_material_t to C++. #2250

Refactor pgp_key_material_t to C++. #2250

ni4 commented Jul 16, 2024

codecov bot commented Jul 22, 2024 •

edited

Loading

ni4 commented Jul 26, 2024

falko-strenzke left a comment

falko-strenzke Jul 29, 2024

falko-strenzke Jul 29, 2024

falko-strenzke Jul 29, 2024

falko-strenzke Jul 29, 2024 •

edited

Loading

ni4 Jul 29, 2024

falko-strenzke Jul 29, 2024

ni4 Jul 29, 2024

falko-strenzke Jul 30, 2024

falko-strenzke Jul 29, 2024

ni4 Jul 29, 2024

falko-strenzke Jul 30, 2024

falko-strenzke Jul 29, 2024

ni4 Jul 29, 2024

falko-strenzke Jul 30, 2024

ni4 Jul 30, 2024

ni4 commented Aug 3, 2024

ronaldtse left a comment

Refactor pgp_key_material_t to C++. #2250

Refactor pgp_key_material_t to C++. #2250

Conversation

ni4 commented Jul 16, 2024

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

ni4 commented Jul 26, 2024

falko-strenzke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

falko-strenzke Jul 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni4 commented Aug 3, 2024

ronaldtse left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 22, 2024 •

edited

Loading

falko-strenzke Jul 29, 2024 •

edited

Loading