Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test kem interface against KATs #38

Merged
merged 20 commits into from
Nov 7, 2024
Merged

Conversation

Colfenor
Copy link
Owner

@Colfenor Colfenor commented Jan 24, 2023

this PR aims to close #37

  • Currently I went with the decision to test the kem feature inside the already existing functions of creating & verifying request/response files, augmenting the Testcase struct, without duplicating much code.

The consequences are that the test function needs to run with increased stack size & that I had to refactor out the katkem.rs functionality into lib.rs plus change the bash script which calls the test functions

I'm not quite sure if this is a great choice, however I couldn't achieve the desired kem testing otherwise since it's functions are private and feature restricted in lib.rs

TODO: update readme md5 hashes, and section about katkem

@Colfenor Colfenor requested review from dkales and prokls January 24, 2023 15:10
src/lib.rs Outdated

std::thread::Builder::new()
.stack_size(10 * 1024 * 1024)
.spawn(|| {
Copy link
Owner Author

@Colfenor Colfenor Jan 24, 2023

Choose a reason for hiding this comment

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

concerning the thread stack size increase, I attempted to write a small wrapper function using generics in order to just wrap the three KEM function calls and not the other code, however could not get it to work out, maybe you have some suggestions to fix that:

fn increase_stack_size_wrapper<F: Fn() -> R + Send>(function: F) {
    std::thread::Builder::new()
        .stack_size(10 * 1024 * 1024)
        .spawn(|| { 
            function
        })
        .unwrap()
        .join()
        .unwrap();
}

To provide a bit more context: the problem seems to be with setting the Result type of the generic function, since it needs to implement Box<dyn error::Error>> aswell as Send + Sync 🤔

@Colfenor Colfenor changed the title test kem interface against KATs Test kem interface against KATs Jan 24, 2023
@Colfenor Colfenor marked this pull request as ready for review March 5, 2023 22:12
Colfenor and others added 4 commits March 24, 2023 21:50
• dependencies are imported often w.r.t. some feature guards
• the use of dependency functionality is also feature guarded
• these should be the same set of feature guards
The updated version exposes the same API and runs the same set of tests,
but the tests have been moved to separate modules.

According to semver-checks, no API breakage was found after running:

  cargo semver-checks --only-explicit-features --baseline-rev main
                      --baseline-features mceliece348864
		      --current-features mceliece348864

Sadly, we cannot move these tests into the tests/ directory,
because the public API does not expose functionality like "fn verify".
As such, we cannot use verify from files in tests/.
Even if we migrate "fn verify" to tests/, there is still AesState.
Since AesState is used in so many place, we can only migrate
the tests to tests/ by code duplication (which I do not want to do).

Furthermore:

• Address TestData with `TestData` after an `use` statement
  instead of `crate::TestData` to make it easier to move TestData around
• a lot of feature guard fixes
• KATKEM tests pass for me with "ulimit -s 12048",
  but if you additionally pass feature="alloc" now,
  the public key will end up on the heap instead of the stack now.
• I ran all tests in all configurations and made sure no performance
  regressions occur.
@meisterluk
Copy link
Collaborator

All my performance and correctness tests passed now. My final latest commit tries to move out test functionality from src/lib.rs. I suggest merging it into the main branch and releasing the updated source code as a minor release.

Recognize that

  • @prokls is my deprecated account and registered as reviewer. If you need my acceptance, please remove prokls and add meisterluk.
  • The github CI instance does not respect our feature flags and criterion-cycles-per-byte does not seem to be compatible with macos. Thus it fails.

@meisterluk
Copy link
Collaborator

meisterluk commented Oct 27, 2024

I found one more cleanup: if there is a module-level feature guard, I remove the corresponding local feature guards. This makes the source code less redundant. No semantic difference are given.

@Colfenor Colfenor requested review from meisterluk and removed request for prokls October 27, 2024 17:51
@Colfenor Colfenor force-pushed the feature/test-kem-interface-vs-KATs branch from b293955 to 81cb9fc Compare November 2, 2024 23:03
@Colfenor Colfenor force-pushed the feature/test-kem-interface-vs-KATs branch from 81cb9fc to 4045c08 Compare November 2, 2024 23:22
Copy link
Collaborator

@meisterluk meisterluk left a comment

Choose a reason for hiding this comment

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

Thanks for catching the mceliece8192128f bug. Not sure how it slipped through.

@Colfenor
Copy link
Owner Author

Colfenor commented Nov 5, 2024

Thanks for catching the mceliece8192128f bug. Not sure how it slipped through.

No worries, I'm glad I could find it :)

@Colfenor Colfenor merged commit c792199 into main Nov 7, 2024
16 of 25 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.

Test the kem interface against KATs
2 participants