diff --git a/internal/agentruntime/runtime.go b/internal/agentruntime/runtime.go index 3cbb7e7f..5dc998e9 100644 --- a/internal/agentruntime/runtime.go +++ b/internal/agentruntime/runtime.go @@ -83,6 +83,23 @@ func DashboardHostname(runtime Runtime, id string) string { return Hostname(runtime, id) } +// dnsLabelMaxLen is the RFC 1123 DNS label limit. +const dnsLabelMaxLen = 63 + +// MaxIDLength returns the longest id that keeps every DNS label this +// package derives from it (namespace/hostname "-", and for +// Hermes the "-ui" DashboardHostname suffix) within the 63-character DNS +// label limit. validate.Name alone allows ids up to 63 chars, which is too +// permissive here since Onboard prepends a runtime prefix before the id +// ever reaches a Kubernetes object. +func MaxIDLength(runtime Runtime) int { + reserved := len(string(runtime)) + 1 // "-" + if runtime == Hermes { + reserved += len("-ui") // DashboardHostname's "hermes--ui" label + } + return dnsLabelMaxLen - reserved +} + func Hostnames(runtime Runtime, id string) []string { if strings.TrimSpace(id) == "" { return nil diff --git a/internal/agentruntime/runtime_test.go b/internal/agentruntime/runtime_test.go index 2bb53778..61a8f140 100644 --- a/internal/agentruntime/runtime_test.go +++ b/internal/agentruntime/runtime_test.go @@ -3,11 +3,42 @@ package agentruntime import ( "os" "path/filepath" + "strings" "testing" "github.com/ObolNetwork/obol-stack/internal/config" ) +// TestMaxIDLengthKeepsDerivedLabelsWithinDNSLimit guards the DNS-label +// overflow finding: an id at MaxIDLength must keep every label this package +// derives (Namespace/Hostname, and for Hermes DashboardHostname's "-ui" +// suffix) at or under the 63-character RFC 1123 limit. +func TestMaxIDLengthKeepsDerivedLabelsWithinDNSLimit(t *testing.T) { + for _, rt := range []Runtime{OpenClaw, Hermes} { + id := strings.Repeat("a", MaxIDLength(rt)) + + if n := len(Namespace(rt, id)); n > 63 { + t.Errorf("%s: Namespace(%q) label is %d chars, want <=63", rt, id, n) + } + if n := len(strings.SplitN(Hostname(rt, id), ".", 2)[0]); n > 63 { + t.Errorf("%s: Hostname(%q) label is %d chars, want <=63", rt, id, n) + } + if n := len(strings.SplitN(DashboardHostname(rt, id), ".", 2)[0]); n > 63 { + t.Errorf("%s: DashboardHostname(%q) label is %d chars, want <=63", rt, id, n) + } + + // One character longer must overflow at least one derived label — + // otherwise MaxIDLength is too conservative, not just safe. + tooLong := id + "a" + overflow := len(Namespace(rt, tooLong)) > 63 || + len(strings.SplitN(Hostname(rt, tooLong), ".", 2)[0]) > 63 || + len(strings.SplitN(DashboardHostname(rt, tooLong), ".", 2)[0]) > 63 + if !overflow { + t.Errorf("%s: MaxIDLength+1 (%d chars) did not overflow any derived DNS label", rt, len(tooLong)) + } + } +} + func TestHermesPaths(t *testing.T) { cfg := &config.Config{ ConfigDir: "/tmp/obol-config", diff --git a/internal/hermes/hermes.go b/internal/hermes/hermes.go index b208c852..98eb52ad 100644 --- a/internal/hermes/hermes.go +++ b/internal/hermes/hermes.go @@ -113,6 +113,13 @@ func Onboard(cfg *config.Config, opts OnboardOptions, u *ui.UI) error { if err := validate.Name(id); err != nil { return fmt.Errorf("invalid agent id: %w", err) } + // validate.Name alone allows ids up to 63 chars, but Onboard derives the + // "hermes-" namespace/hostname (and DashboardHostname's + // "hermes--ui" label) below — bound id here so those stay ≤63 + // instead of failing later with an opaque Kubernetes error. + if max := agentruntime.MaxIDLength(agentruntime.Hermes); len(id) > max { + return fmt.Errorf("agent id %q is too long (%d chars): must be at most %d chars so hermes- fits the 63-character DNS label limit", id, len(id), max) + } deploymentDir := DeploymentPath(cfg, id) namespace := agentruntime.Namespace(agentruntime.Hermes, id) diff --git a/internal/hermes/hermes_test.go b/internal/hermes/hermes_test.go index 41a460b8..b97cf318 100644 --- a/internal/hermes/hermes_test.go +++ b/internal/hermes/hermes_test.go @@ -138,6 +138,26 @@ func TestOnboardRejectsUnsafeID(t *testing.T) { } } +// TestOnboardRejectsIDTooLongForDNSLabel guards against the "hermes--ui" +// DashboardHostname DNS label overflowing 63 characters: validate.Name alone +// allows a 63-char id, but Onboard derives "hermes-" (and, for the +// dashboard, "hermes--ui") from it, so an id at (or near) that limit +// must be rejected here with a clear error instead of failing later with an +// opaque Kubernetes error. +func TestOnboardRejectsIDTooLongForDNSLabel(t *testing.T) { + max := agentruntime.MaxIDLength(agentruntime.Hermes) + + tooLong := "a" + strings.Repeat("b", max) // max+1 chars, still a valid DNS label on its own + cfg := testConfig(t) + err := Onboard(cfg, OnboardOptions{ID: tooLong}, newTestUI()) + if err == nil { + t.Fatalf("Onboard(ID=%d chars) = nil, want error", len(tooLong)) + } + if !strings.Contains(err.Error(), "too long") { + t.Errorf("error should explain the id is too long, got: %v", err) + } +} + // TestGenerateConfig_PrimaryIsRoundTrippable guards the LiteLLM model_name // contract end-to-end: whatever string the agent's `model.default` is set to // MUST match a `model_name` entry in the LiteLLM ConfigMap byte-for-byte, diff --git a/internal/monetizeapi/agentidentity_test.go b/internal/monetizeapi/agentidentity_test.go index cfeba962..aad2042d 100644 --- a/internal/monetizeapi/agentidentity_test.go +++ b/internal/monetizeapi/agentidentity_test.go @@ -66,3 +66,22 @@ func TestRemoveAgentIdentityRegistration_UnknownChainAndEmptyChainAreNoops(t *te t.Errorf("base agentId = %q, want 42 unchanged by an empty-chain no-op", got) } } + +// TestRemoveAgentIdentityRegistration_DoesNotMutateSharedBackingArray guards +// against RemoveAgentIdentityRegistration filtering status.Registrations in +// place: a caller (e.g. an informer-cached object) may hold another +// reference to the same backing array, which an in-place compaction would +// silently corrupt. +func TestRemoveAgentIdentityRegistration_DoesNotMutateSharedBackingArray(t *testing.T) { + backing := []AgentIdentityRegistration{ + {Chain: "base", AgentID: "1"}, + {Chain: "base-sepolia", AgentID: "2"}, + } + shared := AgentIdentityStatus{Registrations: backing} + + _ = RemoveAgentIdentityRegistration(shared, "base") + + if backing[0].Chain != "base" || backing[0].AgentID != "1" { + t.Errorf("shared backing array was mutated: backing[0] = %+v, want unchanged {base 1}", backing[0]) + } +} diff --git a/internal/monetizeapi/types.go b/internal/monetizeapi/types.go index 90ced6a5..91f851e5 100644 --- a/internal/monetizeapi/types.go +++ b/internal/monetizeapi/types.go @@ -1162,7 +1162,10 @@ func RemoveAgentIdentityRegistration(status AgentIdentityStatus, chain string) A if chain == "" { return status } - out := status.Registrations[:0] + // Allocate rather than filter status.Registrations in place: this is an + // exported helper and a future caller could pass in an informer-cached + // object whose backing array must not be mutated out from under it. + out := make([]AgentIdentityRegistration, 0, len(status.Registrations)) for _, registration := range status.Registrations { if strings.EqualFold(strings.TrimSpace(registration.Chain), chain) { continue diff --git a/internal/openclaw/onboard_test.go b/internal/openclaw/onboard_test.go index d54b6679..52deeef0 100644 --- a/internal/openclaw/onboard_test.go +++ b/internal/openclaw/onboard_test.go @@ -1,8 +1,10 @@ package openclaw import ( + "strings" "testing" + "github.com/ObolNetwork/obol-stack/internal/agentruntime" "github.com/ObolNetwork/obol-stack/internal/ui" ) @@ -26,3 +28,22 @@ func TestOnboardRejectsUnsafeID(t *testing.T) { } } } + +// TestOnboardRejectsIDTooLongForDNSLabel guards against the "openclaw-" +// DNS label overflowing 63 characters: validate.Name alone allows a 63-char +// id, but Onboard prepends "openclaw-" to it, so an id at (or near) that +// limit must be rejected here with a clear error instead of failing later +// with an opaque Kubernetes error. +func TestOnboardRejectsIDTooLongForDNSLabel(t *testing.T) { + max := agentruntime.MaxIDLength(agentruntime.OpenClaw) + + tooLong := "a" + strings.Repeat("b", max) // max+1 chars, still a valid DNS label on its own + cfg := testConfig(t) + err := Onboard(cfg, OnboardOptions{ID: tooLong}, ui.New(false)) + if err == nil { + t.Fatalf("Onboard(ID=%d chars) = nil, want error", len(tooLong)) + } + if !strings.Contains(err.Error(), "too long") { + t.Errorf("error should explain the id is too long, got: %v", err) + } +} diff --git a/internal/openclaw/openclaw.go b/internal/openclaw/openclaw.go index 1f7bf166..ac65432d 100644 --- a/internal/openclaw/openclaw.go +++ b/internal/openclaw/openclaw.go @@ -175,6 +175,12 @@ func Onboard(cfg *config.Config, opts OnboardOptions, u *ui.UI) error { if err := validate.Name(id); err != nil { return fmt.Errorf("invalid agent id: %w", err) } + // validate.Name alone allows ids up to 63 chars, but Onboard derives the + // "openclaw-" DNS label below — bound id here so that stays ≤63 + // instead of failing later with an opaque Kubernetes error. + if max := agentruntime.MaxIDLength(agentruntime.OpenClaw); len(id) > max { + return fmt.Errorf("agent id %q is too long (%d chars): must be at most %d chars so %q- fits the 63-character DNS label limit", id, len(id), max, appName) + } deploymentDir := DeploymentPath(cfg, id) diff --git a/internal/stack/safety.go b/internal/stack/safety.go index 643c844d..e76d3823 100644 --- a/internal/stack/safety.go +++ b/internal/stack/safety.go @@ -273,9 +273,11 @@ func pidAlive(pid int) bool { return p.Signal(syscall.Signal(0)) == nil } -// errSafetyAborted lets cmd/obol/stack.go distinguish "user said no" from -// other errors. The CLI maps this to exit code 0 with a brief message -// instead of a noisy error trace. +// errSafetyAborted is a sentinel for "user declined a ConfirmRunningServicesLoss +// prompt". destroyOldBackendIfSwitching returns it so Init stops the whole +// command before switching backends (Down/Purge instead return nil directly +// from their own top-level check); Init maps it to a clean exit-0. Exported via +// ErrSafetyAborted() for callers that want to detect the abort with errors.Is. var errSafetyAborted = errors.New("aborted by operator at safety prompt") // ErrSafetyAborted is exported for callers that want to detect the abort diff --git a/internal/stack/stack.go b/internal/stack/stack.go index 0743ebac..9ef3fcc6 100644 --- a/internal/stack/stack.go +++ b/internal/stack/stack.go @@ -107,6 +107,11 @@ func Init(cfg *config.Config, u *ui.UI, force bool, backendName string, skipConf // live traffic. if hasExistingConfig && force { if err := destroyOldBackendIfSwitching(cfg, u, backendName, stackID, skipConfirm); err != nil { + // A declined safety prompt stops Init entirely (nothing switched, + // old cluster left intact) and, like Down/Purge, exits cleanly. + if errors.Is(err, errSafetyAborted) { + return nil + } return err } } @@ -156,6 +161,9 @@ func destroyOldBackendIfSwitching(cfg *config.Config, u *ui.UI, newBackend, stac return err } if !proceed { + // Signal the decline to Init, which stops the whole command before + // touching anything (the old cluster is still serving traffic — it + // must NOT be left undestroyed while the backend config switches). u.Info("Aborted.") return errSafetyAborted } diff --git a/internal/tunnel/tunnel.go b/internal/tunnel/tunnel.go index 73268851..d20f6553 100644 --- a/internal/tunnel/tunnel.go +++ b/internal/tunnel/tunnel.go @@ -1051,8 +1051,18 @@ func EnsureTunnelForSell(cfg *config.Config, u *ui.UI) (string, error) { // reflected immediately, instead of shadowing the offer's route until some // later, unrelated tunnel/sell invocation happens to run CreateStorefront // again (Canary402). +// +// It no-ops quietly when there is no persistent tunnel/hostname state yet +// (e.g. a first `obol sell ... --hostname X --no-register` before any +// tunnel has ever been created) — CreateStorefront has nothing to publish +// in that case, and EnsureTunnelForSell reconciles the storefront once the +// tunnel comes up. func RefreshStorefront(cfg *config.Config) error { - return CreateStorefront(cfg, storefrontHostnames(cfg, "")...) + hosts := storefrontHostnames(cfg, "") + if len(hosts) == 0 { + return nil + } + return CreateStorefront(cfg, hosts...) } // Stop scales the cloudflared deployment to 0 replicas. diff --git a/internal/tunnel/tunnel_test.go b/internal/tunnel/tunnel_test.go index 6d06c03a..2a116dbc 100644 --- a/internal/tunnel/tunnel_test.go +++ b/internal/tunnel/tunnel_test.go @@ -211,6 +211,30 @@ func TestCreateStorefront_TearsDownWhenAllHostsOfferBound(t *testing.T) { } } +// TestRefreshStorefront_NoOpWithoutTunnelState guards a first `obol sell +// ... --hostname X --no-register` before any persistent tunnel has ever been +// created: storefrontHostnames("") has nothing to report (no persistent +// tunnel state, no quick-tunnel URL to parse), so RefreshStorefront must +// no-op quietly instead of calling CreateStorefront with zero hostnames and +// surfacing its "requires at least one hostname" error as a confusing +// warning. EnsureTunnelForSell reconciles the storefront later once the +// tunnel exists. +func TestRefreshStorefront_NoOpWithoutTunnelState(t *testing.T) { + cfg := newHostnameTestConfig(t) + writeFakeKubeconfig(t, cfg) + + logPath := filepath.Join(cfg.ConfigDir, "kubectl.log") + writeFakeKubectl(t, cfg, logPath, "") + + if err := RefreshStorefront(cfg); err != nil { + t.Fatalf("RefreshStorefront should no-op without tunnel state, got error: %v", err) + } + + if _, err := os.ReadFile(logPath); err == nil { + t.Fatal("RefreshStorefront must not invoke kubectl when there is no hostname to publish") + } +} + func TestPatchAgentBaseURL_Insert(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "values-obol.yaml") diff --git a/obolup.sh b/obolup.sh index fe932955..6f64d8d2 100755 --- a/obolup.sh +++ b/obolup.sh @@ -519,10 +519,11 @@ download_with_retries() { } # Verify a downloaded release binary against the published SHA256SUMS file. -# Fails closed (returns 1) on checksum mismatch, a missing checksum entry, or -# no local sha256 tool. If SHA256SUMS itself can't be fetched, also fails -# closed unless OBOL_ALLOW_UNVERIFIED=1 is explicitly set, in which case the -# install proceeds unverified. +# Fails closed: returns 2 on a *verified* checksum mismatch (tamper — the caller +# must abort hard and NOT fall back to a source build), and returns 1 on a +# missing checksum entry or no local sha256 tool. If SHA256SUMS itself can't be +# fetched, also fails closed unless OBOL_ALLOW_UNVERIFIED=1 is explicitly set, +# in which case the install proceeds unverified (return 0). verify_release_checksum() { local release_tag="$1" local binary_name="$2" @@ -570,7 +571,12 @@ verify_release_checksum() { echo "Please re-run the installer. If the problem persists, report an issue at:" echo " https://github.com/ObolNetwork/obol-stack/issues" echo "" - return 1 + # Exit code 2 signals a *verified* mismatch (as opposed to the + # generic 1 used above for "couldn't verify at all") so callers can + # tell tamper apart from unavailability and refuse to silently fall + # back to another download over the same (possibly compromised) + # channel. + return 2 fi log_success "Checksum verified for $binary_name" @@ -617,10 +623,15 @@ download_release() { return 1 fi - # Verify against the published SHA256SUMS before installing - if ! verify_release_checksum "$release_tag" "$binary_name" "$tmp_obol"; then + # Verify against the published SHA256SUMS before installing. Propagate + # a verified-tamper (exit 2) distinctly from a generic verification + # failure (exit 1) so the caller knows falling back to another download + # method is unsafe. + local verify_status=0 + verify_release_checksum "$release_tag" "$binary_name" "$tmp_obol" || verify_status=$? + if [[ "$verify_status" -ne 0 ]]; then rm -f "$tmp_obol" - return 1 + return "$verify_status" fi chmod +x "$tmp_obol" @@ -715,10 +726,20 @@ install_obol_binary() { # If we got a tag, try to download it if [[ -n "$latest_tag" ]]; then - if download_release "$latest_tag"; then + local dl_status=0 + download_release "$latest_tag" || dl_status=$? + if [[ "$dl_status" -eq 0 ]]; then show_version_change "$current_version" return 0 fi + if [[ "$dl_status" -eq 2 ]]; then + # Verified tamper (checksum mismatch), not mere + # unavailability: building from source would just re-fetch + # over the same, possibly compromised, channel. Abort hard + # instead of silently "recovering" into an unverified install. + log_error "Checksum verification failed for $latest_tag — refusing to fall back to building from source over the same network channel." + return 1 + fi log_warn "Download failed, falling back to building from source..." else log_info "No releases found, building from source..." @@ -730,12 +751,18 @@ install_obol_binary() { else # Specific release requested log_info "Attempting to download release: $release" - if download_release "$release"; then + local dl_status=0 + download_release "$release" || dl_status=$? + if [[ "$dl_status" -eq 0 ]]; then show_version_change "$current_version" return 0 fi + if [[ "$dl_status" -eq 2 ]]; then + log_error "Checksum verification failed for $release — refusing to fall back to building from source over the same network channel." + return 1 + fi - log_warn "Release $release not found, building from source..." + log_warn "Release $release not found or download failed, building from source..." build_from_source "$release" show_version_change "$current_version" fi