Fix for ML-KEM PKESK and adding PQC CLI tests #2221

falko-strenzke · 2024-04-25T10:27:31Z

Fix for ML-KEM PKESK decryption failure.

Added PQC CLI tests that are executed only for RNP build that supports PQC.

codecov · 2024-04-25T11:06:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.66%. Comparing base (a421aa3) to head (19df9df).
Report is 17 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2221   +/-   ##
=======================================
  Coverage   84.65%   84.66%           
=======================================
  Files         116      116           
  Lines       23422    23426    +4     
=======================================
+ Hits        19829    19833    +4     
  Misses       3593     3593

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

falko-strenzke · 2024-05-29T08:05:50Z

The only failing runners are those that use Botan head. The same applies to the PR #2240. So this is apparently not an error in our code.

@ni4 Is it possible that you review this PR in the weeks to come? We have a few other updates in order to make RNP conforming to the PQC draft version 03, see pqc-thunderbird/rnp#69.

@TJ-91

ni4 · 2024-05-30T14:12:46Z

@falko-strenzke Yeah, Botan head introduced changes to FFI aead handling, which is not compatible with our understanding of how it should work :) I'm working on a fix.

Sure, I'll review. Feel free to re-ping me if I'll dive into something other.

falko-strenzke · 2024-08-14T14:08:18Z

@ni4 I just wanted to ask if you currently have a time plan when you will be able to work on this PR. We have one other open PR and a number of other feature branches (listed in this wiki page ), for which we would also make PRs as soon as the existing ones move forward.

ni4 · 2024-08-14T15:05:06Z

@falko-strenzke sorry, I postponed this because of the issue on vcpkg side, which causes CI to fail (and we cannot merge with failing CI): microsoft/vcpkg#40276
Was trying to workaround it but didn't succeed, will make another attempt if they do not solve it timely.

TJ-91 · 2024-09-23T12:46:52Z

@ni4 now all checks pass, can you check if it's ready to merge?

ni4

Please see the review comments.

ni4 · 2024-09-23T14:03:22Z

src/lib/pgp-key.cpp

