Refactor key generation classes #2284

ni4 · 2024-10-21T10:54:30Z

This PR refactors key generation classes.

src/lib/key_material.cpp

src/lib/keygen.cpp

codecov · 2024-10-21T11:32:38Z

Codecov Report

Attention: Patch coverage is 87.06564% with 67 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (135775a) to head (b5b6de2).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/keygen.cpp	81.63%	36 Missing ⚠️
src/lib/rnp.cpp	87.34%	21 Missing ⚠️
src/lib/key_material.cpp	84.00%	8 Missing ⚠️
src/lib/json-utils.cpp	97.72%	1 Missing ⚠️
src/lib/pgp-key.cpp	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2284      +/-   ##
==========================================
+ Coverage   84.67%   84.81%   +0.14%     
==========================================
  Files         116      116              
  Lines       23436    23292     -144     
==========================================
- Hits        19844    19755      -89     
+ Misses       3592     3537      -55

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

ni4 · 2024-11-01T09:47:10Z

@ronaldtse @desvxx @maxirmx ping for review!

maxirmx · 2024-11-01T10:55:18Z

This one is significant
I will find a couple of hours to understand itearly next week

ni4 · 2024-11-01T11:28:01Z

@maxirmx Thanks! Wanna make another also large PR, based on this one.

desvxx · 2024-11-03T22:48:40Z

