Fix obj-caps facility #2018

hu55a1n1 · 2022-03-27T20:21:47Z

Closes: cosmos/ibc-rs#53

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests: integration (for Hermes) or unit/mock tests (for modules).
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

…ve-obj-caps

…text

…o CoreModuleId

hu55a1n1

Adding some comments to help describe the main changes in this PR and the rationale for some design choices.

hu55a1n1 · 2022-04-13T10:08:28Z

modules/src/core/ics05_port/context.rs

 }

-pub trait CapabilityReader {
+pub trait CapabilityReader<M: Into<ModuleId>> {


This is the main change in this PR. CapabilityReader and CapabilityKeeper are now generic over ModuleId. This follows from the intuition that they are always scoped to a module and the library never makes calls to the underlying host system's (unscoped) CapabilityReader.
I experimented with two other designs - one with a composition style approach (see ec20b5a) and another with a freestanding CapabilityReader with a blanket scoped implementation, but both those designs proved to be unwieldy. The benefit of this approach is that it makes the least assumptions on the host's capability system and allows users to write a blanket implementation for all modules.

hu55a1n1 · 2022-04-13T10:10:06Z

modules/src/core/ics05_port/context.rs

    }
 }

-pub trait PortKeeper: CapabilityKeeper + PortReader {
+pub trait PortCapabilityKeeper<M: Into<ModuleId>>:


PortCapabilityKeeper and ChannelCapabilityKeeper have been extracted out of Port/ChannelKeeper. These traits only have provided methods and are provided for convenience.

hu55a1n1 · 2022-04-13T10:11:50Z

modules/src/core/ics24_host/path.rs

@@ -37,6 +37,7 @@ pub enum Path {
    Connections(ConnectionsPath),
    Ports(PortsPath),
    ChannelEnds(ChannelEndsPath),
+    ChannelCapability(ChannelCapabilityPath),


This is a new Path that defines a key for the channel capability in the persistent store.

hu55a1n1 · 2022-04-13T10:15:04Z

modules/src/core/ics26_routing/context.rs

@@ -31,9 +44,12 @@ pub trait Ics26Context:
    + ConnectionKeeper
    + ChannelKeeper
    + ChannelReader
-    + PortReader
+    + ChannelCapabilityKeeper<CoreModuleId, Capability = <Self as Ics26Context>::Capability>


All Capability related keepers that the Ics26Context depends on, are scoped to the CoreModule and all associated Capability types are bound to be the same.

hu55a1n1 · 2022-04-13T10:18:32Z

modules/src/core/ics05_port/capabilities.rs

-    fn from(index: u64) -> Self {
-        Self { index }
-    }
+pub trait Capability {


Capability is a trait now so the host system is free to define it's own non-forgeable and secure Capability type.

hu55a1n1 · 2022-04-13T10:27:00Z

modules/src/core/ics04_channel/handler/chan_open_try.rs

@@ -125,14 +136,18 @@ pub(crate) fn process(
    // Transition the channel end to the new state & pick a version.
    new_channel_end.set_state(State::TryOpen);

+    let (channel_id_state, channel_cap) = if msg.previous_channel_id.is_none() {


We only create a new capability if a previous_channel_id isn't specified.

hu55a1n1 · 2022-04-13T10:33:11Z

modules/src/mock/context.rs

        Self::new(
            ChainId::new("mockgaia".to_string(), 0),
            HostType::Mock,
            5,
            Height::new(0, 5),
        )
+        .with_router(dummy_router(ocap.clone()))


The default implementation now comes with a dummy_router() and an ocap system where the default port is already owned by the DummyModule. This setup simplifies handler tests.

hu55a1n1 · 2022-04-13T10:34:44Z

modules/src/mock/context.rs

+    }
+}
+
+impl Clone for MockContext {


We manually implement Clone because ocap is a pointer. This implementation sets up the ocap pointer to it's initial state as per the Default impl.

hu55a1n1 · 2022-04-13T10:36:38Z

modules/src/mock/context.rs

@@ -365,7 +424,27 @@ impl MockContext {
        channel_end: ChannelEnd,
    ) -> Self {
        let mut channels = self.channels.clone();
-        channels.insert((port_id, chan_id), channel_end);
+        channels.insert((port_id.clone(), chan_id), channel_end);
+        let (module_id, _) = self


Adding a channel automatically creates a new associated capability and is claimed by the module that owns the port capability.

hu55a1n1 · 2022-04-13T10:37:46Z

modules/src/mock/context.rs

@@ -124,19 +128,60 @@ pub struct MockContext {

    /// ICS26 router impl
    router: MockRouter,
+
+    /// Object capabilites impl
+    ocap: Arc<Mutex<MockOCap>>,


This must be shared across modules, hence the Arc<Mutex<>>

hu55a1n1 · 2022-04-13T10:58:12Z

modules/src/core/ics04_channel/handler/send_packet.rs

@@ -32,7 +32,8 @@ pub fn send_packet(ctx: &dyn ChannelReader, packet: Packet) -> HandlerResult<Pac
        return Err(Error::channel_closed(packet.source_channel));
    }

-    let _channel_cap = ctx.authenticated_capability(&packet.source_port)?;
+    // Fixme(hu55a1n1)


To fix this, we would need Ics20Context to have access to a scoped CapabilityReader and since there's already a PR for ICS20 impl, I updated the PR author checklist to include this -> #1989.

romac · 2022-04-29T07:53:28Z

@hu55a1n1 Shall we close this now that we've decided not go forward with the OCap model for the time being?

hu55a1n1 changed the base branch from master to hu55a1n1/1758-complete-ics26 March 27, 2022 20:53

Base automatically changed from hu55a1n1/1758-complete-ics26 to master April 4, 2022 17:43

hu55a1n1 added 9 commits April 6, 2022 11:12

Add ChannelCapabilityPath to ICS24

d7e4ff3

Simplify channel handler

abd9ae7

Fix tests

5576ca6

Provide handlers with assoc chan/port capabilities

8865166

Remove packet_validate and pass caps to packet handlers

bb06bd2

Improve PortReader and ChannelReader's capabilities API

263d09f

Update packet handler API to use new API

973873e

Fix mock caps and tests

ea35d86

Major refactoring

7fe7da1

hu55a1n1 force-pushed the hu55a1n1/2013-improve-obj-caps branch from c4d31de to 7fe7da1 Compare April 6, 2022 05:54

hu55a1n1 added 18 commits April 8, 2022 16:37

Merge remote-tracking branch 'origin/master' into hu55a1n1/2013-impro…

8ad0e3f

…ve-obj-caps

Extract Port/Channel CapabilityReader/Keeper into separate traits

7096036

Store channel capability after handler dispatch

a93f689

Adjust handlers to add {Channel,Port}Capability bound to context

a7b3b48

Introduce ScopedCapability{Keeper,Reader}

ec20b5a

Constrain Capability{Keeper,Reader} scope to core module for Ics26Con…

0f44595

…text

WIP: Update Mock impl to use scoped capability readers/keepers

17f4436

Capability Readers/Keepers are now scoped by definition

6d2cf00

Impl Default and Into<ModuleId> for CoreModuleId

1d0135c

Simplify Ics26Context bounds based on new design

c92fa84

Update all channel handlers to use CapabilityReaders/Keepers scoped t…

77ad8da

…o CoreModuleId

CoreModuleId ToString now returns 'ibc'

4708f05

Update tests to use new API

779a5b9

Implement dummy MockOCap

4c9e28e

Update Mock tests

8cdb2f8

Improve Capability ctor to accept index as input

0659fbb

Make claim_capability() and release_capability() are fallible

fb8e80d

Don't recreate capability in store_channel_capability()

19e92e3

hu55a1n1 added 18 commits April 12, 2022 14:34

Polish MockCap impl and remove legacy ocap facility

aa6da61

Add capability related errors

81e1e17

Impl on_chan_open_init for DummyModule

d993a3b

Manually implement Clone for MockContext

bfe717d

MockContext now initializes a router and ocaps by default

fea6038

Fix missing cap auth in write ack handler

2f55b74

Fix all handler tests

a8b96d4

Make write ack handler public

2d4a0c2

Fix tests

74131a0

Fix clippy errors

287f7df

Remove TypedCapabilites

c21ce3e

Capability as trait

1a71f48

Add .changelog entry

32d8733

Fix clippy warnings

d27b5bb

Resurrect channel/packet_validate()

8d49315

Update tests for API changes

a931d4a

Minor refactoring

ac365f8

Use is_none() instead of matches!(_, None)

9374f2c

hu55a1n1 commented Apr 13, 2022

View reviewed changes

hu55a1n1 marked this pull request as ready for review April 13, 2022 10:40

hu55a1n1 requested review from adizere, ancazamfir and romac as code owners April 13, 2022 10:40

hu55a1n1 commented Apr 13, 2022

View reviewed changes

hu55a1n1 mentioned this pull request Apr 13, 2022

Complete ICS20 Implementation #1989

Merged

7 tasks

adizere assigned plafer and hu55a1n1 Apr 29, 2022

adizere closed this May 2, 2022

romac deleted the hu55a1n1/2013-improve-obj-caps branch November 22, 2023 14:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix obj-caps facility #2018

Fix obj-caps facility #2018

hu55a1n1 commented Mar 27, 2022 •

edited

Loading

hu55a1n1 left a comment

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022 •

edited

Loading

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022 •

edited

Loading

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

hu55a1n1 Apr 13, 2022

romac commented Apr 29, 2022

Fix obj-caps facility #2018

Fix obj-caps facility #2018

Conversation

hu55a1n1 commented Mar 27, 2022 • edited Loading

Description

PR author checklist:

Reviewer checklist:

hu55a1n1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hu55a1n1 Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hu55a1n1 Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romac commented Apr 29, 2022

hu55a1n1 commented Mar 27, 2022 •

edited

Loading

hu55a1n1 Apr 13, 2022 •

edited

Loading

hu55a1n1 Apr 13, 2022 •

edited

Loading