feat: Add measured boot trust approvals#3464
Conversation
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Summary by CodeRabbit
WalkthroughAdds REST API support for creating, listing, and deleting measured-boot trusted machine and profile approvals, including models, validation, Core integration, authorization, routes, OpenAPI documentation, and automated tests. ChangesMeasurement trust API
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant APIClient
participant MeasurementTrustHandler
participant CoreGrpcProxy
APIClient->>MeasurementTrustHandler: Create, list, or delete approval
MeasurementTrustHandler->>MeasurementTrustHandler: Validate request and authorize site
MeasurementTrustHandler->>CoreGrpcProxy: Execute measurement trust operation
CoreGrpcProxy-->>MeasurementTrustHandler: Return approval record or records
MeasurementTrustHandler-->>APIClient: Return HTTP response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3464.docs.buildwithfern.com/infra-controller |
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-14 01:42:37 UTC | Commit: cc4c71c |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc4c71c715
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| apiErr = common.ExecuteCoreGRPC(ctx, stc, corev1.Forge_RemoveMeasurementTrustedMachine_FullMethodName, coreReq, coreResp, siteID) | ||
| if apiErr != nil { | ||
| logAPIError(logger, apiErr, "failed to delete machine trust approval") | ||
| return cutil.NewAPIErrorResponse(c, apiErr.Code, apiErr.Message, nil) |
There was a problem hiding this comment.
When the requested approval or target ID is syntactically valid but no approval row exists, Core's delete path (crates/api-core/src/measured_boot/rpc/site.rs) calls fetch_one and wraps RowNotFound as an internal error, so ExecuteCoreGRPC yields a 500 and this line forwards it. The new DELETE REST contract advertises 404 for missing IDs; map that not-found case before returning here (and in the analogous trusted-profile delete handler) so normal absent-resource deletes do not surface as server failures.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
rest-api/api/pkg/api/model/measurementtrust.go (1)
17-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel approval-type as a named type with receiver methods instead of raw string + free helpers.
approvalTypeis currently a barestringvalidated via a hand-rolled switch (validateMeasurementTrustApprovalType) and converted via free functions (measurementTrustApprovalTypeToProto/FromProto). Per coding guidelines, this should be a named type with receiver methods, and validation should prefer ozzo's built-invalidation.Inrule (or afunc(interface{}) errorvalidator usable withvalidation.By) rather than a bespoke switch.♻️ Suggested refactor
-const ( - MeasurementTrustApprovalTypeOneshot = "Oneshot" - MeasurementTrustApprovalTypePersist = "Persist" -) +type MeasurementTrustApprovalType string + +const ( + MeasurementTrustApprovalTypeOneshot MeasurementTrustApprovalType = "Oneshot" + MeasurementTrustApprovalTypePersist MeasurementTrustApprovalType = "Persist" +) + +func (t MeasurementTrustApprovalType) ToProto() corev1.MeasurementApprovedTypePb { + if t == MeasurementTrustApprovalTypePersist { + return corev1.MeasurementApprovedTypePb_Persist + } + return corev1.MeasurementApprovedTypePb_Oneshot +}Then use
validation.Field(&r.ApprovalType, validation.Required, validation.In(MeasurementTrustApprovalTypeOneshot, MeasurementTrustApprovalTypePersist))insideValidateStruct.Also applies to the wildcard
MachineID != "*"check at Lines 81-85, which could becomevalidation.When(r.MachineID != "*", validationis.UUID.Error(...))insideValidateStructfor a single-pass validation.Also applies to: 72-99, 231-252
🤖 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 `@rest-api/api/pkg/api/model/measurementtrust.go` around lines 17 - 20, Refactor approvalType in the measurement-trust model into a named type with receiver methods for proto conversion, replacing validateMeasurementTrustApprovalType and the free conversion helpers. Update ValidateStruct to validate ApprovalType with validation.Required and validation.In using the two approval constants, and express the MachineID wildcard exception with validation.When and the UUID rule in the same validation pass.Source: Coding guidelines
rest-api/openapi/spec.yaml (1)
1296-1329: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMissing pagination on the two new list endpoints.
Both
get-all-measurement-trusted-machineandget-all-measurement-trusted-profileonly acceptsiteIdand omitpageNumber/pageSize/orderByplus theX-Paginationresponse header, unlike essentially every otherget-all-*endpoint in this spec — including other Site-scoped resources such asget-all-sku.Persist-type approvals have no forced expiry, so this list can grow unbounded at a busy Site, and callers have no way to page through results.♻️ Suggested pagination addition (apply to both list operations)
parameters: - schema: type: string format: uuid name: siteId in: query required: true description: ID of the Site + - schema: + type: integer + example: 1 + default: 1 + minimum: 1 + in: query + name: pageNumber + description: Page number for pagination query + - schema: + type: integer + minimum: 1 + maximum: 100 + example: 20 + in: query + name: pageSize + description: Page size for pagination query responses: '200': description: Measured Boot trusted Machine approvals content: application/json: schema: type: array items: $ref: '`#/components/schemas/MeasurementTrustedMachine`' + headers: + X-Pagination: + schema: + type: string + example: '{"pageNumber":1,"pageSize":20,"total":30}' + description: Pagination result in JSON formatAlso applies to: 1423-1456
🤖 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 `@rest-api/openapi/spec.yaml` around lines 1296 - 1329, Update both list operations, get-all-measurement-trusted-machine and get-all-measurement-trusted-profile, to accept the standard pageNumber, pageSize, and orderBy query parameters used by comparable get-all endpoints such as get-all-sku. Add the X-Pagination response header to each 200 response while preserving the existing siteId parameter and response schemas.
🤖 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 `@rest-api/api/pkg/api/model/measurementtrust.go`:
- Around line 126-180: Move the protobuf conversion logic from the free
functions APIMeasurementTrustedMachineFromProto and
APIMeasurementTrustedProfileFromProto onto FromProto receiver methods of their
respective API model structs. Replace the plural helpers
APIMeasurementTrustedMachinesFromProto and
APIMeasurementTrustedProfilesFromProto with named slice types and corresponding
FromProto receiver methods, preserving nil handling and element conversion
behavior.
- Around line 182-229: Replace MeasurementTrustedMachineRemoveProto and
MeasurementTrustedProfileRemoveProto with delete request DTOs containing
selector and path-derived ID fields, plus Validate and error-free ToProto
methods matching the existing create-request pattern. Populate these DTOs in the
handlers, run authorization before validation/conversion, then call Validate
followed by ToProto; preserve wildcard machine-ID handling and selector-specific
protobuf mapping.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/measurementtrust.go`:
- Around line 17-20: Refactor approvalType in the measurement-trust model into a
named type with receiver methods for proto conversion, replacing
validateMeasurementTrustApprovalType and the free conversion helpers. Update
ValidateStruct to validate ApprovalType with validation.Required and
validation.In using the two approval constants, and express the MachineID
wildcard exception with validation.When and the UUID rule in the same validation
pass.
In `@rest-api/openapi/spec.yaml`:
- Around line 1296-1329: Update both list operations,
get-all-measurement-trusted-machine and get-all-measurement-trusted-profile, to
accept the standard pageNumber, pageSize, and orderBy query parameters used by
comparable get-all endpoints such as get-all-sku. Add the X-Pagination response
header to each 200 response while preserving the existing siteId parameter and
response schemas.
🪄 Autofix (Beta)
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: Enterprise
Run ID: c9957257-ea84-4d0f-b9bf-31302c09ffd4
⛔ Files ignored due to path filters (7)
rest-api/sdk/standard/api_measured_boot_trusted_machine.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/api_measured_boot_trusted_profile.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/client.gois excluded by!rest-api/sdk/standard/client.gorest-api/sdk/standard/model_measurement_trusted_machine.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_measurement_trusted_machine_create_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_measurement_trusted_profile.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_measurement_trusted_profile_create_request.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (7)
rest-api/api/pkg/api/handler/measurementtrust.gorest-api/api/pkg/api/handler/measurementtrust_test.gorest-api/api/pkg/api/model/measurementtrust.gorest-api/api/pkg/api/model/measurementtrust_test.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/openapi/spec.yaml
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rest-api/api/pkg/api/model/measurementtrust.go (1)
85-99: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse named
validation.Byvalidators for the cross-field ID rules.
MachineID != "*"and the matching delete-path selector logic should live in receiver methods, then be composed throughvalidation.Field(..., validation.By(...))instead of inline branching. That keeps the validation reusable and consistent with the rest of the model package.🤖 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 `@rest-api/api/pkg/api/model/measurementtrust.go` around lines 85 - 99, Refactor APIMeasurementTrustedMachineCreateRequest.Validate to move the MachineID wildcard/UUID rule into a named receiver validator method and apply it through validation.Field with validation.By. Apply the same pattern to the matching delete-path selector logic, preserving the existing "*" allowance and UUID validation behavior while removing inline branching.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@rest-api/api/pkg/api/model/measurementtrust.go`:
- Around line 85-99: Refactor APIMeasurementTrustedMachineCreateRequest.Validate
to move the MachineID wildcard/UUID rule into a named receiver validator method
and apply it through validation.Field with validation.By. Apply the same pattern
to the matching delete-path selector logic, preserving the existing "*"
allowance and UUID validation behavior while removing inline branching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b876a0c1-d3a2-4ebe-8d70-719e0c2c86f2
📒 Files selected for processing (4)
rest-api/api/pkg/api/handler/measurementtrust.gorest-api/api/pkg/api/handler/measurementtrust_test.gorest-api/api/pkg/api/model/measurementtrust.gorest-api/api/pkg/api/model/measurementtrust_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- rest-api/api/pkg/api/handler/measurementtrust_test.go
- rest-api/api/pkg/api/handler/measurementtrust.go
1 participant
Resolves #2801
What this PR does
Adds Provider Admin REST operations for measured-boot trust approvals that already exist in Core:
How we verified it
Revision:
cc4c71c715133b071ba57c939a2d7d5622e9060cEnvironment:
kfelter-nico-dev.nvidia.com, Kind contextkind-kind, with the integrated local stack from revision7ea923ea3cca858f53b304f361177721eb3f63b0. Onlydeployment/nico-rest-apiwas replaced with an image built from this PR; Core, Temporal, SiteAgent, Keycloak, and PostgreSQL remained on the existing stack.Hands-on verification:
GET /v2/org/test-org/nico/measured-boot/trusted-machinereturned404, confirming that the running baseline did not expose the new route.localhost:5000/nico-rest-api:pr3464-cc4c71c, loaded it into Kind, and observed one ready REST API pod with zero restarts and image IDsha256:ed4c0c7254d2112c9d8f80ea0531b3613b81bd08fa7e8c2329fb1109ac456103.200with[]for both resource types.400. A Tenant Admin list request returned403.Persistapproval and a trusted-profileOneshotapproval both returned201. Follow-up list requests returned the exact approval IDs, targets, PCR registers, and comments.code=Okfor all six list, add, and remove gRPC methods, and Core spans reported HTTP200with gRPC status code0.200. Final Core counts were zero trusted-machine approvals, zero trusted-profile approvals, and zero temporary system profiles.Supporting checks:
How to reproduce the verification
Prerequisites:
kind-kind, including REST, Temporal, SiteAgent, Core, Keycloak, and PostgreSQL.