Conversation
Carry opaque tenant-signed CoCo sealed-secret references from SDL 2.x into manifest storage parameters. Reject malformed references and references on non-persistent volumes at the tenant input boundary. Keep the new field absent from legacy Go JSON/YAML and TypeScript manifest objects so existing manifest hashes and SDL output remain unchanged. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Contributor
WalkthroughThe change adds KBS configuration, URI credentials, and sealed-storage ChangesKBS credentials, storage references, and manifest projection
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant SDLInput
participant SDLValidator
participant ManifestGenerator
participant Manifest
SDLInput->>SDLValidator: Validate KBS, URI credentials, and keyRef
SDLValidator->>ManifestGenerator: Provide validated service parameters
ManifestGenerator->>Manifest: Project KBS, URI, and keyRef
Manifest-->>ManifestGenerator: Preserve optional serialization
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 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 |
Reject keyRef values larger than 64 KiB in the Go and TypeScript SDL validators before they can be propagated through manifests and Kubernetes annotations. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ts/src/sdl/validateSDL/validateSDL.ts (1)
10-23: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse one shared coercion for
persistentvalues.
stringToBoolean(str || false)treats any non-false string as persistent, whileisPersistentStorage(computeStorage?.attributes?.persistent)treats"on"or"yes"as persistent and"false"/"no"/"0"as non-persistent. If the two functions disagree on input values, the mount-path check andkeyRefcheck can validate the same attribute differently. Reuse one helper for both persistence checks.🤖 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 `@ts/src/sdl/validateSDL/validateSDL.ts` around lines 10 - 23, Replace the separate persistent-value handling with one shared coercion helper, reusing it for both the mount-path check and the keyRef check. Update isPersistentStorage and the stringToBoolean-based path so all boolean and string inputs, including true/on/yes and false/no/0, produce identical persistence results.
🤖 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.
Nitpick comments:
In `@ts/src/sdl/validateSDL/validateSDL.ts`:
- Around line 10-23: Replace the separate persistent-value handling with one
shared coercion helper, reusing it for both the mount-path check and the keyRef
check. Update isPersistentStorage and the stringToBoolean-based path so all
boolean and string inputs, including true/on/yes and false/no/0, produce
identical persistence results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d78e3209-63cd-403a-8fd0-cd801f1fecca
⛔ Files ignored due to path filters (2)
go/manifest/v2beta3/service.pb.gois excluded by!**/*.pb.gots/src/generated/protos/akash/manifest/v2beta3/service.tsis excluded by!**/generated/**
📒 Files selected for processing (16)
go/manifest/v2beta3/service_test.gogo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/sdl-input.schema.yamlgo/sdl/storage.gogo/sdl/storage_key_ref_test.gogo/sdl/v2.gogo/sdl/v2_1.goproto/provider/akash/manifest/v2beta3/service.protots/script/fix-ts-proto-generated-types.tsts/src/sdl/manifest/generateManifest.spec.tsts/src/sdl/manifest/generateManifest.tsts/src/sdl/types.tsts/src/sdl/validateSDL/validateSDL.spec.tsts/src/sdl/validateSDL/validateSDL.tsts/src/sdl/validateSDL/validateSDLInput.ts
chalabi2
marked this pull request as draft
August 1, 2026 16:00
Allow confidential services to identify registry authentication data by a canonical KBS resource URI while preserving inline credentials for ordinary services. Enforce mutual exclusivity and runtime compatibility in Go and TypeScript SDL builders. Refresh Go, Rust, and TypeScript bindings and cover validation and manifest serialization without changing hashes for existing manifests. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Drop the unused wasmd module so dependency resolution and breaking-change checks use only imports present in the protobuf sources. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
The breaking-change worktree already receives the current vendored protos. Copy the matching Buf dependency manifest and lockfile as well so an unused, unlocked dependency on the base branch cannot make a cold compatibility check fail. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
chalabi2
changed the title
chalabi2
marked this pull request as ready for review
August 4, 2026 19:11
Vendor the imported protobuf sources from the existing locked Buf module revisions. This keeps lint and TypeScript generation deterministic when BSR rejects unauthenticated CI fetches. Generated bindings remain unchanged. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
chalabi2
force-pushed
the
jc/confidential-storage-key-ref
branch
from
August 4, 2026 19:34
3f12ce5 to
191566e
Compare
Provider-only KBS settings force tenants to trust provider-selected endpoints and policies. Add an explicit provider-or-tenant choice to the SDL and manifest. Tenant mode carries only public connection and measured policy inputs. Registry credentials and resource contents remain behind kbs:/// references. Generate the Go, Rust, and TypeScript bindings and validate both modes at the SDL boundary. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
go/sdl/kbs.go (3)
70-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the certificate error message with the enforced rule.
The check enforces only a size bound and rejects NUL and CR. It does not verify PEM structure. The test fixture
"tenant public certificate"passes, so the message "must be a bounded PEM value" describes a constraint that is not applied. Either validate the PEM block or state the actual rule.♻️ Proposed message change
- return errors.New("tenant KBS certificate must be a bounded PEM value") + return errors.New("tenant KBS certificate must be a bounded text value without NUL or CR characters")🤖 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 `@go/sdl/kbs.go` around lines 70 - 72, Update the validation error returned by the tenant certificate check in the KBS parameter validation flow to describe only the enforced size and NUL/CR restrictions, unless PEM parsing is explicitly added. Keep the existing validation conditions unchanged and align the message with the fixture values accepted by this check.
125-134: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMake the mode selection explicit.
Any mode other than
providerproduces a tenant source. If validation is ever bypassed or reordered, an empty mode yields aTenantKBSParamswith empty fields inside the manifest. Switch on both known modes and returnnilfor anything else.♻️ Proposed change
result := &manifest.KBSParams{} - if params.Mode == kbsModeProvider { + switch params.Mode { + case kbsModeProvider: result.Source = &manifest.KBSParams_Provider{Provider: &manifest.ProviderKBSParams{}} - } else { + case kbsModeTenant: result.Source = &manifest.KBSParams_Tenant{Tenant: &manifest.TenantKBSParams{ URL: params.URL, Certificate: params.Certificate, ImageSecurityPolicyURI: params.ImageSecurityPolicyURI, AgentPolicy: params.AgentPolicy, }} + default: + return nil } return result🤖 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 `@go/sdl/kbs.go` around lines 125 - 134, Update the mode selection in the KBS source construction logic to explicitly handle both kbsModeProvider and the known tenant mode, and return nil for any other value, including an empty mode. Preserve the existing provider and tenant manifest construction for their respective modes.
111-115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the loop variable to avoid shadowing the parameter.
The loop variable
valueshadows the function parametervalueand holds a rune, not the URI. Rename it for clarity.♻️ Proposed rename
- for _, value := range strings.TrimPrefix(parts[2], "sha256-") { - if !strings.ContainsRune("0123456789abcdef", value) { + for _, digit := range strings.TrimPrefix(parts[2], "sha256-") { + if !strings.ContainsRune("0123456789abcdef", digit) { return errors.New("image security policy URI must end in a lowercase SHA-256 digest") } }🤖 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 `@go/sdl/kbs.go` around lines 111 - 115, Rename the range loop variable in the SHA-256 validation within the relevant function in go/sdl/kbs.go so it no longer shadows the function parameter value; use a descriptive rune-oriented name and update the strings.ContainsRune call accordingly.ts/src/sdl/validateSDL/validateSDL.spec.ts (1)
757-789: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten these two assertions.
Both tests match any error with the given keyword.
keyword: "required"also matches an unrelated missing property, andkeyword: "additionalProperties"also matches an unrelated extra property. Assert the specific property and instance path so the tests fail if the KBS rule stops firing.💚 Proposed assertions
expect(validate()).toEqual(expect.arrayContaining([ - expect.objectContaining({ keyword: "required" }), + expect.objectContaining({ + keyword: "required", + instancePath: "/services/web/params/kbs", + params: expect.objectContaining({ missingProperty: "certificate" }), + }), ]));expect(validate()).toEqual(expect.arrayContaining([ - expect.objectContaining({ keyword: "additionalProperties" }), + expect.objectContaining({ + keyword: "additionalProperties", + instancePath: "/services/web/params/kbs", + params: expect.objectContaining({ additionalProperty: "url" }), + }), ]));🤖 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 `@ts/src/sdl/validateSDL/validateSDL.spec.ts` around lines 757 - 789, Update the assertions in the incomplete tenant-managed KBS and provider-managed KBS tests to match the specific validation error property and instance path for the KBS rule, rather than only matching the keyword. Ensure the assertions verify the missing tenant-managed field and the disallowed tenant field respectively, so unrelated required or additionalProperties errors cannot satisfy either test.go/sdl/kbs_test.go (1)
100-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the uncovered tenant validation branches.
The table covers URL and policy-URI failures. It does not cover two other tenant rules in
go/sdl/kbs.go: theAgentPolicycontainment check forpackage agent_policy(lines 76-80) and the certificate NUL/CR rejection (line 70). Add one case for each so the branches stay covered.💚 Proposed cases
{ name: "tenant rejects mutable policy URI", params: func() v2ServiceKBSParams { params := validTenant params.ImageSecurityPolicyURI = "kbs:///tenant/security-policy/latest" return params }(), hasTEE: true, wantError: "content addressed", }, + { + name: "tenant rejects agent policy without package declaration", + params: func() v2ServiceKBSParams { + params := validTenant + params.AgentPolicy = "default allow = false\n" + return params + }(), + hasTEE: true, + wantError: "agent_policy document", + }, + { + name: "tenant rejects certificate with control characters", + params: func() v2ServiceKBSParams { + params := validTenant + params.Certificate = "tenant public\rcertificate" + return params + }(), + hasTEE: true, + wantError: "certificate", + },🤖 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 `@go/sdl/kbs_test.go` around lines 100 - 109, Add two table-driven cases alongside the existing tenant validation cases in the test using the visible test-case structure: one with an AgentPolicy that violates the package agent_policy containment rule, and one with a certificate containing NUL or carriage-return data. Set each case’s expected error to match the corresponding validation message and ensure both cases exercise the intended branches in tenant validation.
🤖 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 `@go/sdl/kbs.go`:
- Around line 76-80: The TypeScript KBS validation does not enforce the Go
parser’s AgentPolicy constraints. Update `#validateKBS` to reject policies over 1
MiB, missing the literal “package agent_policy”, or containing
NUL/carriage-return characters, matching the checks in the Go parser;
alternatively, add these constraints to the shared schema so both SDKs apply the
same validation.
In `@make/lint.mk`:
- Around line 39-42: Replace the recursive copy in the lint setup with an
operation that removes or clears the existing $(PROTO_AGAINST_DIR)/third_party
destination before copying the repository’s third_party contents, preserving a
single correctly rooted third_party tree across repeated runs.
In `@third_party/google/protobuf/descriptor.proto`:
- Around line 39-104: Update third_party/README.md to document the
protocolbuffers/protobuf upstream release corresponding to the vendored
descriptor.proto, placing it next to the pinned well-known-types commit and
retaining the existing BUF_VERSION and PROTOC_VERSION details.
In `@third_party/README.md`:
- Around line 7-11: Update the module license statement in the third_party
README: identify buf.build/cosmos/ics23 as having no license header in its
vendored file, buf.build/googleapis/googleapis as Apache-2.0, and
buf.build/protocolbuffers/wellknowntypes as 3-clause BSD. Alternatively, remove
the blanket Apache-2.0 claim and reference each module’s upstream license files.
---
Nitpick comments:
In `@go/sdl/kbs_test.go`:
- Around line 100-109: Add two table-driven cases alongside the existing tenant
validation cases in the test using the visible test-case structure: one with an
AgentPolicy that violates the package agent_policy containment rule, and one
with a certificate containing NUL or carriage-return data. Set each case’s
expected error to match the corresponding validation message and ensure both
cases exercise the intended branches in tenant validation.
In `@go/sdl/kbs.go`:
- Around line 70-72: Update the validation error returned by the tenant
certificate check in the KBS parameter validation flow to describe only the
enforced size and NUL/CR restrictions, unless PEM parsing is explicitly added.
Keep the existing validation conditions unchanged and align the message with the
fixture values accepted by this check.
- Around line 125-134: Update the mode selection in the KBS source construction
logic to explicitly handle both kbsModeProvider and the known tenant mode, and
return nil for any other value, including an empty mode. Preserve the existing
provider and tenant manifest construction for their respective modes.
- Around line 111-115: Rename the range loop variable in the SHA-256 validation
within the relevant function in go/sdl/kbs.go so it no longer shadows the
function parameter value; use a descriptive rune-oriented name and update the
strings.ContainsRune call accordingly.
In `@ts/src/sdl/validateSDL/validateSDL.spec.ts`:
- Around line 757-789: Update the assertions in the incomplete tenant-managed
KBS and provider-managed KBS tests to match the specific validation error
property and instance path for the KBS rule, rather than only matching the
keyword. Ensure the assertions verify the missing tenant-managed field and the
disallowed tenant field respectively, so unrelated required or
additionalProperties errors cannot satisfy either test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f728721-287c-4fd6-823f-a803a36b4f30
⛔ Files ignored due to path filters (6)
buf.lockis excluded by!**/*.lockgo/manifest/v2beta3/service.pb.gois excluded by!**/*.pb.gors/src/gen/node/akash.manifest.v2beta3.rsis excluded by!**/gen/**rs/src/gen/node/akash.manifest.v2beta3.serde.rsis excluded by!**/gen/**ts/src/generated/protos/akash/manifest/v2beta3/service.tsis excluded by!**/generated/**ts/src/generated/protos/index.provider.akash.v2beta3.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
buf.yamldocs/proto/provider.mdgo/sdl/_testdata/v2.1-tee-kbs-provider.yamlgo/sdl/_testdata/v2.1-tee-kbs-tenant.yamlgo/sdl/groupBuilder_v2.gogo/sdl/groupBuilder_v2_1.gogo/sdl/kbs.gogo/sdl/kbs_test.gogo/sdl/sdl-input.schema.yamlgo/sdl/storage_key_ref_test.gogo/sdl/v2.gogo/sdl/v2_1.gomake/lint.mkproto/provider/akash/manifest/v2beta3/service.protothird_party/README.mdthird_party/cosmos/ics23/v1/proofs.protothird_party/google/api/annotations.protothird_party/google/api/http.protothird_party/google/protobuf/any.protothird_party/google/protobuf/descriptor.protothird_party/google/protobuf/duration.protothird_party/google/protobuf/empty.protothird_party/google/protobuf/timestamp.protots/src/sdl/manifest/generateManifest.spec.tsts/src/sdl/manifest/generateManifest.tsts/src/sdl/validateSDL/validateSDL.spec.tsts/src/sdl/validateSDL/validateSDL.tsts/src/sdl/validateSDL/validateSDLInput.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- go/sdl/groupBuilder_v2_1.go
- go/sdl/groupBuilder_v2.go
- go/sdl/storage_key_ref_test.go
- ts/src/sdl/validateSDL/validateSDLInput.ts
The TypeScript validator accepted tenant agent policies that the Go parser rejected. SDK behavior therefore depended on the parser used. Enforce the same UTF-8 byte limit, package declaration, and control character rules in TypeScript. Cover the contract in both SDK test suites. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
The vendored module list incorrectly described every dependency as Apache-2.0 and omitted the upstream protobuf release. Record each upstream repository license and identify the well-known types as the exact contents of protocolbuffers/protobuf v33.2. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go/sdl/kbs_test.go (1)
132-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the same precise UTF-8 byte boundary in both bindings.
The Go fixture is larger than 1 MiB. It rejects under either a 512 KiB or 1 MiB limit. It also exceeds 512 Ki runes, so it does not prove byte-based counting. The TypeScript test only rejects data above 1 MiB.
Add accepted-at-limit and rejected-over-limit cases for the same configured byte limit in both suites. Use a multibyte Go fixture with fewer runes than the limit but more UTF-8 bytes than the limit.
go/sdl/kbs_test.go#L132-L140: add exact 512 KiB boundary cases and a multibyte byte-versus-rune case.ts/src/sdl/validateSDL/validateSDL.spec.ts#L839-L843: use the same boundary values as Go to enforce cross-binding parity.🤖 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 `@go/sdl/kbs_test.go` around lines 132 - 140, Update go/sdl/kbs_test.go lines 132-140 to cover an accepted policy at exactly 512 KiB, a rejected policy just over 512 KiB, and a multibyte UTF-8 fixture with fewer than 512 KiB runes but more than 512 KiB bytes; update ts/src/sdl/validateSDL/validateSDL.spec.ts lines 839-843 with matching 512 KiB boundary values so both bindings enforce identical byte-based limits.
🤖 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.
Nitpick comments:
In `@go/sdl/kbs_test.go`:
- Around line 132-140: Update go/sdl/kbs_test.go lines 132-140 to cover an
accepted policy at exactly 512 KiB, a rejected policy just over 512 KiB, and a
multibyte UTF-8 fixture with fewer than 512 KiB runes but more than 512 KiB
bytes; update ts/src/sdl/validateSDL/validateSDL.spec.ts lines 839-843 with
matching 512 KiB boundary values so both bindings enforce identical byte-based
limits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6ed1193c-97f6-47e1-b7c5-71775e48cca5
📒 Files selected for processing (4)
go/sdl/kbs_test.gothird_party/README.mdts/src/sdl/validateSDL/validateSDL.spec.tsts/src/sdl/validateSDL/validateSDL.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- third_party/README.md
- ts/src/sdl/validateSDL/validateSDL.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Confidential workloads need two opaque tenant-to-guest references without giving the Provider secret custody:
What changed
keyRefto persistent storage parametersurito image credentialskbs:///repo/type/tagform with bounded ASCII segmentsChain SDK validates transport shape only. It never verifies a JWS, retrieves a KBS resource, or handles credential bytes.
Validation
Compatibility and security
Ordinary registry credentials are unchanged. Confidential services cannot place inline credentials into initdata. References larger than their boundary limits are rejected before manifest propagation.
The guest remains responsible for signed-reference verification, KBS retrieval, and secret use. Provider integration is in akash-network/provider#427.