x/authz completeness audit

[robert-zaremba]

Summary

Related to: #8982

API x/authz Readiness Audit

Goal: provide summary of finding and overall recommendations.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[robert-zaremba]

Below an audit which covers all parts of the checklist except the x/authz/client and x/spec:

API notes

method_name vs message_name

In Cosmos we talk about Messages, not methods. Also, in ADR we rather define Messages than Methods.
However, the Authorization interface defines MethodName:

// MethodName returns the fully-qualified Msg service method name as described in ADR 031.
MethodName() string

Moreover the GenericAuthorization creates a confusion with it's method_name attribute:

message GenericAuthorization {
  option (cosmos_proto.implements_interface) = "Authorization";

  // method name to grant unrestricted permissions to execute
  // Note: MethodName() is already a method on `GenericAuthorization` type,
  // we need some custom naming here so using `MessageName`
  string method_name = 1 [(gogoproto.customname) = "MessageName"];
}

We have same confusion in sdktypes.ServiceMsg.

Proposal:

• In the interface, use: MessageName
• In the proto file use message instead of method_name

SendAuthorization

• the bank.SendAuthorization.Accept can only use type assertion rather than reflection check.

GetGrantAuthorization

The MsgGrantAuthorizationRequest.GetGrantAuthorization is overwhelmingly long and repeating the parent type. It's a helper method to get cached value of the MsgGrantAuthorizationRequest.Authorization.

Proposal: rename GetGrantAuthorization to GetAuthorization.

Events

• in events we are using types.AttributeValueCategory to refer to module name, and it's used for sdk.AttributeKeyModule event attribute. The AttributeValueCategory is only used with events, and it's name is not clear that it refers to module. Recommendation: use types.ModuleName directly, and remove types.AttributeValueCategory.

Keeper

• Keeper methods have msgType parameter, but in the spec we talk about message name or method name.
  Recommendation: rename msgType to msgName.

• Maybe GetOrRevokeAuthorization should be renamed? We updated documentation, but didn't find a better name.

• types.ExtractAddressesFromGrantKey and types.GetAuthorizationStoreKey functions are private to keeper logic, so should be defined in keeper package, rather than types.

• In keeper.GrantAuthorization, in line 25 (authorization := msg.GetGrantAuthorization()) we don't check for nil.
  Recommendation: GetGrantAuthorization should return error whenever the any is not unpacked or cache value is nil.

Authorization.Accept

The implementation of this method is not in accordance to the ADR. ADR specifies it as:

Accept(msg sdk.ServiceMsg, block abci.Header) (allow bool, updated Authorization, delete bool)

but the Authorization type has (note return values):

Accept(msg sdk.ServiceMsg, block tmproto.Header) (updated Authorization, delete bool, err error)

Moreover, in the ADR we read:

// + delete: true if Authorization has been exhausted and can be deleted from state

but the keeper.DispatchActions will delete and continue the execution as if the authorization is still granted. (or maybe this is not specified correctly).

Suggestion:
Extend the Accept interface return types to (note: the allow and delete attributes):

Accept(msg sdk.ServiceMsg, block abci.Header) (allow, delete bool, updated Authorization, err error)

And error should not be returned on "unauthorized" - this will allow for example to delete a grant, if it should be deleted when the authorization is not valid / exhausted already.

Directory structure

exported package

