[wip] Return a frozen array of bytes when producing keypairs #248
Conversation
|
Important: It only partially addresses it, many cases are still left w/o solution. Quite a few libraries re-calculate public keys. There is no issue in doing that, instead of half-solutions. |
|
Hey @steveluscher, nice to see a fellow Solana developer here! I guess this repo has been getting lots of eyes on it lately after the whole Slope wallet private key hack. It definitely seems like freezing these byte arrays is a smart choice. I'll definitely take a look at this code when I get back to my laptop in a few hours! |
| return Object.defineProperties({}, { | ||
| publicKey: { | ||
| get: function() { | ||
| if (pkArray === undefined) { | ||
| pkArray = Object.freeze(Array.from(pk)); | ||
| } | ||
| return pkArray; | ||
| } | ||
| }, | ||
| secretKey: { | ||
| get: function() { | ||
| if (skArray === undefined) { | ||
| skArray = Object.freeze(Array.from(sk)); | ||
| } | ||
| return skArray; | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This produces an object {publicKey: [...], secretKey: [...]} that can't be mutated.
| if (typeof value !== 'number' || value < 0 || value > 255) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Optional assertion that the array only contains numeric values between 0–255.
| for (var i = 0; i < arguments.length; i++) { | ||
| if (!(arguments[i] instanceof Uint8Array)) | ||
| throw new TypeError('unexpected type, use Uint8Array'); | ||
| if (arguments[i] instanceof Object && typeof arguments[i].length === 'number') { |
There was a problem hiding this comment.
All that the implementation demands of the input is that:
- it be indexable
- it have a
lengthproperty
You don't have to be a Uint8Array to offer those two guarantees.
| type PublicKey = ReadonlyArray<EightBitNumber> & { readonly [tag]: 'PublicKey' } | ||
| type SecretKey = ReadonlyArray<EightBitNumber> & { readonly [tag]: 'SecretKey' } |
There was a problem hiding this comment.
Opaque types! Read more here: https://stackoverflow.com/a/56749647/802047
Preamble
Discussion
Overspecification
It's a common thing to see an API overspecified. This means that its input types overstate what the implementation needs to function. APIs like
nacl.sign.detached, for instance, ask that you bring aUint8Arraywhen really all the implementation requires is:thing[1])lengthproperty that is anumbertweetnacldoesn't need any of the other features offered by theUint8Arrayinterface, like.every()or.byteLengthor any of that. The only other thing offered by that type is a guarantee that each element is a number between 0–255, but that can be either ignored (signatures will be junk otherwise) or validated at runtime.Frozen objects
Uint8Arraycan not be frozen byObject.freezewhich is useful to prevent runtime mutation. Now that the inputs tonacl.sign.detachedare specified just enough, we can actually return plain arrays that can be frozen. This is a runtime protection against the values being mutated.Opaque/branded types
Next, we can apply a type system level protection against the values being mutated.
The idea here is that the only value for
secretKeythat can be supplied tonacl.sign.detachedwas something that was produced bynacl.sign.keyPair.*. If you try to pass an array that you constructed outside ofnacl.sign.keyPair, it will fail to typecheck.Also, if you try to mutate the value that you received from
nacl.sign.keyPair, it will fail to typecheck.Benchmarks
Related to #247.