-
Notifications
You must be signed in to change notification settings - Fork 152
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
GPG advanced key management #446
base: master
Are you sure you want to change the base?
Conversation
Okay, I have not a single clue why CI is failing. |
239df93
to
c0357c0
Compare
Nevermind, I figured it out |
Many thanks for the contribution! |
I'm... not sure how. I know it's big, but it's all part of the same thing. I don't think I could even split it into several commits in any way that makes sense and/or compiles. Just as an example, removing the policy URI packet (which is also done in #298) would prevent the ability to It's all interconnected. |
Maybe it would be please possible to split the following parts into separate commits/PRs?
|
@romanz Sorry for the delayed response. I wanted to be 100% certain about my answer. I'm rather unhappy with the changes you've mentioned. They were mostly hacked together to make testing the GPG changes easier. To a certain extent, the GPG depends on them, but not vice versa, so you are correct in that it can be separated to another PR. However, with your permission, I would like to try porting trezor-agent to trio. I've tested, and it seems to be able to handle signals in a sane way on both Windows and Linux. Although I've only done basic tests and didn't try spawning a server, it's showing signs of having less gotchas than asyncio and Python itself. And it's certainly a better solution that abusing ctypes and hoping it's not broken on certain platform variants in unexpected ways. It would be a big patch, though, since it's essentially replacing all the IO. Your thoughts? (P.S. In the mean time, I've finished another PR that's a bit simpler) |
IIRC, woudn't that require trezorlib to be async-friendly, to prevent blocking the event loop? |
No. You just put all the device stuff in a thread. There are three ways to handle the concurrent calls it would inevitably create. First, avoid them at the caller level, since you can (or rather, have to) just await the thread. If you have concurrent tasks, you can do locks between them. Second, have a regular lock inside the thread. It won't influence the awaiting, since that would allow other tasks to run. Third, and this is one I've done in not-Python a lot, spawn a single thread that acts as a job queue, receiving tasks from the other threads, and executing them one at a time in order. TL;DR You just use |
Just to be sure - is the main issue cross-platform signal handling? |
It's not so much the signal handling itself. Rather, it's the lack of a way to wait for the first of multiple blocking actions, and signals is where it manifests the worst. Another way to look at it is that there's no straightforward way to wait for the first of two For something like sockets, you can create a thread per socket, and you can close the socket from another thread to cancel the operation (which is how you'd usually use a cancel, anyway). Not efficient, but works. With signals, such a workaround does not exist. The main reason is that Python's signal handler doesn't actually run registered callbacks, but simply sets a flag that makes the handler run after the next IO/blocking function ends in the main thread. This means, the following would freeze: hup_event = threading.Event()
def handler(_):
hup_event.set()
signal.signal(SIGHUP, handler)
hup_event.wait() Even if SIGHUP is sent, the handler will not be called until Even if Mind you, this issue is unique to Python (specifically, CPython), due to the way it designed its signal handling and GIL. |
Unfortunately I won't be available to review this and the suggested PR in the upcoming weeks... |
c0357c0
to
719bce7
Compare
Rebased on top of #456 and added support for deleting keys (requires I'm still contemplating extending derivation path setting to primary keys, and refactoring the way the derivation path is extracted for a key. |
@romanz Okay, I've had a thought about taking this PR in a completely different direction. And I want your opinion. Normally, GPG-agent is supposed to manage private/secret keys. It stores them, plain or encrypted, in Trezor-GPG naturally doesn't store any private keys. Instead, it pretends to store them, by querying the public keys stored, and claiming it has a private key for any public key which has a specific metadata attached to it. This method creates two problems:
My suggestion is therefore to copy GPG-agent's behavior, but replace the "secret key" with a derivation path. Trezor-GPG would deterministically create "secret key" files named after the matching keygrip, and containing the derivation path. This method gives the following advantages:
What do you think? |
Any further thought given to this? |
7f88219
to
03ccef3
Compare
03ccef3
to
b41b483
Compare
Since I didn't get any response in a while, so I decided to go ahead and implement key storage in |
This is a big one. Let me try to cover this:
trezor-gpg init
totrezor-gpg init
for creating the homedir andtrezor-gpg add
for creating keys. Allows adding multiple keys, instead of having multiple identities for the same key.--subkey-path
, either for the auto-generated encryption subkey, or for keys made with--subkey
. The derivation path is stored in signature subpacket with id 100. That's a "private use" id, meaning it's free for use under the assumption that it might clash with other programs (As defined by RFC2434). This is fine, since the derivation path is verified by deriving and comparing public keys, so a false positive is impossible. It is theoretically possible to enable this for primary keys as well, as well as store the derivation path regardless of whether a user id is available (So the key can survive having its first user id deleted). I didn't do this, but it would be a simple modification to make.add
, it's now possible to create custom subkeys to hardware-generated primary keys. That means users can set up their own key layout.gpg --delete-secret-and-public-keys <key>
.This is a pretty big change, but it makes GPG far more usable and versatile. I also considered adding a
certify
command to turn an OpenSSH-format public key into a subkey file that can be imported to another home directory. However, I realized that doing that would require adding serialization and keygrip generation for any format of public key that could be delivered, since third-party subkeys are not limited to device-supported curves. I decided against messing with encryption stuff for this PR. But if you can provide some advice, or point at appropriate documentation, I can try tackling it for the next PR.Fixes #341. Makes #358 redundant.
Note: I've tested various combinations of hardware primary keys and subkey, but did not test non-hardware primary key with hardware subkey, which should work, but might not work when signing/encrypting, due to not being able to run commands on the primary key that's in another folder. This requires testing.