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

Upgrade DID RSA Key format #573

Merged
merged 19 commits into from
Feb 4, 2022
Merged

Upgrade DID RSA Key format #573

merged 19 commits into from
Feb 4, 2022

Conversation

matheus23
Copy link
Member

@matheus23 matheus23 commented Jan 25, 2022

Ported over most changes from #550 but adapted to the format with the prefix bytes 0x85 0x24 which is the unsigned varint encoding of 0x1204.
Also, instead of storing the ASN1 DER SubjectPublicKeyInfo, it stores ASN1 DER encoded RSAPublicKeys (which are part of SubjectPublicKeyInfos with RSA public keys) as per the DID spec (w3c-ccg/did-method-key#41).

Depends on #571. Currently that's the "branch this will merge into" for better diffs on github, but I'll change this to the branch that #571 will merge into once that's merged.

@matheus23 matheus23 mentioned this pull request Jan 25, 2022
3 tasks
Base automatically changed from matheus23/hs-ucan to ucan-upgrade January 26, 2022 15:29
Base automatically changed from ucan-upgrade to main January 27, 2022 07:56
@matheus23
Copy link
Member Author

Branch shenanigans resolved!
This PR leaves the whole monorepo in compiling state, so we can merge it into main. The ucan-upgrade branch will be initiated later.

@@ -7,5 +7,7 @@ import qualified System.IO.Unsafe as Unsafe

import Fission.Prelude


-- Wait. This is weird. How did this end up in this module?
instance Arbitrary Ed25519.SecretKey where
Copy link
Member

Choose a reason for hiding this comment

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

Yup that is weird! It's probably me being confused, but the compiler warning is turned off because {-# OPTIONS_GHC -fno-warn-orphans #-}. All the more reason for us to just bite the bullet and newtype everything. The more that I think about it, the more I'm in favour of just doing that.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe to clarify: let's make this change in this PR. This line should not be in this module!

nix/sources.json Outdated Show resolved Hide resolved
shell.nix Show resolved Hide resolved
@matheus23
Copy link
Member Author

@expede Github Actions continues making my life hard by being somewhat flakey:

/home/runner/.stack/setup-exe-cache/x86_64-linux-nix/Cabal-simple_mPHDZzAJ_3.2.1.0_ghc-8.10.7: startProcess: exec: invalid argument (Bad file descriptor)

This only happened in the "🏺 Artifacts (Nix) / 🖥️ ubuntu-latest ❄️ Nix (pull_request)" action. All other actions are running through (including tests now!).

So I think this is ready for another round of review 👍 (no rush but just to make sure you don't think this blocked on me getting the tests working or something)

Copy link
Member

@expede expede left a comment

Choose a reason for hiding this comment

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

Just move that orphan, and then 🚀 🚀 🚀 🚀 🚀 🚀 🚀



@matheus23
Copy link
Member Author

matheus23 commented Feb 4, 2022

😂 Thanks for the enthusiasm.

I need to change/revert one thing, which is the RSA DID encoding should be rolled back to the old format for now, because - as you've experienced - it breaks e.g. the AWAKE protocol, as long as the auth lobby doesn't understand the new DID format.

I'll merge it later today! (Let's not have another PR hanging like the last one 😱 )

The rest of the system needs to understand the new format first!
Still, we can now *consume* the new format in the server/CLI.
@matheus23 matheus23 merged commit c7f5907 into main Feb 4, 2022
@matheus23 matheus23 deleted the matheus23/upgrade-did-key-rsa branch February 4, 2022 16:59
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