Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access PWS slots by name #155

Open
robinkrahl opened this issue Apr 17, 2021 · 8 comments · May be fixed by #156
Open

Access PWS slots by name #155

robinkrahl opened this issue Apr 17, 2021 · 8 comments · May be fixed by #156

Comments

@robinkrahl
Copy link
Collaborator

Similar to #83, we also want to access PWS slots by name. But we need the user PIN to read the slots (while we were able to read the OTP slots without authentication). So this leaves us with these options:

  1. Copy the pinentry module from nitrocli to nitrocli-ext, use it to query the user PIN and then use nitrokey-rs to access the PWS.
  2. Same as 1., but move the pinentry module from nitrocli to nitrocli-ext (and make nitrocli depend on nitrocli-ext).
  3. Call nitrocli pws status and parse its output.

@d-e-s-o What do you think?

robinkrahl added a commit to robinkrahl/nitrocli that referenced this issue Apr 17, 2021
This patch adds the pws-cache core extension that allows accessing the
PWS slots by their name instead of the slot index.

Fixes d-e-s-o#155.
@robinkrahl robinkrahl linked a pull request Apr 17, 2021 that will close this issue
@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 17, 2021

Oh, interesting. My preference would be 3), as that's basically how we envisioned extensions to begin with (at least that was one of the take aways from the discussion that I had). However, that then would again mean that we should have some kind of stable output first. Perhaps now would be the time to get that done?

@robinkrahl
Copy link
Collaborator Author

I agree. I’ll have a look at the output-formats branch and check what’s missing.

If we have serde-able data structures, where should we place them? While the pws status output is trivial, other commands like status are more complex. I’d rather not duplicate them in the extensions. So I think it would make sense to extract them either into nitrocli-ext or into nitrocli-serde (?). What do you think?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 23, 2021

I'll have to look at your branch again, too. I recall that I wasn't very fond of a few things, but also didn't have better ideas :-|

If we have serde-able data structures, where should we place them?

I think for one I was not sure whether we really should have data structures for the output. I was more thinking along the lines of invoking a couple of functions and switching out the functions based on the desired format. (I don't know how feasible that is, but it was my initial thought)

Even if we decide to have data structures, I'd just keep them local to nitrocli. Every language other than Rust will need to parse the output based on some description and I think that I'd want that description to be the defined interface.

We could have a crate for data structures shared between extensions if really multiple core extensions want to parse the same output, but I'd rather we think of it as sharing common functionality between extensions only, without any direct connection between nitrocli and said crate (tentatively, they could fit into nitrocli-ext, but I'd also be fine with just implementing what is necessary in pws-cache itself for now until a second client comes along and then deciding what to do; even with two clients a duplication is not the end of the world). I don't think we want to have a dependency from nitrocli on another crate like that, at least that was my thinking so far.

I hope that makes some sense. I'll see if I can take a closer look at output-formats on the weekend.

Edit: Basically, I think of nitrocli-ext as an implementation detail rather than a stable API (and similar crates would fall into the same category). For that reason, I also did not want to publish it to crates.io either. But I don't claim to have 100% finalized the distribution story for extensions. Perhaps that needs a different thread/issue to discuss.

@robinkrahl
Copy link
Collaborator Author

Okay, let’s take a step back. Do you agree that the machine-readable output format should be part of the public API and therefore subject to the stability guarantee?

Basically, I think of nitrocli-ext as an implementation detail rather than a stable API (and similar crates would fall into the same category). For that reason, I also did not want to publish it to crates.io either. But I don't claim to have 100% finalized the distribution story for extensions. Perhaps that needs a different thread/issue to discuss.

Definitely – I’ve created #165 and #166 for this discussions.

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 23, 2021

Okay, let’s take a step back. Do you agree that the machine-readable output format should be part of the public API and therefore subject to the stability guarantee?

Yep, in the sense that we say that we are producing, for example, a nested JSON object with these keys & values. Not in the sense that we provide the infrastructure for parsing it in any shape or form.

@robinkrahl
Copy link
Collaborator Author

But if we guarantee that there is a JSON object with some fields, what is the downside to publishing a Rust data structure with the same fields? Isn’t it just a different view of the same information?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Apr 24, 2021

It's a concrete implementation. Now we have to maintain two versioned interfaces: the definition of the interface itself as well as some implementation that we came up with.

@robinkrahl
Copy link
Collaborator Author

We have to maintain both (= the code and the specification/documentation) anyway. The only difference is whether we publish it separately or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants