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

Formalize the use of the endpoint ATTRIB and rendering the DID Document #296

Merged
merged 7 commits into from
Jun 24, 2021

Conversation

swcurran
Copy link
Contributor

@swcurran swcurran commented Jun 3, 2021

Signed-off-by: Stephen Curran [email protected]

Formalizes the convention used on Sovrin to add an "endpoint" ATTRIB to a NYM.

Formalizes how to render a DID Document v1.0 (Candidate) using the data from the NYM and ATTRIB, including generating three different service blocks to handle AIP 1.0, AIP 2.0 and DIDComm V2 formats.

The goal in formalizing the rendering is to make sure that the Universal Resolver, and local resolvers, such as the did:sov resolver in ACA-Py produce identical, useful results.

@swcurran
Copy link
Contributor Author

swcurran commented Jun 3, 2021

Heads up to several people -- love to have your feedback on this:

  • @peacekeeper -- Does this look good to you? Notably -- the three service blocks. I assume we can do a PR to the Universal Resolver once this is done. Also, do you know if we need to add some @context entries for the service blocks?
  • @dbluhm @andrewwhitehead -- Does this look good to you? Easy enough to alter the ACA-Py local did:sov resolver if this is accepted? Will the three service blocks get used as I expect them to in ACA-Py -- e.g. that an agent will just use the service block that they recognize?
  • @TelegramSam, @dhh1128 -- Is the service block for DIDComm 2 correct? I notice it does not have the recipientKeys array. Is that because that goes in the message header? What do you think of the rest of this?
  • @Moopli @troyronda -- Could you please look at the rendered DID Doc and confirm that such a result will work for you?

Thanks all!

@swcurran
Copy link
Contributor Author

swcurran commented Jun 3, 2021

@TimoGlastra -- forgot to include you on my last comment. Does this DID Doc rendering look like it achieves what you were suggesting on the Aries WG call the other day?

@andrewwhitehead
Copy link

Shouldn't the DIDComm service entry have recipientKeys as well? Otherwise, looks good to me.

@swcurran
Copy link
Contributor Author

swcurran commented Jun 4, 2021

Shouldn't the DIDComm service entry have recipientKeys as well? Otherwise, looks good to me.

I thought so, but I pulled the example from the DIDComm V2 Spec., and it didn't have the recipientKeys array.

@dbluhm
Copy link
Contributor

dbluhm commented Jun 4, 2021

Similar thoughts from me -- missing recipientKeys feels odd but my head's not in the DIDComm V2 spec. This should be a simple enough modification to the local did:sov resolver in ACA-Py.

@TimoGlastra
Copy link

Looks good to me.

If I remember correctly all keyAgreement keys should be used as recipient keys

@swcurran
Copy link
Contributor Author

swcurran commented Jun 4, 2021

If I remember correctly all keyAgreement keys should be used as recipient keys

@TimoGlastra -- what is the implication of that? Does that change how this should be handled?

@TimoGlastra
Copy link

The keyAgreement entry should be added in that case. I think it should be added either way. Isn't that the one used for DIDComm?

@swcurran
Copy link
Contributor Author

swcurran commented Jun 4, 2021

Apologizes from my lack of knowledge here, but I think that means that we need to do an ed25519 to x25519 transformation to create the Key Agreement, and then add that into the DID Doc. E.g add this to the DIDDoc:

  "keyAgreement": [
    {
      "id": "did:sov:12345678#key-agreement-1",
      "type": "X25519KeyAgreementKey2019", 
      "controller": "did:example:123",
      "publicKeyMultibase": "z9hFgmPVfmBZwRvFEyniQDBkz9LmV7gDEqytWyGZLmDXE"
    }
  ],

And then update the references in the recipientKeys array to be did:sov:12345678#key-agreement-1.

Is that right?

@swcurran
Copy link
Contributor Author

swcurran commented Jun 4, 2021

FYI -- I copied that keyAgreement block from the DID Core Spec. Should the publicKey use multibase?

