Pre-release v 0.4. #90

HatemMn · 2025-07-07T13:21:54Z

Please do not merge this.

Current release workflow does not imply to review whole develop to main. It should be done before - for each PR to avoid massive PR review (like this).

However thanks for the review but requested changes must be handled in separated PRs or postponed in other issues.

* fix: make it compile * fix: update cli commit * fix: clippy * fix: cli * fix: cli last fixes * fix: last fixes * fix: last fixes2 * fix: last fixes3 * fix: update submodule commit * fix: use crypto core (#74) * fix: finish using crypto core * fix: add fixes3 * fix: add fixes4 * fix: add fixes 5 * fix: add fixes 5

fix: fix the test

* chore: bump Findex to 7.1.0 * fix: use last published crates from KMS * fix: align toolchain version with KMS and CLI repos * fix: make clippy happy

* feat: init trait feat: table creation OK findex trait OK feat: improve db error management feat: permissions done feat: finish permission and dataset code, adapt codebase, no tests refactor: table names + all tests pass except last one fix: a lot of fixes idk fix: delegation refactor: huge db refactoring feat: add specialised db errors refactor: huge macros refactoring chore: a LOT of linting chore: most of linting finished feat: updta ecargo tolm style: last linting fix: an update fix: put back connections feat: update cli commit revert: cli commit fix: point to latest develop cli feat: implement instantiation function fix: fix instantiation trait fix: lint the code fix: run cargo update ? fix: revert lock fix: review fixes fix: some fixes chore: update cli fix: update name revert fix: update name import fix: rebase fixes fix: dependency 157413e fix: rebase fixes fix: update cli feat: some updates * fix: critical bugfix for dataset SQL script * feat: add variant counting for tests only refactor: refactor toml tests fix:update cli commit fix: clippy fix: upd cli fix: upd cargo lock revert: cli build all feat: warmup connexions for the slow mac on the ci feat: isolate the test again feat: push db warmup to the maximim feat: push higher warmup feat: update powershell script to skip redis and run the rest feat: isolate more feat: isolate even more feat: isolate change stuff feat: put bac ccr test cli feat: end rocky linux bloody hell feat: rock clli fix: last rocky fixes fix: try an in_memory approach fixes fix: rocky final fix: put ccr tests malone fix: lint fix: lint2 fix: indefinite busy handler fix: add backoff + windows more test windows? fixed the mac for good fixed the mac for good2 feat: use a barrier feat: use a barrier2 fix: some fixes feat: use a barrier3 fix: post cleanup fixes fix: cargo fmt fix: quickfix * fix: more windows? * fix: more windows part 2 * fix: try build all modif * fix: put db elsewhere * fix: another windows friendly bugfix * fix: windows PLEASE$ * fix:upda cli comm * fix: bye redis * fix: sequentialise sqlite permission tests * fix: restaure non debug code * test(windows): add logs * test(windows): skip kms tests * chore: update cli submodule --------- Co-authored-by: Emmanuel Coste <[email protected]>

* chore: move findex clap actions in this repo * ci: take develop branch for reusable workflows --------- Co-authored-by: phochard <[email protected]>

* test: simplify RestClient instantiation * test: reenable concurrent test on Findex encryption layer * test: try fix on test_findex_concurrent_read_write * test: exclude test on windows * test: set GW concurrent number of threads to 20

* fix: invert fips feature * feat: remove crypto_core and cover_crypt where it is possible * fix: windows build * fix: PR review, remove useless error forwarding * fix: use develop branch for github workflow * chore: bump KMS crate version to develop

Copilot

Pull Request Overview

This PR prepares for the v0.4 pre-release by adding test fixtures, certificate generation scripts, updating toolchain/config, and extending the testing harness and data structures.

Add plain text fixtures, CSV dataset, and client/server certificates for integration tests
Provide generate_certs.sh scripts under test_data and test_data/certificates
Introduce the test_findex_server crate to launch Findex servers in tests (with TLS & auth)
Polyfill a Value type in crate/structs for compatibility with Findex v7

Reviewed Changes

Copilot reviewed 142 out of 155 changed files in this pull request and generated 2 comments.

File	Description
test_data/client_server/generate_certs.sh	Script to generate CA and TLS certs for client/server tests
deny.toml	Updated license allowlist for v0.4 release (duplicate entry)
crate/test_findex_server/src/test_server.rs	Added helpers to start Findex test server with TLS/auth
crate/structs/src/findex/value.rs	Introduced `Value` struct polyfill for v7 compatibility

Comments suppressed due to low confidence (4)

test_data/certificates/client_server/README.md:5

The placeholder <[email protected]> does not match the actual CN *.acme.com used by the scripts; update the example to avoid confusion.

- a client certificate to authenticate to the server with CN being <[email protected]>

crate/structs/src/findex/value.rs:1

The new Value type lacks unit tests; consider adding serialization and conversion tests to ensure correct behavior.

//! This module is a polyfill for the Value struct that was deleted from findex v7.0.0.

test_data/client_server/user/user.client.acme.com.key:1

Committing private keys exposes sensitive data and risks misuse; consider excluding these keys from source control or generating them at test runtime.

-----BEGIN PRIVATE KEY-----

deny.toml:107

The CDLA-Permissive-2.0 entry appears twice in allow; remove the duplicate to keep the list concise.

  "CDLA-Permissive-2.0",

test_data/client_server/generate_certs.sh

crate/test_findex_server/src/test_server.rs

tbrezot

Stopping review here: a lot of work and clean-up to do before being release-ready.

.rustfmt.toml

crate/cli/Cargo.toml

crate/findex_client/src/datasets.rs

crate/findex_client/Cargo.toml

tbrezot · 2025-07-07T18:58:07Z

crate/server/Cargo.toml

 [dev-dependencies]
-rand = "0.9.0"
 tempfile = { workspace = true }
+variant_count = "1.1"


What is this dependency used for?

This file : crate/server/src/tests/mod.rs

And then this test :

Test will be eliminated if we take off that dev dep ( but it's a dev dep, so no problem ig )

tbrezot · 2025-07-07T18:58:30Z

crate/server/Cargo.toml

-] }
 thiserror = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
 toml = "0.8"


