Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement external signer for dataposter #1919

Merged
merged 16 commits into from
Oct 17, 2023
Merged

Implement external signer for dataposter #1919

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
a619d43
Implement external signer for dataposter
anodar Oct 13, 2023
62801eb
Merge branch 'master' into external-signer
anodar Oct 13, 2023
4269470
merge master
anodar Oct 13, 2023
c3ed563
Drop accidentally added server binary for manual testing
anodar Oct 13, 2023
c177f02
Fix linter errors
anodar Oct 13, 2023
fa2f6fa
Merge branch 'master' into external-signer
anodar Oct 13, 2023
2f7b867
Make signing method configurable to allow both eth_signTransaction an…
anodar Oct 13, 2023
cfb4bc7
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 13, 2023
c27cb4b
Update arbnode/dataposter/data_poster.go
anodar Oct 16, 2023
73eb315
Merge branch 'master' into external-signer
anodar Oct 16, 2023
4661888
Check that signed transaction is the one sent for signing
anodar Oct 16, 2023
3816204
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 16, 2023
c551ff5
List transactions in correct order in the log line
anodar Oct 16, 2023
412a7e0
Merge branch 'master' into external-signer
anodar Oct 17, 2023
34fe2d0
Add default method for external signer
anodar Oct 17, 2023
1e9b588
Merge branch 'external-signer' of github.com:OffchainLabs/nitro into …
anodar Oct 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions arbnode/dataposter/data_poster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ func parseReplacementTimes(val string) ([]time.Duration, error) {
lastReplacementTime = t
}
if len(res) == 0 {
log.Warn("disabling replace-by-fee for data poster")
log.Warn("Disabling replace-by-fee for data poster")
}
// To avoid special casing "don't replace again", replace in 10 years.
return append(res, time.Hour*24*365*10), nil
Expand Down Expand Up @@ -160,7 +160,7 @@ func NewDataPoster(ctx context.Context, opts *DataPosterOpts) (*DataPoster, erro
redisLock: opts.RedisLock,
errorCount: make(map[uint64]int),
}
if cfg.ExternalSigner.Enabled {
if cfg.ExternalSigner.URL != "" {
signer, sender, err := externalSigner(ctx, &cfg.ExternalSigner)
if err != nil {
return nil, err
Expand All @@ -182,22 +182,26 @@ func externalSigner(ctx context.Context, opts *ExternalSignerCfg) (signerFn, com
if err != nil {
return nil, common.Address{}, fmt.Errorf("error connecting external signer: %w", err)
}

var hasher types.Signer
return func(ctx context.Context, addr common.Address, tx *types.Transaction) (*types.Transaction, error) {
// According to the "eth_signTransaction" API definition, this shoul be
joshuacolvin0 marked this conversation as resolved.
Show resolved Hide resolved
anodar marked this conversation as resolved.
Show resolved Hide resolved
// 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 {
var data hexutil.Bytes
if err := client.CallContext(ctx, &data, opts.Method, tx); err != nil {
return nil, fmt.Errorf("signing transaction: %w", err)
}
data, err := hexutil.Decode(rlpEncTxStr)
if err != nil {
return nil, fmt.Errorf("error decoding hex: %w", err)
}
var signedTx types.Transaction
if err := rlp.DecodeBytes(data, &signedTx); err != nil {
return nil, fmt.Errorf("error decoding signed transaction: %w", err)
}
if hasher == nil {
hasher = types.LatestSignerForChainID(tx.ChainId())
}
if hasher.Hash(tx) != hasher.Hash(&signedTx) {
return nil, fmt.Errorf("transaction: %x from external signer differs from request: %x", hasher.Hash(tx), hasher.Hash(&signedTx))
}
return &signedTx, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}, sender, nil
}
Expand Down Expand Up @@ -703,10 +707,9 @@ type DataPosterConfig struct {
}

type ExternalSignerCfg struct {
// If enabled, this overrides transaction options and uses external signer
// URL of the external signer rpc server, if set this overrides transaction
// options and uses external signer
// for signing transactions.
Enabled bool `koanf:"enabled"`
// URL of the external signer rpc server.
URL string `koanf:"url"`
// Hex encoded ethereum address of the external signer.
Address string `koanf:"address"`
Expand Down Expand Up @@ -750,7 +753,6 @@ func addDangerousOptions(prefix string, f *pflag.FlagSet) {
}

func addExternalSignerOptions(prefix string, f *pflag.FlagSet) {
f.Bool(prefix+".enabled", DefaultDataPosterConfig.ExternalSigner.Enabled, "enable external signer")
f.String(prefix+".url", DefaultDataPosterConfig.ExternalSigner.URL, "external signer url")
f.String(prefix+".address", DefaultDataPosterConfig.ExternalSigner.Address, "external signer address")
f.String(prefix+".method", DefaultDataPosterConfig.ExternalSigner.Method, "external signer method")
Expand All @@ -770,7 +772,7 @@ var DefaultDataPosterConfig = DataPosterConfig{
UseNoOpStorage: false,
LegacyStorageEncoding: true,
Dangerous: DangerousConfig{ClearDBStorage: false},
ExternalSigner: ExternalSignerCfg{Enabled: false, Method: "eth_signTransaction"},
ExternalSigner: ExternalSignerCfg{},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. ( I guess you meant TestDataPosterConfig not DefaultDataPosterConfigForValidator)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I got confused by the github diff

image

}

var DefaultDataPosterConfigForValidator = func() DataPosterConfig {
Expand All @@ -792,7 +794,7 @@ var TestDataPosterConfig = DataPosterConfig{
AllocateMempoolBalance: true,
UseDBStorage: false,
UseNoOpStorage: false,
ExternalSigner: ExternalSignerCfg{Enabled: false, Method: "eth_signTransaction"},
ExternalSigner: ExternalSignerCfg{},
}

var TestDataPosterConfigForValidator = func() DataPosterConfig {
Expand Down
4 changes: 2 additions & 2 deletions arbnode/dataposter/dataposter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func TestExternalSigner(t *testing.T) {
&ExternalSignerCfg{
Address: srv.address.Hex(),
URL: "http://127.0.0.1:1234",
Method: "eth_signTransaction",
Method: "test_signTransaction",
})
if err != nil {
t.Fatalf("Error getting external signer: %v", err)
Expand Down Expand Up @@ -124,7 +124,7 @@ func newServer(ctx context.Context, t *testing.T) (*http.Server, *server) {

s := &server{signerFn: signer, address: address}
s.handlers = map[string]func(*json.RawMessage) (string, error){
"eth_signTransaction": s.signTransaction,
"test_signTransaction": s.signTransaction,
}
m := http.NewServeMux()
httpSrv := &http.Server{Addr: ":1234", Handler: m, ReadTimeout: 5 * time.Second}
Expand Down