Armin Preiml: 3 query: do not allow duplicate keys implement the strict query option (-s) enforce unique entries 17 files changed, 161 insertions(+), 28 deletions(-)
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~sircmpwn/himitsu-devel/patches/47086/mbox | git am -3Learn more about email & git
There are less edge cases in querying entries when disallowing duplicate keys in a query and subsequently an entry. For example to query an entry with the subset `a=a a=b`, the syntax should ideally support different matching rules for entries that have either value in `a` or have multiple `a` keys with specified values and other cases like having exact amounts of duplicates with specific values. Disallowing duplicates will avoid such cases. --- cmd/himitsud/cmd.ha | 11 ++++----- cmd/hiq/main.ha | 4 ++-- docs/himitsu.7.scd | 1 + himitsu/query/parse.ha | 54 ++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 60 insertions(+), 10 deletions(-) diff --git a/cmd/himitsud/cmd.ha b/cmd/himitsud/cmd.ha index de0d153..a30d724 100644 --- a/cmd/himitsud/cmd.ha +++ b/cmd/himitsud/cmd.ha @@ -14,7 +14,7 @@ use strings; use unix::poll::{event}; type servererror = !(io::error | fs::error | exec::error | secstore::error); -type cmderror = !(query::invalid | prompt::error | ...servererror); +type cmderror = !(query::error | prompt::error | ...servererror); fn strerror(err: cmderror) const str = { match (err) { @@ -28,8 +28,8 @@ fn strerror(err: cmderror) const str = { return prompt::strerror(err); case let err: secstore::error => return secstore::strerror(err); - case query::invalid => - return "Invalid query syntax"; + case let err: query::error => + return query::strerror(err); }; }; @@ -71,9 +71,8 @@ fn exec(serv: *server, client: *client, cmd: str) (void | servererror) = { return err; case let err: prompt::error => writefmt(client, "error {}", prompt::strerror(err)); - case query::invalid => - // XXX: Update me when there are more client errors - writefmt(client, "error {}", strerror(query::invalid)); + case let err: query::error => + writefmt(client, "error {}", strerror(err)); }; case void => yield; }; diff --git a/cmd/hiq/main.ha b/cmd/hiq/main.ha index 808c2f8..1f61ba5 100644 --- a/cmd/hiq/main.ha +++ b/cmd/hiq/main.ha @@ -97,8 +97,8 @@ export fn main() void = { const query = match (query::parse_items(cmd.args)) { case let q: query::query => yield q; - case query::invalid => - fmt::fatal("Invalid query"); + case let err: query::error => + fmt::fatal(query::strerror(err)); }; defer query::finish(&query); send(conn, op, &query, flags, field, one); diff --git a/docs/himitsu.7.scd b/docs/himitsu.7.scd index 2b79501..acc9f25 100644 --- a/docs/himitsu.7.scd +++ b/docs/himitsu.7.scd @@ -25,6 +25,7 @@ separated by *=* (the "equal" symbol), e.g. key=value, and each key/value pair is separated with spaces. Keys and values are formatted using shell quoting syntax, such that spaces or other special characters (non-alphanumeric) may appear in keys or values if they are quoted according to shell quoting rules. +Keys must be unique within an entry. Each key may be suffixed with a *!* to indicate that the value is secret. diff --git a/himitsu/query/parse.ha b/himitsu/query/parse.ha index 3bfe61d..95aa524 100644 --- a/himitsu/query/parse.ha +++ b/himitsu/query/parse.ha @@ -3,6 +3,7 @@ use encoding::utf8; use io; use regex; use shlex; +use sort::{sort}; use strings; use fmt; @@ -32,9 +33,25 @@ export type pair = struct { // Returned when the syntax of a query is not valid. export type invalid = !void; +// Returned when keys are not unique within query. +export type dupkeys = !void; + +// An error which may be returned form a query function +export type error = !(invalid | dupkeys); + +// Returns the string representation of a query error. +export fn strerror(err: error) str = { + match (err) { + case invalid => + return "Invalid query"; + case dupkeys => + return "A query must not contain duplicate keys"; + }; +}; + // Parses a query, returning its key/value pairs. The caller must pass the // return value to [[finish]] when they are done with it. -export fn parse(in: io::handle) (query | invalid | io::error) = { +export fn parse(in: io::handle) (query | error | io::error) = { const data = io::drain(in)?; const data = match (strings::fromutf8(data)) { case let data: str => @@ -55,13 +72,16 @@ export fn parse(in: io::handle) (query | invalid | io::error) = { // Parses a list of key/value pairs which has already been split with shlex (or // a shell, for example when parsing a query from argv). -export fn parse_items(items: []str) (query | invalid) = { +export fn parse_items(items: []str) (query | error) = { // XXX: Should do something about the case where the user specifies both // ? and ! let query = query { ... }; let ok = false; defer if (!ok) finish(&query); + let keys: []str = []; + defer free(keys); + for (let i = 0z; i < len(items); i += 1) { const (key, value) = strings::cut(items[i], "="); let optional = false, private = false; @@ -82,12 +102,31 @@ export fn parse_items(items: []str) (query | invalid) = { private = private, optional = optional, }); + append(keys, key); + }; + + if (len(query.items) == 0) { + ok = true; + return query; + }; + + // check dupes + sort(keys, size(str), &cmpkeys); + let prev = keys[0]; + for (let i = 1z; i < len(keys); i += 1) { + if (prev == keys[i]) { + return dupkeys; + }; + prev = keys[i]; }; ok = true; return query; }; +fn cmpkeys(a: const *opaque, b: const *opaque) int = + strings::compare(*(a: *str), *(b: *str)); + // Frees resources associated with this query. export fn finish(q: *query) void = { for (let i = 0z; i < len(q.items); i += 1) { @@ -127,4 +166,15 @@ export fn finish(q: *query) void = { assert(got.optional == expect.3); }; }; + + const errcases = [ + `foo=bar foo=bar`, + `foo=bar foo=baz`, + `foo=bar foo!=baz`, + `foo=bar bar=bay foo=baz`, + ]; + for (let i = 0z; i < len(errcases); i += 1) { + const input = memio::fixed(strings::toutf8(errcases[i])); + assert(parse(&input) is dupkeys); + }; }; -- 2.43.0
--- cmd/himitsu-store/main.ha | 2 +- cmd/himitsud/cmd.ha | 26 +++++++++++++++++++++++--- cmd/hiq/main.ha | 3 +++ docs/himitsu-ipc.5.scd | 9 +++++---- docs/himitsu.7.scd | 4 ++++ docs/hiq.1.scd | 4 ++++ himitsu/client/client.ha | 7 +++++++ secstore/delete.ha | 2 +- secstore/query.ha | 19 ++++++++++++++++--- 9 files changed, 64 insertions(+), 12 deletions(-) diff --git a/cmd/himitsu-store/main.ha b/cmd/himitsu-store/main.ha index 32020f6..a4a23ae 100644 --- a/cmd/himitsu-store/main.ha +++ b/cmd/himitsu-store/main.ha @@ -284,7 +284,7 @@ export fn reencrypt_move(oldpass: []u8, newpass: []u8) (void | secstore::error) fmt::error("Copying all entries to the new store... ")!; const q = query::query { ... }; - const iter = secstore::query(&oldstore, &q); + const iter = secstore::query(&oldstore, &q, false); for (true) { const item = match (secstore::next(&oldstore, &iter)) { case let item: *secstore::entry => diff --git a/cmd/himitsud/cmd.ha b/cmd/himitsud/cmd.ha index a30d724..0c3d72b 100644 --- a/cmd/himitsud/cmd.ha +++ b/cmd/himitsud/cmd.ha @@ -102,7 +102,23 @@ fn exec_add(serv: *server, client: *client, args: []str) (void | cmderror) = { }; fn exec_del(serv: *server, client: *client, args: []str) (void | cmderror) = { - const q = query::parse_items(args[1..])?; + const cmd = getopt::parse(args, + "query the key store", + ('s', "strict match"), + "query..." + ); + defer getopt::finish(&cmd); + + let strict = false; + for (let i = 0z; i < len(cmd.opts); i += 1) { + switch (cmd.opts[i].0) { + case 's' => + strict = true; + case => abort(); + }; + }; + + const q = query::parse_items(cmd.args)?; defer query::finish(&q); let buf = memio::dynamic(); @@ -122,7 +138,7 @@ fn exec_del(serv: *server, client: *client, args: []str) (void | cmderror) = { prompter = new; }; - const iter = secstore::query(serv.store, &q); + const iter = secstore::query(serv.store, &q, strict); let matches: []*secstore::entry = []; for (true) { const item = match (secstore::next(serv.store, &iter)) { @@ -166,15 +182,19 @@ fn exec_query(serv: *server, client: *client, args: []str) (void | cmderror) = { const cmd = getopt::parse(args, "query the key store", ('d', "decrypt private keys"), + ('s', "strict match"), "query..." ); defer getopt::finish(&cmd); let decrypt = false; + let strict = false; for (let i = 0z; i < len(cmd.opts); i += 1) { switch (cmd.opts[i].0) { case 'd' => decrypt = true; + case 's' => + strict = true; case => abort(); }; }; @@ -197,7 +217,7 @@ fn exec_query(serv: *server, client: *client, args: []str) (void | cmderror) = { const q = query::parse_items(cmd.args)?; defer query::finish(&q); - const iter = secstore::query(serv.store, &q); + const iter = secstore::query(serv.store, &q, strict); let matches: []*secstore::entry = []; for (true) { const item = match (secstore::next(serv.store, &iter)) { diff --git a/cmd/hiq/main.ha b/cmd/hiq/main.ha index 1f61ba5..a6dc518 100644 --- a/cmd/hiq/main.ha +++ b/cmd/hiq/main.ha @@ -24,6 +24,7 @@ export fn main() void = { ('D', "delete matching keys"), ('F', "field", "select a field for output"), ('Q', "terminate the daemon (if started with -D)"), + ('s', "strict query"), "<query>", ]; const cmd = getopt::parse(os::args, usage...); @@ -47,6 +48,8 @@ export fn main() void = { field = cmd.opts[i].1; case 'Q' => op = client::operation::QUIT; + case 's' => + flags |= client::flags::STRICT; case => fmt::fatal("Invalid command line option"); }; diff --git a/docs/himitsu-ipc.5.scd b/docs/himitsu-ipc.5.scd index 8ddc4dc..ea1caa4 100644 --- a/docs/himitsu-ipc.5.scd +++ b/docs/himitsu-ipc.5.scd @@ -35,17 +35,18 @@ The following commands are recognized by the server: Adds a new _key_ to the key store. The server will send a *key* reply echoing the new key (without secrets), followed by an *end* reply. -*del* _query_... +*del* [-s] _query_... Removes keys from the key store which match the provided _query_. The user will be prompted to consent to this operation. The server will send a *key* reply for each deleted key (without secrets), followed by an - *end* reply. + *end* reply. The option -s will enable the strict query mode. -*query* [-d] _query_... +*query* [-ds] _query_... Queries the key store for keys matching the provided _query_. Matching keys are returned via a series of *key* replies, terminated with an *end* reply to signal the end of the list. If the -d option is provided, - the private values will be decrypted after requesting user consent. + the private values will be decrypted after requesting user consent. The + option -s will enable the strict query mode. *quit* Requests that the daemon terminate itself. This command is only diff --git a/docs/himitsu.7.scd b/docs/himitsu.7.scd index acc9f25..bc1e95d 100644 --- a/docs/himitsu.7.scd +++ b/docs/himitsu.7.scd @@ -60,6 +60,10 @@ The key store will return all entries with *proto=web*, a *host* and *username* key with any value, a secret *password* key, and an optional *comment* key set to any value. +Per default all given keys in the query must be a subset of the entry for the +match to succeed, meaning the entry can have more keys than specified by the +query. + ## KEY CONVENTIONS Himitsu does not much care about the format of the keys it stores, but tools diff --git a/docs/hiq.1.scd b/docs/hiq.1.scd index 1c227b8..6332565 100644 --- a/docs/hiq.1.scd +++ b/docs/hiq.1.scd @@ -48,6 +48,10 @@ have already been quoted properly. Terminate the Himitsu daemon. Only works if *himitsud*(1) was started with the -D flag. +*-s* + Enable strict query mode. Entries must not have more keys than specified + by the query in order to be matched. + # EXAMPLES To query for all keys matching *proto=web*: diff --git a/himitsu/client/client.ha b/himitsu/client/client.ha index 94c3b2c..7dec0be 100644 --- a/himitsu/client/client.ha +++ b/himitsu/client/client.ha @@ -24,6 +24,9 @@ export type operation = enum uint { export type flags = enum uint { // Requests decryption from the Himitsu daemon. DECRYPT = 1 << 0, + + // Enable strict query mode + STRICT = 1 << 1, }; // All possible errors which may be returned by this module. @@ -86,6 +89,10 @@ export fn query( fmt::fprint(&buf, "-d ")!; }; + if (flags & flags::STRICT != 0) { + fmt::fprint(&buf, "-s ")!; + }; + query::unparse(&buf, q)!; io::writeall(conn, memio::buffer(&buf))?; diff --git a/secstore/delete.ha b/secstore/delete.ha index 1097d9d..aa0c933 100644 --- a/secstore/delete.ha +++ b/secstore/delete.ha @@ -17,7 +17,7 @@ export fn del(store: *secstore, q: *query::query) (void | locked) = { // it could cause problems here. for (let i = 0z; i < len(store.entries); i += 1) { const ent = &store.entries[i]; - if (entry_match(ent, q)) { + if (entry_match(ent, q, false)) { // TODO delkeys(store, &store.entries[i]); delete(store.entries[i]); i -= 1; diff --git a/secstore/query.ha b/secstore/query.ha index 507f88a..7ead686 100644 --- a/secstore/query.ha +++ b/secstore/query.ha @@ -3,6 +3,7 @@ use uuid; export type iterator = struct { query: *query::query, + strict: bool, index: size, }; @@ -11,9 +12,14 @@ export type iterator = struct { // the lifetime of the iterator. // // The iterator is not valid if the store is mutated during the iteration. -export fn query(store: *secstore, query: *query::query) iterator = { +export fn query( + store: *secstore, + query: *query::query, + strict: bool +) iterator = { return iterator { query = query, + strict = strict, ... }; }; @@ -22,14 +28,15 @@ export fn query(store: *secstore, query: *query::query) iterator = { export fn next(store: *secstore, iter: *iterator) (*entry | void) = { for (iter.index < len(store.entries); iter.index += 1) { const ent = &store.entries[iter.index]; - if (entry_match(ent, iter.query)) { + if (entry_match(ent, iter.query, iter.strict)) { iter.index += 1; return ent; }; }; }; -fn entry_match(ent: *entry, query: *query::query) bool = { +fn entry_match(ent: *entry, query: *query::query, strict: bool) bool = { + let nmatched = 0z; for (let i = 0z; i < len(query.items); i += 1) { const q = &query.items[i]; const p = match (findpair(ent, q.key)) { @@ -39,6 +46,7 @@ fn entry_match(ent: *entry, query: *query::query) bool = { }; continue; case let p: *pair => + nmatched += 1; yield p; }; match (p.value) { @@ -52,6 +60,11 @@ fn entry_match(ent: *entry, query: *query::query) bool = { }; }; }; + + if (strict && nmatched != len(ent.pairs)) { + return false; + }; + return true; }; -- 2.43.0
Secstore must not have two entries with an equal set of keys and an equal set of public key/value pairs. --- cmd/himitsud/cmd.ha | 11 +++++++---- himitsu/query/parse.ha | 15 +++++++++++++++ secstore/secstore.ha | 10 +++++++++- secstore/types.ha | 7 ++++++- 4 files changed, 37 insertions(+), 6 deletions(-) diff --git a/cmd/himitsud/cmd.ha b/cmd/himitsud/cmd.ha index 0c3d72b..c6d8dc7 100644 --- a/cmd/himitsud/cmd.ha +++ b/cmd/himitsud/cmd.ha @@ -66,13 +66,15 @@ fn exec(serv: *server, client: *client, cmd: str) (void | servererror) = { case let err: cmderror => // XXX: Probably a harec bug match (err) { - case let err: servererror => - writefmt(client, "error Internal error"); - return err; + case let err: secstore::error => + writefmt(client, "error {}", secstore::strerror(err)); case let err: prompt::error => writefmt(client, "error {}", prompt::strerror(err)); case let err: query::error => writefmt(client, "error {}", strerror(err)); + case let err: servererror => + writefmt(client, "error Internal error"); + return err; }; case void => yield; }; @@ -92,8 +94,9 @@ fn exec_add(serv: *server, client: *client, args: []str) (void | cmderror) = { const q = query::parse_items(args[1..])?; defer query::finish(&q); + // TODO: Prompt user to fill in incomplete keys - let entry = secstore::add(serv.store, &q)!; + let entry = secstore::add(serv.store, &q)?; let buf = memio::dynamic(); fmt::fprint(&buf, "key ")?; secstore::write(serv.store, &buf, entry, false)?; diff --git a/himitsu/query/parse.ha b/himitsu/query/parse.ha index 95aa524..be720cf 100644 --- a/himitsu/query/parse.ha +++ b/himitsu/query/parse.ha @@ -127,6 +127,21 @@ export fn parse_items(items: []str) (query | error) = { fn cmpkeys(a: const *opaque, b: const *opaque) int = strings::compare(*(a: *str), *(b: *str)); +// Duplicates a query, but only keeps values of public key/value pairs. The +// caller must pass the return value to [[finish]] after use. +export fn dup_pub(q: *query) query = { + let query = query { ... }; + for (let i = 0z; i < len(q.items); i += 1) { + let p = q.items[i]; + + p.key = strings::dup(p.key); + p.value = if (p.private) "" else strings::dup(p.value); + + append(query.items, p); + }; + return query; +}; + // Frees resources associated with this query. export fn finish(q: *query) void = { for (let i = 0z; i < len(q.items); i += 1) { diff --git a/secstore/secstore.ha b/secstore/secstore.ha index b6e5169..303dba9 100644 --- a/secstore/secstore.ha +++ b/secstore/secstore.ha @@ -335,11 +335,19 @@ fn free_keys(store: *secstore) void = { // Adds an item to the keystore. The provided query must not have any missing // values. -export fn add(store: *secstore, q: *query::query) (*entry | locked) = { +export fn add(store: *secstore, q: *query::query) (*entry | locked | dupentry) = { if (store.state != state::UNLOCKED) { return locked; }; + const checkq = query::dup_pub(q); + defer query::finish(&checkq); + + let it = query(store, &checkq, true); + if (next(store, &it) is *entry) { + return dupentry; + }; + // TODO: Better error handling let pairs: []pair = []; for (let i = 0z; i < len(q.items); i += 1) { diff --git a/secstore/types.ha b/secstore/types.ha index 79d42bf..eae73ed 100644 --- a/secstore/types.ha +++ b/secstore/types.ha @@ -53,7 +53,10 @@ export type badstore = !void; export type locked = !void; -export type error = !(fs::error | io::error | badstore | badpass | locked); +export type dupentry = !void; + +export type error = !(fs::error | io::error | badstore + | badpass | locked | dupentry); export fn strerror(err: error) const str = { match (err) { @@ -63,6 +66,8 @@ export fn strerror(err: error) const str = { return "Incorrect passphrase"; case locked => return "The keystore is locked"; + case dupentry => + return "The keystore does not allow duplicate entries"; case let err: fs::error => return fs::strerror(err); case let err: io::error => -- 2.43.0