Ensure that all handles and ciphertexts are signed by the co-processor

[jot2re]

Is your feature request related to a problem? Please describe. When we return a ciphertext from the coprocessor we also need to include a signature on the handle and the ciphertext digest. This is needed for reencryption in order for the fhevm-js client to know that the decryption has happened of the correct ciphertext. This is because the handle is just symbolic and hence not linked directly to a ciphertext in the view of the KMS Core, thus a signature on the link is needed by the coprocessor.

Describe the solution you'd like For now it will likely be the simplest to simply sign this in an EIP712 with a dummy (all zero address) on-demand, rather than changing the database storage.

Additional context This is not strictly needed for the Q3 release, but should be fixed soon after as it is needed to avoid trusting the gateway for correctness in reencryption

[dartdart26]

I will work on it. We can merge it before release or later. It would essentially just add a new bytes signature field in the FetchedCiphertext message in the coprocessor.proto file:

message FetchedCiphertext {
  bytes ciphertext_bytes = 1;
  int32 ciphertext_version = 2;
  int32 ciphertext_type = 3;
  bytes signature = 4;
}

I'd like to sync on the signature format. I am suggesting the following.

We would sign this Solidity data structure:

alloy::sol! {
    struct GetCiphertextResponseignatureData {
        uint256 handle;
        bytes ciphertext;
    }
}

Note that the ciphertext would be hashed as part of signing the EIP712 struct, so I think we don't need to change it to a digest ourselves.

The EIP712 domain would be:

        let eip712_domain = alloy::sol_types::eip712_domain! {
            name: "GetCiphertextResponse",
            version: "1",
            chain_id: chain_id as u64,
        };

Meaning we won't specify any other fields such as verifying_contract and so on, they would be set to None.

Please let me know if that looks good.

[mortendahl]

@dartdart26 but do you need the ciphertext to verify the signature? We will need to verify the signature in the ASC for the coprocessor, where we don't have the full ciphertext.

[dartdart26]

@mortendahl Oh, I thought this is not happening on the ASC, but instead in the core part. If it is happening on the ASC, we can change the ciphertext to a ciphertext_digest, that's what you mean?

What would the core do if the signature matched on the ASC, but the ciphertext is bad? I guess that is equivalent to the core checking the signature.

What is the benefit of checking it on the ASC?

[jot2re]

I agree with @mortendahl that it is better design to have the signature on the digest of the ciphertext rather than the actual ciphertext

[dartdart26]

@jot2re @mortendahl Sure, I will change it. Just want to understand why to make sure I am not missing something. Could you please explain why?

[mortendahl]

Oh, I thought this is not happening on the ASC, but instead in the core part. If it is happening on the ASC, we can change the ciphertext to a ciphertext_digest, that's what you mean?

Yes, exactly.

What would the core do if the signature matched on the ASC, but the ciphertext is bad? I guess that is equivalent to the core checking the signature.

The core errors out. This is valid. Another example could be if the core can't find the ciphertext in the gateway store.

What is the benefit of checking it on the ASC?

Because it's application specific, and we're trying to keep the core agnostic.

[dartdart26]

Got it re core being agnostic, thanks!

[dartdart26]

Will use keccak256 for the digest.

Also, I am not quite sure if we need the chain_id for the EIP712 domain, though. I suspect not. It is essentially linked to the FHE server key on the coprocessor. Looking at the ASC code, as of now it doesn't have a notion of the fhEVM chain_id it is serving. Any thoughts on whether we keep it? I say we remove it as I don't see any benefit to adding it.

[mortendahl]

I don't know either.

Should we even be using EIP712 here? Sometimes it feels like we don't have good reasons for not just using a plain signature instead. But I guess that as long as it's contained to Ehthereum-related components then it's okay.

[dartdart26]

Initially, I was suggesting we concatenate handle || ciphertext (which, based on what we agreed now, would be handle || ciphertext_digest). I don't exactly remember why we needed EIP712 here, maybe @jot2re or @manoranjith remember?

[dartdart26]

I guess, in some sense, EIP712 is self-documenting and more flexible, but I don't have a strong opinion on what to do here.

[jot2re]

The reason was to keep using the same signature scheme/approach. The less variability we have, the better. Also it would allow for efficient validation on-chain if this is every needed. It would not necessarily be the case for a normal ecdsa signature afir

[mortendahl]

I'm fine to keep it. I just ask that we stop to think if we ever want to use it in a non-Ethereum context.

[manoranjith]

@dartdart26 Reg handle | ciphertext. As, the handle is not necessarily derived from the content of cipher text, we chose to use the ciphertext digest.

[manoranjith]

Reg the choice of using EIP712, it was just for convenience as we are already using it for proof of storage.

AFAIK, we even agreed to set the verifying_contract field to 0x00... to indicate we do not use it.

[dartdart26]

@dartdart26 Reg handle | ciphertext. As, the handle is not necessarily derived from the content of cipher text, we chose to use the ciphertext digest.

I don't think it matters. Idea is to tie handle to the ciphertext digest (i.e. ciphertext) by signing the concatenation (or EIP struct containing them both). It doesn't matter what handle's value is, doesn't have to be digest of ciphertext.

[dartdart26]

Reg the choice of using EIP712, it was just for convenience as we are already using it for proof of storage.

AFAIK, we even agreed to set the verifying_contract field to 0x00... to indicate we do not use it.

I suggest we only give name and version and not set any fields we don't use. I didn't know we could do that when we discussed, that's why I suggested 0 address. But now I don't think we need any address. Wdyt?

[dartdart26]

I suggest we end up with:

alloy::sol! {
    struct GetCiphertextsResponseSignatureData {
        uint256 handle;
        bytes ciphertext_digest;
    }
}

The EIP712 domain would be:

let eip712_domain = alloy::sol_types::eip712_domain! {
    name: "GetCiphertextResponse",
    version: "1",
};