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

Controller module should claim the channel capability instead of the Auth module #2033

Closed
3 tasks
colin-axner opened this issue Aug 18, 2022 · 3 comments · Fixed by #2146
Closed
3 tasks
Assignees
Milestone

Comments

@colin-axner
Copy link
Contributor

Summary

Move channel capability claiming to the controller module

Problem Definition

Currently the ICA auth module claims the channel capability, but this becomes problematic when we add #2026 as that would require the channel capability to be passed via a message and for the auth module to be aware of ics27 logic

Proposal

Move channel capability from auth module claiming to controller module claiming.

This reduces the security of ics27 to the security model provided by the SDK (all modules on the chain must not be malicious, otherwise a module could simply send tokens from any account to any other account). That means any auth module can send a packet on behalf of any controller account, of course they are still expected to actually validate that the user trying to send on a controller account is actually the owner of the controller account.

The benefit is that auth modules now do not need to be developed with ics27 in mind (gov/groups are automatically supported by default)

Actions:

  • controller module claims capability in channel handshake. See how transfer claims the capability. The underlying app attached to the controller module should be passed "nil" in place of the chanCap
  • Controller module should ignore provided capability in SendTx and retrieve the capability like transfer does
  • add test cases to check the added functions calls

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@puneet2019
Copy link

hey @colin-axner will the capabilities need to be updated if I move from v3.0.0 to 5.1.0? And how will that process look like?
Deleting capabilities and adding the new ones?

@seantking seantking removed their assignment Aug 23, 2022
@colin-axner
Copy link
Contributor Author

colin-axner commented Aug 23, 2022

Hey @puneet2019. Apologies for the delayed reply. We will be adding more detailed documentation on these changes. They are intended to be backwards compatible, so there should be very minimal changes to how you interact with v3 and v6 (aside from having the possibility to remove complexity)

If you move from v3 to v6, then if you continue to use the existing ics27 API (call into RegisterInterchainAccount/SendTx) then you simply can pass "nil" as the channel capability. You can migrate to using the added Msg's (MsgRegisterInterchainAccount, MsgSendTx), but initializing an account via messages will result in the auth module app callbacks not being called

If the upgrade from v3 to v6 occurs on a production chain where you need state to be persisted across upgrades, then we will need to expose channel capability migrations which migrate ownership of all ics27 controller module channel capabilities from being owned by the authentication module to being owned by the controller module.

Do you plan to launch a pre v6 version in production?

edit: changed v5.1 to v6

@seantking seantking self-assigned this Aug 25, 2022
@seantking seantking removed their assignment Aug 29, 2022
@damiannolan damiannolan self-assigned this Aug 30, 2022
@damiannolan damiannolan moved this from Todo to In progress in ibc-go Aug 30, 2022
Repository owner moved this from In progress to Done in ibc-go Aug 30, 2022
@colin-axner
Copy link
Contributor Author

Just fyi @puneet2019, we have added channel capability migrations to ensure any capabilities claimed by underlying applications in pre v6 versions will be able to upgrade without a problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants