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 nonce option to DeriveCredential API #144

Merged
merged 1 commit into from Mar 11, 2021
Merged

add nonce option to DeriveCredential API #144

merged 1 commit into from Mar 11, 2021

Conversation

troyronda
Copy link
Contributor

No description provided.

properties:
nonce:
type: string
description: A nonce provided by the holder of the credential to be included into the LinkedDataProof. For example 6e62f66e-67de-11eb-b490-ef3eeefa55f2
Copy link
Contributor Author

@troyronda troyronda Mar 8, 2021

Choose a reason for hiding this comment

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

If we want to be able to generate a nonce, we might need a bit more nuance in this description or another option.

I tend to think we should just have the client pass in the nonce.

Note: I kept this description similar to the challenge option.

Copy link
Contributor Author

@troyronda troyronda Mar 8, 2021

Choose a reason for hiding this comment

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

To make the default less ambiguous, we can either state that the default is to generate a nonce or that the default is empty nonce.

In the case where the default is a generated nonce, we would also want to specify the behavior of an empty nonce option being provided.

Note: the BBS+ Signatures spec currently states:

A linked data document featuring a BBS+ proof of knowledge linked data proof MAY contain a nonce attribute.

Copy link
Collaborator

Choose a reason for hiding this comment

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

another example of harmful MAY... why is this not a MUST and be of min length ... etc... @tplooker can we make this a MUST ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

description: A nonce provided by the holder of the credential to be included into the LinkedDataProof. For example 6e62f66e-67de-11eb-b490-ef3eeefa55f2
example:
{
"nonce": "d436f0c8-fbd9-4e48-bbb2-55fc5d0920a8",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we need to include the nonce bytes into the proof, perhaps we should specify that the nonce option is base64 encoded. @tplooker @OR13

Copy link
Contributor

@msporny msporny Mar 9, 2021

Choose a reason for hiding this comment

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

As a general rule these days, if you catch your self picking a base-encoding format, you're probably wrong. :)

A few things to keep in mind:

  1. Multibase exists, if you want to base-encode something, we should use multibase for the value.
  2. Protocols can limit the type of base-encoding used (e.g. base58btc only). It's the easiest way to not get locked into a base encoding that doesn't work for some subset of the community.
  3. The nonce value should never be optional and never be "default empty" in a protocol that is susceptible to replays. I think I saw something to this effect in a PR that flew through my inbox... just noting that it'll be a hard objection from us if folks want to default to empty nonces.

That said, the nonce value being encoded as pure bytes really doesn't matter... don't think we need to base-encode it... you just need enough randomness to prevent replays... and you can do that w/ just UUIDs or ASCII characters. If folks have a "but I want to run this protocol over a compressed binary stream" use case, we could say that nonce needs to be multibase encoded in /this particular protocol/.

Copy link
Contributor Author

@troyronda troyronda Mar 9, 2021

Choose a reason for hiding this comment

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

@msporny

Multibase exists, if you want to base-encode something, we should use multibase for the value.

The context for mentioning base64 (or encoding, in general) comes from the BBS+ Signatures implementations and spec examples using it. As the nonce output uses encoded bytes, I was curious if the input should be encoded so they end up being the same.

I agree that multibase is good.

The nonce value should never be optional and never be "default empty" in a protocol that is susceptible to replays.

Yup. In this case the output is a derivation of the VC (and is, itself a VC), but I would imagine the common intention is to use the derived VC once. @OR13 has opened a PR on the BBS+ Signatures spec regarding the MAY vs MUST nonce topic (w3c/vc-di-bbs#43).

That said, the nonce value being encoded as pure bytes really doesn't matter

Note: the holder and the verifier both include the pure bytes into their proof computation. The output BBS+ LD Proof nonce property uses encoded bytes.

Example from BBS+ Signatures spec:

"nonce": "6i3dTz5yFfWJ8zgsamuyZa4yAHPm75tUOOXddR6krCvCYk77sbCOuEVcdBCDd/l6tIY="

Of course, the derive service can transform from the input to bytes for the proof computation and do the output encoding for the nonce property. (and, as currently described in this PR: this is what would happen).

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tplooker I agree with this thread, and am bothered by the base64 use in that suite :) we should move to multibase in the next version.

@OR13
Copy link
Collaborator

OR13 commented Mar 11, 2021

This is spec only, and limited support for it in the test suite, propose we merge before cut, since its already an optional API.

@OR13 OR13 merged commit 016cde2 into w3c-ccg:master Mar 11, 2021
msporny pushed a commit that referenced this pull request Jul 25, 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.

None yet

5 participants