From 1c1007de0b4bb4deecf56fe4a5d47cbf454f0192 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 5 Oct 2020 18:07:44 -0400 Subject: [PATCH 01/28] Initial WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 145 ++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 docs/architecture/adr-033-protobuf-ocaps.md diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md new file mode 100644 index 000000000000..83d7d8813c3a --- /dev/null +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -0,0 +1,145 @@ +# ADR 033: Protobuf Module Object Capabilities + +## Changelog + +- 2020-10-05: Initial Draft + +## Status + +Proposed + + +## Abstract + +> "If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the ADR. +> A short (~200 word) description of the issue being addressed. + + +## Context + +> This section describes the forces at play, including technological, political, social, and project local. These forces are probably in tension, and should be called out as such. The language in this section is value-neutral. It is simply describing facts. It should clearly explain the problem and motivation that the proposal aims to resolve. +> {context body} + + +## Decision + +> This section describes our response to these forces. It is stated in full sentences, with active voice. "We will ..." +> {decision body} + +### Module Keys + +```go +type ModuleKey interface { + DerivedKey(path []byte) ModuleKey + ID() ModuleID +} + +type ModuleID interface { + ModuleName() string + Path() []byte + Address() []byte +} +``` + +### Inter-Module Communication + +```go +func (k keeper) DoSomething(ctx context.Context, req *MsgDoSomething) (*MsgDoSomethingResponse, error) { + // make a query + bankQueryClient := bank.NewQueryClient(sdk.ModuleQueryConn) + res, err := bankQueryClient.Balance(ctx, &QueryBalanceRequest{ + Denom: "foo", + Address: ModuleKeyToBech32Address(k.moduleKey), + }) + + // send a msg + bankMsgClient := bank.NewMsgClient(k.moduleKey) + res, err := bankMsgClient.Send(ctx, &MsgSend{ + FromAddress: ModuleKeyToBech32Address(k.moduleKey), + ToAddress: ..., + Amount: ..., + }) + + // send a msg from a derived module account + derivedKey := k.moduleKey.DerivedKey([]byte("some-sub-pool")) + res, err := bankMsgClient.Send(ctx, &MsgSend{ + FromAddress: ModuleKeyToBech32Address(derivedKey), + ToAddress: ..., + Amount: ..., + }) +} +``` + +### Hooks + +```proto +service Hooks { + rpc AfterValidatorCreated(AfterValidatorCreatedRequest) returns (AfterValidatorCreatedResponse); +} + +message AfterValidatorCreatedRequest { + string validator_address = 1; +} + +message AfterValidatorCreatedResponse { } +``` + + +```go +func (k stakingKeeper) CreateValidator(ctx context.Context, req *MsgCreateValidator) (*MsgCreateValidatorResponse, error) { + ... + + for moduleId := range k.modulesWithHook { + hookClient := NewHooksClient(moduleId) + _, _ := hooksClient.AfterValidatorCreated(ctx, &AfterValidatorCreatedRequest {ValidatorAddress: valAddr}) + } + ... +} +``` + +### Module Registration and Requirements + +```go +type Configurator interface { + MsgServer() grpc.Server + QueryServer() grpc.Server + HooksServer() grpc.Server + + RequireMsgServer(interface{}) + RequireQueryServer(interface{}) + + RequireAdminMsgServer(interface{}) interface{} +} +``` + +## Consequences + +> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. + + +### Backwards Compatibility + +> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. + + +### Positive + +{positive consequences} + +### Negative + +{negative consequences} + +### Neutral + +{neutral consequences} + + +## Test Cases [optional] + +Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. + + +## References + +- {reference link} From 47e47580874f890be4c07625793a6904e0d47132 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 6 Oct 2020 10:58:37 -0400 Subject: [PATCH 02/28] WIP --- docs/architecture/README.md | 1 + docs/architecture/adr-033-protobuf-ocaps.md | 26 +++++++++++++++++---- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index b60a1b38a5f2..11eb7cf03116 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -54,3 +54,4 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 025: IBC Passive Channels](./adr-025-ibc-passive-channels.md) - [ADR 026: IBC Client Recovery Mechanisms](./adr-026-ibc-client-recovery-mechanisms.md) - [ADR 027: Deterministic Protobuf Serialization](./adr-027-deterministic-protobuf-serialization.md) +- [ADR 033: Protobuf Module Object Capabilities](./adr-033-protobuf-ocaps.md) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 83d7d8813c3a..4b669fe66505 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -29,15 +29,31 @@ Proposed ### Module Keys ```go +func Invoker(ctx context.Context, signer ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error + type ModuleKey interface { - DerivedKey(path []byte) ModuleKey + grpc.ClientConn ID() ModuleID } -type ModuleID interface { - ModuleName() string - Path() []byte - Address() []byte +type RootModuleKey struct { + ModuleName string + MsgInvoker Invoker() +} + +type DerivedModuleKey struct { + ModuleName string + Path []byte + MsgInvoker Invoker() +} + +type ModuleID struct { + ModuleName string + Path []byte +} + +func (key ModuleID) Address() []byte { + return AddressHash(key.ModuleName, key.Path) } ``` From e3db2365e1b89587f466af75e3c7980152f3ffcd Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 6 Oct 2020 13:00:12 -0400 Subject: [PATCH 03/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 32 +++++++++++++++------ 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 4b669fe66505..d4f36706b739 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -37,14 +37,14 @@ type ModuleKey interface { } type RootModuleKey struct { - ModuleName string - MsgInvoker Invoker() + moduleName string + msgInvoker Invoker() } type DerivedModuleKey struct { - ModuleName string - Path []byte - MsgInvoker Invoker() + moduleName string + path []byte + msgInvoker Invoker() } type ModuleID struct { @@ -117,17 +117,33 @@ func (k stakingKeeper) CreateValidator(ctx context.Context, req *MsgCreateValida ```go type Configurator interface { + ModuleKey() RootModuleKey + MsgServer() grpc.Server QueryServer() grpc.Server HooksServer() grpc.Server - RequireMsgServer(interface{}) - RequireQueryServer(interface{}) + RequireMsgServer(msgServerInterface interface{}) + RequireQueryServer(queryServerInterface interface{}) +} + +type Provisioner interface { + GetAdminMsgClientConn(msgServerInterface interface{}) grpc.ClientConn + GetPluginClientConn(pluginServerInterface interface{}) func(ModuleID) grpc.ClientConn +} - RequireAdminMsgServer(interface{}) interface{} +type Module interface { + Configure(Configurator) + Provision(Provisioner) +} + +type ModuleManager interface { + GrantAdminAccess(module ModuleID, msgServerInterface interface{}) + GrantPluginAccess(module ModuleID, pluginServerInterface interface{}) } ``` + ## Consequences > This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. From 92a114ea6ea33d4177186521c00cf1b0b5977e5e Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 6 Oct 2020 16:59:06 -0400 Subject: [PATCH 04/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 65 +++++++++++++++++++-- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index d4f36706b739..6a99f359f2bd 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -8,7 +8,6 @@ Proposed - ## Abstract > "If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the ADR. @@ -17,14 +16,70 @@ Proposed ## Context -> This section describes the forces at play, including technological, political, social, and project local. These forces are probably in tension, and should be called out as such. The language in this section is value-neutral. It is simply describing facts. It should clearly explain the problem and motivation that the proposal aims to resolve. -> {context body} +In the current Cosmos SDK documentation on the [Object-Capability Model](../docs/core/ocap.md), it is state that: + +> We assume that a thriving ecosystem of Cosmos-SDK modules that are easy to compose into a blockchain application will contain faulty or malicious modules. + +There is currently not a thriving ecosystem of Cosmos SDK modules. We hypothesize that this is in part due to: +1. lack of a stable v1.0 Cosmos SDK to build modules off of. Module interfaces are changing, sometimes dramatically, from +point release to point release, often for good reasons, but this does not create a stable foundation to build on. +2. lack of a properly implemented object capability or even object-oriented encapsulation system. + +### `x/bank` Case Study + +We use `x/bank as a case study` of this. +Currently the `x/bank` keeper gives pretty much unrestricted access to any module which references it. For instance, the +`SetBalance` method allows the caller to set the balance of any account to anything, bypassing even proper tracking of supply. + +There appears to have been some later attempts to implement some semblance of Ocaps using module-level minting, staking +and burning permissions. These permissions allow a module to mint, burn or delegate tokens with reference to the module’s +own account. These permissions are actually stored as a `[]string` array on the `ModuleAccount` type in state. + +However, these permissions don’t really do much. They control what modules can be referenced in the `MintCoins`, +`BurnCoins` and `DelegateCoins***` methods, but for one there is no unique object capability token that controls access +- just a simple string. So the `x/upgrade` module could mint tokens for the `x/staking` module simple by calling +`MintCoins(“staking”)`. Furthermore, all modules which have access to these keeper methods, also have access to +`SetBalance` negating any level of ocaps or even basic object-oriented encapsulation. ## Decision -> This section describes our response to these forces. It is stated in full sentences, with active voice. "We will ..." -> {decision body} +Starting from the work in [ADR 31](Protobuf Msg Services), we introduce the following inter-module communication system +to replace the existing keeper paradigm. These two pieces together are intended to form the basis of a Cosmos SDK v1.0 +that provides the necessary stability and encapsulation guarantees that allow a thriving module ecosystem to emerge. + +### New "Keeper" Paradigm + +In [ADR 021](./adr-021-protobuf-query-encoding.md), a mechanism for using protobuf service definitions to define queriers +was introduced and in [ADR 31](), protobuf service definition representation of `Msg`s was added. +Protobuf service definitions generate two golang interfaces representing the client and server sides of a service plus +some helper code. Ex: + +```go +package bank + +type MsgClient interface { + Send(context.Context, *MsgSend, opts ...grpc.CallOption) (*MsgSendResponse, error) +} + +type MsgServer interface { + Send(context.Context, *MsgSend) (*MsgSendResponse, error) +} +``` + +[ADR 31](Protobuf Msg Services) specifies how modules can implement the `MsgServer` interface as a replacement for the +legacy handler registration. + +In this ADR we explain how modules can send `Msg`s to other modules using the generated `MsgClient` interfaces and +propose this mechanism as a replacement for the existing `Keeper` paradigm. + +Using this `MsgClient` approach has the following benefits over keepers: +1. Protobuf types are checked for breaking changes using [buf](https://buf.build/docs/breaking-overview) and because of +the way protobuf is designed this will give us strong backwards compatibility guarantees while allowing for forward +evolution. +2. The separation between the client and server interfaces will allow us to insert permission checking code in between +the two which checks if one module is authorized to send the specified `Msg` to the other module providing a proper +object capability system. ### Module Keys From 3ebfae83817341a4320e83e64f3749922b8039eb Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 6 Oct 2020 18:09:28 -0400 Subject: [PATCH 05/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 53 ++++++++++++++++++--- 1 file changed, 47 insertions(+), 6 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 6a99f359f2bd..73059ba5b712 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -67,13 +67,13 @@ type MsgServer interface { } ``` -[ADR 31](Protobuf Msg Services) specifies how modules can implement the `MsgServer` interface as a replacement for the -legacy handler registration. +[ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31]() specifies how modules can implement the generated `QueryServer` +and `MsgServer` interfaces as replacements for the legacy queriers and `Msg` handlers respectively. -In this ADR we explain how modules can send `Msg`s to other modules using the generated `MsgClient` interfaces and -propose this mechanism as a replacement for the existing `Keeper` paradigm. +In this ADR we explain how modules can make queries and send `Msg`s to other modules using the generated `QueryClient` +and `MsgClient` interfaces and propose this mechanism as a replacement for the existing `Keeper` paradigm. -Using this `MsgClient` approach has the following benefits over keepers: +Using this `QueryClient`/`MsgClient` approach has the following key benefits over keepers: 1. Protobuf types are checked for breaking changes using [buf](https://buf.build/docs/breaking-overview) and because of the way protobuf is designed this will give us strong backwards compatibility guarantees while allowing for forward evolution. @@ -81,7 +81,48 @@ evolution. the two which checks if one module is authorized to send the specified `Msg` to the other module providing a proper object capability system. -### Module Keys +This mechanism has the added benefits of: +- reducing boilerplate through code generation, and +- allowing for modules in other languages either through a VM like CosmWasm or sub-processes using gRPC + +In order for code to use the generated `Client` interfaces, a `grpc.ClientConn` implementation is needed. The following +sections will describe the special `grpc.ClientConn` implementations modules will be able to use to make queries and +send `Msg`s to other modules. + +### Inter-module Queries + +Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose +any major security threats assuming basic precautions are taken. The basic precautions identified here are: +- the `sdk.Context` which query methods have access to should not allow writing to the store +- query methods should only be able to make queries against other modules, not send messages +- query methods should not be able to make recursive calls to themselves + +We introduce a singleton `grpc.ClientConn` implementation as the var `sdk.ModuleQueryConn` for making inter-module +queries. It would be used like this from within an example `MsgServer` implementation: + +```go +package foo + +func (msgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { + bankQueryClient := bank.NewQueryClient(sdk.ModuleQueryConn) + res, err := bankQueryClient.Balance(ctx, &QueryBalanceRequest{Denom: "foo", Address: req.Address}) + ... +} +``` + +Under the hood, a query router will be attached to the provided `context.Context` and `sdk.ModuleQueryConn` +will retrieve that query router for routing queries. + +The attached query router will make sure the above security precautions are taken by: +- disabling any ability for `QueryServer` methods to write to the store, +- disabling the ability for `QueryServer` methods to send `Msg`s to other modules, +- keeping track of the call stack of method calls to disable recursion + +### Inter-module Messages + + + +#### Module Keys ```go func Invoker(ctx context.Context, signer ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error From c4e60cecf8ceb2d6013ebb849c7f8a07d7517759 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Thu, 8 Oct 2020 10:23:42 -0400 Subject: [PATCH 06/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 42 +++++++++++---------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 73059ba5b712..39b0fcb6e762 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -85,31 +85,37 @@ This mechanism has the added benefits of: - reducing boilerplate through code generation, and - allowing for modules in other languages either through a VM like CosmWasm or sub-processes using gRPC -In order for code to use the generated `Client` interfaces, a `grpc.ClientConn` implementation is needed. The following -sections will describe the special `grpc.ClientConn` implementations modules will be able to use to make queries and -send `Msg`s to other modules. +### Inter-module Communication -### Inter-module Queries +In order for code to use the `Client` interfaces generated by the protobuf compiler, a `grpc.ClientConn` implementation +is needed. We introduce a new type, `ModuleKey`, to serve this role which we can conceptualize as the "private key" +corresponding to a module account. -Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose -any major security threats assuming basic precautions are taken. The basic precautions identified here are: -- the `sdk.Context` which query methods have access to should not allow writing to the store -- query methods should only be able to make queries against other modules, not send messages -- query methods should not be able to make recursive calls to themselves - -We introduce a singleton `grpc.ClientConn` implementation as the var `sdk.ModuleQueryConn` for making inter-module -queries. It would be used like this from within an example `MsgServer` implementation: +Whereas external clients use their private key to sign transactions containing `Msg`s where they are listed as signers, +modules use their `ModuleKey` to send `Msg`s where they are listed as signers to other modules. For example, modules +could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module to another +account. ```go package foo func (msgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { - bankQueryClient := bank.NewQueryClient(sdk.ModuleQueryConn) - res, err := bankQueryClient.Balance(ctx, &QueryBalanceRequest{Denom: "foo", Address: req.Address}) + bankMsgClient := bank.NewMsgClient(msgServer.moduleKey) + res, err := bankMsgClient.Balance(ctx, &MsgSend{FromAddress: msgServer.moduleKey.Address(), ...}) ... } ``` + +Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose +any major security threats assuming basic precautions are taken. The basic precautions identified here are: +- the `sdk.Context` which query methods have access to should not allow writing to the store +- query methods should only be able to make queries against other modules, not send messages +- query methods should not be able to make recursive calls to themselves + +We introduce a singleton `grpc.ClientConn` implementation as the var `sdk.ModuleQueryConn` for making inter-module +queries. It would be used like this from within an example `MsgServer` implementation: + Under the hood, a query router will be attached to the provided `context.Context` and `sdk.ModuleQueryConn` will retrieve that query router for routing queries. @@ -118,14 +124,10 @@ The attached query router will make sure the above security precautions are take - disabling the ability for `QueryServer` methods to send `Msg`s to other modules, - keeping track of the call stack of method calls to disable recursion -### Inter-module Messages - - - #### Module Keys ```go -func Invoker(ctx context.Context, signer ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error +func Invoker(ctx context.Context, caller ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error type ModuleKey interface { grpc.ClientConn @@ -134,7 +136,7 @@ type ModuleKey interface { type RootModuleKey struct { moduleName string - msgInvoker Invoker() + invoker Invoker() } type DerivedModuleKey struct { From ae0dedec60bddbf144c73871a04520e420487e0b Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Thu, 8 Oct 2020 17:27:53 -0400 Subject: [PATCH 07/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 94 ++++++++++++++++++--- 1 file changed, 83 insertions(+), 11 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 39b0fcb6e762..db4a1f80aebe 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -96,16 +96,93 @@ modules use their `ModuleKey` to send `Msg`s where they are listed as signers to could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module to another account. +`QueryClient`s could also be made with `ModuleKey`s, except that authentication isn't required. + +Ex: ```go package foo -func (msgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { - bankMsgClient := bank.NewMsgClient(msgServer.moduleKey) - res, err := bankMsgClient.Balance(ctx, &MsgSend{FromAddress: msgServer.moduleKey.Address(), ...}) +func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { + bankQueryClient := bank.NewQueryClient(fooMsgServer.moduleKey) + balance, err := bankQueryClient.Balance(&bank.QueryBalanceRequest{Address: fooMsgServer.moduleKey.Address(), Denom: "foo"}) + + ... + + bankMsgClient := bank.NewMsgClient(fooMsgServer.moduleKey) + res, err := bankMsgClient.Balance(ctx, &bank.MsgSend{FromAddress: fooMsgServer.moduleKey.Address(), ...}) + ... } ``` +### `ModuleKey`s and `ModuleID`s + +A `ModuleKey` can be thought of as a "private key" for a module account and a `ModuleID` can be thought of as the +corresponding "public key". From [ADR 028](), modules can have both a root module account and any number of sub-accounts +or derived accounts that can be used for different pools (ex. staking pools) or managed accounts (ex. group +accounts). We can also think of module sub-accounts as similar to derived keys - there is a root key and then some +derivation path. `ModuleID` is a simple struct which contains the module name and optional "derivation" path, +and forms its address based on the `AddressHash` method from [ADR 028](): + +```go +type ModuleID struct { + ModuleName string + Path []byte +} + +func (key ModuleID) Address() []byte { + return AddressHash(key.ModuleName, key.Path) +} +``` + +In addition to being able to generate a `ModuleID` and address, a `ModuleKey` contains a special function closure called +the `Invoker` which is the key to safe inter-module access. This function closure corresponds to the `Invoke` method in +the `grpc.ClientConn` interface and under the hood is able to route messages to the approach `Msg` and `Query` handlers performing +appropriate security checks on `Msg`s. This allows for even safer inter-module access than keeper's whose private member +variables could be manipulated through reflection. Golang does not support reflection of a function closure's captured +variables and direct manipulation of memory would be needed for a truly malicious module to bypass the `ModuleKey` +security. + +The two `ModuleKey` types are `RootModuleKey` and `DerivedModuleKey`: + +```go +func Invoker(ctx context.Context, caller ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error + +type RootModuleKey struct { + moduleName string + invoker Invoker +} + +type DerivedModuleKey struct { + moduleName string + path []byte + invoker Invoker +} +``` + +A module can get access to a `DerivedModuleKey`, using the `Derive(path []byte)` method on `RootModuleKey` and then +would use this key to authenticate `Msg`s from a sub-account: + +```go +package foo + +func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { + derivedKey := fooMsgServer.moduleKey.Derive(req.SomePath) + bankMsgClient := bank.NewMsgClient(derivedKey) + res, err := bankMsgClient.Balance(ctx, &bank.MsgSend{FromAddress: derivedKey.Address(), ...}) + ... +} +``` + +In this way, a module can gain permissioned access to a root account and any number of sub-accounts and send +authenticated `Msg`s from these accounts. The `caller ModuleID` parameter on `Invoker` is used under the hood to +distinguish between different module accounts, but either way the `Invoker` only allows `Msg`s from either the +root or a derived module account to pass through. + +### `AppModule` Wiring + + +### Security Considerations Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose any major security threats assuming basic precautions are taken. The basic precautions identified here are: @@ -145,14 +222,6 @@ type DerivedModuleKey struct { msgInvoker Invoker() } -type ModuleID struct { - ModuleName string - Path []byte -} - -func (key ModuleID) Address() []byte { - return AddressHash(key.ModuleName, key.Path) -} ``` ### Inter-Module Communication @@ -241,6 +310,9 @@ type ModuleManager interface { } ``` +### Future Work + +A separate ADR will address the use cases of "admin" access and inter-module hooks (as used in `x/staking/keeper/hooks.go`). ## Consequences From 646f0dba9f6029d2f49f4aaaa0d539527ce50c09 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Fri, 9 Oct 2020 14:33:29 -0400 Subject: [PATCH 08/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 31 ++++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index db4a1f80aebe..d00271ffc99f 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -146,7 +146,7 @@ security. The two `ModuleKey` types are `RootModuleKey` and `DerivedModuleKey`: ```go -func Invoker(ctx context.Context, caller ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error +func Invoker(ctx context.Context, params InvokeParams, method string, args, reply interface{}, opts ...grpc.CallOption) error type RootModuleKey struct { moduleName string @@ -158,6 +158,10 @@ type DerivedModuleKey struct { path []byte invoker Invoker } + +type InvokeParams struct { + Caller ModuleID +} ``` A module can get access to a `DerivedModuleKey`, using the `Derive(path []byte)` method on `RootModuleKey` and then @@ -175,12 +179,29 @@ func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarRes ``` In this way, a module can gain permissioned access to a root account and any number of sub-accounts and send -authenticated `Msg`s from these accounts. The `caller ModuleID` parameter on `Invoker` is used under the hood to +authenticated `Msg`s from these accounts. The `InvokeParams.Caller` parameter is used under the hood to distinguish between different module accounts, but either way the `Invoker` only allows `Msg`s from either the root or a derived module account to pass through. -### `AppModule` Wiring +### `AppModule` Wiring and Requirements + +In [ADR 031](), the `AppModule.RegisterService(Configurator)` method was introduced. To support inter-module +communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules to specify +their dependencies on other modules using `RequireServer`: + + +```go +type Configurator interface { + QueryServer() grpc.Server + MsgServer() grpc.Server + + ModuleKey() ModuleKey + RequireServer(serverInterface interface{}) +} +``` +The `ModuleKey` is passed to modules in the `RegisterService` method because this is the earliest point at which modules +need it and this ### Security Considerations @@ -312,7 +333,9 @@ type ModuleManager interface { ### Future Work -A separate ADR will address the use cases of "admin" access and inter-module hooks (as used in `x/staking/keeper/hooks.go`). +A separate ADR will address the use cases of unrestricted, "admin" access and inter-module hooks +(as used in `x/staking/keeper/hooks.go`). + ## Consequences From ec2e30b64da16ed22c633bf4ae8fdbc9743cc5eb Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 13 Oct 2020 17:46:55 -0400 Subject: [PATCH 09/28] Update Invoker --- docs/architecture/adr-033-protobuf-ocaps.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index d00271ffc99f..e65c4ae67b97 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -146,7 +146,7 @@ security. The two `ModuleKey` types are `RootModuleKey` and `DerivedModuleKey`: ```go -func Invoker(ctx context.Context, params InvokeParams, method string, args, reply interface{}, opts ...grpc.CallOption) error +func Invoker(method string) func(ctx context.Context, caller ModuleID, request, response interface{}, opts ...interface{}) error type RootModuleKey struct { moduleName string @@ -158,10 +158,6 @@ type DerivedModuleKey struct { path []byte invoker Invoker } - -type InvokeParams struct { - Caller ModuleID -} ``` A module can get access to a `DerivedModuleKey`, using the `Derive(path []byte)` method on `RootModuleKey` and then @@ -179,9 +175,13 @@ func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarRes ``` In this way, a module can gain permissioned access to a root account and any number of sub-accounts and send -authenticated `Msg`s from these accounts. The `InvokeParams.Caller` parameter is used under the hood to -distinguish between different module accounts, but either way the `Invoker` only allows `Msg`s from either the -root or a derived module account to pass through. +authenticated `Msg`s from these accounts. The `Invoker` `caller` parameter is used under the hood to +distinguish between different module accounts, but either way the function returned by `Invoker` only allows `Msg`s +from either the root or a derived module account to pass through. + +Note that `Invoker` returns a function itself based on the method passed in. this will allow client implementations +in the future that cache the invoke function for each method type avoiding the overhead of hash table method lookup, +reducing the overhead of this inter-module communication method to bare minimum required for checking permissions. ### `AppModule` Wiring and Requirements From 3527dcba8c8802bfcf821231fa88365e18f6111e Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 20 Oct 2020 11:03:04 -0400 Subject: [PATCH 10/28] WIP --- docs/architecture/adr-033-protobuf-ocaps.md | 208 ++++++-------------- 1 file changed, 63 insertions(+), 145 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index e65c4ae67b97..8ad1a650e822 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -10,9 +10,9 @@ Proposed ## Abstract -> "If you can't explain it simply, you don't understand it well enough." Provide a simplified and layman-accessible explanation of the ADR. -> A short (~200 word) description of the issue being addressed. - +This ADR introduces a system for permissioned inter-module communication or object capabilites (Ocaps) leveraging the +protobuf `Query` and `Msg` service definitions defined in [ADR 021](./adr-021-protobuf-query-encoding.md) and +[ADR 031](./adr-031-msg-service.md). ## Context @@ -40,20 +40,20 @@ However, these permissions don’t really do much. They control what modules can `BurnCoins` and `DelegateCoins***` methods, but for one there is no unique object capability token that controls access - just a simple string. So the `x/upgrade` module could mint tokens for the `x/staking` module simple by calling `MintCoins(“staking”)`. Furthermore, all modules which have access to these keeper methods, also have access to -`SetBalance` negating any level of ocaps or even basic object-oriented encapsulation. +`SetBalance` negating any other attempt at Ocaps and breaking even basic object-oriented encapsulation. ## Decision -Starting from the work in [ADR 31](Protobuf Msg Services), we introduce the following inter-module communication system +Starting from the work in [ADR 31](./adr-031-msg-service.md), we introduce the following inter-module communication system to replace the existing keeper paradigm. These two pieces together are intended to form the basis of a Cosmos SDK v1.0 that provides the necessary stability and encapsulation guarantees that allow a thriving module ecosystem to emerge. ### New "Keeper" Paradigm In [ADR 021](./adr-021-protobuf-query-encoding.md), a mechanism for using protobuf service definitions to define queriers -was introduced and in [ADR 31](), protobuf service definition representation of `Msg`s was added. +was introduced and in [ADR 31](./adr-031-msg-service.md), a mechanism for using protobuf service to define `Msg`s was added. Protobuf service definitions generate two golang interfaces representing the client and server sides of a service plus -some helper code. Ex: +some helper code. Here is a minimal example for the bank `Send` `Msg`. ```go package bank @@ -83,7 +83,7 @@ object capability system. This mechanism has the added benefits of: - reducing boilerplate through code generation, and -- allowing for modules in other languages either through a VM like CosmWasm or sub-processes using gRPC +- allowing for modules in other languages either via a VM like CosmWasm or sub-processes using gRPC ### Inter-module Communication @@ -92,13 +92,13 @@ is needed. We introduce a new type, `ModuleKey`, to serve this role which we can corresponding to a module account. Whereas external clients use their private key to sign transactions containing `Msg`s where they are listed as signers, -modules use their `ModuleKey` to send `Msg`s where they are listed as signers to other modules. For example, modules -could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module to another +modules use their `ModuleKey` to send `Msg`s where they are listed as the sole signer to other modules. For example, modules +could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module's account to another account. `QueryClient`s could also be made with `ModuleKey`s, except that authentication isn't required. -Ex: +Here's an example of a hypothetical module `foo` interacting with `x/bank`: ```go package foo @@ -137,16 +137,21 @@ func (key ModuleID) Address() []byte { In addition to being able to generate a `ModuleID` and address, a `ModuleKey` contains a special function closure called the `Invoker` which is the key to safe inter-module access. This function closure corresponds to the `Invoke` method in -the `grpc.ClientConn` interface and under the hood is able to route messages to the approach `Msg` and `Query` handlers performing -appropriate security checks on `Msg`s. This allows for even safer inter-module access than keeper's whose private member -variables could be manipulated through reflection. Golang does not support reflection of a function closure's captured -variables and direct manipulation of memory would be needed for a truly malicious module to bypass the `ModuleKey` -security. +the `grpc.ClientConn` interface and under the hood is able to route messages to the appropriate `Msg` and `Query` handlers +performing appropriate security checks on `Msg`s. This allows for even safer inter-module access than keeper's whose +private member variables could be manipulated through reflection. Golang does not support reflection on a function +closure's captured variables and direct manipulation of memory would be needed for a truly malicious module to bypass +the `ModuleKey` security. The two `ModuleKey` types are `RootModuleKey` and `DerivedModuleKey`: ```go -func Invoker(method string) func(ctx context.Context, caller ModuleID, request, response interface{}, opts ...interface{}) error +func Invoker(callInfo CallInfo) func(ctx context.Context, request, response interface{}, opts ...interface{}) error + +type CallInfo { + Method string + Caller ModuleID +} type RootModuleKey struct { moduleName string @@ -161,7 +166,7 @@ type DerivedModuleKey struct { ``` A module can get access to a `DerivedModuleKey`, using the `Derive(path []byte)` method on `RootModuleKey` and then -would use this key to authenticate `Msg`s from a sub-account: +would use this key to authenticate `Msg`s from a sub-account. Ex: ```go package foo @@ -175,19 +180,20 @@ func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarRes ``` In this way, a module can gain permissioned access to a root account and any number of sub-accounts and send -authenticated `Msg`s from these accounts. The `Invoker` `caller` parameter is used under the hood to +authenticated `Msg`s from these accounts. The `Invoker` `callInfo.Caller` parameter is used under the hood to distinguish between different module accounts, but either way the function returned by `Invoker` only allows `Msg`s from either the root or a derived module account to pass through. -Note that `Invoker` returns a function itself based on the method passed in. this will allow client implementations -in the future that cache the invoke function for each method type avoiding the overhead of hash table method lookup, -reducing the overhead of this inter-module communication method to bare minimum required for checking permissions. +Note that `Invoker` itself returns a function closure based on the `CallInfo` passed in. This will allow client implementations +in the future that cache the invoke function for each method type avoiding the overhead of hash table lookup. +This would reduce the performance overhead of this inter-module communication method to the bare minimum required for +checking permissions. ### `AppModule` Wiring and Requirements -In [ADR 031](), the `AppModule.RegisterService(Configurator)` method was introduced. To support inter-module -communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules to specify -their dependencies on other modules using `RequireServer`: +In [ADR 031](./adr-031-msg-service.md), the `AppModule.RegisterService(Configurator)` method was introduced. To support +inter-module communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules t +specify their dependencies on other modules using `RequireServer`: ```go @@ -200,148 +206,60 @@ type Configurator interface { } ``` -The `ModuleKey` is passed to modules in the `RegisterService` method because this is the earliest point at which modules -need it and this - -### Security Considerations - -Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose -any major security threats assuming basic precautions are taken. The basic precautions identified here are: -- the `sdk.Context` which query methods have access to should not allow writing to the store -- query methods should only be able to make queries against other modules, not send messages -- query methods should not be able to make recursive calls to themselves - -We introduce a singleton `grpc.ClientConn` implementation as the var `sdk.ModuleQueryConn` for making inter-module -queries. It would be used like this from within an example `MsgServer` implementation: - -Under the hood, a query router will be attached to the provided `context.Context` and `sdk.ModuleQueryConn` -will retrieve that query router for routing queries. - -The attached query router will make sure the above security precautions are taken by: -- disabling any ability for `QueryServer` methods to write to the store, -- disabling the ability for `QueryServer` methods to send `Msg`s to other modules, -- keeping track of the call stack of method calls to disable recursion - -#### Module Keys - -```go -func Invoker(ctx context.Context, caller ModuleID, method string, args, reply interface{}, opts ...grpc.CallOption) error - -type ModuleKey interface { - grpc.ClientConn - ID() ModuleID -} - -type RootModuleKey struct { - moduleName string - invoker Invoker() -} - -type DerivedModuleKey struct { - moduleName string - path []byte - msgInvoker Invoker() -} - -``` - -### Inter-Module Communication - -```go -func (k keeper) DoSomething(ctx context.Context, req *MsgDoSomething) (*MsgDoSomethingResponse, error) { - // make a query - bankQueryClient := bank.NewQueryClient(sdk.ModuleQueryConn) - res, err := bankQueryClient.Balance(ctx, &QueryBalanceRequest{ - Denom: "foo", - Address: ModuleKeyToBech32Address(k.moduleKey), - }) - - // send a msg - bankMsgClient := bank.NewMsgClient(k.moduleKey) - res, err := bankMsgClient.Send(ctx, &MsgSend{ - FromAddress: ModuleKeyToBech32Address(k.moduleKey), - ToAddress: ..., - Amount: ..., - }) - - // send a msg from a derived module account - derivedKey := k.moduleKey.DerivedKey([]byte("some-sub-pool")) - res, err := bankMsgClient.Send(ctx, &MsgSend{ - FromAddress: ModuleKeyToBech32Address(derivedKey), - ToAddress: ..., - Amount: ..., - }) -} -``` - -### Hooks - -```proto -service Hooks { - rpc AfterValidatorCreated(AfterValidatorCreatedRequest) returns (AfterValidatorCreatedResponse); -} - -message AfterValidatorCreatedRequest { - string validator_address = 1; -} - -message AfterValidatorCreatedResponse { } -``` +The `ModuleKey` is passed to modules in the `RegisterService` method itself so that `RegisterServices` serves as a single +entry point for configuring module services. This is intended to also have the side-effect of reducing boilerplate in +app.go. For now, `ModuleKey`s will be created based on `AppModuleBasic.Name()`, but a more flexible system may be +introduced in the future. The `ModuleManager` will handle creation of module accounts behind the scenes. +Because modules do not get direct access to each other any more, modules may have unfulfilled dependencies. To make sure +that module dependencies are resolved at startup, the `Configurator.RequireServer` method is added. The `ModuleManager` +will make sure that all dependencies declared with `RequireServer` can be resolved before the app starts. An example +module `foo` could declare it's dependency on `x/bank` like this: ```go -func (k stakingKeeper) CreateValidator(ctx context.Context, req *MsgCreateValidator) (*MsgCreateValidatorResponse, error) { - ... +package foo - for moduleId := range k.modulesWithHook { - hookClient := NewHooksClient(moduleId) - _, _ := hooksClient.AfterValidatorCreated(ctx, &AfterValidatorCreatedRequest {ValidatorAddress: valAddr}) - } - ... +func (am AppModule) RegisterServices(cfg Configurator) { + cfg.RequireServer((*bank.QueryServer)(nil)) + cfg.RequireServer((*bank.MsgServer)(nil)) } ``` -### Module Registration and Requirements +### Security Considerations -```go -type Configurator interface { - ModuleKey() RootModuleKey +In addition to checking for `ModuleKey` permissions, a few additional security precautions will need to be taken - MsgServer() grpc.Server - QueryServer() grpc.Server - HooksServer() grpc.Server +#### Recursion and Re-entry - RequireMsgServer(msgServerInterface interface{}) - RequireQueryServer(queryServerInterface interface{}) -} +Methods which call other methods which eventually call the method which called them are a potential security threat. -type Provisioner interface { - GetAdminMsgClientConn(msgServerInterface interface{}) grpc.ClientConn - GetPluginClientConn(pluginServerInterface interface{}) func(ModuleID) grpc.ClientConn -} +A simple way for the router system to deal with this is to maintain a call stack which prevents a module from +being referenced more than once in the call stack. A simple `map[string]interface{}` table in the router could be use to +efficiently perform this security check. -type Module interface { - Configure(Configurator) - Provision(Provisioner) -} +#### Queries -type ModuleManager interface { - GrantAdminAccess(module ModuleID, msgServerInterface interface{}) - GrantPluginAccess(module ModuleID, pluginServerInterface interface{}) -} -``` +Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose +any major security threats assuming basic precautions are taken. The basic precautions that the router system will +need to take is making sure that the `sdk.Context` passed to query methods does not allow writing to the store. This +can be done with a cache wrapper. ### Future Work -A separate ADR will address the use cases of unrestricted, "admin" access and inter-module hooks -(as used in `x/staking/keeper/hooks.go`). +Separate ADRs will address the use cases of: +* unrestricted, "admin" access +* dynamic interface handler routing (ex. `x/gov` `Content` routing) +* inter-module hooks (as used in `x/staking/keeper/hooks.go`) +Other future improvements may include: +* combining `StoreKey`s and `ModuleKey`s into a single interface so that modules have a single Ocaps handle +* code generation which makes inter-module communication more performant +* decoupling `ModuleKey` creation from `AppModuleBasic.Name()` so that app's can override root module account names ## Consequences > This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. - ### Backwards Compatibility > All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. From 426b729c0486bae53329fd1ad7562e5573ab32cb Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 20 Oct 2020 17:20:50 -0400 Subject: [PATCH 11/28] Tidying up --- docs/architecture/adr-033-protobuf-ocaps.md | 57 ++++++++++----------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index 8ad1a650e822..ee12c5094d05 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -118,11 +118,11 @@ func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarRes ### `ModuleKey`s and `ModuleID`s A `ModuleKey` can be thought of as a "private key" for a module account and a `ModuleID` can be thought of as the -corresponding "public key". From [ADR 028](), modules can have both a root module account and any number of sub-accounts +corresponding "public key". From [the ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086), modules can have both a root module account and any number of sub-accounts or derived accounts that can be used for different pools (ex. staking pools) or managed accounts (ex. group accounts). We can also think of module sub-accounts as similar to derived keys - there is a root key and then some derivation path. `ModuleID` is a simple struct which contains the module name and optional "derivation" path, -and forms its address based on the `AddressHash` method from [ADR 028](): +and forms its address based on the `AddressHash` method from [the ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086): ```go type ModuleID struct { @@ -192,8 +192,8 @@ checking permissions. ### `AppModule` Wiring and Requirements In [ADR 031](./adr-031-msg-service.md), the `AppModule.RegisterService(Configurator)` method was introduced. To support -inter-module communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules t -specify their dependencies on other modules using `RequireServer`: +inter-module communication, we extend the `Configurator` interface to pass in the `ModuleKey` and to allow modules to +specify their dependencies on other modules using `RequireServer()`: ```go @@ -208,11 +208,11 @@ type Configurator interface { The `ModuleKey` is passed to modules in the `RegisterService` method itself so that `RegisterServices` serves as a single entry point for configuring module services. This is intended to also have the side-effect of reducing boilerplate in -app.go. For now, `ModuleKey`s will be created based on `AppModuleBasic.Name()`, but a more flexible system may be +`app.go`. For now, `ModuleKey`s will be created based on `AppModuleBasic.Name()`, but a more flexible system may be introduced in the future. The `ModuleManager` will handle creation of module accounts behind the scenes. -Because modules do not get direct access to each other any more, modules may have unfulfilled dependencies. To make sure -that module dependencies are resolved at startup, the `Configurator.RequireServer` method is added. The `ModuleManager` +Because modules do not get direct access to each other anymore, modules may have unfulfilled dependencies. To make sure +that module dependencies are resolved at startup, the `Configurator.RequireServer` method should be added. The `ModuleManager` will make sure that all dependencies declared with `RequireServer` can be resolved before the app starts. An example module `foo` could declare it's dependency on `x/bank` like this: @@ -227,29 +227,31 @@ func (am AppModule) RegisterServices(cfg Configurator) { ### Security Considerations -In addition to checking for `ModuleKey` permissions, a few additional security precautions will need to be taken +In addition to checking for `ModuleKey` permissions, a few additional security precautions will need to be taken by +the underlying router infrastructure. #### Recursion and Re-entry -Methods which call other methods which eventually call the method which called them are a potential security threat. +Recursive or re-entrant method invocations pose a potential security threat. This can be a problem if Module A +calls Module B and Module B calls module A again in the same call. -A simple way for the router system to deal with this is to maintain a call stack which prevents a module from -being referenced more than once in the call stack. A simple `map[string]interface{}` table in the router could be use to -efficiently perform this security check. +One basic way for the router system to deal with this is to maintain a call stack which prevents a module from +being referenced more than once in the call stack so that there is no re-entry. A `map[string]interface{}` table +in the router could be used to perform this security check. #### Queries Queries in Cosmos SDK are generally un-permissioned so allowing one module to query another module should not pose -any major security threats assuming basic precautions are taken. The basic precautions that the router system will +any major security threats assuming basic precautions are taken. The basic precaution that the router system will need to take is making sure that the `sdk.Context` passed to query methods does not allow writing to the store. This -can be done with a cache wrapper. +can be done for now with a `CacheMultiStore` as is currently done for `BaseApp` queries. ### Future Work Separate ADRs will address the use cases of: * unrestricted, "admin" access -* dynamic interface handler routing (ex. `x/gov` `Content` routing) -* inter-module hooks (as used in `x/staking/keeper/hooks.go`) +* dynamic interface routing (ex. `x/gov` `Content` routing) +* inter-module hooks (ex. `x/staking/keeper/hooks.go`) Other future improvements may include: * combining `StoreKey`s and `ModuleKey`s into a single interface so that modules have a single Ocaps handle @@ -258,31 +260,28 @@ Other future improvements may include: ## Consequences -> This section describes the resulting context, after applying the decision. All consequences should be listed here, not just the "positive" ones. A particular decision may have positive, negative, and neutral consequences, but all of them affect the team and project in the future. - ### Backwards Compatibility -> All ADRs that introduce backwards incompatibilities must include a section describing these incompatibilities and their severity. The ADR must explain how the author proposes to deal with these incompatibilities. ADR submissions without a sufficient backwards compatibility treatise may be rejected outright. - +This ADR is intended to provide a pathway to a scenario where there is greater long term compatibility between modules. +In the short-term, this will likely result in breaking certain `Keeper` interfaces which are too permissive and/or +replacing `Keeper` interfaces altogether. ### Positive -{positive consequences} +- proper inter-module Ocaps +- an alternative to keepers which can more easily lead to stable inter-module interfaces ### Negative -{negative consequences} +- modules which adopt this will need significant refactoring ### Neutral -{neutral consequences} - - ## Test Cases [optional] -Test cases for an implementation are mandatory for ADRs that are affecting consensus changes. Other ADRs can choose to include links to test cases if applicable. - - ## References -- {reference link} +- [ADR 021](./adr-021-protobuf-query-encoding.md) +- [ADR 031](./adr-031-msg-service.md) +- [ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086) +- [Object-Capability Model](../docs/core/ocap.md) \ No newline at end of file From 929ecacb322d5b8d8dea84b7273d7191092e0d62 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Tue, 20 Oct 2020 17:25:08 -0400 Subject: [PATCH 12/28] Update title --- docs/architecture/README.md | 2 +- docs/architecture/adr-033-protobuf-ocaps.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 73820206c97b..f300d51e8d30 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -57,4 +57,4 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 029: Fee Grant Module](./adr-029-fee-grant-module.md) - [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md) - [ADR 032: Typed Events](./adr-032-typed-events.md) -- [ADR 033: Protobuf Module Object Capabilities](./adr-033-protobuf-ocaps.md) +- [ADR 033: Protobuf-based Inter-Module Object Capabilities](./adr-033-protobuf-ocaps.md) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-ocaps.md index ee12c5094d05..7490abffe787 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-ocaps.md @@ -1,4 +1,4 @@ -# ADR 033: Protobuf Module Object Capabilities +# ADR 033: Protobuf-based Inter-Module Object Capabilities ## Changelog From 314ca766d8d1c45c35b6316deec6ba9b23bf28f7 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 21 Oct 2020 10:25:08 -0400 Subject: [PATCH 13/28] Cleanup diff From eba46fee912e610ffdd47129e554465a5c47de9b Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 21 Oct 2020 10:26:13 -0400 Subject: [PATCH 14/28] cleanup --- store/types/store.go | 3 +-- x/staking/types/exported.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/store/types/store.go b/store/types/store.go index c166be83de0d..f78fcad3f402 100644 --- a/store/types/store.go +++ b/store/types/store.go @@ -7,10 +7,9 @@ import ( abci "github.com/tendermint/tendermint/abci/types" dbm "github.com/tendermint/tm-db" - tmstrings "github.com/tendermint/tendermint/libs/strings" - snapshottypes "github.com/cosmos/cosmos-sdk/snapshots/types" "github.com/cosmos/cosmos-sdk/types/kv" + tmstrings "github.com/tendermint/tendermint/libs/strings" ) type Store interface { diff --git a/x/staking/types/exported.go b/x/staking/types/exported.go index 19b4d1dc3546..02f97dccea49 100644 --- a/x/staking/types/exported.go +++ b/x/staking/types/exported.go @@ -1,9 +1,8 @@ package types import ( - "github.com/tendermint/tendermint/crypto" - sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/tendermint/tendermint/crypto" ) // DelegationI delegation bond for a delegated proof of stake system From e2de5c134abc857d13ad89ad94b83d483a7ae7fe Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 21 Oct 2020 16:34:05 -0400 Subject: [PATCH 15/28] Update context --- docs/architecture/README.md | 2 +- ....md => adr-033-protobuf-inter-module-comm.md} | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) rename docs/architecture/{adr-033-protobuf-ocaps.md => adr-033-protobuf-inter-module-comm.md} (95%) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index f300d51e8d30..65e4c0977545 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -57,4 +57,4 @@ Please add a entry below in your Pull Request for an ADR. - [ADR 029: Fee Grant Module](./adr-029-fee-grant-module.md) - [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md) - [ADR 032: Typed Events](./adr-032-typed-events.md) -- [ADR 033: Protobuf-based Inter-Module Object Capabilities](./adr-033-protobuf-ocaps.md) +- [ADR 033: Protobuf-based Inter-Module Communication](./adr-033-protobuf-inter-module-comm.md) diff --git a/docs/architecture/adr-033-protobuf-ocaps.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md similarity index 95% rename from docs/architecture/adr-033-protobuf-ocaps.md rename to docs/architecture/adr-033-protobuf-inter-module-comm.md index 7490abffe787..86ea83823a28 100644 --- a/docs/architecture/adr-033-protobuf-ocaps.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -1,4 +1,4 @@ -# ADR 033: Protobuf-based Inter-Module Object Capabilities +# ADR 033: Protobuf-based Inter-Module Communication ## Changelog @@ -10,9 +10,12 @@ Proposed ## Abstract -This ADR introduces a system for permissioned inter-module communication or object capabilites (Ocaps) leveraging the -protobuf `Query` and `Msg` service definitions defined in [ADR 021](./adr-021-protobuf-query-encoding.md) and -[ADR 031](./adr-031-msg-service.md). +This ADR introduces a system for permissioned inter-module communication leveraging the protobuf `Query` and `Msg` +service definitions defined in [ADR 021](./adr-021-protobuf-query-encoding.md) and +[ADR 031](./adr-031-msg-service.md) which provides: +- stable module interfaces to eventually replace the keeper paradigm based on protobuf +- stronger inter-module object capabilities guarantees +- module accounts and sub-account authorization ## Context @@ -23,11 +26,12 @@ In the current Cosmos SDK documentation on the [Object-Capability Model](../docs There is currently not a thriving ecosystem of Cosmos SDK modules. We hypothesize that this is in part due to: 1. lack of a stable v1.0 Cosmos SDK to build modules off of. Module interfaces are changing, sometimes dramatically, from point release to point release, often for good reasons, but this does not create a stable foundation to build on. -2. lack of a properly implemented object capability or even object-oriented encapsulation system. +2. lack of a properly implemented object capability or even object-oriented encapsulation system which makes refactors +of module keeper interfaces inevitable because the current interfaces are poorly constrained. ### `x/bank` Case Study -We use `x/bank as a case study` of this. +We use `x/bank` of this. Currently the `x/bank` keeper gives pretty much unrestricted access to any module which references it. For instance, the `SetBalance` method allows the caller to set the balance of any account to anything, bypassing even proper tracking of supply. From 40cc92ba827e2cd766356df30a0dfe0b526cc3af Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 21 Oct 2020 16:44:58 -0400 Subject: [PATCH 16/28] Add ClientConn impl --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 86ea83823a28..6425373ab681 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -193,6 +193,15 @@ in the future that cache the invoke function for each method type avoiding the o This would reduce the performance overhead of this inter-module communication method to the bare minimum required for checking permissions. +Below is a rough sketch of the implementation of `grpc.ClientConn.Invoke` for `RootModuleKey`: + +```go +func (key RootModuleKey) Invoke(ctx context.Context, method string, args, reply interface{}, opts ...grpc.CallOption) error { + f := key.invoker(CallInfo {Method: method, Caller: ModuleID {ModuleName: key.moduleName}}) + return f(ctx, args, reply) +} +``` + ### `AppModule` Wiring and Requirements In [ADR 031](./adr-031-msg-service.md), the `AppModule.RegisterService(Configurator)` method was introduced. To support From f644baa92d9e1a2392404e6816832d79d5939ffa Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Fri, 23 Oct 2020 09:12:42 -0400 Subject: [PATCH 17/28] WIP --- .../adr-033-protobuf-inter-module-comm.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 6425373ab681..f8548cb1df97 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -1,4 +1,4 @@ -# ADR 033: Protobuf-based Inter-Module Communication +# ADR 033: Protobuf-based Inter-Module RPC ## Changelog @@ -259,12 +259,15 @@ any major security threats assuming basic precautions are taken. The basic preca need to take is making sure that the `sdk.Context` passed to query methods does not allow writing to the store. This can be done for now with a `CacheMultiStore` as is currently done for `BaseApp` queries. -### Future Work +### Authorization + +TODO -Separate ADRs will address the use cases of: -* unrestricted, "admin" access -* dynamic interface routing (ex. `x/gov` `Content` routing) -* inter-module hooks (ex. `x/staking/keeper/hooks.go`) +### Hooks + +TODO + +### Future Work Other future improvements may include: * combining `StoreKey`s and `ModuleKey`s into a single interface so that modules have a single Ocaps handle @@ -281,8 +284,8 @@ replacing `Keeper` interfaces altogether. ### Positive -- proper inter-module Ocaps - an alternative to keepers which can more easily lead to stable inter-module interfaces +- proper inter-module Ocaps ### Negative From 2bf21a612f0c0d451193ccc4d97dee4736783569 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 23 Nov 2020 10:40:10 -0500 Subject: [PATCH 18/28] updates --- docs/architecture/README.md | 1 + ...inter-module-comm.md => adr-033-protobuf-inter-module-rpc.md} | 0 go.mod | 1 - 3 files changed, 1 insertion(+), 1 deletion(-) rename docs/architecture/{adr-033-protobuf-inter-module-comm.md => adr-033-protobuf-inter-module-rpc.md} (100%) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index d3ff03368873..2fe35a0594c9 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -71,5 +71,6 @@ Read about the [PROCESS](./PROCESS.md). - [ADR 028: Public Key Addresses](./adr-028-public-key-addresses.md) - [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md) - [ADR 032: Typed Events](./adr-032-typed-events.md) +- [ADR 033: Inter-module RPC](./adr-033-protobuf-inter-module-rpc.md) - [ADR 035: Rosetta API Support](./adr-035-rosetta-api-support.md) - [ADR 037: Governance Split Votes](./adr-037-gov-split-vote.md) \ No newline at end of file diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-rpc.md similarity index 100% rename from docs/architecture/adr-033-protobuf-inter-module-comm.md rename to docs/architecture/adr-033-protobuf-inter-module-rpc.md diff --git a/go.mod b/go.mod index 5c05b40b717c..39e28042489b 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,6 @@ require ( github.com/golang/mock v1.4.4 github.com/golang/protobuf v1.4.3 github.com/golang/snappy v0.0.2 // indirect - github.com/google/go-cmp v0.5.0 github.com/gorilla/handlers v1.5.1 github.com/gorilla/mux v1.8.0 github.com/grpc-ecosystem/grpc-gateway v1.16.0 From 59963f184bd273ee238ff0e2b42c070b9c66db97 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 23 Nov 2020 10:41:02 -0500 Subject: [PATCH 19/28] revert go.mod From c991c02e0de58de41d9a6b1d8e16eca0a79af414 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 23 Nov 2020 11:00:09 -0500 Subject: [PATCH 20/28] Describe internal methods and authorization --- .../adr-033-protobuf-inter-module-rpc.md | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-rpc.md b/docs/architecture/adr-033-protobuf-inter-module-rpc.md index f8548cb1df97..cea7224fc6f1 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-rpc.md +++ b/docs/architecture/adr-033-protobuf-inter-module-rpc.md @@ -1,4 +1,4 @@ -# ADR 033: Protobuf-based Inter-Module RPC +# ADR 033: Protobuf-based Inter-Module Communication ## Changelog @@ -211,8 +211,8 @@ specify their dependencies on other modules using `RequireServer()`: ```go type Configurator interface { - QueryServer() grpc.Server MsgServer() grpc.Server + QueryServer() grpc.Server ModuleKey() ModuleKey RequireServer(serverInterface interface{}) @@ -259,20 +259,39 @@ any major security threats assuming basic precautions are taken. The basic preca need to take is making sure that the `sdk.Context` passed to query methods does not allow writing to the store. This can be done for now with a `CacheMultiStore` as is currently done for `BaseApp` queries. -### Authorization +### Internal Methods + +In many cases, we may wish for modules to call methods on other modules which are not exposed to clients at all. For this +purpose, we add the `InternalServer` method to `Configurator`: + +```go +type Configurator interface { + MsgServer() grpc.Server + QueryServer() grpc.Server + InternalServer() grpc.Server +} +``` -TODO +Services registered against `InternalServer` will be callable from other modules but not by external clients. -### Hooks +### Authorization -TODO +By default, the inter-module router requires that modules are sent by the first signer returned by `GetSigners`. The +inter-module router should also accept authorization middleware such as that provided by [ADR 030](https://github.com/cosmos/cosmos-sdk/pull/7105). +This middleware will allow accounts to otherwise specific module accounts to perform actions on their behalf. +Authorization middleware should take into account the need to grant certain modules effectively "admin" privileges to +other modules. This will be addressed in separate ADRs or updates to this ADR. ### Future Work Other future improvements may include: +* custom code generation that: + * simplifies interfaces (ex. generates code with `sdk.Context` instead of `context.Context`) + * optimizes inter-module calls - for instance caching resolved methods after first invocation * combining `StoreKey`s and `ModuleKey`s into a single interface so that modules have a single Ocaps handle * code generation which makes inter-module communication more performant * decoupling `ModuleKey` creation from `AppModuleBasic.Name()` so that app's can override root module account names +* inter-module hooks and plugins ## Consequences @@ -299,5 +318,6 @@ replacing `Keeper` interfaces altogether. - [ADR 021](./adr-021-protobuf-query-encoding.md) - [ADR 031](./adr-031-msg-service.md) -- [ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086) +- [ADR 028](./adr-028-public-key-addresses.md) +- [ADR 030 draft](https://github.com/cosmos/cosmos-sdk/pull/7105) - [Object-Capability Model](../docs/core/ocap.md) \ No newline at end of file From c9722d3e0887fedc35cf7d99a44a1b5386f56d39 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 23 Nov 2020 12:47:54 -0500 Subject: [PATCH 21/28] Add comparison to x/capability --- .../adr-033-protobuf-inter-module-rpc.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/docs/architecture/adr-033-protobuf-inter-module-rpc.md b/docs/architecture/adr-033-protobuf-inter-module-rpc.md index cea7224fc6f1..97e63af46440 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-rpc.md +++ b/docs/architecture/adr-033-protobuf-inter-module-rpc.md @@ -293,6 +293,23 @@ Other future improvements may include: * decoupling `ModuleKey` creation from `AppModuleBasic.Name()` so that app's can override root module account names * inter-module hooks and plugins +## Alternatives + +The `x/capability` module does provide a proper object-capability implementation that can be used by any module in the +SDK and could even be used for inter-module Ocaps as described in [\#5931](https://github.com/cosmos/cosmos-sdk/issues/5931). + +The advantages of the approach described in this ADR are mostly around how it integrates with other parts of the SDK, +specifically: + +* protobuf so that: + * code generation of interfaces can be leveraged for a better dev UX + * module interfaces are versioned and checked for breakage using [buf](https://docs.buf.build/breaking-overview) +* sub-module accounts as per ADR 028 +* the general `Msg` passing paradigm and the way signers are specified by `GetSigners` + +Also, this a complete replacement for keepers and could be applied to _all_ inter-module communication whereas the +`x/capability` approach in #5931 would need to be applied method by method. + ## Consequences ### Backwards Compatibility From 08fb6b774f356ff392a0a3c88ec93569c67d4e25 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Mon, 23 Nov 2020 12:52:00 -0500 Subject: [PATCH 22/28] Cleanup --- docs/architecture/README.md | 2 +- ...dule-rpc.md => adr-033-protobuf-inter-module-comm.md} | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) rename docs/architecture/{adr-033-protobuf-inter-module-rpc.md => adr-033-protobuf-inter-module-comm.md} (96%) diff --git a/docs/architecture/README.md b/docs/architecture/README.md index 2fe35a0594c9..64ce5377b29d 100644 --- a/docs/architecture/README.md +++ b/docs/architecture/README.md @@ -71,6 +71,6 @@ Read about the [PROCESS](./PROCESS.md). - [ADR 028: Public Key Addresses](./adr-028-public-key-addresses.md) - [ADR 031: Protobuf Msg Services](./adr-031-msg-service.md) - [ADR 032: Typed Events](./adr-032-typed-events.md) -- [ADR 033: Inter-module RPC](./adr-033-protobuf-inter-module-rpc.md) +- [ADR 033: Inter-module RPC](architecture/adr-033-protobuf-inter-module-comm.md) - [ADR 035: Rosetta API Support](./adr-035-rosetta-api-support.md) - [ADR 037: Governance Split Votes](./adr-037-gov-split-vote.md) \ No newline at end of file diff --git a/docs/architecture/adr-033-protobuf-inter-module-rpc.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md similarity index 96% rename from docs/architecture/adr-033-protobuf-inter-module-rpc.md rename to docs/architecture/adr-033-protobuf-inter-module-comm.md index 97e63af46440..de53a1539aca 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-rpc.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -48,9 +48,10 @@ However, these permissions don’t really do much. They control what modules can ## Decision -Starting from the work in [ADR 31](./adr-031-msg-service.md), we introduce the following inter-module communication system -to replace the existing keeper paradigm. These two pieces together are intended to form the basis of a Cosmos SDK v1.0 -that provides the necessary stability and encapsulation guarantees that allow a thriving module ecosystem to emerge. +Starting from the work in [ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31](./adr-031-msg-service.md), we introduce +the following inter-module communication system to replace the existing keeper paradigm. These pieces are +intended to form the basis of a Cosmos SDK v1.0 that provides the necessary stability and encapsulation guarantees +that allow a thriving module ecosystem to emerge. ### New "Keeper" Paradigm @@ -71,7 +72,7 @@ type MsgServer interface { } ``` -[ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31]() specifies how modules can implement the generated `QueryServer` +[ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31](./adr-031-msg-service.md) specifies how modules can implement the generated `QueryServer` and `MsgServer` interfaces as replacements for the legacy queriers and `Msg` handlers respectively. In this ADR we explain how modules can make queries and send `Msg`s to other modules using the generated `QueryClient` From 142da70b76cefa483133ecd4ab6dec5d8a8919c8 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 25 Nov 2020 09:41:27 -0500 Subject: [PATCH 23/28] Apply suggestions from code review Co-authored-by: Robert Zaremba --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index de53a1539aca..5d27f82fc1cc 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -31,8 +31,6 @@ of module keeper interfaces inevitable because the current interfaces are poorly ### `x/bank` Case Study -We use `x/bank` of this. - Currently the `x/bank` keeper gives pretty much unrestricted access to any module which references it. For instance, the `SetBalance` method allows the caller to set the balance of any account to anything, bypassing even proper tracking of supply. @@ -338,4 +336,4 @@ replacing `Keeper` interfaces altogether. - [ADR 031](./adr-031-msg-service.md) - [ADR 028](./adr-028-public-key-addresses.md) - [ADR 030 draft](https://github.com/cosmos/cosmos-sdk/pull/7105) -- [Object-Capability Model](../docs/core/ocap.md) \ No newline at end of file +- [Object-Capability Model](../docs/core/ocap.md) From 18fc5571e957a61d54a700396cfdfb00acab0757 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 25 Nov 2020 09:42:01 -0500 Subject: [PATCH 24/28] Update docs/architecture/adr-033-protobuf-inter-module-comm.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 5d27f82fc1cc..8ce336a3a1b0 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -139,7 +139,7 @@ func (key ModuleID) Address() []byte { ``` In addition to being able to generate a `ModuleID` and address, a `ModuleKey` contains a special function closure called -the `Invoker` which is the key to safe inter-module access. This function closure corresponds to the `Invoke` method in +the `Invoker` which is the key to safe inter-module access. The `InvokeFn` closure corresponds to the `Invoke` method in the `grpc.ClientConn` interface and under the hood is able to route messages to the appropriate `Msg` and `Query` handlers performing appropriate security checks on `Msg`s. This allows for even safer inter-module access than keeper's whose private member variables could be manipulated through reflection. Golang does not support reflection on a function From 994e270b6026f7265be423d3c0afa27e5c978b83 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 25 Nov 2020 09:42:22 -0500 Subject: [PATCH 25/28] Update docs/architecture/adr-033-protobuf-inter-module-comm.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 8ce336a3a1b0..7529d411f2df 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -306,7 +306,7 @@ specifically: * sub-module accounts as per ADR 028 * the general `Msg` passing paradigm and the way signers are specified by `GetSigners` -Also, this a complete replacement for keepers and could be applied to _all_ inter-module communication whereas the +Also, this is a complete replacement for keepers and could be applied to _all_ inter-module communication whereas the `x/capability` approach in #5931 would need to be applied method by method. ## Consequences From d9c90cf56095bb24e089e1c8baf3756e87aad121 Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 25 Nov 2020 09:42:40 -0500 Subject: [PATCH 26/28] Update docs/architecture/adr-033-protobuf-inter-module-comm.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 7529d411f2df..98ba2311fc36 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -275,7 +275,7 @@ Services registered against `InternalServer` will be callable from other modules ### Authorization -By default, the inter-module router requires that modules are sent by the first signer returned by `GetSigners`. The +By default, the inter-module router requires that messages are sent by the first signer returned by `GetSigners`. The inter-module router should also accept authorization middleware such as that provided by [ADR 030](https://github.com/cosmos/cosmos-sdk/pull/7105). This middleware will allow accounts to otherwise specific module accounts to perform actions on their behalf. Authorization middleware should take into account the need to grant certain modules effectively "admin" privileges to From aa35c168bdf7cdd68308ee11b06150a835e4101b Mon Sep 17 00:00:00 2001 From: Aaron Craelius Date: Wed, 25 Nov 2020 09:44:02 -0500 Subject: [PATCH 27/28] Update docs/architecture/adr-033-protobuf-inter-module-comm.md Co-authored-by: Robert Zaremba --- docs/architecture/adr-033-protobuf-inter-module-comm.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 98ba2311fc36..79abcecb28a1 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -121,7 +121,7 @@ func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarRes ### `ModuleKey`s and `ModuleID`s A `ModuleKey` can be thought of as a "private key" for a module account and a `ModuleID` can be thought of as the -corresponding "public key". From [the ADR 028 draft](https://github.com/cosmos/cosmos-sdk/pull/7086), modules can have both a root module account and any number of sub-accounts +corresponding "public key". From the [ADR 028](./adr-028-public-key-addresses.md), modules can have both a root module account and any number of sub-accounts or derived accounts that can be used for different pools (ex. staking pools) or managed accounts (ex. group accounts). We can also think of module sub-accounts as similar to derived keys - there is a root key and then some derivation path. `ModuleID` is a simple struct which contains the module name and optional "derivation" path, From bb56c515e1a1628e4c66db2ab9421e233685967d Mon Sep 17 00:00:00 2001 From: Cory Date: Thu, 17 Dec 2020 19:40:29 -0800 Subject: [PATCH 28/28] ADR-033 Updates / Amendments (#8190) * updates to ADR33 based on 12.04.20 architecture call * add comments about improved devx * Update docs/architecture/adr-033-protobuf-inter-module-comm.md Co-authored-by: Aaron Craelius * update with atomicity note Co-authored-by: Aaron Craelius --- .../adr-033-protobuf-inter-module-comm.md | 66 ++++++++++++++----- 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/docs/architecture/adr-033-protobuf-inter-module-comm.md b/docs/architecture/adr-033-protobuf-inter-module-comm.md index 79abcecb28a1..8321afa6d63b 100644 --- a/docs/architecture/adr-033-protobuf-inter-module-comm.md +++ b/docs/architecture/adr-033-protobuf-inter-module-comm.md @@ -13,7 +13,7 @@ Proposed This ADR introduces a system for permissioned inter-module communication leveraging the protobuf `Query` and `Msg` service definitions defined in [ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 031](./adr-031-msg-service.md) which provides: -- stable module interfaces to eventually replace the keeper paradigm based on protobuf +- stable protobuf based module interfaces to potentially later replace the keeper paradigm - stronger inter-module object capabilities guarantees - module accounts and sub-account authorization @@ -39,17 +39,22 @@ and burning permissions. These permissions allow a module to mint, burn or deleg own account. These permissions are actually stored as a `[]string` array on the `ModuleAccount` type in state. However, these permissions don’t really do much. They control what modules can be referenced in the `MintCoins`, -`BurnCoins` and `DelegateCoins***` methods, but for one there is no unique object capability token that controls access -- just a simple string. So the `x/upgrade` module could mint tokens for the `x/staking` module simple by calling +`BurnCoins` and `DelegateCoins***` methods, but for one there is no unique object capability token that controls access — +just a simple string. So the `x/upgrade` module could mint tokens for the `x/staking` module simple by calling `MintCoins(“staking”)`. Furthermore, all modules which have access to these keeper methods, also have access to `SetBalance` negating any other attempt at Ocaps and breaking even basic object-oriented encapsulation. ## Decision Starting from the work in [ADR 021](./adr-021-protobuf-query-encoding.md) and [ADR 31](./adr-031-msg-service.md), we introduce -the following inter-module communication system to replace the existing keeper paradigm. These pieces are -intended to form the basis of a Cosmos SDK v1.0 that provides the necessary stability and encapsulation guarantees -that allow a thriving module ecosystem to emerge. +the following inter-module communication system as an new paradigm for secure module based authorization and OCAPS +framework. When implemented, this could also serve as an alternative the existing paradigm of passing keepers between +modules. The approach outlined here-in is intended to form the basis of a Cosmos SDK v1.0 that provides the necessary +stability and encapsulation guarantees that allow a thriving module ecosystem to emerge. + +Of particular note — the decision is to _enable_ this functionality for modules to adopt at their own discretion. +Proposals to migrate existing modules to this new paradigm will have to be a separate conversation, potentially +addressed as amendments to this ADR. ### New "Keeper" Paradigm @@ -74,7 +79,9 @@ type MsgServer interface { and `MsgServer` interfaces as replacements for the legacy queriers and `Msg` handlers respectively. In this ADR we explain how modules can make queries and send `Msg`s to other modules using the generated `QueryClient` -and `MsgClient` interfaces and propose this mechanism as a replacement for the existing `Keeper` paradigm. +and `MsgClient` interfaces and propose this mechanism as a replacement for the existing `Keeper` paradigm. To be clear, +this ADR does not necessitate the creation of new protobuf definitions or services. Rather, it leverages the same proto +based service interfaces already used by clients for inter-module communication. Using this `QueryClient`/`MsgClient` approach has the following key benefits over keepers: 1. Protobuf types are checked for breaking changes using [buf](https://buf.build/docs/breaking-overview) and because of @@ -83,6 +90,9 @@ evolution. 2. The separation between the client and server interfaces will allow us to insert permission checking code in between the two which checks if one module is authorized to send the specified `Msg` to the other module providing a proper object capability system. +3. The router for inter-module communication gives us a convenient place to handle rollback of transactions, +enabling atomicy of operations ([currently a problem](https://github.com/cosmos/cosmos-sdk/issues/8030)). Any failure within a module-to-module call would result in a failure of the entire +transaction This mechanism has the added benefits of: - reducing boilerplate through code generation, and @@ -90,34 +100,37 @@ This mechanism has the added benefits of: ### Inter-module Communication -In order for code to use the `Client` interfaces generated by the protobuf compiler, a `grpc.ClientConn` implementation -is needed. We introduce a new type, `ModuleKey`, to serve this role which we can conceptualize as the "private key" -corresponding to a module account. +To use the `Client` generated by the protobuf compiler we need a `grpc.ClientConn` implementation. For this we introduce +a new type, `ModuleKey`, which implements the `grpc.ClientConn` interface. `ModuleKey` can be thought of as the "private +key" corresponding to a module account, where authentication is provided through use of a special `Invoker()` function, +described in more detail below. -Whereas external clients use their private key to sign transactions containing `Msg`s where they are listed as signers, +Whereas external clients use their account's private key to sign transactions containing `Msg`s where they are listed as signers, modules use their `ModuleKey` to send `Msg`s where they are listed as the sole signer to other modules. For example, modules could use their `ModuleKey` to "sign" a `/cosmos.bank.Msg/Send` transaction to send coins from the module's account to another account. -`QueryClient`s could also be made with `ModuleKey`s, except that authentication isn't required. - Here's an example of a hypothetical module `foo` interacting with `x/bank`: ```go package foo -func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBar) (*MsgBarResponse, error) { +func (fooMsgServer *MsgServer) Bar(ctx context.Context, req *MsgBarRequest) (*MsgBarResponse, error) { bankQueryClient := bank.NewQueryClient(fooMsgServer.moduleKey) balance, err := bankQueryClient.Balance(&bank.QueryBalanceRequest{Address: fooMsgServer.moduleKey.Address(), Denom: "foo"}) ... bankMsgClient := bank.NewMsgClient(fooMsgServer.moduleKey) - res, err := bankMsgClient.Balance(ctx, &bank.MsgSend{FromAddress: fooMsgServer.moduleKey.Address(), ...}) + res, err := bankMsgClient.Send(ctx, &bank.MsgSendRequest{FromAddress: fooMsgServer.moduleKey.Address(), ...}) ... } ``` +This design is also intended to be extensible to cover use cases of more fine grained permissioning like minting by +denom prefix being restricted to certain modules (as discussed in +[#7459](https://github.com/cosmos/cosmos-sdk/pull/7459#discussion_r529545528)). + ### `ModuleKey`s and `ModuleID`s A `ModuleKey` can be thought of as a "private key" for a module account and a `ModuleID` can be thought of as the @@ -192,6 +205,9 @@ in the future that cache the invoke function for each method type avoiding the o This would reduce the performance overhead of this inter-module communication method to the bare minimum required for checking permissions. +To re-iterate, the closure only allows access to authorized calls. There is no access to anything else regardless of any +name impersonation. + Below is a rough sketch of the implementation of `grpc.ClientConn.Invoke` for `RootModuleKey`: ```go @@ -219,7 +235,7 @@ type Configurator interface { ``` The `ModuleKey` is passed to modules in the `RegisterService` method itself so that `RegisterServices` serves as a single -entry point for configuring module services. This is intended to also have the side-effect of reducing boilerplate in +entry point for configuring module services. This is intended to also have the side-effect of greatly reducing boilerplate in `app.go`. For now, `ModuleKey`s will be created based on `AppModuleBasic.Name()`, but a more flexible system may be introduced in the future. The `ModuleManager` will handle creation of module accounts behind the scenes. @@ -271,8 +287,18 @@ type Configurator interface { } ``` +As an example, x/slashing's Slash must call x/staking's Slash, but we don't want to expose x/staking's Slash to end users +and clients. + +This wound also require creating a corresponding `internal.proto` file with a protobuf service in the given module's +proto package. + Services registered against `InternalServer` will be callable from other modules but not by external clients. +An alternative solution to internal-only methods could involve hooks / plugins as discussed [here](https://github.com/cosmos/cosmos-sdk/pull/7459#issuecomment-733807753). +A more detailed evaluation of a hooks / plugin system will be addressed later in follow-ups to this ADR or as a separate +ADR. + ### Authorization By default, the inter-module router requires that messages are sent by the first signer returned by `GetSigners`. The @@ -294,6 +320,8 @@ Other future improvements may include: ## Alternatives +### MsgServices vs `x/capability` + The `x/capability` module does provide a proper object-capability implementation that can be used by any module in the SDK and could even be used for inter-module Ocaps as described in [\#5931](https://github.com/cosmos/cosmos-sdk/issues/5931). @@ -321,6 +349,10 @@ replacing `Keeper` interfaces altogether. - an alternative to keepers which can more easily lead to stable inter-module interfaces - proper inter-module Ocaps +- improved module developer DevX, as commented on by several particpants on + [Architecture Review Call, Dec 3](https://hackmd.io/E0wxxOvRQ5qVmTf6N_k84Q) +- lays the groundwork for what can be a greatly simplified `app.go` +- router can be setup to enforce atomic transactions for moule-to-module calls ### Negative @@ -336,4 +368,4 @@ replacing `Keeper` interfaces altogether. - [ADR 031](./adr-031-msg-service.md) - [ADR 028](./adr-028-public-key-addresses.md) - [ADR 030 draft](https://github.com/cosmos/cosmos-sdk/pull/7105) -- [Object-Capability Model](../docs/core/ocap.md) +- [Object-Capability Model](../docs/core/ocap.md) \ No newline at end of file