Skip to content
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

Updated RSA DID multiformat #550

Closed
wants to merge 5 commits into from
Closed

Updated RSA DID multiformat #550

wants to merge 5 commits into from

Conversation

expede
Copy link
Member

@expede expede commented Sep 10, 2021

rsa-x509-pub has made it into Multicodec. This PR updates the

  • Update RSA format
  • Test backwards compatibility
  • Exchange test cases with Qri team Test against W3C vectors

Comment on lines 32 to 31
ensureM $ sendRequest IPFS.getPeers
ensureM $ sendRequest getIPFSPeers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting! Why did you decide to switch the naming convention? I personally like the Something.x convention, so I'm curious :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also prefer that convention. This is from the new Servant.Generic client, since it's easier to decompose them in one file rather than importing the outer level into separate files and exposing them there. The tradeoff is that it puts them in a flat namespace.

Comment on lines +122 to +111
RSAPublicKey rsa ->
0x12 : 0x05 : BS.unpack (Lazy.toStrict . Builder.toLazyByteString . getUtf8Builder $ display rsa)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just randomly took a stroll through here, and now I know this is actually incorrect, the prefix needs to be 0x85 : 0x24 : Bs.unpack ...
I've implemented it that way in ts-ucan and it tests fine with the test vectors from the DID spec.
https://github.com/ucan-wg/ts-ucan/pull/42/files#diff-109e954c94a80ae8fd78b2ce9734642de7a787f48926c9a5ba18ad1ec76225ffR17

The reason it's 0x85 0x24 is that that's the number 0x1205 encoded as unsigned varint.
Just like a couple of lines above, the multicodec for ed25519-pub 0xed is encoded as unsigned varint into 0xed 0x01.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's a bug. ALSO the entire format has since been updated in the spec (I assume that ASN.1/DER PR is getting merged), so we need to rewrite a bunch of this PR

|> X509.pubKeyToPEM
|> PEM.pemWriteBS
[X509.PubKeyRSA pk]
|> X509.writePubKeyFileToMemory
Copy link
Member Author

@expede expede Sep 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These end up larger than before.

writePubKeyFileToMemory = pemsWriteBS . map pubKeyToPEM

Soooo, that's in PEM with headers and stuff. This is definitely wrong.

Probably want

PEM.pemWriteLBS

@expede expede added this to the 🤝 Standards & Interop milestone Nov 2, 2021
@expede expede self-assigned this Nov 2, 2021
@expede
Copy link
Member Author

expede commented Nov 2, 2021

Rebased. Tests failing, and need to validate the format against what Qri has (and then contribute a test vector in the Multicodec repo)

@expede
Copy link
Member Author

expede commented Nov 2, 2021

Waiting for the dust to settle over here: multiformats/multicodec#235

@expede expede removed their assignment Nov 2, 2021
@expede
Copy link
Member Author

expede commented Nov 25, 2021

The W3C folks merged the format update. MUAHAHHA this is back on the docket 🚀

(The compact representation was dropped — ASN.1/DER was merged here w3c-ccg/did-method-key#41 including test vectors)

matheus23 added a commit that referenced this pull request Jan 24, 2022
@matheus23
Copy link
Member

Closing in favor of #573

@matheus23 matheus23 closed this Jan 25, 2022
matheus23 added a commit that referenced this pull request Jan 27, 2022
matheus23 added a commit that referenced this pull request Feb 4, 2022
* Write DID signature verification & tests

* Write failing rsa DID test

* Move over DID tests

* Manually import core changes from ##550

* Niv update

* Remove redundant test

* Parse ASN1 DER RSAPublicKeys

* Adjust test fixture to use new format

* Fix fission-cli type errors

* Get everything compiling

* Fix one remaining type error

* Fix type error

* Revert "Niv update"

This reverts commit 3a11d68.

* Cherry-pick some changes from "Niv update"

* Remove AllowAmbiguousTypes

* Move Test.Prelude to Test.Web.UCAN.Prelude

* Re-add AllowAmbigousTypes for DelegationSemantics

I'm pretty sure I need this? https://github.com/fission-suite/fission/runs/5005311975

* Revert encoding DIDs in new format

The rest of the system needs to understand the new format first!
Still, we can now *consume* the new format in the server/CLI.

* Remove unused orphan
@expede expede deleted the ucan-rsa-multiformat branch October 30, 2022 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants