Skip to content
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

feat: authz rules grant specific #3

Draft
wants to merge 30 commits into
base: vitwit/v0.50.5
Choose a base branch
from

Conversation

atheeshp
Copy link
Collaborator

@atheeshp atheeshp commented May 10, 2024

Description

Problem:
The existing authorization (authz) module lacks the flexibility to grant permissions (authz grants) for various types of messages along with specific conditions or rules. This limitation constrains users from customizing their transaction behavior based on specific needs or strategies.

Specific Examples of Limitations:

Swapping Reward Tokens:

  • Currently, users cannot set a rule to swap their reward tokens or any other tokens for another token with a specified limit.
    Sending Tokens to Selected Addresses:
  • Users are unable to authorize sending tokens to a pre-defined or selected address, restricting the ability to control where tokens are transferred.
    Staking Tokens with Limitations:
  • The module does not allow users to grant permission to stake tokens with certain limits or to stake only with selected validators. This limits the user's control over staking decisions.

PR diff:

This PR adds a feature which an authz can be granted with some rules,
for example:

  • if a staker wants to stake some portion of rewards he can do that by allowing max stake amount
  • if he wants to stake only to selected validators
  • swap some portion of rewards to another token or liquid staked token
  • also we can add rules to every message before granting.

Changes:
updated the ante handlers flow to check in the message is executing the authz message and any rules need to be checked before processing the message. if the message is not reaching the rules then it will eventually fail.

added an extra ante handler:

// handleCheckSendAuthzRules returns true if the rules are voilated
func (azd AuthzDecorator) handleSendAuthzRules(ctx sdk.Context, msg *banktypes.MsgSend, grantee []byte) error {
	granter, err := azd.ak.AddressCodec().StringToBytes(msg.FromAddress)
	if err != nil {
		return err
	}

	_, rules := azd.azk.GetAuthzWithRules(ctx, grantee, granter, sdk.MsgTypeURL(&banktypes.MsgSend{}))
	for _, rule := range rules {
		if rule.Key == authztypes.AllowedRecipients {
			isAllowed := false
			for _, allowedRecipient := range rule.Values {
				if msg.ToAddress == allowedRecipient {
					isAllowed = true
					break
				}
			}

			if !isAllowed {
				return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Recipient is not in the allowed list of the grant")
			}
		}

		if rule.Key == authztypes.MaxAmount {
			limit, err := sdk.ParseCoinsNormalized(strings.Join(rule.Values, ","))
			if err != nil {
				return err
			}
			if !limit.IsAllGTE(msg.Amount) {
				return errorsmod.Wrap(sdkerrors.ErrTxDecode, "Amount exceeds the max_amount limit set by the granter")
			}
		}

	}

	return nil
}

the above snippet checks the rules for MsgSend likewise we can add checks to every messages.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

@github-actions github-actions bot added the C:CLI label May 15, 2024
}

if rules != "" {
contents, err := os.ReadFile(rules)

Check failure

Code scanning / gosec

Potential file inclusion via variable Error

Potential file inclusion via variable
Copy link

@anilcse anilcse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't quite understand where are you storing the allowed rules (app-level).

x/auth/ante/authz_rules_ante.go Outdated Show resolved Hide resolved
x/auth/ante/authz_rules_ante.go Outdated Show resolved Hide resolved
}

if rules != "" {
contents, err := os.ReadFile(rules)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should validate against allowed list of inputs.

Suppose if the app developers allow only allowed_recipients, max_amount for bankMsg.Send type, it should only accept those inputs. You should check the same in the msg_server implementation too. msg.Validate() may be.

return err
}

