From 05232af17b92775f7df7f79ccbaf2c6ec81c56c0 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 28 Apr 2021 17:45:15 +0200 Subject: [PATCH 1/7] fix(config): support dynamic map keys with dots This adds support for alternative map notation which has to be used when the map key is a string with a dot, for example: Gateway.PublicGateways["gw.example.com"].UseSubdomains Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable Context: https://github.com/ipfs/ipfs-webui/issues/1770 --- repo/common/common.go | 40 +++++++++++++++++++++++++++++------ test/sharness/t0021-config.sh | 7 ++++++ 2 files changed, 41 insertions(+), 6 deletions(-) diff --git a/repo/common/common.go b/repo/common/common.go index dc1e7c50360..14631c41162 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -2,26 +2,50 @@ package common import ( "fmt" + "regexp" "strings" ) +// Find dynamic map key names passed as Parent["foo"] notation +var bracketsRe = regexp.MustCompile(`\[([^\[\]]*)\]`) + +// Normalization for supporting arbitrary dynamic keys with dots: +// Gateway.PublicGateways["gw.example.com"].UseSubdomains +// Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable +func keyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]string) { + bracketedKeys := bracketsRe.FindAllString(key, -1) + dynamicKeys = make(map[string]string, len(bracketedKeys)) + normalizedKey = key + for i, mapKeySegment := range bracketedKeys { + mapKey := strings.TrimLeft(mapKeySegment, "[\"") + mapKey = strings.TrimRight(mapKey, "\"]") + placeholder := fmt.Sprintf("mapKey%d", i) + dynamicKeys[placeholder] = mapKey + normalizedKey = strings.Replace(normalizedKey, mapKeySegment, fmt.Sprintf(".%s", placeholder), 1) + } + return normalizedKey, dynamicKeys +} + func MapGetKV(v map[string]interface{}, key string) (interface{}, error) { var ok bool var mcursor map[string]interface{} var cursor interface{} = v - parts := strings.Split(key, ".") + normalizedKey, dynamicKeys := keyToLookupData(key) + parts := strings.Split(normalizedKey, ".") for i, part := range parts { sofar := strings.Join(parts[:i], ".") mcursor, ok = cursor.(map[string]interface{}) if !ok { - return nil, fmt.Errorf("%s key is not a map", sofar) + return nil, fmt.Errorf("%q key is not a map", sofar) + } + if dynamicPart, ok := dynamicKeys[part]; ok { + part = dynamicPart } - cursor, ok = mcursor[part] if !ok { - return nil, fmt.Errorf("%s key has no attributes", sofar) + return nil, fmt.Errorf("%q key has no attribute %q", sofar, part) } } return cursor, nil @@ -32,12 +56,16 @@ func MapSetKV(v map[string]interface{}, key string, value interface{}) error { var mcursor map[string]interface{} var cursor interface{} = v - parts := strings.Split(key, ".") + normalizedKey, dynamicKeys := keyToLookupData(key) + parts := strings.Split(normalizedKey, ".") for i, part := range parts { mcursor, ok = cursor.(map[string]interface{}) if !ok { sofar := strings.Join(parts[:i], ".") - return fmt.Errorf("%s key is not a map", sofar) + return fmt.Errorf("%q key is not a map", sofar) + } + if dynamicPart, ok := dynamicKeys[part]; ok { + part = dynamicPart } // last part? set here diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index c417ba4de57..6aca21558c3 100755 --- a/test/sharness/t0021-config.sh +++ b/test/sharness/t0021-config.sh @@ -126,6 +126,13 @@ test_config_cmd() { test_cmp replconfig.json newconfig.json ' + # Dynamic keys with dot in their names + test_config_cmd_set "--json" "Gateway.PublicGateways[\"some.example.com\"].UseSubdomains" "true" + test_expect_success "'ipfs config' output for dynamic keys looks good" ' + ipfs config Gateway.PublicGateways["some.example.com"].UseSubdomains > dynkey_out && + grep true dynkey_out + ' + # SECURITY # Those tests are here to prevent exposing the PrivKey on the network From 0a2336ea16349182d54fedb7b112dc0d705c373a Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Wed, 28 Apr 2021 23:27:27 +0200 Subject: [PATCH 2/7] docs: ipfs config --help --- core/commands/config.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/core/commands/config.go b/core/commands/config.go index 86037ceb4f9..b074eb56842 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -56,6 +56,11 @@ Get the value of the 'Datastore.Path' key: Set the value of the 'Datastore.Path' key: $ ipfs config Datastore.Path ~/.ipfs/datastore + +Values behind map key names that include dots can be accessed like this: + + $ ipfs config Pinning.RemoteServices["pins.example.org"].Policies + `, }, Subcommands: map[string]*cmds.Command{ @@ -176,7 +181,13 @@ var configShowCmd = &cmds.Command{ Helptext: cmds.HelpText{ Tagline: "Output config file contents.", ShortDescription: ` -NOTE: For security reasons, this command will omit your private key and remote services. If you would like to make a full backup of your config (private key included), you must copy the config file from your repo. +'ipfs config show' returns config contents without private keys and secrets. +`, + LongDescription: ` +NOTE: For security reasons, this command will omit your private key and any +access tokens for remote services. If you would like to make a full backup of +your config (private key and secrets included), you must copy the config file +from your IPFS repository (IPFS_PATH). `, }, Type: make(map[string]interface{}), From 732adebaf68311df32ad4234d0aebd52f520b06f Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 29 Apr 2021 00:06:39 +0200 Subject: [PATCH 3/7] refactor: addressing code review --- go.mod | 1 + repo/common/common.go | 10 ++++++---- test/sharness/t0021-config.sh | 9 ++++++--- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index fc11cad29e0..d724c3f6cb5 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/fsnotify/fsnotify v1.4.9 github.com/gabriel-vasile/mimetype v1.2.0 github.com/go-bindata/go-bindata/v3 v3.1.3 + github.com/google/uuid v1.1.2 github.com/hashicorp/go-multierror v1.1.1 github.com/ipfs/go-bitswap v0.3.3 github.com/ipfs/go-block-format v0.0.3 diff --git a/repo/common/common.go b/repo/common/common.go index 14631c41162..9f733a3ae7c 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -4,6 +4,8 @@ import ( "fmt" "regexp" "strings" + + "github.com/google/uuid" ) // Find dynamic map key names passed as Parent["foo"] notation @@ -16,10 +18,10 @@ func keyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]s bracketedKeys := bracketsRe.FindAllString(key, -1) dynamicKeys = make(map[string]string, len(bracketedKeys)) normalizedKey = key - for i, mapKeySegment := range bracketedKeys { - mapKey := strings.TrimLeft(mapKeySegment, "[\"") - mapKey = strings.TrimRight(mapKey, "\"]") - placeholder := fmt.Sprintf("mapKey%d", i) + for _, mapKeySegment := range bracketedKeys { + mapKey := strings.TrimPrefix(mapKeySegment, `["`) + mapKey = strings.TrimSuffix(mapKey, `"]`) + placeholder := uuid.New().String() dynamicKeys[placeholder] = mapKey normalizedKey = strings.Replace(normalizedKey, mapKeySegment, fmt.Sprintf(".%s", placeholder), 1) } diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index 6aca21558c3..50d6c10f7c9 100755 --- a/test/sharness/t0021-config.sh +++ b/test/sharness/t0021-config.sh @@ -128,9 +128,12 @@ test_config_cmd() { # Dynamic keys with dot in their names test_config_cmd_set "--json" "Gateway.PublicGateways[\"some.example.com\"].UseSubdomains" "true" - test_expect_success "'ipfs config' output for dynamic keys looks good" ' - ipfs config Gateway.PublicGateways["some.example.com"].UseSubdomains > dynkey_out && - grep true dynkey_out + test_expect_success "'ipfs config show' after Foo[\"bar.buzz\"] returns a valid JSON" ' + test_expect_code 0 ipfs config show | jq > /dev/null 2>&1 + ' + test_expect_success "'ipfs config' after Foo[\"bar.buzz\"] shows updated value" ' + ipfs config Gateway.PublicGateways[\"some.example.com\"].UseSubdomains > bool_out && + grep true bool_out ' # SECURITY From 660cc66d4b81d2645d657356d7ae8f4053250845 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 29 Apr 2021 01:07:51 +0200 Subject: [PATCH 4/7] fix: sanitize secrets accessed via map key notation This ensures we dont leak access tokens of remote pinning services. --- core/commands/config.go | 4 +++- repo/common/common.go | 26 ++++++++++++++++++-------- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/core/commands/config.go b/core/commands/config.go index b074eb56842..d99dc3932c3 100644 --- a/core/commands/config.go +++ b/core/commands/config.go @@ -12,6 +12,7 @@ import ( "github.com/ipfs/go-ipfs/core/commands/cmdenv" "github.com/ipfs/go-ipfs/repo" + . "github.com/ipfs/go-ipfs/repo/common" "github.com/ipfs/go-ipfs/repo/fsrepo" "github.com/elgris/jsondiff" @@ -162,7 +163,8 @@ Values behind map key names that include dots can be accessed like this: // matchesGlobPrefix("foo.bar.baz", []string{"*", "bar"}) returns true // matchesGlobPrefix("foo.bar", []string{"baz", "*"}) returns false func matchesGlobPrefix(key string, glob []string) bool { - k := strings.Split(key, ".") + normalizedKey, _ := ConfigKeyToLookupData(key) + k := strings.Split(normalizedKey, ".") for i, g := range glob { if i >= len(k) { break diff --git a/repo/common/common.go b/repo/common/common.go index 9f733a3ae7c..2ab9ba747af 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -14,7 +14,7 @@ var bracketsRe = regexp.MustCompile(`\[([^\[\]]*)\]`) // Normalization for supporting arbitrary dynamic keys with dots: // Gateway.PublicGateways["gw.example.com"].UseSubdomains // Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable -func keyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]string) { +func ConfigKeyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]string) { bracketedKeys := bracketsRe.FindAllString(key, -1) dynamicKeys = make(map[string]string, len(bracketedKeys)) normalizedKey = key @@ -28,26 +28,36 @@ func keyToLookupData(key string) (normalizedKey string, dynamicKeys map[string]s return normalizedKey, dynamicKeys } +// Produces a part of config key with original map key names. +// Used only for better UX in error messages. +func buildSubKey(i int, parts []string, dynamicKeys map[string]string) string { + subkey := strings.Join(parts[:i], ".") + for placeholder, realKey := range dynamicKeys { + subkey = strings.Replace(subkey, fmt.Sprintf(".%s", placeholder), fmt.Sprintf(`["%s"]`, realKey), 1) + } + return subkey +} + func MapGetKV(v map[string]interface{}, key string) (interface{}, error) { var ok bool var mcursor map[string]interface{} var cursor interface{} = v - normalizedKey, dynamicKeys := keyToLookupData(key) + normalizedKey, dynamicKeys := ConfigKeyToLookupData(key) parts := strings.Split(normalizedKey, ".") for i, part := range parts { - sofar := strings.Join(parts[:i], ".") + sofar := buildSubKey(i, parts, dynamicKeys) mcursor, ok = cursor.(map[string]interface{}) if !ok { - return nil, fmt.Errorf("%q key is not a map", sofar) + return nil, fmt.Errorf("%s key is not a map", sofar) } if dynamicPart, ok := dynamicKeys[part]; ok { part = dynamicPart } cursor, ok = mcursor[part] if !ok { - return nil, fmt.Errorf("%q key has no attribute %q", sofar, part) + return nil, fmt.Errorf("%s key has no attribute %s", sofar, part) } } return cursor, nil @@ -58,13 +68,13 @@ func MapSetKV(v map[string]interface{}, key string, value interface{}) error { var mcursor map[string]interface{} var cursor interface{} = v - normalizedKey, dynamicKeys := keyToLookupData(key) + normalizedKey, dynamicKeys := ConfigKeyToLookupData(key) parts := strings.Split(normalizedKey, ".") for i, part := range parts { mcursor, ok = cursor.(map[string]interface{}) if !ok { - sofar := strings.Join(parts[:i], ".") - return fmt.Errorf("%q key is not a map", sofar) + sofar := buildSubKey(i, parts, dynamicKeys) + return fmt.Errorf("%s key is not a map", sofar) } if dynamicPart, ok := dynamicKeys[part]; ok { part = dynamicPart From 6d295eb9093156a41196f6b847895d9a078bdea9 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 29 Apr 2021 01:13:39 +0200 Subject: [PATCH 5/7] test(config): service secrets with json notation This adds quick test to ensure secrets are not leaked when alternative JSON notation is used for accessing service details when name includes a dot. --- test/sharness/t0021-config.sh | 2 +- test/sharness/t0700-remotepin.sh | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index 50d6c10f7c9..94b559484bf 100755 --- a/test/sharness/t0021-config.sh +++ b/test/sharness/t0021-config.sh @@ -129,7 +129,7 @@ test_config_cmd() { # Dynamic keys with dot in their names test_config_cmd_set "--json" "Gateway.PublicGateways[\"some.example.com\"].UseSubdomains" "true" test_expect_success "'ipfs config show' after Foo[\"bar.buzz\"] returns a valid JSON" ' - test_expect_code 0 ipfs config show | jq > /dev/null 2>&1 + ipfs config show | jq -e > /dev/null 2>&1 ' test_expect_success "'ipfs config' after Foo[\"bar.buzz\"] shows updated value" ' ipfs config Gateway.PublicGateways[\"some.example.com\"].UseSubdomains > bool_out && diff --git a/test/sharness/t0700-remotepin.sh b/test/sharness/t0700-remotepin.sh index 1aeb7a3ec39..06daf6ff16d 100755 --- a/test/sharness/t0700-remotepin.sh +++ b/test/sharness/t0700-remotepin.sh @@ -32,6 +32,7 @@ test_expect_success "test 'ipfs pin remote service ls' JSON on empty list" ' test_expect_success "creating test user on remote pinning service" ' echo CI host IP address ${TEST_PIN_SVC} && ipfs pin remote service add test_pin_svc ${TEST_PIN_SVC} ${TEST_PIN_SVC_KEY} && + ipfs pin remote service add test.dots.in.name ${TEST_PIN_SVC} ${TEST_PIN_SVC_KEY} && ipfs pin remote service add test_invalid_key_svc ${TEST_PIN_SVC} fake_api_key && ipfs pin remote service add test_invalid_url_path_svc ${TEST_PIN_SVC}/invalid-path fake_api_key && ipfs pin remote service add test_invalid_url_dns_svc https://invalid-service.example.com fake_api_key && @@ -100,6 +101,8 @@ test_expect_success "output does not include API.Key" ' test_expect_code 1 grep -q Key config_out ' +# dot notation + test_expect_success "'ipfs config Pinning.RemoteServices.test_pin_svc.API.Key' fails" ' test_expect_code 1 ipfs config Pinning.RemoteServices.test_pin_svc.API.Key 2> config_out ' @@ -116,6 +119,25 @@ test_expect_success "output includes meaningful error" ' test_cmp config_exp config_out ' +# json map key notation + +test_expect_success "'ipfs config Pinning.RemoteServices[\"test.dots.in.name\"]' fails" ' + test_expect_code 1 ipfs config Pinning.RemoteServices[\"test.dots.in.name\"] 2> config_out +' +test_expect_success "output includes meaningful error" ' + test_cmp config_exp config_out +' + +test_expect_success "'ipfs config Pinning.RemoteServices[\"test.dots.in.name\"].API.Key' fails" ' + test_expect_code 1 ipfs config Pinning.RemoteServices[\"test.dots.in.name\"].API.Key 2> config_out +' + +test_expect_success "output includes meaningful error" ' + test_cmp config_exp config_out +' + +# config show + test_expect_success "'ipfs config show' does not include Pinning.RemoteServices[*].API.Key" ' ipfs config show | tee show_config | jq -r .Pinning.RemoteServices > remote_services && test_expect_code 1 grep \"Key\" remote_services && From e1cbee782c2663e66abb4d5f3ba99c7f7142d4a7 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 29 Apr 2021 04:03:26 +0200 Subject: [PATCH 6/7] test(ci): deterministic jq Making jq verbose so we see what failed + using updated version for sharness. --- .circleci/config.yml | 5 +++++ test/sharness/t0021-config.sh | 1 + 2 files changed, 6 insertions(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index 8891a342da8..76e33334d84 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -30,6 +30,9 @@ default_environment: &default_environment CIRCLE_ARTIFACTS: /tmp/circleci-artifacts GIT_PAGER: cat +orbs: + jq: circleci/jq@2.2.0 + executors: golang: docker: @@ -131,6 +134,8 @@ jobs: steps: - run: sudo apt update - run: sudo apt install socat net-tools + - jq/install: + override: true - checkout - run: diff --git a/test/sharness/t0021-config.sh b/test/sharness/t0021-config.sh index 94b559484bf..5a0d42152d0 100755 --- a/test/sharness/t0021-config.sh +++ b/test/sharness/t0021-config.sh @@ -131,6 +131,7 @@ test_config_cmd() { test_expect_success "'ipfs config show' after Foo[\"bar.buzz\"] returns a valid JSON" ' ipfs config show | jq -e > /dev/null 2>&1 ' + # TODO: ipfs config show | jq -e > /dev/null 2>&1 test_expect_success "'ipfs config' after Foo[\"bar.buzz\"] shows updated value" ' ipfs config Gateway.PublicGateways[\"some.example.com\"].UseSubdomains > bool_out && grep true bool_out From 36b62c1f8ff339ce489410c76da754d3cbc6c1f0 Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Thu, 29 Apr 2021 23:55:18 +0200 Subject: [PATCH 7/7] fix: more strict regex https://github.com/ipfs/go-ipfs/pull/8096#discussion_r623167279 --- repo/common/common.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/repo/common/common.go b/repo/common/common.go index 2ab9ba747af..002e9219f9f 100644 --- a/repo/common/common.go +++ b/repo/common/common.go @@ -8,8 +8,8 @@ import ( "github.com/google/uuid" ) -// Find dynamic map key names passed as Parent["foo"] notation -var bracketsRe = regexp.MustCompile(`\[([^\[\]]*)\]`) +// Find dynamic map key names passed with Parent["foo"] notation +var bracketsRe = regexp.MustCompile(`\["([^\["\]]*)"\]`) // Normalization for supporting arbitrary dynamic keys with dots: // Gateway.PublicGateways["gw.example.com"].UseSubdomains