Using PortableDid to import did to sign VC, removing kid as default value

[jiyoonie9]

test vector changes for decentralized-identity/web5-kt#262

[nitro-neal]

We made a new issue to update the credentials create() but this one will work for now (since kt the only one that consumes it)

and yup make sure web5-js works and bump the spec hash of web5-js 👍

[decentralgabe]

I am hesitant to make this change because I am not sure portable did is a concept that should be exposed in the spec.

the spec serves mainly to facilitate interop, and the serialization format of DIDs seems to be an internal feature to the SDKs. if we have a use case to enable interoperability via portable DID that would help change my mind here.

[jiyoonie9]

That's fair... I made this change because this test is about importing an existing did, signing a VC with the equivalent bearer did, and checking that the vcjwt created is what we expect.

In your view, how could we test this aspect of credential creation but still keep interop top of mind?

[decentralgabe]

We could still define input keys (or key seeds) and expected outputs without having a common bearer did structure.

The DID working group argued about a similar concept for a while and agreed to not define a serializable representation of private keys as to avoid recommending doing it, since it's a security risk to transmit private keys.

Some DID methods do provide private keys. Others provide public keys and test for common identifier generation and document expansion, like did key: https://w3c-ccg.github.io/did-method-key/#ed25519-x25519

[jiyoonie9]

hmm... it seems like you may not be onboard with the idea of PortableDid existing in general, because its purpose is basically to transmit private key (imo)?

if that's the case, then we need to have a bigger discussion around why portabledid should or should not exist. i'd appreciate it if you could raise it as a ticket in this repo and document your concern around it.

as far as this test vector is concerned, would you feel more comfortable if i got rid of the signerPortableDid key, and have uri, privateKeys, document directly under input key? then it's not really introducing the concept of portabledid to the test vectors, but allows the test vector consumer to still have the necessary info to create a did they can then sign the VC with to produce the same expected vector.output vcJwt?

[decentralgabe]

as far as this test vector is concerned, would you feel more comfortable if i got rid of the signerPortableDid key, and have uri, privateKeys, document directly under input key? then it's not really introducing the concept of portabledid to the test vectors, but allows the test vector consumer to still have the necessary info to create a did they can then sign the VC with to produce the same expected vector.output vcJwt?

yes this works!

[KendallWeihe]

I think we probably have a number of existing test vectors which have implementation-specific assumptions, such as the concept of a "portable DID" so even under the working assumption "portable DID" is not something to be within the web5 spec (that's my working stance) I think it pragmatic to expect to commit to a work item at some point to twease apart our test vectors for vectors which are spec-specific vs our-implementations-specific

TL;DR I agree test-vectors/portable_did/* shouldn't be where it is but we can come back later b/c we need to review all test vectors

[jiyoonie9]

hmm, so there seems to be a couple opinions emerging on what test vectors should do -

1. allow for interop with external repos that want to make sure their way of building and using DIDs and VCs is consistent with ours (this comment)

2. allow for interop between our per-language sdk implementations to make sure we are all building and using DIDs and VCs consistent with (each other) this comment

i think it should do both, which is why i pulled out the portableDid concept out of this test vector for VC creation, and created a new test-vectors/portable_did/parse.json test vector for parsing in a portable did and importing into a bearerdid.

[mistermoe]

@decentralgabe

if we have a use case to enable interoperability via portable DID that would help change my mind here.

we do. right now across our sdks, you can port a did around using PortableDid. trying to better understand how inclusion of portableDid prevents interop. couldn't third parties just ignore the "portableDid" property and go straight to private keys if desired? asking because leaving it out makes it more confusing / a bit awkward when consuming the vectors in our own sdks

[jiyoonie9]

ok. i reverted back the changes to make it easier for other sdks to consume the portabledid when writing tests against this vector.

i think this needs a bit more discussion so wrote an issue here: #142

[decentralgabe]

you can port a did around using PortableDid

I believe this has security risks and we should not support it, though I acknowledge it's useful for test vectors. Happy to continue the discussion in #142.

[diehuxx]

lgtm. ty for testing this in both JS and KT

[KendallWeihe]

ty for testing this in both JS and KT

@kirahsapong are you aware of this PR? Will the changes here impact web5-swift?

[KendallWeihe]

is it too much to ask to use a did:jwk instead? since we deprecated did:key

[jiyoonie9]

this test vector specifically tests for creating a VC as a jwt with a did:key so i'd like to keep it here :)

[KendallWeihe]

so what was the rationale for removing these kid's in this file?

[jiyoonie9]

the kid is now not populated by default and computed whenever it is necessary. the test vector (previously using JWK) assumed that it would be populated by default so the test would fail.

same for alg - alg also isn't populated by default, since crv (a required field) is more accurate anyway.