iotaledger / iotaledger/identity

[Task] Fix edge cases in `CoreDocument::resolve_method`.

Open
#1,078 0 comments 0 reactions 0 assignees View on GitHub
Bug Rust Wasm
Dominant language
Rust
Stars
346
Forks
100
Avg merge
13h 41m
Merged PRs (30d)
2

Description

## Description

The current `CoreDocument::resolve_method` (resp. `CoreDocument::resolve_service`) method demonstrates unexpected behaviour when documents contain methods (resp. services) whose identifier contains a DID string different from the document's identifier. Here is an example:

Consider the following somewhat strange, yet (to the best of my knowledge) technically spec compliant DID document:
```
{
"id": "did:example:123",
"authentication": [
"did:other:456#key1
{
"id": "did:example:123#key1",
"type": "Ed25519VerificationKey2018",
"controller": "did:example:123",
"publicKeyMultibase": "zAKJP3f7BD6W4iWEQ9jwndVTCBq8ua2Utt8EEjJ6Vxsf"
}
],
"assertionMethod": [
{
"id": "did:other:456#key2",
"type": "Ed25519VerificationKey2018",
"controller": "did:other:456",
"publicKeyMultibase": "zHgo9PAmfeoxHG8Mn2XHXamxnnSwPpkyBHAMNF3VyXJCL"
},
{
"id": "did:example:123#key2",
"type": "Ed25519VerificationKey2018",
"controller": "did:example:123",
"publicKeyMultibase": "z5TVraf9itbKXrRvt2DSS95Gw4vqU3CHAdetoufdcKazA"
}
]
}
```
if the query `#key1` is passed to `CoreDocument::resolve_method` one would *expect* to receive the method whose identifier is
`"did:example:123#key1"` while the actual behaviour is that nothing gets returned from that query (because `"did:other:456#key1"` matches the fragment). Similarly if `#key2"` is passed as a query the verification method with identifier `"did:other:456#key2"` gets returned contrary to our expectations again because it is the first to match the fragment.

## Possible solutions

On a very high level there are two ways forward (that I can think of):

1. Forbid constructing documents containing methods and/or services with identifiers whose DID component does not match the document's identifier.
2. Change the behaviour of `CoreDocument::resolve_*`.

The advantages of 1. is that for many users and use-cases it might be desirable for the edge cases demonstrated in the description to not even be representable. This approach unlocks the ["parse don't validate" principle](https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/) which would allow us to keep the relatively simple `query`/ `resolve_*` logic we currently use.

The disadvantage of 1. is that for the (what we currently believe to be) few who rely on the ability to store methods or services with a DID component different from the base in the document will not be able to use our library, at least not to represent their DID documents. There might turn out to be good use-cases for this sort of thing that we haven't thought of, besides it is often a good idea to be as spec compliant as possible.

As for 2. there are a few possible ways of doing this. Here are a few ideas:

- Enforce queries to be full DID URLs (i.e. change the input parameter to `resolve_*` to take a `DIDUrl` rather than `impl Into`) .
- If a query is just a fragment then prepend the document's identifier internally prior to running the query.
- Only match against methods/services whose DID component matches the documents when resolving the method/service.

Regardless of which flavour is chosen the value the `Queryable` (and to a lesser extent `KeyComparable`) trait brings becomes more questionable, maybe to the point where a redesign or complete removal should be considered. To be clear this is only relevant as long as we do not go with the first proposed option.

Furthermore with #1067 it is currently not possible to insert a reference to a method in another document post document construction, if we want to allow such things this needs to be addressed.

## Motivation
The library should follow the principle of least astonishment which it currently doesn't in the edge cases described in the discription.

## Resources
Loosely related to https://github.com/iotaledger/identity.rs/pull/1067 and https://github.com/iotaledger/identity.rs/issues/1072.

## To-do list
Create a task-specific to-do list. Please link PRs that match the TODO list item behind the item after it has been submitted.
- [ ] Item one
- [ ] Item two
- [ ] Item three

## Change checklist
Add an `x` to the boxes that are relevant to your changes, and delete any items that are not.
- [ ] The feature or fix is implemented in Rust and across all bindings whereas possible.
- [ ] The feature or fix has sufficient testing coverage
- [ ] All tests and examples build and run locally as expected
- [ ] Every piece of code has been document according to the documentation guidelines.
- [ ] If conceptual documentation (mdbook) and examples highlighting the feature exist, they are properly updated.
- [ ] If the feature is not currently documented, a documentation task Issue has been opened to address this.

Contributor guide

Open the contributing guide

Research direction

Start by reading CoreDocument::resolve_method and CoreDocument::resolve_service, then inspect the Queryable and KeyComparable behavior and the related discussions in #1067 and #1072. The issue is done only after one resolution policy is chosen, the cross-DID edge cases are covered by tests, and the required Rust bindings and documentation changes are identified.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.