fix(config): support dynamic map keys with dots #8096

lidel · 2021-04-28T15:52:14Z

This PR adds support for alternative map notation in ipfs config command,
which enables access to values behind a map key that includes a dot, for example:

ipfs config 'Gateway.PublicGateways["dweb.link"].UseSubdomains'
ipfs config 'Pinning.RemoteServices["nft.storage"].Policies'

Context: ipfs/ipfs-webui#1770 (cc @alanshaw @aschmahmann)

Included sharness test + confirmed accessing fields like this works over HTTP API with regular curl as well:

$ ipfs pin remote service add nft.storage https://nft.storage/api [secret]
...
$ curl -sX POST 'http://127.0.0.1:5001/api/v0/config?arg=Pinning.RemoteServices%5B%22nft.storage%22%5D.Policies.MFS.Enable' | jq
{
  "Key": "Pinning.RemoteServices[\"nft.storage\"].Policies.MFS.Enable",
  "Value": false
}

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: ipfs/ipfs-webui#1770

aschmahmann · 2021-04-28T16:18:32Z

repo/common/common.go

+// 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) {


Putting this here isn't sufficient since we need to deal with things like https://github.com/ipfs/go-ipfs/blob/eea198f0811f50ab14c4f713de07ce93dffec9af/core/commands/config.go#L90 that exist above this level.

We'll either need to export some of this behavior from here, or move the other behavior here and export it.

Refactored in 660cc66 and added tests in 6d295eb

ps. I restored %s in error messages because entire error string gets sanitized already upstream, so we want to avoid double escaping.

Sharness tests fail on CI but pass locally, I'm suspecting different versions of jq, needs another look.

I made our CI use the latest jq instead of old 1.5, and it is green now.

This adds fallback to new notation from ipfs/kubo#8096 We default to old one because new one matters only when '.' is present in service name

aschmahmann · 2021-04-28T16:24:07Z

repo/common/common.go

+// Gateway.PublicGateways["gw.example.com"].UseSubdomains
+// Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable


Is this notation standard/used in other software or are we just making this up this form of escaping? Either way we'd need to document this in the config command.

This is how keys in JSON objects are addressed on the web, and since we use JSON for config anyway, means we don't invent anything new, but follow existing JSON convention.:

( {"foo.bar":{"a": "buz"}} )["foo.bar"].a → "buz"

I've added note about this notation to ipfs config --help in 0a2336e

The return value is not documented accurately on a public method. What about dynamicKeys?

repo/common/common.go

test/sharness/t0021-config.sh

aschmahmann · 2021-04-28T16:43:58Z

Thanks for tackling this, a little weird that this hasn't come up previously. I guess it means people have mostly been manipulating the raw JSON regarding config sections like for Gateways.

This ensures we dont leak access tokens of remote pinning services.

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.

repo/common/common.go

Making jq verbose so we see what failed + using updated version for sharness.

#8096 (comment)

This adds fallback to new notation from ipfs/kubo#8096 We default to old one because new one matters only when '.' is present in the service name

* fix: work around dot in pin service name This is a temporary workaround that maintains most of the functionality and works with go-ipfs 0.8.0 Context: #1770 * refactor: support Pinning.RemoteServices["name"] This adds fallback to new notation from ipfs/kubo#8096 We default to old one because new one matters only when '.' is present in the service name

petar · 2021-06-28T14:35:09Z

core/commands/config.go

@@ -157,7 +163,8 @@ Set the value of the 'Datastore.Path' key:
 //	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)


strings.EqualFold (below) no longer works, since now it is applied to a rewritten key which has UUIDs.

petar · 2021-06-28T14:46:42Z

repo/common/common.go

 )

+// Find dynamic map key names passed with Parent["foo"] notation
+var bracketsRe = regexp.MustCompile(`\["([^\["\]]*)"\]`)


This regular expression does not support escape sequences. I wouldn't advice writing your own quoted string parser. It's hard. I would advice using a JS parser library for parsing the ENTIRE glob, e.g. https://pkg.go.dev/github.com/robertkrimen/otto/parser, and then work with the AST.

Alternatively, you can use the Go language parser for the glob. It has a different set of escape sequences than JS, but is still better than rolling your own.

petar · 2021-06-28T14:48:10Z

repo/common/common.go

+// Gateway.PublicGateways["gw.example.com"].UseSubdomains
+// Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable


The return value is not documented accurately on a public method. What about dynamicKeys?

petar · 2021-06-28T14:51:23Z

repo/common/common.go

+		mapKey = strings.TrimSuffix(mapKey, `"]`)
+		placeholder := uuid.New().String()
+		dynamicKeys[placeholder] = mapKey
+		normalizedKey = strings.Replace(normalizedKey, mapKeySegment, fmt.Sprintf(".%s", placeholder), 1)


This will convert a glob string like: ["abc"]["def"] into .UUID1.UUID2
Is this your intention? Specifically, I am referring to the first key.

petar · 2021-06-28T14:57:03Z

repo/common/common.go

 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 := ConfigKeyToLookupData(key)
+	parts := strings.Split(normalizedKey, ".")


