search

decentralized-identity / ion-sdk

TypeScript SDK for ION
Apache License 2.0
30 stars 14 forks source link

Add support for ed25519 (2/2) #23

Closed gjgd closed 2 years ago

gjgd commented 2 years ago

Follow up to https://github.com/decentralized-identity/ion-sdk/pull/20

Adding support for Ed25519 in IonSDK

gjgd commented 2 years ago

@OR13 I addressed your comments. I am not so sure how to deal with the crv vs alg problem because Ed25519 is the name of the digital signature algorithm, so I think it does belong next to ES256k.

OR13 commented 2 years ago

@gjgd https://www.iana.org/assignments/jose/jose.xhtml

@thehenrytsai @csuwildcat thoughts: 

EdDSA | EdDSA signature algorithms | alg
-- | -- | --
Ed25519 | Ed25519 signature algorithm key pairs | Optional
-- | -- | --
ES256K | ECDSA using secp256k1 curve and SHA-256
-- | --
secp256k1 | SECG secp256k1 curve | Optional
-- | -- | --
gjgd commented 2 years ago

Screenshot 2022-01-24 at 13 55 34

This is unfortunate naming. Pretty sure Ed25519 is the name of the digital signature algorithm and it should have been Curve25519 as the name of the associated curve.

ES256k is the implementation of the ECDSA signature algorithm that uses the secp256k1 curve Ed25519 is the implementation of the EdDSA signature algorithm that uses the Curve25519 curve

Edit: from https://en.wikipedia.org/wiki/EdDSA

Ed25519 is the EdDSA signature scheme using SHA-512 (SHA-2) and Curve25519

OR13 commented 2 years ago

@gjgd you are correct, but ION only uses JWS algs... not raw signatures... hence the correct term in ION for a signature is a JWS alg... if ION supported raw ECDSA or EdDSA without a JOSE header this would be a different story.

thehenrytsai commented 2 years ago

Thanks @gjgd and @OR13. I am now aware of the PR, will try take a look tomorrow.

thehenrytsai commented 2 years ago

Forgive me if the question is silly, but shouldn't the reference implementation be updated first to support ED25519 before SDK claims to have the support? Else anyone using the SDK might be stuck with their DID document?

gjgd commented 2 years ago

Planning to use the ion sdk in a project that's not the reference implementation. I assume the reference implementation would throw an error for keys that are not supported?

csuwildcat commented 2 years ago

Planning to use the ion sdk in a project that's not the reference implementation. I assume the reference implementation would throw an error for keys that are not supported?

If you mean the actual ION node code, yes, that will throw if it has not been updated and a release is not propagated across the community in a network upgrade.

gjgd commented 2 years ago

I can add a console.warning indicating that an ion node may reject an operation with ed25519 keys until the reference implementation supports it, would that work?

csuwildcat commented 2 years ago

I can add a console.warning indicating that an ion node may reject an operation with ed25519 keys until the reference implementation supports it, would that work?

There's certainly nothing wrong with Sidetree in general supporting Ed25519, and I'd even like to see ION support it, but my concern is that if we allow people to create long-form IDs in a way that isn't actually valid at the network/protocol level, it will cause strange issues. The best thing would be to see the node code updated with matching support so it actually does work, then push them out together. What's the plan on the node implementation side?

gjgd commented 2 years ago

I understand the concerns and the potential confusion this could create, but are you suggesting that we cannot merge this PR (that is already approved) until Sidetree nodes support Ed25519?

If not, please suggest a set of changes that would alleviate this concerns. My suggestion was to log a warning so that developers would know not to use ion-sdk's Ed25519 methods until the nodes support it.

If yes, why weren't this concerns brought up 2+ months ago when I started to work on this?

thehenrytsai commented 2 years ago

@gjgd supporting ED25519 would be a great feature add to ION, so thank you for working on this, while I don't understand (still don't) why you are doing all this work for free, but wasn't going to complain! So we'd love to have the code merged in as soon as possible, but in principle not before actual ION node support. Having an SDK that allows people to create operations that are going to be rejected by ION just feels wrong. I don't understand why wouldn't we have support for ED25519 in ION itself before adding support for it in ION-SDK, maybe you can elaborate why you think it should be done in reverse?

If yes, why weren't this concerns brought up 2+ months ago https://github.com/decentralized-identity/ion-sdk/pull/20?

My apologies for not making expectations clear in my comment in the previous PR: "The PR looks great overall, given that it really isn't hooked up with anything really, so I see this more as a preparation to more PRs to come".

Basically the PR was approved on the basis that the code was not hooked up to any public facing paths (in fact I thought what you did was clever due to that fact), and by "more PRs" I was imagining both in Sidetree repo as well as ION-SDK repo, and IMO order is important here.

csuwildcat commented 2 years ago

This thread is confusing to me - I am fully supportive of Ed25519 being added as an operation signing option, but was there a plan to have a PR to make it so it was actually useful to folks? I ask because without a matching PR for the node side of things, it seems like we're handing people something confusing that wouldn't actually work as intended. Can't you provide some clarity on your thinking @gjgd?

OR13 commented 2 years ago

We'll keep an eye on this repo and wait till you implement support for Ed25519, until then we will use the fork where the feature is available.

We use this library / the fork for 2 other did methods that are not ION, one on Ethereum, and one on Amazon QLDB.

We also don't use the ION codebase for either of those since it bundles bitcoin dependencies.

Its might have been more accurate to call this package a sidetree-did-method-sdk since that is how we use it... but I can see the logic in not accepting a PR here when the ION side is not supported.

I can't speak for @gjgd , but this is an open source repo and I think it's fine to close a PR when it's not making progress... or when you feel like the suggested changes are more than you have time to volunteer to do for free.

gjgd commented 2 years ago

Was planning to use this PR to generate fixtures, that will be used to test a node implementation. I do think this change can stand on its own, but apparently we disagree on that point.

I hear the concern that the change could be confusing until Ed25519 is implemented on the node side. My suggestion to address this is to:

If this is acceptable, happy to reopen this PR. But to be clear, I'm no longer willing to commit to more work Ed25519 support on the DIF sidetree repo, for the reasons I mentioned on Slack.

csuwildcat commented 2 years ago

@thehenrytsai @gjgd what if we accepted the PR with the mod to warn, defaulted to secp, and basically buried the alternate key path until we have ION node support without any docs that say it exists? Would this be an acceptable compromise?