Conversation
Carry the optional tenant-signed keyRef through the Provider CRD and manifest conversions, with the same shape and size bounds enforced by chain-sdk. The temporary module replacement keeps this review buildable until akash-network/chain-sdk#352 is released. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Render deterministic Kata initdata for sealed environment values and tenant-signed persistent-volume key references. Confidential persistence uses Block PVCs and reserved OCI device paths so Kata and CDH own key verification and non-destructive LUKS handling. Provider configuration contains only the public KBS endpoint, public certificate, image policy URI, and measured agent policy; it never accepts a DEK or KBS administrator credential. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change adds confidential-compute configuration and manifest fields. It generates validated, compressed initdata, supports sealed persistent volumes, handles KBS registry credentials, and integrates these values into workload and deployment resources. ChangesConfidential compute workload flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant WorkloadBuilder
participant SecureStorage
participant CCInitData
participant Kubernetes
ProviderConfig->>WorkloadBuilder: provide validated CC settings
WorkloadBuilder->>SecureStorage: validate sealed volumes and derive IDs
WorkloadBuilder->>CCInitData: build and encode initdata
CCInitData->>Kubernetes: add initdata annotations
SecureStorage->>Kubernetes: add block devices and block-mode PVCs
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 |
Carry the SDL KBS resource URI through the manifest CRD and measure it into the guest CDH configuration. Confidential services reject inline registry credentials, and URI-backed authentication never creates or references a host Kubernetes Secret. Keep the existing inline credential path unchanged for ordinary services and validate the canonical kbs:///repo/type/tag form at the Provider boundary. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
Derive versioned volume identities from lease, service, volume, and signed payload data so re-signing does not orphan an existing LUKS volume. Length-prefix every identity component to prevent ambiguous encodings. Fail closed for replicas, require an operator allowlist of sanitized Block storage classes, and replace untyped initdata maps with explicit descriptor types. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
chalabi2
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
cluster/kube/builder/cc_registry_credentials_test.go (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover confidential credentials through
NewWorkloadBuilder.The production construction path initializes
registryCredentialsURI,ccInitDataAnnotation, andsecretsRefs; current credential tests bypass that path by assigning resolver results directly. Add aNewWorkloadBuilderregression that uses a CBS URI in the manifest and asserts the CDH contains the credential URI, no initdata auth username/password, noimagePullSecrets, no Kubernetes credential Secret, and noImagePullCredentials.🤖 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 `@cluster/kube/builder/cc_registry_credentials_test.go` around lines 18 - 20, Extend the credential regression tests to construct the workload through NewWorkloadBuilder rather than assigning confidential credential resolver results directly. Use a manifest containing a CBS URI, then assert the generated CDH preserves the credential URI while omitting initdata auth username/password, imagePullSecrets, the Kubernetes credential Secret, and ImagePullCredentials.cluster/kube/builder/cc_persistent_storage_test.go (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppress the secret-scanner finding on this fixture.
The scanner reports a JWT at this line. The value is synthetic: the payload decodes to a non-secret JSON object and the signature segment decodes to the literal
signature. No key material is present. Add an inline allow directive so the scan does not fail on a test fixture.♻️ Proposed suppression
-const testSealedKeyRef = "sealed.eyJhbGciOiJFUzI1NiJ9.eyJuYW1lIjoia2JzOi8vL2RlZmF1bHQvdGVzdC9zaGEyNTYtMDAifQ.c2lnbmF0dXJl" +// Synthetic sealed reference for tests. Not a credential. +const testSealedKeyRef = "sealed.eyJhbGciOiJFUzI1NiJ9.eyJuYW1lIjoia2JzOi8vL2RlZmF1bHQvdGVzdC9zaGEyNTYtMDAifQ.c2lnbmF0dXJl" // gitleaks:allowConfirm the directive syntax your scanner honors before applying it.
🤖 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 `@cluster/kube/builder/cc_persistent_storage_test.go` at line 32, Add an inline secret-scanner allow directive to the testSealedKeyRef fixture, using the exact suppression syntax supported by the repository’s scanner. Keep the synthetic token unchanged and limit the suppression to this single fixture line.Source: Linters/SAST tools
cluster/kube/builder/settings.go (2)
146-158: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReject a trailing slash in
KBSURLwith a clearer message.
url.Parse("https://kbs.example.com/")setsPathto"/". The current check then fails with "must not contain credentials, a path, query, or fragment". A trailing slash is a common operator input. Normalize it before the check, or extend the error text to name it.♻️ Proposed normalization
kbsURL, err := url.Parse(settings.KBSURL) if err != nil || kbsURL.Scheme != "https" || kbsURL.Host == "" { return errors.New("KBS URL must be an HTTPS origin") } + if kbsURL.Path == "/" && kbsURL.RawPath == "" { + kbsURL.Path = "" + } if kbsURL.User != nil || kbsURL.Path != "" || kbsURL.RawPath != "" || kbsURL.Opaque != "" ||Note that the normalized value is not written back to
settings.KBSURL, sobuildConfidentialInitDatawould still embed the trailing slash. Trim the flag value inloadCCInitDataSettingsif you want the measured value normalized too.🤖 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 `@cluster/kube/builder/settings.go` around lines 146 - 158, Update validateCCInitDataSettings to handle a single trailing slash in settings.KBSURL as a valid HTTPS origin and produce the intended validation behavior instead of the generic path error. If normalization is applied, also trim the value in loadCCInitDataSettings so buildConfidentialInitData receives the normalized URL.
194-202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDeduplicate the TOML-safety predicate.
Lines 200-201 and
writeInitDataMultilineincluster/kube/builder/cc_initdata.go(Line 203) apply the same rule: reject""", NUL, and CR. Extract one helper so the two checks cannot drift apart. Drift would let a value pass settings validation and then fail at initdata build time.🤖 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 `@cluster/kube/builder/settings.go` around lines 194 - 202, Extract the shared TOML-safety predicate currently checked in settings validation and writeInitDataMultiline into one helper, then reuse it in both locations. Preserve rejection of triple quotes, NUL, and carriage returns, and keep the existing validation errors and initdata behavior unchanged.cmd/provider-services/cmd/run.go (1)
579-585: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject storage classes configured without Trustee settings at startup.
An operator can set
--cc-persistent-storage-classeswhile leaving all four Trustee flags empty.loadCCInitDataSettingsthen returnsnil, and the provider starts. The failure surfaces later, per deployment, inconfidentialInitDataAnnotationwith "requires provider confidential-compute initdata settings". Fail at startup instead, so the misconfiguration is visible to the operator immediately.♻️ Proposed startup check
kubeSettings.CCPersistentStorageClasses = make(map[string]struct{}, len(ccPersistentStorageClasses)) for _, storageClass := range ccPersistentStorageClasses { storageClass = strings.TrimSpace(storageClass) if storageClass != "" { kubeSettings.CCPersistentStorageClasses[storageClass] = struct{}{} } } + if len(kubeSettings.CCPersistentStorageClasses) != 0 && kubeSettings.CCInitData == nil { + return fmt.Errorf("%w: %s requires the confidential-compute Trustee flags", errInvalidConfig, FlagCCPersistentStorageClasses) + }🤖 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 `@cmd/provider-services/cmd/run.go` around lines 579 - 585, In loadCCInitDataSettings, reject startup when any non-empty CCPersistentStorageClasses are configured while all Trustee settings are empty. Validate this after trimming and populating kubeSettings.CCPersistentStorageClasses, before startup can proceed, and return the existing configuration error path instead of allowing loadCCInitDataSettings to return nil.cluster/kube/builder/cc_initdata.go (1)
202-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEmit a line break after the opening
"""delimiter.TOML trims a single newline immediately after the opening delimiter of a multi-line basic string. If
writeInitDataMultiline()writes a value that starts with\n, the trimmed parsed value loses bytes that the provider-sha256 annotation can still cover. Write the delimiter on its own line so TOML discards only that newline and the parsed value matches the original bytes. Update the expected initdata digest as part of this change.♻️ Proposed writer change
escaped := strings.ReplaceAll(value, `\`, `\\`) - fmt.Fprintf(buffer, "%q = \"\"\"%s\"\"\"\n", name, escaped) + fmt.Fprintf(buffer, "%q = \"\"\"\n%s\"\"\"\n", name, escaped) return nil🤖 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 `@cluster/kube/builder/cc_initdata.go` around lines 202 - 209, Update writeInitDataMultiline to place a newline immediately after the opening triple-quote delimiter, keeping the value content and closing delimiter on subsequent lines so TOML preserves leading newlines in the parsed value. Adjust the expected initdata digest to match the resulting serialized content.
🤖 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 `@cluster/kube/builder/cc_initdata.go`:
- Around line 202-209: Update writeInitDataMultiline to place a newline
immediately after the opening triple-quote delimiter, keeping the value content
and closing delimiter on subsequent lines so TOML preserves leading newlines in
the parsed value. Adjust the expected initdata digest to match the resulting
serialized content.
In `@cluster/kube/builder/cc_persistent_storage_test.go`:
- Line 32: Add an inline secret-scanner allow directive to the testSealedKeyRef
fixture, using the exact suppression syntax supported by the repository’s
scanner. Keep the synthetic token unchanged and limit the suppression to this
single fixture line.
In `@cluster/kube/builder/cc_registry_credentials_test.go`:
- Around line 18-20: Extend the credential regression tests to construct the
workload through NewWorkloadBuilder rather than assigning confidential
credential resolver results directly. Use a manifest containing a CBS URI, then
assert the generated CDH preserves the credential URI while omitting initdata
auth username/password, imagePullSecrets, the Kubernetes credential Secret, and
ImagePullCredentials.
In `@cluster/kube/builder/settings.go`:
- Around line 146-158: Update validateCCInitDataSettings to handle a single
trailing slash in settings.KBSURL as a valid HTTPS origin and produce the
intended validation behavior instead of the generic path error. If normalization
is applied, also trim the value in loadCCInitDataSettings so
buildConfidentialInitData receives the normalized URL.
- Around line 194-202: Extract the shared TOML-safety predicate currently
checked in settings validation and writeInitDataMultiline into one helper, then
reuse it in both locations. Preserve rejection of triple quotes, NUL, and
carriage returns, and keep the existing validation errors and initdata behavior
unchanged.
In `@cmd/provider-services/cmd/run.go`:
- Around line 579-585: In loadCCInitDataSettings, reject startup when any
non-empty CCPersistentStorageClasses are configured while all Trustee settings
are empty. Validate this after trimming and populating
kubeSettings.CCPersistentStorageClasses, before startup can proceed, and return
the existing configuration error path instead of allowing loadCCInitDataSettings
to return nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a25a36be-7926-45f3-a195-aec763db9f26
⛔ Files ignored due to path filters (2)
cluster/kube/builder/testdata/cc-initdata-kbs.pemis excluded by!**/*.pemgo.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
cluster/kube/builder/cc_initdata.gocluster/kube/builder/cc_persistent_storage.gocluster/kube/builder/cc_persistent_storage_hardening_test.gocluster/kube/builder/cc_persistent_storage_test.gocluster/kube/builder/cc_registry_credentials.gocluster/kube/builder/cc_registry_credentials_test.gocluster/kube/builder/settings.gocluster/kube/builder/workload.gocluster/kube/client.gocmd/provider-services/cmd/cc_initdata_config.gocmd/provider-services/cmd/cc_initdata_config_test.gocmd/provider-services/cmd/flags.gocmd/provider-services/cmd/run.gogo.modpkg/apis/akash.network/crd.yamlpkg/apis/akash.network/v2beta2/manifest.gopkg/apis/akash.network/v2beta2/types_test.gopkg/client/applyconfiguration/akash.network/v2beta2/manifestservicecredentials.gopkg/client/applyconfiguration/akash.network/v2beta2/manifeststorageparams.go
Reject a persistent-storage allowlist unless Trustee initdata settings are configured, so operator mistakes fail at startup. Preserve leading newlines when TOML encodes measured values and cover registry credentials through the production builder path. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
|
Review follow-up is in signed commit
I kept the KBS URL contract strict: it must be an HTTPS origin with no path. A trailing slash parses as path The focused regressions failed for the expected startup-validation and TOML-round-trip reasons before the fix. Afterward, the focused tests, changed-package vet, and full |
Resolve the manifest KBS source before building Kata initdata. Provider mode uses operator defaults. Tenant mode uses the public bundle carried in the manifest and does not require provider KBS settings. Preserve the selection through CRD conversion and reject empty or mixed states. Secret bytes and KBS administrator credentials remain outside the Provider. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
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 `@pkg/apis/akash.network/crd.yaml`:
- Around line 225-232: Reject null KBS source values by removing nullable: true
from both provider and tenant in pkg/apis/akash.network/crd.yaml lines 225-232
and 466-473, covering both schema versions. Add schema coverage verifying
provider: null and tenant: null are rejected.
🪄 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: 3e5fd192-f52f-4107-94bd-10895cbfd589
📒 Files selected for processing (14)
cluster/kube/builder/cc_initdata.gocluster/kube/builder/cc_persistent_storage_hardening_test.gocluster/kube/builder/cc_persistent_storage_test.gocluster/kube/builder/cc_registry_credentials_test.gocluster/kube/builder/settings.gocmd/provider-services/cmd/flags.gopkg/apis/akash.network/crd.yamlpkg/apis/akash.network/v2beta2/manifest.gopkg/apis/akash.network/v2beta2/types_test.gopkg/apis/akash.network/v2beta2/zz_generated.deepcopy.gopkg/client/applyconfiguration/akash.network/v2beta2/manifestservicekbsparams.gopkg/client/applyconfiguration/akash.network/v2beta2/manifestserviceparams.gopkg/client/applyconfiguration/akash.network/v2beta2/manifestservicetenantkbsparams.gopkg/client/applyconfiguration/utils.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/provider-services/cmd/flags.go
- cluster/kube/builder/cc_registry_credentials_test.go
- cluster/kube/builder/settings.go
- cluster/kube/builder/cc_initdata.go
- cluster/kube/builder/cc_persistent_storage_hardening_test.go
CRD admission accepted provider: null and tenant: null because required checks property presence while the nested schemas allowed null. Conversion then rejected the stored object. Reject null sources at admission in both CRD versions. Exercise the actual OpenAPI schema with valid, null, empty, and mixed source values. Signed-off-by: Joseph Chalabi <chalabi.joseph@gmail.com>
2 participants
Why
Confidential Akash workloads need persistent Block storage and private-registry authentication without exposing tenant keys or credentials to the Provider or host Kubernetes.
What changed
keyRefvalues through the Provider CRDkbs:///repo/type/tagURIs through the Provider CRDimagePullSecretsfor URI-backed confidential authenticationThe Provider never accepts, derives, logs, stores, or unwraps a DEK. It never receives a KBS administrator token or registry credential bytes.
Dependencies
The temporary chain SDK replacement in
go.modis review-only and must be removed after #352 is integrated at a locked release revision.Validation
GOWORK=off GOTOOLCHAIN=go1.26.2 go test -count=1 ./...passesimagePullSecretsHardware status
The persistent Block-volume path was proven on an isolated RTX PRO 6000 Akash provider across fresh Kata VMs. The new SDL registry-URI contract and the post-run initdata serialization hardening are covered locally and in CI but have not yet been rerun on hardware; the earlier private-registry canary used a pre-contract initdata path and is not evidence for this URI flow.
B200, NVSwitch, the destructive nvtrust #150 CC-mode transition, and storage-provisioner sanitization remain separate qualification work.