Add Tx Signers information to the Context in AnteHandler

[ethanfrey]

Summary

Expose the information of who signed the transaction in the context, so all Handlers can refer to that.

Problem Definition

The only permission checks happen in the AnteHandler, which uses GetSigners() on all Msgs to determine which permissions are needed. This works fine for relatively stateless permissions, when you can determine needed signers from the Msg content, but fails when the needed permissions are stored in the data store. To solve that, we can keep the current checks, but also add the signer info to the context, so Handlers can choose to implement stateful permission checks.

Proposal

The ante handler calculates the signer addresses here:

cosmos-sdk/x/auth/ante/ante.go

Line 111 in fb3e0af

signerAddrs := stdTx.GetSigners()

And validates them here:

cosmos-sdk/x/auth/ante/ante.go

Lines 136 to 153 in fb3e0af

	for i := 0; i < len(stdSigs); i++ {
	// skip the fee payer, account is cached and fees were deducted already
	if i != 0 {
	signerAccs[i], res = GetSignerAcc(newCtx, ak, signerAddrs[i])
	if !res.IsOK() {
	return newCtx, res, true
	}
	}
	// check signature, return account with incremented nonce
	signBytes := GetSignBytes(newCtx.ChainID(), stdTx, signerAccs[i], isGenesis)
	signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytes, simulate, params, sigGasConsumer)
	if !res.IsOK() {
	return newCtx, res, true
	}
	ak.SetAccount(newCtx, signerAccs[i])
	}

Simply adding a line before the return newCtx = newCtx.WithSigners(signerAddrs) and adding the method/field to weave.Context: signers []sdk.Address would expose this info, so handlers can check it.

Adding a few helpers like hasAddr(Context, sdk.Address) bool or hasNAddr(Context, []sdk.Address, int) to check for an exact match or N of M matches would enable module developers to easily add custom permission checks.

---

For Admin Use

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

[ethanfrey]

@haasted This would fix your use case I believe. Please give me some feedback here.

[haasted]

Yes, I believe this describes my need. Thanks for capturing it in an issue.

To summarize in different words: I have a situation where the validity of a signature on a message depends on the current state of the entire chain.

A simple example would be in an issuance module, where the fact that an account is allowed to mint new tokens must be registered somewhere. The fact that the account has that permission cannot be contained merely in the transaction. It needs some context. It's possible to imagine a scenario where a list of special accounts are maintained through chain governance. Validating a transacation consequently involves checking that the signer is actually on the list.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[tac0turtle]

still relevant.

[ethanfrey]

It should be a minor change. I would love to see this land soon

[ethanfrey]

I could make a PR if there is support

[tac0turtle]

I am in support of this change.

@aaronc @robert-zaremba @AmauryM do you have thoughts?

[aaronc]

I'm wondering how this relates with authz and group module routing. For instance, when an authorized Msg gets routed would the signers on the Context get replaced?

Also I'm a little unclear with the SDK's current architecture for which use cases it's relevant for a handler to know about the signers of other Msgs in the tx, and if that's even a good idea. I mean theoretically a handler can decode the whole transaction but where should a handler pay attention the the whole tx vs just it's Msg? It seems like we're potentially inviting weird state machine behaviors that could act strangely especially with authz and group routing.

[ethanfrey]

You could take a page from weave's book and just append that there. Basically, a list of all accounts that have authorized the transaction.

Reflecting more on the recent ibc conversations that motivated re opening, just exposing fee payer (the actual payer if using fee grants) may be enough. And should be simpler to reason about.

[robert-zaremba]

I like the idea - sounds like a simplification. If a authz is routing a Msg, it means it's authorizing that message. If we don't want to mess signers, we could have another set: Granters or Authorizers.

[robert-zaremba]

a handler to know about the signers of other Msgs in the tx, and if that's even a good idea.

In fact, that could be a desired feature - it will allow to make Atomic operations and inspect that message1 can only be validated when message2=MsgSend(....), and message3=MsgDelegate(...) is included in transaction as well. It will enable a more advanced composability. I can start a discussion topic about this.

[robert-zaremba]

Discussion: Group Msg Patter: expose tx messages introspection #9244

[aaronc]

Reflecting more on the recent ibc conversations that motivated re opening, just exposing fee payer (the actual payer if using fee grants) may be enough. And should be simpler to reason about.

Can you share more about these discussions and the current use cases @ethanfrey ?

[iramiller]

Adding a comment in support to help me track this issue...

Provenance does quite a bit of multi-party work with our hybrid on/off chain "client execution environment". We have been building with multiple signers per message however the idea of aggregating the different messages to accrue the list of signers could open up some interesting use cases.

[ethanfrey]

Reflecting more on the recent ibc conversations that motivated re opening, just exposing fee payer (the actual payer if using fee grants) may be enough. And should be simpler to reason about.

Can you share more about these discussions and the current use cases @ethanfrey ?

You should check with IG for the current state, but there is desire to actually optionally pay relayers. This would minimally require that the ibctransfer module be aware of who paid the fees (I figured signer, but honestly fee payer is probably more correct). I believe @charleenfei is checking for a legal opinion and will own any changes there.

[aaronc]

My general preference is that AnteDecorators (which I believe should actually be full on decorators/middleware) are the place where the full Tx should be available, and that Msg handlers should just process Msgs with minimal information about where they came from. This is because a Tx is not the only source for Msgs. They can come from CosmWasm, x/authz, x/group and eventually x/gov, someone could write a cron module which dispatches Msgs in EndBlockers, etc.

Specifically with regards to exposing all the signers, I often experience new SDK developers struggling to understand how authentication works and looking for some method on sdk.Context to get the signers. If we add such a method, I wouldn't be surprised if people incorrectly try to use it for authentication.

Anyway, I wonder if AnteDecorators/Tx middleware would serve these use cases equally well.

[charleenfei]

thanks @ethanfrey for the pointer here. Yes, I am speaking to two different law firms to get a solid legal opinion about the design space for relayer incentives payments. I will share these analyses as they come in. As @ethanfrey pointed out, exposing the tx signers information to the context is one way to allow for dynamic payments for relayers.

I would need to dig more deeply into the code to understand what AnteDecorator/Tx middleware would look like, and if that would provide similar solvency for this use case, but preliminarily that does seem to be a more complex change. The worries about new SDK developers are well noted, if but I wonder if more documentation about the correct way to authenticate might also be one way to educate new developers against using signers from this method for auth. I guess my question would be if @aaronc you see enough of a technical issue with this approach that would warrant implementation of a more complex solution, or if we should try to go ahead with this simple fix for now?

[alessio]

I could make a PR if there is support

Yes, please press ahead 🙏

[charleenfei]

Adding additional context for the relayer tip model that this issue relates to here and summarising off band discussions for transparency:

the use case that this proposed change is related to would allow for relayer tip payments on ICS20 token transfers by giving the ibc-transfer module visibility on the signer of a MsgTransfer (in this case, by exposing the signer in the context). This would not cover tips associated with other types of ibc-transfers, which will be more complicated because we don't have direct access to tokens from those transfers.

After discussion with @aaronc @alexanderbez @ethanfrey and @alessio, there has been a suggestion by @ethanfrey to allow for this visibility rather by passing the signer down through the message server handler in the ibc module, as opposed to exposing the signer in the context. this seems to be a clean solution which would solve for this use case and not mix concerns in the context. this solution will be opened in a separate issue.

@ethanfrey please feel free to clarify or add to my comments if I have misconstrued anything.

[julienrbrt]

This seems resolved per @charleenfei comment. If you still think this should be implemented, please re-open.