Closed RangerMauve closed 5 years ago
I'm 👍 to this, @mafintosh is this correct?
LGTM
You forget to use hash
in the verify function :) Personally I'd also drop the detached
part which is somewhat of a libsodium-ism, but maybe it adds clarity
@emilbayes Is that better? :D
~@RangerMauve Sorry, meant on the verify function :) You cannot verify a signature without the original message and the associated public key~
Sorry, looked at the wrong diff! It is ok to calculate the hash inside the function like you did before, but I don't know if that's what happens in practise in hypercore
I think it doesn't matter too much in this case, actually. I think people can infer what's up by seeing how it's the hash from the roots that's being signed.
(This comment isn't a block or veto of the PR, just a suggestion)
My preference would be to use english or pseudo-code rather than Javascript, despite the existing Javascript examples. My reasoning is we don't want to enshrine particular quirks of Javascript libraries or syntax "as it is today" by accident. For instance, is the "detatch" bit an official recommendation? If not, we should clarify, or better, just not include any specific implementation. The existing code snippets were included to demonstrate routines which were hard to explain in English, which I don't think is the case here.
The existing "Root hash signatures" section currently says:
Any time a new root hash is generated, it is signed using the private key. This signature is distributed with the root hash to provide verification of its integrity.
Under "Hash and signature functions":
The signatures are ed25519 with the sha-512 hash function.
@RangerMauve were those the unclear parts? Could we clarify those paragraphs instead of adding code?
The comment about "ed25519 with the sha-512 hash function" I think we could replace with something more specific such as the EdDSA RFC, which is also why I suggested dropping the "detached" part: https://tools.ietf.org/html/rfc8032 (though this doesn't help in understanding what actually is signed). Detached here is just because libsodium appends the Ed25519 signature by default, which is not a specification requirement but because libsodium is designed to be easy to use :)
@bnewbold
I don't think the paragraphs need to be changed, but I still believe that adding the sign and verify functions is useful because with it you can see all the crypto-related pieces in one place which I think will be useful for implementors.
Regarding the detach
part, I think it's necessary, because as @emilbayes mentioned, libsodium appends the signature to the content by default instead of just returning the signature. The signature is sent separately, so I think it's important to note that. I think that it's safe to assume that implementations are likely to use libsodium as it's what's being used under the hood already.
Regarding using code other than JavaScript, what do you have in mind? The code as it stands doesn't use any javascript-specific features. I think that adding type signatures would be useful, however.
Thanks for your patience @RangerMauve !
Closes #39
It wasn't instantly obvious that the hash of the roots was being signed when looking at the crypto functions, so I added the
sign
andverify
psudocode to make it more clear for new implementations.