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

port allocation - centralized method for handing out ports to ensure that callers cannot squat on well-known ports #9165

Closed
aj-agoric opened this issue Mar 28, 2024 · 5 comments · Fixed by #9228
Assignees

Comments

@aj-agoric
Copy link

aj-agoric commented Mar 28, 2024

port allocation - centralized method for handing out ports to ensure that the same ports aren’t reused. Right now Network Vat allows you to request any port.

@michaelfig
Copy link
Member

First, I'd suggest creating a PortAllocator object with vat-network.jss protocol in its state, and methods like:

{
  allocateIBCPort() {
    const { state } = this;
    // Allocate an IBC port with a unique generated name.
    return E(state.protocol).bind(`/ibc-port/`);
  },
  allocateICAControllerPort() {
    const { state } = this;
    state.lastICAPortNum += 1n;
    return E(state.protocol).bind(`/ibc-port/icacontroller-${state.lastICAPortNum}`);
  },
}

Then, I'd suggest rewriting network-proposal.js#setupNetworkProtocols#makePorts as follows:

  { consume: { portAllocator }} // get the allocator object from the promise space
  //...
  const makePorts = async () => {
    // Bind to some fresh ports (either unspecified name or `icacontroller-*`)
    // on the IBC implementation and provide them for the user to have.
    const ibcportP = [];
    for (let i = 0; i < NUM_IBC_PORTS_PER_CLIENT; i += 1) {
      if (i === NUM_IBC_PORTS_PER_CLIENT - 1) {
        const portP = when(E(portAllocator).allocateICAControllerPort());
        ibcportP.push(portP);
      } else {
        const portP = when(E(portAllocator).allocateIBCPort());
        ibcportP.push(portP);
      }
    }
    return Promise.all(ibcportP);
  };

Exercise for the reader: make the PortAllocator available from vat-network.js's root object via a getPortAllocator() function, taking a hint from the use of zone.makeOnce in that file. Then, in network-proposal.js where the network vat is built, put the port allocator in the promise space.

@iomekam iomekam self-assigned this Apr 2, 2024
@aj-agoric
Copy link
Author

marking "in progress" per message with ikenna

@raphdev
Copy link
Contributor

raphdev commented Apr 10, 2024

Right now Network Vat allows you to request any port.

What's the consequence of this behavior? If I know what port a vat is using, could I request it and be able to listen on the same port?

@michaelfig michaelfig changed the title port allocation - centralized method for handing out ports to ensure that the same ports aren’t reused port allocation - centralized method for handing out ports to ensure that callers cannot squat on well-known ports Apr 11, 2024
@michaelfig
Copy link
Member

Right now Network Vat allows you to request any port.

What's the consequence of this behavior?

Port names have special meanings to some IBC protocols or clients. The Network Vat does not restrict the name of what you can allocate/bind. That allows "squatters" to allocate vanity or conventional port names which is too powerful a capability to have for most clients. An attenuated central port allocator can allow port names to be allocated without permitting squatters.

If I know what port a vat is using, could I request it and be able to listen on the same port?

No, it's only allocated/bound once (until the original allocator revokes the port). If you request a port name that is currently bound, you just get rejected.

@0xpatrickdev
Copy link
Member

What's the consequence of this behavior?

Port names have special meanings to some IBC protocols or clients.

In the case of ICS-27, interchain accounts, the same port identifier can be reused to recover an account:

In the event of a channel closing, the active channel may be replaced by starting a new channel handshake with the same port identifiers on the same underlying connection of the original active channel. ICS-27 channels can only be closed in the event of a timeout (if the implementation uses ordered channels) or in the unlikely event of a light client attack. Controller chains must retain the ability to open new ICS-27 channels and reset the active channel for a particular portID (containing {owner-account-address}) and connectionID pair.

The controller and host chains must verify that any new channel maintains the same metadata as the previous active channel to ensure that the parameters of the interchain account remain the same even after replacing the active channel. The Address of the metadata should not be verified since it is expected to be empty at the INIT stage, and the host chain will regenerate the exact same address on TRY, because it is expected to generate the interchain account address deterministically from the controller portID and connectionID (both of which must remain the same).

https://github.com/cosmos/ibc/blob/33d936aeecd649f25d4e357c942fcb5e4dec96e3/spec/app/ics-027-interchain-accounts/README.md#:~:text=In%20the%20event,remain%20the%20same).

@mergify mergify bot closed this as completed in #9228 Apr 30, 2024
mergify bot added a commit that referenced this issue Apr 30, 2024
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺
v                               ✰  Thanks for creating a PR! ✰
☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >  -->

<!-- Most PRs should close a specific Issue. All PRs should at least
reference one or more Issues. Edit and/or delete the following lines as
appropriate (note: you don't need both `refs` and `closes` for the same
one): -->

closes: #9165

## Description

<!-- Add a description of the changes that this PR introduces and the
files that
are the most critical to review.
-->

`bindPort` gives a caller too much power to specify exactly what port
they want allocated. With this change, we create a central object,
`PortAllocator` that handles handing out ports to the caller. As of
today, we are allocating ports for:
1. `/ibc-port`
2. `/ibc-port/icacontroller`
3. `/ibc-port/pegasus`

### Security Considerations

<!-- Does this change introduce new assumptions or dependencies that, if
violated, could introduce security vulnerabilities? How does this PR
change the boundaries between mutually-suspicious components? What new
authorities are introduced by this change, perhaps by new API calls?
-->

This will prevent `squatting`, where callers can come and claim certain
ports that they have no association with

### Documentation Considerations

<!-- Give our docs folks some hints about what needs to be described to
downstream users.

Backwards compatibility: what happens to existing data or deployments
when this code is shipped? Do we need to instruct users to do something
to upgrade their saved data? If there is no upgrade path possible, how
bad will that be for users?

-->

We've removed the `bindPort` method from the network vat. All the
contracts inside of `agoric-sdk` have been changed to use
`portAllocator`

### Testing Considerations

<!-- Every PR should of course come with tests of its own functionality.
What additional tests are still needed beyond those unit tests? How does
this affect CI, other test automation, or the testnet?
-->

I've included some unit test testing the new allocate APIs

---------

Co-authored-by: Michael FIG <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants