Repository navigation
test: cover SSH identity mode end to end - #7363
Conversation
otavio
changed the title
|
Claude finished @otavio's task in 8m 51s —— View job Code Review CompleteReviewed 10 files. 3 inline issues posted. Standards: 0 violations, 2 judgement calls. Spec: 0. Correctness: 1. StandardsNo rule is broken. Every comment the branch adds is a doc comment on an exported declaration or a CorrectnessThe window-boundary subtest leaves the straight-through login only 5 seconds before the server's re-authentication check flips. On a slow CI runner that subtest can fail when nothing is wrong ( SpecThe Spec axis did not run. The PR closes no issue, and none of its commits carry an issue reference, so there was nothing to review against. Additional notesI dropped several lower-value judgement calls rather than posting them: data clumps in the login helpers, the hand-built enroll request, assertions on free-text log lines, and the wording of the To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| requireReauth(t, compose, "minutely", new(period)) | ||
| signer, fingerprint := enrollOwnerKey(t, compose) | ||
|
|
||
| compose.AgeSSHIdentityReauth(t, fingerprint, (period-5)*time.Second) |
There was a problem hiding this comment.
Correctness: the straight-through login only has 5 seconds of margin. Back-dating last_reauth_at to period-5 leaves 5s for the identityByFingerprint GET, the TCP dial and the full SSH handshake before the server runs its >= check (server/ssh/session/auther.go:312). If that path takes longer than 5s on a loaded CI runner, the server asks for a re-authentication. requireStraightThrough then waits on a challenge nobody answers and fails spuriously.
The sleep on line 60 is computed from reauthedAt, the value read back from the database, so widening the margin only needs this constant to change:
| compose.AgeSSHIdentityReauth(t, fingerprint, (period-5)*time.Second) | |
| compose.AgeSSHIdentityReauth(t, fingerprint, (period-20)*time.Second) |
| func grant(t *testing.T, compose *environment.DockerCompose, req *requests.AccessPolicyCreate) models.AccessPolicy { | ||
| t.Helper() | ||
|
|
||
| policy := compose.CreateAccessPolicy(t, req) | ||
| t.Cleanup(func() { compose.DeleteAccessPolicy(t, policy.ID) }) | ||
|
|
||
| return policy | ||
| } |
There was a problem hiding this comment.
Standards (judgement call), Fowler Mysterious Name: grant creates any access policy and registers its deletion, whatever the action. One of the 14 callers passes Action: deny, so identity_policy_test.go:178 reads deny := grant(...). That call site says the opposite of what it does. A neutral name such as createPolicy or withPolicy would read correctly at all 14 call sites.
| func readServerLog(ctx context.Context, source LogSource) (string, error) { | ||
| reader, err := source.Logs(ctx) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| defer func() { _ = reader.Close() }() | ||
|
|
||
| logs, err := io.ReadAll(reader) | ||
|
|
||
| return string(logs), err | ||
| } |
There was a problem hiding this comment.
Standards (judgement call), Fowler Duplicated Code: readServerLog repeats the open → defer Close → io.ReadAll sequence that AwaitLogContains already has inline (docker_compose.go:142-151). Both are in package environment, so AwaitLogContains can call this function and the log-reading path lives in one place:
logs, err := readServerLog(t.Context(), source)
if !assert.NoError(tt, err) {
return
}
assert.Contains(tt, logs, substr)A name that doesn't say "server" would then fit better too, since LogSource is not tied to the server.
153ab77 to
a911608
Compare
a911608 to
d633cd8
Compare
An expired identity and a consumed single-use one are both refused when the key is offered, and neither is sent back through enrollment: the key is known, only dead. A single-use identity belongs to an API key, so that case grants the key through an api-key policy first. Every connection stamps the identity's last use, and a member's key is decided as that member, which a policy granting only the owner then refuses. The API sets an identity's expiry only in whole days ahead, so ExpireSSHIdentity back-dates it in SQL, a minute into the past so the server's own clock is past it too. AgeSSHIdentityLastUse moves the last-use stamp a day back, so the next connection's stamp cannot be mistaken for it. These subtests share one stack, so a server log line another subtest caused would satisfy a plain substring search. ServerLogMark records how much the server has logged, and AwaitServerLogLine reads only what follows and wants every field on one line, which ties a denial reason to the user it was about. The cleanups that delete a policy or untag a device run on context.WithoutCancel(t.Context()) because the test's context is cancelled before cleanups run. startLogin drives a login the way a person at an interactive client does: it reads every keyboard-interactive prompt and answers only the approval one, leaving the denial prompt unanswered as the gateway expects. Its context is detached from the test and bounded at three approval waits, so a login parked on a prompt outlives the subtest's assertions and the cleanup cancels it and waits for the dial to return. ssh.NewClientConn ignores its context, so handshake, now shared with dialSSH, closes the connection when the context ends. result puts the outcome back on its channel so it can be read again. CreateAccessPolicy now returns the policy it created, so a subtest can delete it by id when done.
…lifts Each case connects as a member or API key that only its own policies grant, so the owner's starter policy never decides it and the shared stack needs no policy deleted from under it. A non-member cannot hold an identity: ssh_identities references memberships and cascades on removal. The only way a non-member reaches the policy evaluation is a login parked on an approval, so a member confirms it and is removed before the terminal answers. The server then logs not_a_member for that user and the client is told an access policy does not allow the login, even though an every-member policy would grant a member. The source address case reads the address the gateway saw from the session an owner login opens, because the client's address depends on how the host routes into the stack. It is unmapped before it becomes a prefix, as the server unmaps it before matching. 203.0.113.0/24 is TEST-NET-3, which no client comes from. The API refuses a source entry that is not an address, so BreakAccessPolicySourceIP writes one in SQL to stand in for a row that went bad. The same deny that does not apply while readable refuses the owner once it cannot be evaluated.
A re-authentication is completed the way the console does it, by posting the owner's password, the key's fingerprint and the approval code to /api/web-terminal/reauth, and typing the confirmation code it returns at the terminal. AgeSSHIdentityReauth back-dates the last re-authentication in SQL, standing in for the hour a window takes to lapse. The window boundary case does not: it back-dates the stamp to five seconds short of a one-minute period, logs in straight through, then sleeps until a second past the period and is asked again. The server compares with >=, so the exact instant cannot be hit from outside; the two logins sit on either side of it within the same window. An API key subject with require_reauth goes straight through and is never stamped, which is the server dropping the requirement for anything that is not a person. With two periods the shorter decides, and a policy with no period outweighs any period.
The approval window is the server's SSHApprovalTTL constant, so ExpireSSHApproval back-dates the row in SQL. One second is enough: the lookup compares expires_at with the server's clock, which shares the database host's. The terminal then answers with 23456789, a code in the pairing alphabet that is not the confirmation, so the refusal comes from the expiry or the rejection rather than from a malformed answer. A confirmation sends only expires_in. Marshalling requests.SSHApprovalConfirm also sends its untagged Code field empty, which binds over the path parameter and fails validation. Re-enrolling is reached by two logins with the same unknown key, each parked on its own approval. Confirming both as the owner binds the key once and lets both in. Confirming the second as another member fails with 409 and leaves that approval pending, because the identity and the decision are written in one transaction. An observer holds neither approve permission and gets 403 on confirm and reject. A user of another namespace gets 404 on read, confirm and reject, so a code alone discloses nothing.
Proving the factor moves a day-old last re-authentication to now and marks the approval confirmed. The identity enrollment endpoint answers 403 for a re-authentication approval and leaves it pending, and the proper step-up then releases the same login. Past its window, the step-up answers 404 and the identity's last re-authentication stays empty: the stamp and the release are one transaction, so a step-up that released nothing refreshes nothing.
d633cd8 to
569b0b0
Compare
1 participant
Summary
Covers Domain 13 (SSH Identity Mode) of shellhub-io/team#243 with testcontainers tests: 29 subtests over 28 of the 29 open items. Each slice runs on one shared stack.
Each refusal asserts how the server decided it, not just that the login failed:
reason=not_a_member user=<id>)Expiry and freshness cases back-date the timestamp in SQL. The window-boundary case also waits out the window in real time. No case deletes a row to fake an expiry.
A non-member can never hold an identity (
ssh_identitiesreferencesmembershipsand cascades), so "Non-member is refused" parks a login on an approval. A member confirms it and is removed before the terminal answers.dialSSHnow shares its handshake with the new interactive dialer.CreateAccessPolicyreturns the policy it created.Not covered: "Approval concurrency limit". The per-source cap from 98d60a1 was removed by d22b28d ("drop the per-source cap on parked approval waits"), so master has nothing to test.
Evidence
Before: the 28 items had no test. "Non-member is refused" was unticked by the 2026-10-02 audit because the only refusal case was a member with no grant.
After: every new test passes, run one top-level test per process:
The tests fail when the behaviour breaks. In two rounds I mutated the server code each subtest guards, then reverted it. Every subtest failed except the cross-principal one, which has no practical mutation and asserts
user=<member id>directly. Some of the mutations:identity.ActivefromResolveKeyAuthcontinuestricterReauthPeriodall-membersfor API keysSessionApprovecheckreleaseSSHApprovalsucceed on a lapsed codeMerge Danger
Door: two-way
Test-only. Nothing under
server/,pkg/orui/changes.Blast Radius: CI time
Five more stacks in the
validate (tests - postgres)job, about 45 s each.