Add cache extension

[robinkrahl]

This patch adds the nitrocli-cache extension that caches OTP and PWS
data. The per-device cache stores the names and IDs of the slots (and
the algorithm for OTP slots). It can be used to access the slots by
name instead of slot index.

---

The differences to previous versions (1, 2) are:

• This patch has support for the PWS.
• This patch does not use nitrokey-rs directly, but invokes nitrocli and
  parses its output. The reasoning for this is that re-implementing the
  device selection based on the values of the NITROCLI_MODEL,
  NITROCLI_USB_PATH and NITROCLI_SERIAL_NUMBER is unnecessarily complex.
• This patch does not try to parse data like the OTP algorithm. We
  don’t need this information in nitrocli-cache, so we can treat it as
  an opaque string that is handled only by nitrocli itself.

Fixes #83.

[d-e-s-o]

Thanks for the pull request! I will take a closer look later this weekend. Just one quick question: would it make sense/be reasonable from an implementation stand point to rather than naming the extension cache name it, say, __cache (or move it out of some PATH accessible directory or make it otherwise non-discoverable for nitrocli, whatever) and then have otp-cache and pws-cache be symlinks to it, that work for OTP/PWS, respectively. They would look at argv[0] to deduce what the high level "command" is?

I think I'd like the user interface much more: from my point of view, the caching of OTP & PWS data are separate concerns. It probably makes sense to share the implementation, but I am not sure the meaning of cache is intuitively clear to users. otp-cache at least hints at what it does, whereas cache could pretty much do anything caching related.

If that just gets too messy, perhaps there is some other way to accomplish something similar? Sorry, I wanted to bring that up in the issue today (after thinking about the OTP & PWS unification) but you already have the pull request now.

[robinkrahl]

I thought about that too, but I found it would not be worth the effort, especially as the user has to manually install the extension. From an implementation point of view, it should be possible, but I haven’t tested it.

[d-e-s-o]

I bounced that idea around some more. While the symlink magic may not be worth the trouble (thinking OS-dependent stuff and more difficult to set up as you mentioned; just too much vodoo), why not truly just share the common parts of the implementation from the two extensions? Worst case, we have to do the same dance as we do for the shell completion stuff, where we include! a Rust file, but it should be possible to do and the design should be straight forward to understand.

I just feel like it's much cleaner. Aside from the discoverability & user friendliness aspects that we already touched on, there is the at least theoretical possibility that we'd want some third cache (let's call it "PIN cache" for the sake of argument), and it may or may not fit nicely in here; in which case we run into naming problems (inconsistent naming if we have pin-cache extension and a generic cache one), unless we want to force it in here.

[d-e-s-o]

Do you see value in such a split, Robin?

[robinkrahl]

My main priorities were to have a first proper extension so that we can evaluate the extension design, and to add support for this feature that I really miss. While I’d prefer two separate commands, having a single command does not hurt me that much. If you want to look into splitting the two commands, I’m not objecting, but for me, this very simple but helpful extension is not worth that effort at the moment. I also think that the extensions don’t require the same level of maturity and optimization as the nitrocli core, at least at the current state. Anyway, I guess there are bigger issues that we’ll have to discuss once you had a closer look at the code. :-)

[d-e-s-o]

Hm yeah, it gets a bit hairy. I think at a high level we should do three things (in that order):

1. create a minimally usable module/crate to interface with nitrocli (basically mostly the contents of ext.rs moved into a new workspace item)
2. think about testing extensions. I somewhat disagree with the "extensions don’t require the same level of maturity" statement. I believe I can see where it's coming from (you've always focused more on the scripting aspect of things), but at least for core extensions I'd really want to do them right and have some confidence in their workings, be able to add regression tests etc. Specifically, I'd not want to go through every workflow by hand if we make a change. I understand we probably wouldn't do the same thing for extensions written in other languages, but I am honestly not in favor of having core extensions written in other languages to begin with. I also understand that extensions are more likely to have hard to test UI-style workflows (let's say involving QR code scanning or what have you). But right now it's not clear whether these will ever be core extensions and even if so I'd rather we decide that the operation is fundamentally hard to test than have no infrastructure for testing.
   Also, I don't know if proper testing will be possible with reasonable effort. But I'd at least want to explore that possibility and make an informed decision as opposed to not considering it from the start. I also think it's an interesting problem. But it's entirely possible that I am alone with that mind set :D
3. then start with a caching extension. I am happy to do the work of splitting cache, but I feel like it will be hard to coordinate (I don't want to just go berserk and split what you have; we will just end up with endless merges unless we have a somewhat stable base to work on [i.e., points 1 & 2]).

So my preference would be for us to start with 1) and wrap that up. I understand that may not be what you signed up for and I am happy to spend some time on that myself if you are not interested. Same for 2). Just let me know what you think and/or plan to do.

