diff --git a/.gitignore b/.gitignore index 799011f..e9121bf 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,7 @@ ctrld-* cmd/cli/rsrc_*.syso ctrld ctrld.exe + +# Local planning artifacts - never commit +/SPEC.md +/tasks/ diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index f137737..20f2b5e 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -24,6 +24,7 @@ import ( "strings" "sync/atomic" "time" + "unicode" "github.com/Masterminds/semver/v3" "github.com/cuonglm/osinfo" @@ -220,8 +221,12 @@ func isStableVersion(vs string) bool { func RunCobraCommand(cmd *cobra.Command) { noConfigStart = isNoConfigStart(cmd) firewallModeFlagChanged = cmd.Flags().Changed("firewall-mode") - checkStrFlagEmpty(cmd, cdUidFlagName) - checkStrFlagEmpty(cmd, cdOrgFlagName) + if !checkStrFlagEmpty(cmd, cdUidFlagName) { + return + } + if !checkStrFlagEmpty(cmd, cdOrgFlagName) { + return + } run(nil, make(chan struct{})) } @@ -260,7 +265,8 @@ func CheckDeactivationPin(pin int64, stopCh chan struct{}) int { // run runs ctrld cli with given app callback and stop channel. func run(appCallback *AppCallback, stopCh chan struct{}) { if stopCh == nil { - mainLog.Load().Fatal().Msg("stopCh is nil") + failRunUnclassified(mainLog.Load().Error(), "run() called with a nil stop channel", nil) + return } waitCh := make(chan struct{}) p := &prog{ @@ -298,14 +304,10 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { consoleWriter = newHumanReadableZapCore(io.MultiWriter(os.Stdout, hlc), consoleWriterLevel) p.logConn = hlc } - notifyExitToLogServer := func() { - if p.logConn != nil { - _ = p.logConn.Close() - } - } if daemon && runtime.GOOS == "windows" { - p.Fatal().Msg("Cannot run in daemon mode. Please install a Windows service.") + failRunUnclassified(p.Error(), "cannot run in daemon mode; please install a Windows service", p.notifyExitToLogServer) + return } if !daemon { @@ -316,7 +318,8 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { svcConfig := svcCmd.createServiceConfig() s, err := svcCmd.newService(p, svcConfig) if err != nil { - p.Fatal().Err(err).Msg("Failed to create new service") + failRunUnclassified(p.Error().Err(err), fmt.Sprintf("failed to create new service: %v", err), p.notifyExitToLogServer) + return } if err := s.Run(); err != nil { p.Error().Err(err).Msg("Failed to start service") @@ -327,18 +330,20 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { tryReadingConfig(writeDefaultConfig) if err := readBase64Config(configBase64); err != nil { - p.Fatal().Err(err).Msg("Failed to read base64 config") + failRunUnclassified(p.Error().Err(err), fmt.Sprintf("failed to read base64 config: %v", err), p.notifyExitToLogServer) + return } processNoConfigFlags(noConfigStart) // After s.Run() was called, if ctrld is going to be terminated for any reason, // write msgExit to p.logConn so others (like "ctrld start") won't have to wait for timeout. p.mu.Lock() - if err := v.Unmarshal(&cfg); err != nil { - notifyExitToLogServer() - p.Fatal().Msgf("Failed to unmarshal config: %v", err) - } + unmarshalErr := v.Unmarshal(&cfg) p.mu.Unlock() + if unmarshalErr != nil { + failRunUnclassified(p.Error(), fmt.Sprintf("failed to unmarshal config: %v", unmarshalErr), p.notifyExitToLogServer) + return + } processLogAndCacheFlags(v, &cfg) @@ -359,8 +364,8 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { // Wait for network up. if !ctrldnet.Up() { - notifyExitToLogServer() - p.Fatal().Msg("Network is not up yet") + failRunUnclassified(p.Error(), "network is not up yet", p.notifyExitToLogServer) + return } cs, err := newControlServer(filepath.Join(sockDir, ControlSocketName())) @@ -374,7 +379,9 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { cdUID = uid } if cdUID != "" { - validateCdUpstreamProtocol() + if !validateCdUpstreamProtocol(p.notifyExitToLogServer) { + return + } // Bound API preflight by the service lifetime. Without this, a stop request // arriving while the API is unreachable leaves this retry/backoff loop running // after "Service stopped" was logged, so the process keeps working on behalf of @@ -391,7 +398,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { // not see a failed start and apply its restart policy to a service the // operator just asked to stop. p.Notice().Msg("Stop requested while fetching resolver config — shutting down") - notifyExitToLogServer() + p.notifyExitToLogServer() return case pf.err != nil: if isMobile() { @@ -399,7 +406,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { return } - handleAPIPreflightFailure(p, pf.err, notifyExitToLogServer) + handleAPIPreflightFailure(p, pf.err, p.notifyExitToLogServer) return default: p.mu.Lock() @@ -408,7 +415,7 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { } } - updated := updateListenerConfig(&cfg, notifyExitToLogServer) + updated := updateListenerConfig(&cfg, p.notifyExitToLogServer) // Bootstrap and listener binding both succeeded, so an earlier run's // recorded failure no longer describes this install. @@ -430,9 +437,8 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { // The flag defaults to "off" for help output, so use Changed() to avoid // clobbering a config-file value of firewall_mode = "on" on normal starts. if firewallModeFlagChanged { - if !validFirewallMode(firewallMode) { - notifyExitToLogServer() - p.Fatal().Msgf("invalid --firewall-mode value %q: must be 'off' or 'on'", firewallMode) + if !validateFirewallModeFlag(true, firewallMode, p.notifyExitToLogServer) { + return } if cfg.Service.FirewallMode != firewallMode { cfg.Service.FirewallMode = firewallMode @@ -448,11 +454,10 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { if updated { if err := writeConfigFile(&cfg); err != nil { - notifyExitToLogServer() - p.Fatal().Err(err).Msg("Failed to write config file") - } else { - p.Info().Msg("Writing config file to: " + defaultConfigFile) + failRunUnclassified(p.Error().Err(err), fmt.Sprintf("failed to write config file: %v", err), p.notifyExitToLogServer) + return } + p.Info().Msg("Writing config file to: " + defaultConfigFile) } if newLogPath := cfg.Service.LogPath; newLogPath != "" && oldLogPath != newLogPath { @@ -471,31 +476,28 @@ func run(appCallback *AppCallback, stopCh chan struct{}) { } if err := validateConfig(&cfg); err != nil { - notifyExitToLogServer() - os.Exit(1) + failRunUnclassified(p.Error().Err(err), fmt.Sprintf("invalid config: %v", err), p.notifyExitToLogServer) + return } initCache() if daemon { exe, err := os.Executable() if err != nil { - p.Error().Err(err).Msg("Failed to find the binary") - notifyExitToLogServer() - os.Exit(1) + failRunUnclassified(p.Error().Err(err), "failed to find the binary", p.notifyExitToLogServer) + return } curDir, err := os.Getwd() if err != nil { - p.Error().Err(err).Msg("Failed to get current working directory") - notifyExitToLogServer() - os.Exit(1) + failRunUnclassified(p.Error().Err(err), "failed to get current working directory", p.notifyExitToLogServer) + return } // If running as daemon, re-run the command in background, with daemon off. cmd := exec.Command(exe, append(os.Args[1:], "-d=false")...) cmd.Dir = curDir if err := cmd.Start(); err != nil { - p.Error().Err(err).Msg("Failed to start process as daemon") - notifyExitToLogServer() - os.Exit(1) + failRunUnclassified(p.Error().Err(err), "failed to start process as daemon", p.notifyExitToLogServer) + return } p.Info().Int("pid", cmd.Process.Pid).Msg("DNS proxy started") os.Exit(0) @@ -814,9 +816,10 @@ func permanentAPIRejection(err error) (*controld.ErrorResponse, bool) { } // apiFailureCode maps a bootstrap preflight error to its provisioning code. -// A deleted device gets its own code because it triggers self-uninstall; -// other permanent rejections are generic; anything else counts as -// reachability trouble worth retrying. +// A deleted device gets its own code because it triggers self-uninstall; a +// permanent rejection with a known token reason gets its own code; other +// permanent rejections are generic; anything else counts as reachability +// trouble worth retrying. func apiFailureCode(err error) (provisionFailureCode, bool) { if err == nil { return "", false @@ -825,12 +828,62 @@ func apiFailureCode(err error) (provisionFailureCode, bool) { if errors.As(err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { return provisionCodeAPIDeviceInvalid, true } - if _, ok := permanentAPIRejection(err); ok { + if rejection, ok := permanentAPIRejection(err); ok { + if code, ok := tokenFailureCodeForReason(rejection.ErrorField.Metadata.Reason); ok { + return code, true + } return provisionCodeAPIRejected, true } return provisionCodeAPIUnreachable, true } +// tokenFailureCodeForReason maps error.metadata.reason on a provisioning-token +// rejection to its failure code. An empty or unrecognized reason is not +// classified here; the caller falls back to the generic rejection code. +func tokenFailureCodeForReason(reason string) (provisionFailureCode, bool) { + switch reason { + case controld.ReasonTokenInvalid: + return provisionCodeTokenInvalid, true + case controld.ReasonTokenExpired: + return provisionCodeTokenExpired, true + case controld.ReasonTokenLimitReached: + return provisionCodeTokenLimitReached, true + case controld.ReasonTokenDisabled: + return provisionCodeTokenDisabled, true + default: + return "", false + } +} + +// tokenRejectionMessage returns the human message for a TOKEN_* code: what is +// wrong with the provisioning code and one concrete next action. Empty for +// any other code, so callers know to fall back to a generic message. Never +// includes the token itself. +func tokenRejectionMessage(code provisionFailureCode) string { + switch code { + case provisionCodeTokenInvalid: + return "the provisioning code is not valid; check the code and re-enter it exactly as given" + case provisionCodeTokenExpired: + return "the provisioning code has expired; get a new provisioning code from your administrator" + case provisionCodeTokenLimitReached: + return "the provisioning code reached its device limit; free up a device slot or use a different code" + case provisionCodeTokenDisabled: + return "the provisioning code was invalidated; download a profile from an active provisioning code" + default: + return "" + } +} + +// provisionTokenFailureMessage returns the result-file message for a failed +// provision-token exchange: a token-specific message when the API gave a +// known reason, the generic exchange-failure summary otherwise. +func provisionTokenFailureMessage(code provisionFailureCode, err error) string { + if msg := tokenRejectionMessage(code); msg != "" { + return msg + } + return fmt.Sprintf("provision token exchange failed: %v", err) +} + // apiRejectionSummary reports the HTTP status only. The API's raw error body // can echo back the value the caller sent, so it stays out of the artifact. func apiRejectionSummary(statusCode int) string { @@ -1328,7 +1381,21 @@ func userHomeDir() (string, error) { return ctrld.UserHomeDir() } -// socketDir returns directory that ctrld will create socket file for running controlServer. +// absHomeDir returns the absolute path of filename in the ctrld home +// directory. A custom homedir wins, so a file lands next to the log and +// the config that obey the same override. +func absHomeDir(filename string) string { + if homedir != "" { + return filepath.Join(homedir, filename) + } + dir, err := userHomeDir() + if err != nil { + return filename + } + return filepath.Join(dir, filename) +} + +// socketDir returns directory to use for the controlServer socket. func socketDir() (string, error) { switch { case runtime.GOOS == "windows", isMobile(): @@ -1939,6 +2006,99 @@ func osVersion() string { return oi.String() } +// provisionTokenMinLen, provisionTokenMaxLen, and provisionTokenExpectedPrefix +// describe the shape of a --cd-org value the input stage checks before any +// network call. +const ( + provisionTokenMinLen = 6 + provisionTokenMaxLen = 64 + provisionTokenExpectedPrefix = "org-v1-" +) + +// provisionTokenShapeValid reports whether token looks like something the API +// could parse: a length between provisionTokenMinLen and provisionTokenMaxLen, +// with no whitespace or control characters. It does not require the +// "org-v1-" prefix - legacy provisioning codes predate that convention and +// are handled with a warning, not a failure. +func provisionTokenShapeValid(token string) bool { + if len(token) < provisionTokenMinLen || len(token) > provisionTokenMaxLen { + return false + } + for _, r := range token { + if unicode.IsSpace(r) || unicode.IsControl(r) { + return false + } + } + return true +} + +// validateProvisionTokenShape classifies an obviously malformed --cd-org +// value (PROVISION_TOKEN_MALFORMED) before any network call, so a typo or a +// pasted URL fails fast instead of going through a doomed API round trip. A +// token missing the "org-v1-" prefix only gets a logged warning: legacy +// provisioning codes may lack it and still work. +func validateProvisionTokenShape() bool { + if !provisionTokenShapeValid(cdOrg) { + msg := fmt.Sprintf("--cd-org provisioning code is malformed: must be %d-%d characters with no whitespace or control characters", provisionTokenMinLen, provisionTokenMaxLen) + failProvision(newProvisionResult(provisionCodeProvisionTokenMalformed, msg, nil, provisionSecrets()...), nil) + return false + } + if !strings.HasPrefix(cdOrg, provisionTokenExpectedPrefix) { + mainLog.Load().Warn().Msg("--cd-org provisioning code does not have the expected org-v1- prefix; continuing, since legacy codes may lack it") + } + return true +} + +// validateCustomHostnameFlag classifies an invalid --custom-hostname value +// (CUSTOM_HOSTNAME_INVALID) before any network call. ctrld's accept/reject +// rule (validHostname) is unchanged - this only names the field, the +// offending character(s), and the allowed format instead of a bare fatal +// exit. ControlD's device-name formatting folds or strips some characters +// ctrld still accepts (dot, space, plus), so a hostname using one of those +// gets a one-line notice instead: the registered device name may differ from +// what was requested. +func validateCustomHostnameFlag() bool { + if customHostname == "" { + return true + } + if !validHostname(customHostname) { + failProvision(newProvisionResult(provisionCodeCustomHostnameInvalid, customHostnameFailureMessage(customHostname), nil, provisionSecrets()...), nil) + return false + } + if hostnameMayBeFoldedByServer(customHostname) { + mainLog.Load().Notice().Msgf("device name %q contains characters ControlD may fold or strip when it registers the device; the registered name may differ", customHostname) + } + return true +} + +// validateInterceptModeFlag classifies an invalid --intercept-mode value +// (INTERCEPT_MODE_INVALID) on the provisioning boundary, so a typo fails with +// a stable code instead of a bare fatal exit an installer wrapper cannot tell +// apart from any other crash. An empty mode means the flag was not set. +func validateInterceptModeFlag(mode string) bool { + if mode == "" || validInterceptMode(mode) { + return true + } + msg := fmt.Sprintf("--intercept-mode %q is not valid: must be 'off', 'dns', or 'hard'", mode) + failProvision(newProvisionResult(provisionCodeInterceptModeInvalid, msg, nil, provisionSecrets()...), nil) + return false +} + +// validateFirewallModeFlag classifies an invalid --firewall-mode value on the +// provisioning boundary. Unlike --intercept-mode, no dedicated code exists +// for this flag, so a bad value falls back to UNCLASSIFIED rather than a bare +// fatal exit. changed must be the flag's Cobra Changed() state: the default +// value ("off") is not itself invalid, so an unset flag always proceeds. +// notify unblocks a waiting "ctrld start"; pass nil when there is none. +func validateFirewallModeFlag(changed bool, mode string, notify func()) bool { + if !changed || validFirewallMode(mode) { + return true + } + msg := fmt.Sprintf("--firewall-mode %q is not valid: must be 'off' or 'on'", mode) + failProvisionUnclassified(msg, notify) + return false +} + // cdUIDFromProvToken fetch UID from ControlD API using provision token. func cdUIDFromProvToken() string { // --cd flag supersedes --cd-org, ignore it if both are supplied. @@ -1949,9 +2109,11 @@ func cdUIDFromProvToken() string { if cdOrg == "" { return "" } - // Validate custom hostname if provided. - if customHostname != "" && !validHostname(customHostname) { - mainLog.Load().Fatal().Msgf("Invalid custom hostname: %q", customHostname) + if !validateProvisionTokenShape() { + return "" + } + if !validateCustomHostnameFlag() { + return "" } loggerCtx := ctrld.LoggerCtx(context.Background(), mainLog.Load()) req := &controld.UtilityOrgRequest{ @@ -1967,7 +2129,7 @@ func cdUIDFromProvToken() string { code, _ := apiFailureCode(err) mainLog.Load().Error().Msgf("Failed to fetch resolver uid with provision token: %s: %s", redactToken(cdOrg), redactSecrets(err.Error(), provisionSecrets()...)) - failProvision(newProvisionResult(code, fmt.Sprintf("provision token exchange failed: %v", err), nil, provisionSecrets()...), nil) + failProvision(newProvisionResult(code, provisionTokenFailureMessage(code, err), nil, provisionSecrets()...), nil) return "" } return resolverConfig.UID @@ -2073,34 +2235,55 @@ func newSocketControlClientMobile(dir string, stopCh chan struct{}) *controlClie } // checkStrFlagEmpty validates if a string flag was set to an empty string. -// If yes, emitting a fatal error message. -func checkStrFlagEmpty(cmd *cobra.Command, flagName string) { +// An explicit empty --cd-org is a malformed provisioning token, classified on +// the provisioning boundary; every other flag keeps the bare fatal, since +// this helper is shared with flags outside that boundary. Returns false when +// the caller must stop: provisionExit is stubbed out under test, so nothing +// else stops execution from falling through. +func checkStrFlagEmpty(cmd *cobra.Command, flagName string) bool { fl := cmd.Flags().Lookup(flagName) - if !fl.Changed || fl.Value.Type() != "string" { - return + if !fl.Changed || fl.Value.Type() != "string" || fl.Value.String() != "" { + return true } - if fl.Value.String() == "" { - mainLog.Load().Fatal().Msgf(`flag "--%s" value must be non-empty`, fl.Name) + if flagName == cdOrgFlagName { + msg := fmt.Sprintf("--%s provisioning code is malformed: value must be non-empty", cdOrgFlagName) + failProvision(newProvisionResult(provisionCodeProvisionTokenMalformed, msg, nil, provisionSecrets()...), nil) + return false } + mainLog.Load().Fatal().Msgf(`flag "--%s" value must be non-empty`, fl.Name) + return false } -// validateCdUpstreamProtocol validates the Control D upstream protocol -func validateCdUpstreamProtocol() { +// validateCdUpstreamProtocol validates the Control D upstream protocol, +// classified on the provisioning boundary since it only runs once --cd is +// set. notify unblocks a waiting "ctrld start" on the daemon side; the parent +// process, which has no one waiting, passes nil. Returns false when the +// caller must stop. +func validateCdUpstreamProtocol(notify func()) bool { if cdUID == "" { - return + return true } switch cdUpstreamProto { case ctrld.ResolverTypeDOH, ctrld.ResolverTypeDOH3: + return true default: - mainLog.Load().Fatal().Msg(`Flag "--protocol" must be "doh" or "doh3"`) + msg := fmt.Sprintf("--proto %q is not valid: must be 'doh' or 'doh3'", cdUpstreamProto) + failProvision(newProvisionResult(provisionCodeInvalidFlagCombination, msg, nil, provisionSecrets()...), notify) + return false } } -// validateCdAndNextDNSFlags validates that Control D and NextDNS flags are not used together -func validateCdAndNextDNSFlags() { +// validateCdAndNextDNSFlags validates that Control D and NextDNS flags are +// not used together, classified on the provisioning boundary since it only +// fires once --cd or --cd-org is set. Returns false when the caller must +// stop. +func validateCdAndNextDNSFlags() bool { if (cdUID != "" || cdOrg != "") && nextdns != "" { - mainLog.Load().Fatal().Msgf("--%s/--%s could not be used with --%s", cdUidFlagName, cdOrgFlagName, nextdnsFlagName) + msg := fmt.Sprintf("--%s/--%s cannot be used together with --%s", cdUidFlagName, cdOrgFlagName, nextdnsFlagName) + failProvision(newProvisionResult(provisionCodeInvalidFlagCombination, msg, nil, provisionSecrets()...), nil) + return false } + return true } // removeNextDNSFromArgs removes the --nextdns from command line arguments. @@ -2306,7 +2489,25 @@ func runningIface(s service.Service) *ifaceResponse { return nil } +// apiRejectionMessage returns the result-file message for a permanent API +// rejection: the HTTP status for a device-invalid or generic rejection, or +// the raw error for anything else (network trouble, an unreachable API). +func apiRejectionMessage(err error) string { + var uer *controld.ErrorResponse + if errors.As(err, &uer) && uer.ErrorField.Code == controld.InvalidConfigCode { + return apiRejectionSummary(uer.StatusCode) + } + if rejection, ok := permanentAPIRejection(err); ok { + return apiRejectionSummary(rejection.StatusCode) + } + return fmt.Sprintf("failed to fetch resolver config: %v", err) +} + // doValidateCdRemoteConfig fetches and validates custom config for cdUID. +// fatal distinguishes the two callers: the direct "--cd " install path +// passes true and classifies a fetch failure on the provisioning boundary; +// the restart path passes false and gets the error back to decide for +// itself, with no process exit. func doValidateCdRemoteConfig(cdUID string, fatal bool) error { loggerCtx := ctrld.LoggerCtx(context.Background(), mainLog.Load()) req := &controld.ResolverConfigRequest{ @@ -2314,16 +2515,16 @@ func doValidateCdRemoteConfig(cdUID string, fatal bool) error { Version: appVersion, Metadata: ctrld.SystemMetadata(loggerCtx), } - rc, err := controld.FetchResolverConfig(loggerCtx, req, cdDev) + rc, err := fetchResolverConfig(loggerCtx, req, cdDev) if err != nil { - logger := mainLog.Load().Fatal() - if !fatal { - logger = mainLog.Load().Warn() - } - logger.Err(err).Err(err).Msgf("Failed to fetch resolver uid: %s", cdUID) if !fatal { + mainLog.Load().Warn().Err(err).Msgf("Failed to fetch resolver config for %s", redactToken(cdUID)) return err } + code, _ := apiFailureCode(err) + mainLog.Load().Error().Err(err).Msgf("Failed to fetch resolver config for %s", redactToken(cdUID)) + failProvision(newProvisionResult(code, apiRejectionMessage(err), nil, provisionSecrets()...), nil) + return err } // return earlier if there's no custom config. diff --git a/cmd/cli/cli_provision_test.go b/cmd/cli/cli_provision_test.go index 44f90a1..e924d9d 100644 --- a/cmd/cli/cli_provision_test.go +++ b/cmd/cli/cli_provision_test.go @@ -2,6 +2,8 @@ package cli import ( "context" + "encoding/json" + "errors" "fmt" "net" "net/http" @@ -71,6 +73,140 @@ func TestApiFailureCode(t *testing.T) { } } +// TestApiFailureCodeMapsRejectionReason covers the token_* reasons the API sends in +// error.metadata.reason on a provisioning-token rejection. An absent or unknown +// reason must fall back to the generic API_REJECTED exactly as before this reason +// field existed. +func TestApiFailureCodeMapsRejectionReason(t *testing.T) { + rejectionWithReason := func(reason string) error { + e := &controld.ErrorResponse{StatusCode: http.StatusBadRequest} + e.ErrorField.Code = 40003 + e.ErrorField.Message = "invalid token" + e.ErrorField.Metadata.Reason = reason + return e + } + + tests := []struct { + name string + reason string + wantCode provisionFailureCode + }{ + {name: "token_invalid", reason: "token_invalid", wantCode: provisionCodeTokenInvalid}, + {name: "token_expired", reason: "token_expired", wantCode: provisionCodeTokenExpired}, + {name: "token_limit_reached", reason: "token_limit_reached", wantCode: provisionCodeTokenLimitReached}, + {name: "token_disabled", reason: "token_disabled", wantCode: provisionCodeTokenDisabled}, + {name: "reason absent falls back", reason: "", wantCode: provisionCodeAPIRejected}, + {name: "unknown reason falls back", reason: "some_future_reason", wantCode: provisionCodeAPIRejected}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + code, ok := apiFailureCode(rejectionWithReason(tc.reason)) + if !ok { + t.Fatal("apiFailureCode() ok = false, want true") + } + if code != tc.wantCode { + t.Errorf("apiFailureCode() code = %s, want %s", code, tc.wantCode) + } + }) + } +} + +// TestApiFailureCodeSurvivesMalformedReasonType covers a rejection body whose +// metadata.reason is the wrong JSON type end to end: decode it exactly as +// internal/controld does (json.Unmarshal into the same exported type), then +// classify it. Before the metadata decode fix, this body failed the whole +// decode and apiFailureCode never saw an *ErrorResponse at all, so it fell +// back to API_UNREACHABLE - the retryable bootstrap code - instead of the +// permanent rejection this HTTP 400 with a known error code actually is. +func TestApiFailureCodeSurvivesMalformedReasonType(t *testing.T) { + tests := []struct { + name string + body string + }{ + {name: "reason as a number", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":12345}}}`}, + {name: "reason as an object", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":{"inner":"value"}}}}`}, + {name: "reason as null", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":null}}}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + e := &controld.ErrorResponse{StatusCode: http.StatusBadRequest} + if err := json.Unmarshal([]byte(tc.body), e); err != nil { + t.Fatalf("a malformed reason must not fail the whole decode: %v", err) + } + code, ok := apiFailureCode(e) + if !ok { + t.Fatal("apiFailureCode() ok = false, want true") + } + if code != provisionCodeAPIRejected { + t.Errorf("apiFailureCode() code = %s, want %s (not %s)", code, provisionCodeAPIRejected, provisionCodeAPIUnreachable) + } + }) + } +} + +// TestCdUIDFromProvTokenReasonCodes covers the full path from an API rejection +// reason to a persisted result file: each known reason gets its own code, exit +// code, and stage, with a message naming the field and a next action but never +// echoing the token. Absent and unknown reasons keep the generic rejection. +func TestCdUIDFromProvTokenReasonCodes(t *testing.T) { + const secretToken = "org-secret-token-999" + + tests := []struct { + name string + reason string + wantCode provisionFailureCode + wantContains string + }{ + {name: "token_invalid", reason: "token_invalid", wantCode: provisionCodeTokenInvalid, wantContains: "provisioning code"}, + {name: "token_expired", reason: "token_expired", wantCode: provisionCodeTokenExpired, wantContains: "expired"}, + {name: "token_limit_reached", reason: "token_limit_reached", wantCode: provisionCodeTokenLimitReached, wantContains: "limit"}, + {name: "token_disabled", reason: "token_disabled", wantCode: provisionCodeTokenDisabled, wantContains: "invalidated"}, + {name: "reason absent", reason: "", wantCode: provisionCodeAPIRejected, wantContains: ""}, + {name: "unknown reason", reason: "brand_new_reason", wantCode: provisionCodeAPIRejected, wantContains: ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldFetch, oldHostname := fetchResolverUIDFn, customHostname + t.Cleanup(func() { fetchResolverUIDFn, customHostname = oldFetch, oldHostname }) + cdUID = "" + cdOrg = secretToken + customHostname = "" + + rejected := &controld.ErrorResponse{StatusCode: http.StatusBadRequest} + rejected.ErrorField.Code = 40003 + rejected.ErrorField.Message = "invalid token " + secretToken + rejected.ErrorField.Metadata.Reason = tc.reason + fetchResolverUIDFn = func(context.Context, *controld.UtilityOrgRequest, string, bool) (*controld.ResolverConfig, error) { + return nil, rejected + } + + if got := cdUIDFromProvToken(); got != "" { + t.Errorf("cdUIDFromProvToken() = %q, want empty on failure", got) + } + if *exitCode != provisionExitCodeForCode[tc.wantCode] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[tc.wantCode]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(tc.wantCode) { + t.Errorf("code = %q, want %q", r.Code, tc.wantCode) + } + if r.Stage != string(provisionStageBootstrap) { + t.Errorf("stage = %q, want bootstrap", r.Stage) + } + if tc.wantContains != "" && !strings.Contains(r.Message, tc.wantContains) { + t.Errorf("message = %q, want it to contain %q", r.Message, tc.wantContains) + } + if strings.Contains(r.Message, secretToken) { + t.Errorf("token leaked into result message: %q", r.Message) + } + }) + } +} + func stubProvisionGlobals(t *testing.T) (exitCode *int, notified *bool) { t.Helper() oldCdUID, oldCdOrg := cdUID, cdOrg @@ -177,6 +313,108 @@ func TestHandleAPIPreflightFailure(t *testing.T) { }) } +// TestDoValidateCdRemoteConfigClassifiesAPIFailure covers the direct +// "--cd " install path (fatal=true): a fetch failure must classify on +// the provisioning boundary with the same per-class codes as the daemon-side +// preflight, instead of a bare fatal. +func TestDoValidateCdRemoteConfigClassifiesAPIFailure(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldFetch := fetchResolverConfig + t.Cleanup(func() { fetchResolverConfig = oldFetch }) + + deviceInvalid := func() error { + e := &controld.ErrorResponse{StatusCode: http.StatusNotFound} + e.ErrorField.Code = controld.InvalidConfigCode + e.ErrorField.Message = "device does not exist" + return e + } + rejected := func() error { + e := &controld.ErrorResponse{StatusCode: http.StatusUnauthorized} + e.ErrorField.Message = "bad token" + return e + } + + tests := []struct { + name string + err error + wantCode provisionFailureCode + }{ + {name: "device invalid", err: deviceInvalid(), wantCode: provisionCodeAPIDeviceInvalid}, + {name: "permanent rejection", err: rejected(), wantCode: provisionCodeAPIRejected}, + {name: "unreachable", err: retryableNetworkErr(), wantCode: provisionCodeAPIUnreachable}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, tc.err + } + if err := doValidateCdRemoteConfig("device-uid-123", true); err == nil { + t.Error("doValidateCdRemoteConfig() error = nil, want the fetch error back") + } + if *exitCode != provisionExitCodeForCode[tc.wantCode] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[tc.wantCode]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(tc.wantCode) { + t.Errorf("code = %q, want %q", r.Code, tc.wantCode) + } + }) + } +} + +// TestDoValidateCdRemoteConfigNonFatalReturnsError proves the restart path +// (fatal=false) is unaffected: it still just warns and hands the error back, +// with no process exit and no result file. +func TestDoValidateCdRemoteConfigNonFatalReturnsError(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldFetch := fetchResolverConfig + t.Cleanup(func() { fetchResolverConfig = oldFetch }) + + wantErr := errors.New("network unreachable") + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, wantErr + } + + if err := doValidateCdRemoteConfig("device-uid-123", false); !errors.Is(err, wantErr) { + t.Errorf("doValidateCdRemoteConfig() error = %v, want %v", err, wantErr) + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + if _, err := readProvisionResult(); err == nil { + t.Error("expected no provision result written for the non-fatal path") + } +} + +// TestDoValidateCdRemoteConfigDoesNotSelfUninstall proves the direct-cd +// install path never triggers self-uninstall on a device-invalid failure: +// this runs before the service is installed, so there is nothing to remove. +func TestDoValidateCdRemoteConfigDoesNotSelfUninstall(t *testing.T) { + _, _ = stubProvisionGlobals(t) + oldFetch, oldUninstall := fetchResolverConfig, uninstallInvalidCdUIDFn + t.Cleanup(func() { fetchResolverConfig, uninstallInvalidCdUIDFn = oldFetch, oldUninstall }) + + uninstallCalled := false + uninstallInvalidCdUIDFn = func(*prog, *ctrld.Logger, bool) bool { + uninstallCalled = true + return true + } + e := &controld.ErrorResponse{StatusCode: http.StatusNotFound} + e.ErrorField.Code = controld.InvalidConfigCode + fetchResolverConfig = func(context.Context, *controld.ResolverConfigRequest, bool) (*controld.ResolverConfig, error) { + return nil, e + } + + _ = doValidateCdRemoteConfig("device-uid-123", true) + + if uninstallCalled { + t.Error("doValidateCdRemoteConfig triggered self-uninstall; nothing is installed yet on this path") + } +} + func TestCdUIDFromProvTokenFailureEmitsCode(t *testing.T) { exitCode, _ := stubProvisionGlobals(t) oldFetch, oldHostname := fetchResolverUIDFn, customHostname diff --git a/cmd/cli/commands_service_start.go b/cmd/cli/commands_service_start.go index 18da4fc..bb09c5e 100644 --- a/cmd/cli/commands_service_start.go +++ b/cmd/cli/commands_service_start.go @@ -63,10 +63,21 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { logger := mainLog.Load() logger.Debug().Msg("Service start command started") + // Clear before any check runs, not just before doTasksE: a result from a + // previous attempt must never survive to mislead diag/postinstall on this + // one, even if this attempt fails before reaching doTasksE. + clearProvisionResult() + firewallModeFlagChanged = cmd.Flags().Changed("firewall-mode") - checkStrFlagEmpty(cmd, cdUidFlagName) - checkStrFlagEmpty(cmd, cdOrgFlagName) - validateCdAndNextDNSFlags() + if !checkStrFlagEmpty(cmd, cdUidFlagName) { + return nil + } + if !checkStrFlagEmpty(cmd, cdOrgFlagName) { + return nil + } + if !validateCdAndNextDNSFlags() { + return nil + } svcConfig := sc.createServiceConfig() osArgs := os.Args[2:] @@ -81,18 +92,21 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { // Without this, a typo like "--intercept-mode fds" would install the service, // the child process would Fatal() on the invalid value, and the parent would // then uninstall — confusing and destructive. - if interceptMode != "" && !validInterceptMode(interceptMode) { - logger.Fatal().Msgf("invalid --intercept-mode value %q: must be 'off', 'dns', or 'hard'", interceptMode) + if !validateInterceptModeFlag(interceptMode) { + return nil } - if firewallModeFlagChanged && !validFirewallMode(firewallMode) { - logger.Fatal().Msgf("invalid --firewall-mode value %q: must be 'off' or 'on'", firewallMode) + if !validateFirewallModeFlag(firewallModeFlagChanged, firewallMode, nil) { + return nil } // Initialize service manager with proper configuration s, p, err := sc.initializeServiceManagerWithServiceConfig(svcConfig) if err != nil { logger.Error().Err(err).Msg("Failed to initialize service manager") - return err + // A bare error return would exit 1 with no result file, so support + // could not tell this failure from a start that never ran. + failProvisionUnclassified("initialize service manager: "+err.Error(), nil) + return nil } p.cfg = &cfg @@ -121,7 +135,8 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { // An explicit "off" argument must override a previously persisted config // value while the service clears that value on startup. if err := removeServiceFlag("--intercept-mode"); err != nil { - logger.Fatal().Err(err).Msg("failed to remove existing intercept mode from service arguments") + failRunUnclassified(logger.Error().Err(err), fmt.Sprintf("failed to remove existing intercept mode from service arguments: %v", err), nil) + return nil } if interceptMode == "off" { @@ -130,10 +145,12 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { logger.Notice().Msgf("Existing service detected — appending --intercept-mode %s to service arguments", interceptMode) } if err := appendServiceFlag("--intercept-mode"); err != nil { - logger.Fatal().Err(err).Msg("failed to append intercept flag to service arguments") + failRunUnclassified(logger.Error().Err(err), fmt.Sprintf("failed to append intercept flag to service arguments: %v", err), nil) + return nil } if err := appendServiceFlag(interceptMode); err != nil { - logger.Fatal().Err(err).Msg("failed to append intercept mode value to service arguments") + failRunUnclassified(logger.Error().Err(err), fmt.Sprintf("failed to append intercept mode value to service arguments: %v", err), nil) + return nil } // Stop the service if running (bypasses ctrld pin — this is an @@ -250,7 +267,8 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { if startOnly && isCtrldInstalled { tryReadingConfigWithNotice(false, true) if err := v.Unmarshal(&cfg); err != nil { - logger.Fatal().Msgf("Failed to unmarshal config: %v", err) + failRunUnclassified(logger.Error(), fmt.Sprintf("failed to unmarshal config: %v", err), nil) + return nil } // if already running, dont restart @@ -277,8 +295,6 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { {s.Start, true, "Start"}, {noticeWritingControlDConfig, false, "Notice writing ControlD config"}, } - // Any result found later must come from this attempt, not a stale run. - clearProvisionResult() startAttemptAt := time.Now() logger.Notice().Msg("Starting existing ctrld service") failedTask, taskErr := doTasksE(tasks) @@ -287,12 +303,13 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { failProvision(newProvisionResult(code, serviceTaskErrorSummary(failedTask, taskErr), nil, provisionSecrets()...), nil) return nil } - os.Exit(1) + failProvisionUnclassified(serviceTaskErrorSummary(failedTask, taskErr), nil) + return nil } sockDir, err := socketDir() if err != nil { - logger.Warn().Err(err).Msg("Failed to get socket directory") - os.Exit(1) + failRunUnclassified(logger.Error(), fmt.Sprintf("failed to get socket directory: %v", err), nil) + return nil } // The daemon can start and still fail provisioning (for example a @@ -325,7 +342,9 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { } if cdUID != "" { - _ = doValidateCdRemoteConfig(cdUID, true) + if err := doValidateCdRemoteConfig(cdUID, true); err != nil { + return nil + } } else if uid := cdUIDFromProvToken(); uid != "" { cdUID = uid logger.Debug().Msg("Using uid from provision token") @@ -334,7 +353,9 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { svcConfig.Arguments = append(svcConfig.Arguments, "--cd="+cdUID) } if cdUID != "" { - validateCdUpstreamProtocol() + if !validateCdUpstreamProtocol(nil) { + return nil + } } if configPath != "" { @@ -344,7 +365,8 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { tryReadingConfigWithNotice(writeDefaultConfig, true) if err := v.Unmarshal(&cfg); err != nil { - logger.Fatal().Msgf("Failed to unmarshal config: %v", err) + failRunUnclassified(logger.Error(), fmt.Sprintf("failed to unmarshal config: %v", err), nil) + return nil } initInteractiveLogging() @@ -384,8 +406,6 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { // generated after s.Start, so we notice users here for consistent with nextdns mode. {noticeWritingControlDConfig, false, "Notice writing ControlD config"}, } - // Any result found later must come from this attempt, not a stale run. - clearProvisionResult() startAttemptAt := time.Now() logger.Notice().Msg("Starting service") failedTask, taskErr := doTasksE(tasks) @@ -394,9 +414,9 @@ func (sc *ServiceCommand) Start(cmd *cobra.Command, args []string) error { failProvision(newProvisionResult(code, serviceTaskErrorSummary(failedTask, taskErr), nil, provisionSecrets()...), nil) return nil } - // Not a service-stage task. doTasksE already logged the cause; exit - // non-zero instead of the old silent fall-through that exited 0. - os.Exit(1) + // Not a service-stage task. doTasksE already logged the cause; classify + // UNCLASSIFIED instead of the old silent fall-through that exited 0. + failProvisionUnclassified(serviceTaskErrorSummary(failedTask, taskErr), nil) return nil } diff --git a/cmd/cli/commands_service_start_test.go b/cmd/cli/commands_service_start_test.go new file mode 100644 index 0000000..c240176 --- /dev/null +++ b/cmd/cli/commands_service_start_test.go @@ -0,0 +1,153 @@ +package cli + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +// startFunctionSource extracts the source text of ServiceCommand.Start's body +// from commands_service_start.go. Driving Start() itself end-to-end for every +// early-return branch is not practical in a unit test: within a few lines of +// any check failing, Start() reaches into the real OS service manager. Some +// invariants about its shape are cheaper and more reliable to pin by reading +// the source than by executing it. +func startFunctionSource(t *testing.T) string { + t.Helper() + file := packageSourcePath(t, "commands_service_start.go") + fset := token.NewFileSet() + node, err := parser.ParseFile(fset, file, nil, 0) + if err != nil { + t.Fatalf("could not parse %s: %v", file, err) + } + for _, decl := range node.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Name.Name != "Start" || fn.Recv == nil { + continue + } + src, err := os.ReadFile(file) + if err != nil { + t.Fatalf("could not read %s: %v", file, err) + } + start := fset.Position(fn.Body.Lbrace).Offset + end := fset.Position(fn.Body.Rbrace).Offset + return string(src[start:end]) + } + t.Fatalf("ServiceCommand.Start not found in %s", file) + return "" +} + +// TestServiceCommandStartClearsProvisionResultBeforeAnyCheck pins the +// ordering fix: clearProvisionResult() must run before every check in +// Start() that can fail or return early, not just before doTasksE. Without +// this, a check between the top of Start() and the old call sites could +// return early (whether by writing its own classified failure or, like the +// "service already running" and service-manager-init-error paths, by writing +// nothing at all) while a previous attempt's result file was still sitting +// there to mislead diag/postinstall on retry. +func TestServiceCommandStartClearsProvisionResultBeforeAnyCheck(t *testing.T) { + body := startFunctionSource(t) + + clearIdx := strings.Index(body, "clearProvisionResult()") + if clearIdx == -1 { + t.Fatal("Start() no longer calls clearProvisionResult()") + } + + // Every check or step that can return out of Start() before reaching + // doTasksE. Each must appear after the entry clear. + earlyChecks := []string{ + "checkStrFlagEmpty(", + "validateCdAndNextDNSFlags(", + "validateInterceptModeFlag(", + "validateFirewallModeFlag(", + "initializeServiceManagerWithServiceConfig(", + "doTasksE(", + } + for _, check := range earlyChecks { + idx := strings.Index(body, check) + if idx == -1 { + t.Fatalf("expected Start() to still call %s", check) + } + if idx < clearIdx { + t.Errorf("%s appears before clearProvisionResult(): a failure there could leave a stale result file behind", check) + } + } +} + +// TestServiceCommandStartClassifiesServiceManagerInitFailure pins the fix for +// a bare error return: a service-manager init failure in Start() must fail +// through failProvisionUnclassified, so a result file and the identifier line +// exist, instead of returning the error for a plain exit 1. +func TestServiceCommandStartClassifiesServiceManagerInitFailure(t *testing.T) { + body := startFunctionSource(t) + initIdx := strings.Index(body, "initializeServiceManagerWithServiceConfig(") + if initIdx == -1 { + t.Fatal("Start() no longer calls initializeServiceManagerWithServiceConfig") + } + branchEnd := strings.Index(body[initIdx:], "p.cfg = &cfg") + if branchEnd == -1 { + t.Fatal("could not find the end of the service-manager init branch") + } + branch := body[initIdx : initIdx+branchEnd] + if !strings.Contains(branch, "failProvisionUnclassified(") { + t.Error("service-manager init failure does not fail through failProvisionUnclassified") + } + if strings.Contains(branch, "return err") { + t.Error("service-manager init failure still returns the bare error, which exits 1 with no result file") + } +} + +// startTestCommand builds the minimal cobra.Command ServiceCommand.Start needs +// before it can reach its early --intercept-mode check: the --cd/--cd-org +// flags must exist (checkStrFlagEmpty looks them up unconditionally) but stay +// unchanged, so neither Fatals. +func startTestCommand() *cobra.Command { + cmd := &cobra.Command{} + cmd.Flags().String(cdUidFlagName, "", "") + cmd.Flags().String(cdOrgFlagName, "", "") + return cmd +} + +// TestServiceCommandStartReplacesStaleResultOnEarlyClassifiedFailure is a +// behavioral companion to the structural test above: it drives the real +// Start() through its earliest classified failure (an invalid +// --intercept-mode) and checks the file left behind names the new attempt, +// not a stale one seeded beforehand. +func TestServiceCommandStartReplacesStaleResultOnEarlyClassifiedFailure(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldIntercept, oldNextdns, oldFirewallChanged := interceptMode, nextdns, firewallModeFlagChanged + t.Cleanup(func() { + interceptMode, nextdns, firewallModeFlagChanged = oldIntercept, oldNextdns, oldFirewallChanged + }) + cdUID, cdOrg, nextdns = "", "", "" + interceptMode = "bogus" // fails validateInterceptModeFlag before any OS work + + if err := writeProvisionResult(newProvisionResult(provisionCodeServiceStartFailed, "a previous failed attempt", nil)); err != nil { + t.Fatal(err) + } + + sc := NewServiceCommand() + if err := sc.Start(startTestCommand(), nil); err != nil { + t.Fatalf("Start() error = %v", err) + } + + wantExit := provisionExitCodeForCode[provisionCodeInterceptModeInvalid] + if *exitCode != wantExit { + t.Fatalf("exit = %d, want %d (validateInterceptModeFlag should have run)", *exitCode, wantExit) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code == string(provisionCodeServiceStartFailed) { + t.Fatal("stale result from a previous attempt survived the new attempt") + } + if r.Code != string(provisionCodeInterceptModeInvalid) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeInterceptModeInvalid) + } +} diff --git a/cmd/cli/hostname.go b/cmd/cli/hostname.go index 5b091c2..67d56f3 100644 --- a/cmd/cli/hostname.go +++ b/cmd/cli/hostname.go @@ -1,6 +1,10 @@ package cli -import "regexp" +import ( + "fmt" + "regexp" + "strings" +) // validHostname reports whether hostname is a valid hostname. // A valid hostname contains 3 -> 64 characters and conform to RFC1123. @@ -16,3 +20,67 @@ func validHostname(hostname string) bool { validHostnameRfc1123 := regexp.MustCompile(`^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])$`) return validHostnameRfc1123.MatchString(hostname) } + +// isHostnameChar reports whether r is part of validHostname's accepted +// charset (letters, digits, hyphen, dot). It does not check position, so a +// hostname can fail validHostname on structure (length, leading/trailing +// hyphen) while every one of its characters passes here. +func isHostnameChar(r rune) bool { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + return true + case r == '-' || r == '.': + return true + } + return false +} + +// offendingHostnameChars returns the distinct characters in hostname that +// validHostname's charset does not accept, in first-seen order. Empty when +// every character is accepted - a rejection can still come from structure +// alone (too short, too long, a leading or trailing hyphen). +func offendingHostnameChars(hostname string) string { + seen := make(map[rune]bool) + var bad []rune + for _, r := range hostname { + if isHostnameChar(r) || seen[r] { + continue + } + seen[r] = true + bad = append(bad, r) + } + return string(bad) +} + +// serverFoldedHostnameChars are the characters ControlD's +// DevicesTableModel.formatDeviceName folds to '-' (or strips) when it +// registers a device name server-side. Only '.' can actually reach +// hostnameMayBeFoldedByServer through validateCustomHostnameFlag's guarded +// path: validHostname runs first and already rejects any hostname +// containing a space or a '+' as CUSTOM_HOSTNAME_INVALID, so those two never +// get here from an explicit --custom-hostname value. They stay in this set +// for completeness: a mobile caller can set CustomHostname to an +// OS-derived default directly, without going through +// validateCustomHostnameFlag at all, so a space or '+' can still reach the +// API unvalidated by this client. +const serverFoldedHostnameChars = ". +" + +// hostnameMayBeFoldedByServer reports whether hostname contains a character +// ControlD may fold or strip when it registers the device, so the name ctrld +// accepted may not be the name the dashboard ends up showing. +func hostnameMayBeFoldedByServer(hostname string) bool { + return strings.ContainsAny(hostname, serverFoldedHostnameChars) +} + +// customHostnameFailureMessage names the field, the offending character(s) +// when there are any, and the allowed format for CUSTOM_HOSTNAME_INVALID. +// ctrld's accept/reject rule (validHostname) is unchanged - this only +// explains a rejection that used to be a bare fatal exit. +func customHostnameFailureMessage(hostname string) string { + const allowedFormat = "3-64 characters of letters, digits, hyphens, and dots (RFC1123 hostname format)" + reason := "is not a valid hostname" + if bad := offendingHostnameChars(hostname); bad != "" { + reason = fmt.Sprintf("contains characters a hostname cannot use: %q", bad) + } + return fmt.Sprintf("--custom-hostname (CustomHostname) %q %s; allowed format: %s", hostname, reason, allowedFormat) +} diff --git a/cmd/cli/hostname_test.go b/cmd/cli/hostname_test.go index f44b231..b096215 100644 --- a/cmd/cli/hostname_test.go +++ b/cmd/cli/hostname_test.go @@ -33,3 +33,65 @@ func Test_validHostname(t *testing.T) { }) } } + +// TestOffendingHostnameChars pins the characters surfaced in the +// CUSTOM_HOSTNAME_INVALID message, so the failure names what is actually +// wrong instead of a bare "invalid hostname". +func TestOffendingHostnameChars(t *testing.T) { + tests := []struct { + name string + hostname string + want string + }{ + {"single offender", "foo@bar", "@"}, + {"space", "foo bar", " "}, + {"dot is allowed", "foo.bar", ""}, + {"hyphen is allowed", "foo-bar", ""}, + {"distinct offenders in order", "a!b!c#d", "!#"}, + {"structurally invalid but no bad char", strings.Repeat("a", 65), ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := offendingHostnameChars(tc.hostname); got != tc.want { + t.Errorf("offendingHostnameChars(%q) = %q, want %q", tc.hostname, got, tc.want) + } + }) + } +} + +// TestHostnameMayBeFoldedByServer pins the characters ControlD's +// DevicesTableModel.formatDeviceName folds or strips when it registers a +// device, so a ctrld-accepted name using one gets a heads-up notice instead +// of silently registering under a different name. +func TestHostnameMayBeFoldedByServer(t *testing.T) { + tests := []struct { + name string + hostname string + want bool + }{ + {"dot", "foo.bar", true}, + {"space", "foo bar", true}, + {"plus", "foo+bar", true}, + {"plain", "foobar", false}, + {"hyphen only", "foo-bar", false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := hostnameMayBeFoldedByServer(tc.hostname); got != tc.want { + t.Errorf("hostnameMayBeFoldedByServer(%q) = %v, want %v", tc.hostname, got, tc.want) + } + }) + } +} + +// TestCustomHostnameFailureMessage pins the message shape the T6 contract +// requires: the flag/field name, the offending character(s), and the +// allowed format. +func TestCustomHostnameFailureMessage(t *testing.T) { + msg := customHostnameFailureMessage("foo@bar") + for _, want := range []string{"--custom-hostname", "CustomHostname", "@", "allowed format"} { + if !strings.Contains(msg, want) { + t.Errorf("customHostnameFailureMessage() = %q, want it to contain %q", msg, want) + } + } +} diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index c457f2a..af47a69 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -710,6 +710,16 @@ func (p *prog) setupUpstream(cfg *ctrld.Config) { p.ptrNameservers = ptrNameservers } +// reportServeDNSFailure classifies a listener that bound successfully - the +// LISTENER_* codes already rule out a bind conflict - but failed to actually +// serve DNS. No dedicated code exists for this, so it falls back to +// UNCLASSIFIED. notifyExitToLogServer unblocks a waiting "ctrld start" before +// the process exits. +func (p *prog) reportServeDNSFailure(listenerNum string, err error) { + msg := fmt.Sprintf("unable to start dns proxy on listener.%s: %v", listenerNum, err) + failRunUnclassified(p.Error().Err(err), msg, p.notifyExitToLogServer) +} + // run runs the ctrld main components. // // The reload boolean indicates that the function is run when ctrld first start @@ -827,7 +837,7 @@ func (p *prog) run(reload bool, reloadCh chan struct{}) { // Changes to listeners config require a service restart, not just reload. serveCtx := context.Background() if err := p.serveDNS(serveCtx, listenerNum); err != nil { - p.Fatal().Err(err).Msgf("Unable to start dns proxy on listener.%s", listenerNum) + p.reportServeDNSFailure(listenerNum, err) } p.Debug().Msgf("End of serveDNS listener.%s: %s", listenerNum, addr) }(listenerNum) @@ -994,8 +1004,11 @@ var ( initializeOsResolverWithSystemNameserversFn = ctrld.InitializeOsResolverWithSystemNameservers setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface resetDNSFn = (*prog).resetDNS - refuseFallbackFatal = func(format string, v ...any) { - mainLog.Load().Fatal().Msgf(format, v...) + // refuseFallbackFatal reports a startup failure the interface-DNS fallback + // cannot safely paper over, then exits. No dedicated code exists for this, + // so it falls back to UNCLASSIFIED. + refuseFallbackFatal = func(p *prog, format string, v ...any) { + failRunUnclassified(p.Error(), fmt.Sprintf(format, v...), p.notifyExitToLogServer) } ) @@ -1081,7 +1094,7 @@ func (p *prog) setDNS(systemNameservers []string) { // Leave the host resolvable: restore static settings or DHCP rather than // exiting with an interface still pointed at a ctrld that is not serving. resetDNSFn(p, false, true) - refuseFallbackFatal("Refusing to fall back to interface DNS: it cannot direct queries to %s:%d, which would leave this host with no working resolver. Free port 53 for ctrld, or resolve the intercept failure, then start again.", lc.IP, lc.Port) + refuseFallbackFatal(p, "Refusing to fall back to interface DNS: it cannot direct queries to %s:%d, which would leave this host with no working resolver. Free port 53 for ctrld, or resolve the intercept failure, then start again.", lc.IP, lc.Port) // Unreachable in production - the line above exits - but returning // explicitly keeps the refusal from depending on that, so nothing can // fall through to installing the fallback this just rejected. diff --git a/cmd/cli/prog_intercept_fallback_test.go b/cmd/cli/prog_intercept_fallback_test.go index 96291e8..40b2536 100644 --- a/cmd/cli/prog_intercept_fallback_test.go +++ b/cmd/cli/prog_intercept_fallback_test.go @@ -121,7 +121,7 @@ func newInterceptFallbackHarness(t *testing.T, lc *ctrld.ListenerConfig) *interc return nil } resetDNSFn = func(_ *prog, _ bool, _ bool) { h.resetCalls++ } - refuseFallbackFatal = func(format string, v ...any) { + refuseFallbackFatal = func(_ *prog, format string, v ...any) { h.refusals = append(h.refusals, fmt.Sprintf(format, v...)) } diff --git a/cmd/cli/prog_log.go b/cmd/cli/prog_log.go index 91e797e..456d635 100644 --- a/cmd/cli/prog_log.go +++ b/cmd/cli/prog_log.go @@ -31,3 +31,12 @@ func (p *prog) Error() *ctrld.LogEvent { func (p *prog) Notice() *ctrld.LogEvent { return p.logger.Load().Notice() } + +// notifyExitToLogServer closes this run's connection to the HTTP log server, +// so a waiting "ctrld start" is not left waiting on it after this process +// exits. A nil connection (log server never started) is a no-op. +func (p *prog) notifyExitToLogServer() { + if p.logConn != nil { + _ = p.logConn.Close() + } +} diff --git a/cmd/cli/provision_boundary_guard_test.go b/cmd/cli/provision_boundary_guard_test.go new file mode 100644 index 0000000..3f29862 --- /dev/null +++ b/cmd/cli/provision_boundary_guard_test.go @@ -0,0 +1,209 @@ +package cli + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "runtime" + "testing" +) + +// packageSourcePath resolves a file in this package's source directory, or +// skips the test when the source is not available. The Windows CI job runs +// pre-built test binaries with no checkout, so neither the working directory +// nor the build-time path recorded by runtime.Caller reaches the source +// there. Skipping is safe for these static checks: they analyze the same +// source on every platform, and the jobs with a checkout still enforce them. +func packageSourcePath(t *testing.T, name string) string { + t.Helper() + if _, err := os.Stat(name); err == nil { + return name + } + if _, thisFile, _, ok := runtime.Caller(0); ok { + p := filepath.Join(filepath.Dir(thisFile), name) + if _, err := os.Stat(p); err == nil { + return p + } + } + t.Skipf("package source for %s not available (pre-built test binary without a checkout); this static check runs where source is present", name) + return "" +} + +// provisionBoundaryFiles are the files this lane's provisioning-hardening +// work audited: every terminal path they can reach must report a classified +// failure (a result file, an output line, a stage-scoped exit code), not a +// bare crash installer tooling cannot read. TestProvisioningTerminalPathAudit +// documents the audit by scenario; this test enforces it structurally so a +// later change cannot reintroduce a bare fatal without either classifying it +// or adding it here with a reason. +// +// Scope: the check sees direct calls (".Fatal()", "os.Exit", "panic") in the +// files listed here. A helper defined in one of these files is caught once, +// at the body that holds the call, so an indirect exit through it needs no +// second rule. A helper defined outside the list, such as +// checkHasElevatedPrivilege in service.go wired as the "start" PreRun, is +// outside the reach of this check. TestProvisioningTerminalPathAudit lists +// those by hand in its exclusion comment. +var provisionBoundaryFiles = []string{ + "cli.go", + "commands_service_start.go", + "commands_run.go", + "prog.go", + "provision_result.go", +} + +// provisionBoundaryFatalAllowlist is every call in the files above that +// still reaches a bare ".Fatal()" (on any logger chain), "os.Exit", or +// "panic", keyed +// by "file.go:FuncName" (FuncName includes the receiver type for methods, +// e.g. "prog.setDNS"). Each entry names why it is not a provisioning +// failure, or why it predates and sits outside the --cd-org boundary this +// lane hardened. +var provisionBoundaryFatalAllowlist = map[string]string{ + "cli.go:RunMobile": `panic on a nil AppConfig is a programming error in the mobile host ` + + `app, at an entry point the CLI command tree never calls. Not reachable from ` + + `"ctrld start --cd-org".`, + + "cli.go:run": `os.Exit(0) is the successful exit of the daemon-respawn launcher, right ` + + `after it starts the real background process. Not a failure.`, + + "cli.go:readConfigFile": `shared CLI config-parsing helper used the same way by every ` + + `invocation mode (config-file, no-config, nextdns, --cd, --cd-org). Predates and is ` + + `orthogonal to the --cd-org provisioning boundary.`, + + "cli.go:processNoConfigFlags": `shared CLI helper enforcing --listen/--primary_upstream in ` + + `no-config mode. Applies uniformly across every invocation mode, not specifically to ` + + `provisioning.`, + + "cli.go:processListenFlag": `shared CLI helper parsing --listen. Applies uniformly across ` + + `every invocation mode, not specifically to provisioning.`, + + "cli.go:readConfigWithNotice": `shared CLI helper (userHomeDir failure while locating the ` + + `default config file). Applies uniformly across every invocation mode, not specifically ` + + `to provisioning.`, + + "cli.go:checkStrFlagEmpty": `shared flag-emptiness helper. Only an explicit empty --cd-org ` + + `is a provisioning-token value, classified separately inside this function; every other ` + + `flag (currently --cd) keeps its bare fatal.`, + + "commands_service_start.go:ServiceCommand.Start": `the deactivation pin check's ` + + `os.Exit(126). Out of scope for this lane by contract ("pin-check 126 untouched").`, + + "prog.go:prog.Stop": `the deactivation pin check's os.Exit(126). Out of scope for this ` + + `lane by contract ("pin-check 126 untouched").`, + + "prog.go:prog.setDNS": `the runtime --intercept-mode validation (validInterceptMode), ` + + `distinct from the early "ctrld start" check that already reports ` + + `INTERCEPT_MODE_INVALID. Left alone per T6's own doc comment.`, +} + +// funcDeclKey names a function declaration the way +// provisionBoundaryFatalAllowlist keys it: "FuncName" for a plain function, +// "ReceiverType.FuncName" for a method. +func funcDeclKey(fn *ast.FuncDecl) string { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return fn.Name.Name + } + recvType := fn.Recv.List[0].Type + if star, ok := recvType.(*ast.StarExpr); ok { + recvType = star.X + } + if ident, ok := recvType.(*ast.Ident); ok { + return ident.Name + "." + fn.Name.Name + } + return fn.Name.Name +} + +// isBareFatalOrExitCall reports whether call is ".Fatal(...)" on any +// logger chain, "os.Exit(...)", or a bare "panic(...)". +func isBareFatalOrExitCall(call *ast.CallExpr) bool { + if ident, ok := call.Fun.(*ast.Ident); ok && ident.Name == "panic" { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok { + return false + } + if sel.Sel.Name == "Fatal" { + return true + } + if sel.Sel.Name == "Exit" { + if ident, ok := sel.X.(*ast.Ident); ok && ident.Name == "os" { + return true + } + } + return false +} + +// TestProvisionBoundaryHasNoUnlistedBareFatal parses every provisioning +// boundary file and flags a bare Fatal/os.Exit that is not in the allowlist +// above. A new one appearing here means a change added a crash the +// provisioning boundary's contract does not allow: classify it through +// failProvision/failProvisionUnclassified, or add it to the allowlist with a +// reason if it genuinely sits outside the boundary. +func TestProvisionBoundaryHasNoUnlistedBareFatal(t *testing.T) { + fset := token.NewFileSet() + for _, file := range provisionBoundaryFiles { + node, err := parser.ParseFile(fset, packageSourcePath(t, file), nil, 0) + if err != nil { + t.Fatalf("could not parse %s: %v", file, err) + } + for _, decl := range node.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + key := file + ":" + funcDeclKey(fn) + ast.Inspect(fn.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || !isBareFatalOrExitCall(call) { + return true + } + if _, allowed := provisionBoundaryFatalAllowlist[key]; !allowed { + pos := fset.Position(call.Pos()) + t.Errorf("%s:%d: unlisted bare fatal/exit in %s - classify it through "+ + "failProvision/failProvisionUnclassified, or add it to "+ + "provisionBoundaryFatalAllowlist with a reason", file, pos.Line, key) + } + return true + }) + } + } +} + +// TestProvisionBoundaryAllowlistHasNoStaleEntries is the converse check: every +// allowlisted key must still name a real bare Fatal/os.Exit. A stale entry +// (the call was removed or reclassified) would silently widen the allowlist +// and hide a real regression the next time this test runs. +func TestProvisionBoundaryAllowlistHasNoStaleEntries(t *testing.T) { + fset := token.NewFileSet() + found := make(map[string]bool, len(provisionBoundaryFatalAllowlist)) + for _, file := range provisionBoundaryFiles { + node, err := parser.ParseFile(fset, packageSourcePath(t, file), nil, 0) + if err != nil { + t.Fatalf("could not parse %s: %v", file, err) + } + for _, decl := range node.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + key := file + ":" + funcDeclKey(fn) + ast.Inspect(fn.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || !isBareFatalOrExitCall(call) { + return true + } + found[key] = true + return true + }) + } + } + for key := range provisionBoundaryFatalAllowlist { + if !found[key] { + t.Errorf("provisionBoundaryFatalAllowlist[%q] no longer matches any bare fatal/exit; remove the stale entry", key) + } + } +} diff --git a/cmd/cli/provision_input_test.go b/cmd/cli/provision_input_test.go new file mode 100644 index 0000000..7914c5b --- /dev/null +++ b/cmd/cli/provision_input_test.go @@ -0,0 +1,422 @@ +package cli + +import ( + "context" + "strings" + "testing" + + "github.com/spf13/cobra" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + + "github.com/Control-D-Inc/ctrld" + "github.com/Control-D-Inc/ctrld/internal/controld" +) + +// captureMainLog swaps mainLog for a test-local buffer logger and restores +// the previous logger on cleanup. Asserting on the shared logOutput sink is +// order-dependent: a test that ran earlier can re-store mainLog (Windows' +// Test_validInterfaces calls initConsoleLogging), leaving the shared buffer +// stale for every later test. +func captureMainLog(t *testing.T) *syncBuffer { + t.Helper() + buf := &syncBuffer{} + core := zapcore.NewCore( + zapcore.NewConsoleEncoder(zap.NewDevelopmentEncoderConfig()), + zapcore.AddSync(buf), + zap.DebugLevel, + ) + old := mainLog.Load() + mainLog.Store(&ctrld.Logger{Logger: zap.New(core)}) + t.Cleanup(func() { mainLog.Store(old) }) + return buf +} + +func TestProvisionTokenShapeValid(t *testing.T) { + tests := []struct { + name string + token string + want bool + }{ + {"too short", "abcde", false}, + {"minimum length", "abcdef", true}, + {"maximum length", strings.Repeat("a", 64), true}, + {"too long", strings.Repeat("a", 65), false}, + {"contains space", "org-v1- abc", false}, + {"contains tab", "org-v1-\tabc", false}, + {"contains newline", "org-v1-\nabc", false}, + {"contains control char", "org-v1-\x00abc", false}, + {"sane with prefix", "org-v1-abcdef123456", true}, + {"sane without prefix", "legacytoken123", true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := provisionTokenShapeValid(tc.token); got != tc.want { + t.Errorf("provisionTokenShapeValid(%q) = %v, want %v", tc.token, got, tc.want) + } + }) + } +} + +// TestCdUIDFromProvTokenMalformedToken proves a malformed --cd-org value is +// classified before any network attempt: no call reaches fetchResolverUIDFn, +// and the malformed value itself never appears in the persisted message. +func TestCdUIDFromProvTokenMalformedToken(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldFetch, oldHostname := fetchResolverUIDFn, customHostname + t.Cleanup(func() { fetchResolverUIDFn, customHostname = oldFetch, oldHostname }) + + cdUID = "" + const malformed = "ab cd" + cdOrg = malformed + customHostname = "" + + called := false + fetchResolverUIDFn = func(context.Context, *controld.UtilityOrgRequest, string, bool) (*controld.ResolverConfig, error) { + called = true + return nil, nil + } + + if got := cdUIDFromProvToken(); got != "" { + t.Errorf("cdUIDFromProvToken() = %q, want empty on failure", got) + } + if called { + t.Error("fetchResolverUIDFn was called; malformed token must be rejected before any network attempt") + } + if *exitCode != provisionExitCodeForCode[provisionCodeProvisionTokenMalformed] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeProvisionTokenMalformed]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeProvisionTokenMalformed) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeProvisionTokenMalformed) + } + if r.Stage != string(provisionStageInput) { + t.Errorf("stage = %q, want %q", r.Stage, provisionStageInput) + } + if strings.Contains(r.Message, malformed) { + t.Errorf("malformed token leaked into result message: %q", r.Message) + } +} + +// TestCdUIDFromProvTokenPrefixlessTokenProceeds proves a token missing the +// "org-v1-" prefix is not rejected - only warned about - since legacy codes +// may lack it. +func TestCdUIDFromProvTokenPrefixlessTokenProceeds(t *testing.T) { + _, _ = stubProvisionGlobals(t) + logBuf := captureMainLog(t) + oldFetch, oldHostname := fetchResolverUIDFn, customHostname + t.Cleanup(func() { fetchResolverUIDFn, customHostname = oldFetch, oldHostname }) + + cdUID = "" + cdOrg = "legacytoken123" + customHostname = "" + + called := false + fetchResolverUIDFn = func(context.Context, *controld.UtilityOrgRequest, string, bool) (*controld.ResolverConfig, error) { + called = true + return &controld.ResolverConfig{UID: "resolved-uid"}, nil + } + + if got := cdUIDFromProvToken(); got != "resolved-uid" { + t.Errorf("cdUIDFromProvToken() = %q, want resolved-uid", got) + } + if !called { + t.Error("fetchResolverUIDFn was not called; a prefixless-but-sane token must still proceed") + } + if !strings.Contains(logBuf.String(), "org-v1-") { + t.Error("expected a warning mentioning the org-v1- prefix") + } +} + +// TestCdUIDFromProvTokenInvalidCustomHostname proves an invalid +// --custom-hostname value is classified before any network attempt, with a +// message that names the field and format, and never leaks the provision +// token. +func TestCdUIDFromProvTokenInvalidCustomHostname(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldFetch, oldHostname := fetchResolverUIDFn, customHostname + t.Cleanup(func() { fetchResolverUIDFn, customHostname = oldFetch, oldHostname }) + + cdUID = "" + const secretToken = "org-v1-secret-token-42" + cdOrg = secretToken + customHostname = "foo@bar" + + called := false + fetchResolverUIDFn = func(context.Context, *controld.UtilityOrgRequest, string, bool) (*controld.ResolverConfig, error) { + called = true + return nil, nil + } + + if got := cdUIDFromProvToken(); got != "" { + t.Errorf("cdUIDFromProvToken() = %q, want empty on failure", got) + } + if called { + t.Error("fetchResolverUIDFn was called; invalid custom hostname must be rejected before any network attempt") + } + if *exitCode != provisionExitCodeForCode[provisionCodeCustomHostnameInvalid] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeCustomHostnameInvalid]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeCustomHostnameInvalid) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeCustomHostnameInvalid) + } + if !strings.Contains(r.Message, "allowed format") { + t.Errorf("message = %q, want it to contain the allowed format", r.Message) + } + if strings.Contains(r.Message, secretToken) { + t.Errorf("provision token leaked into result message: %q", r.Message) + } +} + +// TestCdUIDFromProvTokenFoldableHostnameLogsNotice proves a hostname that +// ctrld accepts, but ControlD's device-name formatting would fold or strip, +// gets a notice - not a failure - and provisioning still proceeds. +func TestCdUIDFromProvTokenFoldableHostnameLogsNotice(t *testing.T) { + _, _ = stubProvisionGlobals(t) + logBuf := captureMainLog(t) + oldFetch, oldHostname := fetchResolverUIDFn, customHostname + t.Cleanup(func() { fetchResolverUIDFn, customHostname = oldFetch, oldHostname }) + + cdUID = "" + cdOrg = "org-v1-abcdef123456" + customHostname = "foo.bar" + + fetchResolverUIDFn = func(context.Context, *controld.UtilityOrgRequest, string, bool) (*controld.ResolverConfig, error) { + return &controld.ResolverConfig{UID: "resolved-uid"}, nil + } + + if got := cdUIDFromProvToken(); got != "resolved-uid" { + t.Errorf("cdUIDFromProvToken() = %q, want resolved-uid", got) + } + if !strings.Contains(logBuf.String(), "foo.bar") { + t.Error("expected a notice naming the foldable hostname") + } +} + +func TestValidateInterceptModeFlag(t *testing.T) { + oldMode := interceptMode + t.Cleanup(func() { interceptMode = oldMode }) + + t.Run("valid values proceed", func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + for _, mode := range []string{"", "off", "dns", "hard"} { + interceptMode = mode + if !validateInterceptModeFlag(mode) { + t.Errorf("validateInterceptModeFlag(%q) = false, want true", mode) + } + if *exitCode != -1 { + t.Errorf("mode %q: provisionExit called with %d, want no exit", mode, *exitCode) + } + } + }) + + t.Run("invalid value is classified", func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + interceptMode = "fds" + if validateInterceptModeFlag("fds") { + t.Error("validateInterceptModeFlag(\"fds\") = true, want false") + } + if *exitCode != provisionExitCodeForCode[provisionCodeInterceptModeInvalid] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeInterceptModeInvalid]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeInterceptModeInvalid) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeInterceptModeInvalid) + } + for _, want := range []string{"off", "dns", "hard"} { + if !strings.Contains(r.Message, want) { + t.Errorf("message = %q, want it to contain %q", r.Message, want) + } + } + }) +} + +// TestCheckStrFlagEmptyClassifiesEmptyCdOrg proves an explicit empty --cd-org +// is classified as a malformed provisioning token rather than a bare fatal. +func TestCheckStrFlagEmptyClassifiesEmptyCdOrg(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + cmd := &cobra.Command{} + cmd.Flags().String(cdOrgFlagName, "", "") + if err := cmd.Flags().Set(cdOrgFlagName, ""); err != nil { + t.Fatal(err) + } + + if checkStrFlagEmpty(cmd, cdOrgFlagName) { + t.Error("checkStrFlagEmpty() = true, want false for an explicit empty --cd-org") + } + if *exitCode != provisionExitCodeForCode[provisionCodeProvisionTokenMalformed] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeProvisionTokenMalformed]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeProvisionTokenMalformed) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeProvisionTokenMalformed) + } + if r.Stage != string(provisionStageInput) { + t.Errorf("stage = %q, want %q", r.Stage, provisionStageInput) + } + if !strings.Contains(r.Message, cdOrgFlagName) { + t.Errorf("message = %q, want it to name --%s", r.Message, cdOrgFlagName) + } +} + +// TestCheckStrFlagEmptyProceedsWhenNotChangedOrNotEmpty proves the two cases +// that must not classify: the flag was never set, and it was set to a +// non-empty value. +func TestCheckStrFlagEmptyProceedsWhenNotChangedOrNotEmpty(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + + t.Run("flag never set", func(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String(cdOrgFlagName, "", "") + if !checkStrFlagEmpty(cmd, cdOrgFlagName) { + t.Error("checkStrFlagEmpty() = false, want true when the flag was never set") + } + }) + + t.Run("flag set to a non-empty value", func(t *testing.T) { + cmd := &cobra.Command{} + cmd.Flags().String(cdOrgFlagName, "", "") + if err := cmd.Flags().Set(cdOrgFlagName, "org-v1-abc"); err != nil { + t.Fatal(err) + } + if !checkStrFlagEmpty(cmd, cdOrgFlagName) { + t.Error("checkStrFlagEmpty() = false, want true for a non-empty value") + } + }) + + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } +} + +// TestValidateCdAndNextDNSFlagsClassifiesConflict proves --cd or --cd-org +// combined with --nextdns is classified as INVALID_FLAG_COMBINATION, naming +// every flag involved. +func TestValidateCdAndNextDNSFlagsClassifiesConflict(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldCdUID, oldCdOrg, oldNextdns := cdUID, cdOrg, nextdns + t.Cleanup(func() { cdUID, cdOrg, nextdns = oldCdUID, oldCdOrg, oldNextdns }) + + t.Run("non-conflicting combinations proceed", func(t *testing.T) { + cases := []struct{ cdUID, cdOrg, nextdns string }{ + {"", "", ""}, + {"uid123", "", ""}, + {"", "org-v1-abc", ""}, + {"", "", "nextdns-id"}, + } + for _, tc := range cases { + cdUID, cdOrg, nextdns = tc.cdUID, tc.cdOrg, tc.nextdns + if !validateCdAndNextDNSFlags() { + t.Errorf("validateCdAndNextDNSFlags() = false for cdUID=%q cdOrg=%q nextdns=%q, want true", tc.cdUID, tc.cdOrg, tc.nextdns) + } + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + }) + + t.Run("cd-org with nextdns is classified", func(t *testing.T) { + cdUID, cdOrg, nextdns = "", "org-v1-abc", "nextdns-id" + if validateCdAndNextDNSFlags() { + t.Error("validateCdAndNextDNSFlags() = true, want false") + } + if *exitCode != provisionExitCodeForCode[provisionCodeInvalidFlagCombination] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeInvalidFlagCombination]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeInvalidFlagCombination) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeInvalidFlagCombination) + } + if r.Stage != string(provisionStageInput) { + t.Errorf("stage = %q, want %q", r.Stage, provisionStageInput) + } + for _, want := range []string{cdUidFlagName, cdOrgFlagName, nextdnsFlagName} { + if !strings.Contains(r.Message, want) { + t.Errorf("message = %q, want it to name --%s", r.Message, want) + } + } + }) + + t.Run("cd with nextdns is classified", func(t *testing.T) { + cdUID, cdOrg, nextdns = "uid123", "", "nextdns-id" + if validateCdAndNextDNSFlags() { + t.Error("validateCdAndNextDNSFlags() = true, want false") + } + if *exitCode != provisionExitCodeForCode[provisionCodeInvalidFlagCombination] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeInvalidFlagCombination]) + } + }) +} + +// TestValidateCdUpstreamProtocol proves an invalid --proto value is +// classified only once --cd is in play, and that notify runs when given. +func TestValidateCdUpstreamProtocol(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + oldCdUID, oldProto := cdUID, cdUpstreamProto + t.Cleanup(func() { cdUID, cdUpstreamProto = oldCdUID, oldProto }) + + t.Run("no --cd proceeds regardless of protocol", func(t *testing.T) { + cdUID = "" + cdUpstreamProto = "garbage" + if !validateCdUpstreamProtocol(nil) { + t.Error("validateCdUpstreamProtocol(nil) = false, want true when --cd is not set") + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + }) + + t.Run("valid protocols proceed", func(t *testing.T) { + cdUID = "uid123" + for _, proto := range []string{ctrld.ResolverTypeDOH, ctrld.ResolverTypeDOH3} { + cdUpstreamProto = proto + if !validateCdUpstreamProtocol(nil) { + t.Errorf("validateCdUpstreamProtocol(nil) = false for proto %q, want true", proto) + } + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + }) + + t.Run("invalid protocol is classified and notifies", func(t *testing.T) { + cdUID = "uid123" + cdUpstreamProto = "quic" + notified := false + if validateCdUpstreamProtocol(func() { notified = true }) { + t.Error("validateCdUpstreamProtocol() = true, want false") + } + if !notified { + t.Error("notify not called") + } + if *exitCode != provisionExitCodeForCode[provisionCodeInvalidFlagCombination] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeInvalidFlagCombination]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeInvalidFlagCombination) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeInvalidFlagCombination) + } + if !strings.Contains(r.Message, "quic") || !strings.Contains(r.Message, "doh") { + t.Errorf("message = %q, want it to name the given and allowed values", r.Message) + } + }) +} diff --git a/cmd/cli/provision_result.go b/cmd/cli/provision_result.go index 93a0587..8945343 100644 --- a/cmd/cli/provision_result.go +++ b/cmd/cli/provision_result.go @@ -22,6 +22,7 @@ import ( type provisionStage string const ( + provisionStageInput provisionStage = "input" provisionStageBootstrap provisionStage = "bootstrap" provisionStageListener provisionStage = "listener" provisionStageService provisionStage = "service" @@ -30,50 +31,87 @@ const ( type provisionFailureCode string const ( - provisionCodeAPIUnreachable provisionFailureCode = "API_UNREACHABLE" - provisionCodeAPIRejected provisionFailureCode = "API_REJECTED" - provisionCodeAPIDeviceInvalid provisionFailureCode = "API_DEVICE_INVALID" - provisionCodeListenerBindFailed provisionFailureCode = "LISTENER_BIND_FAILED" - provisionCodeListenerAddrUnavail provisionFailureCode = "LISTENER_CONFIGURED_ADDR_UNAVAILABLE" - provisionCodeServiceInstall provisionFailureCode = "SERVICE_INSTALL_FAILED" - provisionCodeServiceStartFailed provisionFailureCode = "SERVICE_START_FAILED" - provisionCodeServiceSelfCheck provisionFailureCode = "SERVICE_SELFCHECK_FAILED" + provisionCodeProvisionTokenMalformed provisionFailureCode = "PROVISION_TOKEN_MALFORMED" + provisionCodeCustomHostnameInvalid provisionFailureCode = "CUSTOM_HOSTNAME_INVALID" + provisionCodeInterceptModeInvalid provisionFailureCode = "INTERCEPT_MODE_INVALID" + provisionCodeInvalidFlagCombination provisionFailureCode = "INVALID_FLAG_COMBINATION" + provisionCodeAPIUnreachable provisionFailureCode = "API_UNREACHABLE" + provisionCodeAPIRejected provisionFailureCode = "API_REJECTED" + provisionCodeAPIDeviceInvalid provisionFailureCode = "API_DEVICE_INVALID" + provisionCodeTokenInvalid provisionFailureCode = "TOKEN_INVALID" + provisionCodeTokenExpired provisionFailureCode = "TOKEN_EXPIRED" + provisionCodeTokenLimitReached provisionFailureCode = "TOKEN_LIMIT_REACHED" + provisionCodeTokenDisabled provisionFailureCode = "TOKEN_DISABLED" + provisionCodeListenerBindFailed provisionFailureCode = "LISTENER_BIND_FAILED" + provisionCodeListenerAddrUnavail provisionFailureCode = "LISTENER_CONFIGURED_ADDR_UNAVAILABLE" + provisionCodeServiceInstall provisionFailureCode = "SERVICE_INSTALL_FAILED" + provisionCodeServiceStartFailed provisionFailureCode = "SERVICE_START_FAILED" + provisionCodeServiceSelfCheck provisionFailureCode = "SERVICE_SELFCHECK_FAILED" + provisionCodeUnclassified provisionFailureCode = "UNCLASSIFIED" ) var allProvisionFailureCodes = []provisionFailureCode{ + provisionCodeProvisionTokenMalformed, + provisionCodeCustomHostnameInvalid, + provisionCodeInterceptModeInvalid, + provisionCodeInvalidFlagCombination, provisionCodeAPIUnreachable, provisionCodeAPIRejected, provisionCodeAPIDeviceInvalid, + provisionCodeTokenInvalid, + provisionCodeTokenExpired, + provisionCodeTokenLimitReached, + provisionCodeTokenDisabled, provisionCodeListenerBindFailed, provisionCodeListenerAddrUnavail, provisionCodeServiceInstall, provisionCodeServiceStartFailed, provisionCodeServiceSelfCheck, + provisionCodeUnclassified, } var provisionStageForCode = map[provisionFailureCode]provisionStage{ - provisionCodeAPIUnreachable: provisionStageBootstrap, - provisionCodeAPIRejected: provisionStageBootstrap, - provisionCodeAPIDeviceInvalid: provisionStageBootstrap, - provisionCodeListenerBindFailed: provisionStageListener, - provisionCodeListenerAddrUnavail: provisionStageListener, - provisionCodeServiceInstall: provisionStageService, - provisionCodeServiceStartFailed: provisionStageService, - provisionCodeServiceSelfCheck: provisionStageService, + provisionCodeProvisionTokenMalformed: provisionStageInput, + provisionCodeCustomHostnameInvalid: provisionStageInput, + provisionCodeInterceptModeInvalid: provisionStageInput, + provisionCodeInvalidFlagCombination: provisionStageInput, + provisionCodeAPIUnreachable: provisionStageBootstrap, + provisionCodeAPIRejected: provisionStageBootstrap, + provisionCodeAPIDeviceInvalid: provisionStageBootstrap, + provisionCodeTokenInvalid: provisionStageBootstrap, + provisionCodeTokenExpired: provisionStageBootstrap, + provisionCodeTokenLimitReached: provisionStageBootstrap, + provisionCodeTokenDisabled: provisionStageBootstrap, + provisionCodeListenerBindFailed: provisionStageListener, + provisionCodeListenerAddrUnavail: provisionStageListener, + provisionCodeServiceInstall: provisionStageService, + provisionCodeServiceStartFailed: provisionStageService, + provisionCodeServiceSelfCheck: provisionStageService, + provisionCodeUnclassified: provisionStageService, } -// Exit codes are grouped by stage (bootstrap 30-39, listener 40-49, service -// 50-59) so the exit code alone names the failed stage. 0-3 belong to -// "ctrld status" and 126 to the deactivation pin check; never reuse those. +// Exit codes are grouped by stage (input 20-29, bootstrap 30-39, listener +// 40-49, service 50-59) so the exit code alone names the failed stage. 0-3 +// belong to "ctrld status" and 126 to the deactivation pin check; never +// reuse those. var provisionExitCodeForCode = map[provisionFailureCode]int{ - provisionCodeAPIUnreachable: 30, - provisionCodeAPIRejected: 31, - provisionCodeAPIDeviceInvalid: 32, - provisionCodeListenerBindFailed: 41, - provisionCodeListenerAddrUnavail: 42, - provisionCodeServiceInstall: 51, - provisionCodeServiceStartFailed: 52, - provisionCodeServiceSelfCheck: 53, + provisionCodeProvisionTokenMalformed: 21, + provisionCodeCustomHostnameInvalid: 22, + provisionCodeInterceptModeInvalid: 23, + provisionCodeInvalidFlagCombination: 24, + provisionCodeAPIUnreachable: 30, + provisionCodeAPIRejected: 31, + provisionCodeAPIDeviceInvalid: 32, + provisionCodeTokenInvalid: 33, + provisionCodeTokenExpired: 34, + provisionCodeTokenLimitReached: 35, + provisionCodeTokenDisabled: 36, + provisionCodeListenerBindFailed: 41, + provisionCodeListenerAddrUnavail: 42, + provisionCodeServiceInstall: 51, + provisionCodeServiceStartFailed: 52, + provisionCodeServiceSelfCheck: 53, + provisionCodeUnclassified: 59, } const ( @@ -106,7 +144,7 @@ type provisionResult struct { // provisionResultPath is a var so tests can point it at a temp dir. var provisionResultPath = func() string { - return ctrld.AbsHomeDir(provisionResultFileName) + return absHomeDir(provisionResultFileName) } // provisionExit is a var so tests can observe the exit code instead of dying. @@ -247,3 +285,24 @@ func failProvision(r *provisionResult, notify func()) { } provisionExit(r.ExitCode) } + +// failProvisionUnclassified persists an UNCLASSIFIED result (service stage, +// exit 59) and exits. It is the fallback for a terminal path that predates a +// stable code - a config unmarshal, a file-system or environment failure - +// so support still gets a result file and a stage-scoped exit code instead +// of a bare crash with nothing to read. No terminal path on the +// provisioning boundary may bypass this or an existing classified code. +func failProvisionUnclassified(message string, notify func()) { + failProvision(newProvisionResult(provisionCodeUnclassified, message, nil, provisionSecrets()...), notify) +} + +// failRunUnclassified logs msg on ev, then fails provisioning as UNCLASSIFIED +// and unblocks a waiting "ctrld start" via notify (nil if none). Callers +// return immediately after this call: provisionExit is stubbed out under +// test, so nothing stops execution from falling through otherwise. ev +// carries whatever the caller already chained onto it (for example .Err()), +// so each call site keeps its own log fields. +func failRunUnclassified(ev *ctrld.LogEvent, msg string, notify func()) { + ev.Msg(msg) + failProvisionUnclassified(msg, notify) +} diff --git a/cmd/cli/provision_result_test.go b/cmd/cli/provision_result_test.go index 3003904..b21bf6b 100644 --- a/cmd/cli/provision_result_test.go +++ b/cmd/cli/provision_result_test.go @@ -20,8 +20,21 @@ func overrideProvisionResultPath(t *testing.T) string { return path } +// The result file must obey the same homedir override as the log and the +// config file, so a daemon started with --homedir writes it next to them. +func TestProvisionResultPathHonorsHomedir(t *testing.T) { + old := homedir + homedir = t.TempDir() + t.Cleanup(func() { homedir = old }) + want := filepath.Join(homedir, provisionResultFileName) + if got := provisionResultPath(); got != want { + t.Errorf("provisionResultPath() = %q, want %q", got, want) + } +} + func TestProvisionCodesMapToOneStageAndInRangeExit(t *testing.T) { stageRanges := map[provisionStage][2]int{ + provisionStageInput: {20, 29}, provisionStageBootstrap: {30, 39}, provisionStageListener: {40, 49}, provisionStageService: {50, 59}, @@ -55,8 +68,8 @@ func TestProvisionCodesMapToOneStageAndInRangeExit(t *testing.T) { } seenExits[exit] = code } - if len(allProvisionFailureCodes) != 8 { - t.Errorf("expected 8 codes, got %d", len(allProvisionFailureCodes)) + if len(allProvisionFailureCodes) != 17 { + t.Errorf("expected 17 codes, got %d", len(allProvisionFailureCodes)) } } diff --git a/cmd/cli/provision_terminal_paths_test.go b/cmd/cli/provision_terminal_paths_test.go new file mode 100644 index 0000000..747ca5c --- /dev/null +++ b/cmd/cli/provision_terminal_paths_test.go @@ -0,0 +1,277 @@ +package cli + +import ( + "errors" + "strings" + "testing" +) + +// TestFailProvisionUnclassified pins the fallback every terminal path on the +// provisioning boundary uses when it predates a stable code: it persists +// UNCLASSIFIED, calls notify, and exits at the code's exit number. +func TestFailProvisionUnclassified(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + notified := false + + failProvisionUnclassified("something failed with no dedicated code", func() { notified = true }) + + if !notified { + t.Error("notify not called") + } + if *exitCode != provisionExitCodeForCode[provisionCodeUnclassified] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeUnclassified]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeUnclassified) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeUnclassified) + } + if r.Stage != string(provisionStageService) { + t.Errorf("stage = %q, want %q", r.Stage, provisionStageService) + } + if !strings.Contains(r.Message, "something failed") { + t.Errorf("message = %q, want it to contain the given message", r.Message) + } +} + +// TestNotifyExitToLogServer pins the method every run()-side failure uses to +// unblock a waiting "ctrld start": it closes the log connection exactly when +// one is set, and is a no-op otherwise. +func TestNotifyExitToLogServer(t *testing.T) { + t.Run("nil connection is a no-op", func(t *testing.T) { + p := &prog{} + p.notifyExitToLogServer() // must not panic + }) + + t.Run("closes a set connection", func(t *testing.T) { + p := &prog{} + fc := &fakeCloser{} + p.logConn = fc + p.notifyExitToLogServer() + if !fc.closed { + t.Error("expected logConn to be closed") + } + }) +} + +type fakeCloser struct{ closed bool } + +func (f *fakeCloser) Write(p []byte) (int, error) { return len(p), nil } +func (f *fakeCloser) Close() error { f.closed = true; return nil } + +// TestValidateFirewallModeFlag covers the shared validator behind both +// --firewall-mode call sites (the early "ctrld start" check and run()'s own +// check on the daemon side). Neither has a dedicated code, so both fall back +// to UNCLASSIFIED. +func TestValidateFirewallModeFlag(t *testing.T) { + t.Run("flag not changed proceeds regardless of value", func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + if !validateFirewallModeFlag(false, "garbage", nil) { + t.Error("validateFirewallModeFlag() = false, want true when the flag was not set") + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + }) + + t.Run("valid values proceed", func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + for _, mode := range []string{"off", "on"} { + if !validateFirewallModeFlag(true, mode, nil) { + t.Errorf("validateFirewallModeFlag(true, %q) = false, want true", mode) + } + } + if *exitCode != -1 { + t.Errorf("provisionExit called with %d, want no exit", *exitCode) + } + }) + + t.Run("invalid value is classified UNCLASSIFIED and notifies", func(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + notified := false + if validateFirewallModeFlag(true, "bogus", func() { notified = true }) { + t.Error("validateFirewallModeFlag(true, \"bogus\") = true, want false") + } + if !notified { + t.Error("notify not called") + } + if *exitCode != provisionExitCodeForCode[provisionCodeUnclassified] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeUnclassified]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeUnclassified) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeUnclassified) + } + if !strings.Contains(r.Message, "off") || !strings.Contains(r.Message, "on") { + t.Errorf("message = %q, want it to name the allowed values", r.Message) + } + }) +} + +// TestReportServeDNSFailure covers the one prog.go path where a listener +// bound successfully (LISTENER_* already ruled that out) but the proxy +// itself failed to start serving - a distinct failure with no dedicated +// code. +func TestReportServeDNSFailure(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + p := &prog{} + p.logger.Store(mainLog.Load()) + notified := false + p.logConn = &fakeCloserNotify{fn: func() { notified = true }} + + p.reportServeDNSFailure("0", errors.New("bind lost mid-flight")) + + if !notified { + t.Error("expected the log connection to be closed") + } + if *exitCode != provisionExitCodeForCode[provisionCodeUnclassified] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeUnclassified]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeUnclassified) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeUnclassified) + } + if !strings.Contains(r.Message, "listener.0") { + t.Errorf("message = %q, want it to name the listener", r.Message) + } +} + +type fakeCloserNotify struct{ fn func() } + +func (f *fakeCloserNotify) Write(p []byte) (int, error) { return len(p), nil } +func (f *fakeCloserNotify) Close() error { f.fn(); return nil } + +// TestRefuseFallbackFatalDefault covers the production refuseFallbackFatal +// closure (not the test-harness override other tests install): it classifies +// UNCLASSIFIED and notifies through the given prog's log connection. +func TestRefuseFallbackFatalDefault(t *testing.T) { + exitCode, _ := stubProvisionGlobals(t) + p := &prog{} + p.logger.Store(mainLog.Load()) + notified := false + p.logConn = &fakeCloserNotify{fn: func() { notified = true }} + + refuseFallbackFatal(p, "cannot fall back: %s", "port 53 is unavailable") + + if !notified { + t.Error("expected the log connection to be closed") + } + if *exitCode != provisionExitCodeForCode[provisionCodeUnclassified] { + t.Errorf("exit = %d, want %d", *exitCode, provisionExitCodeForCode[provisionCodeUnclassified]) + } + r, err := readProvisionResult() + if err != nil { + t.Fatalf("no provision result written: %v", err) + } + if r.Code != string(provisionCodeUnclassified) { + t.Errorf("code = %q, want %q", r.Code, provisionCodeUnclassified) + } + if !strings.Contains(r.Message, "port 53 is unavailable") { + t.Errorf("message = %q, want it to contain the formatted detail", r.Message) + } +} + +// terminalPathCase documents one terminal path this lane audited under +// "ctrld start --cd-org": where it lives, what triggers it, and the code it +// now reports. reachable names a test that exercises the real call site +// end-to-end (through the seams this package already exposes); a path deep +// inside Start()/run() with no seam to trigger it in isolation is documented +// here with reachable == "" and relies on the shared helper tests above +// (TestFailProvisionUnclassified, TestNotifyExitToLogServer, +// TestValidateFirewallModeFlag, TestReportServeDNSFailure, +// TestRefuseFallbackFatalDefault) to prove the wiring it calls into. +type terminalPathCase struct { + file string + scenario string + code provisionFailureCode + reachable string // name of the test proving this exact site, or "" if only the shared helper is exercised +} + +// TestProvisioningTerminalPathAudit is the committed enumeration required for +// "ctrld start --cd-org": every Fatal/os.Exit found by re-deriving the +// requirement (grepping commands_service_start.go, cli.go's run() and its +// callers cdUIDFromProvToken/handleAPIPreflightFailure, commands_run.go, and +// prog.go's pre-daemonization paths), reconciled against a code. None of them +// may reach a bare Fatal/os.Exit any more; each row's code is asserted +// against the known contract maps so a typo'd code name fails this test. +func TestProvisioningTerminalPathAudit(t *testing.T) { + cases := []terminalPathCase{ + // commands_service_start.go (parent "ctrld start" process) + {"commands_service_start.go", "--intercept-mode is not off/dns/hard", provisionCodeInterceptModeInvalid, "TestValidateInterceptModeFlag"}, + {"commands_service_start.go", "--firewall-mode is not off/on", provisionCodeUnclassified, "TestValidateFirewallModeFlag"}, + {"commands_service_start.go / cli.go RunCobraCommand", "explicit empty --cd-org", provisionCodeProvisionTokenMalformed, "TestCheckStrFlagEmptyClassifiesEmptyCdOrg"}, + {"commands_service_start.go", "--cd/--cd-org used together with --nextdns", provisionCodeInvalidFlagCombination, "TestValidateCdAndNextDNSFlagsClassifiesConflict"}, + {"commands_service_start.go", "initializeServiceManagerWithServiceConfig fails", provisionCodeUnclassified, "TestServiceCommandStartClassifiesServiceManagerInitFailure"}, + {"commands_service_start.go / cli.go run()", "--proto is not doh/doh3 once --cd is set", provisionCodeInvalidFlagCombination, "TestValidateCdUpstreamProtocol"}, + {"commands_service_start.go", "removeServiceFlag fails while upgrading an existing service's intercept mode", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "appendServiceFlag(\"--intercept-mode\") fails during the same upgrade", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "appendServiceFlag(mode) fails during the same upgrade", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "config unmarshal fails restarting an already-installed service", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "doTasksE fails on a non-service-stage task restarting an existing service", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "socketDir() fails restarting an existing service", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "config unmarshal fails on a fresh install", provisionCodeUnclassified, ""}, + {"commands_service_start.go", "doTasksE fails on a non-service-stage task on a fresh install", provisionCodeUnclassified, ""}, + + // cli.go run() (the "ctrld run" child process --cd-org spawns, or a direct "ctrld run --cd-org" invocation) + {"cli.go run()", "called with a nil stop channel", provisionCodeUnclassified, ""}, + {"cli.go run()", "--daemon on windows", provisionCodeUnclassified, ""}, + {"cli.go run()", "newService fails building the background service handle", provisionCodeUnclassified, ""}, + {"cli.go run()", "readBase64Config fails", provisionCodeUnclassified, ""}, + {"cli.go run()", "config unmarshal fails", provisionCodeUnclassified, ""}, + {"cli.go run()", "network is not up", provisionCodeUnclassified, ""}, + {"cli.go run()", "--firewall-mode is not off/on (daemon-side check)", provisionCodeUnclassified, "TestValidateFirewallModeFlag"}, + {"cli.go run()", "writeConfigFile fails", provisionCodeUnclassified, ""}, + {"cli.go run()", "validateConfig fails", provisionCodeUnclassified, ""}, + {"cli.go run()", "os.Executable fails in daemon respawn", provisionCodeUnclassified, ""}, + {"cli.go run()", "os.Getwd fails in daemon respawn", provisionCodeUnclassified, ""}, + {"cli.go run()", "cmd.Start fails in daemon respawn", provisionCodeUnclassified, ""}, + + // prog.go (pre-daemonization: listener startup and DNS-intercept setup) + {"prog.go (p *prog) run()", "serveDNS fails after the listener already bound", provisionCodeUnclassified, "TestReportServeDNSFailure"}, + {"prog.go (p *prog) setDNS()", "DNS intercept fails and the interface-DNS fallback cannot reach a non-53 listener", provisionCodeUnclassified, "TestRefuseFallbackFatalDefault"}, + } + + for _, tc := range cases { + if _, ok := provisionStageForCode[tc.code]; !ok { + t.Errorf("%s: %s: code %s is not a known contract code", tc.file, tc.scenario, tc.code) + } + if tc.reachable != "" { + continue + } + t.Logf("documented (exercised only via shared helper wiring, not a standalone seam): %s: %s -> %s", tc.file, tc.scenario, tc.code) + } + + // Out of scope, left as bare Fatal/os.Exit deliberately, with reasons: + // + // - cli.go run(): stopCh nil check is still bare-Fatal-free (converted + // above) but is genuinely unreachable from any CLI invocation - the + // CLI always constructs a fresh channel. Converted anyway above for + // consistency, not because it is reachable. + // - prog.go (p *prog) setDNS() line ~995's own --intercept-mode check: + // explicitly left alone per T6 - it is the "runtime validation in + // prog.go" validInterceptMode's own doc comment calls out as distinct + // from "the early start command check" that got INTERCEPT_MODE_INVALID. + // - prog.go line ~912 (deactivation pin) and commands_service_start.go's + // own deactivation-pin os.Exit: explicitly out of scope per the lane + // contract ("pin-check 126 untouched"). + // - service.go: checkHasElevatedPrivilege's os.Exit(1) in the "start" + // PreRun. It runs before RunE, in a process that cannot write to + // /etc/controld, so no result file is possible there. The postinstall + // always runs as root, and the message names the fix. + // - cli.go: general CLI/config-parsing helpers (readConfigFile, + // decoderErrorFromTomlFile, processNoConfigFlags, processListenFlag, + // tryReadingConfigWithNotice's userHomeDir failure): these predate and + // apply uniformly across every ctrld invocation mode (config-file + // mode, no-config mode, nextdns mode), not specifically to --cd-org + // provisioning. Out of scope for this lane's --cd-org enumeration. + // - checkStrFlagEmpty for --cd (cdUidFlagName) keeps its bare fatal: + // only the --cd-org case above is a provisioning-token value. +} diff --git a/docs/provisioning-failure-codes.md b/docs/provisioning-failure-codes.md index ac23056..f807b7d 100644 --- a/docs/provisioning-failure-codes.md +++ b/docs/provisioning-failure-codes.md @@ -14,9 +14,9 @@ stable code on three surfaces: Installer or MDM wrappers can extract exactly this line (fixed charset: `stage=[a-z]* code=[A-Z_]* (exit [0-9]*)`) into their own logs without risking token leakage from other output. -- **Exit code** — stage-scoped: bootstrap 30–39, listener 40–49, - service 50–59. Unrelated existing contracts are unchanged - (`ctrld-client status` exits 0–3; invalid deactivation pin exits 126). +- **Exit code** — stage-scoped: input 20–29, bootstrap 30–39, + listener 40–49, service 50–59. Unrelated existing contracts are + unchanged (`ctrld-client status` exits 0–3; invalid deactivation pin exits 126). A customer or administrator only needs to report the code (or the whole output line). The table below is the maintained support mapping; it must @@ -26,14 +26,23 @@ stay in sync with `cmd/cli/provision_result.go` and changes in the same MR. | Code | Stage | Exit | Failure scenario | Next action / evidence | |---|---|---|---|---| +| `PROVISION_TOKEN_MALFORMED` | input | 21 | The `--cd-org` value is clearly not a provisioning code: explicitly empty, too short, too long, or containing whitespace or control characters. Checked before any network call. A missing `org-v1-` prefix is not this code — that only logs a warning, since legacy codes may lack it. | Check the provisioning code was copied in full, with no extra whitespace. The result file never echoes the value. | +| `CUSTOM_HOSTNAME_INVALID` | input | 22 | The `--custom-hostname` value fails ctrld's own hostname rule (`validHostname`): not 3–64 characters, or not RFC1123 hostname format. Checked before any network call. The API itself does not reject a bad hostname during provisioning — ControlD folds/strips characters like space, `+`, and `.` when it registers the device name, so a value ctrld accepts may still register under an adjusted name (logged as a notice, not a failure). | Fix `--custom-hostname` per the message: it names the offending character(s) and the allowed format. | +| `INTERCEPT_MODE_INVALID` | input | 23 | The `--intercept-mode` value is not one of `off`, `dns`, or `hard`. Checked before installing the service. | Re-run with a valid `--intercept-mode` value. | +| `INVALID_FLAG_COMBINATION` | input | 24 | `--cd`/`--cd-org` used together with `--nextdns`, or `--proto` set to anything other than `doh`/`doh3` once `--cd` is in play. Checked before any network call or service install. | Re-run without the conflicting flag, or fix the invalid value. The message names the exact flags involved. | | `API_UNREACHABLE` | bootstrap | 30 | The Control D API could not be reached or answered with a retryable error (network failure, proxy interference, 5xx, timeout) and retries ran out. The service manager may retry the service later. | Check the device's network path to `api.controld.com` (DNS, proxy, firewall, captive portal). Ask for the result file's `message` and whether other TLS traffic works. | | `API_REJECTED` | bootstrap | 31 | The API answered and permanently rejected the configuration (4xx other than 408/429): bad or revoked token, malformed request. ctrld exits without burning service-manager restarts because retrying cannot change the answer. | Verify the provision token / org configuration in the Control D dashboard. Re-push after fixing credentials. Evidence: HTTP status in the result file `message`. | -| `API_DEVICE_INVALID` | bootstrap | 32 | The API reports the device/resolver no longer exists (error code 40402). ctrld self-uninstalls its service because the identity is gone server-side. | Confirm the device was deleted or re-provisioned in the dashboard; re-provision with a current token. No local evidence needed beyond the code. | -| `LISTENER_BIND_FAILED` | listener | 41 | No listen address could be bound after all fallbacks (configured address, 0.0.0.0:53, localhost:53, port 5354, random) were exhausted. `detail.attempts` records each tried address with the UDP/TCP OS error, e.g. `address already in use` (another DNS service owns the port) or `can't assign requested address` (address not on any interface). | Read `detail.attempts`: `address already in use` → find the process owning the port (`sudo lsof -i :53 -nP`); `can't assign requested address` → the configured IP is not present on the device. Then fix the conflict or the listener config. | +| `API_DEVICE_INVALID` | bootstrap | 32 | The API reports the device/resolver no longer exists (error code 40402). When the daemon's own bootstrap preflight discovers this, it self-uninstalls because the identity is gone server-side. The direct `--cd ` install path (`ctrld-client start --cd`) can also hit this code, before the service exists; there is nothing to uninstall yet on that path. | Confirm the device was deleted or re-provisioned in the dashboard; re-provision with a current token. No local evidence needed beyond the code. | +| `TOKEN_INVALID` | bootstrap | 33 | The API rejected the `--cd-org` provisioning code with reason `token_invalid`: the code is not recognized. | Check the code and re-enter it exactly as given. | +| `TOKEN_EXPIRED` | bootstrap | 34 | The API rejected the `--cd-org` provisioning code with reason `token_expired`. | Get a new provisioning code from your administrator. | +| `TOKEN_LIMIT_REACHED` | bootstrap | 35 | The API rejected the `--cd-org` provisioning code with reason `token_limit_reached`: it has reached its device limit. | Free up a device slot or use a different provisioning code. | +| `TOKEN_DISABLED` | bootstrap | 36 | The API rejected the `--cd-org` provisioning code with reason `token_disabled`: the code was invalidated. | Download a profile from an active provisioning code. | +| `LISTENER_BIND_FAILED` | listener | 41 | No listen address could be bound after all fallbacks (configured address, 0.0.0.0:53, localhost:53, random) were exhausted. `detail.attempts` records each tried address with the UDP/TCP OS error, e.g. `address already in use` (another DNS service owns the port) or `can't assign requested address` (address not on any interface). | Read `detail.attempts`: `address already in use` → find the process owning the port (`sudo lsof -i :53 -nP`); `can't assign requested address` → the configured IP is not present on the device. Then fix the conflict or the listener config. | | `LISTENER_CONFIGURED_ADDR_UNAVAILABLE` | listener | 42 | An explicitly configured listener address could not be bound and configuration checks forbid falling back to another address, or (macOS intercept mode) the required explicit address is unavailable. | The configured `ip:port` in the listener config is wrong for this device or occupied. Verify the address exists on an interface and nothing else binds it; correct the config rather than expecting fallback. | | `SERVICE_INSTALL_FAILED` | service | 51 | The OS service manager refused to install the service (launchd/systemd/SCM registration failed). | Check OS-level constraints: permissions/elevation, MDM policy blocking daemon installation, corrupted previous install. Evidence: result file `message` (service manager error), plus `launchctl print system/ctrld-client` / `systemctl status ctrld-client` / SCM state. | | `SERVICE_START_FAILED` | service | 52 | The service installed but the service manager could not start it. | Check the service manager's own log for the start error, then the ctrld home dir `ctrld.log`. Often permissions or a binary quarantined by security tooling. | -| `SERVICE_SELFCHECK_FAILED` | service | 53 | The service started but never became healthy: no fresher failure was reported by the daemon, and the post-install DNS self-check failed. The just-installed service is rolled back (uninstalled). If the daemon itself recorded a more specific failure (e.g. a listener code), that code is reported instead of this one. | Ask for the drained service log printed by `ctrld-client start` and the result file. If the service was running but unreachable, check host firewall rules intercepting DNS to the listener. | +| `SERVICE_SELFCHECK_FAILED` | service | 53 | The service started but never became healthy: no fresher failure was reported by the daemon, and the post-install DNS self-check failed. On a fresh install or an upgrade, the just-installed service is rolled back (uninstalled). A restart of an already installed service keeps that service installed. If the daemon itself recorded a more specific failure (e.g. a listener code), that code is reported instead of this one. | Ask for the drained service log printed by `ctrld-client start` and the result file. If the service was running but unreachable, check host firewall rules intercepting DNS to the listener. | +| `UNCLASSIFIED` | service | 59 | A terminal failure on the provisioning boundary that predates a dedicated code: a config unmarshal, a file-system or environment failure (writing the config file, reading `socketDir`, respawning as a daemon), a service-argument update failing mid-upgrade, or a DNS-intercept failure the interface-DNS fallback cannot safely take over from. Every one of these used to be a bare crash with nothing to read; now they all persist a result file. | Read the result file `message`: it names the specific operation that failed and the underlying OS error. Treat this the same as any other stage-scoped failure when reporting it. | ## Reading the result file diff --git a/internal/controld/config.go b/internal/controld/config.go index 2e0cd14..dbda427 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -37,6 +37,16 @@ const ( sendLogTimeout = 300 * time.Second ) +// Provisioning-token rejection reasons the API sends in error.metadata.reason +// (HTTP 400, code 40003). This list can grow; a value outside it is not an +// error, just one cmd/cli does not classify yet. +const ( + ReasonTokenInvalid = "token_invalid" + ReasonTokenExpired = "token_expired" + ReasonTokenLimitReached = "token_limit_reached" + ReasonTokenDisabled = "token_disabled" +) + // ResolverConfig represents Control D resolver data. type ResolverConfig struct { DOH string `json:"doh"` @@ -64,10 +74,41 @@ type utilityResponse struct { } `json:"body"` } +// errorMetadata carries additive, optional detail on top of Code/Message. +// Older API deployments omit it, so it must decode to its zero value rather +// than fail the whole response. Its custom UnmarshalJSON gives the same +// tolerance to a malformed value: a metadata that is not an object, or a +// Reason that is not a string (a number, an object, or null), degrades to +// the zero value rather than failing the response that contains it. +type errorMetadata struct { + // Reason is a machine-readable rejection reason sent on provisioning-token + // errors (HTTP 400, code 40003): token_invalid, token_expired, + // token_limit_reached, or token_disabled. Empty when absent or malformed; + // callers must treat any other value as unknown rather than reject the + // response. + Reason string `json:"reason"` +} + +func (m *errorMetadata) UnmarshalJSON(data []byte) error { + var raw struct { + Reason json.RawMessage `json:"reason"` + } + // Best-effort: a metadata that is not an object, or a reason that is not + // a string (number, object, null), leaves the zero value instead of + // failing this decode. Code and Message still classify the failure. + if err := json.Unmarshal(data, &raw); err != nil { + *m = errorMetadata{} + return nil + } + _ = json.Unmarshal(raw.Reason, &m.Reason) + return nil +} + type ErrorResponse struct { ErrorField struct { - Message string `json:"message"` - Code int `json:"code"` + Message string `json:"message"` + Code int `json:"code"` + Metadata errorMetadata `json:"metadata"` } `json:"error"` // StatusCode is the HTTP status the API answered with. It is not part of the JSON // body: this type is built for *any* non-200 whose body decodes, so the body alone diff --git a/internal/controld/config_test.go b/internal/controld/config_test.go index 2b3d6d5..04cb132 100644 --- a/internal/controld/config_test.go +++ b/internal/controld/config_test.go @@ -98,6 +98,116 @@ func TestAPIErrorRecordsHTTPStatus(t *testing.T) { }) } +// TestAPIErrorDecodesRejectionReason pins the additive metadata.reason field the +// API sends on provisioning-token rejections. cmd/cli maps known reasons to their +// own failure codes, and must fall back cleanly when the field is absent or holds +// a value this build does not recognize yet. +func TestAPIErrorDecodesRejectionReason(t *testing.T) { + tests := []struct { + name string + body string + wantReason string + }{ + { + name: "known reason", + body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":"token_disabled"}}}`, + wantReason: "token_disabled", + }, + { + name: "reason absent", + body: `{"error":{"message":"invalid token","code":40003}}`, + wantReason: "", + }, + { + name: "unknown reason value", + body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":"something_new"}}}`, + wantReason: "something_new", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := json.NewDecoder(strings.NewReader(tc.body)) + errResp, err := apiErrorFromResponse(http.StatusBadRequest, d) + if err != nil { + t.Fatalf("unexpected decode error: %v", err) + } + if errResp.ErrorField.Metadata.Reason != tc.wantReason { + t.Errorf("reason = %q, want %q", errResp.ErrorField.Metadata.Reason, tc.wantReason) + } + }) + } +} + +// TestAPIErrorToleratesMalformedRejectionReason pins the fix for a decode error +// confined to metadata.reason: a reason sent as the wrong JSON type must not +// discard the rest of the response. Before this fix, apiErrorFromResponse +// returned the raw decode error and nothing else, which cmd/cli's +// apiFailureCode cannot recognize as an *ErrorResponse - it falls back to +// API_UNREACHABLE (a retryable bootstrap failure) instead of the permanent +// rejection the HTTP status and code actually describe. +func TestAPIErrorToleratesMalformedRejectionReason(t *testing.T) { + tests := []struct { + name string + body string + }{ + {name: "reason as a number", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":12345}}}`}, + {name: "reason as an object", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":{"inner":"value"}}}}`}, + {name: "reason as null", body: `{"error":{"message":"invalid token","code":40003,"metadata":{"reason":null}}}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := json.NewDecoder(strings.NewReader(tc.body)) + errResp, err := apiErrorFromResponse(http.StatusBadRequest, d) + if err != nil { + t.Fatalf("a malformed reason must not fail the whole decode: %v", err) + } + if errResp.ErrorField.Message != "invalid token" { + t.Errorf("message = %q, want it to survive the malformed reason", errResp.ErrorField.Message) + } + if errResp.ErrorField.Code != 40003 { + t.Errorf("code = %d, want it to survive the malformed reason", errResp.ErrorField.Code) + } + if errResp.ErrorField.Metadata.Reason != "" { + t.Errorf("reason = %q, want empty for a malformed value", errResp.ErrorField.Metadata.Reason) + } + }) + } +} + +// TestAPIErrorToleratesMalformedMetadata pins the same tolerance one level +// up: a metadata field that is not a JSON object must not discard the rest +// of the response. Code and Message still classify the failure, and Reason +// stays empty. +func TestAPIErrorToleratesMalformedMetadata(t *testing.T) { + tests := []struct { + name string + body string + }{ + {name: "metadata as a string", body: `{"error":{"message":"invalid token","code":40003,"metadata":"foo"}}`}, + {name: "metadata as a number", body: `{"error":{"message":"invalid token","code":40003,"metadata":7}}`}, + {name: "metadata as an array", body: `{"error":{"message":"invalid token","code":40003,"metadata":["reason"]}}`}, + {name: "metadata as a boolean", body: `{"error":{"message":"invalid token","code":40003,"metadata":true}}`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + d := json.NewDecoder(strings.NewReader(tc.body)) + errResp, err := apiErrorFromResponse(http.StatusBadRequest, d) + if err != nil { + t.Fatalf("unexpected decode error: %v", err) + } + if errResp.ErrorField.Code != 40003 { + t.Errorf("code = %d, want 40003", errResp.ErrorField.Code) + } + if errResp.ErrorField.Message != "invalid token" { + t.Errorf("message = %q, want %q", errResp.ErrorField.Message, "invalid token") + } + if errResp.ErrorField.Metadata.Reason != "" { + t.Errorf("reason = %q, want empty for a malformed metadata", errResp.ErrorField.Metadata.Reason) + } + }) + } +} + // TestUtilityResponseDecodesDestinationIPs pins the API field that carries the // organization's effective Allowed Destination IP list. The list is enforced as a // set of Firewall Mode exceptions, so a silent decode change - a renamed field, a