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

Add more detail to DID Authentication section. #18

Merged
merged 8 commits into from Aug 14, 2022
Merged

Conversation

msporny
Copy link
Contributor

@msporny msporny commented Aug 7, 2022

This PR adds more detail around the DID Authentication section, including adding the didMethod and cryptosuite values.


Preview | Diff

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Thanks!

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated
listed, the value MUST be an array of string values and each string value
MUST be a string containing the DID Method name. Listing multiple values
expresses that the <a>verifier</a> would accept any DID Method listed. Valid
example values include: <code>"key"</code> and <code>["key", "web"]</code>.
Copy link
Contributor

@dlongley dlongley Aug 7, 2022

Choose a reason for hiding this comment

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

I would support making this always be an array instead to ease some concerns from other implementers around native type handling in some languages. I'm fine either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the open/decentralized ecosystem, I believe the common pattern will be for Verifiers to accept multiple types of DID methods, so +1 to always having this as an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll make this change and merge it into this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

might it make sense to make the didMethod array contain objects, instead of just strings? There's a very good chance that various DID methods require multiple parameters for a given method. (For example, how Veres One DID method has a testnet vs main, cryptonyms vs not, etc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this on the 2022-08-09 telecon. The group seemed to have consensus around the didMethod value always being an array. The group didn't object to renaming didMethod to acceptedMethods to ensure that the contents of the array is always a set of objects. Each object MUST contain at least an entry for method and MAY contain other parameters that are specific to the DID Method.

Copy link
Contributor Author

@msporny msporny Aug 14, 2022

Choose a reason for hiding this comment

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

Fixed in d939bad.

index.html Outdated Show resolved Hide resolved
index.html Outdated
An optional string value that conveys a cryptography suite that MUST be
used by the <a>holder</a> to generate a cryptographic proof. Valid example
values include: <code>"eddsa-2022"</code>, <code>"ecdsa-2022"</code> and
<code>"bbs-2022"</code>.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only a single option? Wouldn't verifiers support multiple crypto suites?

Copy link
Contributor

Choose a reason for hiding this comment

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

nvm, seen below that a request may contain multiple DIDAuth requests.

Copy link
Contributor Author

@msporny msporny Aug 8, 2022

Choose a reason for hiding this comment

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

Yeah, I went back and forth on this. There is a combinatoric concern here with listing all the DID Methods and all the cryptosuites, making it an NxN selection, which then gets more multidimensional if we have other "possible things you could do". I'm suggesting that we start with a single value and then depend on the ability to send multiple queries (some of them optional) to suggest the support for multiple cryptosuites. The downside there, of course, is if you support multiple cryptosuites, you have a new query for every cryptosuite. I expect that we might just have to make this an array in time, and if that's the case, we should just do it now... but the counter-argument to that is to make it painful to support more than a handful of cryptosuites to force an increase in interop!

In the end, I expect that we'll probably have to support an array of cryptosuites here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just start with an array of cryptosuites? To match the array of did methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We discussed this on the 2022-08-09 telecon. @dlongley suggested that we rename the value to acceptedCryptosuites and make the value an array of objects where we could parameterize each cryptosuite. Each object MUST contain a cryptosuite property and MAY contain other cryptosuite-specific parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e7c4a51.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Show resolved Hide resolved
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
Co-authored-by: Mike Varley <mavarley@gmail.com>
@dmitrizagidulin
Copy link
Contributor

Should we add a (temporary) warning that the VC v2 context doesn't exist yet, and that this is forward-looking?

Copy link
Contributor

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

👍 from me (though see my comment about didMethod, cryptosuite and the context warning)

index.html Outdated Show resolved Hide resolved
index.html Outdated
supported by the <a>verifier</a>. If only one DID Method is listed, the value
MUST be a string containing the DID Method name. If more than one DID Method is
listed, the value MUST be an array of string values and each string value
MUST be a string containing the DID Method name. Listing multiple values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MUST be a string containing the DID Method name. Listing multiple values
MUST be a string containing a DID Method name. Listing multiple values

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@David-Chadwick
Copy link

David-Chadwick commented Aug 9, 2022

Here is how OIDC4VCI provides proof of possession of the DID before issuing the VC to that did. It requires the proof type parameter and optionally the proof parameter. Currently only the jwt proof type is specified, and it must contain the iss, aud, iat and nonce (or possibly jti instead) claims. Here is an example taken from the spec

"proof":"{
  "proof_type": "jwt",
  "jwt": "eyJraWQiOiJkaWQ6ZXhhbXBsZTplYmZlYjFmNzEyZWJjNmYxYzI3NmUxMmVjMjEva2V5cy8
  xIiwiYWxnIjoiRVMyNTYiLCJ0eXAiOiJKV1QifQ.eyJpc3MiOiJzNkJoZFJrcXQzIiwiYXVkIjoiaHR
  0cHM6Ly9zZXJ2ZXIuZXhhbXBsZS5jb20iLCJpYXQiOiIyMDE4LTA5LTE0VDIxOjE5OjEwWiIsIm5vbm
  NlIjoidFppZ25zbkZicCJ9.ewdkIkPV50iOeBUqMXCC_aZKPxgihac0aW9EkL1nOzM"
}

Where the jwt unencoded and without the signature comprises

{
  "alg": "ES256",
  "typ": "JWT",
  "kid":"did:example:ebfeb1f712ebc6f1c276e12ec21/keys/1"
}.
{
  "iss": "s6BhdRkqt3",
  "aud": "https://server.example.com",
  "iat": 1659145924,
  "nonce": "tZignsnFbp"
}

@msporny
Copy link
Contributor Author

msporny commented Aug 14, 2022

Multiple reviews, changes requested and made, no objections in 7 days, merging.

@msporny msporny merged commit c9a4fe9 into main Aug 14, 2022
@msporny msporny deleted the msporny-didauth-detail branch August 14, 2022 19:13
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.

None yet

6 participants