network/marshal: fix ServerIdentity unmarshalling

[tharvik]

PR failing on purpose (for now).

ServerIdentity is not checked for consistency when unmarshaling. As such, one can create an invalid one, send it accross network and it won't be checked when deserializing it, which can yield security issues (unchecked attacker-controlled input).

To fix that, one would implement encoding.BinaryUnmarshaler, which I'm trying to do now, but I need some help coding it, if someone has the time :)
So, I'm adding func (si *ServerIdentity) UnmarshalBinary(data []byte) error; as I just want to fix ID after most deserialisation is done, I'm trying first to Unmarshal or protobuf.Decode{,WithConstructors}, which in turn calls UnmarshalBinary as it can now be casted to encoding.BinaryUnmarshaler :/
And I don't want to redo protobuf decoding by hand, so, is there a way I'm missing?
A workaround that might work is to have a ServerIdentityWrapper which is also registered in onet and is trivialy always correct (removed the ID field), but that still allow people to send broken ServerIdentity and is quite an API-breaking change.

[Gilthoniel]

The server identity ID is deterministic so instead of including the value in the serialization/deserialization, it should be gotten through a getter that generates the value on the fly. The field shouldn't exist but backward compatibility reason, it shall remain there.

[tharvik]

So, what do we do? Do we break compatibility for the sake of security (which I'm all for)? I don't see a golang way to add a getter on a value that would allow this kind of checking.

[Gilthoniel]

I guess for the moment, a deprecation notice on the ID field and add a GetID() function ? Then switch the usage of the field to the function.

Breaking compatibility is not an option for sure even if it's clearly not an ideal situation. The field should still be filled with the actual ID value unfortunately.

[tharvik]

Hum, I'm looking at how to check for Deprecated: use, which is not really implemented in standard golang tools (neither in golint nor go {build,vet}), so users won't check/use the new function if the field is still available.

So, I'm proposing the following:

• add a staticcheck pass to the repo, with a config file to only check for deprecation (it's so much more powerful but let's start small)
  • btw, master is currently not passing 54 deprecated uses
• deprecate ID and tell users to check for GetID (which not much people will do but we can push staticcheck use; I guess that's the best we can do)
• the next (or the one after) breaking-changes release, we remove the field, as it is clearly unsecure to rely upon

[ineiti]

This PR should only include the second point of your list:

• deprecate ID and tell users to check for GetID (which not much people will do but we can push staticcheck use; I guess that's the best we can do)

For the other two, please open two issues

[tharvik]

Adding the GetID and use it thoughout the codebase.

staticcheck pass is followed in dedis/Coding#5
ServerIdentity.ID removal in #635

[tharvik]

So yeah, github crashed and that's probably why the tests ran again and failed. Also, I've rebase it on master.