Don't use exported package name - it's confusing. The package defines two interface: Keeper and Authorization and it's meant to be easily imported by other modules, without creating import cycles. Note: it's seams that the x/authz follow the practice used in other modules to have the exported package.
The problem is that files in x/authz/types are importing almost all other types: x/*/types*, which is causing the import cycles.

Recommendation:

1. Try to relay on interfaces as much as possible.
2. Rename exported package. Normally we would put it into the module root directory, but this won't work, because we have a AppModule defined there, which imports types package.
   • Solution (a): move AppModule code to x/authz/module and move exported to x/authz
   • Otherwise keep as is.

Fees

• check if event emitter is charging fees: keeper.Grant, keeper.Revoke
• authz Exec should have a small fee for checking and routing messages. Currently It only charges for reading and updating the state.

To verify:

• Is storage iterator applying fees correctly?

Undocumented behavior

There is only one possible authz Grant per (grantee, granter, Message) triple.
Question: if this not OK, then we can consider generating an ID for each grant Message, and use the ID Exec and Revoke instead of grantee and granter information.

[aaronc]

API notes

method_name vs message_name

In Cosmos we talk about Messages, not methods. Also, in ADR we rather define Messages than Methods.
However, the Authorization interface defines MethodName:

// MethodName returns the fully-qualified Msg service method name as described in ADR 031.
MethodName() string

Moreover the GenericAuthorization creates a confusion with it's method_name attribute:

message GenericAuthorization {
  option (cosmos_proto.implements_interface) = "Authorization";

  // method name to grant unrestricted permissions to execute
  // Note: MethodName() is already a method on `GenericAuthorization` type,
  // we need some custom naming here so using `MessageName`
  string method_name = 1 [(gogoproto.customname) = "MessageName"];
}

We have same confusion in sdktypes.ServiceMsg.

Proposal:

• In the interface, use: MessageName
• In the proto file use message instead of method_name

I don't actually agree with this. In protobuf message means every "struct" type. We have never really described SDK Msg's using "message" but rather just the terse "Msg". MethodName is the fully qualified service method name and to me feels more descriptive than MessageName which could either be a generic protobuf message or Cosmos SDK Msg.

To be completely honest my preference would be to move away from Msg within Cosmos which has never felt right to me, but if MethodName is not acceptable here we can maybe use MsgName.

Also, IMHO in ServiceMsg, MethodName is correct - it is the fully qualified service method name.

[robert-zaremba]

@aaronc, yes, agree, these terms are overloaded and proto.Message is not sdk.Msg nor handler. My main concern is in:

   string method_name = 1 [(gogoproto.customname) = "MessageName"];

Also see the comment above that field in the OP. If we have too much confusions here, then let's find another, completely different name. This is neither a method name nor message name. It's a name / id of authorized RPC. How about: RPCName or MsgRPC?
Also we should rename the MethodName in the Authorization` interface:

type Authorization interface {
        // ....
	// MethodName returns the fully-qualified Msg service method name as described in ADR 031.
	MethodName() string
}

I would call it RefRPC (from referenced RPC), or RefServiceMethod.

---

We have never really described SDK Msg's using "message" but rather just the terse "Msg"

In documentation and code we user "Message" term a lot (eg: building-modules/messages-and-queries.md, and proto.Message is used to reference any Msg* type).

[anilcse]

I believe there's no confusion in using MethodName here. May be MessageName is bit ambiguous as we might get confused with proto.MessageName(msg) sometimes. We can may be rename it to MsgName or MsgType as that is what it is referring to. RefServiceMethod or RefRPC aren't self explanatory.

[anilcse]

One more thing confusing is GrantAuthorization and AuthorizationGrant.

// AuthorizationGrant gives permissions to execute
// the provide method with expiration time.
message AuthorizationGrant {
  google.protobuf.Any       authorization = 1 [(cosmos_proto.accepts_interface) = "Authorization"];
  google.protobuf.Timestamp expiration    = 2 [(gogoproto.stdtime) = true, (gogoproto.nullable) = false];
}

GrantAuthorization looks self explanatory but AuthorizatoinGrant is bit misleading. I think AuthorizationBody sounds better. Also should update the description.
Option-2: may be we can rename the interface to AuthorizationI and rename AuthorizationGrant as Authorization

[anilcse]

Simulations audit report is here: https://hackmd.io/s/BkMemp4B_

[robert-zaremba]

One more thing confusing is GrantAuthorization and AuthorizationGrant.

How about renaming AuthorizationGrant -> Grant.

[aaronc]

Based on #9063, let's:

• rename MethodName -> MessageName
• use the type URLs specified in mitigation 2 in ADR-031: reconsider type url format for service msgs encoded as google.Protobuf.Any #9063
• rename MsgExecAuthorizedRequest, etc. -> MsgExecAuthorized, etc.

[robert-zaremba]

Fees and spam attack analysis

• keeper.GetAuthorizations only accumulates storage cost. The loop is small - only appending. Recommendation: we can consider accumulating a small additional value which will represent the return value.
  Alternatively we can consider charging gas based on the size of return bytes.

• IterateGrants is dangerous because it iterates over all grants. This method must not be exposed in anything except Geensis Export. It seams we don't have any good practice for that in the SDK. Recommendation: add a chargeGas: uint parameter to the IterateGrants. If not zero, we will charge extra gas on each iteration step. We don't have to charge a lot because we only collect elements.
  Alternatively, as above, we can consider charging based on returned bytes size.

• Charge extra flat fee per message handler call (k.DispatchActions) in ExecAuthorized

[aaronc]

There is only one possible authz Grant per (grantee, granter, Message) triple.
Question: if this not OK, then we can consider generating an ID for each grant Message, and use the ID Exec and Revoke instead of grantee and granter information.

@robert-zaremba, I can think of one reason not to do this which is that it complicates calling Exec. With the current API, it's easier for callers just to assume that they're authorized to do something by the granter and do it. If we add an ID, then they need to first look up the authorization, get the ID, and then call. This is a small added effort but it adds complexity that I'm not sure if merited by any use case I've seen. And it if it's needed it can be supported by simply creating an Authorization type that composes multiple authorizations rather than introducing ID's to the core API. So my preference would be to keep the scoping to a single (grantee, granter, Message) triple.

[robert-zaremba]

Summary of changes

• Proto: string method_name = 1 [(gogoproto.customname) = "MessageName"]
  rename to msg_type_url

• use type assertion -> nice to have

Protobuf

• MSG: Rename RPC -> remove Authoziation suffix
• QUERY RPC: removed Authorizaion RPC
• Create proto types for events: EventGrant, EventRevoke, EventExec with same attributes

Keeper:

• rename funtcion argument msgType to msgTypeURL
• rename GetOrRevokeAuthorization to GetAuthorization
• types.ExtractAddressesFromGrantKey and types.GetAuthorizationStoreKey functions are private to keeper logic, so should be defined in keeper package, rather than types. -> OK
• In keeper.GrantAuthorization, in line 25 (authorization := msg.GetGrantAuthorization()) we don't check for nil.
  Recommendation: GetGrantAuthorization should return error whenever the any is not unpacked or cache value is nil. -> OK
• IterateGrants -> this should be private and let's move Import / Export Genesis functions to keeper.

Events: use proto events instead of legacy events