src/lib/key_material.cpp

 {
-    if (dsa_generate(&params.ctx->rng, &key_, params.dsa.p_bitlen, params.dsa.q_bitlen)) {
+    auto &dsa = dynamic_cast<const DSAKeyParams &>(params);


Maybe I'm missing something, but is it ok to use just static_cast here?

The idea was to avoid theoretical possibility of getting wrong object here by throwing exception. Not sure how that could happen, just to be cautious.

desvxx · 2024-11-03T22:53:09Z

src/lib/key_material.hpp

+    size_t qbits_;
+
+  public:
+    DSAKeyParams();


qbits_ init missing

It is actually initialized in constructor implementation, but let's move it to the header for better clarity, thanks.

desvxx · 2024-11-03T23:06:00Z

src/lib/keygen.hpp

+    pgp_pubkey_alg_t                alg_;
+    pgp_hash_alg_t                  hash_;
+    pgp_version_t                   version_;
+    SecurityContext &               ctx_;


Risky solution imho (to use reference here)

SecurityContext is a singleton which lives the whole life of the top-level rnp_ffi_t object, and it would be destroyed after all the internal stuff. Is this good enough or there could be other solution?

if it is singleton, I would do
static SecurityContext& SecurityContext::getContext()
and do not keep the reference

But this is probably not inportant

desvxx · 2024-11-03T23:12:35Z

src/lib/pgp-key.cpp

@@ -2618,14 +2525,14 @@ pgp_key_t::add_sub_binding(pgp_key_t &                       subsec,
    pgp_signature_t sig;
    sign_init(ctx.rng, sig, hash, ctx.time(), version());
    sig.set_type(PGP_SIG_SUBKEY);
-    if (binding.key_expiration) {
-        sig.set_key_expiration(binding.key_expiration);
+    if (binding.expiration) {


May be confusing - "what expiration?". IMHO keep key_expiration will be better.

Thanks, renamed.

desvxx · 2024-11-03T23:23:37Z

src/tests/cipher.cpp

-    key_desc.rsa.modulus_bit_len = 1024;
-    key_desc.ctx = &global_ctx;
+    rnp::KeygenParams keygen(PGP_PKA_RSA, global_ctx);
+    auto &            rsa = dynamic_cast<pgp::RSAKeyParams &>(keygen.key_params());


weird spacing:)

Yeah, unfortunately that's how clang-format 11 behaves. Don't like these as well.

maxirmx

OK

maxirmx · 2024-11-04T22:56:28Z

src/lib/keygen.hpp

+    pgp_pubkey_alg_t                alg_;
+    pgp_hash_alg_t                  hash_;
+    pgp_version_t                   version_;
+    SecurityContext &               ctx_;


if it is singleton, I would do
static SecurityContext& SecurityContext::getContext()
and do not keep the reference

But this is probably not inportant

ni4 · 2024-11-05T12:34:09Z

@maxirmx thanks for the review. Having static variable would have issues with multi-threaded applications, which now could use at least separate rnp_ffi_t objects in different threads.

…ucts.

ni4 · 2024-11-10T13:52:34Z

Merging with two approvals and green CI. Thanks!

ni4 force-pushed the ni4-refactor-keygen-classes branch 4 times, most recently from de160d5 to 1aaa2e6 Compare October 21, 2024 11:18

github-advanced-security bot found potential problems Oct 21, 2024

View reviewed changes

src/lib/key_material.cpp Dismissed Show dismissed Hide dismissed

src/lib/key_material.cpp Dismissed Show dismissed Hide dismissed

src/lib/keygen.cpp Dismissed Show dismissed Hide dismissed

ni4 force-pushed the ni4-refactor-keygen-classes branch 3 times, most recently from 83d4c18 to f7971e7 Compare October 22, 2024 16:14

ni4 requested review from ronaldtse, maxirmx, antonsviridenko and desvxx October 23, 2024 08:50

desvxx approved these changes Nov 3, 2024

View reviewed changes

ni4 force-pushed the ni4-refactor-keygen-classes branch from 5b04e51 to 43d5524 Compare November 4, 2024 11:08

maxirmx approved these changes Nov 5, 2024

View reviewed changes

ni4 added 10 commits November 10, 2024 13:01

Add classes rnp::KeygenParams and pgp::KeyParams.

44aab3a

Add some more JSON helper functions.

0fb4046

Add function merge_defaults() to pgp_user_prefs_t.

2789642

Update KeygenParams class with key generation functionality.

68ba868

Use KeygenParams class for the key generation.

5725ba8

Remove old code, related to key generation.

ae2cb63

Remove no longer used files from the codebase.

0cae376

Fix missing dsa.h include.

7484cca

Add classes rnp::CertParams and rnp::BindingParams instead of old str…

1824458

…ucts.

Add class rnp::UserPrefs to replace old struct.

a61305e

ni4 added 5 commits November 10, 2024 13:01

Remove redundant fields from the pgp_subsig_t.

1cd74bb

Fix GCC warnings.

ca4589e

Use std::vector instead of raw pointer in pgp_userid_pkt_t.

b3ddf7f

Move DSAKeyParams constructor to header for better clarity.

9051257

Rename CertParams/BindingParams expiration property to key_expiration.

b5b6de2

ni4 force-pushed the ni4-refactor-keygen-classes branch from 43d5524 to b5b6de2 Compare November 10, 2024 11:01

ni4 merged commit 0fc76d5 into main Nov 10, 2024
124 checks passed

ni4 deleted the ni4-refactor-keygen-classes branch November 10, 2024 13:52

ni4 mentioned this pull request Jan 7, 2025

Refactor out crypto.cpp #2006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor key generation classes #2284

Refactor key generation classes #2284

ni4 commented Oct 21, 2024

codecov bot commented Oct 21, 2024 •

edited

Loading

ni4 commented Nov 1, 2024

maxirmx commented Nov 1, 2024

ni4 commented Nov 1, 2024

desvxx Nov 3, 2024

ni4 Nov 4, 2024

desvxx Nov 3, 2024

ni4 Nov 4, 2024

desvxx Nov 3, 2024

ni4 Nov 4, 2024

maxirmx Nov 4, 2024

desvxx Nov 3, 2024

ni4 Nov 4, 2024

desvxx Nov 3, 2024

ni4 Nov 4, 2024

maxirmx left a comment

maxirmx Nov 4, 2024

ni4 commented Nov 5, 2024

ni4 commented Nov 10, 2024

Refactor key generation classes #2284

Refactor key generation classes #2284

Conversation

ni4 commented Oct 21, 2024

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

ni4 commented Nov 1, 2024

maxirmx commented Nov 1, 2024

ni4 commented Nov 1, 2024

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

maxirmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ni4 commented Nov 5, 2024

ni4 commented Nov 10, 2024

codecov bot commented Oct 21, 2024 •

edited

Loading