Fix deal backward compatibility

[thehoul]

Fix compatibility issues between V3 and V4 when Deal struct is being encoded in VSS (both Pedersen and Rabin) and DKG. See this comment below for more information.

Depends on #604

[github-actions]

🔒 Could not start CI tests due to missing safe PR label. Please contact a DEDIS maintainer.

[thehoul]

I have been trying to wrap my head around fixing the serialization problem that we have between V3 and V4. I tried my best to boil it down to the source of the problem. Warning: pavé ahead.

So, it all starts from the fact that we needed to switch all the int occurrences for the appropriate intXX, uintXX for security reasons. Because of this, some struct have been modified accordingly. A relevant example in this case is the share.PriShare struct which contains what was a int and now is a uint32 indicating the index of the share (i.e. the i at which the secret polynomial has been evaluated). From here, it is obvious how this can be an issue: Because two nodes in the same network using respectively Kyber V3 and V4 will serialize the same struct differently. When one sends its serialized struct to the other, it needs to be adapted. Even more so when we know that protobuf encodes int using zigzag (i.e. mapping positive numbers to even positive numbers and negatives to odd positive numbers) but not for uint. Thus, encoding the int 150 will result in 300 being encoded and when 300 is decoded as a uint, it will be read as 300, not 150.
Okay, is this a problem though? Yes, but the question is rather, can we do anything about it? I don't think so! This is a change that we have to do (security reasons). And actually, when I think about it, Kyber does not enforce any kind of serialization. Instead, it is up to the users of the library to serialize the structs before sending them on the wire. Thus, I think that it is fair to say that this is a problem for the users to handle. We need to advertise these changes, of course, but after that, the interface with the network should not be our concern.
Yay! Nothing to do then, right? Well, not exactly. Because I said that Kyber does not enforce any kind of serialization but this is not actually entirely true. See, I said before that the struct share.PriShare is a relevant example. Why? Because it is used in the Deal struct in VSS (both Pedersen and Rabin) and this struct is serialized internally! In the code, when a dealer is created, a bunch of Deal structs are created for each participants in the network and then encrypted. protobuf is used at that point to serialize the Deal before it can be encrypted. This is within the library where the users don't have access. This is something that we have to take care of. I also searched throughout the code other instances where we used protobuf and luckily, there are only 3 instances: as mentioned above in both VSS. But also in Rabin DKG where a Deal is serialized in order to compute a hash. Here too, this is a problem since the hash will be different in V3 and V4 for a same Deal.
Okay, what is the solution then? I feel that we can probably just add marshal/unmarshal methods to Deal in V4 to handle this. In practice, the method would create an identical struct but with int instead of uint, convert its uint to int to put in this struct and encode using protobuf and reverse the process when decoding. This ensures compatibility with V3.

TL;DR

Because of the changes from int to int/uint32/64 in V4, a lot of struct don't match anymore between V3 and V4. It is a problem in a network with some nodes running version V3 and others V4 because sharing such struct will cause incompatibilities. But, my argument is that the way Kyber nodes interface with the network it not our concern, it's up to them to decide how they want to serialize and to ensure compatibility. Especially that these changes are needed in terms of security.

But, we also have 3 occurrences where protobuf is used internally to encode a struct that has a field that changed from int to uint in V4. This struct is Dealin VSS (Rabin and Pedersen) and in Rabin DKG and needs to be serialized in order to encrypt it. For reasons mentioned above, this will cause issues when nodes using V3 and V4 interact. I believe that the solution is to introduce in V4 methods to marshal and unmarshal a Deal that is compatible with V3.

[thehoul]

But, my argument is that the way Kyber nodes interface with the network it not our concern, it's up to them to decide how they want to serialize and to ensure compatibility.

I really need feedback to know if this is really a fair assumption. I really don't think that there is a way to make V4 compatible with V3 without forcing users to adapt how they serialize if they want to introduce V4. I especially think this is not a big deal for Kyber users (?)

[thehoul]

Tests don't pass because of protobuf shenanigans: we need to call registerInterface to make sure that decoding works. When running the test (in compatibility_test.go) individually, they work but not all at once because of this. I need to look into it.

[thehoul]

For now I have skipped test with 32b architectures because the tests compare with V3 which does not handle 32b architectures (because of the use of int everywhere). But before merging, these test should end up in kyber-test, not here

[jbsv]

does this remove dependency of kyber v4 on dedis version of protobuf ?

[thehoul]

Nope. Since Deal is encoded and decoded internally, we need in V4 to follow the same encoding as V3 to ensure compatibility and this encoding uses protobuf. Although, we only need to do this in two places so we could potentially mock the encoding/decoding process to remove this dependency.

[jbsv]

shouldn't we put this test in kyber-test to avoid direct dependency on kyber v3 ?

[thehoul]

Yes absolutely ! It's only there for now because I needed to be able to modify v4

[AnomalRoil]

agreed, let's try not to depend on the previous version in the new version

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
31.2% Coverage on New Code
4.9% Duplication on New Code

See analysis details on SonarQube Cloud

[jbsv]

But, my argument is that the way Kyber nodes interface with the network it not our concern, it's up to them to decide how they want to serialize and to ensure compatibility.

I really need feedback to know if this is really a fair assumption. I really don't think that there is a way to make V4 compatible with V3 without forcing users to adapt how they serialize if they want to introduce V4. I especially think this is not a big deal for Kyber users (?)

I would assume you're right. It might be also very helpful for the user to have an example on what and how to adapt their code from V3 to V4.

[AnomalRoil]

A few changes and suggestions

[AnomalRoil]

agreed, let's try not to depend on the previous version in the new version

[AnomalRoil]

do you really need both? I thought Protobuf would handle "unknown" fields like nonce nicely, thus only needing the first one

[AnomalRoil]

Make sure to add a note to the README.md about how to generate this, and potentially a go generate instuction to generate these, along with a Github Action that checks generated code hasn't been changed (re-generate it in a GHA and then checks git status is clean)

[thehoul]

This PR has been addressed completely by #619