Hiya

Re: fission-codes/fission-cli#14

Could libsecp256k1 be bumped to ^0.7.1 so the base64 dependency doesn't get duplicated ? thx

warning[B004]: found 2 duplicate entries for crate 'base64'
   ┌─ /home/foobar/pp/fission-cli/Cargo.lock:9:1
   │  
 9 │ ╭ base64 0.12.3 registry+https://github.com/rust-lang/crates.io-index
10 │ │ base64 0.13.0 registry+https://github.com/rust-lang/crates.io-index
   │ ╰───────────────────────────────────────────────────────────────────^ lock entries
   │  
   = base64 v0.12.3
     └── libsecp256k1 v0.5.0
         └── did-key v0.1.1
             └── fission v0.1.0
   = base64 v0.13.0
     ├── did-key v0.1.1
     │   └── fission v0.1.0
     └── fission v0.1.0 (*)

It should be possible to zeroize memory, especially for the private key parts. If this sounds like a welcome feature, happy to work on a PR at some point!

Re: fission-codes/fission-cli#21 (comment)

I investigated and ockam seems to have dropped support for bls and it looks like they don't have a repository anymore for it either.

In any case this causes dependency duplication with bls12_381_plus and the latest signature_bls only brings ^0.5 whilst did-key brings ^0.7

I also queried with ockam at: build-trust/ockam#3864 to clarify it.

Might need new bls signature crate to use or fork teh ockam one and bring the bls12_381_plus up.

Hello,

we would like to point out a potential issue that you might want to prevent. The did-key crate depends on did_url which in turn depends on form_urlencoded. The latter crate is considering changes in their default-features which would break users of did_url. We assume that the did_url crate won't be changed to fix this issue, in particular that this PR won't be merged, so we're pointing it out to you instead. A fix on your side would be to add form_urlencoded to your dependencies and pin it, i.e.:

form_urlencoded = { version = "=1.1.0" }

We recently released a similar fix for reference.

Best regards.

I have been reading the code and saw this: https://docs.rs/did-key/latest/src/did_key/ed25519.rs.html#40
The bitwise operations appear to be from here:

Is there a function perhaps in curve25519-dalek that performs this same stuff?

And for the SHA-512 hash, I could not find reference to that in the did-key or OSCORE spec. Where does that come from?

The README says:

This crate is intended to provide basic support for did:key methods. It has no external dependencies and can be compiled for any target.

But there clearly are external dependencies in the Cargo.toml.

Did you mean that the crate and its dependencies are pure rust and compiled from source? If so that verbiage could be corrected.

As stated. the mentioned function panics because it wants 48 bytes while the generated secret j\key bytes are 32!

Based on w3c-ccg/did-method-key#29

As described here https://github.com/decentralized-identity/did-spec-extensions/blob/master/parameters/signed-ietf-json-patch.md

Objective

The key DID method is designed as deterministic key resolution that resolves into a fixed DID document. The identifier of the DID method is fingerprint of the public part of the key pair; as such it doesn't now provide registration for additional fields in the DID document.
RFC 6902 defines a mechanic that can be applied to a JSON object to patch the document which can be used to extend the DID document.
The Signed IETF JSON Patch DID Extension describes how this can be utilized in a DID Document by allowing the DID URI to extend it's document with signed data.

Implementation

This implementation should ensure that the API can be extended with the JSON Patch operations by taking an input of (operation, path, json) and producing a result that contains the DID URI of the key in question with the signedIetfJsonPatch URL parameter and it's value.
The reverse operation for resolving this URL would resolve the DID document with the patched JSON. The library should perform all needed validations to ensure that the controller is the original signer of the patched data.

Test Vectors

TODO

Breaking Changes

None expected, but there may be changes in API and DID Document structs that will impact current implementations.

Related Issues

transmute-industries/did-key.js#72

w3c-ccg/did-method-key#17

Currently used version of p256 crate is "0.5.2", which, in turn, depends on signature crate of version >=1.2.2, <1.3.0.
All updated crates around RustCrypto suite use version 1.3+ and it creates conflicts.

failed to select a version for signature.
... required by package ecdsa v0.8.0
... which is depended on by p256 v0.5.2
... which is depended on by did-key v0.0.12

use did_key::*;

let key = generate::<Ed25519KeyPair>(None);
let did_doc = key.get_did_document(Config::default());

This example is not working since Config has private visibility (only CONFIG_JOSE_PRIVATE, CONFIG_JOSE_PUBLIC, CONFIG_LD_PRIVATE, CONFIG_LD_PUBLIC work).

fn main() {
    did_key::resolve("did:key:zQ3shokFTS3brHcDQrn82RUDfCZESWL1ZdCEJwekUDPQiYBme").unwrap();
}

