Skip to content

Drop unencrypted transactions by default #340

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

Open
matevz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Drop unencrypted transactions by default #340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions conf/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,9 @@ type GatewayConfig struct {

// OasisRPCs controls whether to enable the `oasis_*` methods. Default is not exposed.
ExposeOasisRPCs bool `koanf:"oasis_rpcs"`

// AllowUnencryptedTxs also accepts transacions with CallFormat=0. Default is false.
AllowUnencryptedTxs bool `koanf:"allow_unencrypted_txs"`
}

// GatewayMonitoringConfig is the gateway prometheus configuration.
Expand Down
3 changes: 2 additions & 1 deletion conf/server.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,5 @@ gateway:
port: 9999
method_limits:
get_logs_max_rounds: 100
oasis_rpcs: false
oasis_rpcs: true
allow_unencrypted_txs: false
38 changes: 38 additions & 0 deletions conf/tests-c10l.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
runtime_id: "8000000000000000000000000000000000000000000000000000000000000000"
node_address: "unix:/tmp/eth-runtime-test/net-runner/network/client-0/internal.sock"

log:
level: debug
format: json

cache:
block_size: 10
metrics: true

database:
host: "127.0.0.1"
port: 5432
db: "postgres"
user: "postgres"
password: "postgres"
dial_timeout: 5
read_timeout: 10
write_timeout: 5
max_open_conns: 0

gateway:
chain_id: 42262
http:
host: "localhost"
port: 8545
ws:
host: "localhost"
port: 8546
monitoring:
host: "localhost"
port: 9999
method_limits:
get_logs_max_rounds: 100
oasis_rpcs: true
allow_unencrypted_txs: false

2 changes: 1 addition & 1 deletion rpc/apis.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func GetRPCAPIs(
var apis []ethRpc.API

web3Service := web3.NewPublicAPI()
ethService := eth.NewPublicAPI(client, archiveClient, logging.GetLogger("eth_rpc"), config.ChainID, backend, gasPriceOracle, config.MethodLimits)
ethService := eth.NewPublicAPI(client, archiveClient, logging.GetLogger("eth_rpc"), config.ChainID, backend, gasPriceOracle, config.MethodLimits, !config.ExposeOasisRPCs || config.AllowUnencryptedTxs)
netService := net.NewPublicAPI(config.ChainID)
txpoolService := txpool.NewPublicAPI()
filtersService := filters.NewPublicAPI(client, logging.GetLogger("eth_filters"), backend, eventSystem)
Expand Down
64 changes: 50 additions & 14 deletions rpc/eth/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/ethereum/go-ethereum/eth/filters"
"github.com/ethereum/go-ethereum/rlp"
ethrpc "github.com/ethereum/go-ethereum/rpc"
"github.com/oasisprotocol/oasis-core/go/common/cbor"
"github.com/oasisprotocol/oasis-core/go/common/logging"
"github.com/oasisprotocol/oasis-sdk/client-sdk/go/client"
"github.com/oasisprotocol/oasis-sdk/client-sdk/go/crypto/signature/secp256k1"
Expand Down Expand Up @@ -108,13 +109,14 @@ type API interface {
}

type publicAPI struct {
client client.RuntimeClient
archiveClient *archive.Client
backend indexer.Backend
gasPriceOracle gas.Backend
chainID uint32
Logger *logging.Logger
methodLimits *conf.MethodLimits
client client.RuntimeClient
archiveClient *archive.Client
backend indexer.Backend
gasPriceOracle gas.Backend
chainID uint32
Logger *logging.Logger
methodLimits *conf.MethodLimits
allowUnencryptedTxs bool
}

// NewPublicAPI creates an instance of the public ETH Web3 API.
Expand All @@ -126,15 +128,17 @@ func NewPublicAPI(
backend indexer.Backend,
gasPriceOracle gas.Backend,
methodLimits *conf.MethodLimits,
allowUnencryptedTxes bool,
) API {
return &publicAPI{
client: client,
archiveClient: archiveClient,
chainID: chainID,
Logger: logger,
backend: backend,
gasPriceOracle: gasPriceOracle,
methodLimits: methodLimits,
client: client,
archiveClient: archiveClient,
chainID: chainID,
Logger: logger,
backend: backend,
gasPriceOracle: gasPriceOracle,
methodLimits: methodLimits,
allowUnencryptedTxs: allowUnencryptedTxes,
}
}

Expand Down Expand Up @@ -463,6 +467,13 @@ func (api *publicAPI) SendRawTransaction(ctx context.Context, data hexutil.Bytes
return common.Hash{}, ErrMalformedTransaction
}

if !api.checkOasisTxEncrypted(ethTx.Data()) {
Copy link
Copy Markdown
Member

@ptrus ptrus Dec 9, 2022

Choose a reason for hiding this comment

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

Maybe rename the method to isEncrypted(tx) and only call it if allowUnencryptedTxs is set to false. Then it can also be made a standalone function (and not a method on the api backend), which will make it easy to write some unit tests for it.
It might also make sense to return a more descriptive error on rejection.

if !api.allowUnencryptedTxs && !isEncrypted(ethTx.Data()) {
    logger.Debug("unencrypted transaction rejected", "hash" ..., 
    return common.Hash{}, ErrUnencryptedNotAllowed
}

logger.Debug("dropped unencrypted transaction", "hash", ethTx.Hash())
return common.Hash{}, ErrInvalidRequest
}

ethTx.Data()
Copy link
Copy Markdown
Member

@ptrus ptrus Dec 9, 2022

Choose a reason for hiding this comment

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

This looks like a leftover?


// Generate an Ethereum transaction that is handled by the EVM module.
utx := types.UnverifiedTransaction{
Body: data,
Expand Down Expand Up @@ -751,3 +762,28 @@ func (api *publicAPI) getBlockRound(ctx context.Context, logger *logging.Logger,
return 0, nil
}
}

// checkOasisTxEncrypted checks, if the Oasis transaction wrapped inside Ethereum tx is encrypted.
func (api *publicAPI) checkOasisTxEncrypted(data []byte) bool {
if api.allowUnencryptedTxs {
// Unencrypted transactions are allowed or encryption not supported by the gateway.
return true
}
if data == nil {
// Transaction is not Oasis transaction, ignore.
return true
}

var tx types.Transaction
Copy link
Copy Markdown
Member

@kostko kostko Jan 11, 2023

Choose a reason for hiding this comment

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

This does not seem correct as the data field does not contain an entire SDK transaction, but rather just a call? This should have a corresponding E2E test.

if err := cbor.Unmarshal(data, &tx); err != nil {
// Transaction is not Oasis transaction, ignore.
return true
}

if tx.Call.Format == types.CallFormatPlain {
return false
}

// Transaction marked as encrypted.
return true
}