From d5ac6c11d865d211bb68292432b8386d77b8652e Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:03:37 -0300 Subject: [PATCH 01/11] fix(agent): refuse a tenant id that is not a uuid A mistyped tenant reached the network and was retried forever, because the server answers it with the same not-found it uses for a namespace that does not exist yet. Validate at load instead, after the persisted tenant is adopted so a corrupted tenant file is refused the same way. --- agent/pkg/agentd/agent.go | 20 ++++++++------ agent/pkg/agentd/agent_test.go | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/agent/pkg/agentd/agent.go b/agent/pkg/agentd/agent.go index 21f954941e6..c69684acc75 100644 --- a/agent/pkg/agentd/agent.go +++ b/agent/pkg/agentd/agent.go @@ -93,7 +93,7 @@ type Config struct { // It is optional: when empty (and no tenant was persisted from a previous // pairing), the agent boots into pairing mode and waits for a user to // accept it into a namespace, learning the tenant from the server. - TenantID string `env:"TENANT_ID"` + TenantID string `env:"TENANT_ID" validate:"omitempty,uuid"` // PairingCode is a pre-authorized pairing code handed to the agent at install // time (minted from the console's Add Device page). When set and no tenant is @@ -167,7 +167,11 @@ func (c *Config) HasNamespaceCredential() bool { // LoadConfigFromEnv reads the agent's configuration from SHELLHUB_-prefixed environment // variables, falling back to the .env file next to the binary when one is present. // -// The second return value carries the environment as parsed, for callers that log it. +// A tenant persisted by a previous pairing is adopted before validation, so a malformed tenant is +// refused whether it came from the environment or from the file, rather than being carried into an +// authorization the server can only reject. +// +// The second return value carries the fields that failed validation, for callers that log them. func LoadConfigFromEnv() (*Config, map[string]any, error) { applyEnvFileFallback(defaultEnvFilePath) @@ -178,12 +182,6 @@ func LoadConfigFromEnv() (*Config, map[string]any, error) { return nil, nil, err } - if ok, fields, err := validator.New().StructWithFields(cfg); err != nil || !ok { - log.WithFields(fields).Error("failed to validate the configuration loaded from envs") - - return nil, fields, err - } - if persisted, err := ReadPersistedTenant(TenantFilePath(cfg.PrivateKey)); err == nil && persisted != "" { switch { case cfg.TenantID == "": @@ -196,6 +194,12 @@ func LoadConfigFromEnv() (*Config, map[string]any, error) { } } + if ok, fields, err := validator.New().StructWithFields(cfg); err != nil || !ok { + log.WithFields(fields).Error("failed to validate the configuration loaded from envs") + + return nil, fields, err + } + return cfg, nil, nil } diff --git a/agent/pkg/agentd/agent_test.go b/agent/pkg/agentd/agent_test.go index 9cb3db8cbba..c3adee28015 100644 --- a/agent/pkg/agentd/agent_test.go +++ b/agent/pkg/agentd/agent_test.go @@ -1,6 +1,7 @@ package agentd import ( + "path/filepath" "testing" "github.com/pkg/errors" @@ -108,6 +109,55 @@ func TestLoadConfigFromEnv(t *testing.T) { err: validator.ErrStructureInvalid, }, }, + { + description: "fail to load the environment variables when the tenant is not a uuid", + requiredMocks: func() { + envs := new(Config) + + envMock.On("Process", "SHELLHUB_", envs).Return(nil).Once().Run(func(args mock.Arguments) { + cfg, ok := args.Get(1).(*Config) + require.True(t, ok) + + cfg.ServerAddress = "http://localhost" + cfg.TenantID = "1c462afa-e4b6-41a5-ba54-7236a177O466" + cfg.PrivateKey = "/tmp/shellhub.key" + cfg.MaxRetryConnectionTimeout = 30 + }) + }, + expected: expected{ + cfg: nil, + fields: map[string]any{ + "TenantID": "uuid", + }, + err: validator.ErrStructureInvalid, + }, + }, + { + description: "fail to load the environment variables when the persisted tenant is not a uuid", + requiredMocks: func() { + key := filepath.Join(t.TempDir(), "shellhub.key") + require.NoError(t, PersistTenant(TenantFilePath(key), "not-a-uuid")) + + envs := new(Config) + + envMock.On("Process", "SHELLHUB_", envs).Return(nil).Once().Run(func(args mock.Arguments) { + cfg, ok := args.Get(1).(*Config) + require.True(t, ok) + + cfg.ServerAddress = "http://localhost" + cfg.TenantID = "" + cfg.PrivateKey = key + cfg.MaxRetryConnectionTimeout = 30 + }) + }, + expected: expected{ + cfg: nil, + fields: map[string]any{ + "TenantID": "uuid", + }, + err: validator.ErrStructureInvalid, + }, + }, { description: "success to load the environmental variables", requiredMocks: func() { From 105eee9ccc3ceed46fadd717b6deedfaaf051e71 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:04:44 -0300 Subject: [PATCH 02/11] feat(agent): record where the tenant came from Adopting a persisted tenant assigned it into the field the environment uses, so nothing downstream could tell an operator's tenant from one the agent wrote. Any recovery that clears a stale tenant has to, or a wrong server address would destroy a valid enrollment. Recorded only; nothing branches on it yet. --- agent/pkg/agentd/agent.go | 14 ++++++-- agent/pkg/agentd/agent_test.go | 59 ++++++++++++++++++++++++++++++++++ agent/pkg/agentd/tenant.go | 18 +++++++++++ 3 files changed, 89 insertions(+), 2 deletions(-) diff --git a/agent/pkg/agentd/agent.go b/agent/pkg/agentd/agent.go index c69684acc75..57eaaf517f1 100644 --- a/agent/pkg/agentd/agent.go +++ b/agent/pkg/agentd/agent.go @@ -95,6 +95,10 @@ type Config struct { // accept it into a namespace, learning the tenant from the server. TenantID string `env:"TENANT_ID" validate:"omitempty,uuid"` + // TenantOrigin records where TenantID came from. It is not read from the environment; + // [LoadConfigFromEnv] and [Agent.SetTenantID] set it as they resolve the tenant. + TenantOrigin TenantOrigin + // PairingCode is a pre-authorized pairing code handed to the agent at install // time (minted from the console's Add Device page). When set and no tenant is // configured, the agent claims it: the server accepts the device into the @@ -182,10 +186,15 @@ func LoadConfigFromEnv() (*Config, map[string]any, error) { return nil, nil, err } + if cfg.TenantID != "" { + cfg.TenantOrigin = TenantFromEnvironment + } + if persisted, err := ReadPersistedTenant(TenantFilePath(cfg.PrivateKey)); err == nil && persisted != "" { switch { case cfg.TenantID == "": cfg.TenantID = persisted + cfg.TenantOrigin = TenantFromFile case cfg.TenantID != persisted: log.WithFields(log.Fields{ "env_tenant": cfg.TenantID, @@ -367,10 +376,11 @@ func (a *Agent) Authorize() error { return nil } -// SetTenantID injects the tenant learned from a pairing so the agent can be -// authorized. +// SetTenantID injects the tenant learned from a pairing so the agent can be authorized, and +// attributes it to that pairing so a later recovery can tell it from a tenant an operator set. func (a *Agent) SetTenantID(tenant string) { a.config.TenantID = tenant + a.config.TenantOrigin = TenantFromPairing } // ClearPairingCode drops a pre-authorized pairing code after the server rejected diff --git a/agent/pkg/agentd/agent_test.go b/agent/pkg/agentd/agent_test.go index c3adee28015..afb5e4a68ac 100644 --- a/agent/pkg/agentd/agent_test.go +++ b/agent/pkg/agentd/agent_test.go @@ -177,6 +177,7 @@ func TestLoadConfigFromEnv(t *testing.T) { cfg: &Config{ ServerAddress: "http://localhost", TenantID: "1c462afa-e4b6-41a5-ba54-7236a1770466", + TenantOrigin: TenantFromEnvironment, PrivateKey: "/tmp/shellhub.key", MaxRetryConnectionTimeout: 30, }, @@ -576,3 +577,61 @@ func TestAgentAuthorizeRequiresANamespaceCredential(t *testing.T) { assert.Equal(t, ErrAuthorizeNoNamespaceCredential, agent.Authorize()) } + +func TestLoadConfigFromEnvRecordsTenantOrigin(t *testing.T) { + const tenant = "1c462afa-e4b6-41a5-ba54-7236a1770466" + + tests := []struct { + description string + envTenant string + fileTenant string + expected TenantOrigin + }{ + { + description: "no tenant anywhere leaves the origin unset", + expected: TenantFromNowhere, + }, + { + description: "a tenant from the environment is attributed to it", + envTenant: tenant, + expected: TenantFromEnvironment, + }, + { + description: "a tenant read from the file is attributed to the file", + fileTenant: tenant, + expected: TenantFromFile, + }, + { + description: "the environment wins, and keeps its own attribution", + envTenant: tenant, + fileTenant: "00000000-0000-4000-0000-000000000000", + expected: TenantFromEnvironment, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + envMock := new(env_mocks.MockBackend) + envs.DefaultBackend = envMock + + key := filepath.Join(t.TempDir(), "shellhub.key") + if test.fileTenant != "" { + require.NoError(t, PersistTenant(TenantFilePath(key), test.fileTenant)) + } + + envMock.On("Process", "SHELLHUB_", new(Config)).Return(nil).Once().Run(func(args mock.Arguments) { + cfg, ok := args.Get(1).(*Config) + require.True(t, ok) + + cfg.ServerAddress = "http://localhost" + cfg.TenantID = test.envTenant + cfg.PrivateKey = key + cfg.MaxRetryConnectionTimeout = 30 + }) + + cfg, _, err := LoadConfigFromEnv() + require.NoError(t, err) + assert.Equal(t, test.expected, cfg.TenantOrigin) + }) + } +} diff --git a/agent/pkg/agentd/tenant.go b/agent/pkg/agentd/tenant.go index 19310eb7138..f0c9245e656 100644 --- a/agent/pkg/agentd/tenant.go +++ b/agent/pkg/agentd/tenant.go @@ -6,6 +6,24 @@ import ( "strings" ) +// TenantOrigin names where the tenant a device enrolls with came from. A recovery that clears a +// stale tenant needs it: the file is the agent's to rewrite, the environment is the operator's and +// must be left alone. +type TenantOrigin string + +const ( + // TenantFromNowhere is the origin of a configuration carrying no tenant, which enrolls by + // pairing or with an install key instead. + TenantFromNowhere TenantOrigin = "" + // TenantFromEnvironment is the origin of a tenant an operator supplied to the agent. + TenantFromEnvironment TenantOrigin = "environment" + // TenantFromFile is the origin of a tenant a previous pairing persisted beside the private key. + TenantFromFile TenantOrigin = "file" + // TenantFromPairing is the origin of a tenant the server resolved during a pairing this process + // performed, before it is persisted. + TenantFromPairing TenantOrigin = "pairing" +) + // TenantFilePath returns the path where the agent persists the tenant learned // from a pairing. It is a sibling of the private key so it lands on the same // persistent mount, and suffixing the key name avoids collisions when multiple From 50c6d5886f924d87a4c16939dc9381ebb1b3de30 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:07:15 -0300 Subject: [PATCH 03/11] fix(pkg): keep reporting a refusal that does not change Severity decayed to debug after the first attempt, so an agent stuck on a namespace that no longer exists looked exactly like an idle one. An unreachable server resolves itself and can stay quiet; a refusal may never resolve, so raise it back to warn every ten attempts. --- pkg/api/client/connectivity_test.go | 37 +++++++++++++++++++++++++++ pkg/connectivity/connectivity.go | 19 ++++++++++++-- pkg/connectivity/connectivity_test.go | 15 +++++++++++ 3 files changed, 69 insertions(+), 2 deletions(-) diff --git a/pkg/api/client/connectivity_test.go b/pkg/api/client/connectivity_test.go index 2da4276cd1f..f17bafde421 100644 --- a/pkg/api/client/connectivity_test.go +++ b/pkg/api/client/connectivity_test.go @@ -243,3 +243,40 @@ func TestTheServerAnswerIsBoundedBeforeItReachesTheLog(t *testing.T) { assert.Less(t, len(reported.Error()), len(page)) assert.Contains(t, reported.Error(), "502") } + +func TestAPersistentRefusalKeepsBeingReported(t *testing.T) { + backend, hook := logtest.NewNullLogger() + backend.SetLevel(logrus.DebugLevel) + + cli, err := NewClient("https://www.cloud.shellhub.io/", withImmediateRetries(), WithLogger(backend)) + require.NoError(t, err) + + client, ok := cli.(*client) + require.True(t, ok) + + mock.ActivateNonDefault(client.http.GetClient()) + defer mock.DeactivateAndReset() + + attempts := 0 + accepted, _ := mock.NewJsonResponder(200, models.DeviceAuthResponse{Name: "83-18-77-25-78-0d"}) + mock.RegisterResponder("POST", "/api/devices/auth", func(r *http.Request) (*http.Response, error) { + attempts++ + if attempts > 25 { + return accepted(r) + } + + return mock.NewStringResponse(http.StatusNotFound, `{"message":"namespace not found"}`), nil + }) + + _, err = cli.AuthDevice(authRequest()) + require.NoError(t, err) + + surfaced := 0 + for _, entry := range hook.AllEntries() { + if entry.Level == logrus.WarnLevel && strings.Contains(entry.Message, "Cannot authorize the device") { + surfaced++ + } + } + + assert.Greater(t, surfaced, 1, "a refusal that keeps repeating must not decay to silence") +} diff --git a/pkg/connectivity/connectivity.go b/pkg/connectivity/connectivity.go index 58064e84236..c801b699e2b 100644 --- a/pkg/connectivity/connectivity.go +++ b/pkg/connectivity/connectivity.go @@ -44,14 +44,29 @@ func Recovered(logger logrus.FieldLogger, attempt int, elapsed time.Duration) { Info("Recovered after retrying") } +const refusalResurfaceEvery = 10 + +func refusalLevel(attempt int) logrus.Level { + if attempt%refusalResurfaceEvery == 1 { + return logrus.WarnLevel + } + + return logrus.DebugLevel +} + // Refused reports one attempt the server answered by refusing to authorize the device. It reads as // a distinct condition from an unreachable server because it is: the server is up, and what has to -// change is the namespace or the device limit. Levels follow Lost. +// change is the namespace or the device limit. +// +// Unlike Lost it does not fall silent after the first attempt. An unreachable server resolves +// itself; a refusal may name a namespace that will never exist, and an operator reading the log +// after the fact needs it to still be saying so. It is raised back to warn every tenth attempt and +// logged at debug in between. func Refused(logger logrus.FieldLogger, attempt int, err error) { logger. WithError(err). WithField("attempt", attempt). - Log(level(attempt), "Cannot authorize the device, retrying until the server accepts it") + Log(refusalLevel(attempt), "Cannot authorize the device, retrying until the server accepts it") } // Tracker counts consecutive failures for a caller whose retry loop has no attempt counter to pass diff --git a/pkg/connectivity/connectivity_test.go b/pkg/connectivity/connectivity_test.go index db01fdee89c..58fb2ff55e5 100644 --- a/pkg/connectivity/connectivity_test.go +++ b/pkg/connectivity/connectivity_test.go @@ -71,6 +71,21 @@ func TestRefused(t *testing.T) { attempt: 9, expected: logrus.DebugLevel, }, + { + description: "stays at debug through the end of the window", + attempt: 10, + expected: logrus.DebugLevel, + }, + { + description: "surfaces again when the window turns over", + attempt: 11, + expected: logrus.WarnLevel, + }, + { + description: "keeps surfacing however long the refusal lasts", + attempt: 101, + expected: logrus.WarnLevel, + }, } for _, test := range tests { From 3d7643487fb2f51e1f16557eb2d9f754b7a0d79e Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:09:08 -0300 Subject: [PATCH 04/11] fix(agent): name the credential a refused authorization was for The failure read "Failed to initialize agent" and dumped the whole configuration, which named nothing useful and put the install key in the log. Say which credential was refused and where it came from, and log the tenant and its origin instead of the struct. --- agent/main.go | 8 +++-- agent/pkg/agentd/agent.go | 21 +++++++++++- agent/pkg/agentd/agent_test.go | 60 ++++++++++++++++++++++++++++++++++ tests/install_key_test.go | 2 +- 4 files changed, 86 insertions(+), 5 deletions(-) diff --git a/agent/main.go b/agent/main.go index 023c7b3379b..8c58c2294f1 100644 --- a/agent/main.go +++ b/agent/main.go @@ -113,9 +113,11 @@ func main() { if err := ag.Authorize(); err != nil { log.WithError(err).WithFields(log.Fields{ - "version": AgentVersion, - "configuration": cfg, - }).Fatal("Failed to initialize agent") + "version": AgentVersion, + "server_address": cfg.ServerAddress, + "tenant_id": cfg.TenantID, + "tenant_origin": cfg.TenantOrigin, + }).Fatal("Failed to authorize the device") } ctx := cmd.Context() diff --git a/agent/pkg/agentd/agent.go b/agent/pkg/agentd/agent.go index 57eaaf517f1..74b1b2d311b 100644 --- a/agent/pkg/agentd/agent.go +++ b/agent/pkg/agentd/agent.go @@ -168,6 +168,25 @@ func (c *Config) HasNamespaceCredential() bool { return c.TenantID != "" || c.InstallKey != "" } +func (c *Config) credential() string { + if c.TenantID == "" && c.InstallKey != "" { + return "the install key" + } + + switch c.TenantOrigin { + case TenantFromEnvironment: + return fmt.Sprintf("the tenant %s from SHELLHUB_TENANT_ID", c.TenantID) + case TenantFromFile: + return fmt.Sprintf("the tenant %s persisted at %s", c.TenantID, TenantFilePath(c.PrivateKey)) + case TenantFromPairing: + return fmt.Sprintf("the tenant %s learned from pairing", c.TenantID) + case TenantFromNowhere: + return "no namespace credential" + } + + return "the tenant " + c.TenantID +} + // LoadConfigFromEnv reads the agent's configuration from SHELLHUB_-prefixed environment // variables, falling back to the .env file next to the binary when one is present. // @@ -353,7 +372,7 @@ func (a *Agent) Authorize() error { } if err := a.authorize(); err != nil { - return errors.Wrap(err, "failed to authorize device") + return errors.Wrap(err, "failed to authorize device with "+a.config.credential()) } if a.config.TenantID == "" { diff --git a/agent/pkg/agentd/agent_test.go b/agent/pkg/agentd/agent_test.go index afb5e4a68ac..21c0776eb33 100644 --- a/agent/pkg/agentd/agent_test.go +++ b/agent/pkg/agentd/agent_test.go @@ -1,6 +1,8 @@ package agentd import ( + "crypto/rand" + "crypto/rsa" "path/filepath" "testing" @@ -635,3 +637,61 @@ func TestLoadConfigFromEnvRecordsTenantOrigin(t *testing.T) { }) } } + +func TestAuthorizeNamesTheCredentialItWasRefusedFor(t *testing.T) { + refused := errors.New("namespace not found") + + tests := []struct { + description string + config *Config + expected string + }{ + { + description: "a tenant an operator supplied names the variable it came from", + config: &Config{ + TenantID: "1c462afa-e4b6-41a5-ba54-7236a1770466", + TenantOrigin: TenantFromEnvironment, + }, + expected: "SHELLHUB_TENANT_ID", + }, + { + description: "a tenant left by a previous pairing names the file holding it", + config: &Config{ + TenantID: "1c462afa-e4b6-41a5-ba54-7236a1770466", + TenantOrigin: TenantFromFile, + PrivateKey: "/etc/shellhub.key", + }, + expected: "/etc/shellhub.key.tenant", + }, + { + description: "an install key is named rather than the tenant it would have resolved", + config: &Config{ + InstallKey: "a-key", + }, + expected: "install key", + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + cli := new(client_mocks.MockClient) + cli.On("AuthDevice", mock.Anything).Return(nil, refused).Once() + + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + agent := &Agent{ + cli: cli, + config: test.config, + pubKey: &key.PublicKey, + Info: new(models.DeviceInfo), + Identity: &models.DeviceIdentity{MAC: "83:18:77:25:78:0d"}, + } + + err = agent.Authorize() + require.Error(t, err) + assert.Contains(t, err.Error(), test.expected) + assert.ErrorIs(t, err, refused) + }) + } +} diff --git a/tests/install_key_test.go b/tests/install_key_test.go index 74eef98fdcd..b47fba8f576 100644 --- a/tests/install_key_test.go +++ b/tests/install_key_test.go @@ -69,7 +69,7 @@ func TestInstallKeyEnrollment(t *testing.T) { agent := startAgent(t, ctx, compose, NewAgentContainerWithInstallKey(unissuedInstallKey)) - environment.AwaitLogContains(t, agent, `error="failed to authorize device: bad request"`) + environment.AwaitLogContains(t, agent, `error="failed to authorize device with the install key: bad request"`) require.EventuallyWithT(t, func(tt *assert.CollectT) { state, err := agent.State(ctx) From d91c9aafa3f470f3b54dac062c0d345f3ffc3956 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:11:24 -0300 Subject: [PATCH 05/11] fix(install): report a tenant a previous enrollment left behind The summary read only the variables given to this run, so a machine wedged on a stale tenant was told its enrollment was none, which is the opposite of what was about to happen. A credential passed to this run still wins. --- install.bats | 40 ++++++++++++++++++++++++++++++++++++++++ install.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/install.bats b/install.bats index 909316a05bc..6e913bdedbf 100644 --- a/install.bats +++ b/install.bats @@ -150,6 +150,34 @@ enter_wsl() { [ "$output" = "none — enroll with 'shellhub-agent login'" ] } +@test "enrollment_summary reports a tenant a previous enrollment left behind" { + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$PRIVATE_KEY.tenant" + + call_install enrollment_summary + + [ "$output" = "tenant 00000000-0000-4000-0000-000000000000 (persisted by a previous enrollment)" ] +} + +@test "enrollment_summary prefers a tenant given to this run over the persisted one" { + with_tenant + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$PRIVATE_KEY.tenant" + + call_install enrollment_summary + + [ "$output" = "tenant $TENANT_ID (device lands pending)" ] +} + +@test "enrollment_summary reads the tenant file a container install names under /host" { + export PRIVATE_KEY="/host$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$BATS_TEST_TMPDIR/shellhub.key.tenant" + + call_install enrollment_summary + + [ "$output" = "tenant 00000000-0000-4000-0000-000000000000 (persisted by a previous enrollment)" ] +} + @test "enroll_agent_interactively runs the login flow when no credential names a namespace" { stub_bin shellhub-agent @@ -211,6 +239,18 @@ enter_wsl() { assert_output_contains "install key's namespace" } +@test "enroll_agent_interactively skips the login flow for a persisted tenant" { + echo "00000000-0000-4000-0000-000000000000" > "$AGENT_KEY.tenant" + export PRIVATE_KEY="$AGENT_KEY" + stub_bin shellhub-agent + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" + + [ "$status" -eq 0 ] + refute_called "shellhub-agent" + assert_output_contains "remembered at $AGENT_KEY.tenant" +} + @test "enroll_agent_interactively skips the login flow for a tenant" { with_tenant stub_bin shellhub-agent diff --git a/install.sh b/install.sh index 6da41b71545..5fdf73a834c 100755 --- a/install.sh +++ b/install.sh @@ -65,16 +65,34 @@ EOF echo "✅ Installed shellhub-agent wrapper at $WRAPPER_PATH." } +tenant_file() { + _KEY="${PRIVATE_KEY:-/etc/shellhub.key}" + + echo "${_KEY#/host}.tenant" +} + +persisted_tenant() { + _TENANT_FILE=$(tenant_file) + + [ -r "$_TENANT_FILE" ] || return 0 + + head -n 1 "$_TENANT_FILE" | tr -d ' \t\r\n' +} + # Names the credential that will put this device in a namespace, in the same order # enroll_agent_interactively picks one. Reported before installing so a wrong or missing credential # is visible then, rather than only in the agent's log once it is already running. enrollment_summary() { + _PERSISTED=$(persisted_tenant) + if [ -n "$CODE" ]; then echo "pairing code (pre-authorized)" elif [ -n "$INSTALL_KEY" ]; then echo "install key" elif [ -n "$TENANT_ID" ]; then echo "tenant $TENANT_ID (device lands pending)" + elif [ -n "$_PERSISTED" ]; then + echo "tenant $_PERSISTED (persisted by a previous enrollment)" else echo "none — enroll with 'shellhub-agent login'" fi @@ -117,6 +135,16 @@ enroll_agent_interactively() { return 0 fi + _PERSISTED=$(persisted_tenant) + + if [ -n "$_PERSISTED" ]; then + echo "" + echo "The device will enroll into tenant $_PERSISTED, remembered at $(tenant_file)." + echo "Delete that file to enroll it somewhere else." + + return 0 + fi + # Wait for the agent to generate its key so the login flow reuses it instead # of racing the daemon to create one. _i=0 From cb352079bf5862858e26a32d587546f3d1086937 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:11:39 -0300 Subject: [PATCH 06/11] docs(install): correct what an empty TENANT_ID does It does not override a persisted tenant. The guard above never passes an empty value, and the agent treats absent and blank the same, adopting the persisted tenant either way. --- install.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/install.sh b/install.sh index 5fdf73a834c..6acabc93a9a 100755 --- a/install.sh +++ b/install.sh @@ -192,8 +192,8 @@ podman_install() { [ -n "${PREFERRED_IDENTITY}" ] && ARGS="$ARGS -e SHELLHUB_PREFERRED_IDENTITY=$PREFERRED_IDENTITY" [ -n "${CODE}" ] && ARGS="$ARGS -e SHELLHUB_PAIRING_CODE=$CODE" [ -n "${INSTALL_KEY}" ] && ARGS="$ARGS -e SHELLHUB_INSTALL_KEY=$INSTALL_KEY" - # An empty assignment is not the same as an absent one: the agent reads the variable as set and - # blank, which overrides a tenant it had persisted from an earlier enrollment. + # Passing the variable empty would not clear a persisted tenant: the agent reads absent and blank + # alike as no tenant, and adopts the persisted one in both cases. [ -n "${TENANT_ID}" ] && ARGS="$ARGS -e SHELLHUB_TENANT_ID=$TENANT_ID" if [ -n "$AGENT_IMAGE_OVERRIDDEN" ]; then @@ -280,8 +280,8 @@ docker_install() { [ -n "${PREFERRED_IDENTITY}" ] && ARGS="$ARGS -e SHELLHUB_PREFERRED_IDENTITY=$PREFERRED_IDENTITY" [ -n "${CODE}" ] && ARGS="$ARGS -e SHELLHUB_PAIRING_CODE=$CODE" [ -n "${INSTALL_KEY}" ] && ARGS="$ARGS -e SHELLHUB_INSTALL_KEY=$INSTALL_KEY" - # An empty assignment is not the same as an absent one: the agent reads the variable as set and - # blank, which overrides a tenant it had persisted from an earlier enrollment. + # Passing the variable empty would not clear a persisted tenant: the agent reads absent and blank + # alike as no tenant, and adopts the persisted one in both cases. [ -n "${TENANT_ID}" ] && ARGS="$ARGS -e SHELLHUB_TENANT_ID=$TENANT_ID" if [ -n "$AGENT_IMAGE_OVERRIDDEN" ]; then From 3d3d9dd4e8ebd68d6d6f609358f00e6ae19de068 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:13:02 -0300 Subject: [PATCH 07/11] fix(install): name the tenant file uninstall leaves behind Uninstall named the private key but not the tenant beside it, so an operator who cleaned up by hand cleaned the wrong file and the reinstall enrolled into the same namespace. Reported, not removed: it is the operator's to keep. --- install.bats | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++ install.sh | 15 ++++++++++++--- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/install.bats b/install.bats index 6e913bdedbf..c2ef300aabd 100644 --- a/install.bats +++ b/install.bats @@ -736,6 +736,57 @@ enter_wsl() { assert_output_contains "not found (may already be removed)" } +@test "uninstall names the tenant file it leaves behind" { + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$PRIVATE_KEY.tenant" + stub_bin docker + + call_install docker_uninstall + + assert_output_contains "$PRIVATE_KEY.tenant" +} + +@test "uninstall names the tenant file a container install left behind under /host" { + export PRIVATE_KEY="/host$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$BATS_TEST_TMPDIR/shellhub.key.tenant" + stub_bin docker + + call_install docker_uninstall + + assert_output_contains "$BATS_TEST_TMPDIR/shellhub.key.tenant" + [[ "$output" != *"/host$BATS_TEST_TMPDIR"* ]] +} + +@test "podman_uninstall names the tenant file it leaves behind" { + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$PRIVATE_KEY.tenant" + stub_bin podman + + call_install podman_uninstall + + assert_output_contains "$PRIVATE_KEY.tenant" +} + +@test "standalone_uninstall names the tenant file it leaves behind" { + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + echo "00000000-0000-4000-0000-000000000000" > "$PRIVATE_KEY.tenant" + fake_agent_binary + cp "$AGENT_BINARY" "$INSTALL_DIR/shellhub-agent" + + call_install standalone_uninstall + + assert_output_contains "$PRIVATE_KEY.tenant" +} + +@test "uninstall stays quiet about a tenant file that is not there" { + export PRIVATE_KEY="$BATS_TEST_TMPDIR/shellhub.key" + stub_bin docker + + call_install docker_uninstall + + [[ "$output" != *".tenant"* ]] +} + @test "standalone_uninstall reports a missing binary" { call_install standalone_uninstall diff --git a/install.sh b/install.sh index 6acabc93a9a..c1e4054d5dc 100755 --- a/install.sh +++ b/install.sh @@ -467,6 +467,15 @@ standalone_install() { rm -rf "$TMP_DIR" } +report_files_left_behind() { + echo "ℹ️ The private key file was left in place. Remove it manually if no longer needed." + + [ -n "$(persisted_tenant)" ] || return 0 + + echo "ℹ️ The namespace this device enrolled into is remembered in $(tenant_file)." + echo " Remove it too, or a reinstall will enroll into the same namespace." +} + docker_uninstall() { _FSUDO="" [ "$(id -u)" -ne 0 ] && _FSUDO="sudo" @@ -485,7 +494,7 @@ docker_uninstall() { fi echo "✅ ShellHub agent uninstalled." - echo "ℹ️ The private key file was left in place. Remove it manually if no longer needed." + report_files_left_behind } podman_uninstall() { @@ -506,7 +515,7 @@ podman_uninstall() { fi echo "✅ ShellHub agent uninstalled." - echo "ℹ️ The private key file was left in place. Remove it manually if no longer needed." + report_files_left_behind } standalone_uninstall() { @@ -532,7 +541,7 @@ standalone_uninstall() { $SUDO rm -f "$INSTALL_BIN" echo "✅ ShellHub agent uninstalled." - echo "ℹ️ The private key file was left in place. Remove it manually if no longer needed." + report_files_left_behind } wsl_install() { From 8fbfcbb97ef48216a6515cd20af80095c9963092 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:18:05 -0300 Subject: [PATCH 08/11] fix(install): dispatch uninstall before any install work Uninstall ran the version lookup, arch detection and settings summary first, so it reported install findings and hit the network for nothing. Detection and both dispatches move into functions so uninstall can run detection alone. --- install.bats | 23 +++++ install.sh | 253 +++++++++++++++++++++++++++------------------------ 2 files changed, 158 insertions(+), 118 deletions(-) diff --git a/install.bats b/install.bats index c2ef300aabd..48a3b6ac4e6 100644 --- a/install.bats +++ b/install.bats @@ -794,6 +794,15 @@ enter_wsl() { assert_output_contains "ShellHub agent binary not found" } +@test "standalone_uninstall looks in the default install dir when the environment names none" { + unset INSTALL_DIR + + call_install standalone_uninstall + + [ "$status" -eq 1 ] + assert_output_contains "/usr/local/bin/shellhub-agent" +} + @test "standalone_uninstall stops the service and removes the binary" { with_tenant fake_agent_binary @@ -1005,6 +1014,20 @@ enter_wsl() { assert_called "docker rm -f shellhub" } +@test "uninstall does no installer work before removing the agent" { + stub_bin docker + stub_bin curl 'echo "curl $*" >> "$CALLS"' + stub_bin wget 'echo "wget $*" >> "$CALLS"' + + run_install uninstall + + [ "$status" -eq 0 ] + refute_called "curl" + refute_called "wget" + [[ "$output" != *"Detected settings"* ]] + [[ "$output" != *"ShellHub Agent Installer"* ]] +} + @test "uninstall is refused for install methods that do not support it" { export INSTALL_METHOD=snap diff --git a/install.sh b/install.sh index c1e4054d5dc..d37536dcc90 100755 --- a/install.sh +++ b/install.sh @@ -524,7 +524,7 @@ standalone_uninstall() { SUDO="sudo" fi - INSTALL_BIN="$INSTALL_DIR/shellhub-agent" + INSTALL_BIN="${INSTALL_DIR:-/usr/local/bin}/shellhub-agent" if [ ! -f "$INSTALL_BIN" ]; then echo "❌ ShellHub agent binary not found at $INSTALL_BIN." @@ -585,73 +585,7 @@ http_get() { fi } -main() { - if [ "$(uname -s)" = "FreeBSD" ]; then - echo "👹 This system is running FreeBSD." - echo "❌ ERROR: Automatic installation is not supported on FreeBSD." - echo - echo "Please refer to the ShellHub port at https://github.com/shellhub-io/ports" - exit 1 - fi - - # TENANT_ID is optional wherever something else names the namespace: an install key does so on its - # own, a pairing code claims one, and with neither the container methods boot into pairing and - # enroll via 'shellhub-agent login'. Snap always requires it (checked in its function). - - SERVER_ADDRESS="${SERVER_ADDRESS:-https://cloud.shellhub.io}" - TENANT_ID="${TENANT_ID}" - INSTALL_METHOD="$INSTALL_METHOD" - AGENT_VERSION="${AGENT_VERSION:-$(http_get $SERVER_ADDRESS/info | sed -E 's/.*"version":\s?"?([^,"]*)"?.*/\1/')}" - [ -n "$AGENT_IMAGE" ] && AGENT_IMAGE_OVERRIDDEN="1" - AGENT_IMAGE="${AGENT_IMAGE:-docker.io/shellhubio/agent:$AGENT_VERSION}" - BINARY_ARCH="$BINARY_ARCH" - INSTALL_DIR="${INSTALL_DIR:-/usr/local/bin}" - TMP_DIR="${TMP_DIR:-$(mktemp -d -t shellhub-installer-XXXXXX)}" - - # Auto detect arch if it has not already been set - if [ -z "$BINARY_ARCH" ]; then - case $(uname -m) in - x86_64) - BINARY_ARCH=amd64 - ;; - armv6l) - BINARY_ARCH=armv6 - ;; - armv7l) - BINARY_ARCH=armv7 - ;; - aarch64) - BINARY_ARCH=arm64 - ;; - i386|i486|i586|i686) - BINARY_ARCH=386 - ;; - esac - fi - - echo "🛠️ ShellHub Agent Installer" - echo - if [ -z "$INSTALL_METHOD" ]; then - echo "This script will install the ShellHub agent on your system." - echo "It will auto-detect the best available installation method." - echo - echo "Installation methods (priority order):" - echo " 1. Docker - If Docker is installed and accessible in rootful mode" - echo " 2. Podman - If Podman is installed and accessible in rootful mode" - echo " 3. Snap - If Snap package manager is available" - echo " 4. WSL - If running in WSL2 with systemd and mirrored networking" - echo " 5. Standalone - Native binary with systemd" - echo - fi - - echo "⚙️ Detected settings:" - echo "- Server address: $SERVER_ADDRESS" - echo "- Enrollment: $(enrollment_summary)" - echo "- Agent version: $AGENT_VERSION" - echo "- Architecture: $BINARY_ARCH" - [ -n "$INSTALL_METHOD" ] && echo "- Install method: $INSTALL_METHOD" - echo - +detect_install_method() { if [ -z "$INSTALL_METHOD" ] && type docker >/dev/null 2>&1; then echo "🔍 Checking if Docker is available and accessible in rootful mode..." @@ -684,20 +618,13 @@ main() { [ -z "$INSTALL_METHOD" ] && echo "ℹ️ Podman is not accessible in rootful mode." fi - if [ -z "$INSTALL_METHOD" ]; then - echo - echo "⚠️ NOTE: No recommended installation method was detected." - echo "⚠️ For best performance, easier updates, and better isolation, it is strongly recommended to use Docker or Podman." - echo "ℹ️ The installer will proceed with an alternative method (Snap, Standalone, or WSL), but these may have limitations." - echo - fi + CONTAINER_RUNTIME_METHOD="$INSTALL_METHOD" if [ -z "$INSTALL_METHOD" ] && type snap >/dev/null 2>&1; then echo "🔍 Detected Snap package manager..." INSTALL_METHOD="snap" fi - # Check if running on WSL if grep -qi Microsoft "${PROC_VERSION:-/proc/version}"; then echo "🔍 Detected WSL environment..." @@ -719,56 +646,146 @@ main() { [ -z "$INSTALL_METHOD" ] && INSTALL_METHOD="standalone" - case "$1" in - uninstall) - case "$INSTALL_METHOD" in - standalone|wsl) - echo "🗑️ Uninstalling ShellHub using standalone method..." - standalone_uninstall - ;; - docker) - echo "🐳 Uninstalling ShellHub using docker method..." - docker_uninstall - ;; - podman) - echo "🐳 Uninstalling ShellHub using podman method..." - podman_uninstall - ;; - *) - echo "❌ Uninstall is not yet supported for '$INSTALL_METHOD' install method." - exit 1 - ;; - esac + return 0 +} + +recommend_container_runtime() { + [ -z "$CONTAINER_RUNTIME_METHOD" ] || return 0 + + echo + echo "⚠️ NOTE: No recommended installation method was detected." + echo "⚠️ For best performance, easier updates, and better isolation, it is strongly recommended to use Docker or Podman." + echo "ℹ️ The installer will proceed with an alternative method (Snap, Standalone, or WSL), but these may have limitations." + echo +} + +uninstall_agent() { + case "$INSTALL_METHOD" in + standalone|wsl) + echo "🗑️ Uninstalling ShellHub using standalone method..." + standalone_uninstall + ;; + docker) + echo "🐳 Uninstalling ShellHub using docker method..." + docker_uninstall + ;; + podman) + echo "🐳 Uninstalling ShellHub using podman method..." + podman_uninstall ;; *) - case "$INSTALL_METHOD" in - podman) - echo "🐳 Installing ShellHub using podman method..." - podman_install "$@" - ;; - docker) - echo "🐳 Installing ShellHub using docker method..." - docker_install "$@" + echo "❌ Uninstall is not yet supported for '$INSTALL_METHOD' install method." + exit 1 + ;; + esac +} + +install_agent() { + case "$INSTALL_METHOD" in + podman) + echo "🐳 Installing ShellHub using podman method..." + podman_install "$@" + ;; + docker) + echo "🐳 Installing ShellHub using docker method..." + docker_install "$@" + ;; + snap) + echo "📦 Installing ShellHub using snap method..." + snap_install + ;; + standalone) + echo "🐧 Installing ShellHub using standalone method..." + standalone_install + ;; + wsl) + echo "🪟 Installing ShellHub using WSL method..." + wsl_install + ;; + *) + echo "❌ Install method not supported." + exit 1 + ;; + esac +} + +main() { + if [ "$(uname -s)" = "FreeBSD" ]; then + echo "👹 This system is running FreeBSD." + echo "❌ ERROR: Automatic installation is not supported on FreeBSD." + echo + echo "Please refer to the ShellHub port at https://github.com/shellhub-io/ports" + exit 1 + fi + + if [ "$1" = "uninstall" ]; then + detect_install_method + uninstall_agent + + return + fi + + # TENANT_ID is optional wherever something else names the namespace: an install key does so on its + # own, a pairing code claims one, and with neither the container methods boot into pairing and + # enroll via 'shellhub-agent login'. Snap always requires it (checked in its function). + + SERVER_ADDRESS="${SERVER_ADDRESS:-https://cloud.shellhub.io}" + TENANT_ID="${TENANT_ID}" + INSTALL_METHOD="$INSTALL_METHOD" + AGENT_VERSION="${AGENT_VERSION:-$(http_get $SERVER_ADDRESS/info | sed -E 's/.*"version":\s?"?([^,"]*)"?.*/\1/')}" + [ -n "$AGENT_IMAGE" ] && AGENT_IMAGE_OVERRIDDEN="1" + AGENT_IMAGE="${AGENT_IMAGE:-docker.io/shellhubio/agent:$AGENT_VERSION}" + BINARY_ARCH="$BINARY_ARCH" + INSTALL_DIR="${INSTALL_DIR:-/usr/local/bin}" + TMP_DIR="${TMP_DIR:-$(mktemp -d -t shellhub-installer-XXXXXX)}" + + # Auto detect arch if it has not already been set + if [ -z "$BINARY_ARCH" ]; then + case $(uname -m) in + x86_64) + BINARY_ARCH=amd64 ;; - snap) - echo "📦 Installing ShellHub using snap method..." - snap_install + armv6l) + BINARY_ARCH=armv6 ;; - standalone) - echo "🐧 Installing ShellHub using standalone method..." - standalone_install + armv7l) + BINARY_ARCH=armv7 ;; - wsl) - echo "🪟 Installing ShellHub using WSL method..." - wsl_install + aarch64) + BINARY_ARCH=arm64 ;; - *) - echo "❌ Install method not supported." - exit 1 + i386|i486|i586|i686) + BINARY_ARCH=386 ;; esac - ;; - esac + fi + + echo "🛠️ ShellHub Agent Installer" + echo + if [ -z "$INSTALL_METHOD" ]; then + echo "This script will install the ShellHub agent on your system." + echo "It will auto-detect the best available installation method." + echo + echo "Installation methods (priority order):" + echo " 1. Docker - If Docker is installed and accessible in rootful mode" + echo " 2. Podman - If Podman is installed and accessible in rootful mode" + echo " 3. Snap - If Snap package manager is available" + echo " 4. WSL - If running in WSL2 with systemd and mirrored networking" + echo " 5. Standalone - Native binary with systemd" + echo + fi + + echo "⚙️ Detected settings:" + echo "- Server address: $SERVER_ADDRESS" + echo "- Enrollment: $(enrollment_summary)" + echo "- Agent version: $AGENT_VERSION" + echo "- Architecture: $BINARY_ARCH" + [ -n "$INSTALL_METHOD" ] && echo "- Install method: $INSTALL_METHOD" + echo + + detect_install_method + recommend_container_runtime + install_agent "$@" } [ "${INSTALL_SH_LIB:-}" = "1" ] || main "$@" From 82707ec1f8b4ae6806a2af7e3dc62ce53e4a07c9 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 11:21:46 -0300 Subject: [PATCH 09/11] fix(install): report the agent's real enrollment outcome The three credential branches printed a prediction and returned, so a refused device still read as a successful install. Where the runtime's output is reachable the installer now polls it briefly and reports what happened; where it is not, it names the command to inspect instead of claiming an outcome. A refusal is reported, not returned: the agent is installed and still retrying, so there is nothing to undo. An agent that refuses its own configuration is reported the same way, by the settings it named, so a malformed tenant does not read as a device that has simply not enrolled yet. refusal_reason takes the error field out of the logrus line and drops the rest, which is the agent's own bookkeeping. The escaped quotes are parked on a placeholder first: no POSIX sed expression can match up to the first quote that is not escaped. $_LOG_CMD expands unquoted so a caller can pass a command with arguments, such as 'docker logs --tail 50 shellhub-agent'. Twenty seconds is long enough for the agent to reach the server and be refused over a working link, and short enough not to hold the installer open over a slow one. --- install.bats | 155 +++++++++++++++++++++++++++++++++++++++++++++++++++ install.sh | 112 ++++++++++++++++++++++++++++++++++++- 2 files changed, 264 insertions(+), 3 deletions(-) diff --git a/install.bats b/install.bats index 48a3b6ac4e6..baa0a029ed2 100644 --- a/install.bats +++ b/install.bats @@ -33,6 +33,11 @@ skip_enrollment_wait() { stub_bin sleep 'exit 0' } +stub_agent_log() { + cat > "$BATS_TEST_TMPDIR/agent.log" + stub_bin agent-log "cat '$BATS_TEST_TMPDIR/agent.log'" +} + with_tenant() { export TENANT_ID="00000000-0000-4000-a000-000000000000" } @@ -262,6 +267,156 @@ enter_wsl() { assert_output_contains "appear as pending in the console" } +@test "observe_enrollment reports an agent that enrolled" { + stub_bin agent-log 'echo "time=now level=info msg=\"Listening for connections\""' + + call_install observe_enrollment agent-log + + [ "$status" -eq 0 ] + assert_output_contains "enrolled" +} + +@test "refusal_reason prints only the reason a log line carries" { + call_install refusal_reason 'time="now" level=warning msg="Cannot authorize the device" attempt=1 error="the server answered 404 Not Found: {\"message\":\"namespace not found\"}" server_address="http://localhost:80"' + + [ "$status" -eq 0 ] + [ "$output" = 'the server answered 404 Not Found: {"message":"namespace not found"}' ] +} + +@test "refusal_reason falls back to the line when it carries no reason" { + call_install refusal_reason 'level=warning msg="Cannot authorize the device"' + + [ "$status" -eq 0 ] + [ "$output" = 'level=warning msg="Cannot authorize the device"' ] +} + +@test "observe_enrollment reports the server's refusal" { + stub_agent_log <<'LOG' +time="now" level=fatal msg="Failed to authorize the device" error="the server answered 404 Not Found: {\"message\":\"namespace not found\"}" server_address="http://localhost:80" +LOG + + call_install observe_enrollment agent-log + + assert_output_contains "refused" + assert_output_contains 'the server answered 404 Not Found: {"message":"namespace not found"}' +} + +@test "observe_enrollment keeps the agent's own bookkeeping out of what it reports" { + stub_agent_log <<'LOG' +time="now" level=fatal msg="Failed to authorize the device" error="the server answered 404 Not Found" server_address="http://localhost:80" +LOG + + call_install observe_enrollment agent-log + + refute_output_contains "level=fatal" + refute_output_contains "server_address" +} + +@test "observe_enrollment reports the settings an agent refused its own configuration over" { + stub_agent_log <<'LOG' +time="now" level=error msg="SHELLHUB_TENANT_ID must be a UUID" +time="now" level=fatal msg="Failed to load the configuration from the environment variables" error="validation failed" +LOG + + call_install observe_enrollment agent-log + + [ "$status" -eq 0 ] + assert_output_contains "refused its own configuration" + assert_output_contains "SHELLHUB_TENANT_ID must be a UUID" + refute_output_contains "has not enrolled yet" +} + +@test "observe_enrollment falls back to the error when a refused configuration names no setting" { + stub_agent_log <<'LOG' +time="now" level=fatal msg="Failed to load the configuration from the environment variables" error="missing required value: SERVER_ADDRESS" +LOG + + call_install observe_enrollment agent-log + + [ "$status" -eq 0 ] + assert_output_contains "missing required value: SERVER_ADDRESS" + refute_output_contains "has not enrolled yet" +} + +@test "observe_enrollment reports an agent still retrying when the window closes" { + stub_agent_log <<'LOG' +time="now" level=warning msg="Cannot authorize the device, retrying until the server accepts it" attempt=1 error="the server answered 404 Not Found: {\"message\":\"namespace not found\"}" +LOG + + call_install observe_enrollment agent-log + + [ "$status" -eq 0 ] + assert_output_contains "still" + assert_output_contains 'the server answered 404 Not Found: {"message":"namespace not found"}' + refute_output_contains "level=warning" +} + +@test "observe_enrollment names the command to inspect an agent that says nothing" { + stub_bin agent-log + + call_install observe_enrollment agent-log + + [ "$status" -eq 0 ] + assert_output_contains "agent-log" +} + +@test "observe_enrollment does not claim an outcome for a runtime it cannot read" { + call_install observe_enrollment "" + + [ "$status" -eq 0 ] + assert_output_contains "does not expose" +} + +@test "enroll_agent_interactively observes the outcome of a pairing code enrollment" { + export CODE=ABC123 + stub_bin agent-log 'echo "level=info msg=\"Listening for connections\""' + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" agent-log + + assert_output_contains "pre-authorized" + assert_output_contains "enrolled" +} + +@test "enroll_agent_interactively observes the outcome of an install key enrollment" { + export INSTALL_KEY=key-1 + stub_bin agent-log 'echo "level=fatal msg=\"Failed to authorize the device\" error=\"the server answered 404 Not Found\""' + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" agent-log + + assert_output_contains "install key's namespace" + assert_output_contains "404 Not Found" +} + +@test "enroll_agent_interactively does not claim an outcome it cannot observe" { + export CODE=ABC123 + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" + + assert_output_contains "pre-authorized" + assert_output_contains "does not expose" +} + +@test "enroll_agent_interactively observes the outcome of a persisted tenant enrollment" { + echo "00000000-0000-4000-0000-000000000000" > "$AGENT_KEY.tenant" + export PRIVATE_KEY="$AGENT_KEY" + stub_bin agent-log 'echo "level=fatal msg=\"Failed to authorize the device\" error=\"the server answered 404 Not Found\""' + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" agent-log + + assert_output_contains "remembered at $AGENT_KEY.tenant" + assert_output_contains "404 Not Found" +} + +@test "enroll_agent_interactively observes the outcome of a tenant enrollment" { + with_tenant + stub_bin agent-log 'echo "level=fatal msg=\"Failed to authorize the device\" error=\"the server answered 404 Not Found\""' + + call_install enroll_agent_interactively shellhub-agent "$AGENT_KEY" agent-log + + assert_output_contains "appear as pending in the console" + assert_output_contains "404 Not Found" +} + @test "docker_install runs the container with unless-stopped so it survives a reboot" { container_install diff --git a/install.sh b/install.sh index d37536dcc90..8dc061aa34d 100755 --- a/install.sh +++ b/install.sh @@ -98,6 +98,98 @@ enrollment_summary() { fi } +# refusal_reason prints the part of an agent log line an operator can act on. logrus writes the line +# as logfmt, so the reason is the error field and everything around it is the agent's own +# bookkeeping. The line is printed whole when it carries no error field, because a reason that +# cannot be parsed out is still worth more than nothing. The quotes logrus escapes inside the field +# are parked on a placeholder first, because no POSIX sed expression can say "up to the first quote +# that is not escaped". +refusal_reason() { + _LOG_LINE="$1" + + case "$_LOG_LINE" in + *'error="'*) ;; + *) + echo "$_LOG_LINE" + + return 0 + ;; + esac + + echo "$_LOG_LINE" | sed -e 's/\\"/@@Q@@/g' -e 's/.*error="\([^"]*\)".*/\1/' -e 's/@@Q@@/"/g' +} + +# observe_enrollment reports what the agent did with the credential rather than what the installer +# predicted, so a device the server refuses is visible here instead of only in the agent's own log. +# $1 is a command that prints the agent's recent output, left unquoted on purpose so the caller can +# pass one with arguments; empty means this runtime cannot be read from the installer, and no +# outcome is claimed. A refusal is reported, never returned: the agent is already installed and +# keeps retrying, so there is nothing for the caller to undo. +observe_enrollment() { + _LOG_CMD="$1" + + if [ -z "$_LOG_CMD" ]; then + echo "ℹ️ This install method does not expose the agent's output to the installer." + echo " Check the console to confirm the device enrolled." + + return 0 + fi + + _OBSERVED="" + _WAITED=0 + + while [ "$_WAITED" -lt "${ENROLLMENT_OBSERVE_SECONDS:-20}" ]; do + _OBSERVED=$($_LOG_CMD 2>&1) + + case "$_OBSERVED" in + *"Listening for connections"*) + echo "✅ The agent enrolled and is listening for connections." + + return 0 + ;; + *"Failed to authorize the device"*) + _REFUSAL=$(echo "$_OBSERVED" | grep "Failed to authorize the device" | tail -n 1) + + echo "❌ The server refused this device:" + echo " $(refusal_reason "$_REFUSAL")" + + return 0 + ;; + *"Failed to load the configuration"*) + _INVALID=$(echo "$_OBSERVED" | sed -n 's/.*msg="\(SHELLHUB_[^"]*\)".*/\1/p') + + echo "❌ The agent refused its own configuration:" + + if [ -n "$_INVALID" ]; then + echo "$_INVALID" | sed 's/^/ /' + else + _REFUSAL=$(echo "$_OBSERVED" | grep "Failed to load the configuration" | tail -n 1) + + echo " $(refusal_reason "$_REFUSAL")" + fi + + return 0 + ;; + esac + + sleep 1 + _WAITED=$((_WAITED + 1)) + done + + case "$_OBSERVED" in + *"Cannot authorize the device"*) + _REFUSAL=$(echo "$_OBSERVED" | grep "Cannot authorize the device" | tail -n 1) + + echo "⚠️ The server is still refusing this device, and the agent is still retrying:" + echo " $(refusal_reason "$_REFUSAL")" + ;; + *) + echo "⚠️ The agent has not enrolled yet. Inspect it with:" + echo " $_LOG_CMD" + ;; + esac +} + # Enrolls a freshly installed agent. Without a tenant the device does not belong # to any namespace yet, so we run the login flow in the foreground: it prints the # accept URL (opening the browser when possible) and waits until a user accepts @@ -109,14 +201,19 @@ enrollment_summary() { # methods this is the wrapper (which execs into the container); for native # methods it is the agent binary itself, possibly prefixed with sudo. # $2: host-visible path of the agent key to wait for before pairing. +# $3: command that prints the agent's recent output, or empty when this runtime has none the +# installer can read. enroll_agent_interactively() { _AGENT_CMD="$1" _WAIT_KEY="$2" + _AGENT_LOG="$3" if [ -n "$CODE" ]; then echo "" echo "The device is pre-authorized and will be accepted automatically once it connects." + observe_enrollment "$_AGENT_LOG" + return 0 fi @@ -125,6 +222,8 @@ enroll_agent_interactively() { echo "The device will enroll into the install key's namespace." echo "Whether it is accepted straight away or left pending is the key's own setting." + observe_enrollment "$_AGENT_LOG" + return 0 fi @@ -132,6 +231,8 @@ enroll_agent_interactively() { echo "" echo "The device will appear as pending in the console — accept it there." + observe_enrollment "$_AGENT_LOG" + return 0 fi @@ -142,6 +243,8 @@ enroll_agent_interactively() { echo "The device will enroll into tenant $_PERSISTED, remembered at $(tenant_file)." echo "Delete that file to enroll it somewhere else." + observe_enrollment "$_AGENT_LOG" + return 0 fi @@ -270,7 +373,7 @@ podman_install() { # The key path is under /host (the agent mounts the host root there); strip # that prefix so the installer waits on the real host path. _CKEY="${PRIVATE_KEY:-/host/etc/shellhub.key}" - enroll_agent_interactively "$WRAPPER_PATH" "${_CKEY#/host}" + enroll_agent_interactively "$WRAPPER_PATH" "${_CKEY#/host}" "$SUDO podman logs --tail 50 $CONTAINER_NAME" fi } @@ -359,7 +462,7 @@ docker_install() { # The key path is under /host (the agent mounts the host root there); strip # that prefix so the installer waits on the real host path. _CKEY="${PRIVATE_KEY:-/host/etc/shellhub.key}" - enroll_agent_interactively "$WRAPPER_PATH" "${_CKEY#/host}" + enroll_agent_interactively "$WRAPPER_PATH" "${_CKEY#/host}" "$SUDO docker logs --tail 50 $CONTAINER_NAME" fi } @@ -462,7 +565,10 @@ standalone_install() { # Native install: the binary is the command and opens the browser itself, so # no wrapper is needed — enroll by invoking it directly. Reads the root-owned # key, hence $SUDO. - enroll_agent_interactively "$SUDO $INSTALL_BIN" "${PRIVATE_KEY:-/etc/shellhub.key}" + AGENT_LOG_CMD="" + command -v journalctl >/dev/null 2>&1 && AGENT_LOG_CMD="$SUDO journalctl -u shellhub-agent --no-pager -n 50" + + enroll_agent_interactively "$SUDO $INSTALL_BIN" "${PRIVATE_KEY:-/etc/shellhub.key}" "$AGENT_LOG_CMD" rm -rf "$TMP_DIR" } From 6067360636e2097a34682acdc0bbe400390cbb11 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Fri, 11 Sep 2026 14:06:15 -0300 Subject: [PATCH 10/11] fix(agent): name the environment variable a bad setting came from The validator reports Go field names and rule names, so a typo surfaced as 'TenantID=uuid' over two log lines. It now reads 'SHELLHUB_TENANT_ID must be a UUID', once. Values are never echoed: InstallKey and SingleUserPassword pass through the same map. --- agent/main.go | 8 ++-- agent/pkg/agentd/agent.go | 68 ++++++++++++++++++++++++++++++++-- agent/pkg/agentd/agent_test.go | 53 ++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 8 deletions(-) diff --git a/agent/main.go b/agent/main.go index 8c58c2294f1..35f0220d37f 100644 --- a/agent/main.go +++ b/agent/main.go @@ -34,7 +34,7 @@ func main() { cfg, fields, err := agentd.LoadConfigFromEnv() if err != nil { - log.WithError(err).WithFields(fields).Fatal("Failed to load de configuration from the environmental variables") + agentd.FatalInvalidConfig[agentd.Config](fields, err) } cfg.Version = AgentVersion @@ -227,9 +227,7 @@ func main() { cfg, fields, err := LoadConfigConnectorFromEnv() if err != nil { - log.WithError(err). - WithFields(fields). - Fatal("Failed to load de configuration from the environmental variables") + agentd.FatalInvalidConfig[ConfigConnector](fields, err) } logger := log.WithFields( @@ -345,7 +343,7 @@ waits until the device is accepted, rejected, or the code expires.`, cfg, fields, err := agentd.LoadConfigFromEnv() if err != nil { - log.WithError(err).WithFields(fields).Fatal("Failed to load the configuration from the environmental variables") + agentd.FatalInvalidConfig[agentd.Config](fields, err) } cfg.Version = AgentVersion diff --git a/agent/pkg/agentd/agent.go b/agent/pkg/agentd/agent.go index 74b1b2d311b..50ea7873ea9 100644 --- a/agent/pkg/agentd/agent.go +++ b/agent/pkg/agentd/agent.go @@ -56,7 +56,9 @@ import ( "os" "os/exec" "path/filepath" + "reflect" "runtime" + "sort" "strings" "sync/atomic" "time" @@ -198,7 +200,7 @@ func (c *Config) credential() string { func LoadConfigFromEnv() (*Config, map[string]any, error) { applyEnvFileFallback(defaultEnvFilePath) - cfg, err := envs.ParseWithPrefix[Config]("SHELLHUB_") + cfg, err := envs.ParseWithPrefix[Config](envPrefix) if err != nil { log.Error("failed to parse the configuration") @@ -223,14 +225,74 @@ func LoadConfigFromEnv() (*Config, map[string]any, error) { } if ok, fields, err := validator.New().StructWithFields(cfg); err != nil || !ok { - log.WithFields(fields).Error("failed to validate the configuration loaded from envs") - return nil, fields, err } return cfg, nil, nil } +const envPrefix = "SHELLHUB_" + +// FatalInvalidConfig reports every invalid setting in fields by the environment variable an +// operator sets, then exits the process. T is the configuration the fields came from. It does not +// return. +func FatalInvalidConfig[T any](fields map[string]any, err error) { + for _, message := range InvalidConfigMessages[T](fields) { + log.Error(message) + } + + log.WithError(err).Fatal("Failed to load the configuration from the environment variables") +} + +// InvalidConfigMessages turns the field map [LoadConfigFromEnv] returns into one message per +// invalid setting, naming the environment variable an operator sets rather than the struct field +// the validator reported. T is the configuration the map came from, read for its env tags; a field +// it does not carry is named as it stands. No value is ever included, because some settings are +// credentials and a log line is not where those belong. +func InvalidConfigMessages[T any](fields map[string]any) []string { + if len(fields) == 0 { + return nil + } + + structure := reflect.TypeFor[T]() + messages := make([]string, 0, len(fields)) + + for field, rule := range fields { + messages = append(messages, configEnvName(structure, field)+" "+requirementOf(rule)) + } + + sort.Strings(messages) + + return messages +} + +func configEnvName(structure reflect.Type, field string) string { + structField, ok := structure.FieldByName(field) + if !ok { + return field + } + + name, _, _ := strings.Cut(structField.Tag.Get("env"), ",") + if name == "" { + return field + } + + return envPrefix + name +} + +func requirementOf(rule any) string { + switch rule { + case "required": + return "is required" + case "uuid", "uuid4": + return "must be a UUID" + case "min", "max": + return "is out of range" + default: + return fmt.Sprintf("is invalid (%v)", rule) + } +} + // Agent is a device's connection to a ShellHub server: it authenticates, keeps the device // record current, and serves the SSH sessions the server routes to it. // diff --git a/agent/pkg/agentd/agent_test.go b/agent/pkg/agentd/agent_test.go index 21c0776eb33..b65c2e82fc5 100644 --- a/agent/pkg/agentd/agent_test.go +++ b/agent/pkg/agentd/agent_test.go @@ -695,3 +695,56 @@ func TestAuthorizeNamesTheCredentialItWasRefusedFor(t *testing.T) { }) } } + +func TestInvalidConfigMessagesNamesTheEnvironmentVariable(t *testing.T) { + cases := []struct { + description string + fields map[string]any + expected []string + }{ + { + description: "names the variable and spells out the rule", + fields: map[string]any{"TenantID": "uuid"}, + expected: []string{"SHELLHUB_TENANT_ID must be a UUID"}, + }, + { + description: "reports a missing required variable", + fields: map[string]any{"ServerAddress": "required"}, + expected: []string{"SHELLHUB_SERVER_ADDRESS is required"}, + }, + { + description: "reports a value outside its range", + fields: map[string]any{"MaxRetryConnectionTimeout": "max"}, + expected: []string{"SHELLHUB_MAX_RETRY_CONNECTION_TIMEOUT is out of range"}, + }, + { + description: "orders the messages so a run is reproducible", + fields: map[string]any{"TenantID": "uuid", "ServerAddress": "required"}, + expected: []string{ + "SHELLHUB_SERVER_ADDRESS is required", + "SHELLHUB_TENANT_ID must be a UUID", + }, + }, + { + description: "falls back to the field name when it reads no environment variable", + fields: map[string]any{"Version": "required"}, + expected: []string{"Version is required"}, + }, + { + description: "falls back to the rule's name when it has no plain wording", + fields: map[string]any{"TenantID": "startswith"}, + expected: []string{"SHELLHUB_TENANT_ID is invalid (startswith)"}, + }, + { + description: "reports nothing when nothing failed", + fields: nil, + expected: nil, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + assert.Equal(t, tc.expected, InvalidConfigMessages[Config](tc.fields)) + }) + } +} From 779f61f147b0ba5ef939431d50d807685da2fb24 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Mon, 14 Sep 2026 15:07:47 -0300 Subject: [PATCH 11/11] fix(install): report a container uninstall did not find docker rm -f exits 0 for a container that does not exist, so the branch meant to report one that was already gone was unreachable and uninstall claimed success having removed nothing. Ask docker whether the container exists first. The name filter is anchored because it matches substrings. --- install.bats | 23 +++++++++++++++++------ install.sh | 16 ++++++++++++---- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/install.bats b/install.bats index baa0a029ed2..413bf4b9bc0 100644 --- a/install.bats +++ b/install.bats @@ -837,7 +837,7 @@ LOG } @test "docker_uninstall removes the container and the wrapper" { - stub_bin docker + stub_bin docker 'echo "docker $*" >> "$CALLS"; [ "$1" = ps ] && echo deadbeef; exit 0' call_install install_agent_wrapper docker call_install docker_uninstall @@ -848,7 +848,7 @@ LOG } @test "docker_uninstall escalates when it is not already root" { - stub_bin docker + stub_bin docker 'echo "docker $*" >> "$CALLS"; [ "$1" = ps ] && echo deadbeef; exit 0' call_install install_agent_wrapper docker as_non_root @@ -860,7 +860,7 @@ LOG } @test "podman_uninstall removes the container and the wrapper" { - stub_bin podman + stub_bin podman 'echo "podman $*" >> "$CALLS"; [ "$1" = ps ] && echo deadbeef; exit 0' call_install install_agent_wrapper podman call_install podman_uninstall @@ -871,7 +871,7 @@ LOG } @test "podman_uninstall escalates when it is not already root" { - stub_bin podman + stub_bin podman 'echo "podman $*" >> "$CALLS"; [ "$1" = ps ] && echo deadbeef; exit 0' call_install install_agent_wrapper podman as_non_root @@ -883,12 +883,23 @@ LOG } @test "docker_uninstall reports a container that was already gone" { - stub_bin docker 'echo "docker $*" >> "$CALLS"; exit 1' + stub_bin docker call_install docker_uninstall [ "$status" -eq 0 ] assert_output_contains "not found (may already be removed)" + refute_called "docker rm -f" +} + +@test "podman_uninstall reports a container that was already gone" { + stub_bin podman + + call_install podman_uninstall + + [ "$status" -eq 0 ] + assert_output_contains "not found (may already be removed)" + refute_called "podman rm -f" } @test "uninstall names the tenant file it leaves behind" { @@ -1160,7 +1171,7 @@ LOG } @test "uninstall dispatches to the detected method" { - stub_bin docker + stub_bin docker 'echo "docker $*" >> "$CALLS"; [ "$1" = ps ] && echo deadbeef; exit 0' run_install uninstall diff --git a/install.sh b/install.sh index 8dc061aa34d..f8c6aafe0a9 100755 --- a/install.sh +++ b/install.sh @@ -590,8 +590,12 @@ docker_uninstall() { CONTAINER_NAME="${CONTAINER_NAME:-shellhub}" - echo "🗑️ Stopping and removing ShellHub container..." - $SUDO docker rm -f "$CONTAINER_NAME" 2>/dev/null || echo "⚠️ Container '$CONTAINER_NAME' not found (may already be removed)." + if [ -z "$($SUDO docker ps -a -q -f "name=^${CONTAINER_NAME}$" 2>/dev/null)" ]; then + echo "⚠️ Container '$CONTAINER_NAME' not found (may already be removed)." + else + echo "🗑️ Stopping and removing ShellHub container..." + $SUDO docker rm -f "$CONTAINER_NAME" >/dev/null + fi WRAPPER_PATH="${INSTALL_DIR:-/usr/local/bin}/shellhub-agent" if [ -f "$WRAPPER_PATH" ]; then @@ -611,8 +615,12 @@ podman_uninstall() { CONTAINER_NAME="${CONTAINER_NAME:-shellhub}" - echo "🗑️ Stopping and removing ShellHub container..." - $SUDO podman rm -f "$CONTAINER_NAME" 2>/dev/null || echo "⚠️ Container '$CONTAINER_NAME' not found (may already be removed)." + if [ -z "$($SUDO podman ps -a -q -f "name=^${CONTAINER_NAME}$" 2>/dev/null)" ]; then + echo "⚠️ Container '$CONTAINER_NAME' not found (may already be removed)." + else + echo "🗑️ Stopping and removing ShellHub container..." + $SUDO podman rm -f "$CONTAINER_NAME" >/dev/null + fi WRAPPER_PATH="${INSTALL_DIR:-/usr/local/bin}/shellhub-agent" if [ -f "$WRAPPER_PATH" ]; then