URDNA2015Canonicalizer: Custom loader not being assigned to LdProof's object

[vitorpamplona]

Here a custom loader is correctly assigned to jsonLdObjectWithoutProof but not to ldProofWithoutProofValues. I am not sure why. But I think it is prudent to assign to both objects.

This has to do with not instantiating default DocumentLoaders to avoid the need for java.net.http.HttpClient on runtime (Android's don't have it)

[peacekeeper]

@vitorpamplona I'd like to follow up on the issues you opened..

I'm not sure I understand this one here.. When the ldProofWithoutProofValues is constructed using LdProof.builder(), a default DocumentLoader for that is used, which is LDSecurityContexts.DOCUMENT_LOADER.

Is this still an issue after we merged https://github.com/decentralized-identity/jsonld-common-java/pull/6?

If yes, could you maybe propose some change to address this issue?

[vitorpamplona]

The idea is to NEVER use the default loader. It looks like ldProofWithoutProofValues should use the same loader as jsonLdObjectWithoutProof, unless I am missing something about the difference between these two variables

[vitorpamplona]

I just can't see why the custom loader is used in one but not in the other.

[peacekeeper]

Well the idea is that ldProofWithoutProofValues would only contain something like the following JSON-LD object:

  "proof": {
    "type": "Ed25519Signature2018",
    "created": "2017-06-18T21:19:10Z",
    "proofPurpose": "assertionMethod",
    "verificationMethod": "https://example.edu/issuers/565049/keys/1",
  }

I.e. only the proof, and without the actual proof value.

This should only need standard contexts using the standard document loader (see here).

[vitorpamplona]

And that is the loader I am trying to avoid at all costs :)

[peacekeeper]

And that is the loader I am trying to avoid at all costs :)

But why? Didn't we fix the HttpClient dependency problem when we merged https://github.com/decentralized-identity/jsonld-common-java/pull/6 ?

[vitorpamplona]

Because in the end, we don't want all those JSONs in memory (we still use them in memory as of now, but it is not a good solution).

[vitorpamplona]

And remember, the way Android manages resources is different than Java applications. I'd expect any serious use of this library to have its own preference for resource loading/caching mechanisms. Especially, JSON files which are still quite slow to parse in Java.

[peacekeeper]

I see, makes sense. Not sure though if I just want to use the same document loader as for jsonLdObjectWithoutProof.

How about we make this configurable, e.g. by having a member variable in the class URDNA2015Canonicalizer that can be changed, or a getter method that can be overridden by subclasses?

[vitorpamplona]

Not sure though if I just want to use the same document loader as for jsonLdObjectWithoutProof.

Why would you want to use two loaders? That's the part I am not understanding.

[peacekeeper]

Why would you want to use two loaders?

I'm not sure if I have a good answer to that. We're basically talking about this algorithm here: https://w3c-ccg.github.io/data-integrity-spec/#create-verify-hash-algorithm.

I guess I want to make sure that the "canonicalized options document" (step 4.1) is guaranteed to be calculated from a JSON-LD context that correctly defines the relevant security terms such as proofPurpose, verificationMethod. If a custom document loader is used, especially one with JSON-LD contexts supplied by external users, I'm worried that might create an attack surface that affects the proof generation.

But since the proof eventually becomes a part of the overall JSON-LD document, I suppose it also makes sense to just use the same document loader, which (in theory) should always define the security terms correctly.

[vitorpamplona]

hum.. that doesn't seem to be protecting much.

I would be more concerned about making sure all JSON fields were included (in any way) into the canonicalization string.

I am finding several implementations that simply skip JSON fields that don't follow the spec (say, when the ID is not a URI). Since developers don't manually check if all fields were part of the canonicalization, we have seen instances of VCs that can be changed entirely and would still be verifiable (credentialSubject wasn't included in the canonicalization due to a given field failing to meet the spec).

[peacekeeper]

Okay I guess I would also be fine with using the same document loader as for the main document, as you suggested. Do you want to create a PR?

BTW I think some time next week we'll do 1.0 releases of both jsonld-common-java and ld-signatures-java... It would be great if you could do PRs for the issues we discussed here and in jsonld-common-java, perhaps in the next few days?