Configuration file about nitrocli HOT 14 CLOSED

The thought has crossed my mind. It's probably a good idea. I hope it can be implemented easily such that command line arguments take precedence over what is in the config. Things may get tricky with the conflicts_with attributes that structopt supports and we use (we may have to find a better way for those; I recently stumbled over this issue, which may be of interest in this context). Perhaps we can layer this functionality before argument parsing (it's what I've done elsewhere and it has served me well so far)? Just a thought.

I am not familiar with config and only had a cursory glance. I suspect we will also need one of the backends, presumably toml (or yaml?). I am actually not quite sure what value-add the crate provides over just using serde with toml and a custom derive. Well, I sort of see it, just not really the benefit for our context. Have you looked closer already by any chance?

from nitrocli.

from nitrocli.

Perhaps we can layer this functionality before argument parsing (it's what I've done elsewhere and it has served me well so far)?

Yes, I agree. I thought of adding the data to the {Run,Exec}Ctx, similar to the environment variables.

Yep, seems reasonable.

I suspect we will also need one of the backends, presumably toml (or yaml?).

I’d prefer toml. It is simple enough for non-Rust users to get used to, and it is quite similar the common INI-style configuration files.

Sounds good.

I am actually not quite sure what value-add the crate provides over just using serde with toml and a custom derive.

It would make it easier to merge multiple configuration sources (e. g. system configuration in /etc/xdg/nitrocli/config(.toml?), user configuration in ~/.config/nitrocli/config(.toml?) and environment variables).

Ah, interesting. That seems reasonable then.

I think the major decision is which arguments to support. We could support all arguments, with subgroups for subcommands, e. g.: model = "pro" [otp.get] algorithm = "hotp" This would be the most elegant and complete approach, but honestly, I can’t think of a scenario where one would like to set defaults for any of the subcommand arguments. So to keep things simple, and until a user requests anything else, I would limit the supported arguments to: - serial number (once implemented) - USB path (if implemented) - model - verbose - no_cache These should also be read from the environment as NITROKEY_SERIAL_NUMBER etc.

Yeah, sounds good to me. I'd say it would be good if we could rule out the "supporting all arguments" approach based on reasons of complexity of the implementation and not on the basis of whether the feature will be used. If it's easy and elegant to implement, I'd prefer that approach.
If we don't see a way of doing that (or if it requires listing every supported argument manually outside of our existing structopt definitions) then we can limit. And then we can support additional arguments step by step.

We could also add the other environment variables (*_ADMIN_PIN etc.) but I’m reluctant to allowing the user to store their passwords and PINs in a configuration file.

Agreed.

from nitrocli.

I'd say it would be good if we could rule out the "supporting all arguments" approach based on reasons of complexity of the implementation and not on the basis of whether the feature will be used.

That’s true. I omitted that part of my argument – I would only consider the complex implementation if there is a use case.

If it's easy and elegant to implement, I'd prefer that approach.

I did not yet write any code, but I see the following problems:

• We cannot have configuration for positional arguments, because that would break our argument parsing. (The positional arguments are required. If we make them optional, we have to manually re-implement clap’s argument validation.) Maybe it would be possible to fix this by marking them with #[serde(skip)].
• We would have to manually set the default values. We can no longer set them using the structopt attributes, because we have to know if the argument was not set. This would also mean that the default values are no longer shown in the help text.
• We would have to manually merge the two sets of arguments. It might be possible to serialize the output of structopt, give it to config as the most important config source, and then de-serialize it back into Args. But then we have to make sure that unset arguments don’t override the defaults – maybe using #[serde(skip_serializing_if = "Option::is_none")].

So it might be possible to implement this using our Args struct, but it would require some additional annotations and we would probably lose the default values in the help output.

from nitrocli.

I did some experiments and noticed another problem: The commands and subcommands are enums. Even if everything else works, config could only give us an Args struct for one command variant. We would have to tell config and serde for which command and subcommand to look. And I think that’s impossible.

from nitrocli.

What I meant with my initial comment "Perhaps we can layer this functionality before argument parsing" is basically the following: when the program starts up we read the config file(s). We also get the program arguments. Then we translate the config-file provided configuration into program arguments (as the user would supply them on the command line) and insert them into the argument vector we ultimately pass to structopt accordingly.
I believe that could be a reasonably nice way of layering concerns here. I've used that technique in the past and surprisingly (to me; it appears a bit hacky and perhaps it is with, nested sub commands) don't recall having any problems (but the project had a little different requirements). The main question then is, I'd say:

How can we infer the option to use from the configuration? In the past I literally just put the config options into the config file; hypothetical example (using YAML, just because that's what I have at hand right now not because it's necessarily my preference):

- 'status': [
    '--verbose',
  ]

This configuration would result in nitrocli status --verbose <user-supplied-options> being passed to the argument parser. This way, <user-supplied-options> could naturally overwrite earlier arguments. E.g., if there was a --quiet that would cancel out the --verbose. That should be handled by structopt already. I would guess we can come up with a way to make something like

[otp.get]
algorithm = "hotp"

work as well (just prepend two dashes :P)

As indicated above, though, things may get interesting with nested sub commands. I don't claim to have thought that through in its entirety.

from nitrocli.

I did not yet write any code, but I see the following problems:

• We cannot have configuration for positional arguments, because that would break our argument parsing. (The positional arguments are required. If we make them optional, we have to manually re-implement clap’s argument validation.) Maybe it would be possible to fix this by marking them with #[serde(skip)].

