feat: add validation for public key #2841
Conversation
Signed-off-by: Ivaylo Nikolov <ivaylo.nikolov@limechain.tech>
| it("should throw error when creating ECDSA key from invalid bytes", function () { | ||
| // Test cases with invalid byte | ||
| const invalidPrivateKeyBytes = Uint8Array.from([ | ||
| 0x00, 0xca, 0x35, 0x4b, 0x7c, 0xf4, 0x87, 0xd1, 0xbc, 0x43, 0x5a, |
There was a problem hiding this comment.
Are we sure that padding the zero byte makes the key invalid?
There was a problem hiding this comment.
I copied the invalid bytes from the Java SDK tests. I think the first byte checks if the key is compressed or not (0x02 or 0x03). Therefore 0x00 making it invalid.
There was a problem hiding this comment.
It is really a question of convention:
048ae371ee33535a3fa40e31c20485cd8bf869c151184c56cae2775b0156eb3a4b29421847d57caf98b271ab954a42bfb1bed565a9c238b2f7b30274cbd793a163 - also a valid key.
The bad thing about conventions around this topic is that many would trim the 0x02, 0x03 prefix or the 0x04 prefix. So to do a fully invalid key check i would go with bytes which I know for sure a wrong like 1+ the maximum value of the curve which would be N.
There was a problem hiding this comment.
This way you can validate the point verification function and will be really useful.
| * @returns {boolean} | ||
| */ | ||
| static isValid(data) { | ||
| if (!data) return false; |
There was a problem hiding this comment.
Does this check if data is empty? If it does would make it more verbose.
There was a problem hiding this comment.
Also if you have this here why not have it in the other public key class as well?
| const EMPTY_BUFFER_33_BYTES = Buffer.from(new Uint8Array(33).fill(0)); | ||
| const EMPTY_BUFFER_65_BYTES = Buffer.from(new Uint8Array(65).fill(0)); |
There was a problem hiding this comment.
Do you need those buffers in memory to do the following checks?
There was a problem hiding this comment.
Could you elaborate a little more?
There was a problem hiding this comment.
I mean do you need to allocate those buffers and save them in variables to do the checks? Isn't there some sort of faster check for this without having EMPTY_BUFFER_33_BYTES, EMPTY_BUFFER_65_BYTES
| const EMPTY_BUFFER = Buffer.from(new Uint8Array(33).fill(0)); | ||
| const bufferData = Buffer.from(data); | ||
| if (bufferData.equals(EMPTY_BUFFER)) return true; |
There was a problem hiding this comment.
Seem like some redundant operations to me.
There was a problem hiding this comment.
if (Buffer.from(data).equals(Buffer.alloc(33))) return true;
| ed25519.ExtendedPoint.fromHex(data); | ||
| return true; | ||
| } catch { | ||
| return false; |
There was a problem hiding this comment.
If an error is caught wouldn't it make sense to propagate it.
There was a problem hiding this comment.
I think considering the method's name is isValidKey you would expect it to return true or false.
There was a problem hiding this comment.
throw new BadKeyError("invalid public key"); is thrown in another layer. The message of the error for the key parse would be useful there imho. I know you've defined it so but that error could get up there somehow
There was a problem hiding this comment.
Since this method is created in this PR, we can ditch it and do the validation in the fromBytesRaw func. There we can throw new BadKeyError("invalid public key: ", e);, giving more info to the user as Georgi said.
Description:
Add validation to
PublicKeys by installing a new NPM package that checks if the public key is on the curve.Related issue(s):
#2728
Notes for reviewer:
Checklist