authzRuleKeys := authz.AuthzRuleKeys{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should formally construct these rules. Or define a type instead of bytes, so we can marshal, unmarshal easily.

proto/cosmos/authz/v1beta1/tx.proto Show resolved Hide resolved
x/auth/ante/authz_rules_ante.go Outdated Show resolved Hide resolved
x/authz/generic_authorization.go Outdated Show resolved Hide resolved
x/authz/keeper/keeper.go Outdated Show resolved Hide resolved
@atheeshp
Copy link
Collaborator Author

I didn't quite understand where are you storing the allowed rules (app-level).

I am storing them within the grants.
https://github.com/vitwit/cosmos-sdk/pull/3/files#diff-274f19e8658baf03803b8371531f3e3ae0e5cbd7d8ccc667f8adaf2e21fd4489R35

@anilcse
Copy link

anilcse commented May 21, 2024

I didn't quite understand where are you storing the allowed rules (app-level).

I am storing them within the grants. https://github.com/vitwit/cosmos-sdk/pull/3/files#diff-274f19e8658baf03803b8371531f3e3ae0e5cbd7d8ccc667f8adaf2e21fd4489R35

That's the grant settings. We need to store the allowed keys per msg-type somewhere right? Otherwise users can send anything in the grant. That would be security concerns

@anilcse
Copy link

anilcse commented May 21, 2024

Also, please add the description for the PR. Let's do it as some spec style

Comment on lines +36 to +47
app.AuthzKeeper.SetAuthzRulesKeys(ctx, &authz.AllowedGrantRulesKeys{
Keys: []*authz.Rule{
{
Key: sdk.MsgTypeURL(&banktypes.MsgSend{}),
Values: []string{authz.MaxAmount, authz.AllowedRecipients},
},
{
Key: sdk.MsgTypeURL(&stakingtypes.MsgDelegate{}),
Values: []string{authz.AllowedStakeValidators, authz.AllowedMaxStakeAmount},
},
},
})

Check warning

Code scanning / gosec

Errors unhandled. Warning

Errors unhandled.
@atheeshp atheeshp requested a review from anilcse June 13, 2024 07:18
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

# Description

Problem:
The existing authorization (authz) module lacks the flexibility to grant permissions (authz grants) for various types of messages along with specific conditions or rules. This limitation constrains users from customizing their transaction behavior based on specific needs or strategies.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The existing authorization (authz) module lacks the flexibility to grant permissions (authz grants) for various types of messages along with specific conditions or rules. This limitation constrains users from customizing their transaction behavior based on specific needs or strategies.
The current authorization (authz) module lacks the flexibility needed to grant permissions (authz grants) for various types of messages along with specific conditions or rules. This limitation prevents users from customizing their transaction behavior according to specific needs or strategies.
To address this issue, we propose enhancing the authz module to support more granular permissions and conditional rules, allowing for greater customization and control over transaction authorization.

Comment on lines 7 to 8
Swapping Reward Tokens:
- Currently, users cannot set a rule to swap their reward tokens or any other tokens for another token with a specified limit.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Swapping Reward Tokens:
- Currently, users cannot set a rule to swap their reward tokens or any other tokens for another token with a specified limit.
Managing Reward Tokens:
At present, users are able to restake their tokens via authz. But it can do more. Currently users are unable to establish rules for swapping their reward tokens as a strategy as it requires IBCTransfer or PacketForward msgs access. It's not secure to give this grant currently as the recipient address can be anything and grantee can behave maliciously. But if there's a way to restrict recipient address to match with granter's address, this problem is solved. This functionality is necessary to enable users to automate and customize their token management strategies effectively.

Swapping Reward Tokens:
- Currently, users cannot set a rule to swap their reward tokens or any other tokens for another token with a specified limit.
Sending Tokens to Selected Addresses:
- Users are unable to authorize sending tokens to a pre-defined or selected address, restricting the ability to control where tokens are transferred.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Users are unable to authorize sending tokens to a pre-defined or selected address, restricting the ability to control where tokens are transferred.
- Users currently cannot authorize sending tokens to a pre-defined or selected address. This restriction limits their ability to control and automate the transfer of tokens to specific recipients, thereby reducing the efficiency and flexibility of their token management strategies. For example, if an organization wants to authorize an accountant to process salaries every month, the current system's limitations prevent this. Implementing an authz grant to recurrently allow a user to send a specified amount to certain accounts would solve this issue. This feature would automate salary payments, ensuring timely and accurate transactions while reducing administrative overhead.

- Currently, users cannot set a rule to swap their reward tokens or any other tokens for another token with a specified limit.
Sending Tokens to Selected Addresses:
- Users are unable to authorize sending tokens to a pre-defined or selected address, restricting the ability to control where tokens are transferred.
Staking Tokens with Limitations:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace it with gov usecase..

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, consider spec format of cosmos-sdk. It describes the problem, but clearly explain about solution, conclusion etc instead of PR diff.

@atheeshp atheeshp requested a review from anilcse July 19, 2024 05:29
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Aug 26, 2024
@github-actions github-actions bot closed this Aug 30, 2024
@anilcse anilcse reopened this Aug 30, 2024
@github-actions github-actions bot removed the Stale label Aug 31, 2024
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Sep 30, 2024
@github-actions github-actions bot closed this Oct 5, 2024
@anilcse anilcse reopened this Oct 5, 2024
@anilcse anilcse removed the Stale label Oct 5, 2024
Copy link

github-actions bot commented Nov 5, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the Stale label Nov 5, 2024
@anilcse anilcse removed the Stale label Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants