latchset / kryoptic

a pkcs#11 software token written in Rust
GNU General Public License v3.0
11 stars 4 forks source link

Add configuration file support #102

Closed simo5 closed 3 weeks ago

simo5 commented 1 month ago

To aid cases where an environment variable cannot be easily set nor init arguments can be easily passed we need a place where we can store the tokens configuration, including the ability to specify multiple slots.

The ability to use init args and ignore any configuration is retained but the init arguments format has been enhanced to be a set of comma separated key/value pairs so that additional info can be passed there in the future.

On build a CONFDIR prefix can be specified as: CONFDIR=/etc cargo build

Resolves: #100

Jakuje commented 3 weeks ago

regarding the configuration file format, I have just few thoughts:

simo5 commented 3 weeks ago

regarding the configuration file format, I have just few thoughts:

* the type + path sounds to me like a duplicated information as the type could be derived from the filename (except for the memory, but that is special). It might also not work that well when we will support NSS DB, which will likely be just a path to a directory? So this is just a though rather than something to change.

I did not want to assume the URI could always tell, and wanted a preamble exactly due to nssdb. As you know I am working on it, and the config structure of NSS is completely different. I have an heuristic to check if the arguments passed in pReserved are NSSDB vs our format and then have completely separate parsing routines ...

Also while NSSDB has the complete set of options in their parser, the standard config for kryoptic cannot be fully represented on the arguments, beyond the basic db+slot argument you have to pass in a configuration file to define anything else.

So the questions is ... should we just support the fallback to just the uri for legacy/expedenciy purposes and really only have a kryoptic_conf= parameter support instead for anything more complicated including the need to specify a slot number ?

On one hand that would require writing out a configuration file even for simple stuff. OTOH I wonder if most admins will rather have a config file written somewhere than a long weird configuration string that admits no spaces and stuff.

I am willing to change this code radically if needed.

* In softhsm, I was adding a support to configure allowed mechanism through configuration file mostly for testing purposes to be able to emulate a pkcs11 module which does not support for example RSA-PSS, raw RSA or something else. But this can be added later if needed (extending the configuration file with more non-mandatory keys should not be an issue).

Yes, I have no issues adding more config options in the file, I would not add them to the argument parsing though, hence see above question.

Jakuje commented 3 weeks ago

So the questions is ... should we just support the fallback to just the uri for legacy/expedenciy purposes and really only have a kryoptic_conf= parameter support instead for anything more complicated including the need to specify a slot number ?

I would say so. The configuration file is nice for anything more complicated than "I just want a pkcs11 token working" -- for this, having just env variable pointing to the sql file should be enough.

Yes, I have no issues adding more config options in the file, I would not add them to the argument parsing though, hence see above question.

Agree. I think this is not urgent. I just wanted to point it out, that this was very helpful when we did introduce support for PSS and wanted to test this in different libraries against pksc11. But I do not think it is needed now.

simo5 commented 3 weeks ago

Ok, so based on recent comments and discussion I changed and simplified the code. Now the only allowed arguments in the reserved C_initialized argument are: 1) kryoptic_conf=/path/to/config.file 2) [dbtype:]/path/to/database.file (ex: json:/file.json or /db.sql)

2, is for quick adding a database with defaults otherwise, and support the simplest case available in the previous code (CI compatibility).

A slot is automatically assigned for legacy files

simo5 commented 3 weeks ago

I also had to squash together the test and code commits, because this changed forced me to change tests at the same time due to less backwards compatibility with the testing infrastructure

simo5 commented 3 weeks ago

One more push where I moved all config related stuff in src/config.rs where it makes more sense at this point