diff --git a/instconf/instconf.go b/instconf/instconf.go new file mode 100644 index 0000000000000000000000000000000000000000..8a48f86a3ff5996f691f4c3de314256bb973e69a --- /dev/null +++ b/instconf/instconf.go @@ -0,0 +1,307 @@ +// Package instconf is one reading of the origins in a self-hosted SourceHut +// instance's shared config.ini — the "where does this service live" strings +// that every custom service on the instance has to agree about. +// +// It exists because they did not agree. Five services and this library each +// grew their own copy of the same three lines, and the copies drifted in ways +// that are invisible until something breaks far from the config: one service +// canonicalized an origin with strings.TrimRight(o, "/") and another with +// strings.TrimSuffix(o, "/"), so a config.ini carrying "https://x//" produced +// two different origins in two daemons that must produce the same string when +// one of them checks a browser-supplied Origin header against it. One repo held +// two origin-to-host extractors that disagreed about a malformed origin — one +// returned an error, the other quietly answered "localhost". A third copy feeds +// the DNS-rebinding guard on an MCP endpoint, where an empty host means the +// guard turns itself off. That is not a place for a fourth spelling. +// +// So: one canonicalization, one host extraction, one internal/external +// distinction, used by everyone. +// +// The origin vocabulary this package reads is upstream sourcehut's, and the +// accessors correspond to core-go's config.GetOrigin and config.GetAPI: +// +// - [ExternalOrigin] — [] origin, the address a browser is sent to. +// - [InternalOrigin] — [] internal-origin, falling back to origin: the +// address a daemon dials from inside the instance's network. +// - [InternalAPIOrigin] — the four-key ladder api-internal-origin, +// internal-origin, api-origin, origin, for a service calling a sibling's +// GraphQL API. +// +// Two named accessors rather than one taking an external bool, deliberately. +// The donors spelled it config.GetOrigin(conf, svc, true) and +// config.GetOrigin(conf, svc, false) in different files, each with a comment +// explaining which was which, and a flipped flag is invisible at the call site: +// it shows up as a browser redirected to an address only the daemon can reach, +// or as a daemon dialing out through the public load balancer. +// +// Everything returned is canonical — whitespace-trimmed and stripped of every +// trailing slash — so a URL built by joining an origin with a path never grows +// a double slash and two callers never hold two spellings of the same address. +// An absent key, an absent section and a present-but-blank value all read as +// "", which the caller is expected to treat as "not configured": see [Require] +// for reporting that in one pass rather than one restart per missing key. +package instconf + +import ( + "errors" + "fmt" + "net/url" + "strings" + + "github.com/vaughan0/go-ini" +) + +// ErrIncompleteConfig is the sentinel behind every error [Require] returns, so +// a daemon can tell "the operator has not finished configuring me" apart from +// the failures that come later. +var ErrIncompleteConfig = errors.New("incomplete configuration") + +// apiOriginKeys is the internal API-origin ladder, in preference order. It is +// upstream core-go's own candidate list for config.GetAPI with external=false, +// restated here because config.GetAPI panics when none of them is set, which is +// no way to tell an operator about a missing config key. +var apiOriginKeys = []string{ + "api-internal-origin", + "internal-origin", + "api-origin", + "origin", +} + +// APIOriginKeys returns the key names [InternalAPIOrigin] consults, in +// preference order. Mostly useful for [NeedAny], so that a startup check and +// the lookup itself cannot drift apart. +func APIOriginKeys() []string { + return append([]string(nil), apiOriginKeys...) +} + +// CanonicalOrigin returns the one spelling of an origin: surrounding whitespace +// removed, then every trailing slash removed. "" stays "". +// +// Both trims matter, and both come from a donor that had been bitten: +// +// - TrimSpace, because a value can reach this function from somewhere other +// than the ini parser (a test fixture, a flag, a config assembled in code), +// and " https://x" does not even parse as a URL. +// - TrimRight over TrimSuffix, because TrimSuffix removes one slash and +// leaves "https://x/" from "https://x//". The donors used one each. This is +// the stricter reading, and it is the one the comparison callers need: an +// Origin header from a browser never carries a trailing slash, so an origin +// that kept one silently fails every equality check made against it. +// +// Only trailing slashes are touched. The scheme, host, port and any path +// prefix are left exactly as configured — an instance that serves a service +// under https://example.org/git means it. +func CanonicalOrigin(origin string) string { + return strings.TrimRight(strings.TrimSpace(origin), "/") +} + +// ExternalOrigin returns the canonical external origin of a service: the +// address a browser is sent to, from [
] origin. +// +// This is the origin that belongs in a page, a redirect, a webhook payload or +// an email — anything a user's own client will follow. It returns "" when the +// section or the key is missing, or the value is blank. +func ExternalOrigin(conf ini.File, section string) string { + return CanonicalOrigin(lookup(conf, section, "origin")) +} + +// InternalOrigin returns the canonical origin a daemon should dial to reach a +// service from inside the instance: [
] internal-origin when set, +// otherwise [
] origin. +// +// It is the address of the same service, not the same address: an instance may +// route service-to-service traffic over a private network, and on such an +// instance the external origin resolves to a load balancer the daemon cannot +// or should not use. Never put this string in front of a user. +func InternalOrigin(conf ini.File, section string) string { + if v := lookup(conf, section, "internal-origin"); v != "" { + return CanonicalOrigin(v) + } + return CanonicalOrigin(lookup(conf, section, "origin")) +} + +// InternalAPIOrigin returns the canonical origin at which a service's GraphQL +// API can be reached from inside the instance, and whether one is configured at +// all. The returned origin does not include the /query path. +// +// It walks [APIOriginKeys] in order, taking the first key that is present and +// non-blank. The ladder exists because an instance may put the API behind a +// different address than the web UI, and may or may not have a separate +// internal route to either; every service that calls a sibling's API has to +// walk it, because core-go's config.GetAPI panics when it reaches the end of +// the ladder with nothing. +// +// The bool is the point: it turns that panic into a decision the caller makes +// at startup, alongside every other missing key, rather than a stack trace on +// the first authorization request. +func InternalAPIOrigin(conf ini.File, section string) (string, bool) { + for _, key := range apiOriginKeys { + if v := lookup(conf, section, key); v != "" { + return CanonicalOrigin(v), true + } + } + return "", false +} + +// OriginHost returns the host name of an origin, without any port: the string +// to compare a request's Host header against. It returns "" when the origin is +// blank, does not parse as a URL, or parses to no host at all — which is what a +// scheme-less value such as "example.org" does, since a URL without a scheme is +// a path. +// +// "" means "this origin names no host", and callers on a security path must +// treat it as a configuration error rather than as permission to skip a check. +// One donor uses this value for the DNS-rebinding guard on an MCP endpoint and +// disables the guard when it comes back empty (loudly, at warn level, which is +// the only reason that is defensible). Another donor's copy answered "localhost" +// for anything it could not parse, which is worse: it is a guess that looks like +// an answer, and it silently makes every malformed origin agree with a local +// client. +// +// The host is returned exactly as configured, case included, because it is also +// what identifies the endpoint elsewhere. Host names are case-insensitive, so +// compare with strings.EqualFold and not ==. +// +// A host name is not an origin, so the two live under two names: use +// [CanonicalOrigin] when what is wanted back is an address to fetch. +func OriginHost(origin string) string { + u, err := url.Parse(CanonicalOrigin(origin)) + if err != nil { + return "" + } + return u.Hostname() +} + +// OriginAuthority returns the authority of an origin — host[:port], with an +// IPv6 host still bracketed — or "" under exactly the conditions [OriginHost] +// returns "". +// +// It is the other half of a disagreement between two copies in one repo: a +// Host-header check wants the name alone, because it compares against a request +// Host whose port it has already stripped, while a JWT audience, a sealed URL +// or a synthesized email domain wants the authority that actually identifies +// the endpoint — https://x:8080 and https://x:9090 are two audiences. Neither +// is the general answer, so both are here and the caller names which one it +// means. +func OriginAuthority(origin string) string { + u, err := url.Parse(CanonicalOrigin(origin)) + if err != nil { + return "" + } + if u.Hostname() == "" { + return "" + } + return u.Host +} + +// Key is one configuration requirement: a section, and the key names that +// satisfy it. More than one name means an alternation — any of them will do — +// which is how the internal API-origin ladder is expressed. Build one with +// [Need] or [NeedAny]. +type Key struct { + Section string + Names []string +} + +// Need is a requirement for one named key in a section. +func Need(section, name string) Key { + return Key{Section: section, Names: []string{name}} +} + +// NeedAny is a requirement satisfied by any one of several keys in a section, +// e.g. NeedAny("git.sr.ht", instconf.APIOriginKeys()...). +func NeedAny(section string, names ...string) Key { + return Key{Section: section, Names: names} +} + +// String renders the requirement the way an operator has to read it back into +// config.ini: "[git.sr.ht] origin", or "[git.sr.ht] one of api-internal-origin, +// internal-origin, api-origin, origin". +func (k Key) String() string { + switch len(k.Names) { + case 0: + return fmt.Sprintf("[%s] ", k.Section) + case 1: + return fmt.Sprintf("[%s] %s", k.Section, k.Names[0]) + default: + return fmt.Sprintf("[%s] one of %s", k.Section, strings.Join(k.Names, ", ")) + } +} + +// satisfied reports whether the config has a non-blank value for any of the +// key's names. A requirement with no names is unsatisfiable by construction, +// which surfaces the programming mistake instead of silently passing. +func (k Key) satisfied(conf ini.File) bool { + for _, name := range k.Names { + if lookup(conf, k.Section, name) != "" { + return true + } + } + return false +} + +// MissingKeysError reports every configuration key a caller required and did +// not find. It wraps [ErrIncompleteConfig]. +type MissingKeysError struct { + Keys []Key +} + +// Error lists every missing key on one line. +func (e *MissingKeysError) Error() string { + return fmt.Sprintf("%s; missing required keys: %s", + ErrIncompleteConfig, strings.Join(e.Strings(), ", ")) +} + +// Unwrap makes errors.Is(err, ErrIncompleteConfig) work. +func (e *MissingKeysError) Unwrap() error { return ErrIncompleteConfig } + +// Strings renders the missing keys one per element, for a structured log +// attribute that keeps them a list — a slog handler will then render them as a +// list, and the operator gets every gap at once instead of one per line. +func (e *MissingKeysError) Strings() []string { + out := make([]string, 0, len(e.Keys)) + for _, k := range e.Keys { + out = append(out, k.String()) + } + return out +} + +// Require checks that every given key is present and non-blank, and reports +// *all* the missing ones in a single [*MissingKeysError]. It returns nil when +// nothing is missing. +// +// Reporting all of them is the entire point, and it is the reason this is a +// function rather than five ifs at each daemon's startup: a daemon that fatals +// on the first gap it finds costs the operator one restart per missing key, and +// an operator editing config.ini wants the whole list in front of them. It also +// wants to run before anything reaches core-go's config.GetAPI or +// crypto.InitCrypto, both of which panic on missing configuration with a +// message aimed at a programmer. +// +// A present-but-blank value counts as missing: "origin =" in config.ini is a +// half-finished edit, not a configured empty origin. +func Require(conf ini.File, keys ...Key) error { + var missing []Key + for _, k := range keys { + if !k.satisfied(conf) { + missing = append(missing, k) + } + } + if len(missing) == 0 { + return nil + } + return &MissingKeysError{Keys: missing} +} + +// lookup reads one key, treating a present-but-blank value as absent and +// trimming what it returns. The ini parser already trims, but a File can be +// built in code — every test fixture in this repo does — and a lookup that +// depends on which of the two produced the map is exactly the kind of +// disagreement this package exists to remove. +func lookup(conf ini.File, section, key string) string { + v, ok := conf.Get(section, key) + if !ok { + return "" + } + return strings.TrimSpace(v) +} diff --git a/instconf/instconf_test.go b/instconf/instconf_test.go new file mode 100644 index 0000000000000000000000000000000000000000..4dc23a14ded790f9f369d39945b37312c96f7da3 --- /dev/null +++ b/instconf/instconf_test.go @@ -0,0 +1,296 @@ +package instconf_test + +import ( + "errors" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/vaughan0/go-ini" + + "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" +) + +// parse builds a File the way a daemon gets one, so the tests exercise the +// parser's own trimming rather than a map the test wrote by hand. +func parse(t *testing.T, text string) ini.File { + t.Helper() + conf, err := ini.Load(strings.NewReader(text)) + require.NoError(t, err) + return conf +} + +func TestCanonicalOriginTrimsTrailingSlashesAndSpace(t *testing.T) { + for name, tc := range map[string]struct{ in, want string }{ + "empty stays empty": {"", ""}, + "blank stays empty": {" ", ""}, + "no slash untouched": {"https://x.example.org", "https://x.example.org"}, + "one trailing slash": {"https://x.example.org/", "https://x.example.org"}, + "several slashes": {"https://x.example.org///", "https://x.example.org"}, + "trailing space": {"https://x.example.org ", "https://x.example.org"}, + "leading space": {" https://x.example.org", "https://x.example.org"}, + "space then slashes": {" https://x.example.org// ", "https://x.example.org"}, + "port kept": {"https://x.example.org:8443/", "https://x.example.org:8443"}, + "path prefix kept": {"https://example.org/git/", "https://example.org/git"}, + "scheme-less untouched": {"x.example.org/", "x.example.org"}, + } { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.want, instconf.CanonicalOrigin(tc.in)) + }) + } +} + +// The donors disagreed here: one canonicalized with strings.TrimSuffix, which +// leaves "https://x/" from "https://x//", and one with strings.TrimRight, which +// does not. This pins the strict reading, because the whole point is that the +// string compares equal to a browser's Origin header. +func TestCanonicalOriginBeatsTrimSuffix(t *testing.T) { + const doubled = "https://x.example.org//" + assert.Equal(t, "https://x.example.org/", strings.TrimSuffix(doubled, "/"), + "sanity: TrimSuffix is the behaviour being overruled") + assert.Equal(t, "https://x.example.org", instconf.CanonicalOrigin(doubled)) +} + +func TestExternalOriginReadsOriginOnly(t *testing.T) { + conf := parse(t, ` +[git.sr.ht] +origin=https://git.example.org/ +internal-origin=http://git.internal:5001 +`) + assert.Equal(t, "https://git.example.org", instconf.ExternalOrigin(conf, "git.sr.ht"), + "internal-origin must never reach a browser") +} + +func TestExternalOriginMissing(t *testing.T) { + conf := parse(t, ` +[git.sr.ht] +internal-origin=http://git.internal:5001 + +[meta.sr.ht] +origin= +`) + assert.Empty(t, instconf.ExternalOrigin(conf, "git.sr.ht"), "key absent") + assert.Empty(t, instconf.ExternalOrigin(conf, "meta.sr.ht"), "key present but blank") + assert.Empty(t, instconf.ExternalOrigin(conf, "nope.sr.ht"), "section absent") + assert.Empty(t, instconf.ExternalOrigin(nil, "git.sr.ht"), "no config at all") +} + +func TestInternalOriginPrefersInternal(t *testing.T) { + conf := parse(t, ` +[git.sr.ht] +origin=https://git.example.org +internal-origin=http://git.internal:5001/ +`) + assert.Equal(t, "http://git.internal:5001", instconf.InternalOrigin(conf, "git.sr.ht")) +} + +func TestInternalOriginFallsBackToOrigin(t *testing.T) { + conf := parse(t, ` +[git.sr.ht] +origin=https://git.example.org/ + +[meta.sr.ht] +origin=https://meta.example.org +internal-origin= +`) + assert.Equal(t, "https://git.example.org", instconf.InternalOrigin(conf, "git.sr.ht"), + "internal-origin absent") + assert.Equal(t, "https://meta.example.org", instconf.InternalOrigin(conf, "meta.sr.ht"), + "internal-origin present but blank must not shadow origin") + assert.Empty(t, instconf.InternalOrigin(conf, "nope.sr.ht")) +} + +func TestInternalAPIOriginLadder(t *testing.T) { + full := ` +[git.sr.ht] +api-internal-origin=http://git.internal:5101/ +internal-origin=http://git.internal:5001 +api-origin=https://api.git.example.org +origin=https://git.example.org +` + for name, tc := range map[string]struct { + drop []string + want string + }{ + "api-internal-origin wins": {nil, "http://git.internal:5101"}, + "then internal-origin": { + []string{"api-internal-origin"}, "http://git.internal:5001"}, + "then api-origin": { + []string{"api-internal-origin", "internal-origin"}, "https://api.git.example.org"}, + "then origin": { + []string{"api-internal-origin", "internal-origin", "api-origin"}, + "https://git.example.org"}, + } { + t.Run(name, func(t *testing.T) { + text := full + for _, k := range tc.drop { + text = dropKey(t, text, k) + } + got, ok := instconf.InternalAPIOrigin(parse(t, text), "git.sr.ht") + require.True(t, ok) + assert.Equal(t, tc.want, got) + }) + } +} + +func TestInternalAPIOriginNotConfigured(t *testing.T) { + conf := parse(t, ` +[git.sr.ht] +repos=/var/lib/git + +[meta.sr.ht] +origin= +`) + got, ok := instconf.InternalAPIOrigin(conf, "git.sr.ht") + assert.False(t, ok, "no candidate key at all") + assert.Empty(t, got) + + got, ok = instconf.InternalAPIOrigin(conf, "meta.sr.ht") + assert.False(t, ok, "a blank origin is not a configured API origin") + assert.Empty(t, got) + + got, ok = instconf.InternalAPIOrigin(conf, "nope.sr.ht") + assert.False(t, ok, "section absent") + assert.Empty(t, got) +} + +func TestAPIOriginKeysMatchesTheLadder(t *testing.T) { + want := []string{"api-internal-origin", "internal-origin", "api-origin", "origin"} + assert.Equal(t, want, instconf.APIOriginKeys()) + + keys := instconf.APIOriginKeys() + keys[0] = "clobbered" + assert.Equal(t, want, instconf.APIOriginKeys(), "the exported ladder must not be writable") +} + +func TestOriginHostAndAuthority(t *testing.T) { + for name, tc := range map[string]struct{ in, host, authority string }{ + "plain": {"https://git.example.org", "git.example.org", "git.example.org"}, + "trailing slash": {"https://git.example.org/", "git.example.org", "git.example.org"}, + "several slashes": {"https://git.example.org//", "git.example.org", "git.example.org"}, + "surrounding space": {" https://git.example.org ", "git.example.org", "git.example.org"}, + "with port": {"https://git.example.org:8443", "git.example.org", "git.example.org:8443"}, + "path prefix": {"https://example.org/git", "example.org", "example.org"}, + "ipv6 with port": {"http://[::1]:8080", "::1", "[::1]:8080"}, + "scheme relative": {"//git.example.org", "git.example.org", "git.example.org"}, + "case as configured": {"https://Git.Example.ORG", "Git.Example.ORG", "Git.Example.ORG"}, + + // Everything below has no host to report. "" is the answer in every + // case; a caller on a security path must not read it as "allow". + "empty": {"", "", ""}, + "blank": {" ", "", ""}, + "scheme-less": {"git.example.org", "", ""}, + "bare word": {"not-a-url", "", ""}, + "no scheme": {"://git.example.org", "", ""}, + "space inside": {"https://git example.org", "", ""}, + "bad escape": {"https://example.org/%zz", "", ""}, + "unclosed ipv6": {"http://[::1", "", ""}, + } { + t.Run(name, func(t *testing.T) { + assert.Equal(t, tc.host, instconf.OriginHost(tc.in), "OriginHost") + assert.Equal(t, tc.authority, instconf.OriginAuthority(tc.in), "OriginAuthority") + }) + } +} + +// The donor that answered "localhost" for an unparseable origin made every +// malformed config agree with a local client. This package refuses to guess. +func TestOriginHostNeverInventsLocalhost(t *testing.T) { + for _, bad := range []string{"", " ", "not-a-url", "://x", "http://[::1"} { + assert.Empty(t, instconf.OriginHost(bad), "origin %q", bad) + assert.Empty(t, instconf.OriginAuthority(bad), "origin %q", bad) + } +} + +func TestRequireSatisfied(t *testing.T) { + conf := parse(t, ` +[sr.ht] +network-key=abc + +[webhooks] +private-key=def + +[git.sr.ht] +internal-origin=http://git.internal:5001 + +[compare.sr.ht] +origin=https://compare.example.org +`) + err := instconf.Require(conf, + instconf.Need("sr.ht", "network-key"), + instconf.Need("webhooks", "private-key"), + instconf.Need("compare.sr.ht", "origin"), + instconf.NeedAny("git.sr.ht", instconf.APIOriginKeys()...), + ) + assert.NoError(t, err) + assert.NoError(t, instconf.Require(conf), "no requirements is not a failure") +} + +func TestRequireReportsEveryMissingKeyAtOnce(t *testing.T) { + conf := parse(t, ` +[sr.ht] +network-key=abc + +[meta.sr.ht] +origin= + +[git.sr.ht] +repos=/var/lib/git +`) + err := instconf.Require(conf, + instconf.Need("sr.ht", "network-key"), // present + instconf.Need("webhooks", "private-key"), // section absent + instconf.Need("meta.sr.ht", "origin"), // present but blank + instconf.Need("compare.sr.ht", "origin"), // key absent + instconf.NeedAny("git.sr.ht", instconf.APIOriginKeys()...), // ladder empty + ) + require.Error(t, err) + require.ErrorIs(t, err, instconf.ErrIncompleteConfig) + + var missing *instconf.MissingKeysError + require.ErrorAs(t, err, &missing) + assert.Equal(t, []string{ + "[webhooks] private-key", + "[meta.sr.ht] origin", + "[compare.sr.ht] origin", + "[git.sr.ht] one of api-internal-origin, internal-origin, api-origin, origin", + }, missing.Strings(), "every gap in one pass, in the order asked") + + msg := err.Error() + assert.Contains(t, msg, "incomplete configuration") + assert.Contains(t, msg, "[compare.sr.ht] origin") + assert.NotContains(t, msg, "network-key", "a satisfied key must not be reported") +} + +func TestRequireNamelessKeyIsUnsatisfiable(t *testing.T) { + conf := parse(t, "[sr.ht]\nnetwork-key=abc\n") + err := instconf.Require(conf, instconf.NeedAny("sr.ht")) + require.Error(t, err) + assert.Contains(t, err.Error(), "[sr.ht] ") +} + +// dropKey removes an "key=..." line from an ini fixture. +func dropKey(t *testing.T, text, key string) string { + t.Helper() + var kept []string + var dropped bool + for _, line := range strings.Split(text, "\n") { + if strings.HasPrefix(line, key+"=") { + dropped = true + continue + } + kept = append(kept, line) + } + require.True(t, dropped, "fixture has no %q line", key) + return strings.Join(kept, "\n") +} + +// The sentinel is what a daemon matches on to tell "not configured yet" apart +// from the failures that come after startup, so check it with errors.Is +// directly rather than only through testify. +func TestErrIncompleteConfigIsTheSentinel(t *testing.T) { + err := instconf.Require(nil, instconf.Need("sr.ht", "network-key")) + require.Error(t, err) + assert.True(t, errors.Is(err, instconf.ErrIncompleteConfig)) +}