It seems we have two different dependencies to manage configuration. Maybe just pick one.

IMO both are needed and server different configurations :

toml configs are for human users, actual users on linux computers

env configs are typically for machines and automated CI flows (while those can use TOML, it's technically time consuming with no gain on doing it)

correct me if I'm wrong @Manuthor

While I'm confirming what you are saying @HatemMn , I do not understand the @tbrezot comment. Is it about sqlx?

tbrezot · 2025-07-07T19:00:48Z

crate/server/src/config/command_line/db.rs

+// This creates the database if it does not exist, and tries to open it if it does
+fn ensure_sqlite_db(database_url: &str, alternate_env_variable: &str) -> FResult<PathBuf> {
+    let path = &retrieve_database_location(database_url, alternate_env_variable)?;
+    drop(


Why is this explicit call to drop required?

to say in a clear verbose manner that we do not want that connection to exist, as per the comment, the goal is ensuring we can open the DB

Well this connection is dropped since it goes out of scope. Do you really need to be that verbose ?

I tried to change but I remembered the compiler prefers the call to drop, too :

crate/server/src/database/database_traits.rs

tbrezot · 2025-07-07T19:06:55Z

crate/server/src/database/database_traits.rs

+    where
+        Self: Sized;
+}
 #[allow(dead_code)] // false positive, used in crate/server/src/database/redis/mod.rs


If there really is a false-positive, then this is a clippy bug: report it and link the issue instead. From what I see this is not a false positive, but the current state of the repo is so messy that it's hard to tell it's not used (I tried to remove plus some other things it and it builds).

I can't build without this trait, and deleting that "allow dead code" raises multiple warnings

it's quite probably a clippy issue as you said, but the repo is big and I think putting this line of code is more effective than submitting issues to the clippy te that will need them a fork our codebase in order to debug (we're hopefully open source)

HatemMn

thank you for this deep analysis ! two major points from my end :

I opened a new pr with my fixes : #91 . If you have new comments, please add them on the 91 ! not this PR, in order to maintain the classic git flow and avoid savage commits on the develop branc
I co authored those edits with @Manuthor , hence, about half the reviewed code wasn't mine. I tried to fix the "obvious" comments + the technical ones related to my own code. For those of Manu, I marked them with the eyes emoji ( 👀 ) so @Manuthor please address those comments ( no rush :) ! ) and when you finish **please commit on the new PR #91 **

crate/cli/Cargo.toml

crate/findex_client/src/datasets.rs

crate/findex_client/Cargo.toml

crate/findex_client/src/datasets.rs

crate/findex_client/src/findex_rest_client.rs

HatemMn · 2025-07-14T15:18:30Z

crate/server/Cargo.toml

 alcoholic_jwt = "4091"
-async-trait = "0.1.86"
+async-trait = "0.1.88"
+async-sqlite = { workspace = true }


If I understood the comment correctly, yes and to find the code you need to search the library is called using and underscore _ not the hyphen -

If this wasn't the question, then yes (for example, check windows CI )

HatemMn · 2025-07-14T15:23:08Z

crate/server/Cargo.toml

+async-trait = "0.1.88"
+async-sqlite = { workspace = true }
 chrono = "0.4"
 clap = { workspace = true, features = [


hoping I say no mistake, to my understanding, some of the server parameters ( see crate/server/src/config/command_line/db.rs or also crate/server/src/config/command_line/http_config.rs ) are also CLI parameters at the same time, hence the import

HatemMn · 2025-07-14T15:26:00Z

crate/server/Cargo.toml

-] }
 thiserror = { workspace = true }
 tokio = { workspace = true, features = ["full"] }
 toml = "0.8"


IMO both are needed and server different configurations :

toml configs are for human users, actual users on linux computers

env configs are typically for machines and automated CI flows (while those can use TOML, it's technically time consuming with no gain on doing it)

correct me if I'm wrong @Manuthor

crate/server/src/database/database_traits.rs

HatemMn · 2025-07-14T15:38:04Z

crate/server/src/database/database_traits.rs

+    where
+        Self: Sized;
+}
 #[allow(dead_code)] // false positive, used in crate/server/src/database/redis/mod.rs


I can't build without this trait, and deleting that "allow dead code" raises multiple warnings

it's quite probably a clippy issue as you said, but the repo is big and I think putting this line of code is more effective than submitting issues to the clippy te that will need them a fork our codebase in order to debug (we're hopefully open source)

