-
Notifications
You must be signed in to change notification settings - Fork 24
Port shared secret encryption from nucypher-core #646
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
base: epic-port-nucypher-core-into-ts
Are you sure you want to change the base?
Port shared secret encryption from nucypher-core #646
Conversation
✅ Deploy Preview for taco-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for taco-nft-demo ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -0,0 +1,235 @@ | |||
| { | |||
| "test_vectors": [ | |||
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 noticed the fixed nonce, specific shared secret - does it make sense to also have more random values in the test vector? Or is that something planned for later in the process?
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 values are randomly generated at the Rust code and fed as constants to the compatibility tests. For values that are randomly generated in this project, there is the unit test in the other file.
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 values are randomly generated at the Rust code and fed as constants to the compatibility tests.
Perhaps I'm misunderstanding. The values for the nonce, shared secret don't seem that "random"...? In one case the nonce is just 0s or 1s and the shared secret is just ints 0-31...?
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.
Yeah, sorry my bad. I forgot that I updated this in Rust. Yes, the values are fixed. But since the values does not have special numerical meanings, in relation to the algorithm, they are still counted as random (or you may say simi-random).
However, we can generate different values every time. But it will make debugging harder in case of an issue.
Hey @cygnusv, Could you please, provide your input if testing with new random values is needed for this cryptography validation.
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, the values are fixed.
Just to clarify: the test vectors should still contain specific values, but I expected each vector in the generated file to use different values for completeness. These values would originally have been generated randomly or pseudo-randomly.
For example, to generate the test vector file, you could use a fixed seed. Then, using that seed, you can deterministically generate pseudo-random nonces, shared secrets, etc., rather than reusing the same values or incrementing bytes only sequentially.
The seed approach ensures that the same file is generated every time (due to the fixed seed), but each test vector contains distinct, realistic inputs like unique nonces and shared secrets. This avoids test vectors that only differ trivially (e.g. same nonce in every case), which could miss edge cases or failures.
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.
Actually, in all cases, the typescript tests should run on any provided data in this json 😄 . So, whatever will be generated from nucypher-core, should be usable here regardless of what the values are.
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.
Yep. I'm referring to the strategy used in the code that generated the test vectors from nucypher-core.
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.
Would like continue this the conversation about the generated test vectors at nucypher/nucypher-core#105.
Thanks,
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.
Sorry for being a bit late to the discussion. I've been following along while working on nucypher/nucypher-core#105 and I changed my mind a few times during this process. The production and consumption of test vectors are a bit more complicated than expected due to how our internal protocol objects work, since in some cases serialization/deserialization is more difficult or not possible without adding code. I'm still iterating a proper method for producing test vectors in my PR, but on the PRs in taco-web we should strive to consume test vectors without resorting to duplicated implementations.
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.
With respect to the specific issue discussed here, the test vector ideally should be generated in a 100% deterministic way (i.e. using a seed). However, there's nothing wrong about adding some hard-coded, obviously not random examples, like [1, 1, 1, .....1].
| // Counter to ensure each call generates different values | ||
| let counter = 0; | ||
| return { | ||
| randomBytes: (size: number) => { |
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.
For my benefit, could you clarify what this is doing?
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 is to mock this function to make it give random but deterministic values (simi-random). Which is to make debugging the tests easy when needed. Because for every run of the tests the same simi-random value will be used.
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.
random but deterministic values (simi-random)
Does the test only work for a specific order of randomness - is there some limitation in the test that requires determinism?
My overall concern is about the test not actually using proper randomness/pseudo-randomness, which is closer to what would occur in the wild. Just trying to understand why encryption/decryption testing needs to use deterministic keys/values...
Similar comment regarding why the shared secret needs to be deterministic (L36-L39)?
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.
Does the test only work for a specific order of randomness - is there some limitation in the test that requires determinism?
No, the tests work with any values and there is no limitations in the tests. This mocking of random value generation can be totally removed and the tests still works great.
My overall concern is about the test not actually using proper randomness/pseudo-randomness, which is closer to what would occur in the wild. Just trying to understand why encryption/decryption testing needs to use deterministic keys/values...
As far as I know, as long as the provided values are not special in relevance to the algorithm they are random and there is no concern about them being generated. The values would be special to the algorithm, if it is for example the algorithm is the raising to the power of 2, and the values are just zeros.
So, as far as I know, having deterministic values is not of any concern. But it still make it easy to debug any issue if it is faced later. Such that every run to the test would have the same inputs.
I think @cygnusv would help us if there is any concern regarding the deterministic values. If yes, the mocking will be just removed.
Thanks all,
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.
No, the tests work with any values and there is no limitations in the tests. This mocking of random value generation can be totally removed and the tests still works great.
If this is the case, then let's just remove the mocking.
So, as far as I know, having deterministic values is not of any concern. But it still make it easy to debug any issue if it is faced later.
Not sure what you mean by debugging (?) - tough to debug ciphertext issues and additionally it's likely any failure would signify an issue with our use of the libraries, and not the underlying crypto since we are simply using existing crypto libraries and not rolling our own crypto.
Such that every run to the test would have the same inputs.
This is exactly what I'm hoping to avoid - that is a very limited test.
My overall mental model for our testing of this "taco-on-mobile" library falls into two key categories (correct me if I’m mistaken):
-
Rust Interoperability / Test Vector Parity:
We need to verify that the library can correctly match encryption and decryption behaviour with the existingnucypher-coreRust code. These tests rely on fixed test vectors—includes associated shared secrets, nonces, and payloads—to ensure deterministic parity. The goal is to validate that data encrypted/decrypted by the Rust implementation matches what was produced by "taco-on-mobile" library, ensuring compatibility across platforms.
(this covers the deterministic part) -
Testing the TACo on Mobile library Itself:
We also need to verify that the "taco-on-mobile" library can properly perform encryption and decryption on data it generates. By using randomly generated shared secrets and nonces, this simulates real-world usage, where inputs are not fixed. These tests should be run with randomized inputs each time they are run (locally or in CI). Of course we are not looking to test the underlying crypto (since those are the underlying libraries we are using), but I do think that us generally using randomness for testing where we can is a good thing.
(this covers the random part - the randomness is fine because encryption/decryption are mirror operations of each other i.e. if the same combination of sharedsecret,nonce is used for both encryption/decryption it should be fine)
This test code seems to be for 2), but is only testing specific input for every run of the test - as opposed to the test using random nonces/shared secrets on every run of the tests (locally, CI etc.). Using random values would be a more robust and complete set of tests that ensures that the code is continuously tested with new inputs every run.
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.
Many thanks @derekpierre for the discussion.
I did more search and I am more now in favor of having deterministic values for the randomness of our tests here. And probably if we are doing the cryptographic library we would keep this simi-random but have additional completely random values that change with every run.
However, since I am not very much into cryptography related stuff, I will leave this to you (@derekpierre) and @cygnusv to recommend.
When I first showed @cygnusv this way of generating the nonce, I think he liked it. But I hope him to provide his input, if it is better for it to be like this, or if not, after he read this discussion.
Thanks all,
| @@ -0,0 +1,235 @@ | |||
| { | |||
| "test_vectors": [ | |||
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.
Yep. I'm referring to the strategy used in the code that generated the test vectors from nucypher-core.
| const { sharedSecret, plaintext, fixedNonce, expectedCiphertext } = | ||
| vector; | ||
|
|
||
| if (fixedNonce && sharedSecret) { |
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 there cases where this is not true? i.e. do test vectors ever not contain a fixedNonce or sharedSecret value? I'm wondering if we need this if statement.
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.
[Sorry, it should be only checking for fixedNonce. The code is now updated.]
And this is just to give the confidence that even if the test vectors did not contain the nonce (so the encryption of data would not be possible, and the decryption is the only available), the plaintext could be retrieved by decrypting the cipher text.
However, if this case is unnecessary to check, the nucypher/nucypher-core#105 would not generate it and I will remove this if from here 😄 .
| // Counter to ensure each call generates different values | ||
| let counter = 0; | ||
| return { | ||
| randomBytes: (size: number) => { |
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.
No, the tests work with any values and there is no limitations in the tests. This mocking of random value generation can be totally removed and the tests still works great.
If this is the case, then let's just remove the mocking.
So, as far as I know, having deterministic values is not of any concern. But it still make it easy to debug any issue if it is faced later.
Not sure what you mean by debugging (?) - tough to debug ciphertext issues and additionally it's likely any failure would signify an issue with our use of the libraries, and not the underlying crypto since we are simply using existing crypto libraries and not rolling our own crypto.
Such that every run to the test would have the same inputs.
This is exactly what I'm hoping to avoid - that is a very limited test.
My overall mental model for our testing of this "taco-on-mobile" library falls into two key categories (correct me if I’m mistaken):
-
Rust Interoperability / Test Vector Parity:
We need to verify that the library can correctly match encryption and decryption behaviour with the existingnucypher-coreRust code. These tests rely on fixed test vectors—includes associated shared secrets, nonces, and payloads—to ensure deterministic parity. The goal is to validate that data encrypted/decrypted by the Rust implementation matches what was produced by "taco-on-mobile" library, ensuring compatibility across platforms.
(this covers the deterministic part) -
Testing the TACo on Mobile library Itself:
We also need to verify that the "taco-on-mobile" library can properly perform encryption and decryption on data it generates. By using randomly generated shared secrets and nonces, this simulates real-world usage, where inputs are not fixed. These tests should be run with randomized inputs each time they are run (locally or in CI). Of course we are not looking to test the underlying crypto (since those are the underlying libraries we are using), but I do think that us generally using randomness for testing where we can is a good thing.
(this covers the random part - the randomness is fine because encryption/decryption are mirror operations of each other i.e. if the same combination of sharedsecret,nonce is used for both encryption/decryption it should be fine)
This test code seems to be for 2), but is only testing specific input for every run of the test - as opposed to the test using random nonces/shared secrets on every run of the tests (locally, CI etc.). Using random values would be a more robust and complete set of tests that ensures that the code is continuously tested with new inputs every run.
|
Removing myself as reviewer while this is not ready to be reviewed. Don't hesitate to add me if there is progress on this! |
Type of PR:
Required reviews:
What this does:
encrypt_with_shared_secretanddecrypt_with_shared_secret.Issues fixed/closed: