Implemented manifest signatures #27
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
taoeffect
reviewed
Feb 9, 2024
src/keygen.ts
Outdated
Comment on lines
5
to
20
| export const keygen = async (args: string[]) => { | ||
| await revokeNet() | ||
| const parsedArgs = flags.parse(args) | ||
| const key = cryptoKeygen(EDWARDS25519SHA512BATCH) | ||
| const textEncoder = new TextEncoder() | ||
| const result = textEncoder.encode(JSON.stringify({ | ||
| version: '1.0.0', | ||
| pubkey: serializeKey(key, false), | ||
| privkey: serializeKey(key, true) | ||
| })) | ||
|
|
||
| if (parsedArgs['out']) { | ||
| await Deno.writeFile(parsedArgs['out'], result) | ||
| } else { | ||
| await Deno.stdout.write(result) | ||
| } |
There was a problem hiding this comment.
Request:
- output 2 files, one for the public key (for verifying with
chel latestState), and one for the private key for signing withchel manifest(like SSH) - remove the STDOUT. you can have a default filename based on the key type (make sure to output to STDOUT where this file is saved, and check to see if it already exists, in which case, just add a suffix like
id_edssa.1.pubandid_edssa.1.priv) or something to that effect
taoeffect
requested changes
Feb 9, 2024
There was a problem hiding this comment.
Great work @corrideat!!
I left some feedback and also realized we need two tests for this:
- A test to verify that contracts are signed correctly using a new
chel verifyContractcommand that takes a pubkey file and a manifest (can update one of the manifest files intest/data/) - A test to verify that a modified contract (after it's been signed) fails a signature check (can use the
test/data/mailbox.*files for this one)
* New verifySignature command * Tests for validating signatures * keygen command now outputs to two files * manifest command now uses public key files instead of strings
taoeffect
reviewed
Feb 9, 2024
src/keygen.ts
Outdated
Comment on lines
24
to
28
| await Deno.writeTextFile(outFile, result) | ||
| console.log(colors.green('wrote:'), outFile, colors.green('(secret)')) | ||
|
|
||
| await Deno.writeTextFile(pubOutFile, pubResult) | ||
| console.log(colors.green('wrote:'), pubOutFile, colors.green('(public)')) |
There was a problem hiding this comment.
Can you make the color for "(secret)" and "(public)" blue or grey?
taoeffect
reviewed
Feb 9, 2024
src/verifySignature.ts
Outdated
Comment on lines
39
to
49
| if (!internal) console.log(colors.green('signature ok'), '(verification not yet complete)') | ||
|
|
||
| const body = JSON.parse(manifest.body) | ||
| if (!Array.isArray(body.signingKeys)) throw new Error('Missing signingKeys in manifest') | ||
| if (body.signingKeys.every((k: string) => { | ||
| return keyId(k) !== id | ||
| })) { | ||
| throw new Error('The signing key is not listed in signingKeys') | ||
| } | ||
|
|
||
| if (!internal) console.log(colors.green('ok'), 'all checks passed') |
There was a problem hiding this comment.
- If the keyfile isn't specified in the arguments, see if the signature passes one of the keys in the manifest
- If the keyfile is specified, check if the signature passes that key only, and IF that key is not also specified in the manifest, then output a yellow bold
console.warnmessage saying that the check passed the keyfile but that key wasn't found on in the manifest- alternatively, you can output a red console.error saying the signature is OK but the key isn't in the manifest
- For errors that are not unexpected (as many of the ones are here), use the
exit()function fromutils.ts
In all cases only output one message. If the output is an error then call Deno.exit(1)
taoeffect
reviewed
Feb 9, 2024
src/manifest.ts
Outdated
| key: "<which of the 'authors' keys was used to sign 'body'>", | ||
| signature: "<signature>" | ||
| keyId: keyId(signingKey), | ||
| value: sign(signingKey, serializedBody + head.manifestVersion) |
taoeffect
reviewed
Feb 9, 2024
| } | ||
| if (!manifest.signature.value) { | ||
| exit('Invalid manifest: missing signature value', internal) | ||
| } |
There was a problem hiding this comment.
We also need to load both contract files (slim — if it exists — and bundle) and verify the hash matches the hash that's in the manifest.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes issue #26