search

Nitrokey / pynitrokey

Python client for Nitrokey devices
Apache License 2.0
93 stars 28 forks source link

Log cli parameters #399

Closed Laborratte5 closed 1 year ago

Laborratte5 commented 1 year ago

This PR aims to simplify log file interpretation by also writing the cli parameters that where used to run the program into the log file.

Changes

Checklist

Make sure to run make check and make fix before creating a PR, otherwise the CI will fail.

Test Environment and Execution

Relevant Output Example

After executing nitropy fido2 --help The log file now contains a line like the following:

513        INFO pynitrokey.cli cli arguments: ['fido2', '--help']

The representation as a list of parameters is done on purpose to make it easier to see how the Arguments got parsed. (i.e. nitropy parameter1 "parameter2 with spaces") would be represented as ['parameter1', 'parameter2 with spaces']

Fixes #309

Laborratte5 commented 1 year ago

I made this a draft, because the current implementation would also log parameters like the nitropy nk3 secrets set-pin --password TEXT which is not ideal. However I would like to get some opinions if these parameters should be excluded from the log using some sort of global white-/blacklist Or if sensitive parameters should be excluded per command for instance by using a decorator.

robin-nitrokey commented 1 year ago

For me it would be fine to assume that sensitive parameters always take exactly one argument. I’m not aware of any other cases. But keep in mind that it is also possible to use the --option=vallue syntax.

Laborratte5 commented 1 year ago

For me it would be fine to assume that sensitive parameters always take exactly one argument

I also did not found any command using more than one sensitive parameter (Maybe I should have looked before implementing this). Should I change this to reduce the codes complexity, or leave it as it is?

But keep in mind that it is also possible to use the --option=vallue syntax.

I now added support for this. However I want to take another look to ensure the redaction does not break when using strings containing = signs

Laborratte5 commented 1 year ago

I've now squashed the commits.

robin-nitrokey commented 1 year ago

Thank you! :)