I want to be sure that we get this precise, so there is not ambiguity in generating or using the DID Doc, hence my confusion with the "looks good to me" comment and then the reference to the need for the keyAgreement block...

@TimoGlastra
Copy link

Sorry about that, I realised it just after posting my comment. Don't have a computer at hand at the moment. I'll double check tomorrow and post a correct example.

<h4>DID Service Endpoint</h4>
<p>By convention, Indy DID Controllers often create an Indy ATTRIB object (using the <a href="https://hyperledger-indy.readthedocs.io/projects/node/en/latest/requests.html#attrib">"ATTRIB"</a>
transaction) related to the NYM with a raw value of "endpoint", and a URL that represents the service endpoint for the DID. As noted in the "Read" section of this specification
such an "endpoint" ATTRIB is read when resolving a DID and it's data included in the resulting DID Document.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"it's data" is a misspelling; should be without the apostrophe: "its data"

@dhh1128
Copy link
Contributor

dhh1128 commented Jun 4, 2021

If I remember correctly all keyAgreement keys should be used as recipient keys

Yes, this is correct. All of Alice's keyAgreement keys should be targeted as a recipient in the final leg of a DIDComm message route that is sent to Alice. However, we do need a way to specify routingKeys so that it is possible to use a mediator with a did:sov.