Hm. You basically would want otp to evaluate to otp get unless a set is provided explicitly? Is that correct? I guess it would be nice to have, but if that wasn't possible I wouldn't be too concerned.

• We would have to manually set the default values. We can no longer set them using the structopt attributes, because we have to know if the argument was not set. This would also mean that the default values are no longer shown in the help text.

Yeah, sucks a bit, but I don't see a solution to that either.

• We would have to manually merge the two sets of arguments. It might be possible to serialize the output of structopt, give it to config as the most important config source, and then de-serialize it back into Args. But then we have to make sure that unset arguments don’t override the defaults – maybe using #[serde(skip_serializing_if = "Option::is_none")].

That, I would say should be covered by the approach I outlined above. But let me know if I am misunderstanding.

from nitrocli.

Then we translate the config-file provided configuration into program arguments (as the user would supply them on the command line) and insert them into the argument vector we ultimately pass to structopt accordingly.

Ah, I see. I’ll have to think a bit more about that. My first concern is that it would make error reporting a bit confusing (if there is an error in the configuration file, the user will get an error message about the arguments).

You basically would want otp to evaluate to otp get unless a set is provided explicitly? Is that correct?

No. I was thinking about this: Consider the subcommand otp set [slot] [name] [secret] with three positional arguments. The user could have a setting like otp.set.name = "GitHub", but that is hard to implement and use. For example, otp set 1 secretstring should use the configured name and evaluate to otp set 1 GitHub secretstring. But how should we tell structopt that we no longer require the second argument? And would that really be a user-friendly interface? So my suggestion was to not accept configuration for positional arguments.

I started implementing the simple version of my initial approach (parsing the config file for selected arguments and storing them in *Ctx). It is working fine, but it turns out that the config API is not as elegant as I thought and hoped. The crate has been inactive for quite a while. In May 2019, the author started a discussion about a redesign (mehcode/config-rs#111), but as far as I see, there have not yet been significant improvements.

from nitrocli.

Then we translate the config-file provided configuration into program arguments (as the user would supply them on the command line) and insert them into the argument vector we ultimately pass to structopt accordingly.

Ah, I see. I’ll have to think a bit more about that. My first concern is that it would make error reporting a bit confusing (if there is an error in the configuration file, the user will get an error message about the arguments).

Good point. I'd hope the user sees the connection between the two. Obviously, that would be more apparent if we actually added the options into the config file directly (but I understand that is not necessarily the preferred way for other reasons).

You basically would want otp to evaluate to otp get unless a set is provided explicitly? Is that correct?

No. I was thinking about this: Consider the subcommand otp set [slot] [name] [secret] with three positional arguments. The user could have a setting like otp.set.name = "GitHub", but that is hard to implement and use. For example, otp set 1 secretstring should use the configured name and evaluate to otp set 1 GitHub secretstring. But how should we tell structopt that we no longer require the second argument? And would that really be a user-friendly interface? So my suggestion was to not accept configuration for positional arguments.

Understood. Yeah, I am fine with not overloading the feature.

I started implementing the simple version of my initial approach (parsing the config file for selected arguments and storing them in *Ctx).

Nice!

It is working fine, but it turns out that the config API is not as elegant as I thought and hoped. The crate has been inactive for quite a while. In May 2019, the author started a discussion about a redesign (mehcode/config-rs#111), but as far as I see, there have not yet been significant improvements.

Not so nice :-| If you think it adds value okay, as long as it does not permeate into every part of the program and could be swapped it out with reasonable effort (which, I would say, hasn't really been the case with argparse).

from nitrocli.

If you think it adds value okay, as long as it does not permeate into every part of the program and could be swapped it out with reasonable (which, I would say, hasn't really been the case with argparse).

Yes, it’s pretty isolated. In my example implementation in #101, only src/config.rs depends on config. The main benefit over using serde and toml directly is that it nicely merges different configuration sources – though that might not be necessary if we use the argument approach.

from nitrocli.

Okay, I looked at your pull request. Thanks for working on it! Looks pretty good to me. I can't say much more, I guess, without seeing an alternative approach :D But this seems isolated enough that I think it would be reasonable to merge.

The only thing that annoys me is the multitude of new dependencies, but I guess we crossed that line already when we switched to structopt. It's also another 200 KiB size increase. Not an issue per se, but not my preferred direction either.

I guess we get for free a new NITROCLI_MODEL environment variable now, which is pretty neat.
Just wondering how this part:

   match Args::from_iter_safe(args.iter()) {
     Ok(args) => {
+      let mut config = ctx.config;
+      config.update(&args);
       let mut ctx = ExecCtx {
-        model: args.model,
         stdout: ctx.stdout,
         stderr: ctx.stderr,
         admin_pin: ctx.admin_pin.take(),
         user_pin: ctx.user_pin.take(),
         new_admin_pin: ctx.new_admin_pin.take(),
         new_user_pin: ctx.new_user_pin.take(),
         password: ctx.password.take(),
-        no_cache: ctx.no_cache,
+        config,
         verbosity: args.verbose.into(),
       };
       args.cmd.execute(&mut ctx)
     }

can work. If we have a mutable reference to a RunCtx, how can we move the config out of it?

from nitrocli.

from nitrocli.

Yep, thank you (I did check for that but must have missed it...)

from nitrocli.

Implemented in #111.

from nitrocli.