Prerequisite changes for encrypted cloud backup SDK #9154
Conversation
…/celo-monorepo into victor/encrypted-backup-library
…r/encrypted-backup-library
…lo/phone-number-privacy-common
nategraf
requested review from
asaj,
gastonponti,
mcortesi and
timmoreton
as code owners
nategraf
requested review from
a team,
alecps and
codyborn
and removed request for
a team,
timmoreton,
mcortesi,
gastonponti and
asaj
| const bodyString = JSON.stringify(body) | ||
|
|
||
| let authHeader = '' | ||
| // Sign payload using provided account and authentication method. | ||
| // NOTE: Signing and verifying signatures over JSON encoded blobs relies on both the serializer |
There was a problem hiding this comment.
Great point. Would a better approach be to parse out the fields and sign them according to a function exposed in phone-number-privacy-common?
There was a problem hiding this comment.
So the main result of this is that anyone who wants to verify the signature over this data must not decode and re-encode it (i.e. they need to save and pass along the original encoded string). This is a situation that seems to be pretty common, including in standards like JWTs, and is not deadly but is unfortunate. I get the sense from what I've read on this topic that its not uncommon for production systems to simply accept the requirement of passing around the originally encoded string. (I was actually talking to my friend this morning about a AWS Sigv4 which has this issue to an extent)
One solution that gets deployed is deterministic encoding rules which are restricted versions of the original encoding rules. Examples include ASN.1 DER which is a subset of BER and is used in certificates, OJSON, and CBOR core deterministic encoding requirements.
In CIP-40, the solution I chose is EIP-712 which provides deterministic "encoding" specifically for the purposes of hashing and signing. With that, the idea is that you can encode, decode and pass around the data to your hearts content and ensure that when its reassembled you will get the same hash. (It also ensures domain seperation to prevent the tail-risk of some colliding message format)
I wouldn't rush to change anything, although if we face any problem with this we may consider using EIP-712 in the future.
There was a problem hiding this comment.
Awesome! Thanks for the detailed explanation. Using eip712 makes a lot of sense
…o-org/celo-monorepo into victor/encrypted-backup-library-base
Description
A number of prerequisite changes to the encrypted backup SDK implemented in #8896
@celo/identityand@celo/phone-number-privacy-commonpackages to make the former depend on the latter. Removed duplicate request and response definitions in@celo/identity.@celo/identity/lib/odis/queryto support aNONEauthentication option for POPRF queries and to return an augmented error in case fetch throws.rootLoggerin@celo/phone-number-privacy-commonto be a lazily initialized via a function to avoid import errors when used in other packages.io-tsschemas forDomaintypes to allow for serialization into backups.notBeforeoutput to theSequentialDelayDomainrate limiting predicate to allow for telling the client when they can make their next request.disabledtoSequentialDelayDomainStateand a check in the predicate to respect that state field.NO_GANACHEenvironment variable flag to the@celo/identitytest setup code to more quickly run tests that do not require a mock blockchain.Tested
Covered by existing tests
Related issues
Backwards compatibility
No concerns