<li>The DID Document text is boilerplate except for the "did:sov..." identifier, the public key ("publicKeyBase58) and the references to the service endpoint ("serviceEndpoint").</li>
<li>If there is no result from the GET_ATTRIB transaction, the "service" block is not included in the document.</li>
<li>The three entries in the service block cover the three "eras" of DIDComm, AIP 1.0 (type "endpoint"), AIP 2.0 (type "did-communication") and DIDComm V2 (type "DIDComm").</li>
<li>Because there is just one endpoint specified, the service blocks cannot make use of the DIDComm routing capabilities for messaging based on the DIDDoc.</li>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there is just one endpoint specified

I don't understand this comment. I see several different places where serviceEndpoint is given in the service array. All of them have the same value, but I don't understand why that would matter. Can the sentence be clarified?

Copy link
Contributor Author

@swcurran swcurran Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Daniel -- I'm trying not to limit what we're doing here. Currently, the convention that Indy users are following is to have an ATTRIB with a raw value of `{"endpoint":{"endpoint":"http://example.com/didService"}}. There are 313 such instances (as I type this) on Sovrin MainNet today.

If you look at the Universal Resolver, it's output includes that information, in the form of the simple "endpoint" service block. What this PR does is just expand the single "endpoint" we have based on the convention being used, and provide proper DIDComm service blocks for AIP 2.0 and DIDComm V2.

Assuming we don't change the convention, we have no where to specify a mediator/routingKeys -- hence that is hard-wired to be an empty array.

What we really want is did:indy -- but that's not happening fast enough...

Make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yes. Thank you for the clarification. That helps a lot.

With this PR, would it be possible to start up a new convention (without writing any new code in the ledger--only in clients) where the raw value would also include routingKeys data, and this would then be returned by the UR? If so, then maybe it would be better to NOT insert an empty array for routingKeys, and just let the data stored by the ledger be returned. That way we could start defining endpoints without routing keys if we were motivated to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to the see the focus put on did:indy for that, so I personally would prefer not. I suppose it would take off the pressure to get did:indy implemented. I'd love to know how much people would use this if we did it now. Easy enough though -- like this I think:

{
  "endpoint": {
    "endpoint": "http://example.com/didService",
    "routingKeys": [ "did:key:12345", "did:key:67890" ]
  }
}

BTW -- my reply to you above was terrible. Just noticed my opening line was the opposite of intended. I said "I'm trying not to limit", but meant to say "I'm trying not to limit". Doh :rofl

Copy link
Contributor

@dhh1128 dhh1128 Jun 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the logic of limiting coding effort -- but what I'm proposing is not extra coding. It's actually less coding. Instead of hard-coding some logic into UR that always adds an empty routingKeys, I'm suggesting that we just ignore the issue entirely. If someone writes an ATTRIB that includes routingKeys inside raw, then UR returns it because the ledger returns it. If they don't have an ATTRIB with routingKeys, UR doesn't emit one, either. All the current ATTRIBs on the record would lack routingKeys and thus be invalid DIDComm endpoints -- and so would new ones written to the ledger using the status quo convention. But by doing nothing, we'd make it possible to adopt a new convention if we were forced to -- without any more changes to the did:sov spec, the ledger, or the UR.

My suggestion is motivated by a lack of confidence in the timeline for did:indy. If we had did:indy today, I wouldn't suggest it at all, because we'd have a better alternative. Perhaps I'm being pessimistic to want to leave us a pressure release valve. Or perhaps there are reasons not to do this that I haven't considered. I wasn't advocating super strongly -- just thinking out loud. I'm content to have the PR merged as-is, if others feel that's the best tradeoff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not as simple as you say. First, the resolver has to know about each raw type (e.g. "endpoint", "routingKeys") and specifically make a call to the ledger to ask for each value. There is no way to know what ATTRIBs a client has written and we don't want the resolver to have to ask for a whole bunch of ATTRIBs on the off chance a DID controller used a specific set of ATTRIBs. As such, I would not want to add any other ATTRIBs as part of this process. "did:indy" is needed for that.

As noted, we could add an extra format for "endpoint" to add routingKeys, but the resolvers would have to add support for that format (update all existing deployments) and clients would have to know about it to use it. Since these endpoints are likely used only during connection establishment, and will be replaced by peer DIDs, the lack of mediators could be worked around.

I agree that "did:indy" won't be around for awhile. However, I think this will be sufficient for the institutional issuers we have today -- the main use case we have for DIDs on Indy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. I had forgotten that each raw value was a separate call. Then I agree with your conclusion.

@TimoGlastra
Copy link

I think the following did document is correct. I removed the duplicate key declarations and instead replaced them with key references. I also added the key agreement with the x25519 key type.

It is a bit weird however as for DIDComm v1 the Ed25519VerificationKey2018 type is used, and for didcomm v2 now (the correct) X25519KeyAgreementKey2019 type.

{
  "@context": ["https://www.w3.org/ns/did/v1"],
  "id": "did:sov:HR6vs6GEZ8rHaVgjg2WodM",
  "verificationMethod": [
    {
      "type": "Ed25519VerificationKey2018",
      "id": "did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-1",
      "publicKeyBase58": "9wvq2i4xUa5umXoThe83CDgx1e5bsjZKJL4DEWvTP9qe"
    },
    {
	  "type": "X25519KeyAgreementKey2019",
      "id": "did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-agreement-1",
      "publicKeyBase58": "3mHtKcQFEzqeUcnce5BAuzAgLEbqKaV542pUf9xQ5Pf8"
    }
  ],
  "authentication": [
    "did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-1"
  ],
  "assertionMethod": [
    "did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-1"
  ],
  "keyAgreement": [
	"did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-agreement-1"
  ],
  "service": [
    { 
	  "type": "endpoint",
	  "serviceEndpoint": "https://example.com/endpoint"
	},
    {
      "id": "did:sov:HR6vs6GEZ8rHaVgjg2WodM#did-communication",
      "type": "did-communication",
      "priority": 0,
      "recipientKeys": ["did:sov:HR6vs6GEZ8rHaVgjg2WodM#key-1"],
      "routingKeys": [],
      "accept": ["didcomm/aip2;env=rfc19"],
      "serviceEndpoint": "https://example.com/endpoint"
    },
    {
      "id": "did:sov:HR6vs6GEZ8rHaVgjg2WodM#didcomm-1",
      "type": "DIDComm",
      "serviceEndpoint": "https://example.com/endpoint",
      "routingKeys": []
    }
  ]
}

@swcurran
Copy link
Contributor Author

swcurran commented Jun 5, 2021

@TimoGlastra -- I agree that we should minimize the number of inline key definitions and use references where possible. That said, I think (but could be wrong...) the inline definition for the key agreement key should be in the keyAgreement block -- it's not a verification key.

Also, wondering... should the recipientKey entries reference the key agreement key (x25519) or the verification key (ed25519)?

@TimoGlastra
Copy link

TimoGlastra commented Jun 5, 2021

the inline definition for the key agreement key should be in the keyAgreement block -- it's not a verification key.

I took reference from https://did.key.transmute.industries/. verificationMethod is the new name for publicKey and it can contain any key for any purpose (as you could use biometric verification methods for example).

From the DID spec:

verification method

... other text ...

"Verification" and "proof" in this definition are intended to apply broadly. For example, a cryptographic public key might be used during Diffie-Hellman key exchange to negotiate a shared symmetric key for encryption. This guarantees the integrity of the key agreement process. It is thus another type of verification method, even though descriptions of the process might not use the words "verification" or "proof."


Also, wondering... should the recipientKey entries reference the key agreement key (x25519) or the verification key (ed25519)?

If we want to be correct I think it should be x25519. However current implementations all expect ed25519 keys. So we should make sure implementations accept x25519 keys before breaking stuff.

@swcurran
Copy link
Contributor Author

swcurran commented Jun 5, 2021

Good stuff. @andrewwhitehead @TelegramSam -- are you in agreement that the key referenced in the recipientKeys array should be ED25519 keys for now?

swcurran added 2 commits June 5, 2021 11:32
…the keys once and use references thereafter; additional clarifying notes about the DIDDoc

Signed-off-by: Stephen Curran <[email protected]>
…IDComm v2 service entry

Signed-off-by: Stephen Curran <[email protected]>
@swcurran
Copy link
Contributor Author

swcurran commented Jun 5, 2021

Updated the PR to change the DID Doc template to match (more or less) Timo's proposal. Added to it the @context for DIDComm v2 and the accept item in the DIDComm v2 service entry. Also correct "its" typo.

I've posted to the DIDComm v2 DIF Slack a question about the lack of recipientKeys item in the DIDComm v2 spec.

@peacekeeper
Copy link

@peacekeeper -- Does this look good to you?

@swcurran Yes, in general the use of ATTRIB with a raw value of {"endpoint":{"endpoint":"http://example.com/didService"}} looks consistent with what the did:sov driver of the Universal Resolver has been doing for a few years.

The Universal Resolver currently doesn't add the X25519KeyAgreementKey2019 verification method. I'd be happy to update that, or alternatively, we could replace the current did:sov driver altogether by using more "official" code from Indy or Aries (see Driver Development).

Notably -- the three service blocks.

It's not quite clear to me why there is one endpoint in the ATTRIB, but then there are three services in the DID document. Does this mean that if the resolver sees the "endpoint" ATTRIB value with an "endpoint" service type, it will automatically generate all three services instead of just one?

@swcurran
Copy link
Contributor Author

swcurran commented Jun 7, 2021

From @peacekeeper:

It's not quite clear to me why there is one endpoint in the ATTRIB, but then there are three services in the DID document. Does this mean that if the resolver sees the "endpoint" ATTRIB value with an "endpoint" service type, it will automatically generate all three services instead of just one?

Yes -- from the one endpoint generate the three service blocks. Since the universal resolver is by definition a single deployment, it can't infer from just a single "endpoint" what the DID is intended to support, so it blindly generates all three. A client using the universal resolver can use the one that it wants -- allowing the client to be "dumber" about how to use a "did:sov". Of course, if a client tries to access the DID Controller's Agent using too advanced a service block, it could fail -- e.g. send a DIDComm V2 message to an Agent that only supports AIP 2.0 (which uses DIDComm V1).

We could define a different convention, but we still have to support the existing endpoint, unchanged.

Proposal: As I type this, I've changed how I think it should work. I suggest that we drop the DIDComm V2 block on the assumption that by the time DIDComm V2 is a thing in the wild, either "did:indy" will also be a thing, or we will have defined a new convention for DIDComm V2.

Extended Convention: We keep the "endpoint" as the raw value, but we add an "types" and routingKeys array, as follows:

{
  "endpoint": {
    "endpoint": "http://example.com/didService",
    "types": [ "endpoint", "aip2", "didcommv2" ],
    "routingKeys": [ "did:key:12345", "did:key:67890" ]
  }
}

@dhh1128
Copy link
Contributor

dhh1128 commented Jun 7, 2021

@swcurran: I don't understand where you're getting the routing keys from, in this new proposal.

@swcurran
Copy link
Contributor Author

swcurran commented Jun 7, 2021

@dhh1128 -- I'm flip-flopping on my comment to you that we not change the convention. :-) I'm going off the idea that we noodled in this comment above. Adding in both routingKeys and types if the DID Controller wants to add them to the ATTRIB.

@dhh1128
Copy link
Contributor

dhh1128 commented Jun 7, 2021

Okay. Makes sense.

@swcurran
Copy link
Contributor Author

swcurran commented Jun 9, 2021

Hey folks -- I've done another update on this spec, as follows:

  • Support the "endpoint" ATTRIB as we have used it to this point and optionally extend it's value to allow specifying the service block entries to include (default to just "endpoint" and "did-communication") and "routingKeys" (default to "[ ]").
  • Put the inline key definitions in the "validationMethods" block (ed25519 and x25519), and use key references everywhere else.
  • Although there is the x25519 keyAgreement key defined -- reference in the ed25519 in the "did-communication" block, because that's what we have always done...

I think that is good to go. Any final comments.

@dbluhm -- would you be able to update the ACA-Py resolver with this revised definition?
@peacekeeper -- we'll take a look at doing the same for the Universal Resolver.

Copy link
Contributor

@dhh1128 dhh1128 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@swcurran
Copy link
Contributor Author

@WadeBarnes -- can you please merge this? Thanks!

@WadeBarnes
Copy link
Member

@swcurran, Will do. We're working on fixing the build, but since the changes here should not affect the build I'll get it merged soon if we don't have the build fixed today or tomorrow.

peacekeeper added a commit to decentralized-identity/uni-resolver-driver-did-sov that referenced this pull request Jun 21, 2021
Comment on lines 247 to 250
"@context": [
"https://www.w3.org/ns/did/v1",
"https://didcomm.org/messaging/contexts/v2"
],
Copy link

@peacekeeper peacekeeper Jun 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"@context": [
"https://www.w3.org/ns/did/v1",
"https://didcomm.org/messaging/contexts/v2"
],
"@context": [
"https://www.w3.org/ns/did/v1",
"https://didcomm.org/messaging/contexts/v2",
"https://w3id.org/security/suites/ed25519-2018/v1",
"https://w3id.org/security/suites/x25519-2019/v1"
],

This is the new approach -> Use more, but smaller, more focused JSON-LD contexts, instead of a single huge JSON-LD context that defines everything.

@peacekeeper
Copy link

Looks good I think. I tried to implement this in our did:sov driver of the Universal Resolver.

E.g. try this: https://dev.uniresolver.io/#did:sov:HR6vs6GEZ8rHaVgjg2WodM

Or:

curl -H "Accept: application/did+ld+json" -X GET https://dev.uniresolver.io/1.0/identifiers/did:sov:HR6vs6GEZ8rHaVgjg2WodM

@swcurran
Copy link
Contributor Author

I think this is ready to go. Recent changes:

  • Update the READ example to have the default (plain) endpoint with just the two service blocks
  • Update the list of contexts to use, per @peacekeeper 's suggestion.
  • Add note about the need to add the DIDComm service block context
  • Changed the recipientKey reference to be to the keyagreement key -- against @TimoGlastra 's advice
    • Timo -- it seems to be just wrong to define a key agreement key and not reference it when it should be used...

Barring any complaints, we'll merge this Real Soon Now.

@WadeBarnes WadeBarnes merged commit e5209c2 into sovrin-foundation:master Jun 24, 2021
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

Successfully merging this pull request may close these issues.

7 participants