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

Support for did param "signed-ietf-json-patch" #17

Closed
OR13 opened this issue Aug 24, 2020 · 20 comments
Closed

Support for did param "signed-ietf-json-patch" #17

OR13 opened this issue Aug 24, 2020 · 20 comments

Comments

@OR13
Copy link
Contributor

OR13 commented Aug 24, 2020

did:key did document representations are currently immutable, and can not be used for testing various cases where services / other keys are needed. This means did:key does not work with DIDComm, and did key cannot be bi-directionally linked to a domain with well known did configuration.

propose we add support for signed-ietf-json-patch:

https://github.com/decentralized-identity/did-spec-extensions/blob/master/parameters/signed-ietf-json-patch.md

This would allow did key to be used with didcomm.

because it would allow did key resolvers to be configured to return did documents that contained service endpoints.

@tmarkovski
Copy link

tmarkovski commented Feb 22, 2021

Bumping this, as we're considering adding support for this as tracked by decentralized-identity/did-key.rs#8

Primary motivation is to allow communicating service related data in offline use cases, like BLE addresses and parameters using DIDComm, but not limited to that transport.

What should the spec define in terms of implementers guidance? Is the patch allowed to remove a verification method? Add other keys? Should it be restricted to managing service related data only, considering the keys are immutable?

@OR13
Copy link
Contributor Author

OR13 commented Feb 22, 2021

We are also considering support for this, as tracked by transmute-industries/did-key.js#72

Similar use case, we want to be able to send a did url that contains a did-key + a service.

Right now, this is a major reason to use did:web, or other did methods over did:key (lack of service support is a major missing feature)

@OR13
Copy link
Contributor Author

OR13 commented Feb 22, 2021

ping @dlongley @dmitrizagidulin @tplooker

@OR13
Copy link
Contributor Author

OR13 commented Sep 23, 2021

did key remains incompatible with didcomm, use did:web instead of did key.

@tmarkovski
Copy link

@OR13 Can you please comment a little on the incompatibility with didcomm that was discovered?

@OR13
Copy link
Contributor Author

OR13 commented Jan 31, 2022

@tmarkovski AFAIK, didcomm requires the service endpoint definition in the did document... did key does not support service.

@tmarkovski
Copy link

Thanks, I was wondering if there's a different issue. This extension aims to solve that problem. We'll be taking a first pass at adding support for this in the rust implementation.

@OR13
Copy link
Contributor Author

OR13 commented Jan 31, 2022

it won't work... see https://github.com/OR13/did-params-and-you

Specifically the credentials would be bound to a different identifier.

https://github.com/OR13/did-params-and-you/blob/master/examples/case-2-illegal-working.json

the issuer.id needs to match the didDocument.id... which is forbidden from being a DID URL.

@tmarkovski
Copy link

What about use case outside credentials? DIDComm would utilize this mostly for communicating service endpoint to complete key exchange. Issuance of credentials doesn't need to happen using a patched DID Document, if someone chooses to use the method for that purpose (which should be discouraged anyway).

@dlongley
Copy link
Contributor

I think the right solution to this problem is for DIDComm to support other mechanisms for specifying service endpoints, such as using VCs -- that can have (potentially very short) expiration periods on them. Expiring this sort of information is important so that there's no confusion over values that are no longer valid. It's also important that no new "patch" or additional update is required in order to know that something has become invalid. This sort of information may be omitted to create vulnerabilities -- so the same piece of information that is used to declare the presence of a service endpoint should also indicate when that information expires.

Otherwise, IMO, you need some kind of VDR to make this sort of thing work safely -- and did key is specifically designed to work without one.

@tmarkovski
Copy link

tmarkovski commented Feb 2, 2022

I agree, DIDComm is limited in how service information can be communicated, and seem to have adopted the did:peer way of doing this through message exchange. I should be able to specify did:example:alice#service-3 to my negotiating party, when I have a list of multiple services in my DID Doc. Dereferencing can play a bigger role here.

On an unrelated note, any objections to simplifying signed-ietf-json-patch parameter to just patch and updating the extension? The reason for this is mostly for use with QR codes which favor shorter character sequences. If there aren't any strong objections, I can make a PR and continue the discussion there.

@OR13
Copy link
Contributor Author

OR13 commented Feb 2, 2022

I think DIDComm becomes nearly useless if it only supports 1 DID Method... But that does not mean that it will support all DID Methods... I think that DIDComm will never support DID Key.

the place to make the change to did extensions is here: https://w3c.github.io/did-spec-registries/#signedIetfJsonPatch-param PRs are always welcome :)

@tmarkovski
Copy link

DIDComm should support any DIDs within the scope of the DID Core spec, or risk becoming DIDPeerComm. DID Key does have a service entry through the use of this patch, and that does not change the controller of the DID, since the output from the dereferencing of the DID URI is going to be a DID Document with service entry, i.e. controller of did:example:alice#service-1 is still did:example:alice.

I understand this DIDComm issue is more political and not technical at this point.

@OR13
Copy link
Contributor Author

OR13 commented Feb 2, 2022

DID Key does have a service entry through the use of this patch, and that does not change the controller of the DID, since the output from the dereferencing of the DID URI is going to be a DID Document with service entry, i.e. controller of did:example:alice#service-1 is still did:example:alice.

I answered this question, above here: #17 (comment)

Please don't assert that "did:key" supports DIDComm or "ietf-json-patch"... I don't think that is true or will become true without changes to the "did:key" specification.

You are welcome to open a PR here, to try and make it true, but be careful saying it's true without that PR having been merged.

@OR13
Copy link
Contributor Author

OR13 commented Feb 2, 2022

The reason I opened this issue originally was to open the door to such a PR, but as I noted in the examples above.

I have become convinced that is not supported today, and will not work in the future.

Perhaps we should close this issue with the note that "did:key" does not support "signed-ietf-json-patch".

To make it clearer for future readers.

@dlongley
Copy link
Contributor

dlongley commented Feb 2, 2022

+1 to close and not support signed-ietf-json-patch. I don't think it solves use cases safely.

@OR13
Copy link
Contributor Author

OR13 commented Feb 2, 2022

Until further notice, did:key will not support signed-ietf-json-patch. If you don't agree, please open a PR that addresses adding support for it to the did key method spec.

@OR13 OR13 closed this as completed Feb 2, 2022
@tmarkovski
Copy link

tmarkovski commented Feb 2, 2022

I think it's premature to close this based on the current compatibility with DIDComm. Verbiage and extension descriptions on how to handle URL parameters can be clarified both in DID Core and in the extension itself. I still think there's value in supporting this for did:key.

@OR13
Copy link
Contributor Author

OR13 commented Feb 4, 2022

@tmarkovski I can reopen and assign you if you like?

Next step would be a PR on this repo to describe how to support the feature, and you would have the ball for that PR.

@tmarkovski
Copy link

I'm working on one, will just submit a PR when ready. I think I gathered all the issues/discussions related to this over the past couple of years and will address them in the PR.

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

No branches or pull requests

3 participants