Fix for ML-KEM PKESK and adding PQC CLI tests

[falko-strenzke]

Fix for ML-KEM PKESK decryption failure.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.99%. Comparing base (47fee37) to head (8e59c0c).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2221      +/-   ##
==========================================
- Coverage   84.02%   83.99%   -0.03%     
==========================================
  Files         114      114              
  Lines       23062    23066       +4     
==========================================
- Hits        19377    19374       -3     
- Misses       3685     3692       +7     

Flag	Coverage Δ
	83.99% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[falko-strenzke]

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]

@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]

@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]

@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]

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

[ni4]

Please see the review comments.

[ni4]

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

[TJ-91]

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

[ni4]

@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().

[TJ-91]

@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]

Do we need this empty line here?

[TJ-91]

fixed

[TJ-91]

@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]

@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]

@ni4 yes, of course, thank you!