ceramicnetwork / js-did

A simple interface to interact with DIDs that conform to the DID-provider interface.
Other
95 stars 28 forks source link

Session DID Changes In Reauthentication Preventing Stream Updates #131

Closed alex-mccreary closed 1 year ago

alex-mccreary commented 1 year ago

Description

If I authenticate with DIDSession.authorize using an EthereumAuthProvider, I am provided with a session.did.id and session.did.parent. However, when I re-authenticate later on, a different session.did.id is provided; the session.did.parent remains the same. This prevents me from updating any streams created by the previous session.did.id

Technical Information

Versions -"@ceramicnetwork/blockchain-utils-linking": "^2.3.0", -"@ceramicnetwork/http-client": "^2.3.0", -"did-session": "^0.1.1",

  1. Create a DIDSession using an EthereumAuthProvider
  2. Create a stream and set the controller as controllers: [this.ceramic.did.id]
  3. Re-authenticate the DIDSession
  4. Update the previously created stream and encounter an error: {"error":"Can not verify signature for commit bagcqcera4i776kf624dvn5h2avj37jjgepgbqd2yw3fdilpkttllwxfj42gq: invalid_jws: not a valid verificationMethod for issuer: did:key:z6MkmSwWYbLtgRVoAoVpaeHafXHWuGvDAt1nzMvRtxjMAfiT#z6MkmSwWYbLtgRVoAoVpaeHafXHWuGvDAt1nzMvRtxjMAfiT"

I also tried setting the parent as the controller like: controllers: [this.ceramic.did.parent], but when creating a NEW stream, this resulted in: {"error":"Can not verify signature for commit bagcqcera7og333rcoajf4uvujlskpcxfjpqtnmknmbiu7ersrf42gvw32ftq: Signature does not belong to issuer"}

It seems others on Discord were encountering this same issue: image

zachferland commented 1 year ago

Not sure about the last error yet, that should work from description, but this the expected behavior for did session as Mohsin described in the commend above. The did you are signing with is a temporary session key that will change with every session, the controller should be your did:pkh (blockchain account), which of course is always the same.

could you try creating and updating a stream with did-session without changing the controller? ceramic/libs will handle the dids/controllers, no need to pass in that option, any reason you chose to pass that in?

also could you update did-session to v1 and did-session no longer uses blockchain-utils-linking, docs are up to date - https://did.js.org/docs/api/modules/did_session

alex-mccreary commented 1 year ago

Before updating the version, I tried passing an empty metadata object instead of the controller and it resulted in: Can not verify signature for commit bagcqceraxr2pmuonpk4ej7ldftcb726tqp3ea77zvrcbjfg753osxszz4lda: Signature does not belong to issuer

I was using EthereumAuthProvider from blockchain-utils-linking and I tried using this as the ethProvider from the documentation:

const ethProvider = new EthereumAuthProvider(eip1193Provider, accounts[0])
const addresses = await ethProvider.request({ method: 'eth_requestAccounts' })
const accountId = await getAccountId(ethProvider, addresses[0])
const authMethod = await EthereumWebAuth.getAuthMethod(ethprovider, accountId)

But the results in Property 'request' does not exist on type 'EthereumAuthProvider'

@zachferland What is the alternative to EthereumAuthProvider now that you asked me to remove blockchain-utils-linking?

zachferland commented 1 year ago

'@didtools/pkh-ethereum', which it looks like you also used (this only works with v1) but i would reference the docs linked above, they are currently up to date

getAccountId expects an eth web3 provider, like an injected web3provider from metamask for example

let me know if that helps

alex-mccreary commented 1 year ago

Thanks @zachferland . I was able to authenticate using v1.0.0 of did-session using:

    const addresses = await eip1193Provider.request({ method: 'eth_requestAccounts' })
    const accountId = await getAccountId(eip1193Provider, addresses[0])
    const authMethod = await EthereumNodeAuth.getAuthMethod(eip1193Provider, accountId, 'AppName')
    const session = await DIDSession.authorize(authMethod, { resources: ['ceramic://*'], 'domain': process.env.CERAMIC_ORIGIN })

However, without setting a controller, I'm still encountering the same error: "Can not verify signature for commit bagcqceragl3d4uct6esv3ldfitblnac7tgwgi4qp5morbf4n5cdgzalvcpvq: Signature does not belong to issuer"

When I re-add controller as controllers: [this.ceramic.did.id], I am able to create a stream, but when I re-authenticate, I still cannot update streams with this error: {"error":"Can not verify signature for commit bagcqcera3gdz7lfcofzcz3ixuy3zlvf5yecrltlodsobbfa7k55orta6y2eq: invalid_jws: not a valid verificationMethod for issuer: did:key:z6MkeisHE5xe6baQ4fVezMHZqP6RnuAwaGPC3XtF9VMk5S1z#z6MkeisHE5xe6baQ4fVezMHZqP6RnuAwaGPC3XtF9VMk5S1z"}

I also tried setting the controller as [this.ceramic.did.parent] ,but I'm still seeing the same error: {"error":"Can not verify signature for commit bagcqceraav5vzofqkk2z2oua37s3xxgf3uybd33ee2j2u3g5hxcys5tyjwnq: Signature does not belong to issuer"}

zachferland commented 1 year ago

Note you wont be able to update a stream you created before. If all dependencies are upgraded, you should be able to create a new stream and then update it.

Where are you using the session after? and what do you mean by re-authenticate?

any reason if in node env, you are using did-session instead of a did:key alone?

zachferland commented 1 year ago

are you using an ethereum account?

alex-mccreary commented 1 year ago

Yes, I am able to create new streams and update those new streams. I just need to be able to update streams that was created using a previous DID that share the same parent.

Here is how I'm authenticating:

          const eip1193Provider = new FireblocksWeb3Provider({
            privateKey: process.env.FIREBLOCKS_API_PRIVATE_KEY_PATH,
            apiKey: process.env.FIREBLOCKS_API_KEY,
            vaultAccountIds: process.env.CERAMIC_AUTHENTICATION_VAULT_ID,
            chainId: ChainId.GOERLI,
          });

          const addresses = await eip1193Provider.request({
            method: "eth_requestAccounts",
          });
          const accountId = await getAccountId(eip1193Provider, addresses[0]);
          const authMethod = await EthereumNodeAuth.getAuthMethod(
            eip1193Provider,
            accountId,
            "Company Name"
          );
          const session = await DIDSession.authorize(authMethod, {
            resources: ["ceramic://*"],
            domain: process.env.CERAMIC_ORIGIN,
          });

          this.ceramic.did = session.did;

This code is run in a constructor, so it is run when our app starts. If we restart our app, a new DID is generated and we can no longer update existing streams.

If there is an alternative to DID Session for node environments I should be using, I'll give it a try.

zachferland commented 1 year ago

couldnt tell exactly, are you able to create a new stream, then restart app, then update the same newly created stream?

yeah if its a key just for you app, and it can be long lived/secure like the private key env here, then you can just use a simple ed25519 key did https://did.js.org/docs/api/modules/key_did_provider_ed25519, no need for any sessions or things like that

sessions are more typically used for client side users in your app and with their wallets/keys

alex-mccreary commented 1 year ago

When the app is restarted, a new DID is provided and I can no longer update streams created using the previous DID.

Your example seems to be using a seed instead of wallet authentication. Is it possible to use wallet authentication with a persistent DID?

We were hoping to use wallet authentication for the extra security Fireblocks provides.

zachferland commented 1 year ago

In fireblocks is the process.env.FIREBLOCKS_API_PRIVATE_KEY_PATH the wallet seed/priv key? or is that just an api secret and they manage the wallet seed/priv key for you?

If it is the wallet seed/key than it is an equivalent security model to keeping the did seed and making writes to ceramic with that, would be no added benefit/security with Fireblocks.

Any reason your app writes to ceramic on the backend, vs users logging in and writing data to ceramic on the client side?

alex-mccreary commented 1 year ago

It's an API secret and Fireblocks manages the private key.

Our app is an API without a frontend.

mattdavis0351 commented 1 year ago

Tagging in @ukstv to see if he can shed some more perspective on this. He may have a trick or two up his sleeve when it comes to this.

zachferland commented 1 year ago

ok, and do you use one account for your app? or are creating many addresses for api consumers?

maybe if still building/not live, use a did key for now, we may have to try it with fireblocks, not sure if something is maybe a little bit different about the provider, so far not seeing anything obviously wrong from the example given

if you are passing a controller still, make sure its did.parent, but there isnt any need to pass that options

would also try logging accountId, stream controllers and the did.parent, may surface is something is changing or not set correctly

alex-mccreary commented 1 year ago

Yes, we just use one account.

I believe @mattdavis0351 was able to recreate the issue of the session did changing w/o fireblocks.

From my previous comment (https://github.com/ceramicnetwork/js-did/issues/131#issuecomment-1348907423), removing the controller or setting controllers: [this.ceramic.did.parent] results in an error when I try to create new streams. I am able to create and update streams (in current session) with controllers: [this.ceramic.did.id].

I wasn't sure how to log the controllers, but I logged the rest: Authentication 1 ceramic.did.id - did:key:z6M....4G ceramic.did.parent - did:pkh:eip155:5:0x29... accountId eip155:5:0x29...

Authentication 2 ceramic.did.id - did:key:z6M....iW ceramic.did.parent - did:pkh:eip155:5:0x29... accountId eip155:5:0x29...

The values for ceramic.did.parent and accountId remain the same, but ceramic.did.id changes, which is what I'm setting as the controller. I either need a way to set the parent as the controller or have ceramic.did.id persist.

zachferland commented 1 year ago

thanks!

Yeah it is expected the did changes for each session (with or without fireblocks), behavior logged here looks good so far, expect the parent to be same, the accountid to match parent and be same, and session did to change.

The controller can not be set to ceramic.did.id, that will not work with did-session. Without setting the controller (or setting ceramic.did.parent), is the error still "Signature does not belong to issuer" when creating a stream?

Should be able to log stream.controller, if you can share that

Sorry for the slow turn around, a lot of the team is out for the holidays, Ill be back fully next week and will try myself with fireblocks and your code here if not resolved by then

alex-mccreary commented 1 year ago

@zachferland Yes, I still receive "Signature does not belong to issuer" when creating new streams with controllers: [this.ceramic.did.parent] or when removing controllers entirely.

Unfortunately, since the error occurs on the await TileDocument.create(), I cannot log the response stream's controller.

Is there any more info I can provide to help resolve this? Thanks.

zachferland commented 1 year ago

Im back now, will try to recreate issue myself with fireblock today/tmrrw, nothing stood out as wrong from info provided here

zachferland commented 1 year ago

hmm i guess fireblocks api is not available publicly, makes it difficult to figure out

could you also try temporarily using an authprovider configured as follows and see if your code works, and help narrow it down if it could be the provider

const seed = new Uint8Array(32)
 const did = new DID({
    resolver: getResolver(),
    provider: new Ed25519Provider(seed),
  })
 const authMethod = await createEthereumAuthMethod()
alex-mccreary commented 1 year ago

@zachferland I couldn't find any reference to your createEthereumAuthMethod() function online, so I tried with EthereumNodeAuth.getAuthMethod instead, but I'm not sure where to get the accountId now:

const authMethod = await EthereumNodeAuth.getAuthMethod(
              Ed25519Provider,
              accountId,
              "Company Name"
            )

How can I get the accountId using your example? Or, do you have a working example you can share?

zachferland commented 1 year ago

ah sorry, yeah mixed up what i wanted to share

But if possible, if you could create it as follows in the tests here - https://github.com/ceramicnetwork/js-did/blob/main/packages/did-session/test/lib.test.ts#L72

I expect this would work, but would help narrow down

alex-mccreary commented 1 year ago

@zachferland Thanks. I was able to confirm that authentication using a mnemonic and controllers: [this.ceramic.did.parent] is working fine for creating/updating streams even after re-authentication.

What can I do next to help narrow this issue down?

In the meantime, I noticed the FireblocksWeb3Provider package has recently updated. I'll see if any of their updates may have fixed this problem.

alex-mccreary commented 1 year ago

I just confirmed I am still unable to create new streams with controllers: [this.ceramic.did.parent] in the new v1.0.0 of FireblocksWeb3Provider

oed commented 1 year ago

@alex-mccreary can you log this.ceramic.did.parent and see what the value is?

alex-mccreary commented 1 year ago

The parent looks like this: did:pkh:eip155:5:0x290...2fE6

And it persists after re-authentication

oed commented 1 year ago

@alex-mccreary can you show the full output please?

@zachferland looks like the DID maybe doesn't have a lower case ethereum address?

alex-mccreary commented 1 year ago

Parent DID did:pkh:eip155:5:0x290A878d45E13c9DA9D4B100c3fE2684AB4E2fE6

zachferland commented 1 year ago

Pretty sure normalized where needed (ie verification) or not really needed pkh/cacao, although it probably still makes sense to normalize on did object for anyone checking equivalence (will add that)

But i still cant replicate the issue in tests with an uppercase/checksum address, they all pass still in this format. Will have to continue looking.

zachferland commented 1 year ago

no updates still, trying to sign up for fireblocks now so i can try it myself and replicate, difficult to debug when it fails on creation

alex-mccreary commented 1 year ago

@zachferland Is there anything I can do to help you debug?

orenyomtov commented 1 year ago

no updates still, trying to sign up for fireblocks now so i can try it myself and replicate, difficult to debug when it fails on creation

@zachferland dm me on twitter @orenyomtov and I'll get you access to fireblocks so you can replicate and debug this

zachferland commented 1 year ago

@orenyomtov messaged you there, thanks!

zachferland commented 1 year ago

was able to fully replicate now, thanks @orenyomtov & @alex-mccreary, will hopefully be able to follow up with a fix/solution in next few days

zachferland commented 1 year ago

@alex-mccreary support/fix has been released in @didtools/pkh-ethereum@0.3.0, sorry this took so long, it was quite easy once i could run locally, let me know if it works now

@orenyomtov a few param type checks in the fireblocks provider could help improve support as well, eth_sign expects a fixed length hex message (hash), while personal_signs expects a variable length hex encoded utf8 message and most common wallet providers (including metamask) support passing both a utf8 string or a hex encoded utf8 string to personal_sign

linked pr has some details as well, closing now

alex-mccreary commented 1 year ago

@zachferland I updated to @didtools/pkh-ethereum@0.3.0 and tested the authentication method from my previous comment (https://github.com/ceramicnetwork/js-did/issues/131#issuecomment-1359798341)

But, I get an error on the const session = await DIDSession.authorize(authMethod, { line for authMethod:

Argument of type 'import("c:/Git/project/node_modules/@didtools/pkh-ethereum/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).AuthMethod' is not assignable to parameter of type 'import("c:/Git/project/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).AuthMethod'.
  Type 'Promise<import("c:/Git/project/node_modules/@didtools/pkh-ethereum/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).Cacao>' is not assignable to type 'Promise<import("c:/Git/project/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).Cacao>'.
    Type 'import("c:/Git/project/node_modules/@didtools/pkh-ethereum/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).Cacao' is not assignable to type 'import("c:/Git/project/node_modules/@didtools/cacao/dist/cacao", { assert: { "resolution-mode": "import" } }).Cacao'.
      The types of 's.t' are incompatible between these types.
        Type '"eip191" | "eip1271" | "solana:ed25519" | "tezos:ed25519" | "stacks:secp256k1"' is not assignable to type '"eip191" | "eip1271" | "solana:ed25519"'.
          Type '"tezos:ed25519"' is not assignable to type '"eip191" | "eip1271" | "solana:ed25519"'.

Could you share the package versions and code you ran to get it working locally?

Thanks,

zachferland commented 1 year ago

@alex-mccreary did you try removing all node modules and reinstalling? and make sure you are using did-session@^2.0.0 now as well

alex-mccreary commented 1 year ago

@zachferland I updated to did-session@^2.0.0. @ceramicnetwork/http-client has a dev dependancy of dids which seems to be conflicting with did-session. I'm now getting this error: this.ceramic.did = session.did;

Type 'import("c:/Git/project/node_modules/did-session/node_modules/dids/dist/did", { assert: { "resolution-mode": "import" } }).DID' is not assignable to type 'import("c:/Git/project/node_modules/dids/dist/did", { assert: { "resolution-mode": "import" } }).DID'.
  Types have separate declarations of a private property '_client'.ts(2322)
zachferland commented 1 year ago

would also make sure to install all the latest ceramic dependencies and reinstall everything, typically these errors are just from version mismatches between ceramic packages

alex-mccreary commented 1 year ago

@zachferland I am still encountering this error after deleting /dist and /node_modules and running yarn again. Here are my package versions: "@ceramicnetwork/http-client": "^2.20.0", "@didtools/pkh-ethereum": "^0.3.0", "@fireblocks/fireblocks-web3-provider": "^1.2.1", "did-session": "^2.0.0",

These are all the newest versions at this time. Are you able to provide your working example?

zachferland commented 1 year ago

@alex-mccreary are you using it with @ceramicnetwork/stream-tile ? i was able to briefly replicate, and then when i updated @ceramicnetwork/stream-tile it was able to build/resolve fine

"@ceramicnetwork/http-client": "^2.21.0",
"@ceramicnetwork/stream-tile": "^2.20.0",
"@didtools/pkh-ethereum": "^0.3.0",
"did-session": "^2.0.0",
alex-mccreary commented 1 year ago

After digging into this more, I found that when installing the "dids" package, it was pulling v3.4.0 instead of v4.0.0 as defined in @ceramicnetwork/common/package.json and did-session/package.json.

After explicitly setting "dids": "^4.0.0", in my main package.json's devDependencies, this issue was resolved.

I confirmed that updates to streams is now working after re-authentication.

Thank you for your help!