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.

Sign up for GitHub

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

Jump to bottom

Add otp-cache extension #146

Closed
wants to merge 9 commits into from
Closed

Add otp-cache extension #146

wants to merge 9 commits into from

Conversation

robinkrahl
Copy link
Collaborator

This PR is an iteration of PR #144: Add cache extension. It introduces the
NITROCLI_RESOLVED_USB_PATH environment variable to avoid having to duplicate
the device selection logic, it moves the ext.rs module into a new extension
support crate, nitrokey-ext, and adds the nitrocli-otp-cache extension.

@robinkrahl
Pass on resolved USB path to extensions
d07d1d8 
To avoid reimplementing the device selection logic in extensions, we
introduce a new environment variable NITROCLI_RESOLVED_USB_PATH that is
set to the USB path of the single matching Nitrokey device.  If no
device matches, or if there are multiple matching devices, the variable
is not set.
@robinkrahl
Add extension support crate nitrocli-ext
176f751 
This patch adds the extension support crate nitrocli-ext as a workspace
member.  This crate contains useful methods for extensions written in
Rust, providing access to the nitrocli binary and to the nitrokey-rs
library.
@robinkrahl
Add otp-cache extension
f0beb56 
This patch adds the nitrocli-otp-cache extension that caches OTP data.
The per-device cache stores the names, OTP algorithms and IDs of the
slots It can be used to access the slots by name instead of slot index.
@robinkrahl robinkrahl mentioned this pull request Apr 12, 2021
Closed
d-e-s-o
d-e-s-o reviewed Apr 13, 2021
View reviewed changes
Copy link
Owner

@d-e-s-o d-e-s-o left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it looks great! Thanks for splitting and reworking the earlier pull request! I may only get to take a final look and be able to merge it later this week (around Thursday or later).

ext/ext/src/lib.rs Outdated Show resolved Hide resolved
ext/ext/src/lib.rs Outdated Show resolved Hide resolved
ext/otp-cache/src/main.rs Outdated Show resolved Hide resolved
ext/otp-cache/src/main.rs Outdated Show resolved Hide resolved
ext/otp-cache/src/main.rs Outdated Show resolved Hide resolved
@robinkrahl
Copy link
Collaborator Author

Thanks for the review! I’ll fix the issues and update the PR later today.

@robinkrahl
Copy link
Collaborator Author

I think I’ve addressed all issues.

@d-e-s-o
Copy link
Owner

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

Hi Robin, why do you think we should have a dedicated update command? Conceptually I am fine with it (just as an alternative to --force-update), but it seems unnecessary to require users to use it. Can you please share your thoughts?

@robinkrahl
Copy link
Collaborator Author

Good point! The cache lookup logic became pretty convoluted so I decided to separate reading and writing the cache. (For example we have to make sure that the nitrokey::Manager is dropped before calling nitrocli.) But I‘m fine with including the update logic in the get command too. I’m not sure which way is more intuitive and useful.

@d-e-s-o
Copy link
Owner

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

Sounds good. I vote for integration.

"There is no cached slot data. Run the update command to initialize the cache."

If a computer can tell me precisely what to do most of the time I just want it to do it for me. It was definitely the case for me here.

@robinkrahl
Copy link
Collaborator Author

I’ve removed the update command.

@robinkrahl
Copy link
Collaborator Author

Do we want separate man pages for extensions?

@d-e-s-o
Copy link
Owner

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

Awesome, thank you!

Do we want separate man pages for extensions?

It's definitely not a priority right now for me. I'd hope we could cover most of it with a some decent help text. I'd rather we think about tests first on the extension front.

@d-e-s-o
Copy link
Owner

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

Fantastic work, Robin! Merged into devel.

@d-e-s-o d-e-s-o closed this Apr 17, 2021
@robinkrahl robinkrahl deleted the ext-otp-cache branch April 17, 2021 19:22
@robinkrahl robinkrahl mentioned this pull request Apr 17, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-e-s-o d-e-s-o d-e-s-o left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@robinkrahl @d-e-s-o