Implement external signer for dataposter #1919
Conversation
cla-bot
bot
added
the
s
arbnode/dataposter/data_poster.go
Outdated
| // According to the "eth_signTransaction" API definition, this shoul be | ||
| // RLP encoded transaction object. | ||
| // https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_signtransaction | ||
| var rlpEncTxStr string |
There was a problem hiding this comment.
We can make this a hexutil.Bytes to simplify decoding
| if err := rlp.DecodeBytes(data, &signedTx); err != nil { | ||
| return nil, fmt.Errorf("error decoding signed transaction: %w", err) | ||
| } | ||
| return &signedTx, nil |
There was a problem hiding this comment.
I'd like to add a safety check that this is the same transaction we requested to be signed. I think there's potential security issues without that, where a remote signer MITM returns a different tx with a bad signature, and then convinces the data poster to request that different tx be signed against the real remote signer.
The ideal way to do this would be to get a geth types.Signer (really confusingly named, it's more of a signature verifier than a signer, it doesn't have a private key or anything), and use its Hash method which returns the pre-signature hash, to verify that aside from the signature the txs are the same. I think we can get a "signer" by calling types.LatestSignerForChainID with the parent chain chain id from the config (we'll probably add that as a constructor argument to the data poster).
There was a problem hiding this comment.
Good point! Added the check, although doing the lazy initialization, otherwise passing the signer (or chain ID) all the way from nitro.go would unnecessarily pollute call chain.
arbnode/dataposter/data_poster.go
Outdated
| type ExternalSignerCfg struct { | ||
| // If enabled, this overrides transaction options and uses external signer | ||
| // for signing transactions. | ||
| Enabled bool `koanf:"enabled"` |
There was a problem hiding this comment.
How about we enable the external signer when the URL is set at all?
There was a problem hiding this comment.
Sure, sounds good, changed to that.
arbnode/dataposter/data_poster.go
Outdated
| // RLP encoded transaction object. | ||
| // https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_signtransaction | ||
| var rlpEncTxStr string | ||
| if err := client.CallContext(ctx, &rlpEncTxStr, "eth_signTransaction", tx); err != nil { |
There was a problem hiding this comment.
I think this should use opts.Method
Co-authored-by: Joshua Colvin <[email protected]>
arbnode/dataposter/data_poster.go
Outdated
| @@ -770,7 +772,7 @@ var DefaultDataPosterConfig = DataPosterConfig{ | |||
| UseNoOpStorage: false, | |||
| LegacyStorageEncoding: true, | |||
| Dangerous: DangerousConfig{ClearDBStorage: false}, | |||
| ExternalSigner: ExternalSignerCfg{Enabled: false, Method: "eth_signTransaction"}, | |||
| ExternalSigner: ExternalSignerCfg{}, | |||
There was a problem hiding this comment.
I think we should still keep the default method as eth_signTransaction as that seems to be the most common method name (also applies to DefaultDataPosterConfigForValidator)
There was a problem hiding this comment.
Done. ( I guess you meant TestDataPosterConfig not DefaultDataPosterConfigForValidator)
There was a problem hiding this comment.
Yeah, I got confused by the github diff
No description provided.