Fails of normalizedKey starts with a ".", which technically it can.

lidel · 2021-10-22T12:47:32Z

Note for myself or someone else who will pick this up again at some point:

we should fix both maps and arrays
array example: ipfs config --json Datastore.Spec.mounts.[0].child.sync false → does not work
- is anyone needs a workaround, leveraging jq does the trick: new_config=$( jq '.Datastore.Spec.mounts[0].child.sync = false' ~/.ipfs/config) && echo "${new_config}" > ~/.ipfs/config

lidel assigned aschmahmann Apr 28, 2021

aschmahmann reviewed Apr 28, 2021

View reviewed changes

lidel added a commit to ipfs/ipfs-webui that referenced this pull request Apr 28, 2021

refactor: support Pinning.RemoteServices["name"]

1c77960

This adds fallback to new notation from ipfs/kubo#8096 We default to old one because new one matters only when '.' is present in service name

aschmahmann reviewed Apr 28, 2021

View reviewed changes

aschmahmann requested changes Apr 28, 2021

View reviewed changes

repo/common/common.go Outdated Show resolved Hide resolved

repo/common/common.go Outdated Show resolved Hide resolved

test/sharness/t0021-config.sh Show resolved Hide resolved

lidel added 3 commits April 28, 2021 23:27

docs: ipfs config --help

0a2336e

refactor: addressing code review

732adeb

fix: sanitize secrets accessed via map key notation

660cc66

This ensures we dont leak access tokens of remote pinning services.

BigLep added this to the go-ipfs 0.9 milestone Apr 28, 2021

test(config): service secrets with json notation

6d295eb

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.

lidel force-pushed the fix/dot-in-config-maps-keys branch from 6a409fb to 6d295eb Compare April 29, 2021 00:58

lidel mentioned this pull request Apr 29, 2021

chore: update webui to 2.12.2 #8097

Merged

lidel force-pushed the fix/dot-in-config-maps-keys branch 5 times, most recently from f713b01 to 8eb4f00 Compare April 29, 2021 15:16

alanshaw reviewed Apr 29, 2021

View reviewed changes

repo/common/common.go Show resolved Hide resolved

lidel force-pushed the fix/dot-in-config-maps-keys branch 2 times, most recently from 56a34b5 to c772215 Compare April 29, 2021 19:27

lidel added 2 commits April 29, 2021 23:46

test(ci): deterministic jq

e1cbee7

Making jq verbose so we see what failed + using updated version for sharness.

fix: more strict regex

36b62c1

#8096 (comment)

lidel force-pushed the fix/dot-in-config-maps-keys branch from c772215 to 36b62c1 Compare April 29, 2021 21:56

lidel requested a review from aschmahmann April 29, 2021 22:23

lidel added a commit to ipfs/ipfs-webui that referenced this pull request Apr 30, 2021

refactor: support Pinning.RemoteServices["name"]

4aa78ba

This adds fallback to new notation from ipfs/kubo#8096 We default to old one because new one matters only when '.' is present in the service name

lidel added the topic/config Topic config label May 5, 2021

aschmahmann removed this from the go-ipfs 0.9 milestone May 13, 2021

aschmahmann assigned petar and unassigned aschmahmann Jun 25, 2021

petar requested changes Jun 28, 2021

View reviewed changes

lidel marked this pull request as draft September 3, 2021 17:46

lidel mentioned this pull request Sep 3, 2021

Docker override config from env variables #8326

Closed

lidel unassigned petar Oct 22, 2021

lidel added effort/hours Estimated to take one or several hours help wanted Seeking public contribution on this issue labels Oct 22, 2021

BigLep added this to the TBD milestone Mar 10, 2022

BigLep modified the milestones: TBD, Best Effort Track Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(config): support dynamic map keys with dots #8096

fix(config): support dynamic map keys with dots #8096

lidel commented Apr 28, 2021 •

edited

Loading

aschmahmann Apr 28, 2021

lidel Apr 28, 2021 •

edited

Loading

lidel Apr 29, 2021

lidel Apr 29, 2021

aschmahmann Apr 28, 2021 •

edited

Loading

lidel Apr 28, 2021 •

edited

Loading

petar Jun 28, 2021

aschmahmann commented Apr 28, 2021

petar Jun 28, 2021

petar Jun 28, 2021

petar Jun 28, 2021

petar Jun 28, 2021

petar Jun 28, 2021

petar Jun 28, 2021

lidel commented Oct 22, 2021

		// Gateway.PublicGateways["gw.example.com"].UseSubdomains
		// Pinning.RemoteServices["pins.example.org"].Policies.MFS.Enable

fix(config): support dynamic map keys with dots #8096

Are you sure you want to change the base?

fix(config): support dynamic map keys with dots #8096

Conversation

lidel commented Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

lidel Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschmahmann Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

lidel Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschmahmann commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidel commented Oct 22, 2021

lidel commented Apr 28, 2021 •

edited

Loading

lidel Apr 28, 2021 •

edited

Loading

aschmahmann Apr 28, 2021 •

edited

Loading

lidel Apr 28, 2021 •

edited

Loading