Skip to content

YubiKey plugin#1

Closed
str4d wants to merge 4 commits intomainfrom
yubikey
Closed

YubiKey plugin#1
str4d wants to merge 4 commits intomainfrom
yubikey

Conversation

@str4d
Copy link
Copy Markdown
Owner

@str4d str4d commented Aug 30, 2020

Discuss this draft on the age-dev mailing list thread!

Current draft specification:

https://hackmd.io/@str4d/age-plugin-yubikey

@str4d str4d mentioned this pull request Aug 30, 2020
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 30, 2020

Codecov Report

❗ No coverage uploaded for pull request base (main@05f7f1b). Click here to learn what that means.
The diff coverage is 2.86%.

Impacted file tree graph

@@          Coverage Diff           @@
##             main      #1   +/-   ##
======================================
  Coverage        ?   2.86%           
======================================
  Files           ?       5           
  Lines           ?     279           
  Branches        ?       0           
======================================
  Hits            ?       8           
  Misses          ?     271           
  Partials        ?       0           
Impacted Files Coverage Δ
src/format.rs 0.00% <0.00%> (ø)
src/main.rs 0.00% <0.00%> (ø)
src/p256.rs 0.00% <0.00%> (ø)
src/plugin.rs 0.00% <0.00%> (ø)
src/yubikey.rs 7.01% <7.01%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05f7f1b...9b3cc73. Read the comment docs.

@str4d str4d force-pushed the yubikey branch 6 times, most recently from e89b729 to 9b3cc73 Compare August 31, 2020 11:36
@joonas-fi
Copy link
Copy Markdown

Adding my 5 cents: I would not call it "A YubiKey stanza", but instead "P-256" and not even tie it to PIV (= smartcard stuff), because all of the operations are:

  • something that can be done in another manufacturer's PIV implementation
  • or even in a software smartcard implementation
  • or even outside of PIV context. It's just P-256
  • the plugin itself is fine being scoped to YubiKey-stored P-256. I'd just decouple the Age stanza and YubiKey from each other.

@joonas-fi
Copy link
Copy Markdown

where SEC-1-C is the 33-byte compressed SEC-1 encoding.

FYI in case you're not aware, if "compressed encoding" means omitting the y coordinate, it needs to be re-computed from x and it seems to be pretty expensive: https://twitter.com/joonas_fi/status/1318829182743359489

@codecov-io
Copy link
Copy Markdown

codecov-io commented Nov 14, 2020

Codecov Report

Merging #1 (169029d) into main (05f7f1b) will increase coverage by 3.42%.
The diff coverage is 3.42%.

Impacted file tree graph

@@           Coverage Diff           @@
##           main      #1      +/-   ##
=======================================
+ Coverage      0   3.42%   +3.42%     
=======================================
  Files         0       5       +5     
  Lines         0     292     +292     
=======================================
+ Hits          0      10      +10     
- Misses        0     282     +282     
Impacted Files Coverage Δ
src/format.rs 0.00% <0.00%> (ø)
src/main.rs 0.00% <0.00%> (ø)
src/p256.rs 0.00% <0.00%> (ø)
src/plugin.rs 0.00% <0.00%> (ø)
src/yubikey.rs 8.77% <8.77%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05f7f1b...169029d. Read the comment docs.

@str4d str4d force-pushed the yubikey branch 2 times, most recently from 65670c1 to 7f1834e Compare December 31, 2020 03:17
@str4d
Copy link
Copy Markdown
Owner Author

str4d commented Dec 31, 2020

Now that (unstable) plugin support is merged into rage (str4d/rage#99), I'll be working on this PR again!

@joonas-fi The stanza tag used in the header is going to end up named something like piv-p256, since (as you note) it's not tied to YubiKey specifically (though it is tied to a specific protocol involving choices like including tags; it's not just P-256). The recipients will still be named age1yubikey because that's how the age plugin system will know to look up this plugin.

Re: the cost of decompression, it only needs to be paid at recipient parsing time, and we gain more benefit from having shorter recipient strings. I think the current implementation might end up decompressing once per wrapped stanza, but that's easy to fix if it actually proves to be a bottleneck (vs the actual file encryption).

