less permissive VPs #2213
less permissive VPs #2213
Conversation
tzemanovic
force-pushed
the
tomas/vp-less-permissive
branch
from
4df9a92 to
cfee2cd
Compare
|
looks like lots of e2e tests are affected, looking into it |
tzemanovic
force-pushed
the
tomas/vp-less-permissive
branch
from
31c3779 to
e4bcccc
Compare
|
there are some more changes needed for IBC |
| }); | ||
| if let Some(bond_id) = bond_id { | ||
| // Bonds and unbonds changes for this address must be signed | ||
| return &bond_id.source != owner || **valid_sig; |
There was a problem hiding this comment.
Should the &bond_id.source != owner still be here now? How do we enforce that a delegation is signed / truly submitted by the delegator?
There was a problem hiding this comment.
I believe that’s what this does - if the source is eq to the owner then the sig is required
There was a problem hiding this comment.
ah yes ok, forgot that this is now the VP for normal users and validators. So in the delegator's VP, the sig will be required, but in the validator's VP it won't be, as expected
wasm/wasm_source/src/vp_user.rs
Outdated
| (None, Some(_post)) => { | ||
| // Becoming a validator must be authorized | ||
| **valid_sig |
There was a problem hiding this comment.
maybe unnecessary, but should we have a address == owner && *valid_sig here too?
There was a problem hiding this comment.
It would be better yes, though it won’t be an issue with our existing tx where it’s a single action
wasm/wasm_source/src/vp_user.rs
Outdated
| let is_valid_reward_claim = || { | ||
| if let Some(bond_id) = storage::is_last_pos_reward_claim_epoch_key(key) | ||
| { | ||
| // Claims for this address must be signed | ||
| return &bond_id.source != owner || **valid_sig; | ||
| } | ||
| false | ||
| }; |
There was a problem hiding this comment.
do we need to consider the rewards counter key?
There was a problem hiding this comment.
Ah yes! Otherwise a claim might get rejected
| let is_valid_become_validator = || { | ||
| if storage::is_validator_addresses_key(key) | ||
| || storage::is_consensus_keys_key(key) | ||
| || storage::is_validator_eth_cold_key_key(key).is_some() | ||
| || storage::is_validator_eth_hot_key_key(key).is_some() | ||
| || storage::is_validator_max_commission_rate_change_key(key) | ||
| .is_some() | ||
| || storage::is_validator_address_raw_hash_key(key).is_some() | ||
| { | ||
| // A signature is required to become validator | ||
| return **valid_sig; | ||
| } | ||
| false | ||
| }; |
There was a problem hiding this comment.
should this look at the data in these keys from pre and post and be valid if the pres are None and posts are Some?
There was a problem hiding this comment.
I think we don’t need to go that deep in here. The detailed PoS validation should happen in PoS VP and this VP should only be concerned with authorization
wasm/wasm_source/src/vp_user.rs
Outdated
| protocol_key, | ||
| commission_rate, | ||
| max_commission_rate_change, | ||
| email: "[email protected]".to_string(), |
tzemanovic
force-pushed
the
tomas/vp-less-permissive
branch
from
8788f52 to
763ea16
Compare
* tomas/vp-less-permissive: additions from comments wasm/vp_user+vp_implicit: update for IBC actions benches: fix foreign key write tx sigs wasm/vp_user+vp_implicit: always print rejected keys wasm/vp_user+vp_implicit: impl PoS key checks changelog: add #2213 wasm/vp_implicit: require valid sig for unknown changes wasm/vp_user: require valid sig for unknown changes wasm/vp_implicit: port Masp and PgfSteward key handling from vp_user wasm/vp_user: fix a typo
* origin/tomas/vp-less-permissive: additions from comments wasm/vp_user+vp_implicit: update for IBC actions benches: fix foreign key write tx sigs wasm/vp_user+vp_implicit: always print rejected keys wasm/vp_user+vp_implicit: impl PoS key checks changelog: add #2213 wasm/vp_implicit: require valid sig for unknown changes wasm/vp_user: require valid sig for unknown changes wasm/vp_implicit: port Masp and PgfSteward key handling from vp_user wasm/vp_user: fix a typo
Describe your changes
fixes #408
Unknown changes in vp_user and vp_implicit now require valid signature(s).
For PoS changes, we have to check against all possible storage changes that can be applied from a tx and require signature(s) for those that have to be authorized by the source (for vp_user it can be both delegator and validator actions, for vp_implicit only delegator actions). If the change within PoS account subspace is not recognized we default to require sig(s).
To ease debugging in vp_user and vp_implicit, I changed the logging of rejected keys to also be be emitted in release build.
Indicate on which release or other PRs this topic is based on
v0.28.0
Checklist before merging to
draft