[d-e-s-o]

Is this TODO still applicable? Doesn't look like it is to me.

[robinkrahl]

I think it is. Currently, stderr is just ignored for text.

[d-e-s-o]

Hm, I was basing my comment on the fact that below we use output.stderr in the error. So that should do what we want it to, right? (whether we inherit stderr or read from it and print the output afterwards doesn't make much of a difference to me conceptually)

[robinkrahl]

Yes, but if the user invokes nitrocli with the --verbose option, they want to see the debug messages from the nitrocli instance invoked by the extension. Currently they won’t be displayed because we ignore stderr (unless there is an error).

[d-e-s-o]

Ah! True. Yeah, then we should address that TODO before checking in. Unfortunately then I think we can't capture the output to include it in errors programmatically. Otherwise I don't see how we can split the two concerns at this point.

--- ext/cache/src/ext.rs
+++ ext/cache/src/ext.rs
@@ -53,6 +53,12 @@ pub struct Nitrocli {
 impl Nitrocli {
   pub fn from_context(ctx: &Context) -> Nitrocli {
     let mut cmd = process::Command::new(&ctx.nitrocli);
+    // We want additional nitrocli emitted output to be visible to the
+    // user (typically controlled through -v/--verbose below). Note that
+    // this means that we will not be able to access this output for
+    // error reporting purposes.
+    cmd.stderr(process::Stdio::inherit());
+
     if let Some(verbosity) = ctx.verbosity {
       for _ in 0..verbosity {
         cmd.arg("--verbose");
@@ -76,15 +82,11 @@ impl Nitrocli {
   }

   pub fn text(&mut self) -> anyhow::Result<String> {
-    // TODO: inherit stderr from this process to show nitrocli debug messages
     let output = self.cmd.output().context("Failed to invoke nitrocli")?;
     if output.status.success() {
       String::from_utf8(output.stdout).map_err(From::from)
     } else {
-      Err(anyhow::anyhow!(
-        "nitrocli call failed: {}",
-        String::from_utf8_lossy(&output.stderr)
-      ))
+      Err(anyhow::anyhow!("nitrocli call failed"))
     }
   }

Not sure I like it, though.

[robinkrahl]

Alternatively, we could always capture stderr. If the command succeeds, we print it to stderr, if it fails, we include it in the error message. But as the error message will end up in stderr anyway, I don’t really see a benefit.

What exactly do you dislike?

[d-e-s-o]

I think I dislike that we have two concerns that we are clubbing together and can't disentangle: the actual error (if there is such a thing) and tracing information. If we capture it then we have a potentially huge amount of (multi-line) data in an error that I am sure we eventually make the assumption is a one line thing.

[robinkrahl]

I see, but we only have the tracing information if the user explicitly asks for it. I think that’s okay.

[d-e-s-o]

Yeah. I'd suggest we stick to inheriting for now and don't expect programmatic access to errors (which I don't think we had plans of interpreting anyway, so they would always have been opaque, it's just a question of how we control surfacing them). Let's see when this bites us and then we will have to see what to do. What do you think? Do you insist on capturing (didn't sound like it)?

[robinkrahl]

No, not at all!

[d-e-s-o]

I recall we wanted to make this a generic module available to all extensions, no? I suppose we could cross that bridge when the second one rolls around, but then why start this way when we know we will change it fairly soon. It's also already foreseeable that we will have more than one from the discussions so far. It will take a tad more work, but shouldn't be much, I'd think.

I'd suggest we introduce <root>/ext/nitrocli-ext and move it there. Can we do that in a separate commit please?

[robinkrahl]

Exactly. But let’s first discuss what this module should contain, okay?

[d-e-s-o]

Yep, I've laid out some thoughts in my general response.

[d-e-s-o]

I am probably touching a hornets' nest here, but I don't think we should parse the output like this. Didn't we have plans for a stable JSON & tab based output format?

I'd just use nitrokey-rs here for the time being and get the serial number through it. I vaguely recall you objecting to that (correct me if I am wrong; and perhaps refresh my mind regarding the reasons if I am not, I can't find the thread off hand), but I think it's vastly better than a regex match. And it does not have to stay like that forever.

Edit: Sorry, I missed the reasoning you provided in your initial description.

[robinkrahl]

That’s the bigger issue I mentioned previously. :-)

I made a draft supporting multiple output formats, but it still requires some work. I agree that this is not optimal, but it relies on the format tested in our unit tests.

I dislike calling nitrokey-rs directly because it requires us:

• to know all options that affect the connection
• to re-implement parsing these options (e. g. model)
• to re-implement the selection of the correct device

If we would decide to do something like this, then I’d prefer doing it in a crate that can be shared between nitrocli and its extensions, but I recall you didn’t want to do that.

[robinkrahl]

Here is the draft for output formats: https://github.com/robinkrahl/nitrocli/tree/output-formats

[d-e-s-o]

Do you think you could split the commit in two: one for the OTP functionality and another for PWS stuff? (if for nothing else than for reviewing changes and just having tangentially smaller sized commits)

[robinkrahl]

I'd really want to do them right and have some confidence in their workings, be able to add regression tests etc.

I didn’t mean that we shouldn’t test them. I was rather thinking about stability. For me, it wouldn’t be a problem to start with a cache extension and then split it up into otp-cache and pws-cache later. Of course, the extension should work fine and be tested reasonably. But its interface does not have to be as stable the nitrocli core interface.

Otherwise, I mostly agree with your plan. But I’m not sure it makes sense to separate these steps strictly. For example, how should we write the extension support crate if we don’t know how it should look like and what features it should have? So my suggestion is:

We get the cache extension into a state where we say: Yes, that’s how an extension should work. Then we can go into the details of extracting the nitrocli-ext crate and potentially splitting up cache into otp-cache and pws-cache.

[d-e-s-o]

I find late restructurings rather annoying and confusing, which includes renames. If I want to base the --copy argument work on top of cache I'd have to integrate that again once we split the extension. So I'd really rather that we start small with one thing, say, otp-cache instead of doing more and then splitting it.

The same applies to the extension module. While I am not fixated on the interface it provides per se (and I agree that stability at least in this early phase of development is not that important), we should at least get the layout right and not have to shuffle things around three commits down the line.

I am fine with it (the extension crate) being minimal at this stage and just doing whatever it takes to get the caching extensions working on top. From what I gather, that's mostly:

• read nitrocli binary path and verbosity from env variables (if we don't need the other data right now, we can leave it out)
• allow for invocation of nitrocli (potentially reading its output)
• deduce base directory for use by the extension (provided its name)
• the above ^ wrapped up somewhat ergonomically

Let's agree on the name and location and then start filling in the gaps. I've given a proposal for the same above, but if you have another suggestion that's probably fine, too.

[robinkrahl]

Just to avoid misunderstandings, I want to have the extension support crate in this PR, not somewhere in the future. But I’d rather decide how to deal with the nitrocli output parsing vs. nitrokey invocation issue before starting the refactoring.

Regarding the question whether to split up the two commands: Splitting the single commit into two commits is not an issue. We could also restrict this PR to otp-cache to avoid future changes, but this would effectively be a decision to have two separate extensions.

[d-e-s-o]

We could also restrict this PR to otp-cache to avoid future changes

Yeah, I'd prefer that.

[d-e-s-o]

But I’d rather decide how to deal with the nitrocli output parsing vs. nitrokey invocation issue before starting the refactoring.

I dislike calling nitrokey-rs directly because it requires us:

• to know all options that affect the connection
• to re-implement parsing these options (e. g. model)
• to re-implement the selection of the correct device

If we would decide to do something like this, then I’d prefer doing it in a crate that can be shared between nitrocli and its extensions, but I recall you didn’t want to do that.

I suppose we can do that. The other potential alternative is to, instead of passing down all the filters and stuff (model, serial number, ...), just let nitrocli establish the connection and then pass down the USB path or something uniquely identifying the device in the form of an environment variable (i.e., changing the nitrocli <-> extension interface). It would potentially mean a rather useless connection being established unconditionally for extensions, but yeah.

[robinkrahl]

That should be possible, yes. The serial number does not work for Storage devices, so we have to use the USB path. We don’t even need a connection, only HID enumeration. There is a slight risk for race conditions, but we would have that in any case. As most extensions will want to use a device, I think performing the lookup before invoking the extension would be a better and more flexible solution than compiling the logic into the extension, e. g. via a shared crate. I favor prefer invoking nitrocli, but if we go with using nitrokey-rs directly, then I’d prefer this approach.

[d-e-s-o]

I favor prefer invoking nitrocli, but if we go with using nitrokey-rs directly, then I’d prefer this approach.

Yeah, agreed. I am trying to minimize the number of constructions sites that we are working on at any point in time while at the same time not setting precedent for parsing potentially unstable output; even more so on an interface boundary that we have full control over. In the future -- once we have well defined stable output -- I think we can go back to invoking nitrocli.

In any case, let me know what (if anything) you want me to do on that front. I hope we clarified everything, but if we missed something just bring it up.

[robinkrahl]

Next iteration: #146.