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

[Core] Add a Signer validation method to include host-specific checks for validate() processes #619

Closed
Tracked by #233
Farhad-Shabani opened this issue Apr 13, 2023 · 5 comments · Fixed by #651
Closed
Tracked by #233
Assignees
Labels
O: security Objective: aims to enhance security and improve safety
Milestone

Comments

@Farhad-Shabani
Copy link
Member

Problem Statement

IBC messages contain a Singer field, which have not yet been used in any operations of IBC-rs, but should be checked and disallowed for a counterfactual signing. So, from the IBC module standpoint, we get sure that once a message passes the validation phase, all the fields are valid.

  • Similar checks are done in IBC-go upon calling a sdk method like here

Proposal

It can be accomplished by exposing an additional interface through ValidationContext that provides a room for hosts to verify signer accounts as needed.

@Farhad-Shabani Farhad-Shabani added the O: reliability Objective: cause to improve trustworthiness and consistent performing label Apr 13, 2023
@Farhad-Shabani Farhad-Shabani moved this to 📥 To Do in ibc-rs Apr 13, 2023
@Farhad-Shabani Farhad-Shabani added O: security Objective: aims to enhance security and improve safety and removed O: reliability Objective: cause to improve trustworthiness and consistent performing labels Apr 13, 2023
@plafer
Copy link
Contributor

plafer commented Apr 14, 2023

Should this be included in #233 as well?

@hdevalence
Copy link

Why does the signer field need to be validated?

IBC messages are self-authenticating, the relayer is not a trusted part of the system design. They're authenticated by the counterparty chain, not the relayer who happened to send the transaction.

The current domain types in ibc-rs ban empty signer fields, but this is does not seem to be part of the IBC specification.

@Farhad-Shabani
Copy link
Member Author

Why does the signer field need to be validated?
IBC messages are self-authenticating, the relayer is not a trusted part of the system design. They're authenticated by the counterparty chain, not the relayer who happened to send the transaction.

Sounds like we are on the same page. Maybe, I didn't phrase this issue well.
The main goal is to make sure that every field, even sub-fields, of a domain IBC message undergoes thorough checks when going through the validate() process (e.g. here). Thus, from IBC-rs point of view, passing the validate() means an IBC message is fully trustworthy.

To achieve this, a validation check:

  1. might be performed internally (without needing to assume anything about host chains),
  2. or be delegated to hosts by exposing the necessary interface, allowing them to implement whatever validation they believe is necessary. E.g. here for the Signer we can have ctx.validate_signer(signer) within that validate() function.
    (as a side note, for signers in IBC-go, we can use the SDK's AccAddressFromBech32 to verify their authenticity, so no additional signer check method exists there for the context).

This way, we are sure that all the needed validation steps are being considered. None are being missed. Besides, the boundaries between the checks that should be carried out by IBC-rs and those that should be handled by hosts are clearly defined.

The current domain types in ibc-rs ban empty signer fields, but this is does not seem to be part of the IBC specification.

Given the above points, yes. To me, it seems more appropriate to drop this too and leave it to the hosts.

Hopefully, I didn't miss anything, but if so, let me know.

@Farhad-Shabani Farhad-Shabani changed the title [Core] Missing Signer validation [Core] Add a Signer validation method to include host-specific checks for validate() processes Apr 18, 2023
@Farhad-Shabani
Copy link
Member Author

Should this be included in #233 as well?

I think it's better to handle this separately

@plafer
Copy link
Contributor

plafer commented May 1, 2023

Why does the signer field need to be validated?

IBC messages are self-authenticating, the relayer is not a trusted part of the system design. They're authenticated by the counterparty chain, not the relayer who happened to send the transaction.

The current domain types in ibc-rs ban empty signer fields, but this is does not seem to be part of the IBC specification.

ibc-go ensures that the Signer field is a valid Bech32 address (e.g. here). A validate_signer() method would allow chains to do similar validation if they want

@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs May 1, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.39.0 milestone May 1, 2023
@Farhad-Shabani Farhad-Shabani self-assigned this May 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: security Objective: aims to enhance security and improve safety
Projects
Status: Done
3 participants