-
Notifications
You must be signed in to change notification settings - Fork 413
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
More Granular Authz perms for cosmwasm contracts #817
More Granular Authz perms for cosmwasm contracts #817
Conversation
Codecov Report
@@ Coverage Diff @@
## main #817 +/- ##
==========================================
- Coverage 59.25% 58.72% -0.53%
==========================================
Files 51 51
Lines 5898 5951 +53
==========================================
Hits 3495 3495
- Misses 2150 2203 +53
Partials 253 253
|
|
||
// MsgTypeURL implements Authorization.MsgTypeURL. | ||
func (a ExecuteContractAuthorization) MsgTypeURL() string { | ||
return "/cosmos.wasm.base.MsgExecuteContract" |
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.
return "/cosmos.wasm.base.MsgExecuteContract" | |
return sdk.MsgTypeURL(&MsgExecuteContract{}) |
should be /cosmwasm.wasm.v1.MsgExecuteContract
, ref sendAuthz
// NOTE: exec.Msg is base64 encoded, so we need to decode it first | ||
// If there are allowedMessages then decode the call and check if the message is allowed | ||
callBz := []byte{} | ||
if _, err := base64.RawStdEncoding.Decode(exec.Msg.Bytes(), callBz); err != nil { |
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.
exec.Msg is json data
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.
Could you extract the whole JSON matching logic into a separate function that can easily be unit tested? Like func isJSONObjectWithTopLevelKey(jsonBytes, allowedKeys)
.
I extracted and implemented the JSON object matching in #819 (last commit). I wanted to create a PR into your PR but chan't easily choose your branch as the base branch. Can you check the last commit and cherry-pick it? |
With #819 being merged, you can now use |
A version using func (a ExecuteContractAuthorization) Accept(ctx sdk.Context, msg sdk.Msg) (authztypes.AcceptResponse, error) {
exec, ok := msg.(*MsgExecuteContract)
if !ok {
return authztypes.AcceptResponse{}, sdkerrors.ErrInvalidType.Wrap("type mismatch")
}
var allowed *AllowedContract
for _, allowedContract := range a.AllowedContracts {
// ctx.GasMeter().ConsumeGas(gasCostPerIteration, "wasm execute authorization")
if allowedContract.ContractAddress == exec.Contract {
allowed = allowedContract
break
}
}
// verify contract is allowed
if allowed == nil {
return authztypes.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf("cannot run %s contract", exec.Contract)
}
// verify msg is allowed
err := IsJSONObjectWithTopLevelKey(exec.Msg, allowed.AllowedMessages)
if err != nil {
return authztypes.AcceptResponse{}, sdkerrors.ErrUnauthorized.Wrapf("no allowed msg: %s", err.Error())
}
// remove if is Once
return authztypes.AcceptResponse{Accept: true, Delete: allowed.Once}, nil
} are valid duplicate contracts in allowed list? |
staking authz consumes gas for iterations, in this case it can be applied to contract or msg allowed list. |
Duplicates are no problem IMO. If you allow a contract twice, well, you still allowed it. But this could be checked in ValidateBasic to point devs to accidental misuse.
👍 When it comes to payload specific gas consumtion I would definitely start with the size of the JSON document since parsing that JSON is more load than those loops. |
yes, json parsing. I think it might also be necessary to add a command |
No description provided.