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

Select OTP slots by name #83

Closed
bircoph opened this issue May 24, 2019 · 10 comments
Closed

Select OTP slots by name #83

bircoph opened this issue May 24, 2019 · 10 comments
Assignees
Labels
extension Functionality that is related to extensions, in one form or another

Comments

@bircoph
Copy link

bircoph commented May 24, 2019

Hi!

Please add functionality to select OTP slots by name, e.g. if I have many slots it is more convenient to call
nitrocli otp get github
than
nitrocli otp get $(nitrocli otp status | awk '$3 ~ /github/ { print $2 }')
or remembering in what slot github's OTP lives.

Of course it may cause problems if several slots have the same name (report an error than or get all of them) or if a slot name is a number within valid slot number range (for such cases a suboption indicating whether slot name or slot number was requested may be added).

@d-e-s-o
Copy link
Owner

d-e-s-o commented May 25, 2019

I do see why remembering slot numbers would be cumbersome. But truth be told, I haven't used the otp functionality much, so I can't really judge how bad it is to work with what we have.
That being said, I am not a fan of overlaying arguments with functionality as you are proposing. There rarely ever comes anything good out of that and it is pretty much always a source of bugs/unexpected behavior.

So my tendency would be to get rid of the means for addressing slots by number altogether (although off hand I can't say whether that is possible; there may be some bit that I am missing) and only work with names (well, perhaps add a way to disambiguate in case a name is ambiguous).

@robinkrahl do you have an opinion on this request by any chance?

@robinkrahl
Copy link
Collaborator

We can only query the name for one slot at a time. So if we would remove access by number, we would have to execute n + 1 commands instead of only one command to retrieve an OTP code (where n is the number of used OTP slots). Therefore I don’t want to change the behaviour of the otp command.

As discussed in #50, I’d prefer an extension, let’s call it otpcache, that caches the names of the OTP slots and also provides an option to get an OTP code by slot name. So the first invocation of the otpcache subcommand would initialize the cache (per device). It could be updated with nitrocli otpcache update, and queried using nitrocli otpcache status or nitrocli otpcache get <name>.

@d-e-s-o
Copy link
Owner

d-e-s-o commented May 27, 2019

Ah, thanks for refreshing my memory here, Robin. :-)

I agree with your inclination of making this an extension. Yet another reason to finally prioritize this work...

@d-e-s-o d-e-s-o added the extension Functionality that is related to extensions, in one form or another label Jun 8, 2019
@robinkrahl
Copy link
Collaborator

As the v0.4.0 release includes support for extensions (thanks!), we should be able to implement this feature. There is a prototype on the topic/otp-cache branch. Do you plan to continue working on this branch? Should I have a look at it?

@d-e-s-o
Copy link
Owner

d-e-s-o commented Mar 22, 2021

Feel free to give it a go, Robin. It was your original proposal. It's working great for me, but it requires the killing of scdaemon to prevent smartcard lock ups as I am sure you recall. I am not sure we want to keep that, but at the same time without that or a different workaround the functionality basically has negative value for me (and I suspect for others).
Clipboard management is another issue: not sure how many ways there are to set the clipboard on Linux alone. But I suppose that does not have to be included in a first version.

@robinkrahl
Copy link
Collaborator

I think the first version should be minimal and include neither the scdaemon hack nor the clipboard access. (We already have the scdaemon problem with the otp subcommand. I don’t think we should treat the otpcache extension differently.)

I’ll personally use the extension with a wrapper script that opens dmenu, like passmenu. This wrapper script will probably contain the scdaemon hack and the clipboard handling. We could add this script to the repository too.

Anyway, I’ll continue to work on the draft and open a PR once it’s ready.

@robinkrahl
Copy link
Collaborator

I realized that we have the same problem with PWS, so I’d suggest to have a cache extension with otp and pws subcommands instead.

@robinkrahl robinkrahl self-assigned this Apr 9, 2021
@d-e-s-o
Copy link
Owner

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

Sounds good!

@robinkrahl
Copy link
Collaborator

#146 has been merged (thank you!) so we can close this issue, right?

@d-e-s-o
Copy link
Owner

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

Yep.

@d-e-s-o d-e-s-o closed this as completed Apr 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension Functionality that is related to extensions, in one form or another
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants