-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: add findex impl - client and server sides #15
base: develop
Are you sure you want to change the base?
Conversation
3fbfcb8
to
f8a13c0
Compare
#[clap(long, short = 'k')] | ||
pub key: String, |
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.
Since we don't want the key to be client-side, this should be replaced by a JWT, or at least an enum Key | JWT
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.
Do you mean the Findex key only persist in a KMS?
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.
Exactly. We need to decide whether we want to allow both flows or only the KMS-as-crypto-oracle one. Supporting both will need to switch to Findex-v7 which is more modular.
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
|
||
/// The url of the database for findex-redis | ||
#[clap( | ||
long, | ||
env = "FINDEX_SERVER_DATABASE_URL", | ||
required_if_eq_any([("database_type", "redis-findex")]) | ||
required_if_eq_any([("database_type", "redis-findex")]), | ||
default_value = "redis://localhost:6379" |
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.
I wouldn't use a default value here, but return an error upon failure to parse this information.
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 idea was to give the format of the URL redis://XXXX:6379
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 [email protected]
- Use this token in tests
Co-authored-by: Hatem M'naouer <[email protected]>
done |
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.
Test
* 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
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.cloudproof_*
langages repos and make the CI OKfindex
repository:findex
7.0.0to be used in
cloudproof_rust`Tokens
,TokenWithEncryptedValueList
andTokenToEncryptedValueMap
: fix: Add missing serialization impls for structsTokens
,TokenWithEncryptedValueList
andTokenToEncryptedValueMap
. findex#88cloudproof_rust
Auth0
:findex-server
Auth0 tenant[email protected]
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