feat: configure insecure DID resolution - #401
Conversation
| AuthorizedRetrievals *bool `mapstructure:"authorized_retrievals" toml:"authorized_retrievals"` | ||
| // InsecureDIDResolution enables HTTP (instead of HTTPS) for did:web resolution. | ||
| // NB: this should only be used for development purposes. | ||
| InsecureDIDResolution bool `mapstructure:"insecure_did_resolution" toml:"insecure_did_resolution,omitempty"` |
There was a problem hiding this comment.
Sorry for the long comment here:
There is a Viper registration gap here, GUPPY_NETWORK_INSECURE_DID_RESOLUTION silently won't work, and this is a pattern I think we keep getting wrong across our projects, so I want to flag it here before it lands, not meaning to single out this PR, but it makes a good example of the problem.
Adding InsecureDIDResolution with just a mapstructure tag registers the field with our struct, but Viper has no idea the key exists until something tells it so. Viper only "knows" a key when one of these has happened:
viper.SetDefault("network.insecure_did_resolution", …)viper.BindEnv("network.insecure_did_resolution")viper.BindPFlag("network.insecure_did_resolution", …)- The key is physically present in a config file that Viper reads.
viper.AutomaticEnv() is misleading, it does not scan the environment for arbitrary keys. It only consults env vars for keys Viper already knows about (see spf13/viper#761, spf13/viper#584, spf13/viper#1012 for examples). And Unmarshal decodes from AllSettings(), which is built from that same known-keys registry.
What this means in practice is that commands like GUPPY_NETWORK_INSECURE_DID_RESOLUTION=true guppy retrieve are silently ignored. The key isn't registered, so Viper never looks at the env var, and Unmarshal leaves the struct field as its zero value. But, dropping insecure_did_resolution = true into ~/.config/guppy/config.toml works, because reading the file registers the key as a side effect, which then makes the env var fallback "magically" start working for subsequent runs (until the key is removed from the file again).
So the feature appears to work during manual testing with a config file, then breaks the moment someone tries to enable it via an env var in a container/CI environment which, given this is a dev/debug knob that people will reach for from a shell, is where I suspect it'll be used.
More broadly, every network.* field that has a matching BindPFlag in cmd/root.go works correctly via env vars. network.authorized_retrievals and now network.insecure_did_resolution do not, since they rely on a config file being the source of truth. I suspect we have the same latent bug in other repos where we've leaned on struct tags alone, Piri 100% has this issue in some edge cases.
I think the rule we could adopt (and document) is: every Viper-backed config field must be explicitly registered, via one of:
viper.SetDefault(...)at minimum, if it's purely file/env driven, orBindPFlagagainst a cobra flag, preferred for anything user-facing, because it also makes the option discoverable via--help.
(I find both options to be tedious, but can't think of a better alternative, some nice-ish work arounds in the viper issues linked to above)
For this specific field I'd lean toward a hidden persistent flag (like we do for --replicas) bound with BindPFlag, so env var, flag, and config-file paths all go through the same registered key. At the very least, a SetDefault("network.insecure_did_resolution", false) in initConfig would make the env-var path work, but we'd still be missing the discoverability that a flag gives us.
There was a problem hiding this comment.
I am adding BindPFlag and MarkHidden for insecure-did-resolution. Not doing anything with authorized-retrievals since it is now legacy and can be removed - at the end of the month there is no network that setting this flag would work with.
3aae4bc
into
ash/feat/configure-replication-count
No description provided.