YubiKeys are managed as age identities via a "stub" that indicates the
slot to be used on a particular YubiKey. The stub can be placed
alongside any age keys, and tells rage that it should attempt to decrypt
matching YubiKey recipient lines.
Generates stubs for existing identities, and new identities for empty
slots.
@jstasiak
Copy link
Copy Markdown

I tested this with YubiKey 4 on Mac and it's working very nicely! The only issue I found is I need to enter PIN on every decryption even though the PIN policy has been set to "once" (?).

I opened a PR (#3) to address a use case that's important to me (first mentioned here: str4d/rage#25 (comment)).

@str4d
Copy link
Copy Markdown
Owner Author

str4d commented Jan 11, 2021

Hi, thanks for testing! Just FYI, I reworked this branch on a recent Twitch stream, and will shortly be opening new PRs with the improved plugin implementation.

Re: always needing a PIN, that's an artifact of plugins being started fresh each time rage runs. This is intentional; plugins are short-lived. The reason I offer a Once policy is because it will be used by an agent plugin. The expectation is that yubikey-agent will add support for this recipient type, and then this plugin will be used for encryption, while the agent can be used for decryption if the user desires.

@jstasiak
Copy link
Copy Markdown

jstasiak commented Jan 11, 2021

Cool, I'll update my branch when you post your changes. Fair enough on the PIN prompts, I somehow thought the device kept track of PIN entries and it wasn't per-session.

Edit: "kept track" as in "if PIN was entered once since the device has been plugged in then it wouldn't be requested anymore for any app".

@str4d str4d marked this pull request as draft January 11, 2021 15:08
@str4d
Copy link
Copy Markdown
Owner Author

str4d commented Jan 11, 2021

PIN entries are only cached per-connection to the YubiKey, so decrypting multiple files with the same YubiKey would leverage a Once policy correctly (not that age or rage support that use-case, but the plugin system does), but you need some process holding that connection open (and in doing so, holding a lock on the YubiKey) if you want it to last beyond a single plugin invocation.

@jstasiak
Copy link
Copy Markdown

Thanks, that's what I figured. You mentioned yubikey-agent, is it about this one? https://github.com/FiloSottile/yubikey-agent If there was a single agent that'd serve both my SSH and (r)age needs it'd be glorious.

@str4d str4d mentioned this pull request Jan 11, 2021
@jstasiak
Copy link
Copy Markdown

The expectation is that yubikey-agent will add support for this recipient type, and then this plugin will be used for encryption, while the agent can be used for decryption if the user desires.

One more question about this: would this reuse the SSH agent protocol and extend it to support decryption or provide a separate protocol for (r)age? Is there an existing discussion of this yubikey-agent extension? I've played around with sshcrypt[1] and yubikey-agent (a version modified by me to get deterministic RSA signatures working instead of ECDSA signatures which are randomized) to get encryption and decryption using SSH keys with agent support but it has a range of downsides (like: encryption requires SSH sign operation with a touch action and requiring signature to encrypt means a third party can't encrypt stuff to me). I had an idea for an age plugin that implements something like that using SSH agent but it'd have the same set of downsides which is a showstopper for me.

[1] https://github.com/leighmcculloch/sshcrypt

@str4d
Copy link
Copy Markdown
Owner Author

str4d commented Feb 1, 2021

Replaced by #8.

@str4d str4d closed this Feb 1, 2021
@str4d
Copy link
Copy Markdown
Owner Author

str4d commented Feb 1, 2021

One more question about this: would this reuse the SSH agent protocol and extend it to support decryption or provide a separate protocol for (r)age? Is there an existing discussion of this yubikey-agent extension?

The existing discussion is mostly in @FiloSottile's Twitch streams IIRC, along with he and I chatting about it. I suspect it will be a protocol specific to yubikey-agent, i.e. yubikey-agent would speak the SSH agent protocol for SSH usage, and then have its own protocol between its short-lived plugin binary and its long-lived agent. Let's shift that conversation over to yubikey-agent.

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 this pull request may close these issues.

5 participants