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.

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

Redact sensitive values on config logging #409

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion rolling-shutter/keyper/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func NewShuttermintConfig() *ShuttermintConfig {
type ShuttermintConfig struct {
ShuttermintURL string
ValidatorPublicKey *keys.Ed25519Public `shconfig:",required"`
EncryptionKey *keys.ECDSAPrivate `shconfig:",required"`
EncryptionKey *keys.ECDSAPrivate `shconfig:",required,sensitive"`
DKGPhaseLength int64 // in shuttermint blocks
DKGStartBlockDelta uint64
}
Expand Down
8 changes: 5 additions & 3 deletions rolling-shutter/medley/configuration/command/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,11 @@ func Build[T configuration.Config](
if err != nil {
return errors.Wrap(err, "unable to parse configuration")
}
log.Debug().
Interface("config", cfg).
Msg("got config")
err = LogConfig(log.Debug(), cfg, "using configuration")
// XXX should the program panic when the config can't be logged?
if err != nil {
return err
}
return main(cfg)
},
}
Expand Down
15 changes: 15 additions & 0 deletions rolling-shutter/medley/configuration/command/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/mitchellh/mapstructure"
"github.com/pkg/errors"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
"github.com/spf13/afero"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -136,3 +137,17 @@ func ParseViper(v *viper.Viper, config configuration.Config) error {
}
return config.Validate()
}

func LogConfig(ev *zerolog.Event, config configuration.Config, msg string) error {
s := configuration.GetSensitiveRecursive(config)
redactedPaths := []string{}
for redacted := range s {
redactedPaths = append(redactedPaths, redacted)
}
dc, err := configuration.ToDict(config, redactedPaths)
if err != nil {
return err
}
ev.Interface("config", dc).Msg(msg)
return nil
}
2 changes: 1 addition & 1 deletion rolling-shutter/medley/configuration/ethereum.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ func NewEthnodeConfig() *EthnodeConfig {
}

