-
Notifications
You must be signed in to change notification settings - Fork 61
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
Implement parsing GPG keys #373
Conversation
76c8592
to
479cde7
Compare
Will also note I should probably use some other dummy test data for the GPG keyring instead of my own from github. EDIT: Done |
7e3289d
to
62e45a4
Compare
Hi @fairingrey, I am wondering how signatures will be made. I am seeing somewhat of a mismatch between what is implemented here and the referenced PgpVerificationKey2021 draft specification. This PR is parsing the public key into a verification method object containing a JWK - similar to what In the webkey:ssh implementation, converting a SSH public key to a verification method object with JWK works because SSH Agent can be used to request a signature over an arbitrary payload. Signing over an arbitrary payload allows using existing signature formats. Using existing signature formats is desirable for interoperability. PgpVerificationKey2021 represents a new signature format, but it has a specification (albeit not yet in the CCG or a working group apparently). I assumed that GPG Agent would not sign arbitrary payloads and that therefore this new suite would be necessary. Looking into GPG Agent now, I see it can sign a hash from particular algorithms, but not an arbitrary payload: Many of the existing signature algorithms and proof types are based on signing a SHA-256 hash, fortunately; but some are not, in particular the Ed25519 ones. I see some general possible ways to proceed:
With SSH, translating the public keys is fairly straightforward (except for certificates, DSS, and the comment field, all of which are dropped in the current implementation), but GPG public keys and signatures are significantly more complex. Option 1 I think would be simpler, as there would be a more well-defined interface between the signature suite and the OpenPGP implementation, i.e. the ASCII-armored public key and signature values. If using Option 2, it might be good to implement more of OpenPGP here, i.e. handling subkeys, expirations/revocations and key use/type flags. (e.g. currently the primary key is returned only, but the primary key might be expired, or it might be flagged as for certification purpose only rather than for signing; a user might want to sign with a subkey, etc.) To fully translate the PGP public key data into verification methods, I wonder if it would make sense to verify signature packets and apply their updates. |
@clehner Thanks for your review! Yes I had a bit of trouble understanding the spec in https://or13.github.io/lds-pgp2021/ so I thought I'd start by at least trying to understand what the ssh implementation does and just copy that for the time being. I was worried there would be a few discrepancies and I'm glad they were pointed out. I guess for right now working on Option 1 would be ideal, but I do feel as if I need some more guidance on how exactly it would look; so disregard parsing it into a JWK, and pass the ASCII-armored block as |
Option 1 sounds great to support, let's aim for Option 3 too! Eventually SSI should have clean separation across VMs as well. |
Yes, in the verification method object with type PgpVerificationKey2021. The id fragment should probably also be changed, e.g. to the PGP key id. (So the key data would still need to be parsed to get the primary key id.) The existing verification method objects using publicKeyJwk (for Option 2/3) could have their type changed to a type that works with publicKeyJwk, i.e. either JsonWebKey2020 or the various types used in |
@clehner I've pushed a few new commits with the changes requested (after a lot of digging through the openpgp library I was using, figured I wanted the high-level Would appreciate another look, thanks again! |
1753aa2
to
7cfb6e7
Compare
@fairingrey the build seems to be breaking, could you investigate how to fix this? We may need to add more dependencies to build your parts. |
I believe the build machine requires nettle as a dependency because of sequoia, I'll try looking into how to pull that in EDIT: May also want to edit the relevant Cargo.toml to allow certain features https://gitlab.com/sequoia-pgp/sequoia#note |
@fairingrey yes, looks great! Looks like Fingerprint is right rather than Key ID. The The nettle-dev package, if that is what is needed, could be installed in the CI script and mentioned in the Dependencies section of the readme. But maybe it would be better to use the Sequoia's The Document should have some additional context set to define the "publicKeyPgp" and "PgpVerificationKey2021" terms, e.g. using https://w3id.org/pgp/v1. But this could be done separately. Dependencies added
Build failure with WASM
|
required for sequoia pgp dependency
allows users to pick between which crypto lib (cng or nettle) uses flate compression by default
I'll squash my commits once I'm sure the CI finishes the job. |
Huh. So nettle won't build for WASM, which I guess should be specified under a feature flag. It might be worth using the Rust PGP implementation to support this (https://github.com/rpgp/rpgp) |
3c33b72
to
72f33df
Compare
It might be related to rpgp/rpgp#131 |
e42af6f
to
dd7f535
Compare
Alright I think this is ready for one more review @clehner. Some things of note:
|
Also just as an aside to the rest of the code in this crate, I find the reliance on |
add note regarding bug pgp::composed::signed_key::parse::from_armor_many seems to yield only one key when multiple are present use feature resolver v2 (necessary since nettle will not compile on wasm32) https://doc.rust-lang.org/cargo/reference/features.html#feature-resolver-version-2 add notes on pgp specific dependencies
4fcaa9d
to
d0d964a
Compare
Two options:
I like the multiple verification methods approach, as it makes it more obvious what keys the DID document contains by inspection. But if it's not practical, I'm not sure it adds much other benefits. It would be useful to know if OpenPGP implementations (i.e. including rPGP) commonly support verifying signatures against these multiple-public-key files (I assume they do. But we might not find out until later). Here's a new issue on the PGP Vocabulary v1 asking about use of multiple public keys in a
OK. Maybe it could work to allow the old zeroize dependency, like how we have
OK, thanks.
Using Errors have to be returned as strings in the following two APIs:
|
also include test output for both pgp libs
I also prefer the multiple verification methods approach as I prefer more explicitness over implicitness and the format is generally more readable to me. Both sequoia and rpgp support verifying messages/signatures against pgp pubkeys, but I find sequoia much easier to work with.
Unfortunately not, doing so would require downgrading the I guess for now I can rework the error types. I'll leave in the |
include anyhow; upgrade rest of deps
With these changes, didkit-wasm fails to build, because the ring and ed25519-dalek features are enabled in ssi, which is a conflict in the jwk module. I'm guessing this is because of Cargo resolver v2, but haven't figured out if it needs to be addressed in didkit or in ssi (or both). |
Looking at this again... --- a/did-webkey/Cargo.toml
+++ b/did-webkey/Cargo.toml
@@ -12,17 +12,13 @@ homepage = "https://github.com/spruceid/ssi/tree/main/did-webkey/"
documentation = "https://docs.rs/did-webkey/"
[features]
-default = ["sequoia-openpgp/crypto-nettle"]
+default = ["ssi/ring", "sequoia-openpgp/crypto-nettle"]
crypto-cng = ["sequoia-openpgp/crypto-cng"]
crypto-nettle = ["sequoia-openpgp/crypto-nettle"]
p256 = ["ssi/p256"]
[dependencies]
-ssi = { version = "0.3", path = "../", features = [
- "rand",
- "ring",
- "p256",
-], default-features = false }
+ssi = { version = "0.3", path = "../", default-features = false, features = ["secp256r1"] }
anyhow = "1.0.52"
async-trait = "0.1.52"
reqwest = { version = "0.11.9", features = ["json"] }
Makes sense... But I'm confused by the wasm32 targeting. I'm not sure what are the advantages of using rpgp. sequoia-openpgp supports WASM with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Attempting to address didkit-wasm in #390
Alright, I can follow up with this diff.
These are really good points. I hadn't caught on that sequoia-openpgp compiles to WASM so I opted to use rpgp for it instead. The usage of crypto libraries was due in part to a batteries-enabled approach to this issue as I assumed in the future didkit could also be responsible for verifying If we're going to stick with |
You may have to check back around with me on how to let sequoia-openpgp compile on wasm32-unknown-unknown, I'm having a bit of trouble figuring out how to specify to the manifest to avoid enabling both crypto backends when I don't want them. |
I think using just the If this can work without changes in didkit, cool; otherwise, a change like this can be made: spruceid/didkit@b293fe0 If you want to test building didkit-wasm:
|
I think I am reproducing this; it seems that the architecture-conditional dependency features ( |
Merged with additional relevant changes in #390 |
Addresses #204