From 2abce8345a1d6f3813a3b1104a71438da6e94285 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Wed, 7 Oct 2026 08:48:42 -0300 Subject: [PATCH 1/5] refactor: share the e2e helpers the API key tests build on withAPIKey replaces the closure TestRoutesThatRefuseAPIKeys kept for the X-API-Key header, so the next tests in the file send a key the same way. tagNames takes a tag list instead of a device, so it reads the tags a namespace lists as well as the ones a device carries. enrollAPIKeyIdentity is newAPIKeyIdentity with an expiry, for an API key identity that has to expire. expireIdentityAndRequireRefused lifts the expired-identity steps out of TestSSHIdentityConnection; the name says it expires the identity, because the caller's later assertions depend on it having done so. --- tests/api_key_test.go | 16 +++++++-------- tests/enrollment_automatic_test.go | 2 +- tests/enrollment_reregistration_test.go | 2 +- tests/enrollment_test.go | 6 +++--- tests/identity_connection_test.go | 10 +-------- tests/identity_helpers_test.go | 27 ++++++++++++++++++++++++- 6 files changed, 40 insertions(+), 23 deletions(-) diff --git a/tests/api_key_test.go b/tests/api_key_test.go index a7d4e2a1248..4b244fb73b8 100644 --- a/tests/api_key_test.go +++ b/tests/api_key_test.go @@ -28,14 +28,8 @@ func TestRoutesThatRefuseAPIKeys(t *testing.T) { OptRole: authorizer.RoleAdministrator, }) - withKey := func(t *testing.T) *resty.Request { - t.Helper() - - return compose.Anonymous(t.Context()).SetHeader("X-API-Key", key.Key) - } - t.Run("the key authenticates on a namespace route", func(t *testing.T) { - resp, err := withKey(t).Get("/api/devices") + resp, err := withAPIKey(t, compose, key.Key).Get("/api/devices") require.NoError(t, err) assert.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) }) @@ -159,9 +153,15 @@ func TestRoutesThatRefuseAPIKeys(t *testing.T) { for _, tc := range cases { t.Run("refuses "+tc.description, func(t *testing.T) { - resp, err := withKey(t).Execute(tc.method, tc.path) + resp, err := withAPIKey(t, compose, key.Key).Execute(tc.method, tc.path) require.NoError(t, err) assert.Equal(t, http.StatusForbidden, resp.StatusCode(), resp.String()) }) } } + +func withAPIKey(t *testing.T, compose *environment.DockerCompose, plaintext string) *resty.Request { + t.Helper() + + return compose.Anonymous(t.Context()).SetHeader("X-API-Key", plaintext) +} diff --git a/tests/enrollment_automatic_test.go b/tests/enrollment_automatic_test.go index 0ba8686d706..f223e694f12 100644 --- a/tests/enrollment_automatic_test.go +++ b/tests/enrollment_automatic_test.go @@ -63,7 +63,7 @@ func testAutomaticEnrollment(t *testing.T, compose *environment.DockerCompose) { device := enroll(t, compose, newKeyedDeviceAuthRequest(t, key, "automatic-tagged", "02:00:00:00:10:06")) assert.Equal(t, models.DeviceStatusAccepted, device.Status) - assert.ElementsMatch(t, []string{"fleet", "edge"}, tagNames(device)) + assert.ElementsMatch(t, []string{"fleet", "edge"}, tagNames(device.Tags)) }) t.Run("the device is ephemeral when the key is", func(t *testing.T) { diff --git a/tests/enrollment_reregistration_test.go b/tests/enrollment_reregistration_test.go index 4b9aa022e17..a2f59103b72 100644 --- a/tests/enrollment_reregistration_test.go +++ b/tests/enrollment_reregistration_test.go @@ -39,7 +39,7 @@ func testReRegistration(t *testing.T, compose *environment.DockerCompose) { assert.Equal(t, enrolled.UID, reregistered.UID) assert.Equal(t, models.DeviceStatusAccepted, reregistered.Status) assert.Equal(t, key.ID, reregistered.ProvisioningKeyID) - assert.ElementsMatch(t, []string{"enrolled", "reenrolled"}, tagNames(reregistered), + assert.ElementsMatch(t, []string{"enrolled", "reenrolled"}, tagNames(reregistered.Tags), "re-registration adds the key's current tags to those the removed device kept") assert.True(t, reregistered.Ephemeral) assert.Equal(t, 4, reregistered.EphemeralTimeout) diff --git a/tests/enrollment_test.go b/tests/enrollment_test.go index 585dbe803ab..bc2f01f26d5 100644 --- a/tests/enrollment_test.go +++ b/tests/enrollment_test.go @@ -92,9 +92,9 @@ func awaitStatusOnReauth(t *testing.T, compose *environment.DockerCompose, req r }, deviceAuthCacheTTL+30*time.Second, 2*time.Second) } -func tagNames(device models.Device) []string { - names := make([]string, 0, len(device.Tags)) - for _, tag := range device.Tags { +func tagNames(tags []models.Tag) []string { + names := make([]string, 0, len(tags)) + for _, tag := range tags { names = append(names, tag.Name) } diff --git a/tests/identity_connection_test.go b/tests/identity_connection_test.go index 5a83029b93e..8cfd5e8869c 100644 --- a/tests/identity_connection_test.go +++ b/tests/identity_connection_test.go @@ -39,15 +39,7 @@ func TestSSHIdentityConnection(t *testing.T) { requireStraightThrough(t, compose, sshid, signer) - compose.ExpireSSHIdentity(t, fingerprint) - - mark := compose.ServerLogMark(t) - - refused := startLogin(t, compose, sshid, signer) - require.Error(t, refused.result(t)) - assert.Zero(t, refused.approvals(), "a known key past its expiry must not be sent to enrollment again") - - compose.AwaitServerLogLine(t, mark, deadIdentityLog, `error="ssh access denied by policy"`) + expireIdentityAndRequireRefused(t, compose, sshid, signer) identity := identityByFingerprint(t, compose, fingerprint) assert.False(t, identity.Active(time.Now()), //nolint:forbidigo // the expiry the server compares against its own wall clock diff --git a/tests/identity_helpers_test.go b/tests/identity_helpers_test.go index 8b968acb4c3..9b77c57028a 100644 --- a/tests/identity_helpers_test.go +++ b/tests/identity_helpers_test.go @@ -191,6 +191,20 @@ func requireRefusedAtAuth(t *testing.T, compose *environment.DockerCompose, sshi require.ErrorContains(t, err, "unable to authenticate") } +func expireIdentityAndRequireRefused(t *testing.T, compose *environment.DockerCompose, sshid string, signer ssh.Signer) { + t.Helper() + + compose.ExpireSSHIdentity(t, ssh.FingerprintSHA256(signer.PublicKey())) + + mark := compose.ServerLogMark(t) + + refused := startLogin(t, compose, sshid, signer) + require.Error(t, refused.result(t)) + assert.Zero(t, refused.approvals(), "a known key past its expiry must not be sent to enrollment again") + + compose.AwaitServerLogLine(t, mark, deadIdentityLog, `error="ssh access denied by policy"`) +} + func approvalRequest(ctx context.Context, compose *environment.DockerCompose, token string) *resty.Request { req := compose.R(ctx) if token != "" { @@ -312,12 +326,23 @@ func newMember(t *testing.T, compose *environment.DockerCompose, username string func newAPIKeyIdentity(t *testing.T, compose *environment.DockerCompose, name string, singleUse bool) (*responses.CreateAPIKey, ssh.Signer) { t.Helper() + return enrollAPIKeyIdentity(t, compose, name, nil, singleUse) +} + +func enrollAPIKeyIdentity(t *testing.T, compose *environment.DockerCompose, name string, expiresIn *int, singleUse bool) (*responses.CreateAPIKey, ssh.Signer) { + t.Helper() + key := compose.CreateAPIKey(t, &requests.CreateAPIKey{Name: name, ExpiresAt: -1}) signer, data := newSigner(t) + body := map[string]any{"name": name, "data": data, "single_use": singleUse} + if expiresIn != nil { + body["expires_in"] = *expiresIn + } + resp, err := compose.R(t.Context()). - SetBody(map[string]any{"name": name, "data": data, "single_use": singleUse}). + SetBody(body). Post("/api/namespaces/api-key/" + name + "/ssh-identities") require.NoError(t, err) require.Equal(t, 200, resp.StatusCode(), resp.String()) From f651d4b8dd5fc99da94f1cd787407b7567394f86 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Wed, 7 Oct 2026 08:48:51 -0300 Subject: [PATCH 2/5] test: cover namespace API key authentication A key manages the namespace's tags, on their own and on a device, and acts within its role: an observer key reads devices and nothing more, an operator key also manages tags, and only an administrator key reaches the access policies. Renaming a tag is left out because PATCH /api/tags/:name answers 200 with no body while the OpenAPI spec declares the renamed tag, so the strict-validating stack turns it into a 500. A key created with -1 is stored and listed with -1 and authenticates. -1 is the marker for a key that never expires; were it read as a Unix time it would be a second before 1970. The expiring key is created with a 30-day expiry, the only kind the API mints, and ExpireAPIKeyIn then moves it 10 seconds ahead. That has to happen before the key's first use: the server caches a key it authenticated for two minutes, with the expiry it read. The first request then caches it, and the 401 that follows comes from the cached copy, since the server checks the expiry on a cache hit too. The wait allows three times the TTL for the clock to pass it. --- tests/api_key_test.go | 143 +++++++++++++++++++++++++++++++++++ tests/environment/api_key.go | 18 +++++ 2 files changed, 161 insertions(+) create mode 100644 tests/environment/api_key.go diff --git a/tests/api_key_test.go b/tests/api_key_test.go index 4b244fb73b8..a1f096bc932 100644 --- a/tests/api_key_test.go +++ b/tests/api_key_test.go @@ -2,11 +2,15 @@ package main import ( "net/http" + "slices" "testing" + "time" "github.com/go-resty/resty/v2" "github.com/shellhub-io/shellhub/pkg/api/authorizer" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/models" + "github.com/shellhub-io/shellhub/pkg/uuid" "github.com/shellhub-io/shellhub/tests/environment" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -160,6 +164,145 @@ func TestRoutesThatRefuseAPIKeys(t *testing.T) { } } +// TestNamespaceAPIKeyAuthentication covers what a namespace API key can do once minted: manage the +// namespace's tags, act only within its role, keep working when it never expires, and stop working +// once its expiry passes. The routes that refuse a key whatever its role are covered by +// [TestRoutesThatRefuseAPIKeys]. +func TestNamespaceAPIKeyAuthentication(t *testing.T) { + compose := environment.New(t, run).Up(t.Context()) + t.Cleanup(compose.Down) + + compose.NewUser(t, ShellHubUsername, ShellHubEmail, ShellHubPassword) + compose.NewNamespace(t, ShellHubUsername, ShellHubNamespaceName, ShellHubNamespace, "") + + compose.JWT(compose.AuthUser(t, ShellHubUsername, ShellHubPassword).Token) + + _, device := startAcceptedAgent(t, t.Context(), compose) + + deviceTags := func(t *testing.T) []string { + t.Helper() + + current, resp, err := compose.GetDevice(t.Context(), device.UID) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + return tagNames(current.Tags) + } + + namespaceTags := func(t *testing.T, req *resty.Request) []string { + t.Helper() + + tags := []models.Tag{} + resp, err := req.SetResult(&tags).Get("/api/tags") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + return tagNames(tags) + } + + t.Run("a key creates, lists, attaches, detaches and deletes tags", func(t *testing.T) { + key := compose.CreateAPIKey(t, &requests.CreateAPIKey{Name: "tagger", ExpiresAt: -1, OptRole: authorizer.RoleOperator}) + + resp, err := withAPIKey(t, compose, key.Key).SetBody(map[string]string{"name": "staging"}).Post("/api/tags") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + assert.Contains(t, namespaceTags(t, withAPIKey(t, compose, key.Key)), "staging") + + resp, err = withAPIKey(t, compose, key.Key).Post("/api/devices/" + device.UID + "/tags/staging") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + assert.Equal(t, []string{"staging"}, deviceTags(t)) + + resp, err = withAPIKey(t, compose, key.Key).Delete("/api/devices/" + device.UID + "/tags/staging") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + assert.Empty(t, deviceTags(t)) + + resp, err = withAPIKey(t, compose, key.Key).Delete("/api/tags/staging") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + assert.NotContains(t, namespaceTags(t, compose.R(t.Context())), "staging") + }) + + t.Run("a key acts only within its role", func(t *testing.T) { + listDevices := func(req *resty.Request) (*resty.Response, error) { return req.Get("/api/devices") } + createTag := func(req *resty.Request) (*resty.Response, error) { + return req.SetBody(map[string]string{"name": "role" + uuid.Generate()[:8]}).Post("/api/tags") + } + listAccessPolicies := func(req *resty.Request) (*resty.Response, error) { return req.Get("/api/access-policies") } + + cases := []struct { + role authorizer.Role + devices, tags, policies int + }{ + {role: authorizer.RoleObserver, devices: http.StatusOK, tags: http.StatusForbidden, policies: http.StatusForbidden}, + {role: authorizer.RoleOperator, devices: http.StatusOK, tags: http.StatusOK, policies: http.StatusForbidden}, + {role: authorizer.RoleAdministrator, devices: http.StatusOK, tags: http.StatusOK, policies: http.StatusOK}, + } + + for _, tc := range cases { + t.Run(tc.role.String(), func(t *testing.T) { + key := compose.CreateAPIKey(t, &requests.CreateAPIKey{Name: tc.role.String(), ExpiresAt: -1, OptRole: tc.role}) + require.Equal(t, tc.role, key.Role) + + for _, check := range []struct { + request func(*resty.Request) (*resty.Response, error) + want int + }{ + {request: listDevices, want: tc.devices}, + {request: createTag, want: tc.tags}, + {request: listAccessPolicies, want: tc.policies}, + } { + resp, err := check.request(withAPIKey(t, compose, key.Key)) + require.NoError(t, err) + assert.Equal(t, check.want, resp.StatusCode(), "%s %s: %s", resp.Request.Method, resp.Request.URL, resp.String()) + } + }) + } + }) + + t.Run("a key that never expires is stored without an expiry and authenticates", func(t *testing.T) { + key := compose.CreateAPIKey(t, &requests.CreateAPIKey{Name: "forever", ExpiresAt: -1}) + require.Equal(t, int64(-1), key.ExpiresIn) + + keys := []models.APIKey{} + resp, err := compose.R(t.Context()).SetResult(&keys).Get("/api/namespaces/api-key") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + index := slices.IndexFunc(keys, func(k models.APIKey) bool { return k.Name == "forever" }) + require.GreaterOrEqual(t, index, 0) + assert.Equal(t, int64(-1), keys[index].ExpiresIn, + "-1 is the marker for a key that never expires, not a time") + + resp, err = withAPIKey(t, compose, key.Key).Get("/api/devices") + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + }) + + t.Run("a key is refused once its expiry passes", func(t *testing.T) { + const ttl = 10 * time.Second + + key := compose.CreateAPIKey(t, &requests.CreateAPIKey{Name: "expiring", ExpiresAt: 30}) + compose.ExpireAPIKeyIn(t, key.Name, ttl) + + resp, err := withAPIKey(t, compose, key.Key).Get("/api/devices") + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + require.EventuallyWithT(t, func(tt *assert.CollectT) { + resp, err := withAPIKey(t, compose, key.Key).Get("/api/devices") + if !assert.NoError(tt, err) { + return + } + + assert.Equal(tt, http.StatusUnauthorized, resp.StatusCode(), resp.String()) + }, 3*ttl, time.Second) + }) +} + func withAPIKey(t *testing.T, compose *environment.DockerCompose, plaintext string) *resty.Request { t.Helper() diff --git a/tests/environment/api_key.go b/tests/environment/api_key.go new file mode 100644 index 00000000000..5cc00fcd1a3 --- /dev/null +++ b/tests/environment/api_key.go @@ -0,0 +1,18 @@ +package environment + +import ( + "testing" + "time" +) + +// ExpireAPIKeyIn moves the expiry of the API key named name to ttl from now, failing t unless +// exactly that key changed. The API sets an expiry only in whole days ahead, so it writes the row +// directly, standing in for the days a real key waits to expire. A key the server has already +// cached keeps the expiry it was cached with, so call it before the key's first use. +func (dc *DockerCompose) ExpireAPIKeyIn(t *testing.T, name string, ttl time.Duration) { + t.Helper() + + dc.updateOne(t, + "UPDATE api_keys SET expires_in = extract(epoch FROM now() + :'ttl'::interval)::bigint WHERE name = :'name'", + map[string]string{"name": name, "ttl": interval(ttl)}) +} From 79b165de342e2531e4558c9cbd00987ba2c1b9c8 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Wed, 7 Oct 2026 08:48:59 -0300 Subject: [PATCH 3/5] test: refuse an instance API key on a namespace route The server honours a key carrying the instance key prefix only under /admin/api, and refuses it everywhere else before looking it up, so a plain prefixed key stands in for a real one. The routes that mint and accept instance keys belong to the enterprise admin API, which the community stack this suite runs on does not serve: the gateway proxies /admin/api only when enterprise is enabled. The rest of the instance key cases need that stack. --- tests/api_key_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/api_key_test.go b/tests/api_key_test.go index a1f096bc932..e1051e5c22b 100644 --- a/tests/api_key_test.go +++ b/tests/api_key_test.go @@ -166,7 +166,8 @@ func TestRoutesThatRefuseAPIKeys(t *testing.T) { // TestNamespaceAPIKeyAuthentication covers what a namespace API key can do once minted: manage the // namespace's tags, act only within its role, keep working when it never expires, and stop working -// once its expiry passes. The routes that refuse a key whatever its role are covered by +// once its expiry passes. It also covers a key carrying the instance key prefix, which the server +// honours only on the admin API, being refused on a namespace route. The routes that refuse a key whatever its role are covered by // [TestRoutesThatRefuseAPIKeys]. func TestNamespaceAPIKeyAuthentication(t *testing.T) { compose := environment.New(t, run).Up(t.Context()) @@ -282,6 +283,12 @@ func TestNamespaceAPIKeyAuthentication(t *testing.T) { assert.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) }) + t.Run("a key carrying the instance key prefix is refused on a namespace route", func(t *testing.T) { + resp, err := withAPIKey(t, compose, models.InstanceAPIKeyPrefix+uuid.Generate()).Get("/api/devices") + require.NoError(t, err) + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode(), resp.String()) + }) + t.Run("a key is refused once its expiry passes", func(t *testing.T) { const ttl = 10 * time.Second From dc76162e061f176884870d4960eb37d791c73309 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Wed, 7 Oct 2026 08:49:06 -0300 Subject: [PATCH 4/5] test: cover what decides an API key's SSH login With no policy naming it, an API key's identity is refused for want of a grant, and a deny policy naming the key refuses it although an allow naming it let it in a moment before. The identity works within its expiry and is refused once it passes, without being offered for enrollment again. Deleting the key takes its identities and its access policies with it, through the foreign keys that cascade from api_keys. The policy is created without the cleanup grant registers, since there is nothing left to delete; the test asserts it is gone, and that the gateway now asks to enroll the key as one it has never seen. --- tests/api_key_ssh_test.go | 102 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+) create mode 100644 tests/api_key_ssh_test.go diff --git a/tests/api_key_ssh_test.go b/tests/api_key_ssh_test.go new file mode 100644 index 00000000000..909d05d65d1 --- /dev/null +++ b/tests/api_key_ssh_test.go @@ -0,0 +1,102 @@ +package main + +import ( + "context" + "net/http" + "testing" + "time" + + "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestAPIKeySSHAccess covers what decides an API key's SSH login in the identity mode: no policy +// naming the key lets it in, a deny naming it beats the allow that would, its identity works until +// its expiry, and deleting the key takes its identities with it. That an every-member policy does +// not grant a key is covered by [TestAccessPolicyEvaluation], single-use identities by +// [TestSSHIdentityConnection], and that a key is never asked to re-authenticate by +// [TestAccessPolicyReauth]. +func TestAPIKeySSHAccess(t *testing.T) { + ctx := context.Background() + compose := newSSHEnvironment(t, ctx, models.SSHAccessModeIdentity) + _, device := startAcceptedAgent(t, ctx, compose) + + sshid := deviceSSHID(device) + + t.Run("a key no policy names is refused", func(t *testing.T) { + key, signer := newAPIKeyIdentity(t, compose, "ungranted", false) + + mark := compose.ServerLogMark(t) + requireRefusedAtAuth(t, compose, sshid, signer) + compose.AwaitServerLogLine(t, mark, "reason="+string(models.ReasonNoGrant), "user="+key.ID) + }) + + t.Run("a deny policy naming the key beats the allow that names it", func(t *testing.T) { + key, signer := newAPIKeyIdentity(t, compose, "denied", false) + + grant(t, compose, &requests.AccessPolicyCreate{ + Name: "allowed", + Subject: apiKeySubject(key.ID), + Logins: []string{"*"}, + }) + + requireStraightThrough(t, compose, sshid, signer) + + grant(t, compose, &requests.AccessPolicyCreate{ + Name: "denied", + Subject: apiKeySubject(key.ID), + Logins: []string{"*"}, + Action: string(models.PolicyActionDeny), + }) + + mark := compose.ServerLogMark(t) + requireRefusedAtAuth(t, compose, sshid, signer) + compose.AwaitServerLogLine(t, mark, "reason="+string(models.ReasonDeniedByPolicy), "user="+key.ID) + }) + + t.Run("a key's identity is accepted until its expiry and refused after", func(t *testing.T) { + key, signer := enrollAPIKeyIdentity(t, compose, "expiring", new(1), false) + + grant(t, compose, &requests.AccessPolicyCreate{ + Name: "expiring", + Subject: apiKeySubject(key.ID), + Logins: []string{"*"}, + }) + + live := apiKeyIdentity(t, compose, key.Name) + require.NotNil(t, live.ExpiresAt) + require.True(t, live.Active(time.Now()), //nolint:forbidigo // the expiry the server compares against its own wall clock + "an identity a day from its expiry should be live") + + requireStraightThrough(t, compose, sshid, signer) + + expireIdentityAndRequireRefused(t, compose, sshid, signer) + }) + + t.Run("deleting a key revokes its identities", func(t *testing.T) { + key, signer := newAPIKeyIdentity(t, compose, "revoked", false) + + policy := compose.CreateAccessPolicy(t, &requests.AccessPolicyCreate{ + Name: "revoked", + Subject: apiKeySubject(key.ID), + Logins: []string{"*"}, + }) + + requireStraightThrough(t, compose, sshid, signer) + + resp, err := compose.R(t.Context()).Delete("/api/namespaces/api-key/" + key.Name) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode(), resp.String()) + + resp, err = compose.R(t.Context()).Get("/api/access-policies/" + policy.ID) + require.NoError(t, err) + assert.Equal(t, http.StatusNotFound, resp.StatusCode(), "the policy naming the key should go with it: %s", resp.String()) + + unknown := startLogin(t, compose, sshid, signer) + prompt := unknown.awaitApproval(t) + assert.Equal(t, models.SSHApprovalIdentity, prompt.kind, + "the gateway should no longer know the key, and ask to enroll it as a new one") + }) +} From 83b20fb3066be4260ac108ccffe760a0ba760241 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Wed, 7 Oct 2026 08:49:12 -0300 Subject: [PATCH 5/5] test: cover the SHA256 password upgrade on login Every path that sets a password hashes it with bcrypt, so the test writes an unsalted SHA256 digest into the users row to stand in for an account created before bcrypt. A wrong password is refused and leaves that digest alone; the right one logs in, the stored digest becomes a bcrypt hash of the password, and the password keeps working against it. --- tests/environment/user.go | 38 +++++++++++++++++++++++++ tests/password_hash_test.go | 55 +++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 tests/environment/user.go create mode 100644 tests/password_hash_test.go diff --git a/tests/environment/user.go b/tests/environment/user.go new file mode 100644 index 00000000000..b33dde5a3d8 --- /dev/null +++ b/tests/environment/user.go @@ -0,0 +1,38 @@ +package environment + +import ( + "regexp" + "testing" + + "github.com/stretchr/testify/require" +) + +// SetUserPasswordDigest replaces the stored password digest of the user username with digest, +// failing t unless exactly that user changed. Every path that sets a password hashes it with +// bcrypt, so it writes the row directly, standing in for an account created before that. +func (dc *DockerCompose) SetUserPasswordDigest(t *testing.T, username, digest string) { + t.Helper() + + dc.updateOne(t, + "UPDATE users SET password_digest = :'digest' WHERE username = :'username'", + map[string]string{"username": username, "digest": digest}) +} + +var passwordDigestPattern = regexp.MustCompile(`digest=(\S+)`) + +// UserPasswordDigest returns the stored password digest of the user username, reading the row +// directly because no route returns it. It fails t unless psql runs the query and prints a +// non-empty digest, so a missing user and a user with no digest both fail it. +func (dc *DockerCompose) UserPasswordDigest(t *testing.T, username string) string { + t.Helper() + + output, err := dc.stack.SQL(t.Context(), + "SELECT 'digest=' || password_digest FROM users WHERE username = :'username'", + map[string]string{"username": username}) + require.NoError(t, err) + + match := passwordDigestPattern.FindStringSubmatch(output) + require.NotNil(t, match, "psql printed no digest for %s: %s", username, output) + + return match[1] +} diff --git a/tests/password_hash_test.go b/tests/password_hash_test.go new file mode 100644 index 00000000000..0306611e377 --- /dev/null +++ b/tests/password_hash_test.go @@ -0,0 +1,55 @@ +package main + +import ( + "crypto/sha256" + "encoding/hex" + "net/http" + "testing" + + "github.com/shellhub-io/shellhub/tests/environment" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" +) + +// TestLegacyPasswordDigest covers an account whose password is still stored as an unsalted SHA256 +// digest, as accounts created before bcrypt were: the right password logs in and the login rehashes +// it with bcrypt, and a wrong one leaves the digest as it was. +func TestLegacyPasswordDigest(t *testing.T) { + compose := environment.New(t, run).Up(t.Context()) + t.Cleanup(compose.Down) + + const username = "legacy" + + compose.NewUser(t, username, username+"@ossystems.com.br", ShellHubPassword) + + sum := sha256.Sum256([]byte(ShellHubPassword)) + legacy := hex.EncodeToString(sum[:]) + compose.SetUserPasswordDigest(t, username, legacy) + + login := func(t *testing.T, password string) int { + t.Helper() + + resp, err := compose.Anonymous(t.Context()). + SetBody(map[string]string{"username": username, "password": password}). + Post("/api/login") + require.NoError(t, err) + + return resp.StatusCode() + } + + t.Run("a wrong password is refused and leaves the digest alone", func(t *testing.T) { + assert.Equal(t, http.StatusUnauthorized, login(t, "not-"+ShellHubPassword)) + assert.Equal(t, legacy, compose.UserPasswordDigest(t, username)) + }) + + t.Run("the right password logs in and rehashes the digest with bcrypt", func(t *testing.T) { + require.Equal(t, http.StatusOK, login(t, ShellHubPassword)) + + upgraded := compose.UserPasswordDigest(t, username) + require.NoError(t, bcrypt.CompareHashAndPassword([]byte(upgraded), []byte(ShellHubPassword)), + "the stored digest should be a bcrypt hash of the password") + + assert.Equal(t, http.StatusOK, login(t, ShellHubPassword), "the password should keep working against the bcrypt digest") + }) +}