feat: Add measured boot trust approvals#3464
Open
kfelternv wants to merge 4 commits into
Open
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. |
Contributor
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}
Contributor
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>
Contributor
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
Collaborator
|
@kfelternv same comments about PR description, far too verbose. |
Contributor
Author
How we verified itRevision: Environment: Hands-on verification:
Supporting checks: How to reproduce the verificationPrerequisites:
git clone https://github.com/NVIDIA/infra-controller.git infra-controller
cd infra-controller
git fetch origin pull/3464/head:pr-3464
git checkout pr-3464
test "$(git rev-parse HEAD)" = cc4c71c715133b071ba57c939a2d7d5622e9060c
test -z "$(git status --porcelain)"
IMAGE=localhost:5000/nico-rest-api:pr3464-cc4c71c
docker build -t "$IMAGE" -f rest-api/docker/local/Dockerfile.nico-rest-api rest-api
kind load docker-image --name kind "$IMAGE"
ORIGINAL_IMAGE=$(kubectl --context kind-kind -n nico-rest get deployment nico-rest-api \
-o jsonpath='{.spec.template.spec.containers[0].image}')
kubectl --context kind-kind -n nico-rest set image deployment/nico-rest-api api="$IMAGE"
kubectl --context kind-kind -n nico-rest rollout status deployment/nico-rest-api --timeout=120s
kubectl --context kind-kind -n nico-rest port-forward service/nico-rest-api 18388:8388API=http://localhost:18388
ORG=test-org
ADMIN_TOKEN='<provider-admin bearer token>'
TENANT_TOKEN='<tenant-admin bearer token>'
SITE_ID=$(curl -fsS "$API/v2/org/$ORG/nico/site" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -er '.[0].id')
PROFILE_ID=$(kubectl --context kind-kind -n postgres exec postgres-0 -- \
psql -U postgres -d nico -Atqc \
"insert into measurement_system_profiles (name) select gen_random_uuid()::text returning profile_id")
curl -fsS "$API/v2/org/$ORG/nico/measured-boot/trusted-machine?siteId=$SITE_ID" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e 'length == 0'
curl -fsS "$API/v2/org/$ORG/nico/measured-boot/trusted-profile?siteId=$SITE_ID" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e 'length == 0'
status=$(curl -sS -o /dev/null -w '%{http_code}' \
-X POST "$API/v2/org/$ORG/nico/measured-boot/trusted-machine" \
-H "Authorization: Bearer $ADMIN_TOKEN" -H 'Content-Type: application/json' \
-d "$(jq -nc --arg site "$SITE_ID" '{siteId:$site,machineId:"*",approvalType:"Invalid"}')")
test "$status" = 400
status=$(curl -sS -o /dev/null -w '%{http_code}' \
"$API/v2/org/$ORG/nico/measured-boot/trusted-machine?siteId=$SITE_ID" \
-H "Authorization: Bearer $TENANT_TOKEN")
test "$status" = 403
machine=$(curl -fsS -X POST "$API/v2/org/$ORG/nico/measured-boot/trusted-machine" \
-H "Authorization: Bearer $ADMIN_TOKEN" -H 'Content-Type: application/json' \
-d "$(jq -nc --arg site "$SITE_ID" \
'{siteId:$site,machineId:"*",approvalType:"Persist",pcrRegisters:"0,2,7",comments:"measured boot runtime machine"}')")
MACHINE_APPROVAL_ID=$(jq -er '.approvalId' <<<"$machine")
profile=$(curl -fsS -X POST "$API/v2/org/$ORG/nico/measured-boot/trusted-profile" \
-H "Authorization: Bearer $ADMIN_TOKEN" -H 'Content-Type: application/json' \
-d "$(jq -nc --arg site "$SITE_ID" --arg profile "$PROFILE_ID" \
'{siteId:$site,profileId:$profile,approvalType:"Oneshot",pcrRegisters:"0,2,7",comments:"measured boot runtime profile"}')")
PROFILE_APPROVAL_ID=$(jq -er '.approvalId' <<<"$profile")
curl -fsS "$API/v2/org/$ORG/nico/measured-boot/trusted-machine?siteId=$SITE_ID" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e --arg id "$MACHINE_APPROVAL_ID" \
'length == 1 and .[0].approvalId == $id and .[0].machineId == "*" and .[0].approvalType == "Persist"'
curl -fsS "$API/v2/org/$ORG/nico/measured-boot/trusted-profile?siteId=$SITE_ID" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e --arg id "$PROFILE_APPROVAL_ID" --arg profile "$PROFILE_ID" \
'length == 1 and .[0].approvalId == $id and .[0].profileId == $profile and .[0].approvalType == "Oneshot"'
curl -fsS -X DELETE \
"$API/v2/org/$ORG/nico/measured-boot/trusted-machine/$MACHINE_APPROVAL_ID?siteId=$SITE_ID&selector=ApprovalId" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e --arg id "$MACHINE_APPROVAL_ID" '.approvalId == $id'
curl -fsS -X DELETE \
"$API/v2/org/$ORG/nico/measured-boot/trusted-profile/$PROFILE_ID?siteId=$SITE_ID&selector=ProfileId" \
-H "Authorization: Bearer $ADMIN_TOKEN" | jq -e --arg id "$PROFILE_APPROVAL_ID" '.approvalId == $id'
kubectl --context kind-kind -n postgres exec postgres-0 -- psql -U postgres -d nico -v ON_ERROR_STOP=1 -qc \
"delete from measurement_system_profiles where profile_id = '$PROFILE_ID'::uuid"
test "$(kubectl --context kind-kind -n postgres exec postgres-0 -- psql -U postgres -d nico -Atqc \
'select count(*) from measurement_approved_machines')" = 0
test "$(kubectl --context kind-kind -n postgres exec postgres-0 -- psql -U postgres -d nico -Atqc \
'select count(*) from measurement_approved_profiles')" = 0
kubectl --context kind-kind -n nico-rest set image deployment/nico-rest-api api="$ORIGINAL_IMAGE"
kubectl --context kind-kind -n nico-rest rollout status deployment/nico-rest-api --timeout=120s |
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
2 participants
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.
What this PR does
Adds Provider Admin REST operations for measured-boot trust approvals that already exist in Core:
This exposes the existing Core capability through the Provider Admin REST API while preserving the established workflow path.