@@ -471,6 +471,19 @@ find_suitable_key(pgp_op_t op, pgp_key_t *key, rnp::KeyProvider *key_provider, b
        if (!cur || !cur->usable_for(op)) {
            continue;
        }
+#if defined(ENABLE_PQC)
+        /* prefer PQC over non-PQC. Assume non-PQC key is only there for backwards


I don't think we should force PQC here without any flag set by the user. Imagine somebody generated PQC subkey, but still has to continue communicate with people whose software doesn't support PQC yet. This should be done via some flag or something like this, set via rnp_op_encrypt (or just ignored for now).

This implementation choice is due to the PQC draft stating in 8.1 Key Preferences:

When encrypting to a certificate that has both a valid PQ/T and a valid traditional encryption subkey, an implementation SHOULD use the PQ/T subkey only.

The idea is that PQC encryption is favored in order to mitigate store-now-decrypt-later type attacks and the key has an ECC component to hedge against ML-KEM being broken.

If I'm not mistaken, the code will also prefer PQC signature subkeys. For this scenario, I share your concerns, and it should not be chosen by default. It seems I overlooked this when writing the code. Would you be ok with changing the code such that it only prefers PQC-encryption subkeys but not signature subkeys? This should not cause any interoperability problems since in that scenario both the sender and recipient support PQC and other recipients are not affected by the PKESK (it's not addressed to them and they should ignore it).

@TJ-91 I still think that it's better to leave this decision to the caller in both casses, probably introducing flag like RNP_KEY_PREFER_PQC to the rnp_key_get_default_key().

@ni4 Ok. It's now implemented for the encryption case only, since I think it doesn't make too much sense for signatures. I also added a test for it.

ni4 · 2024-09-23T14:04:14Z

src/tests/cli_tests.py

@@ -398,6 +409,7 @@ def rnp_encrypt_and_sign_file(src, dst, recipients, encrpswd, signers, signpswd,
    if ret != 0:
        raise_err('rnp encrypt-and-sign failed', err)

+


Do we need this empty line here?

TJ-91 · 2024-09-27T07:26:52Z

@ni4 sorry for bothering you with this. Before adding the last commit, all checks passed. Now codecov says, there is less code coverage: codecov/project Failing after 1s — 83.99% (-0.03%) compared to 47fee37 I'm afraid I don't really know how to read the output of that tool and make sense of it.

First, I don't add new untested cases in the new commit. You can check it yourself here: 8e59c0c . So it's unintuitive to me that adding this commit makes codecov fail now.

Second, when looking at https://app.codecov.io/gh/rnpgp/rnp/pull/2221, the overview of changed files suggest that the first 4 files change 0% and the last file actually changes +0.01%, meaning more coverage.

Third, in the indirect changes tab, I get a few lines that are not even remotely correlated to the patch but it seems to affect the code coverage for this patch negatively.

Fourth, the codecov report only lists 2 commits, not sure what this is about, since the PR has 16 commits.

All in all I'm confused and not sure how to proceed.

ni4 · 2024-09-27T07:37:19Z

@TJ-91 No worries, it's codecov who bothers, not you :) At some point I spent a while trying to understand it's behaviour and it's actually quite simple. The coverage decrease is caused by two reasons:

in cli_tests we generate random input for encryption/decryption, and in some cases it hits the bzip2 edge case, and in some cases (your one) - not. It should be fixed via the issue Add test case which covers bz->avail_out overflow. #2229
in CLI code we use function which returns number of days in the month, and once month changes some lines are getting uncovered. I noticed that yesterday and PR CI: update test cases with more date parsing cases #2275 must fix this.

Once these two issues are fixed I'll just ping you to rebase/repush, will this work for you?

TJ-91 · 2024-09-27T07:45:53Z

@ni4 yes, of course, thank you!

…key id

ni4

Please see the review comments. Not major ones, but still would be worth fixing.

ni4 · 2024-10-27T10:56:08Z

include/rnp/rnp.h

@@ -104,6 +104,9 @@ typedef uint32_t rnp_result_t;
 * Flags for default key selection.
 */
 #define RNP_KEY_SUBKEYS_ONLY (1U << 0)
+#if defined(RNP_EXPERIMENTAL_CRYPTO_REFRESH)


Should we use PQC define here?

ni4 · 2024-10-27T10:57:58Z

src/librepgp/stream-ctx.h

@@ -108,6 +110,9 @@ typedef struct rnp_ctx_t {
    bool           no_wrap{};   /* do not wrap source in literal data packet */
 #if defined(ENABLE_CRYPTO_REFRESH)
    bool enable_pkesk_v6{}; /* allows pkesk v6 if list of recipients is suitable */
+#endif
+#if defined(ENABLE_CRYPTO_REFRESH)


Should we use PQC define here?

ni4 · 2024-10-27T10:59:54Z

src/tests/cli_tests.py

+            if not found_this_entry:
+                raise RuntimeError("did not match the expected UI choice for algorithm: " + expected_line)
+
+    """ zzz_ prefix makes it the last test. This is a workaround against a gnupg import error with the


This should be better worked out by using the separate home folder, like it is done in test_subkey_binding_on_uid and others.

Good suggestion, I changed it

ni4 · 2024-10-27T11:00:24Z

src/tests/cli_tests.py

+                self.verify_pqc_algo_ui_nb_to_algo_ui_str(stdout, algo_ui_exp_strs)
+                verified_algo_nums = True
+
+        #gpg_import_pubring()


Do we need to have these?

You mean checking for the UI strings? The idea was that changing the UI will likely make this test not do what it should do, so we fail explicitly here

Nope, just commented out code lines.

Ok great, then I think it's ready. I just force-pushed a minor correction and hopefully the pipeline has no hiccups

ni4

LGTM, thanks!!

maxirmx

LGTM

ni4 · 2024-10-28T20:22:41Z

Having two approvals let's finally merge this. Thanks all!

falko-strenzke mentioned this pull request Apr 25, 2024

Overview of Branches pqc-thunderbird/rnp#69

Closed

falko-strenzke force-pushed the pqc-cli-tests branch from 3e12bb1 to 701c882 Compare September 19, 2024 13:46

ni4 self-requested a review September 23, 2024 13:37

ni4 requested changes Sep 23, 2024

View reviewed changes

TJ-91 force-pushed the pqc-cli-tests branch from 8e59c0c to b96809d Compare October 14, 2024 09:34

falko-strenzke mentioned this pull request Oct 22, 2024

Check our pqc-cli-tests PR is "green" pqc-thunderbird/rnp#85

Closed

falko-strenzke and others added 15 commits October 25, 2024 16:47

started PQC cli tests

e6f4095

pqc tests working (only keys WITH password protection)

6f3badd

removed some debug output

5767182

PQC tests now with and without passwd protected key

546af51

added algo param for SLH-DSA tests

4c0f6a2

add check for PQC support, add test to encrypt to v4 key

52cec8f

fix PKESKv6 PQC encryption case

0814ecb

corrected pqc test uids (informal only), added parsing of pqc enc sub…

0600c83

…key id

reverted changes to parse for pqc encryption subkey

c97a65f

fixed test data array, cli-encryption tests now running OK

c69a1d0

adjust find_suitable_key() to prefer PQC subkeys

ba7317a

edited only comment

2f6167f

testing aead algorithms in pqc public test

f3a9b52

checking expected algo ID choices in key gen UI

4a7e55c

removed type hint causing problems on some platforms

7035446

address review comments; add flag for preferring pqc encryption subkey

374cebc

TJ-91 force-pushed the pqc-cli-tests branch from b96809d to 374cebc Compare October 25, 2024 14:49

ni4 self-requested a review October 27, 2024 10:55

ni4 requested changes Oct 27, 2024

View reviewed changes

TJ-91 force-pushed the pqc-cli-tests branch from 9f1138e to 9bdb8ec Compare October 28, 2024 11:29

address review comments

19df9df

TJ-91 force-pushed the pqc-cli-tests branch from 9bdb8ec to 19df9df Compare October 28, 2024 12:57

ni4 approved these changes Oct 28, 2024

View reviewed changes

ni4 requested review from ronaldtse, maxirmx, antonsviridenko and desvxx October 28, 2024 13:52

maxirmx approved these changes Oct 28, 2024

View reviewed changes

ni4 merged commit 578968c into rnpgp:main Oct 28, 2024
124 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for ML-KEM PKESK and adding PQC CLI tests #2221

Fix for ML-KEM PKESK and adding PQC CLI tests #2221

falko-strenzke commented Apr 25, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading

falko-strenzke commented May 29, 2024

ni4 commented May 30, 2024

falko-strenzke commented Aug 14, 2024

ni4 commented Aug 14, 2024

TJ-91 commented Sep 23, 2024

ni4 left a comment

ni4 Sep 23, 2024

TJ-91 Sep 23, 2024

ni4 Sep 23, 2024

TJ-91 Sep 24, 2024

ni4 Sep 23, 2024

TJ-91 Sep 24, 2024

TJ-91 commented Sep 27, 2024

ni4 commented Sep 27, 2024

TJ-91 commented Sep 27, 2024

ni4 left a comment

ni4 Oct 27, 2024

ni4 Oct 27, 2024

ni4 Oct 27, 2024

TJ-91 Oct 28, 2024

ni4 Oct 27, 2024

TJ-91 Oct 28, 2024

ni4 Oct 28, 2024

TJ-91 Oct 28, 2024

ni4 left a comment

maxirmx left a comment

ni4 commented Oct 28, 2024

		@@ -398,6 +409,7 @@ def rnp_encrypt_and_sign_file(src, dst, recipients, encrpswd, signers, signpswd,
		if ret != 0:
		raise_err('rnp encrypt-and-sign failed', err)

Fix for ML-KEM PKESK and adding PQC CLI tests #2221

Fix for ML-KEM PKESK and adding PQC CLI tests #2221

Conversation

falko-strenzke commented Apr 25, 2024

codecov bot commented Apr 25, 2024 • edited Loading

Codecov Report

falko-strenzke commented May 29, 2024

ni4 commented May 30, 2024

falko-strenzke commented Aug 14, 2024

ni4 commented Aug 14, 2024

TJ-91 commented Sep 23, 2024

ni4 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TJ-91 commented Sep 27, 2024

ni4 commented Sep 27, 2024

TJ-91 commented Sep 27, 2024

ni4 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

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 left a comment

Choose a reason for hiding this comment

maxirmx left a comment

Choose a reason for hiding this comment

ni4 commented Oct 28, 2024

codecov bot commented Apr 25, 2024 •

edited

Loading