Skip to content

Commit

Permalink
*: implement audit fixes (#2821)
Browse files Browse the repository at this point in the history
Cherry-picks audit fixes tagged with `qs-audit` on `main-v0.19`

category: misc
ticket: none
  • Loading branch information
gsora authored Jan 25, 2024
1 parent ea9814e commit 8483677
Show file tree
Hide file tree
Showing 41 changed files with 566 additions and 250 deletions.
3 changes: 2 additions & 1 deletion app/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ type Config struct {
PrivKeyFile string
PrivKeyLocking bool
MonitoringAddr string
DebugAddr string
ValidatorAPIAddr string
BeaconNodeAddrs []string
JaegerAddr string
Expand Down Expand Up @@ -259,7 +260,7 @@ func Run(ctx context.Context, conf Config) (err error) {
return err
}

wireMonitoringAPI(ctx, life, conf.MonitoringAddr, tcpNode, eth2Cl, peerIDs,
wireMonitoringAPI(ctx, life, conf.MonitoringAddr, conf.DebugAddr, tcpNode, eth2Cl, peerIDs,
promRegistry, qbftDebug, pubkeys, seenPubkeys, vapiCalls)

err = wireCoreWorkflow(ctx, life, conf, cluster, nodeIdx, tcpNode, p2pKey, eth2Cl,
Expand Down
2 changes: 2 additions & 0 deletions app/lifecycle/order.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const (
StartAggSigDB
StartRelay
StartMonitoringAPI
StartDebugAPI
StartValidatorAPI
StartP2PPing
StartP2PRouters
Expand All @@ -42,5 +43,6 @@ const (
StopP2PPeerDB
StopP2PTCPNode
StopP2PUDPNode
StopDebugAPI
StopMonitoringAPI
)
25 changes: 13 additions & 12 deletions app/lifecycle/orderstart_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions app/lifecycle/orderstop_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 25 additions & 12 deletions app/monitoringapi.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ var (

// wireMonitoringAPI constructs the monitoring API and registers it with the life cycle manager.
// It serves prometheus metrics, pprof profiling and the runtime enr.
func wireMonitoringAPI(ctx context.Context, life *lifecycle.Manager, addr string,
func wireMonitoringAPI(ctx context.Context, life *lifecycle.Manager, promAddr, debugAddr string,
tcpNode host.Host, eth2Cl eth2wrap.Client,
peerIDs []peer.ID, registry *prometheus.Registry, qbftDebug http.Handler,
pubkeys []core.PubKey, seenPubkeys <-chan core.PubKey, vapiCalls <-chan struct{},
Expand Down Expand Up @@ -76,18 +76,8 @@ func wireMonitoringAPI(ctx context.Context, life *lifecycle.Manager, addr string
writeResponse(w, http.StatusOK, "ok")
})

// Serve sniffed qbft instances messages in gzipped protobuf format.
mux.Handle("/debug/qbft", qbftDebug)

// Copied from net/http/pprof/pprof.go
mux.HandleFunc("/debug/pprof/", pprof.Index)
mux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
mux.HandleFunc("/debug/pprof/profile", pprof.Profile)
mux.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
mux.HandleFunc("/debug/pprof/trace", pprof.Trace)

server := &http.Server{
Addr: addr,
Addr: promAddr,
Handler: mux,
ReadHeaderTimeout: time.Second,
}
Expand All @@ -99,6 +89,29 @@ func wireMonitoringAPI(ctx context.Context, life *lifecycle.Manager, addr string
QuorumPeers: cluster.Threshold(len(peerIDs)),
}, registry)

if debugAddr != "" {
debugMux := http.NewServeMux()

// Serve sniffed qbft instances messages in gzipped protobuf format.
debugMux.Handle("/debug/qbft", qbftDebug)

// Copied from net/http/pprof/pprof.go
debugMux.HandleFunc("/debug/pprof/", pprof.Index)
debugMux.HandleFunc("/debug/pprof/cmdline", pprof.Cmdline)
debugMux.HandleFunc("/debug/pprof/profile", pprof.Profile)
debugMux.HandleFunc("/debug/pprof/symbol", pprof.Symbol)
debugMux.HandleFunc("/debug/pprof/trace", pprof.Trace)

debugServer := &http.Server{
Addr: debugAddr,
Handler: debugMux,
ReadHeaderTimeout: time.Second,
}

life.RegisterStart(lifecycle.AsyncBackground, lifecycle.StartDebugAPI, httpServeHook(debugServer.ListenAndServe))
life.RegisterStop(lifecycle.StopDebugAPI, lifecycle.HookFunc(debugServer.Shutdown))
}

life.RegisterStart(lifecycle.AsyncBackground, lifecycle.StartMonitoringAPI, httpServeHook(server.ListenAndServe))
life.RegisterStart(lifecycle.AsyncBackground, lifecycle.StartMonitoringAPI, lifecycle.HookFuncCtx(checker.Run))
life.RegisterStop(lifecycle.StopMonitoringAPI, lifecycle.HookFunc(server.Shutdown))
Expand Down
18 changes: 6 additions & 12 deletions cmd/cmd_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,8 @@ func TestCmdFlags(t *testing.T) {
LokiService: "charon",
},
P2P: p2p.Config{
Relays: []string{"https://0.relay.obol.tech"},
TCPAddrs: nil,
Allowlist: "",
Denylist: "",
Relays: []string{"https://0.relay.obol.tech", "https://1.relay.obol.tech"},
TCPAddrs: nil,
},
Feature: featureset.Config{
MinStatus: "stable",
Expand All @@ -89,10 +87,8 @@ func TestCmdFlags(t *testing.T) {
Args: slice("create", "enr"),
Datadir: ".charon",
P2PConfig: &p2p.Config{
Relays: []string{"https://0.relay.obol.tech"},
TCPAddrs: nil,
Allowlist: "",
Denylist: "",
Relays: []string{"https://0.relay.obol.tech"},
TCPAddrs: nil,
},
},
{
Expand All @@ -114,10 +110,8 @@ func TestCmdFlags(t *testing.T) {
LokiService: "charon",
},
P2P: p2p.Config{
Relays: []string{"https://0.relay.obol.tech"},
TCPAddrs: nil,
Allowlist: "",
Denylist: "",
Relays: []string{"https://0.relay.obol.tech", "https://1.relay.obol.tech"},
TCPAddrs: nil,
},
Feature: featureset.Config{
MinStatus: "stable",
Expand Down
15 changes: 13 additions & 2 deletions cmd/createcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func runCreateCluster(ctx context.Context, w io.Writer, conf clusterConfig) erro
conf.Network = eth2util.Goerli.Name
}

if err = validateCreateConfig(conf); err != nil {
if err = validateCreateConfig(ctx, conf); err != nil {
return err
}

Expand Down Expand Up @@ -301,7 +301,7 @@ func runCreateCluster(ctx context.Context, w io.Writer, conf clusterConfig) erro
}

// validateCreateConfig returns an error if any of the provided config parameters are invalid.
func validateCreateConfig(conf clusterConfig) error {
func validateCreateConfig(ctx context.Context, conf clusterConfig) error {
if conf.NumNodes == 0 && conf.DefFile == "" { // if there's a definition file, infer this value from it later
return errors.New("missing --nodes flag")
}
Expand All @@ -320,6 +320,17 @@ func validateCreateConfig(conf clusterConfig) error {
return errors.New("number of --keymanager-addresses do not match --keymanager-auth-tokens. Please fix configuration flags")
}

for _, addr := range conf.KeymanagerAddrs {
keymanagerURL, err := url.Parse(addr)
if err != nil {
return errors.Wrap(err, "failed to parse keymanager addr", z.Str("addr", addr))
}

if keymanagerURL.Scheme != "https" {
log.Warn(ctx, "Keymanager URL does not use https protocol", nil, z.Str("addr", addr))
}
}

if conf.SplitKeys {
if conf.NumDVs != 0 {
return errors.New("can't specify --num-validators with --split-existing-keys. Please fix configuration flags")
Expand Down
13 changes: 13 additions & 0 deletions cmd/debug_tools.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright © 2022-2023 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1

package cmd

import (
"github.com/spf13/cobra"
)

// bindDebugMonitoringFlags binds Prometheus monitoring and debug address CLI flags. The debug address defaults to an empty address.
func bindDebugMonitoringFlags(cmd *cobra.Command, monitorAddr, debugAddr *string, defaultMonitorAddr string) {
cmd.Flags().StringVar(monitorAddr, "monitoring-address", defaultMonitorAddr, "Listening address (ip and port) for the monitoring API (prometheus).")
cmd.Flags().StringVar(debugAddr, "debug-address", "", "Listening address (ip and port) for the pprof and QBFT debug API. It is not enabled by default.")
}
110 changes: 110 additions & 0 deletions cmd/debug_tools_internal_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
// Copyright © 2022-2023 Obol Labs Inc. Licensed under the terms of a Business Source License 1.1

package cmd

import (
"testing"

"github.com/spf13/cobra"
"github.com/stretchr/testify/require"

"github.com/obolnetwork/charon/app"
)

func genTestCmd(t *testing.T, f func(config app.Config)) *cobra.Command {
t.Helper()

var conf app.Config

cmd := &cobra.Command{
Use: "test",
Short: "test",
}

cmd.Run = func(cmd *cobra.Command, args []string) {
f(conf)
}

bindDebugMonitoringFlags(cmd, &conf.MonitoringAddr, &conf.DebugAddr, "")

return cmd
}

func Test_bindDebugMonitoringFlags(t *testing.T) {
cmd := &cobra.Command{
Use: "testcmd",
}

t.Run("both present", func(t *testing.T) {
var (
mAddr = "127.0.0.1:9999"
dAddr = "127.0.0.1:8888"
)

cmd.ResetCommands()

testCmd := genTestCmd(t, func(config app.Config) {
require.Equal(t, mAddr, config.MonitoringAddr)
require.Equal(t, dAddr, config.DebugAddr)
})

cmd.AddCommand(testCmd)

cmd.SetArgs([]string{
"test",
"--monitoring-address",
mAddr,
"--debug-address",
dAddr,
})

require.NoError(t, cmd.Execute())
})

t.Run("only monitor", func(t *testing.T) {
var (
mAddr = "127.0.0.1:9999"
dAddr = ""
)
cmd.ResetCommands()

testCmd := genTestCmd(t, func(config app.Config) {
require.Equal(t, mAddr, config.MonitoringAddr)
require.Equal(t, dAddr, config.DebugAddr)
})

cmd.AddCommand(testCmd)

cmd.SetArgs([]string{
"test",
"--monitoring-address",
mAddr,
})

require.NoError(t, cmd.Execute())
})

t.Run("only debug", func(t *testing.T) {
var (
mAddr = ""
dAddr = "127.0.0.1:8888"
)

cmd.ResetCommands()

testCmd := genTestCmd(t, func(config app.Config) {
require.Equal(t, mAddr, config.MonitoringAddr)
require.Equal(t, dAddr, config.DebugAddr)
})

cmd.AddCommand(testCmd)

cmd.SetArgs([]string{
"test",
"--debug-address",
dAddr,
})

require.NoError(t, cmd.Execute())
})
}
2 changes: 1 addition & 1 deletion cmd/relay.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ func newRelayCmd(runFunc func(context.Context, relay.Config) error) *cobra.Comma

bindDataDirFlag(cmd.Flags(), &config.DataDir)
bindRelayFlag(cmd, &config)
bindDebugMonitoringFlags(cmd, &config.MonitoringAddr, &config.DebugAddr, "")
bindP2PFlags(cmd, &config.P2PConfig)
bindLogFlags(cmd.Flags(), &config.LogConfig)
bindLokiFlags(cmd.Flags(), &config.LogConfig)
Expand All @@ -43,7 +44,6 @@ func newRelayCmd(runFunc func(context.Context, relay.Config) error) *cobra.Comma

func bindRelayFlag(cmd *cobra.Command, config *relay.Config) {
cmd.Flags().StringVar(&config.HTTPAddr, "http-address", "127.0.0.1:3640", "Listening address (ip and port) for the relay http server serving runtime ENR.")
cmd.Flags().StringVar(&config.MonitoringAddr, "monitoring-address", "127.0.0.1:3620", "Listening address (ip and port) for the prometheus and pprof monitoring http server.")
cmd.Flags().BoolVar(&config.AutoP2PKey, "auto-p2pkey", true, "Automatically create a p2pkey (secp256k1 private key used for p2p authentication and ENR) if none found in data directory.")
cmd.Flags().StringVar(&config.RelayLogLevel, "p2p-relay-loglevel", "", "Libp2p circuit relay log level. E.g., debug, info, warn, error.")

Expand Down
Loading

0 comments on commit 8483677

Please sign in to comment.