-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add findex impl - client and server sides #15
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
Conversation
3fbfcb8
to
f8a13c0
Compare
let config = Configuration::Rest( | ||
findex_rest_client.client.clone(), | ||
findex_rest_client.server_url.clone(), | ||
findex_rest_client.server_url.clone(), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems strange to me.
If I follow you well, upon calling a CLI command, you call this function process with the client object (idk what it is). But since I doubt you serialize and store this client upon returning from the CLI, it means it is instantiated each time the CLI is called. Moreover, it seems to me that only one Findex operation can be performed per CLI invocation. Therefore I conclude that this clone is useless since the caller of process
will not reuse this FindexClient
.
Please, point to me where my reasoning is flawed.
More generally, I do not understand where the necessity of this FindexClient
object and this action mechanism comes from. Imho, the ideally simplistic flow is:
- user invokes the CLI with some arguments
- the CLI parses its conf/ENV variables to findex the server URL. If it cannot be found, it becomes a required argument.
- the CLI parses the provided arguments, returning an error upon reading an invalid one. There are 3 (or 4) possible commands:
search
,insert
,delete
(additionallycompact
). The validity of the rest of the argument depends on the identity of the command, and thus the main body of the CLI is just amatch
on the parsed command. - after parsing all arguments, if no error was thrown, then it means the parsed command will be performed: the CLI therefore instantiates a Findex object.
- The CLI runs the appropriate Findex command with its arguments, and returns the result to the user.
As I said, this is rather simplistic and may need to be developed, for example in order to add a non-Findex command like it seems you are planning to. However, I don't think we need dedicated types with their own proceed
functions and to pass around some FindexClient
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The FindexClient
is just a HTTPS Client which is initialized once and then pass to clap
actions. About this clone, your reasoning is correct, I've removed the shared reference and passed directly the object to be consumed by actions. Yes the CLI instantiates a HTTPS client each time and does not chain Findex operations (that's what we want).
About the CLI flow, since the 4 operations search
, add
, delete
and compact
have different arguments, the 4 corresponding process
functions (which are basic functions) are used:
- to parse those arguments
- to give them properly to Findex API
- to display to stdout the Findex output
@@ -21,11 +21,13 @@ use crate::{ | |||
#[derive(Clone)] | |||
pub struct FindexClient { | |||
pub server_url: String, | |||
client: Client, | |||
pub client: Client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this is just an HTTPS client. I would suggest not to store it in some FindexClient
. This would make everything simpler to read (this HTTPS client has nothing to do with Findex, and could be used by any other protocol built over HTTPS).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FindexClient
could be renamed at least I suppose
Great summary. Some questions:
|
@Manuthor some updates of the cosmian all big :
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following points were done with my commits, edit them according to nesecssituy
- Create a proper findex-server Auth0 tenant
- Configure it, create a test JWT token for user tech@cosmian.com
- Use this token in tests
Co-authored-by: Hatem M'naouer <19950216+HatemMn@users.noreply.github.com>
done |
* ci: fix * test: authentication failing * test: ignore auth tests * test: re-enable test server auth tests * test: re-enable client auth tests * test: disable client auth tests * test: do not clear db * chore: rename access to permission * refactor: check_permission * test: try mt tests * chore: remove all lints * test: remove dependency_on_unit_never_type_fallback * test: make default server port to 6666 * fix: lint
* feat: encrypt datasets - add routes and write on db * build: docker image * fix: permissions on datasets (add/del/get) * chore: split database traits * fix: reuse config_utils * feat: add list_permissions * fix: strip error conversions * ci: merge workflows between debug and release * docs: add main README * docs: add mkdocs * docs: small changes in mkdocs * chore: reduce verbosity * ci: include windows build by default * docs: fix menu * fix(config): use only toml format (client and server) * test: index a 3MB dataset * fix(redis): use pipe everywhere Redis needs to read/write * feat: support docker on arm cpu * ci: build ARM docker only on nightly build * fix: remove pandoc files * docs: add CLI usage * fix: use develop branch from http_client_server repo * ci(docker build): remove useless arg * fix: PR review * fix(only docs): PR review * test: add/delete/get datasets * fix: Redis writing concurrency (on permissions) and Windows tests (#19) * fix: windows tests * fix: clean permissions redis writing in server side * fix: make dataset redis writing sync * fix(redis): delete datasets * test: make consistent the Redis writing. Use pipe everywhere except transaction on permissions creation * fix: increase server waiting for redis to start * fix: move cli conf in client * feat: use Serializable trait * docs: add rust doc on database traits * fix: avoid a user to lookup who are the more powerful users * fix: database parameters not optional but with default values * fix: reenable cli auth tests * fix: remove useless r2d2 redis-feature and FindexRestClientResultHelper::context * fix: removing useless clippy::blocks_in_conditions * fix: remove uneeded RUSTSECs from deny.toml * ci: fix Github actions warnings * ci: fix Github actions warnings - download-artifact to v4 * fix: remove some todos * fix: remove ahash feature from redis crate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work, not much to say
crate/cli/src/actions/datasets.rs
Outdated
let mut encrypted_entries = HashMap::new(); | ||
for (key, value) in &self.entries { | ||
encrypted_entries.insert(*key, general_purpose::STANDARD.decode(value)?); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try with HashMap::with_capacity(self.entries.len())
(and optionally a fold
avoid declaring a mutable variable.
Are these entries already encrypted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thanks.
These entries are indeed already encrypted
let serialized = encrypted_entries.serialize()?; | ||
let deserialized = EncryptedEntries::deserialize(&serialized)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some serialization tests you can import from crypto core.
Prerequisites:
cloudproof_findex
: add a basic REST client Findex implementation (old Findex Cloud not reusable): feat(findex): add basic REST client instanciation - without Findex Cloud cloudproof_rust#72cloudproof_rust
for Findex v7.rest-interface
for the new REST basic interface (without the custom Findex CloudAuthorizationToken
). Old work on Findex Cloud is kept for non regression.Adapt othercloudproof_*
langages repos and make the CI OKfindex
repository:Release thefindex
7.0.0to be used in
cloudproof_rust`Tokens
,TokenWithEncryptedValueList
andTokenToEncryptedValueMap
: fix: Add missing serialization impls for structsTokens
,TokenWithEncryptedValueList
andTokenToEncryptedValueMap
. findex#88Eventually release the 6.0.1 to be used incloudproof_rust
Auth0
:findex-server
Auth0 tenanttech@cosmian.com
Client:
login
andlogout
server-version
KEM: RFC5649 (done by KMS with ckms)
DEM: AES GCM (done locally by ckms)
Merge CLI inReexposeckms
ckms
in new repo https://github.com/Cosmian/cli. Same forcosmian_findex_cli
Server:
[ ] SqliteWon't be done for Findex v6 versionID
ID
[ ] Convert TOML conf in JSONWon't be done unless a real need occursCloses #1
Closes #4
Closes #6
Closes #9
Closes #3