search

interledger-deprecated / five-bells-shared

Common elements that are shared between Five Bells components
Other
4 stars 5 forks source link

BREAKING: Json signature verifications returns false on invalid json #127

Closed alandotcom closed 8 years ago

alandotcom commented 8 years ago

Returning false avoids having to wrap the verify call in a try/catch everywhere it's used. For example, if a Connector receives an unsigned notification

alandotcom commented 8 years ago

cc @naoitoi @MatthewPhinney @gip

MatthewPhinney commented 8 years ago

I think this generally looks good, but we conflate "json without signature" and "json with invalid signature" (since both return false). I think these two cases are important to distinguish between. A clearer API might be something like

verify = function (json, signature, pubKey)

I'm fine with this change, provided the connenctor logs the json that failed to validate, so we can pinpoint if it was due to an incorrect signature, or a missing signature.

clark800 commented 8 years ago

LGTM

alandotcom commented 8 years ago

@clark800 @MatthewPhinney I updated this so that verify returns an object with some information rather than true/false. The problem is the underlying libraries throw exceptions on anything that's invalid (include invalid public key or public key mismatch) which isn't useful to a caller of this method

alandotcom commented 8 years ago

but we conflate "json without signature" and "json with invalid signature" (since both return false)

The caller shouldn't have to care about the shape of the object or know that a signed JSON object contains a signature at the top level key signature. Imagine if we wanted to change from JCS to some other signing implementation that created a signed object with a different shape? The caller code would have to change.

@MatthewPhinney

alandotcom commented 8 years ago

There's some duplication in the unit tests, but I'd prefer to clean that up in a later commit

alandotcom commented 8 years ago

@clark800 can you look at the changes from the latest commit?

clark800 commented 8 years ago

latest changes LGTM

MatthewPhinney commented 8 years ago

The caller shouldn't have to care about the shape of the object or know that a signed JSON object contains a signature at the top level key signature.

I wasn't trying to argue that the caller should need to understand how the json is structured, but rather that we may want more granular error messages, in the case of failed validations. Looks like 4c28bd0c149a03fc18adf52d0f7e148ee5661173 addresses this, so LGTM.