From 671425ea3321df62ce6719f03f1ce3f633494e16 Mon Sep 17 00:00:00 2001 From: Jakub Sztandera Date: Mon, 29 Aug 2016 19:06:48 +0200 Subject: [PATCH 1/3] cmd: harden config show with key License: MIT Signed-off-by: Jakub Sztandera --- core/commands/config.go | 19 +++++++++++++++++-- test/sharness/t0021-config.sh | 14 ++++++++++++-- 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/core/commands/config.go b/core/commands/config.go index 09a106f50e2..b2d953f4335 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -15,6 +15,7 @@ import ( repo "github.com/ipfs/go-ipfs/repo" config "github.com/ipfs/go-ipfs/repo/config" fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo" + u "gx/ipfs/QmZNVWh8LLjAavuQ2JXuFmuYH3C11xo988vSgp7UQrTRj1/go-ipfs-util" ) @@ -164,11 +165,25 @@ included in the output of this command. idmap, ok := cfg["Identity"].(map[string]interface{}) if !ok { - res.SetError(fmt.Errorf("config has no identity"), cmds.ErrNormal) + res.SetError(errors.New("config has no identity"), cmds.ErrNormal) return } - delete(idmap, "PrivKey") + privKeyKey := "" // make sure we both find the name of privkey and we delete it + for key, _ := range idmap { + if strings.ToLower(key) == "privkey" { + if privKeyKey != "" { + res.SetError(errors.New("found multiple PrivKey keys"), cmds.ErrNormal) + return + } + privKeyKey = key + } + } + if privKeyKey == "" { + res.SetError(errors.New("haven't found PriveKey key"), cmds.ErrNormal) + } + + delete(idmap, privKeyKey) output, err := config.HumanOutput(cfg) if err != nil { diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index 1fc35e176b2..3366df9e6e2 100755 --- a/test/sharness/t0021-config.sh +++ b/test/sharness/t0021-config.sh @@ -111,10 +111,10 @@ test_config_cmd() { test_expect_success "'ipfs config replace' injects privkey back" ' ipfs config replace show_config && - grep "\"PrivKey\":" "$IPFS_PATH/config" | grep -e ": \".\+\"" >/dev/null + grep "\"PrivKey\":" "$IPFS_PATH/config" | grep -e ": \".\+\"" >/dev/null ' - test_expect_success "'ipfs config replace' with privkey erors out" ' + test_expect_success "'ipfs config replace' with privkey errors out" ' cp "$IPFS_PATH/config" real_config && test_expect_code 1 ipfs config replace - < real_config 2> replace_out ' @@ -124,6 +124,16 @@ test_config_cmd() { test_cmp replace_out replace_expected ' + test_expect_success "'ipfs config replace' with lower case privkey errors out" ' + cp "$IPFS_PATH/config" real_config && + sed -i -e '\''s/PrivKey/privkey/'\'' real_config && + test_expect_code 1 ipfs config replace - < real_config 2> replace_out + ' + + test_expect_success "output looks good" ' + echo "Error: setting private key with API is not supported" > replace_expected + test_cmp replace_out replace_expected + ' } test_init_ipfs From 91dd044d55dcb0b8d12388e2194cb4dac01bafe1 Mon Sep 17 00:00:00 2001 From: Jeromy Date: Mon, 29 Aug 2016 14:56:55 -0700 Subject: [PATCH 2/3] config: guard against privkey being overwritten in fsrepo setConfig License: MIT Signed-off-by: Jeromy --- core/commands/config.go | 76 +++++++++++++++++++++++++---------------- repo/config/identity.go | 4 +++ repo/fsrepo/fsrepo.go | 13 +++++++ 3 files changed, 63 insertions(+), 30 deletions(-) diff --git a/core/commands/config.go b/core/commands/config.go index b2d953f4335..c2701054234 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -163,28 +163,12 @@ included in the output of this command. return } - idmap, ok := cfg["Identity"].(map[string]interface{}) - if !ok { - res.SetError(errors.New("config has no identity"), cmds.ErrNormal) + err = scrubValue(cfg, []string{config.IdentityTag, config.PrivKeyTag}) + if err != nil { + res.SetError(err, cmds.ErrNormal) return } - privKeyKey := "" // make sure we both find the name of privkey and we delete it - for key, _ := range idmap { - if strings.ToLower(key) == "privkey" { - if privKeyKey != "" { - res.SetError(errors.New("found multiple PrivKey keys"), cmds.ErrNormal) - return - } - privKeyKey = key - } - } - if privKeyKey == "" { - res.SetError(errors.New("haven't found PriveKey key"), cmds.ErrNormal) - } - - delete(idmap, privKeyKey) - output, err := config.HumanOutput(cfg) if err != nil { res.SetError(err, cmds.ErrNormal) @@ -195,6 +179,47 @@ included in the output of this command. }, } +func scrubValue(m map[string]interface{}, key []string) error { + find := func(m map[string]interface{}, k string) (string, interface{}, bool) { + lckey := strings.ToLower(k) + for mkey, val := range m { + lcmkey := strings.ToLower(mkey) + if lckey == lcmkey { + return mkey, val, true + } + } + return "", nil, false + } + + cur := m + for _, k := range key[:len(key)-1] { + foundk, val, ok := find(cur, k) + if !ok { + return fmt.Errorf("failed to find specified key") + } + + if foundk != k { + // case mismatch, calling this an error + return fmt.Errorf("case mismatch in config, expected %q but got %q", k, foundk) + } + + mval, mok := val.(map[string]interface{}) + if !mok { + return fmt.Errorf("%s was not a map", foundk) + } + + cur = mval + } + + todel, _, ok := find(cur, key[len(key)-1]) + if !ok { + return fmt.Errorf("%s, not found", strings.Join(key, ".")) + } + + delete(cur, todel) + return nil +} + var configEditCmd = &cmds.Command{ Helptext: cmds.HelpText{ Tagline: "Opens the config file for editing in $EDITOR.", @@ -265,19 +290,10 @@ func getConfig(r repo.Repo, key string) (*ConfigField, error) { } func setConfig(r repo.Repo, key string, value interface{}) (*ConfigField, error) { - keyF, err := getConfig(r, "Identity.PrivKey") - if err != nil { - return nil, errors.New("failed to get PrivKey") - } - privkey := keyF.Value - err = r.SetConfigKey(key, value) + err := r.SetConfigKey(key, value) if err != nil { return nil, fmt.Errorf("failed to set config value: %s (maybe use --json?)", err) } - err = r.SetConfigKey("Identity.PrivKey", privkey) - if err != nil { - return nil, errors.New("failed to set PrivKey") - } return getConfig(r, key) } @@ -301,7 +317,7 @@ func replaceConfig(r repo.Repo, file io.Reader) error { return errors.New("setting private key with API is not supported") } - keyF, err := getConfig(r, "Identity.PrivKey") + keyF, err := getConfig(r, config.PrivKeySelector) if err != nil { return fmt.Errorf("Failed to get PrivKey") } diff --git a/repo/config/identity.go b/repo/config/identity.go index d997bc2b632..55fc11e70e0 100644 --- a/repo/config/identity.go +++ b/repo/config/identity.go @@ -5,6 +5,10 @@ import ( ic "gx/ipfs/QmUWER4r4qMvaCnX5zREcfyiWN7cXN9g3a7fkRqNz8qWPP/go-libp2p-crypto" ) +const IdentityTag = "Identity" +const PrivKeyTag = "PrivKey" +const PrivKeySelector = IdentityTag + "." + PrivKeyTag + // Identity tracks the configuration of the local node's identity. type Identity struct { PeerID string diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 28b0c6f2461..005953fbf63 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -482,6 +482,14 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { return err } + // Load private key to guard against it being overwritten. + // NOTE: this is a temporary measure to secure this field until we move + // keys out of the config file. + pkval, err := common.MapGetKV(mapconf, config.PrivKeySelector) + if err != nil { + return err + } + // Get the type of the value associated with the key oldValue, err := common.MapGetKV(mapconf, key) ok := true @@ -523,6 +531,11 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { return err } + // replace private key, in case it was overwritten. + if err := common.MapSetKV(mapconf, "Identity.PrivKey", pkval); err != nil { + return err + } + // This step doubles as to validate the map against the struct // before serialization conf, err := config.FromMap(mapconf) From 667f8a6f13a151a096599a56778c01db78074e0d Mon Sep 17 00:00:00 2001 From: Jeromy Date: Fri, 2 Sep 2016 06:40:23 -0700 Subject: [PATCH 3/3] replace string with constant in fsrepo privkey check License: MIT Signed-off-by: Jeromy --- repo/fsrepo/fsrepo.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 005953fbf63..683a7fa1b46 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -532,7 +532,7 @@ func (r *FSRepo) SetConfigKey(key string, value interface{}) error { } // replace private key, in case it was overwritten. - if err := common.MapSetKV(mapconf, "Identity.PrivKey", pkval); err != nil { + if err := common.MapSetKV(mapconf, config.PrivKeySelector, pkval); err != nil { return err }