digitalbazaar / edv-client

An Encrypted Data Vault Client
BSD 3-Clause "New" or "Revised" License
13 stars 9 forks source link

Errors when controller !== publicKey.id #16

Closed OR13 closed 4 years ago

OR13 commented 4 years ago

When attempting to use did methods other than did:key, it is common that,

did !== publicKey.id && did === publicKey.controller

The controller is expanded here:

https://github.com/digitalbazaar/http-signature-zcap-verify/blob/master/main.js#L69

Setting:

verificationMethod.controller = 'did:github:OR13';

will resolve this issue for me...is my DID Document constructed incorrectly?

https://uniresolver.io/1.0/identifiers/did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o

I believe this may be a logical error resulting from testing with did:key... I think it would advisable for did keys to not overload the concept or id, in other words, did:key would be a better test method if:

did !== publicKey.id && did === publicKey.controller

IMO, public keys listed in a did document SHOULD have id property of the following form:

${did}#${publicKey.fingerprint()}

OR13 commented 4 years ago
>  main.js 73:  { id: 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3',
>    type: 'Ed25519VerificationKey2018',
>    assertionMethod:
>     [ 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3' ],
>    authentication:
>     [ 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3' ],
>    capabilityDelegation:
>     [ 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3' ],
>    capabilityInvocation:
>     [ 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3' ],
>    controller: 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3',
>    keyAgreement:
>     [ { id:
>          'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3#zC6JZRVUXwWkJGLRvvTtaJbxuNjq4x6Cs1VnPLCun5miSe',
>         type: 'did:/X25519KeyAgreementKey2019',
>         controller: 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3',
>         publicKeyBase58: '6oqi8Hik911qENxU4Q1LSoE5VZFzXWFWbu6JMpygfULS' } ],
>    publicKey: 'did:key:z6Mkrq9n8us7jJqp9kaSaRtKisdW2sz5xD52hrrRp7EeCyV3',
>    publicKeyBase58: 'DNtjYfcgPmMM3FjjtrvUsn5WDJiEYKpg1qwVyqGdHkhf' }

vs

>  main.js 73:  { id:
>     'did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o',
>    type: 'Ed25519VerificationKey2018',
>    controller:
>     { id: 'did:github:OR13',
>       assertionMethod:
>        [ 'did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o' ],
>       capabilityDelegation:
>        [ 'did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o' ],
>       capabilityInvocation:
>        [ 'did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o' ],
>       keyAgreement: [ [Object] ],
>       'sec:proof': {},
>       publicKey:
>        [ [Object],
>          [Object],
>          'did:github:OR13#XJZ3sHC-idqckHQCYQSQBlNYcBNxh8FePiD_KenzZ2o' ] },
>    publicKeyBase58: 'DNtjYfcgPmMM3FjjtrvUsn5WDJiEYKpg1qwVyqGdHkhf' }

https://github.com/digitalbazaar/ocapld.js/blob/master/lib/utils.js#L91

Here my controller is my expanded didDoc...

OR13 commented 4 years ago

Seems like, controller is always expected to be a string... https://github.com/digitalbazaar/crypto-ld/blob/9de52b8032dea212b8815e53769e986881f2eef1/lib/LDKeyPair.js#L23

Yet, _getVerificationMethod returns an object because of framing.

I'm not sure how to fix this. I'm surprised at how different the verificationMethod object is...

If I had to guess, I would think the error is in here:

async function _getVerificationMethod({keyId, documentLoader}) {
  // Note: `expansionMap` is intentionally not passed; we can safely drop
  // properties here and must allow for it
  const {'@graph': [framed]} = await frame(keyId, {
    '@context': SECURITY_CONTEXT_V2_URL,
    '@embed': '@always',
    id: keyId
  }, {documentLoader, compactToRelative: false});
  if(!framed) {
    throw new Error(`Verification method ${keyId} not found.`);
  }

  // ensure verification method has not been revoked
  if(framed.revoked !== undefined) {
    throw new Error('The verification method has been revoked.');
  }

  return framed;
}

This is a fairly complicated way of getting the publicKey object from the didDocument, and in this case, the JSON-LD alteration of the controller property causes it to no longer be a string, which then errors later.

OR13 commented 4 years ago

I believe the github-did doc is constructed correctly, because the latest version of github-did supports ed25519 json-ld-signature verification

The code is a bit different, but when the public key is passed here, there is no issue (because the controller is pulled directly from the did document).

dlongley commented 4 years ago

I think that github-did doc is fine. I'll have to look more closely at what's going on here to figure out what the issue/resolution is. The structure of the did:key doc is going to change to have different IDs for the keys vs. the DID subject (this was just a mistake), however, it still should have worked either way.

dlongley commented 4 years ago

It may be that the solution is just to update the frame in _getVerificationMethod to ensure controller is present and uses a reference instead of embedding controller information in it, i.e.:

  const {'@graph': [framed]} = await frame(keyId, {
    '@context': SECURITY_CONTEXT_V2_URL,
    '@embed': '@always',
    id: keyId,
    controller: {'@embed': 'never'},
  }, {documentLoader, compactToRelative: false});

It may also be the case that LDKey needs to allow for more than one controller, but that is a separate issue.

OR13 commented 4 years ago

@dlongley Thanks for looking into this.

Your comment and reviewing: https://w3c.github.io/did-core/#dfn-did-controller-s

Has reminded me that the controller property of both did document and the public key can be either a did string, or an array of did strings.

However, in the case of capability invocation, the keyId will resolve to a single public key object. There will always be at least a single did string in the controller to match against the capability's invokers list.

Just to make sure, if we implement your suggestion, we can later makes changes to LDKey to support controller values that are arrays.

If thats the case, I'm happy to open a PR with your suggested changes.

This issue is a current blocker for universal resolver DID Methods, and next steps for edv interop on our side, I'd be happy to address it.

dlongley commented 4 years ago

Just to make sure, if we implement your suggestion, we can later makes changes to LDKey to support controller values that are arrays.

Yeah, I don't see anything blocking that.

@dmitrizagidulin -- you have any thoughts/want to weigh in?

OR13 commented 4 years ago

I found this issue to be resolved with:

    "http-signature-zcap-verify": "^1.0.2",
dlongley commented 4 years ago

Thanks, @OR13. I'm going to go ahead and close this then.