type EthnodeConfig struct {
PrivateKey *keys.ECDSAPrivate `shconfig:",required"`
PrivateKey *keys.ECDSAPrivate `shconfig:",required,sensitive"`
ContractsURL string ` comment:"The JSON RPC endpoint where the contracts are accessible"`
DeploymentDir string ` comment:"Contract source directory"`
EthereumURL string ` comment:"The layer 1 JSON RPC endpoint"`
Expand Down
137 changes: 117 additions & 20 deletions rolling-shutter/medley/configuration/traverse.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package configuration

import (
"encoding"
"fmt"
"io"
"reflect"
Expand All @@ -20,6 +21,7 @@ func sliceToSet[T comparable](sl []T) map[T]bool {
type tagOptions struct {
setExplicitly bool
required bool
sensitive bool
}

func extractTags(field *reflect.StructField) tagOptions {
Expand All @@ -38,12 +40,31 @@ func extractTags(field *reflect.StructField) tagOptions {
}
tags := sliceToSet(strings.Split(strings.TrimSpace(tagVal), ","))
_, required := tags["required"]
_, sensitive := tags["sensitive"]
return tagOptions{
setExplicitly: true,
required: required,
sensitive: sensitive,
}
}

// GetSensitiveRecursive will return all sensitive config paths.
func GetSensitiveRecursive(root Config) map[string]any {
Copy link
Contributor

Choose a reason for hiding this comment

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

We use struct{} as the value type for maps used like sets. I think we should also do this here as it makes the code a bit clearer. In case I'm wrong and we need the value, I don't understand why this isn't a bool.

sensitive := map[string]any{}
execFn := func(n *node) error {
if n.isBranch {
return nil
}
opts := extractTags(n.fieldType)
if opts.sensitive {
sensitive[strings.ToLower(n.path)] = true
}
return nil
}
_ = traverseRecursive(root, execFn, true)
return sensitive
}

// GetRequiredRecursive will return all required config paths
// A required field is a field that is not present as a default is considered required.
func GetRequiredRecursive(root Config) map[string]any {
Expand All @@ -54,11 +75,11 @@ func GetRequiredRecursive(root Config) map[string]any {
}
opts := extractTags(n.fieldType)
if opts.required {
required[n.path] = true
required[strings.ToLower(n.path)] = true
}
return nil
}
_ = traverseRecursive(root, execFn)
_ = traverseRecursive(root, execFn, true)
return required
}

Expand All @@ -68,7 +89,7 @@ func SetDefaultValuesRecursive(root Config, excludePaths []string) error {
exPth[p] = true
}
execFn := func(n *node) error {
_, excluded := exPth[n.path]
_, excluded := exPth[strings.ToLower(n.path)]
if !n.isBranch || excluded {
return nil
}
Expand All @@ -78,7 +99,7 @@ func SetDefaultValuesRecursive(root Config, excludePaths []string) error {
// we need a second pass to reset excluded fields.
// this is not efficient, but ok for config parsing
execFnResetExcluded := func(n *node) error {
_, excluded := exPth[n.path]
_, excluded := exPth[strings.ToLower(n.path)]
if !n.isBranch && excluded {
if n.fieldValue.CanSet() {
n.fieldValue.SetZero()
Expand All @@ -87,11 +108,11 @@ func SetDefaultValuesRecursive(root Config, excludePaths []string) error {
}
return nil
}
err := traverseRecursive(root, execFn)
err := traverseRecursive(root, execFn, true)
if err != nil {
return err
}
return traverseRecursive(root, execFnResetExcluded)
return traverseRecursive(root, execFnResetExcluded, true)
}

func SetExampleValuesRecursive(root Config) error {
Expand All @@ -101,7 +122,7 @@ func SetExampleValuesRecursive(root Config) error {
}
return n.config.SetExampleValues()
}
return traverseRecursive(root, execFn)
return traverseRecursive(root, execFn, true)
}

func GetEnvironmentVarsRecursive(root Config) map[string][]string {
Expand All @@ -110,7 +131,7 @@ func GetEnvironmentVarsRecursive(root Config) map[string][]string {
if n.isBranch {
return nil
}
vars[n.path] = []string{
vars[strings.ToLower(n.path)] = []string{
// this is the old naming scheme, without the sub-config
// qualifiers ("P2P", "ETHEREUM") and with a subcommand specific
// prefix (e.g. KEYPER_CUSTOMBOOTSTRAPADDRESSES)
Expand All @@ -119,11 +140,11 @@ func GetEnvironmentVarsRecursive(root Config) map[string][]string {
// full path with a generic prefix not dependend on the
// subcommand executed
// (e.g. SHUTTER_P2P_CUSTOMBOOTSTRAPADDRESSES)
"SHUTTER_" + strings.ToUpper(strings.ReplaceAll(n.path, ".", "_")),
"SHUTTER_" + strings.ToUpper(strings.ReplaceAll(strings.ToLower(n.path), ".", "_")),
}
return nil
}
_ = traverseRecursive(root, execFn)
_ = traverseRecursive(root, execFn, true)
return vars
}

Expand All @@ -137,7 +158,7 @@ func writeTOMLHeadersRecursive(root Config, w io.Writer) error {
totalBytesWritten += i
return err
}
err := traverseRecursive(root, execFn)
err := traverseRecursive(root, execFn, true)
if err != nil {
return err
}
Expand Down Expand Up @@ -168,15 +189,15 @@ func (e *execErr) error() error {
return errors.Wrapf(e.err, "error during recursion at path '%s'", e.node.path)
}

func traverseRecursive(root Config, exec execFunc) error {
func traverseRecursive(root Config, exec execFunc, tailRecursive bool) error {
rootNode := &node{
isBranch: true,
previousNodes: []*node{},
config: root,
path: "",
fieldType: nil,
}
err := execRecursive(rootNode, exec, stopNever)
err := execRecursive(rootNode, exec, stopNever, tailRecursive)
if err != nil {
return err.error()
}
Expand All @@ -196,7 +217,15 @@ func stopNever(_ *node) bool {

// execRecursive recursively traverses the config struct like a tree.
// The implementation is not optimized.
func execRecursive(n *node, exec execFunc, stop stopFunc) *execErr {
func execRecursive(n *node, exec execFunc, stop stopFunc, tailRecursion bool) *execErr {
if !tailRecursion {
if err := exec(n); err != nil {
return &execErr{
err: err,
node: n,
}
}
}
// if the node is a branch, first handle potential subtrees.
// this results in child nodes always being executed before their parent node
if n.isBranch {
Expand Down Expand Up @@ -231,7 +260,7 @@ func execRecursive(n *node, exec execFunc, stop stopFunc) *execErr {
if nextPath != "" {
nextPath += "."
}
nextPath += strings.ToLower(structField.Name)
nextPath += structField.Name
nextNode := &node{
isBranch: false,
previousNodes: newPath,
Expand All @@ -249,17 +278,85 @@ func execRecursive(n *node, exec execFunc, stop stopFunc) *execErr {
// if the stop function returns false,
// we can dive into the next subtree
if !stop(nextNode) {
if err := execRecursive(nextNode, exec, stop); err != nil {
if err := execRecursive(nextNode, exec, stop, tailRecursion); err != nil {
return err
}
}
}
}
if err := exec(n); err != nil {
return &execErr{
err: err,
node: n,
if tailRecursion {
if err := exec(n); err != nil {
return &execErr{
err: err,
node: n,
}
}
}
return nil
}

func UpsertDictForPath(d map[string]interface{}, path string) (map[string]interface{}, error) {
var current map[string]interface{}
if path == "" {
return d, nil
}
for _, elem := range strings.Split(path, ".") {
val, ok := d[elem]
if !ok {
current = map[string]interface{}{}
d[elem] = current
continue
}
current, ok = val.(map[string]interface{})
if !ok {
return nil, errors.Errorf("path dict (path=%s) has wrong type", path)
}
}
return current, nil
}

func ToDict(root Config, redactPaths []string) (map[string]interface{}, error) {
rdPth := map[string]bool{}
for _, p := range redactPaths {
rdPth[p] = true
}
out := map[string]interface{}{}
execFn := func(n *node) error {
if len(n.previousNodes) == 0 {
return nil
}
if n.isBranch {
_, err := UpsertDictForPath(out, n.path)
if err != nil {
return err
}
} else {
subdict, err := UpsertDictForPath(out, n.previousNodes[len(n.previousNodes)-1].path)
if err != nil {
return err
}

_, redacted := rdPth[strings.ToLower(n.path)]
var value any
if redacted {
value = "(sensitive)"
} else {
v := n.fieldValue.Interface()
marshaller, ok := v.(encoding.TextMarshaler)
if !ok {
value = v
} else {
res, err := marshaller.MarshalText()
if err != nil {
return err
}
value = string(res)
}
}
subdict[n.fieldType.Name] = value
}
return nil
}
err := traverseRecursive(root, execFn, false)
return out, err
}
6 changes: 3 additions & 3 deletions rolling-shutter/medley/decodehooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func TextMarshalerHook(from reflect.Value, to reflect.Value) (interface{}, error
return string(mshl), nil
}

func mapstructureDecode(input, result any, hookFunc mapstructure.DecodeHookFunc) error {
func MapstructureDecode(input, result any, hookFunc mapstructure.DecodeHookFunc) error {
decoder, err := mapstructure.NewDecoder(
&mapstructure.DecoderConfig{
Result: result,
Expand All @@ -71,7 +71,7 @@ func mapstructureDecode(input, result any, hookFunc mapstructure.DecodeHookFunc)
}

func MapstructureMarshal(input, result any) error {
return mapstructureDecode(
return MapstructureDecode(
input,
result,
mapstructure.ComposeDecodeHookFunc(
Expand All @@ -81,7 +81,7 @@ func MapstructureMarshal(input, result any) error {
}

func MapstructureUnmarshal(input, result any) error {
return mapstructureDecode(
return MapstructureDecode(
input,
result,
mapstructure.ComposeDecodeHookFunc(
Expand Down
2 changes: 1 addition & 1 deletion rolling-shutter/p2p/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (c *Config) Init() {
}

type Config struct {
P2PKey *keys.Libp2pPrivate `shconfig:",required"`
P2PKey *keys.Libp2pPrivate `shconfig:",required,sensitive"`
ListenAddresses []*address.P2PAddress
CustomBootstrapAddresses []*address.P2PAddress `comment:"Overwrite p2p boostrap nodes"`
Environment env.Environment
Expand Down