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

Remove proofValue MUST be a detached ECDSA normative statement #15

Closed
JSAssassin opened this issue Jul 9, 2023 · 6 comments
Closed
Assignees
Labels
before CR This issue needs to be processed before Candidate Recommendation

Comments

@JSAssassin
Copy link

https://github.com/w3c/vc-di-ecdsa/blob/main/index.html#L409-L410

@TallTed
Copy link
Member

TallTed commented Jul 10, 2023

I think this is what you're referring to --

          <p>
The `proofValue` property of the proof MUST be a detached ECDSA
produced according to [[FIPS-186-5]], encoded according to [[MULTIBASE]] using
the base58-btc base encoding.
          </p>

Are you asking that this entire paragraph be deleted? Can you provide a reason for this?

@JSAssassin
Copy link
Author

@TallTed, not the entire paragraph but the statement "The proofValue property of the proof MUST be a detached ECDSA produced according to [[FIPS-186-5]]". @msporny suggested raising this issue since he thinks that the statement is probably unnecessary.

@TallTed
Copy link
Member

TallTed commented Jul 11, 2023

@JSAssassin — So are you suggesting that the paragraph should be reduced to the following, by deleting the entire "statement" you've quoted?

          <p>
encoded according to [[MULTIBASE]] using the base58-btc base encoding.
          </p>

Or the following, which removes only the phrase, a detached ECDSA produced according to [[FIPS-186-5]], which appears to be the salient portion of that quoted "statement"?

          <p>
The `proofValue` property of the proof MUST be
encoded according to [[MULTIBASE]] using the base58-btc base encoding.
          </p>

@JSAssassin
Copy link
Author

@TallTed Sorry for the confusion. The second option is what I am suggesting. The phrase a detached ECDSA produced according to [[FIPS-186-5]] can be removed, leaving the paragraph with the text:
The proofValue property of the proof MUST be encoded according to [[MULTIBASE]] using the base58-btc base encoding.

@dlongley
Copy link
Contributor

dlongley commented Jul 11, 2023

Perhaps the statement should just remove "detached":

The proofValue property of the proof MUST be an ECDSA signature
produced according to [[FIPS-186-5]], encoded according to [[MULTIBASE]] using
the base58-btc base encoding.

This may also be insufficient since FIPS-186-5 just says to output the r and s components. What we really want to say (if we want to be extra clear), is that the IEEE P1363 format is used for the signature and then it is encoded using the multibase base58-btc base encoding. We can perhaps reuse some language or references mentioned in this MDN docs article around producing an ECDSA signature using Web Crypto, which uses the same (and most commonly used) format as we do here:

https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/sign#ecdsa

We could also leave this to tests to confirm if that's more appropriate. Here's all the Web Crypto spec has to say: https://www.w3.org/TR/WebCryptoAPI/#ecdsa-description

As another example, the http signatures talks about the concatenation of r and s here:
https://www.ietf.org/archive/id/draft-ietf-httpbis-message-signatures-17.html#section-3.3.4

The signature algorithm returns two integer values, r and s. These are both encoded as big-endian unsigned integers, zero-padded to 32-octets each. These encoded values are concatenated into a single 64-octet array consisting of the encoded value of r followed by the encoded value of s.

@msporny msporny added before CR This issue needs to be processed before Candidate Recommendation ready for pr This issue is ready for a Pull Request to be created to resolve it labels Jul 17, 2023
@msporny
Copy link
Member

msporny commented Jul 30, 2023

PR #18 has been merged to address this issue. Closing.

@msporny msporny removed the ready for pr This issue is ready for a Pull Request to be created to resolve it label Jul 30, 2023
@msporny msporny closed this as completed Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before CR This issue needs to be processed before Candidate Recommendation
Projects
None yet
Development

No branches or pull requests

5 participants