From 8ba716c343fd6c99a38fe208cb07268c08ac2f18 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 9 Aug 2026 00:27:40 +0300 Subject: [PATCH] instconf: take the origin and required-key helpers from ecore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two copies of one function disagreed in this repo: hostFromOrigin returned an error for a malformed origin and web's hostOf answered "localhost", which is a guess that looks like an answer. Both are gone; the caller now names which half it means, and both wanted OriginAuthority — a port is part of a sealed-URL host, a JWT audience and the synthesized commit-author domain alike. The startup checks become one Require, so an operator filling in a fresh config.ini reads every missing key off one boot instead of one per restart. The hook's internal-origin read becomes InternalOrigin, which falls back to the external origin: an instance with only a public address is not misconfigured and used to be refused. And the git-description mirror is wired only when git.sr.ht has an API origin — web.Config already documented a nil Git as no mirroring, but nothing produced one, so an instance without git.sr.ht met config.GetAPI's panic on the first push. --- cmd/dolt-git-hook/main.go | 32 +++++++++------ cmd/doltsrht/main.go | 70 +++++++++++++++++--------------- cmd/doltsrht/main_test.go | 84 +++++++++++++++++++++++---------------- 3 files changed, 107 insertions(+), 79 deletions(-) diff --git a/cmd/dolt-git-hook/main.go b/cmd/dolt-git-hook/main.go index 38008328a57ba89cc8273548235b281aadac0966..fa2994aee688d125a18df7414e45b2a1ac77b433 100644 --- a/cmd/dolt-git-hook/main.go +++ b/cmd/dolt-git-hook/main.go @@ -34,6 +34,7 @@ import ( "sourcecraft.dev/bigbes/sr-ht-core/config" "sourcecraft.dev/bigbes/sr-ht-core/crypto" + "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" "sourcecraft.dev/bigbes/sr-ht-ecore/internalauth" "sourcecraft.dev/bigbes/sr-ht-dolt/core" @@ -44,6 +45,9 @@ import ( // to this binary. Overridable via env for testing. const defaultDelegate = "/usr/bin/git.sr.ht-update-hook" +// doltService is the config section of the service this hook provisions into. +const doltService = "dolt.sr.ht" + // provisionTimeout bounds the internal create call so a slow or down dolt.sr.ht // never adds more than this to a push. const provisionTimeout = 5 * time.Second @@ -125,21 +129,23 @@ func provisionDolt() { // crypto.InitCrypto log.Fatalf's (os.Exit) on a missing key, which recover // cannot catch — check the keys ourselves first so a misconfigured instance - // degrades to a skipped companion, never a hard-exiting hook. - if _, ok := conf.Get("sr.ht", "network-key"); !ok { - fmt.Fprintln(os.Stderr, "dolt-git-hook: [sr.ht]network-key not set; skipping companion provisioning") - return - } - if _, ok := conf.Get("webhooks", "private-key"); !ok { - fmt.Fprintln(os.Stderr, "dolt-git-hook: [webhooks]private-key not set; skipping companion provisioning") + // degrades to a skipped companion, never a hard-exiting hook. Everything + // this call needs is asked for at once, so an operator whose config is + // incomplete reads the whole list off one push instead of one key per push. + if err := instconf.Require(conf, + instconf.Need("sr.ht", "network-key"), + instconf.Need("webhooks", "private-key"), + instconf.NeedAny(doltService, "internal-origin", "origin"), + ); err != nil { + fmt.Fprintf(os.Stderr, "dolt-git-hook: %v; skipping companion provisioning\n", err) return } - origin, ok := conf.Get("dolt.sr.ht", "internal-origin") - if !ok || origin == "" { - fmt.Fprintln(os.Stderr, "dolt-git-hook: [dolt.sr.ht]internal-origin not set; skipping companion provisioning") - return - } + // The internal origin when the instance has one, the external origin when it + // does not. An instance that routes service-to-service traffic over a private + // network sets the first; one that has only the public address is not + // misconfigured, and used to be refused here for it. + origin := instconf.InternalOrigin(conf, doltService) crypto.InitCrypto(conf) @@ -175,7 +181,7 @@ func createCompanion(out io.Writer, origin string, pc pushContext) { } req, err := http.NewRequest("POST", - strings.TrimRight(origin, "/")+"/internal/repos", bytes.NewReader(body)) + instconf.CanonicalOrigin(origin)+"/internal/repos", bytes.NewReader(body)) if err != nil { return } diff --git a/cmd/doltsrht/main.go b/cmd/doltsrht/main.go index 31867ad8fffd176ea3c16e07db920efc73f9bf06..c8a082a94f37ce10d977d18244e501bec7191b51 100644 --- a/cmd/doltsrht/main.go +++ b/cmd/doltsrht/main.go @@ -22,7 +22,6 @@ import ( "database/sql" "fmt" "log/slog" - "net/url" "os" "github.com/go-chi/chi/v5" @@ -38,6 +37,8 @@ import ( "sourcecraft.dev/bigbes/sr-ht-core/database" "sourcecraft.dev/bigbes/sr-ht-core/server" + "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" + "sourcecraft.dev/bigbes/sr-ht-dolt/authn" "sourcecraft.dev/bigbes/sr-ht-dolt/remoteapi" "sourcecraft.dev/bigbes/sr-ht-dolt/storage" @@ -94,26 +95,36 @@ type settings struct { // resolveSettings reads the [dolt.sr.ht] section, applying defaults and failing // on the keys that have no sensible default (connection-string and origin). +// +// Both gaps are reported together rather than one per boot. An operator filling +// in a fresh config.ini wants the whole list in front of them, not one key per +// restart, which is what instconf.Require is for. func resolveSettings(conf ini.File) (settings, error) { - connString, ok := conf.Get(serviceName, "connection-string") - if !ok || connString == "" { + if err := instconf.Require(conf, + instconf.Need(serviceName, "connection-string"), + instconf.Need(serviceName, "origin"), + ); err != nil { // A hint rather than a longer sentence: scribe prints it on its own - // line, and what an operator meeting this needs is the key to add, not + // line, and what an operator meeting this needs is the keys to add, not // a restatement of the failure. - return settings{}, culpa.WithHint( - culpa.New("no connection-string is configured"), - "set [dolt.sr.ht]connection-string in config.ini") + return settings{}, culpa.WithHint(culpa.Wrap(err, "reading the config"), + "origin is what places this service in every other service's nav") } - host, err := hostFromOrigin(config.GetOrigin(conf, serviceName, true)) - if err != nil { + // The authority and not the bare host: the port is part of what identifies + // this endpoint, and https://x:8443 and https://x:9443 are two different + // sealed-URL hosts and two different JWT audiences. "" here means the origin + // is set but names no host — a scheme-less "dolt.example.org" is a path, not + // a URL — which is a configuration error and not a reason to guess. + host := instconf.OriginAuthority(instconf.ExternalOrigin(conf, serviceName)) + if host == "" { return settings{}, culpa.WithHint( - culpa.Wrapf(err, "[%s]origin", serviceName), - "origin must be protocol://host; it is what places this service in every other service's nav") + culpa.New(fmt.Sprintf("[%s]origin names no host", serviceName)), + "origin must be protocol://host, e.g. https://dolt.example.org") } return settings{ - connString: connString, + connString: config.GetString(conf, serviceName, "connection-string", ""), reposRoot: config.GetString(conf, serviceName, "repos", defaultReposRoot), staticDir: config.GetString(conf, serviceName, "static-dir", defaultStaticDir), remotesapiAddr: config.GetString(conf, serviceName, "remotesapi-listen", defaultRemotesapiAddr), @@ -122,25 +133,6 @@ func resolveSettings(conf ini.File) (settings, error) { }, nil } -// hostFromOrigin extracts the host[:port] authority from a service origin URL -// (e.g. "https://dolt.srht.bigb.es" -> "dolt.srht.bigb.es"). A missing or -// host-less origin is an error: the origin is required (it is what places the -// service in every other service's nav) and drives sealed-URL and JWT-audience -// correctness. -func hostFromOrigin(origin string) (string, error) { - if origin == "" { - return "", fmt.Errorf("empty origin; set origin=https:// in config.ini") - } - u, err := url.Parse(origin) - if err != nil { - return "", fmt.Errorf("parse origin %q: %w", origin, err) - } - if u.Host == "" { - return "", fmt.Errorf("origin %q has no host", origin) - } - return u.Host, nil -} - func main() { conf := config.LoadConfig() @@ -193,6 +185,20 @@ func main() { stores := &storeManager{cache: rsrv.Cache()} + // The git-description mirror is wired only on an instance that has a + // git.sr.ht to ask. web.Config documents a nil Git as "no mirroring", but + // nothing used to produce one: core-go's client.Do walks the API-origin + // ladder through config.GetAPI, which panics when it reaches the end, so an + // instance without git.sr.ht met that as a stack trace on the first push + // rather than as a description it simply did not copy. + var git web.GitDescriber + if _, ok := instconf.InternalAPIOrigin(conf, "git.sr.ht"); ok { + git = web.GitDescriptionResolver{} + } else { + slog.Warn("no git.sr.ht API origin is configured; companion databases will not mirror their git twin's description", + "component", "web", "keys", instconf.APIOriginKeys()) + } + srv.AnonRouter().Group(func(r chi.Router) { r.Use(chimw.RealIP, chimw.Recoverer) r.Use(config.Middleware(conf, serviceName), database.Middleware(db)) @@ -206,7 +212,7 @@ func main() { Repos: web.DBAdapter{}, Browse: web.BrowseAdapter{}, Users: web.MetaUserResolver{}, - Git: web.GitDescriptionResolver{}, + Git: git, RepoDiskPath: func(owner, name string) string { return storage.RepoDiskPath(cfg.reposRoot, owner, name) }, diff --git a/cmd/doltsrht/main_test.go b/cmd/doltsrht/main_test.go index 47c251cea4c5a9cceda805f016d44ada625f4fac..e5c7d0b3cbe529e2aa5013aeec2399f98dff23b6 100644 --- a/cmd/doltsrht/main_test.go +++ b/cmd/doltsrht/main_test.go @@ -4,7 +4,11 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/vaughan0/go-ini" + + "sourcecraft.dev/bigbes/sr-ht-ecore/instconf" ) // loadConf builds an ini.File directly from a literal, bypassing @@ -88,9 +92,10 @@ func TestResolveSettingsMissingConnString(t *testing.T) { origin=https://dolt.example.org `) - if _, err := resolveSettings(conf); err == nil { - t.Fatal("expected error for missing connection-string, got nil") - } + _, err := resolveSettings(conf) + require.Error(t, err) + assert.ErrorIs(t, err, instconf.ErrIncompleteConfig) + assert.Contains(t, err.Error(), "connection-string") } func TestResolveSettingsMissingOrigin(t *testing.T) { @@ -98,37 +103,48 @@ func TestResolveSettingsMissingOrigin(t *testing.T) { connection-string=postgres://u@localhost/d `) - if _, err := resolveSettings(conf); err == nil { - t.Fatal("expected error for missing origin, got nil") - } + _, err := resolveSettings(conf) + require.Error(t, err) + assert.ErrorIs(t, err, instconf.ErrIncompleteConfig) + assert.Contains(t, err.Error(), "origin") } -func TestHostFromOrigin(t *testing.T) { - cases := []struct { - origin string - want string - wantErr bool - }{ - {"https://dolt.srht.bigb.es", "dolt.srht.bigb.es", false}, - {"https://dolt.srht.bigb.es:443", "dolt.srht.bigb.es:443", false}, - {"http://127.0.0.1:5307", "127.0.0.1:5307", false}, - {"", "", true}, - {"not-a-url-with-no-host", "", true}, - } - for _, c := range cases { - got, err := hostFromOrigin(c.origin) - if c.wantErr { - if err == nil { - t.Errorf("hostFromOrigin(%q): expected error", c.origin) - } - continue - } - if err != nil { - t.Errorf("hostFromOrigin(%q): %v", c.origin, err) - continue - } - if got != c.want { - t.Errorf("hostFromOrigin(%q) = %q, want %q", c.origin, got, c.want) - } - } +// TestResolveSettingsReportsEveryMissingKey is the reason the two checks became +// one Require: an operator with an empty section gets both keys off one boot +// rather than one key per restart. +func TestResolveSettingsReportsEveryMissingKey(t *testing.T) { + _, err := resolveSettings(loadConf(t, "[dolt.sr.ht]\n")) + require.Error(t, err) + assert.Contains(t, err.Error(), "connection-string") + assert.Contains(t, err.Error(), "origin") +} + +// TestResolveSettingsHostlessOrigin covers the gap Require cannot: a present, +// non-blank origin that names no host. "dolt.example.org" without a scheme is a +// URL path, and a service whose sealed chunk URLs and JWT audience are built +// from an empty authority must refuse to start rather than guess a host. +func TestResolveSettingsHostlessOrigin(t *testing.T) { + conf := loadConf(t, `[dolt.sr.ht] +connection-string=postgres://u@localhost/d +origin=dolt.example.org +`) + + _, err := resolveSettings(conf) + require.Error(t, err) + assert.Contains(t, err.Error(), "names no host") +} + +// TestResolveSettingsTrimsTheOrigin: an origin written with a trailing slash is +// the same origin. It used to reach url.Parse untouched, which happened to +// answer the same host; the authority now comes off the canonical form, so the +// two spellings cannot diverge for any other consumer either. +func TestResolveSettingsTrimsTheOrigin(t *testing.T) { + conf := loadConf(t, `[dolt.sr.ht] +connection-string=postgres://u@localhost/d +origin=https://dolt.example.org:8443/ +`) + + got, err := resolveSettings(conf) + require.NoError(t, err) + assert.Equal(t, "dolt.example.org:8443", got.httpHost) }