Support Signing and Authentication subkeys#358
Conversation
|
@jrruethe Have you tried generating a ssh key after these changes? I just tried and am getting the following error:
It appears there is a parameter in |
|
I have not, but I can look at that and fix it. I only did testing with GPG, not SSH. Thanks for the heads up! |
|
@slvrfn I believe I fixed the issue you had with SSH. |
|
Hey, awesome work on this @jrruethe! Just wanted to check what the status of this is? |
|
@lukechilds I've been using my branch without issues for my own purposes, so for what I needed it is complete. I used it to generate deterministic subkeys from the Trezor and load them onto my Yubikey 5 NFC, which can then be used with the standard GPG installation. |
|
Sorry for the very delayed response... |
@jrruethe Do you have a writeup or can you point me in the right direction for moving loading subkeys onto the Yubikey 5 NFC. |
@jrruethe Hi Joe! This is exactly what I wanted to get working. As @McCodeman would you please be able to provide instructions on how to do exactly what you said? I would love to backup the Trezor master key and create/revoke subkeys and use them with my Yubikey. Sounds amazing! Thanks!! |
|
Essentially, I used the script in this pull request "contrib/trezor_agent_recover.py" with the "--smartcard" option. I used a Tails environment that was not connected to the internet, and entered my Trezor seed into the script, which output the private GPG keys (both the master and subkeys). After that, I just used the standard instructions for moving the subkeys to a Yubikey, such as this guide: https://github.com/drduh/YubiKey-Guide There isn't a way to extract the private keys directly from the Trezor itself, which is why I supplied the recovery script. The recovery script generates the same keys as the Trezor when the same seed is used. This means that signing and encryption operations done with the Trezor can be used with the Yubikey, and vice versa. |
Thanks for replying so quick! I'm an absolute novice on github. I don't mind running the python file on my regular computer as I'm just testing around with my Trezor, there's nothing on it. My question is how do I just run the file? Just open Terminal and run the saved file? Or do I enter my seed somewhere into the python script and run it? I did a shamir seed, so I would have enter the entire thing? Then do I remove do a "rm -r trezor" that I currently have and then run the script as if I'm doing the "trezor-gpg init... -- smartcard" command? I sent you an email in hopes that you can guide me, I don't mind compensating for your time as I'm very interested in this. I came across GPG not long ago and became very interested. Thanks! |
|
@jrruethe sorry, how do you run this version of the script instead of the usual "trezor-gpg init..." ? |
|
For an ubuntu-based system, the following commands should get you started: |
|
@jrruethe thanks for getting back! I got a traceback, maybe I'm doing something wrong?
|
|
@jrruethe any advice for why I'm getting the Traceback for the Keyflags error that I listed above? |
|
@jrruethe your |
|
Unfortunately, I don't have the capacity to work on this any more right now. For SSH, I have my GPG agent set up to also be an SSH agent. SSH connections utilize the authorization subkey on the Yubikey. This guide has instructions on how to go that route: |
|
No worries @jrruethe, that sounds ideal. I'll look into it, thanks! |
| curve_name_to_curve = {"nist256p1": curves.NIST256p} | ||
| curve_name_to_seed = {"nist256p1": "Nist256p1 seed"} |
There was a problem hiding this comment.
@jrruethe Thanks for this work!
Any chance of you having some time to add ed25519 support to your script and rebase on top of master?
I'd be very much interested in using it with my current keys, but they're edwards.
Thanks
|
@jrruethe Could you please rebase your PR over latest |
|
I'll try to do this soon, I gotta test it out again with a real trezor |
|
After rebasing, I can't seem to decrypt files, so apparently I messed something up. I'll try to dig in deeper if I get a chance |
|
@romanz Ok, I got it all working and retested it with a real trezor. I also tested the Ready to merge! Thanks! EDIT - I certainly hope I did that right, my git is not as strong as it should be :) |
|
I suggest cherry-picking only your commits into a clean branch over |
|
It seems that CI didn't run on this PR... My bad - fixed in 2b49eac (please rebase over latest |
romanz
left a comment
romanz
left a comment
There was a problem hiding this comment.
@jrruethe Could you please fix the CI errors here:
https://github.com/romanz/trezor-agent/actions/runs/5162519295/jobs/9300268171#step:5:67
You can also run the tests locally via tox, which used to run the following checks:
$ tox
...
py3: commands[0]> pycodestyle libagent
py3: commands[1]> isort --skip-glob .tox -c libagent
py3: commands[2]> pylint --reports=no --rcfile .pylintrc libagent
py3: commands[3]> pydocstyle libagent
py3: commands[4]> coverage run --source libagent -m pytest -v libagent
...
|
I think thats better? |
|
@romanz Anything else needed to get this merged? |
|
@jrruethe any chance of getting the same capabilities for |
|
@andrevmatos I don't have any plans to implement that right now. I am just trying to get the current PR completed. |
Will do the final review this weekend - many thanks for the contribution! |
There was a problem hiding this comment.
Let's keep this file in a separate repository/snippet please.
I prefer not to instruct the users to enter their seed on a non-secure device.
| def create_identity(user_id): | ||
| """Create AGE identity for hardware device.""" | ||
| result = interface.Identity(identity_str='age://', curve_name="ed25519") | ||
| result = interface.Identity(identity_str='age://', curve_name="ed25519", keyflag=None) |
There was a problem hiding this comment.
Wouldn't keyflag_to_index throw an exception?
$ age-plugin-trezor -i foo
2023-06-09 19:03:29,483 ERROR age plugin failed: None [__init__.py:178]
Traceback (most recent call last):
File "/home/user/trezor-agent/libagent/age/__init__.py", line 174, in main
run_pubkey(device_type=device_type, args=args)
File "/home/user/trezor-agent/libagent/age/__init__.py", line 47, in run_pubkey
pubkey = c.pubkey(identity=client.create_identity(args.identity), ecdh=True)
File "/home/user/trezor-agent/libagent/age/client.py", line 15, in create_identity
result = interface.Identity(identity_str='age://', curve_name="ed25519", keyflag=None)
File "/home/user/trezor-agent/libagent/device/interface.py", line 69, in __init__
self.identity_dict['index'] = keyflag_to_index(keyflag)
File "/home/user/trezor-agent/libagent/formats.py", line 49, in keyflag_to_index
return {
KeyError: None
| user_ids += [p for p in packets if p['type'] == 'user_id'] | ||
|
|
||
| for packets in packets_per_key: | ||
|
|
There was a problem hiding this comment.
nit: remove the line
| # The key packet contains the keygrip | ||
| # The signature packet contains the keyflag in the hashed area | ||
| # Map them together | ||
| # pylint: disable=consider-using-enumerate |
There was a problem hiding this comment.
Why not to use for i, p in enumerate(packets) below?
| mapping = {} | ||
| for i in range(0, len(packets)): | ||
| p = packets[i] | ||
| if p['type'] == 'pubkey' or \ |
There was a problem hiding this comment.
nit: no need to break into 2 lines
| if p['type'] == 'pubkey' or \ | |
| if p['type'] == 'pubkey' or p['type'] == 'subkey': |
There was a problem hiding this comment.
it's just a random note by a random passerby, but... i also like structuring boolean trees as one entry per line. it's easier to parse for my brain when the textual structure is in sync with the semantic structure.
| embedded_sig = None | ||
| else: | ||
| if subkey.keyflag == KeyFlags.SIGN: | ||
|
|
There was a problem hiding this comment.
nit: remove empty line
| KeyFlags.SIGN, | ||
| KeyFlags.AUTHENTICATE, | ||
| KeyFlags.CERTIFY_AND_SIGN): | ||
|
|
| self.ecdh_packet = b'' | ||
|
|
||
| elif keyflag == KeyFlags.ENCRYPT: | ||
|
|
| @@ -181,7 +182,8 @@ def parse_config(contents): | |||
| """Parse config file into a list of Identity objects.""" | |||
| for identity_str, curve_name in re.findall(r'\<(.*?)\|(.*?)\>', contents): | |||
There was a problem hiding this comment.
Does this regexp work for the new identity format?
|
|
||
| def _create_identity(user_id): | ||
| result = interface.Identity(identity_str='signify://', curve_name='ed25519') | ||
| result = interface.Identity(identity_str='signify://', curve_name='ed25519', keyflag=None) |
There was a problem hiding this comment.
Wouldn't keyflag_to_index throw an exception?
|
I also tested it using It does seems to work on @jrruethe Could you please take a look? |
The reason for the signing failure is that the key is being generated as |
|
any update to this? |
|
@talosgt There is an open issue (#358 (comment)) with this PR. |
|
@jrruethe have you had a chance to look into the open issue? |
9 participants
This pull request is for adding a Signing subkey and an Authentication subkey, in addition to the existing Encryption subkey. It also changes the Primary key to be Certify only. This structure matches best practices found here and here.
One of the benefits of this structure is that it becomes possible to use the Trezor to deterministically generate the Primary and Subkeys, use the included recovery script to mimic the generation and recover the secret portions (suggested to only do this in an offline Tails environment), then move the secret subkeys to a smartcard device like a Yubikey 5. This allows one to achieve the security and determinism of the Trezor (especially when utilizing Shamir), while maintaining the compatability and convienience of a smartcard like the Yubikey. Note that with this scheme, the Primary (certifying) key remains completely offline, and is not intended to be used.
The current code creates a Primary key with Certify and Sign capabilities, and a single Encryption Subkey. The Primary key uses SLIP 13, while the Encryption Subkey uses SLIP 17. Both use derivation index 0 of their respective schemes. The code uses a single boolean named
ecdhto differentiate the two.This pull request adds an enum named
KeyFlags. This enum accomplishes 3 things simultaneously:The derivation index comes into play because we want the Primary Key, Signing Subkey, and Authentication Subkey to all be different. I chose to use derivation indices 0, 1, 2 for these respectively. The derivation index is normally passed to the Trezor via the IdentityType message, using a field named
index. Currently, this value is not set bytrezor-agent, so it defaults to 0. In order for _identity_proto to properly set the index, it needs to be available in the identity_dict of the Identity class. Per SLIP 13/17, the derivation index is separate from theidentity_str(RFC 3986 URI), therefore the index is derived from aKeyFlagparameter added to the constructor. This change does end up rippling through some code, notably necessatating an Identity for each Public Key created in init.A majority of the rest of the changes involve replacing the
ecdhvariable with theKeyFlagsenum and adjusting functions and calls accordingly.A second change was needed to accomplish the goal. Now that multiple subkeys (and therefore multiple
KeyFlags) are at play, thedecodeclass needed to be able to parse theseKeyFlagsfrom the keys. TheKeyFlagsare stored inside the hashed area of thesignaturepackets for a key. According to page 69 of RFC 4880, thesignaturepacket always follows thepubkey/subkeypacket, so the load_by_keygrip was rewritten to map them together and return it to get_identity. A unit test was added for this.Finally,
gpgversion2.1.14adds a new subpacket type33, which is not part of RFC 4880 (and has caused some problems for other clients). It is used for verifying cross-certification of signing subkeys, and is only specifically required for signing subkeys (not authentication subkeys, this was verified by usinggpgto generate a new key with subkeys, then dump the packets). Here are some handy links:That last link shows a "gotcha", where the signature creation date must be greater than 0 for
gpgto register it as a cross-certification signature. I chose to add 1 to the embedded signature creation time. This does not affect the public keys themselves.Lastly, I rearranged some fields to match the same order that
gpgproduces, refactoredlibagent/gpg/__init__.pysince the inclusion of the subkeys has introduced some repetition.I did multiple tests to ensure backward compatability. First, I wiped a Trezor and initialized it with a mnemonic of
all all all all all all all all all all all all. Then, I used the current version oftrezor-agentto both sign and encrypt a file.Next, I used the version of
trezor-agentfrom this pull request to both verify and decrypt the file, using the "normal" key layout. The fingerprints and keygrips were also compared and found to be the same. Afterwards, I did the test again by passing the--smartcardflag to generate the three subkeys. The fingerprint and keygrip of the certifying key and encryption key are the same, as expected, and the "smartcard" layout is able to both verify and decrypt the same file.Lastly, I used the
contrib/trezor_agent_recovery.pyscript to recover the public and private keys. The public key was compared to the one generated bytrezor-agent init --smartcardand found to be the same, and the private key was the proper pair. Using the private key, I was able to decrypt the file without the Trezor.This confirms the ability to both generate and recover the private subkeys, for moving to a Yubikey 5.
I tried to maintain the same coding style that was present, and made sure that
pylintandpytestboth passed. If you have any questions, let me know. Thank you!