🐛 Fail-closed token revocation on intermediate stage failure - #133
Conversation
…konveyor#109) Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
Assisted-By: Claude Code <noreply@anthropic.com> Signed-off-by: Savitha Raghunathan <saveetha13@gmail.com>
📝 WalkthroughWalkthroughThe migration harness now validates Hub token IDs, revokes valid tokens on workflow exit, and defers revocation only after successful intermediate stages. Tests cover parsing, stage detection, and revocation decisions. A changelog entry documents the behavior. ChangesToken revocation lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MigrationHarness
participant WorkflowStage
participant HubAPI
MigrationHarness->>HubAPI: Parse and validate token ID
MigrationHarness->>WorkflowStage: Run migration stage
WorkflowStage-->>MigrationHarness: Return success or failure
MigrationHarness->>HubAPI: Revoke token unless successful intermediate stage
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@harness/cmd/migration-harness/main.go`:
- Around line 452-456: Update the Hub token ID parsing in the relevant helper in
main.go to use strconv.IntSize instead of a fixed 64-bit size, rejecting values
that exceed the target uint range before conversion. Add boundary tests in
main_test.go covering the maximum uint value and the value immediately above it,
including 32-bit behavior where applicable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8404ad0-3103-4dd8-9f8c-8860fedc5440
📒 Files selected for processing (3)
changes/unreleased/109-fail-closed-token-revocation.yamlharness/cmd/migration-harness/main.goharness/cmd/migration-harness/main_test.go
| tokenID, err := strconv.ParseUint(cfg.HubTokenID, 10, 64) | ||
| if err != nil { | ||
| return 0, false | ||
| } | ||
| if cfg.WorkflowStage == "" && cfg.WorkflowStageCount == "" { | ||
| return uint(tokenID), true | ||
| return uint(tokenID), true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect declared build targets. A 32-bit target makes the uint64-to-uint
# conversion lossy for token IDs above math.MaxUint32.
rg -n -i \
--glob 'Dockerfile*' \
--glob '*.yaml' \
--glob '*.yml' \
--glob '*.json' \
--glob '*.sh' \
'GOARCH|GOOS|--platform|architecture' .
# Show the parser, its tests, and the downstream uint API contract.
rg -n -C 4 'func parseHubTokenID|ParseUint|func \(c \*Client\) RevokeToken|func TestParseHubTokenID' \
harness/cmd/migration-harness/main.go \
harness/cmd/migration-harness/main_test.go \
harness/internal/hub/client.goRepository: konveyor/agentic-controller
Length of output: 6771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== parser and tests =="
sed -n '440,505p' harness/cmd/migration-harness/main.go
echo
sed -n '75,95p' harness/cmd/migration-harness/main_test.go
echo
echo "== Dockerfile multiarch build context =="
sed -n '1,35p' Dockerfile
echo
echo "== runtime semantics probe =="
cat > /tmp/probe.go <<'EOF'
package main
import (
"fmt"
"strconv"
)
func main() {
for _, s := range []string{"281474976710656", "42"} {
n, err := strconv.ParseUint(s, 10, 64)
if err != nil {
panic(err)
}
u := uint(n)
fmt.Printf("%s -> uint64=%d -> uint=%d (overflow=%t)\n", s, n, u, n != uint64(u))
}
}
EOF
go run /tmp/probe.go
rm /tmp/probe.goRepository: konveyor/agentic-controller
Length of output: 4171
🌐 Web query:
Go strconv.IntSize documentation
💡 Result:
In the Go programming language, strconv.IntSize is a constant that represents the size in bits of an int or uint value on the machine where the code is currently running [1][2]. Because the Go int and uint types have a size that is platform-dependent (typically 32 bits on 32-bit systems and 64 bits on 64-bit systems), this constant allows developers to programmatically determine the bit size of these types at runtime [2][3]. It is defined as a constant within the standard library's strconv package [1]. You can access it as follows: import "strconv" fmt.Println(strconv.IntSize) // Outputs 32 or 64 depending on the architecture [2][3]
Citations:
- 1: https://pkg.go.dev/strconv
- 2: https://www.educative.io/answers/what-is-the-strconvintsize-constant-in-golang
- 3: https://how.dev/answers/what-is-the-strconvintsize-constant-in-golang
Reject Hub token IDs that overflow the target uint.
strconv.ParseUint(..., 10, 64) accepts values larger than uint on 32-bit targets. In a 32-bit build, a parsed ID truncates when converted to uint, so the harness can revoke the wrong token. Parse with strconv.IntSize and add boundary tests around the maximum uint value and one value above it.
📍 Affects 2 files
harness/cmd/migration-harness/main.go#L452-L456(this comment)harness/cmd/migration-harness/main_test.go#L82-L85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@harness/cmd/migration-harness/main.go` around lines 452 - 456, Update the Hub
token ID parsing in the relevant helper in main.go to use strconv.IntSize
instead of a fixed 64-bit size, rejecting values that exceed the target uint
range before conversion. Add boundary tests in main_test.go covering the maximum
uint value and the value immediately above it, including 32-bit behavior where
applicable.
ibolton336
left a comment
ibolton336
left a comment
There was a problem hiding this comment.
Verified the semantics this leans on; all hold on main:
runStagehas exactly one success exit andstageSucceeded = truesits immediately before it — every other path returns an error and revokes. Viewer-cancel lands on the failure path too, which is right since the workflow run dies with it.- The workflow controller fails the entire run on a failed stage (never creates stage N+1), and sandbox pods run
RestartPolicy: Never— so nothing today can retry a stage into a revoked token. Harness tests pass locally at this head.
Two non-blocking notes:
- The sandbox pod spec comment reserves "bounded retry can be added later" (agentrun_controller.go ~388). When that lands, revoke-on-failure fights it — a retried stage boots with a dead token. Worth one breadcrumb line in the fail-closed comment here tying it to the no-retry semantics (the coupling predates this PR for standalone runs; this widens it to workflow stages).
- This closes the in-process exit paths, but two #109 paths still leak until TTL: a pod killed without defers running (OOM/node loss), and an intermediate success where the next stage never actually runs (create failure, run deleted between stages). Only a controller/Hub-side janitor covers those — is that tracked anywhere, or should #109 stay partially open rather than auto-close?
Nit: TestTokenRevocationDecision recomputes !(intermediate && tt.stageSucceeded) — the same expression as the defer — so it can't catch a regression in the real decision. Extracting the condition into a named func and testing that would make it bite.
3 participants
Summary by CodeRabbit
Bug Fixes
Tests