input-output-hk / prism-did-method-spec

Apache License 2.0
15 stars 1 forks source link

support for Ed25519 keys #19

Closed EzequielPostan closed 1 year ago

EzequielPostan commented 1 year ago

In the current implementation we only mention secp256K1 curve and corresponding keys We use those both for operation's signing, but also as the types of keys supported in prism DID Documents.

We would like to add support for Ed25519 curve and keys on our DID Documents. We were thinking if we should describe the supported keys separately "as a registry" that talks about the possible key types that the DID method supports. Each key type would need to define the curve string that it uses, and the way to encode public key coordinates in at least one of the supported protobuf models to represent keys, namely:

message ECKeyData {
    string curve = 1; // The curve name, like secp256k1.
    bytes x = 2; // The x coordinate, represented as bytes.
    bytes y = 3; // The y coordinate, represented as bytes.
}

/**
 * Holds the compressed representation of data needed to recover Elliptic Curve (EC)'s public key.
message CompressedECKeyData {
    string curve = 1; // The curve name, like secp256k1.
    bytes data = 2; // compressed Elliptic Curve (EC) public key data.
}

would this be a good way to document this in the spec? Or we should have the full description in a single document. Having the registry would allow us to extend the spec in a modular way too.

peacekeeper commented 1 year ago

Hmm I think creating your own registry sounds complex to set up and maintain. Is there a reason why you would want to restrict the supported keys, as opposed to just allowing anything and being as extensible as possible?

It might be better to re-use something that exists already, such as JWK, or Multicodec.

I'm not experienced with protobuf, but couldn't it just look like this:

message KeyData {
    string kty = 1;
    string crv = 2;
    string kid = 3;
    bytes x = 4;
    bytes y = 5;
    // ... standard JWK fields here ...
}

Or:

message KeyData {
    bytes multicodec = 1;
}
EzequielPostan commented 1 year ago

Is there a reason why you would want to restrict the supported keys, as opposed to just allowing anything and being as extensible as possible?

I am unfortunately also not well versed with already existing standards like JWK or Multicodec either. For the JWK case, we would need to track what the "... standard JWK fields here ..." are and possibly we would need to run validations on the node side to check they are well constructed. If multicodec can always be encoded as a simple array of bytes, it looks simple to add as a MulticodecKeyData in the spec, and add it as a oneOf field we have for PublicKey

message MulticodecKeyData {
    bytes multicodec = 1;
}

message PublicKey {
    reserved 3, 4;
    string id = 1; // The key identifier within the DID Document.
    KeyUsage usage = 2; // The key's purpose.
    LedgerData added_on = 5; // The ledger details related to the event that added the key to the DID Document.
    LedgerData revoked_on = 6; // The ledger details related to the event that revoked the key to the DID Document.

    // The key's representation.
    oneof key_data {
        ECKeyData ec_key_data = 8; // The Elliptic Curve (EC) key.
        CompressedECKeyData compressed_ec_key_data =  9; // Compressed Elliptic Curve (EC) key.
        MulticodecKeyData multicodec = 10; // Encoded public key according to MultiCodec rules 
    };
}

but this likely also would be added only to the spec for now. The implementation would be behind. I would need to think about the general impact of having more keys/encoding (in particular in the node's db)

As we are approaching a release, I would like to avoid making a big change. Given that we followed a custom way to encode secp keys, I wouldn't incline to make an entire new encoding at last minute.

Maybe a question to consider is, what process is there in W3C to update a DID method's spec? Because, secp and Ed255 would likely be enough for a while to our intended use cases. We could update the DID method and add a more generic encoding in a future version if this is feasible

peacekeeper commented 1 year ago

I understand the preference to avoid a big change at this stage. We can also just start with the two types you mention, and then add additional ones later, that sounds fine too. I just think the design should be done in a way that doesn't require a big effort in the future when new key types come up (which is why I suggested JWK or Multicodec).

what process is there in W3C to update a DID method's spec

There is no process for that, you can update and version your DID method spec however you like.

EzequielPostan commented 1 year ago

There is no process for that, you can update and version your DID method spec however you like.

got it, the concern was that in order to update the method, we had to re-submit to W3C. If, after acceptance, we can keep the evolution in a relatively simple way, then adding the generic encoding in the next version sounds good. The drawback I see when linking us to an existing standard, is that a new type of key may be desired while no standard supports it