Repository navigation
v0.1.0 readiness: release pipeline, operator-plane proto contract, storage units, SDK parity - #589
Conversation
…nfig in CI cmd/sam-box and cmd/nano-init were removed in f6abfb8, after v0.1.0-rc.8, but .goreleaser.yaml still built both, so the goreleaser job on the next tag would fail before any binary or SDK package was published. install.sh listed both binaries as well. The test workflow now runs goreleaser check and a single-target snapshot build so a stale build id fails on the pull request, not on the tag. The chart appVersion follows the release line (0.1.0).
The reference, security architecture and concept pages described an --ext-authz-addr flag and the extraction of a SPIFFE principal from AttributeContext.Source.Principal or X-Forwarded-Client-Cert. sam-node has neither: the Envoy endpoints are served on the local API listeners, and the caller presents a Biscuit or a platform JWT that sam-node exchanges at the control plane. The pages now name the headers the code reads and returns.
/admin/status, /user/status, /admin/bootstrap-tokens, /user/bootstrap-tokens and /admin/enrollments encoded Go structs with json tags, internal/storage records and map[string]any. AGENTS.md reserves every SAM-defined surface for messages in api/sam.proto, and after v0.1.0 the proto is additive-only, so this is the last release that can change these shapes. New messages: BootstrapTokenCreateRequest/Response, BootstrapToken and BootstrapTokenListResponse, EnrollmentRequest and EnrollmentRequestListResponse, User, EnrolledNode, RouterLease, NodeServices, AdminStatusResponse and UserStatusResponse. Instants are Timestamps named *_time; the policy travels as an embedded PolicyConfig instead of a JSON string; the enrollment status is the enum name. The handlers read and write protojson with proto field names and reject unknown fields. Field names of the request body (role, ttl_hours, max_usages, description, owner_id, autonomous_recovery) and the `token` in the response are unchanged, so the Helm bootstrap job, deploy.yaml and the bats suites keep working. /admin/status no longer carries each enrolled node's biscuit and public key; the integration tests that compared the issued biscuit now read it from the control plane's own store. api/bootstrap_token.go is removed; sam-one, the console and the tests use the generated bindings. SDK bindings regenerated.
bootstrap_tokens, enrollment_requests, users and banned_identities held unix seconds while every other table held milliseconds. Each table was consistent with itself, so nothing was wrong on the wire, but a query that compared one of these columns against a millisecond clock would have been off by a factor of a thousand with no test to notice. Migration 15 multiplies the existing rows by 1000 behind a magnitude guard (10^11 is the year 5138 in seconds and 1973 in milliseconds), so a row is converted exactly once and a database that already holds milliseconds is left alone. The round-trip test now compares at millisecond precision, so a table falling back to seconds fails there.
The bats helpers selected enrolled nodes and routers by the Go field names the control plane no longer serializes.
The control plane redeems only the last biscuit it issued, so two refreshes in flight leave the loser holding a spent one and the member re-enrolling. sam-node holds refreshMu and the Python SDK a lock; the JS SDK let a scheduled refresh and a manual one race. refresh() now queues behind the refresh in flight, and a failed refresh does not block the next. The test drives eight concurrent refreshes against a fake control plane that, like the real one, rejects a refresh presenting a stale biscuit.
…e, console warning
- The control plane's Close drained its HTTP server without a deadline; a
stuck connection held the pod until the kubelet killed it. The drain is
now bounded to 20s, under the default termination grace period.
- /sam/identity and /sam/peer/{id}/evidence rendered protojson with
lowerCamelCase names while every other SAM JSON surface uses proto field
names; they now match.
- Only sam-node reported its version. sam-control-plane, sam-router, sam-one,
sam-console and mcp-client answer --version, every goreleaser build and
every image build stamps the tag, and mcp-client identifies itself to MCP
servers with the build version instead of a constant.
- sam-console warns at startup when --external-url is unset, since the OIDC
redirect_uri and the cookie Secure flag then follow request headers.
- A ban wrote the node row and the identity row in two statements; a failure between them left the device banned while its owner could enroll a new one, or the reverse on unban. SetNodeAndIdentityBanned does both in one transaction and SetNodeBan uses it. - Migration 16 indexes the scans that run on every bootstrap (/info's ban set), every lease lookup, every revocation pull and the node GC. nodes is the one table that grows with the mesh. - The in-memory revocation set dropped expired entries only when /revocations was read; it now drops them on write as well.
…k grammar - The inference facade served a local model without any authorization when its authorizer seam was unset. Missing wiring is now a 503, the same convention as the provider label verifier. - sam-control-plane warns at startup when --insecure-skip-tls-verify is set. - Both SDKs refuse an appended block that carries a rule, a check or a second fact beside tar_block; the tests now say so explicitly.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the operator plane API to use protobuf-defined messages serialized via protojson with proto field names, replacing ad-hoc JSON structs and updating the console frontend, SDKs, CLI commands, and tests accordingly. It also introduces database schema migrations (converting timestamps to milliseconds and adding indexes), adds an atomic node and identity banning transaction, enforces local authorization checks in the OpenAI facade, and implements concurrent refresh serialization in the JS SDK. A review comment highlights a performance bottleneck in internal/controlplane/sts.go where pruning expired revoked biscuits on every write results in an O(N) map scan, suggesting a periodic or deferred cleanup strategy instead.
Review feedback on google#589: a full scan of the in-memory revocation set on every write is O(N) in live revocations. The sweep now runs when the set reaches a threshold that doubles after each pass, so a write costs O(1) amortized and the set never holds more than twice its live entries.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request transitions the operator plane (/admin/* and /user/*) to use official protobuf messages defined in api/sam.proto (such as AdminStatusResponse and UserStatusResponse) for consistent wire shapes and schema enforcement. It also introduces database migrations to standardize timestamps to milliseconds and add indexes on frequently scanned columns, implements atomic node and identity banning, and ensures the OpenAI facade fails closed if local authorization is missing. Feedback on these changes suggests validating that OIDC claims yield a valid identity key during node banning to prevent silent bypasses, and explicitly returning a 413 Request Entity Too Large status code when the request body exceeds the maximum allowed size.
| if err := json.Unmarshal([]byte(node.ClaimsJSON), &claims); err != nil { | ||
| return fmt.Errorf("stored claims for %s are unreadable: %w", node.PeerID, err) | ||
| } | ||
| identity = oidcIdentityKey(claims) |
There was a problem hiding this comment.
If node.ClaimsJSON is not empty (meaning the node was enrolled via OIDC), but oidcIdentityKey(claims) returns an empty string due to missing or malformed claims, identity will be empty. This means store.SetNodeAndIdentityBanned will only ban the node and silently skip banning the identity, allowing the user to potentially bypass the ban by enrolling a new node under the same identity. We should explicitly validate that a non-empty ClaimsJSON yields a valid identity key.
identity = oidcIdentityKey(claims)
if identity == "" {
return fmt.Errorf("stored claims for %s do not contain a valid identity key", node.PeerID)
}There was a problem hiding this comment.
Right diagnosis, but the fix belongs one step earlier. Returning an error here would run before the transaction, so a record whose claims carry no subject could not be banned at all, including the device half; the identity half is already unreachable because an identity without sub has no key to match on at /register either.
The gap was that /register accepted a JWT without sub. f8f4f03 refuses such a token (401), so every OIDC-enrolled record has a bannable identity key. SetNodeBan keeps banning the device for a record from before that check and logs a warning instead of staying silent. Covered by TestAuthDenialPaths/JWT_without_a_subject_is_rejected.
…rsized bodies Review feedback on google#589. Bans and ownership are keyed on issuer|subject, so a token without a sub claim would enroll an identity that can never be banned; /register now refuses it (OIDC ID tokens, Kubernetes service account tokens and JWT-SVIDs all carry one). SetNodeBan still bans the device when a record from before that check yields no identity key, and says so in the log instead of silently. readProtoJSON answers 413 rather than 400 when the body exceeds the limit.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the operator plane API to use structured Protobuf messages and 'protojson' serialization instead of ad-hoc JSON structures or internal storage types. It removes the 'sam-box' and 'nano-init' binaries, adds a '-version' flag to all main commands, and implements version injection during builds. On the storage side, migrations 15 and 16 are added to convert timestamp storage from seconds to milliseconds and to introduce database indexes for performance. Additionally, a new atomic 'SetNodeAndIdentityBanned' transaction is introduced to prevent inconsistent ban states, local authorization in the OpenAI facade is updated to fail closed if unconfigured, and concurrent SDK token refreshes are serialized to prevent race conditions. Documentation and tests have been updated accordingly. I have no feedback to provide as no review comments were submitted.
Review feedback on #589: a full scan of the in-memory revocation set on every write is O(N) in live revocations. The sweep now runs when the set reaches a threshold that doubles after each pass, so a write costs O(1) amortized and the set never holds more than twice its live entries.
1 participant
Pre-release review of
mainbefore v0.1.0: seven analysis passes (security, API contract, control plane, dataplane, SDK parity, tests/CI, docs) plus a build/lint/test baseline. Every finding below was verified against the code before it was fixed; the false positives from the review are listed at the end so nobody re-chases them.Release blockers
.goreleaser.yamlstill builtsam-boxandnano-init, whosecmd/directories were removed after rc.8, so the tag job would have failed before any binary, npm or PyPI package was published.install.shlisted both binaries too. The test workflow now runsgoreleaser checkand a single-target snapshot build on every PR. ChartappVersionfollows the release line.api/sam.protomessages./admin/status,/user/status,/admin/bootstrap-tokens,/user/bootstrap-tokensand/admin/enrollmentsencoded Go structs with json tags,internal/storagerecords andmap[string]any, which AGENTS.md forbids on a SAM-defined surface, and which the additive-only promise after v0.1.0 would have frozen. New messages:BootstrapTokenCreateRequest/Response,BootstrapToken,EnrollmentRequest,User,EnrolledNode,RouterLease,NodeServices,AdminStatusResponse,UserStatusResponse, with*_timeTimestamps and the policy embedded asPolicyConfig. Request bodies and thetokenresponse field keep their names, so the Helm bootstrap job,deploy.yamland the bats suites are unchanged./admin/statusno longer ships each node's live biscuit and public key to the browser.api/bootstrap_token.gois gone;sam-one, the console and the tests use the generated bindings.--ext-authz-addrflag and XFCC/SPIFFE principal extraction that do not exist. The reference, the security architecture page and three concept pages now describe the credential path the code implements.Storage
Correctness and parity
refresh()runs one refresh at a time, assam-node'srefreshMuand the Python SDK's lock already did; the control plane redeems only the last biscuit it issued.Closeis bounded (20s); the in-memory revocation set prunes on write./sam/identityand/sam/peer/{id}/evidenceuse proto field names like every other SAM JSON surface.--versionon every binary; every goreleaser build and every image stamps the tag.sam-consolewarns when--external-urlis unset;sam-control-planewarns when--insecure-skip-tls-verifyis set.tar_blockis refused.Review findings that turned out to be wrong
protojson.Unmarshaldrops unknown policy fields": the default rejects them.tar_block, i.e. an unattenuated token under full Block 0 RBAC.math/randused for tokens": used for router shuffling only.Operator notes for the release
peer_id,expire_time,policy, ...). Scripts that only readtokenfrom a mint response are unaffected.Verification
make build,go vet,golangci-lint, deadcode gate,verify-generated.sh,verify-sdk-generated.sh, unit tests with-race, the full integration suite, JS SDK 125/125, Python SDK 136/136, Playwrightmake ui-test21/21,goreleaser check+ snapshot build (all six binaries report the tag).