Panics with not implemented: unsupported key type, but this is a test vector from the did:key method spec: https://w3c-ccg.github.io/did-method-key/#secp256k1
The other two test vectors in the spec also cause the same panic.

The fix is

diff --git a/src/lib.rs b/src/lib.rs
index d641679..5bc3414 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -177,7 +177,7 @@ impl TryFrom<&str> for KeyPair {
             [0xec, 0x1] => KeyPair::X25519(X25519KeyPair::from_public_key(&pub_key[2..])),
             [0xee, 0x1] => KeyPair::Bls12381G1G2(Bls12381KeyPairs::from_public_key(&pub_key[2..])),
             [0x80, 0x24] => KeyPair::P256(P256KeyPair::from_public_key(&pub_key[2..])),
-            [0xe7, 0x0] => KeyPair::Secp256k1(Secp256k1KeyPair::from_public_key(&pub_key[2..])),
+            [0xe7, 0x1] => KeyPair::Secp256k1(Secp256k1KeyPair::from_public_key(&pub_key[2..])),
             _ => unimplemented!("unsupported key type"),
         });
     }

There was no test exercising the code path for Secp256k1, so the bug was not exposed.

More test cases here: https://github.com/LedgerDomain/did-key.rs/blob/main/src/lib.rs

In particular, the BLS key type test vectors don't resolve correctly. There's a difference between the did-key crate generated BLS keys, which start with did:key:z5T (or maybe did:key:z5Tc), whereas the ones in the spec https://w3c-ccg.github.io/did-method-key/#bls-12381 which start with did:key:zQ3s.

This trivial program doesn't compile:

use did_key::Fingerprint;

fn main() {
    match did_key::resolve("did:key:z9Bg7Lbpq3RzgN8RkgEuZqLDF4nT1ZzobJe9ZPbS6HJpQ") {
        Ok(k) => println!("{:?}", k.fingerprint()),
        Err(e) => println!("{:?}", e),
    }
}

The output is:

❯ cargo run
   Compiling form_urlencoded v1.2.1
error: the `alloc` feature must currently be enabled
  --> /home/harm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/form_urlencoded-1.2.1/src/lib.rs:24:1
   |
24 | compile_error!("the `alloc` feature must currently be enabled");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `Cow<'_, _>: From<PercentDecode<'_>>` is not satisfied
  --> /home/harm/.cargo/registry/src/index.crates.io-6f17d22bba15001f/form_urlencoded-1.2.1/src/lib.rs:72:55
   |
72 |     decode_utf8_lossy(match percent_decode(&replaced).into() {
   |                                                       ^^^^ the trait `From<PercentDecode<'_>>` is not implemented for `Cow<'_, _>`
   |
   = help: the following other types implement trait `From<T>`:
             <Cow<'a, CStr> as From<CString>>
             <Cow<'a, CStr> as From<&'a CStr>>
             <Cow<'a, CStr> as From<&'a CString>>
             <Cow<'a, str> as From<&'a str>>
             <Cow<'a, str> as From<String>>
             <Cow<'a, str> as From<&'a String>>
             <Cow<'a, [T]> as From<&'a [T]>>
             <Cow<'a, [T]> as From<Vec<T>>>
             <Cow<'a, [T]> as From<&'a Vec<T>>>
   = note: required for `PercentDecode<'_>` to implement `Into<Cow<'_, _>>`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `form_urlencoded` (lib) due to 2 previous errors

Adding the form_urlencoded crate explicitly fixes the error:

 ❯ cargo add form_urlencoded
    Updating crates.io index
      Adding form_urlencoded v1.2.1 to dependencies.
             Features:
             + alloc
             + std

I doubt this is expected behaviour. Notably I was able to run the test of this repo without this issue and no mention of alloc was made anywhere.

Crate version 0.2.1.

Due to the breaking change mess with funty (see ferrilab/funty#3), I'm finding it a bit intractable to upgrade my dependency on did-key.rs to 0.1. Would you be open to having a v0.0 branch with a 0.0.16 release? This would include the fixes to secp256k1 codec in DID resolution, correctly returning error instead of panicking, but I believe couldn't include the recent BLS key refactor.

Hey there. I was planning to use this lib for my project but am unable to if signature verification panics on invalid signature data. Since signature content can come from untrusted sources, I need to be able to gracefully handle invalid signature data.

https://docs.rs/did-key/0.1.0/src/did_key/ed25519.rs.html#180-186

The verify method for various keypairs returns a Result type, but the signature data uses .expect() instead of mapping the error so I can't handle that case.

Thanks!