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 "challenge","domain","nonce" to LinkedDataProof. #249

Merged
merged 2 commits into from
Dec 4, 2021

Conversation

peacekeeper
Copy link
Member

Addresses #194.

@peacekeeper
Copy link
Member Author

^ @vdods

Copy link
Contributor

@msporny msporny left a comment

Choose a reason for hiding this comment

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

I think we're going to have to remove challenge?

Comment on lines 25 to 27
challenge:
type: string
description: A challenge to mitigate replay attacks.
Copy link
Contributor

@msporny msporny Nov 12, 2021

Choose a reason for hiding this comment

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

Hrm, on second thought -- I forget what we're doing here presently... don't we just include the challenge in the nonce wrt. CHAPI presentation? We don't encode challenge and nonce separately, do we? Although, it feels like the former could lead to an attack... so, perhaps having them separate is the right thing to do?

I'll note that we definitely don't define challenge in the latest 2020 contexts:

https://github.com/digitalbazaar/ed25519-signature-2020-context/blob/master/contexts/ed25519-signature-2020-v1.jsonld

/cc @dlongley @dmitrizagidulin

Copy link
Member Author

Choose a reason for hiding this comment

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

I admit I don't know exactly how those properties are currently used in CHAPI, DIDComm, OIDC, etc.

But challenge appears here in the VC data model spec:

https://www.w3.org/TR/vc-data-model/#example-a-simple-example-of-a-verifiable-presentation

Choose a reason for hiding this comment

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

@msporny - we use challenge in DIDAuth, to prevent replay attacks. The 2020 contexts don't define it because it's only really used for VPs, and it's defined in the VC v1 context.
We don't use nonce anywhere currently; I think it's left in for some sort of backwards compat purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 2020 contexts don't define it because it's only really used for VPs, and it's defined in the VC v1 context.

Ah, ok, so that'll become a problem when we pull all the crypto stuff out of the VC v2 context.

But challenge appears here in the VC data model spec

hmm, right, will need @dlongley to weigh in.

We don't use nonce anywhere currently; I think it's left in for some sort of backwards compat purposes.

For a long time, we depended on the datetime on the proof to establish a pseudo-nonce. If you need to do signatures faster than every second depended on nonce existing (if you wanted to prevent replays within a second time scale). So, I guess the question is whether or not we want to remove challenge and just use nonce... but if we were to do that, how is a Verifier requesting a VP with a specific challenge supposed to encode it. The answer is using challenge, I imagine. So, maybe this is an argument to NOT remove `challenge from the VC v2 context?

The only other viable alternative I can think of is getting rid of challenge and using nonce everywhere. The downside there is that challenge and nonce have slightly different semantic connotations. Challenge is something someone else gives you as a part of an authentication ceremony (to ensure an authn isn't being replayed). Nonce is something you generate and embed to ensure your signatures can't be replayed... and might be used outside of authentication ceremonies.

All that to say, I think we're good and both challenge and nonce should be allowed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I talked about nonce and challenge more here: https://github.com/w3c/vc-data-model/issues/823#issuecomment-930257979

In short, challenge is what is used for challenge-based protocols because it's a more specific, better word for it. We've been using challenge in VP proofs for quite a long time now, nonce has fallen out of favor for that purpose for years but it still has utility for things like ensuring a proof for a derived credential is properly randomized, etc.

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 right there.

Gah, I swear I CTRL-F searched for it and nothing came up... thx @dlongley -- it's there, and it's clear that we can merge this PR now (confirmation that challenge is defined, and nonce is still useful).

components/LinkedDataProof.yml Outdated Show resolved Hide resolved
components/LinkedDataProof.yml Outdated Show resolved Hide resolved
Co-authored-by: Dave Longley <dlongley@digitalbazaar.com>
@OR13
Copy link
Contributor

OR13 commented Nov 16, 2021

really there are 2 types of proofs... one for VCs that proofPurpose: assertionMethod and one for VPs that contain proofPurpose: authentication and challenge and optionally domain... There ought to be a schema for each imo.

@msporny msporny merged commit 5b09b29 into main Dec 4, 2021
@msporny
Copy link
Contributor

msporny commented Dec 4, 2021

Normative, multiple reviews, changes requested and made, no objections, merging.

@msporny msporny deleted the peacekeeper-ldp-challenge-domain-nonce branch December 4, 2021 19:11
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.

6 participants