Merge pull request #2957 from ipfs/patch-1

core/commands/config: do not show private key on local network

core/commands/config.go

@@ -9,6 +9,7 @@ import (
 	"io/ioutil"
 	"os"
 	"os/exec"
+	"strings"
 
 	cmds "github.com/ipfs/go-ipfs/commands"
 	repo "github.com/ipfs/go-ipfs/repo"
@@ -58,6 +59,14 @@ Set the value of the 'datastore.path' key:
 		args := req.Arguments()
 		key := args[0]
 
+		// This is a temporary fix until we move the private key out of the config file
+		switch strings.ToLower(key) {
+		case "identity", "identity.privkey":
+			res.SetError(fmt.Errorf("cannot show or change private key through API"), cmds.ErrNormal)
+			return
+		default:
+		}
+
 		r, err := fsrepo.Open(req.InvocContext().ConfigRoot)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
@@ -134,18 +143,40 @@ included in the output of this command.
 	},
 
 	Run: func(req cmds.Request, res cmds.Response) {
-		filename, err := config.Filename(req.InvocContext().ConfigRoot)
+		fname, err := config.Filename(req.InvocContext().ConfigRoot)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
 
-		output, err := showConfig(filename)
+		data, err := ioutil.ReadFile(fname)
 		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
-		res.SetOutput(output)
+
+		var cfg map[string]interface{}
+		err = json.Unmarshal(data, &cfg)
+		if err != nil {
+			res.SetError(err, cmds.ErrNormal)
+			return
+		}
+
+		idmap, ok := cfg["Identity"].(map[string]interface{})
+		if !ok {
+			res.SetError(fmt.Errorf("config has no identity"), cmds.ErrNormal)
+			return
+		}
+
+		delete(idmap, "PrivKey")
+
+		output, err := config.HumanOutput(cfg)
+		if err != nil {
+			res.SetError(err, cmds.ErrNormal)
+			return
+		}
+
+		res.SetOutput(bytes.NewReader(output))
 	},
 }
 
@@ -219,22 +250,20 @@ func getConfig(r repo.Repo, key string) (*ConfigField, error) {
 }
 
 func setConfig(r repo.Repo, key string, value interface{}) (*ConfigField, error) {
-	err := r.SetConfigKey(key, value)
+	keyF, err := getConfig(r, "Identity.PrivKey")
 	if err != nil {
-		return nil, fmt.Errorf("Failed to set config value: %s (maybe use --json?)", err)
+		return nil, errors.New("failed to get PrivKey")
 	}
-	return getConfig(r, key)
-}
-
-func showConfig(filename string) (io.Reader, error) {
-	// TODO maybe we should omit privkey so we don't accidentally leak it?
-
-	data, err := ioutil.ReadFile(filename)
+	privkey := keyF.Value
+	err = r.SetConfigKey(key, value)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to set config value: %s (maybe use --json?)", err)
 	}
-
-	return bytes.NewReader(data), nil
+	err = r.SetConfigKey("Identity.PrivKey", privkey)
+	if err != nil {
+		return nil, errors.New("failed to set PrivKey")
+	}
+	return getConfig(r, key)
 }
 
 func editConfig(filename string) error {
@@ -251,8 +280,23 @@ func editConfig(filename string) error {
 func replaceConfig(r repo.Repo, file io.Reader) error {
 	var cfg config.Config
 	if err := json.NewDecoder(file).Decode(&cfg); err != nil {
-		return errors.New("Failed to decode file as config")
+		return errors.New("failed to decode file as config")
+	}
+	if len(cfg.Identity.PrivKey) != 0 {
+		return errors.New("setting private key with API is not supported")
+	}
+
+	keyF, err := getConfig(r, "Identity.PrivKey")
+	if err != nil {
+		return fmt.Errorf("Failed to get PrivKey")
+	}
+
+	pkstr, ok := keyF.Value.(string)
+	if !ok {
+		return fmt.Errorf("private key in config was not a string")
 	}
 
+	cfg.Identity.PrivKey = pkstr
+
 	return r.SetConfig(&cfg)
 }

repo/config/identity.go

@@ -8,7 +8,7 @@ import (
 // Identity tracks the configuration of the local node's identity.
 type Identity struct {
 	PeerID  string
-	PrivKey string
+	PrivKey string `json:",omitempty"`
 }
 
 // DecodePrivateKey is a helper to decode the users PrivateKey

test/sharness/t0021-config.sh

@@ -71,6 +71,59 @@ test_config_cmd() {
     grep "\"beep2\": false," actual &&
     grep "\"beep3\": false," actual
   '
+
+  test_expect_success "'ipfs config Identity' fails" '
+       test_expect_code 1 ipfs config Identity 2> ident_out
+  '
+
+  test_expect_success "output looks good" '
+       echo "Error: cannot show or change private key through API" > ident_exp &&
+       test_cmp ident_exp ident_out
+  '
+
+  # SECURITY
+  # Those tests are here to prevent exposing the PrivKey on the network
+  test_expect_success "'ipfs config Identity.PrivKey' fails" '
+       test_expect_code 1 ipfs config Identity.PrivKey 2> ident_out
+  '
+
+  test_expect_success "output looks good" '
+       test_cmp ident_exp ident_out
+  '
+
+  test_expect_success "lower cased PrivKey" '
+       sed -i -e '\''s/PrivKey/privkey/'\'' "$IPFS_PATH/config" &&
+       test_expect_code 1 ipfs config Identity.privkey 2> ident_out
+  '
+
+  test_expect_success "output looks good" '
+       test_cmp ident_exp ident_out
+  '
+
+  test_expect_success "fix it back" '
+       sed -i -e '\''s/privkey/PrivKey/'\'' "$IPFS_PATH/config"
+  '
+
+  test_expect_success "'ipfs config show' doesn't include privkey" '
+       ipfs config show > show_config &&
+       test_expect_code 1 grep PrivKey show_config
+  '
+
+  test_expect_success "'ipfs config replace' injects privkey back" '
+       ipfs config replace show_config &&
+	   grep "\"PrivKey\":" "$IPFS_PATH/config" | grep -e ": \".\+\"" >/dev/null
+  '
+
+  test_expect_success "'ipfs config replace' with privkey erors out" '
+       cp "$IPFS_PATH/config" 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