Topic/extensions v2

[d-e-s-o]

No description provided.

[d-e-s-o]

@robinkrahl I've reworked the extension patch stack that I created back in the day to work with structopt. I think it's much nicer now, because structopt already provides native support for what we want to do.
It's not yet fully polished (and I need to get a few days of distance before looking at it again), but feel free to let me know what you think when you get a chance.

[robinkrahl]

The code looks good to me! I’ll have a closer look later today. But I want to bring up some issues regarding the extension ecosystem: # Backwards compatibility One issue that I see with this approach is that a non-breaking change in nitrocli, adding a new global option that is passed to the extensions, could easily break older extensions (unsupported argument). So should we either consider adding a global option to be a breaking change, or should we recommend that extensions only print a warning for unsupported arguments instead of aborting? # Extension support crate It would be nice to have an extension support crate, let’s say nitrocli-ext, that provides the pass-through options and utility methods to invoke nitrocli. We could even use this crate in nitrocli to avoid code duplication. # Curated extensions We will have some core extensions that are useful for most nitrocli users, for example getting an OTP code by name. In my opinion, we should treat these extensions as a part of nitrocli regarding maintenance and support. We could even include them in the main nitrocli repository, or we could create a new nitrocli-extensions repository for curated extensions. # Data directories We should define a place where extensions should store their data (in terms of the XDG Base Directory spec: config, cache, data). My suggestion is: We reserve the subdirectories of the nitrocli data directories for extensions. We could even allow extensions to put their configuration in sections of nitrocli’s main config file.

[d-e-s-o]

Thanks for your comments! These are all valid points.

Backwards compatibility One issue that I see with this approach is that a non-breaking change in nitrocli, adding a new global option that is passed to the extensions, could easily break older extensions (unsupported argument). So should we either consider adding a global option to be a breaking change, or should we recommend that extensions only print a warning for unsupported arguments instead of aborting?

I don't even think we do have a notion of what is a breaking change for nitrocli to begin with. If I recall in the past I mostly bumped the minor version when major enough features landed or it just felt like the right time ;-)

But yes, I do share your sentiment. I am fairly sure we touched on that in the past, and wanted to have a policy that extensions just ignore arguments they don't understand. But we should have an "extension writer's guide" or something, perhaps (or at least cover it in the man page).

Extension support crate It would be nice to have an extension support crate, let’s say nitrocli-ext, that provides the pass-through options and utility methods to invoke nitrocli. We could even use this crate in nitrocli to avoid code duplication.

