Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go implementation of encrypt/decrypt and ECDH to work together with the circuits #57

Merged
merged 17 commits into from
Sep 6, 2024

Conversation

jimthematrix
Copy link
Contributor

@jimthematrix jimthematrix commented Sep 4, 2024

Porting the implementation of poseidon based encryption and decryption in zkp/js/lib/utils, based on https://drive.google.com/file/d/1EVrP3DzoGbmzkRmYnyEDcIQcXVU7GlOd/view.

Also added GenerateECDHSharedSecret() as a convenient method, to be used by the receiver of encrypted transaction outputs.

Currently dependent on an enhancement to go-iden3-crypto, which has been submitted as iden3/go-iden3-crypto#66. But until that's merged, we are using the fork in kaleido-io/go-iden3-crypto as a go dependency replacement

Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff - took me a while to get my head around a few things. Reviewed with the paper on the side and it looks great

Comment on lines 21 to 24
func GenerateECDHSharedSecret(privKey *babyjub.PrivateKey, pubKey *babyjub.PublicKey) *babyjub.Point {
privKeyForZkp := babyjub.SkToBigInt(privKey)
return babyjub.NewPoint().Mul(privKeyForZkp, pubKey.Point())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be something to contribute to go-iden3-crypto/babyjub

l := big.NewInt(int64(len(msg)))
state := []*big.Int{big.NewInt(0), key[0], key[1], big.NewInt(0).Add(nonce, big.NewInt(0).Mul(l, &two128))}

cipherText := make([]*big.Int, length+1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is there one more length for the cipher text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cipher text array is always of length 3n+1, because the plain text array is always padded to length 3n (since we use poseidon hash of 4 input elements), plus one extra element recording the internal state of the final hash at index 1, as additional safety check during decrypt

if len(key) != 2 {
return nil, fmt.Errorf("the key must have 2 elements, but got %d", len(key))
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing a check that the nonce < 2^128 or maybe big.Int already limits it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need this in decrypt as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yes good point, agreed

i := 0
for ; i < n; i++ {
// Iterate Poseidon on the state
state, err = poseidon.HashEx(state, 4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think I fully understand why the first 4 Outs outputs that include intermediate states, because the state has 4 parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's tracked by iden3/go-iden3-crypto#66

Comment on lines 107 to 108
if len(cipherText)%3 != 1 {
return nil, fmt.Errorf("the length of the cipher text must be 3n+1, but got %d", len(cipherText))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be constraint as the paper just states that if the cipherText is not divisible by 3 then you check that the last 3 - (l mod 3) elements of the message are 0. Which means that it's always padded since in the encrypt the last cipher text is added... confused by C.add(S[1])

Definitely easier to implement it the way it is now but then you have to return the length from the Encrypt so they can pass in the correct mod 3 == 1 length on decrypt.

Copy link
Contributor Author

@jimthematrix jimthematrix Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the extra padding including the last element in the cipher text are all for safety checks, to ensure that unless the right keys are used, the decrypt will fail. otherwise you'll always get some number even if the wrong keys are used, but obviously they won't be the original plain text numbers, but the party performing the decryption may not have a way to check that.

Comment on lines 92 to 94
// Record the last ciphertext element
cipherText[i*3] = state[1]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost caught me but because you are doing i++ the loop will add to that for checking and those i is incremented for you to use here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the paper is confusing on this Release last ciphertext element: and C.add(S[1])

// See the License for the specific language governing permissions and
// limitations under the License.

package utxo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odd to see this under the utxo package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true. moved the encryption, ecdh along with salt and nonce generation methods to a new crypto package

assert.Equal(t, 10, len(cipherText))

// Decrypt the message
decryptedMsg, err := PoseidonDecrypt(cipherText, sharedKey, nonce, 7)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait wouldn't you want to test with a different shared key so public B, private A encrypt and public A, private B decrypt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using ecdh to derive the shared secret is performed in the integration-test under Test 2. here we are focused on the encryption and decryption, which is a symmetric scheme so the same key is used for both

Comment on lines 228 to 250
// the receiver would be able to get the encrypted values and salts
// from the transaction events
encryptedValues := make([]*big.Int, 4)
for i := 0; i < 4; i++ {
v, ok := new(big.Int).SetString(proof.PubSignals[i], 10)
assert.True(t, ok)
encryptedValues[i] = v
}

// the first two elements in the public signals are the encrypted value and salt
// for the first output. decrypt using the receiver's private key and compare with
// the UTXO hash
secret := utxo.GenerateECDHSharedSecret(receiver.PrivateKey, sender.PublicKey)
decrypted, err := utxo.PoseidonDecrypt(encryptedValues, []*big.Int{secret.X, secret.Y}, encryptionNonce, 2)
assert.NoError(t, err)
assert.Equal(t, outputValues[0].String(), decrypted[0].String())
assert.Equal(t, salt3.String(), decrypted[1].String())

// as the receiver, to check if the decryption was successful, we hash the decrypted
// value and salt and compare with the output commitment
calculatedHash, err := poseidon.Hash([]*big.Int{decrypted[0], decrypted[1], receiver.PublicKey.X, receiver.PublicKey.Y})
assert.NoError(t, err)
assert.Equal(t, output1.String(), calculatedHash.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha for some reason I had missed this!

@Chengxuan
Copy link
Contributor

Chengxuan commented Sep 6, 2024

Looks great to me. Will leave the final approval and merge to @EnriqueL8

Copy link
Contributor

@EnriqueL8 EnriqueL8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on nits above but left an approve for you to decide

jimthematrix and others added 2 commits September 6, 2024 14:12
Co-authored-by: Chengxuan Xing <[email protected]>
Signed-off-by: jimthematrix <[email protected]>
Co-authored-by: Chengxuan Xing <[email protected]>
Signed-off-by: jimthematrix <[email protected]>
@jimthematrix jimthematrix merged commit b022e1d into main Sep 6, 2024
5 checks passed
@jimthematrix jimthematrix deleted the go-decrypt branch September 6, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants