-
Notifications
You must be signed in to change notification settings - Fork 16
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
Conversation
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
There was a problem hiding this 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
go-sdk/internal/utxo/ecdh.go
Outdated
func GenerateECDHSharedSecret(privKey *babyjub.PrivateKey, pubKey *babyjub.PublicKey) *babyjub.Point { | ||
privKeyForZkp := babyjub.SkToBigInt(privKey) | ||
return babyjub.NewPoint().Mul(privKeyForZkp, pubKey.Point()) | ||
} |
There was a problem hiding this comment.
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
go-sdk/internal/utxo/encryption.go
Outdated
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
go-sdk/internal/utxo/encryption.go
Outdated
if len(key) != 2 { | ||
return nil, fmt.Errorf("the key must have 2 elements, but got %d", len(key)) | ||
} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 yes good point, agreed
go-sdk/internal/utxo/encryption.go
Outdated
i := 0 | ||
for ; i < n; i++ { | ||
// Iterate Poseidon on the state | ||
state, err = poseidon.HashEx(state, 4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning on getting this in main kaleido-io/go-iden3-crypto@bc160bf#diff-0008711cc27d4bc9f740d866c7935cac289ad9d844a8dd3c831e9b73f3cc22a6R152?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
go-sdk/internal/utxo/encryption.go
Outdated
if len(cipherText)%3 != 1 { | ||
return nil, fmt.Errorf("the length of the cipher text must be 3n+1, but got %d", len(cipherText)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
go-sdk/internal/utxo/encryption.go
Outdated
// Record the last ciphertext element | ||
cipherText[i*3] = state[1] | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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])
go-sdk/internal/utxo/encryption.go
Outdated
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
package utxo |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
go-sdk/integration-test/e2e_test.go
Outdated
// 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()) |
There was a problem hiding this comment.
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!
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Looks great to me. Will leave the final approval and merge to @EnriqueL8 |
There was a problem hiding this 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
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]>
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 inkaleido-io/go-iden3-crypto
as a go dependency replacement