I didn't want to get into the business of having a library providing a stable API to be honest (arguably that's more dogma than anything else at this point, though...).
But can you explain what kind of functionality you see in there? After all, I was under the impression that extensions would be scripted in one way or another? Or are you more talking about the implementation inside of nitrocli itself (didn't sound like it)?

In all likelihood we will need some better way to get data provided by nitrocli, and I recall us talking about parsing its output and not agreeing on the format (I still have a change to output JSON data from nitrocli if a flag is provided...[that would probably be covered by a compatibility contract])

Curated extensions We will have some core extensions that are useful for most nitrocli users, for example getting an OTP code by name. In my opinion, we should treat these extensions as a part of nitrocli regarding maintenance and support. We could even include them in the main nitrocli repository, or we could create a new nitrocli-extensions repository for curated extensions.

Yes, agreed. I am happy to include them in the repository itself.

Data directories We should define a place where extensions should store their data (in terms of the XDG Base Directory spec: config, cache, data). My suggestion is: We reserve the subdirectories of the nitrocli data directories for extensions. We could even allow extensions to put their configuration in sections of nitrocli’s main config file.

That sounds pretty good to me!

[robinkrahl]

I don't even think we do have a notion of what is a breaking change for nitrocli to begin with.

That’s true. Technically, we don’t have to deal with breaking changes while we are still 0.*, but we should definitely write up a short description of our stability promises before we reach v1.0.0.

But can you explain what kind of functionality you see in there?

I don’t want a stable API to nitrocli – or at least that’s not what I meant –, but utility methods for extensions written in Rust, for example:

• Args: The arguments that nitrocli passes to the extension, with a nitrocli() method that returns a Nitrocli instance with the correct path and the correct arguments (if set).
• Nitrocli: A builder similar to the Nitrocli struct in tests with additional methods to execute nitrocli, verify the exit status, return the standard output or the parsed JSON data (if we use it as the exchange format).
• The enums that we use in nitrocli, for example OtpSecretFormat. These could be marked as non-exhaustive so that we don’t break them by extending them.

The Args struct (or a subset of the Args struct) and the enums could than be used by nitrocli too – but we could also keep that separate.

Maybe this pseudo code makes it clearer (nitrocli_ext would be the utility crate):

#[derive(StructOpt)]
struct Args {
    name: String,
    #[structopt(flatten)]
    ext: nitrocli_ext::Args,
}

fn main() -> anyhow::Result<()> {
    let args = Args::from_args()?;
    let slots = args.ext.nitrocli()
        .args(&["otp", "list"])
        .json()
        .context("Could not list OTP slots")?;
    let slot = slots.iter()
        .filter_map(|v| v.as_object())
        .filter(|o| o.get("name").as_str() == Some(&args.name))
        .filter_map(|o| o.get("slot").as_i64())
        .next()
        .context("Could not find OTP slot with the given name")?;
    let code = args.ext.nitrocli()
        .args(&["otp", "get", &slot.to_string()])
        .text()
        .context("Could not generate OTP code")?;
    println!("{}", code);
    Ok(())
}

and I recall us talking about parsing its output and not agreeing on the format

Indeed. I’m not against having JSON as an output format, I just wanted (and still want) to also have a simpler output format that does not require a JSON parser.

[d-e-s-o]

But can you explain what kind of functionality you see in there?

I don’t want a stable API to nitrocli – or at least that’s not what I meant –, but utility methods for extensions written in Rust, for example:
[...]

I see. Yeah, we could think about that. I didn't see too many use cases for extensions written in Rust itself, but why not. And if I recall correctly you already had some QR code utility that may lend itself towards being extensionized.

and I recall us talking about parsing its output and not agreeing on the format

Indeed. I’m not against having JSON as an output format, I just wanted (and still want) to also have a simpler output format that does not require a JSON parser.

I suppose if we could express this non-JSON format in the form of a serde data format we could fudge both cases in one go.

[robinkrahl]

And if I recall correctly you already had some QR code utility that may lend itself towards being extensionized.

True: https://git.ireas.org/nitrocli-otp-qr/ I will also prepare a prototype for an extension that generates a OTP by slot name.

I suppose if we could express this non-JSON format in the form of a serde data format we could fudge both cases in one go.

We could use the csv crate to generate tab-separated values, but we will probably have to make some slight adjustments. For example, otp get would probably return {"code: "123456"} in JSON, but the “plain” output should just be the generated code.

[robinkrahl]

Here is a prototype for generating one-time passwords by slot name, based on the extensions-v2 branch: https://github.com/robinkrahl/nitrocli/tree/otp-cache

It’s working quite well, but I have not yet found a good way to tell structopt to store additional arguments in a vector that we can then pass to nitrocli. Also, it’s a bit annoying that I can’t pass on the --verbositiy option to nitrocli.

[d-e-s-o]

Here is a prototype for generating one-time passwords by slot name, based on the extensions-v2 branch: https://github.com/robinkrahl/nitrocli/tree/otp-cache

Really nice! Yeah, I think this one we could include as a core extension.

It’s working quite well, but I have not yet found a good way to tell structopt to store additional arguments in a vector that we can then pass to nitrocli.

Isn't the standard for these cases doing something along the lines of: nitrocli otp-cache --your-fancy-otp-cache-option=42 -- --model=storage

Then you can capture nitrocli option (everything after --) as positional arguments.

But yeah, overall not many command line argument parsers will lend themselves too well towards what we are doing. That's certainly the biggest concern I have. I'd of course still be willing to go with a different way for interfacing with extensions (or them interfacing with nitrocli), but haven't been able to come up with something so far.

Also, it’s a bit annoying that I can’t pass on the --verbositiy option to nitrocli.

Can you elaborate?

[robinkrahl]

Isn't the standard for these cases doing something along the lines of: `nitrocli otp-cache --your-fancy-otp-cache-option=42 -- --model=storage`

I think typically it’s the other way round: `nitrocli otp-cache --global-option -- --extension-option`. But we could use this as a convention for the extensions. Still, it only allows me to catch all pass-through options as strings. I’d like to parse the known options using structopt so that I can use them in my code and collect only the unknown options as strings to be compatible with newer nitrocli versions.

> Also, it’s a bit annoying that I can’t pass on the --verbositiy > option to nitrocli. Can you elaborate?

nitrocli calls the extension with a --verbosity <n> option. If the extension in return calls nitrocli, it would want to set the verbosity for nitrocli. But nitrocli expects a --verbose flag, so I have to transform --verbosity <n> into n times --verbose.

[d-e-s-o]

I’d like to parse the known options using structopt so that I can use them in my code and collect only the unknown options as strings to be compatible with newer nitrocli versions.

Yeah, that has always been a concern of mine. If I recall correctly in Python it's more or less easily possible, but in Rust I wouldn't know, off hand. Do you know whether other argument parsing libraries would support that better? I'd hate for us to use a different crate for that, at least for core extensions, but if it's an easy way out, perhaps.

nitrocli calls the extension with a --verbosity option. If the extension in return calls nitrocli, it would want to set the verbosity for nitrocli. But nitrocli expects a --verbose flag, so I have to transform --verbosity into n times --verbose.

That I would hope is fixable. Preferably we could see if recognizing both --verbose=X and multiple --verbose can be made to work. If not we could still mirror the --verbosity argument in nitrocli directly. Or we just switch to --verbose=X everywhere (though I kind of dislike that, as X has no obvious semantic to the user, it's just a number [is higher more or less?]).

[robinkrahl]

If you don’t mind, I’m going to prepare a prototype for setting the output format as a basis for further discussion, okay?

[d-e-s-o]

If you don’t mind, I’m going to prepare a prototype for setting the output format as a basis for further discussion, okay?

I actually thought about this prototype some more. I think the extension lookup is fine, but the "recursive" invocation of nitrocli isn't really something that is common. And reading through a bit more of our original discussion, it is basically the cause of contention, because that requires us to have a stable format. And ultimately it seems neither of us is really happy with it. You mentioned the library approach and I'd be interested in seeing how it pans out and how much work it would be. So I guess the question is whether you'd still insist on having some form of stable output? (though I fear that the answer is "yes")

That's the first point. The second is that after a bit more thought I'd want to try out using environment variables for conveying the "execution context". I think I just prototyped with arguments because it was simpler and I had not weighed all pros and cons. But I agree with your comment that they may be more suitable.

Does that make any sense to you? Or are we just starting back at zero at this point? From my perspective not a whole lot changes for this pull request: just the way we pass the context. The first point is more long term and directly related to your question about future work.

[robinkrahl]

I think it wouldn’t be that hard to add support for stable output formats. And as long as we have both a format that is easy to parse (e. g. tsv) and a structured format (e. g. JSON), I’m not unhappy with this solution.

Regarding the invocation method, let’s take otp-cache as an example. I can see three ways it could be implemented:

• Using the nitrokey crate. This would require re-implementing large parts of nitrocli, so I don’t think it’s an option.
• Using a nitrocli library crate. This would also allow us to reuse parts of nitrocli, but it would mean that extensions can only be written in Rust.
• Calling nitrocli recursively. This would allow us to reuse parts of nitrocli while implementing the extension in a language of our choice.

With our current approach, I don’t really see the benefits of a library crate. It would still have to parse the arguments one more time etc. But it would probably be harder to maintain extensions because the extension’s nitrocli lib version would always have to match the nitrocli binary’s version. On the other hand, re-invoking nitrocli makes sure that both the “outer” and the “inner” version are the same.

But even if we go with the library crate, I think it would be good to have a stable output format for scripting. Consider for example the passmenu use case that I described in issue #50.

Regarding the environment variables: I think your suggestion would be to set NITROCLI_ARGS="--model model" instead of calling nitrocli-extension --model model, right? I’ll have to test whether that helps with the parsing problem. (The extension would still have to parse some arguments and ignore the rest.) Another option is to use one environment variable per argument, but I think that would be rather complicated to implement.

[d-e-s-o]

Regarding the environment variables: I think your suggestion would be to set NITROCLI_ARGS="--model model" instead of calling nitrocli-extension --model model, right? I’ll have to test whether that helps with the parsing problem. (The extension would still have to parse some arguments and ignore the rest.) Another option is to use one environment variable per argument, but I think that would be rather complicated to implement.

I was actually thinking one environment variable per argument. Can you explain what complication you see?

Would having a single variable buy us that much? You'd still have to run some more or less complex parsing on the contents (as opposed to a rather simple XXX-to-string conversion), which I am not sure is helpful.

[robinkrahl]

I was actually thinking one environment variable per argument. Can you explain what complication you see?

You’re right, I was thinking about the subcommands. But we would only use the global options that already support environment variables, right? So it would result in something like this: NITROCLI_MODEL="pro" nitrocli-opt-cache --force-update get github.com

Would having a single variable buy us that much? You'd still have to run some more or less complex parsing on the contents (as opposed to a rather simple XXX-to-string conversion), which I am not sure is helpful.

It would be a small improvement because we would no longer have to separate the extension arguments and the nitrocli base arguments, but it wouldn’t be much.

[d-e-s-o]

I was actually thinking one environment variable per argument. Can you explain what complication you see?

You’re right, I was thinking about the subcommands. But we would only use the global options that already support environment variables, right? So it would result in something like this: NITROCLI_MODEL="pro" nitrocli-opt-cache --force-update get github.com

Yep, exactly.

[d-e-s-o]

I think it wouldn’t be that hard to add support for stable output formats. And as long as we have both a format that is easy to parse (e. g. tsv) and a structured format (e. g. JSON), I’m not unhappy with this solution.

Ah, okay :) I'll have to think through things a bit more, I guess. Anyway, doesn't hurt to experiment with output some. So if you want to come up with something, let me just share my preliminary thoughts:

Given that we could not really agree on a format or sort of agreed on going with two, I was thinking about the following:
we could have global --json/--parsable (whatever) options. If either is set we'd take one path in nitrocli on which we prepare for any output. Basically, we could craft, say, a hashmap of key-value pairs (may need nesting or something; TBD). But the point would be that then we could just use serde_json for emitting data as JSON (potentially with some preprocessing step to make it serde compatible). For tab-separated-values we could write our own serializer (or whatever the terminology). But the point being that we have some intermediate object used internally that subsequently can be converted into any of the formats we desire.

I am pretty sure that would work easily with JSON (and others such as YAML). I am not quite sure whether we can make it work with TSV. It sort of depends on how you want it to look. Theoretically we could just print keys followed by a tab followed by a value and have some indentation. That should map fairly cleanly.

It's just a thought. Perhaps there is so little duplication/difference in the paths for two formats that it doesn't matter.

[robinkrahl]

That’s similar to what I had in mind: On my output-formats branch, I added the output module that contains data types for the output. These data types can be converted to human-readable text, JSON or tsv. (I’m currently using the csv crate to generate the tsv data, but we could probably also just print the data directly.)

The prototype still needs some work, but I think it demonstrates that this approach is valid. The least elegant part are the fmt::Display implementations in command.rs for the human-readable output, but that’s basically what we are currently doing, just with automatic alignment of the fields to make it easier to maintain.

Example 1: objects

$ nitrocli status
Status:
  model:             storage
  serial number:     0x00050000
  firmware version:  v0.53
  user retry count:  3
  admin retry count: 3
  Storage:
    SD card ID:      0x23efab16
    firmware:        unlocked
    storage keys:    created
    volumes:
      unencrypted:   read-only
      encrypted:     inactive
      hidden:        inactive

$ nitrocli status --output-format json
{
  "status": {
    "model": "storage",
    "serial_number": "0x00050000",
    "firmware_version": "v0.53",
    "user_retry_count": 3,
    "admin_retry_count": 3,
    "storage_status": {
      "card_id": 602909462,
      "firmware_locked": false,
      "stick_initialized": true,
      "unencrypted_volume": {
        "read_only": true,
        "active": true
      },
      "encrypted_volume": {
        "read_only": false,
        "active": false
      },
      "hidden_volume": {
        "read_only": false,
        "active": false
      }
    }
  }
}

$ nitrocli status --output-format tsv
key     value
admin_retry_count       3
firmware_version        v0.53
model   storage
serial_number   0x00050000
storage_status.card_id  602909462
storage_status.encrypted_volume.active  false
storage_status.encrypted_volume.read_only       false
storage_status.firmware_locked  false
storage_status.hidden_volume.active     false
storage_status.hidden_volume.read_only  false
storage_status.stick_initialized        true
storage_status.unencrypted_volume.active        true
storage_status.unencrypted_volume.read_only     true
user_retry_count        3

Example 2: tables

$ nitrocli otp status
algorithm       slot    name
hotp            1       test-hotp
hotp            2       test2
totp            1       test-totp
totp            2       test2

$ nitrocli otp status --output-format json
{
  "otp-slots": [
    {
      "algorithm": "hotp",
      "slot": 1,
      "name": "test-hotp"
    },
    {
      "algorithm": "hotp",
      "slot": 2,
      "name": "test2"
    },
    {
      "algorithm": "totp",
      "slot": 1,
      "name": "test-totp"
    },
    {
      "algorithm": "totp",
      "slot": 2,
      "name": "test2"
    }
  ]
}

$ nitrocli otp status --output-format tsv
algorithm       slot    name
hotp    1       test-hotp
hotp    2       test2
totp    1       test-totp
totp    2       test2

[d-e-s-o]

Haven't look in too much detail yet, but at a high level it looks great!

I've rebased this pull request against top of tree devel and changed passing of the execution context to be environment variable based.

[d-e-s-o]

Here is a prototype for generating one-time passwords by slot name, based on the extensions-v2 branch: https://github.com/robinkrahl/nitrocli/tree/otp-cache

One reason for picking up the extension work again is because I am freakin' annoyed by Nitrokey/libnitrokey#137 and the OTP hang (now that I am actually using nitrocli and its OTP capabilities). So, as sad as it is, I'd actually want a gpgconf --kill gpg-agent somewhere in there...(although that is a huge stop-gap measure in so many respects...).

[robinkrahl]

I am freakin' annoyed by Nitrokey/libnitrokey#137 and the OTP hang (now that I am actually using nitrocli and its OTP capabilities).

Me too. I’ve made it a habit to remove my Nitrokey after every OTP use, but that’s pretty annoying.

[robinkrahl]

What’s the next step for this PR?

[d-e-s-o]

What’s the next step for this PR?

I intend to roll this support out locally for me, get some experience with your otp-cache, see if we are missing/should change anything (off hand I'd say aliases may be nice; but of course they are mostly an orthogonal feature), and if not will polish up the pull request and get it out of the draft status.

So, as sad as it is, I'd actually want a gpgconf --kill gpg-agent somewhere in there...(although that is a huge stop-gap measure in so many respects...).

FYI: gpg-connect-agent 'SCD KILLSCD' /bye is less invasive and accomplishes the same goal based on my tests.

[d-e-s-o]

Okay, I think this logic is ready for review now. I suppose we could add a bit more to the man page. Not sure. Open to suggestions. Robin, mind taking a look?

[robinkrahl]

Thanks, Daniel! The architecture looks good to me, I just have some comments regarding implementation details.

And I’d like to define the extensions’ data and configuration directories in the man page, as described in my first comment in this PR (last heading).

[d-e-s-o]

Thanks for your review, Robin! I addressed some of your comments. Will have to think a bit about the "Writing extensions" section. Not happening today.

[d-e-s-o]

FYI, for me the following is broken:

$ NITROCLI_SERIAL_NUMBERS='' nitrocli status
Failed to parse environment variables

Caused by:
    Library error: The supplied string is not in hexadecimal format

(I noticed that because that's what otp-cache failed with now)

I think supporting an empty variable for a Vec<String> is the right thing to do -- to me empty means no elements. Initially I had fixed that up: 53e48ea. But I got the feeling I may be missing something and that the fix perhaps should not be necessary (it's also ugly and introduces more weirdness now). Somehow the serde behavior seems strange to me. Let me know if you have an idea.
I think we also should think about adding tests for those environment variables & config options.

[robinkrahl]

Thanks for the update! I’ve opened an issue for the vector deserialization problem: softprops/envy#50

[robinkrahl]

Looks good! I still want to change the man page (separate the information for users and programmers, define the data directories), but we can discuss that separately.

[d-e-s-o]

Looks good! I still want to change the man page (separate the information for users and programmers, define the data directories), but we can discuss that separately.

Yes, absolutely. I'll look into this soon.

[d-e-s-o]

Okay, I've adjusted the man page a bit. Not super happy with it, but perhaps it will do. Let me know what you think. I'd prefer to use bold & italic for the extension part in \fB${XDG_DATA_HOME}/\fIextension/\fR. But I can't seem to figure out the syntax...

Do you think we should list these directories in the FILES section?

[robinkrahl]

Okay, I've adjusted the `man` page a bit. Not super happy with it, but perhaps it will do. Let me know what you think.

Thanks, I’ll have a closer look later today!

I'd prefer to use bold & italic for the `extension` part in `\fB${XDG_DATA_HOME}/\fIextension/\fR`. But I can't seem to figure out the syntax...

As far as I know, you can select only one of roman (= regular), italic and bold. Combining bold and italic is not possible.

Do you think we should list these directories in the `FILES` section?

No, I think that’s not necessary. Extensions should have their own man pages that list the files they actually use. Many extensions might not even need it.

[d-e-s-o]

I've added some copy-to-clipboard functionality to otp-cache: https://github.com/d-e-s-o/nitrocli/tree/topic/otp-cache

[d-e-s-o]

Merged.

[d-e-s-o]

Thanks for the update! I’ve opened an issue for the vector deserialization problem: softprops/envy#50

Seems envy is no longer maintained. At least not in any acceptable form, if pull requests are not acknowledged in a week's time. Any suggestions what we do about that?