tbrezot · 2025-07-15T09:03:39Z

crate/structs/src/findex/value.rs

Use define_byte_type! instead. And what is a polyfill?

Edit: Okay, so you actually cannot use this macro since your type is not constant-size. Which means this is different from the values defined in Findex. I therefore don't understand this doc at all. Please remove or amend it.

polyfill is a web dev term and maybe has no place here - best definition I found was " workaround software implementing backward compatibility of functionality added to a programming interface in earlier versions of that interface "

anyway

When we use findex-server to search for a Hashset<stuff> , we call those values a "Value" like defined in value.rs

It was the terminology with findex v7.0 and when migrating I have deleted the non necessary "capabilities" of the values and left the bare minimum. I will update the doc, but it felt weird to say that a Value is "a searched value" - i will try to better articulate it

tbrezot · 2025-07-15T09:07:39Z

crate/structs/src/findex/keywords.rs

 /// A [`Keyword`] is a byte vector used to index other values.
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]


Maybe we ought to have a macro to generate this kind of implementations.

after a quick search I didn't find code duplication of this part that would motivate the usage of macros

tbrezot · 2025-07-15T09:23:04Z

crate/server/src/routes/findex.rs

-    let mut server_address = Address::<{ SERVER_ADDRESS_LENGTH }>::default();
+    let mut server_address = Address::<{ SERVER_ADDRESS_LENGTH }>::from([0; SERVER_ADDRESS_LENGTH]);


Prepending your index ID statically reveals a lot of information. If this is required or decide to proceed with it anyway, you may as well instantiate one Redis per tenant. This would enforce isolation without allocating, reduce the storage cost and allow for a trivial implementation of per-tenant back-up.

tbrezot · 2025-07-15T09:25:47Z

crate/server/src/main.rs

        unsafe {
            std::env::set_var(
                "RUST_LOG",
-                "info,cosmian=info,cosmian_findex_server=info,actix_web=info,sqlx::query=error,\
-                 mysql=info",
+                "info,cosmian=info,cosmian_findex_server=info,actix_web=info",
            );
        }


Unsafe code really is not welcome in a security-focused implementation: is there really no way to do without it?

Manuthor · 2025-07-18T06:20:57Z

thank you for this deep analysis ! two major points from my end :

* I opened a new pr with my fixes : #91 . If you have new comments, please add them on the 91 ! not this PR, in order to maintain the classic git flow and avoid savage commits on the develop branc

* I co authored those edits with @Manuthor , hence, about half the reviewed code wasn't mine. I tried to fix the "obvious" comments + the technical ones related to my own code. For those of Manu, I marked them with the eyes emoji ( 👀  ) so @Manuthor please address those comments ( no rush :) ! ) and when you finish **please commit on the new PR #91 **

Ok thanks for this.

I've created issues and answered to some comments. For the time being, I won't go any further since created issues are not blocking the next release.
Please clean all PRs on Findex in order to release the Findex v8. This PR can be closed.

HatemMn · 2025-07-21T13:24:57Z

I finished double checking the comments, we can safely close this PR I guess

Manuthor and others added 21 commits April 11, 2025 15:19

chore: Merge branch 'release/0.3.0' into develop

0130916

chore: update cli submodule

734150b

ci: fix missing attached assets on GH release

eef9a30

chore: use cosmian published crates

688bb40

ci: reuse GitHub workflow to publish artifacts

1d63435

fix: fix the test

d0a8152

Merge pull request #78 from Cosmian/fix/ccr_test_fix

a73aef0

fix: fix the test

chore: bump Findex to 7.1.0 (#82)

80b1b66

* chore: bump Findex to 7.1.0 * fix: use last published crates from KMS * fix: align toolchain version with KMS and CLI repos * fix: make clippy happy

fix: SQLITE_TEST_DB_URL path

effd476

refactor: move clap actions from client repo (#83)

2c94796

* chore: move findex clap actions in this repo * ci: take develop branch for reusable workflows --------- Co-authored-by: phochard <[email protected]>

chore: bump cosmian_kms_cli to 5.3

60a291b

fix: reexport kms_cli from findex_cli

971ea31

fix: cargo fmt

82bfba2

fix: remove packaging of cli

2f40765

test: add helper for RestClient instantiation

802dcb4

fix: guarded write concurrent test (#88)

ce91a83

* test: simplify RestClient instantiation * test: reenable concurrent test on Findex encryption layer * test: try fix on test_findex_concurrent_read_write * test: exclude test on windows * test: set GW concurrent number of threads to 20

fix: Invert fips feature (#89)

8a4e839

* fix: invert fips feature * feat: remove crypto_core and cover_crypt where it is possible * fix: windows build * fix: PR review, remove useless error forwarding * fix: use develop branch for github workflow * chore: bump KMS crate version to develop

chore: bump some crates version

f8617d9

fix: add description on public functions

a082e0c

HatemMn requested review from bgrieder, Copilot and tbrezot July 7, 2025 13:22

Copilot AI reviewed Jul 7, 2025

View reviewed changes

test_data/client_server/generate_certs.sh Show resolved Hide resolved

crate/test_findex_server/src/test_server.rs Show resolved Hide resolved

tbrezot requested changes Jul 7, 2025

View reviewed changes

HatemMn commented Jul 14, 2025

View reviewed changes

tbrezot reviewed Jul 15, 2025

View reviewed changes

HatemMn mentioned this pull request Jul 16, 2025

feat: use Findex 7.1 and add SqliteMemory with MemoryADT impl #91

Merged

HatemMn closed this Jul 21, 2025

Copilot AI mentioned this pull request Aug 14, 2025

Implement Word struct wrapper for [u8; WORD_LENGTH] with base64 Display #112

Open

		/// A [`Keyword`] is a byte vector used to index other values.
		#[derive(Clone, Debug, Hash, PartialEq, Eq)]

		let mut server_address = Address::<{ SERVER_ADDRESS_LENGTH }>::default();
		let mut server_address = Address::<{ SERVER_ADDRESS_LENGTH }>::from([0; SERVER_ADDRESS_LENGTH]);

Pre-release v 0.4. #90

Pre-release v 0.4. #90

Uh oh!

Conversation

HatemMn commented Jul 7, 2025 • edited by Manuthor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please do not merge this.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

tbrezot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HatemMn Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HatemMn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbrezot Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manuthor commented Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

HatemMn commented Jul 7, 2025 •

edited by Manuthor

Loading

HatemMn Jul 16, 2025 •

edited

Loading

tbrezot Jul 15, 2025 •

edited

Loading

Manuthor commented Jul 18, 2025 •

edited

Loading