Require required hash fields (security improvement) #1818
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@mjurbanski-reef thank you for the contribution. Since this introduces breaking changes we will be reviewing and getting back to you. |
|
@mjurbanski-reef we plan on getting this into |
|
@gus-opentensor did plan change? I see #1899 was opened without this |
gus-opentensor
approved these changes
May 20, 2024
opendansor
reviewed
May 20, 2024
…elopers-ltd/bittensor into require_required_hash_fields
|
@gus-opentensor @opendansor solved the conflicts caused by pydantic v2 PR, can we get this security fix merged so I don't have to keep redoing it? |
gus-opentensor
approved these changes
May 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previous implementation allowed complete bypass of
body_hashchecks. This PR fixes it.This is more-or-less breaking change.
I say more-or-less, since, for most it should work without any trouble. Even if they don't update their side of the network. It will just mean they will be still vulnerable, but their nodes will still be able to communicate with the updated nodes.
Fixed issues
required_hash_fieldsno longer can be overridden (enabling hash bypass) by the remote noderequired_hash_fieldsare now STRICTLY required, i.e. if subnet developer, like in filetao, made a typo and had weaker hash because of it, now the code will immediately explode instead of issue going unnoticed for a long timeBreaking changes
required_hash_fieldspydantic field in bittensor.Synapse, they will no longer find it - IDK how much code out there uses it directly. Seeing the amount of copy&pasting going on in bittensor community, probably some does.axon.required_hash_fieldsmapping got removed (again, should not affect people in theory, but likely someone will be)Otherwise, there is some support for legacy subclasses of Synapse, i.e. if they define
required_hash_fieldspydantic fields, we will use it as usual.Upgrade steps
required_hash_fieldsas Synapse ClassVar, to list your field names in order of their definition, as otherwise you will get different hash values than before i.e. break compatibility across updated/unupdated nodes. Also if you had a typo in a field name, just don't add it if you don't want to break compatiblity with unupdated nodes.