feat(rest-api): add Tenant Identity secrets reencryption to rest-api#3381
Draft
parmani-nv wants to merge 1 commit into
Draft
feat(rest-api): add Tenant Identity secrets reencryption to rest-api#3381parmani-nv wants to merge 1 commit into
parmani-nv wants to merge 1 commit into
Conversation
…n) endpoint
Expose Core's ReencryptTenantIdentitySecrets through NICo REST so provider admins
can re-wrap tenant_identity_config ciphertext to the site's current master key
after a KEK rotation, instead of dropping to raw grpcurl.
Adds POST /v2/org/{org}/nico/site/{siteID}/tenant-identity/reencrypt end to end:
handler (provider-admin scoped, optional organizationId + dryRun), API models,
site workflow + activity calling Core gRPC, RBAC grant for SiteAgent, and
OpenAPI/SDK. Also implements the endpoint in the mock Core gRPC server as a
stateful KEK-rotation simulation so the full path is drivable in local/kind.
- [x] **Add** - New feature or capability
- [ ] **This PR contains breaking changes**
- [x] Unit tests added/updated
- [x] Manual testing performed
Operator workflow (edit current_encryption_key_id + restart Core, then dry-run,
apply, verify) is unchanged; this only adds a REST surface for the apply step.
Targets the unified Core proto package (corev1) from NVIDIA#3238.
Signed-off-by: Parham Armani <parmani@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 provider-admin REST support for tenant identity secret re-encryption. The request flows through Temporal to a site activity and Core RPC, with dry-run, organization filtering, idempotency, response counters, RBAC updates, tests, OpenAPI schemas, and design documentation. ChangesTenant identity secret re-encryption
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RESTClient
participant ReencryptTenantIdentitySecretsHandler
participant TemporalWorkflow
participant SiteActivity
participant CoreRPC
RESTClient->>ReencryptTenantIdentitySecretsHandler: POST tenant-identity/reencrypt
ReencryptTenantIdentitySecretsHandler->>TemporalWorkflow: start ReencryptTenantIdentitySecrets
TemporalWorkflow->>SiteActivity: execute ReencryptTenantIdentitySecretsOnSite
SiteActivity->>CoreRPC: ReencryptTenantIdentitySecrets
CoreRPC-->>SiteActivity: counters and failures
SiteActivity-->>TemporalWorkflow: workflow response
TemporalWorkflow-->>ReencryptTenantIdentitySecretsHandler: completed response
ReencryptTenantIdentitySecretsHandler-->>RESTClient: HTTP 200 response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![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: 6
🧹 Nitpick comments (1)
rest-api/api/pkg/api/model/tenantidentity.go (1)
334-334: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaming deviates from the
ToProto/FromProtoconvention.Sibling conversions elsewhere use
FromProto; naming this oneFromResponseProtobreaks discoverability with the rest of the codebase's convention. As per path instructions, "protobuf conversion should live on API model structs as ToProto and FromProto receiver functions."-func (resp *APIReencryptTenantIdentitySecretsResponse) FromResponseProto(proto *corev1.ReencryptTenantIdentitySecretsResponse) { +func (resp *APIReencryptTenantIdentitySecretsResponse) FromProto(proto *corev1.ReencryptTenantIdentitySecretsResponse) {🤖 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/tenantidentity.go` at line 334, Rename APIReencryptTenantIdentitySecretsResponse.FromResponseProto to FromProto, preserving its behavior and updating all call sites to use the standard protobuf conversion naming convention.Source: Path instructions
🤖 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 `@docs/design/machine-identity/spiffe-svid-sdd.md`:
- Around line 111-114: Add the `text` language hint to the fenced code block
containing “SetTenantIdentityConfiguration (PUT tenant-identity/config)” in the
machine identity design documentation, changing the opening fence to ```text.
- Around line 553-559: Add the missing language identifier to the fenced request
block following the tenant-identity endpoint list, using the existing
bash-tagged block style (```bash) before the PUT command.
In `@rest-api/api/pkg/api/handler/tenantidentity.go`:
- Around line 1122-1152: Use dedicated longer execution and context timeouts for
the tenant-identity reencrypt workflow instead of cutil.WorkflowExecutionTimeout
and cutil.WorkflowContextTimeout. Update the workflow options and context
creation in the reencrypt handler surrounding ExecuteWorkflow and we.Get, sizing
both values to cover the activity’s 5-minute StartToCloseTimeout plus its retry
budget.
In `@rest-api/api/pkg/api/model/tenantidentity.go`:
- Around line 294-312: Validate
APIReencryptTenantIdentitySecretsRequest.OrganizationID in Validate using
ozzo-validation composition, applying is.UUID only when the optional pointer is
non-nil and preserving nil as valid; return the validation error so malformed
organization IDs produce a REST 400 before ToProto forwards the value.
In `@rest-api/openapi/spec.yaml`:
- Around line 13377-13415: Update the description for the
reencrypt-tenant-identity-secrets operation to explicitly document its 404
conditions, including an unknown siteID and an unknown organization when
organizationId is provided, while retaining the existing role and parameter
semantics.
In `@rest-api/site-workflow/pkg/grpc/server/nico_test_server.go`:
- Around line 1744-1748: When `GetOrganizationId()` supplies a non-empty filter
in the identity-state lookup, verify that the scoped organization exists and
return a gRPC `codes.NotFound` error when no matching `orgID` is found, instead
of returning an empty result. Update the relevant test to assert
`codes.NotFound` for a non-existent organization.
---
Nitpick comments:
In `@rest-api/api/pkg/api/model/tenantidentity.go`:
- Line 334: Rename APIReencryptTenantIdentitySecretsResponse.FromResponseProto
to FromProto, preserving its behavior and updating all call sites to use the
standard protobuf conversion naming convention.
🪄 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: 154594be-43d7-4780-b0ae-4c0407dae5f2
⛔ Files ignored due to path filters (4)
rest-api/sdk/standard/api_tenant_identity.gois excluded by!rest-api/sdk/standard/api_*.gorest-api/sdk/standard/model_tenant_identity_reencrypt_failure.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_identity_reencrypt_secrets_request.gois excluded by!rest-api/sdk/standard/model_*.gorest-api/sdk/standard/model_tenant_identity_reencrypt_secrets_response.gois excluded by!rest-api/sdk/standard/model_*.go
📒 Files selected for processing (13)
crates/api-core/src/auth/internal_rbac_rules.rsdocs/design/machine-identity/spiffe-svid-sdd.mdrest-api/api/pkg/api/handler/tenantidentity.gorest-api/api/pkg/api/model/tenantidentity.gorest-api/api/pkg/api/routes.gorest-api/api/pkg/api/routes_test.gorest-api/docs/index.htmlrest-api/openapi/spec.yamlrest-api/site-agent/pkg/components/managers/tenantidentity/subscriber.gorest-api/site-workflow/pkg/activity/tenantidentity.gorest-api/site-workflow/pkg/grpc/server/nico_test_server.gorest-api/site-workflow/pkg/grpc/server/nico_test_server_test.gorest-api/site-workflow/pkg/workflow/tenantidentity.go
Comment on lines
+111
to
+114
| Per-org signing keys are created when an admin first configures machine identity for an org via `PUT tenant-identity/config` (SetTenantIdentityConfiguration). | ||
|
|
||
| ``` | ||
| SetTenantIdentityConfiguration (PUT identity/config) | ||
| SetTenantIdentityConfiguration (PUT tenant-identity/config) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language hint to the fenced diagram block (MD040).
Static analysis (markdownlint MD040) flags the fence opened here as missing a language identifier.
📝 Proposed fix
-```
+```text
SetTenantIdentityConfiguration (PUT tenant-identity/config)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Per-org signing keys are created when an admin first configures machine identity for an org via `PUT tenant-identity/config` (SetTenantIdentityConfiguration). | |
| ``` | |
| SetTenantIdentityConfiguration (PUT identity/config) | |
| SetTenantIdentityConfiguration (PUT tenant-identity/config) | |
| Per-org signing keys are created when an admin first configures machine identity for an org via `PUT tenant-identity/config` (SetTenantIdentityConfiguration). | |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/machine-identity/spiffe-svid-sdd.md` around lines 111 - 114, Add
the `text` language hint to the fenced code block containing
“SetTenantIdentityConfiguration (PUT tenant-identity/config)” in the machine
identity design documentation, changing the opening fence to ```text.
Source: Linters/SAST tools
Comment on lines
+553
to
+559
| PUT tenant-identity/config | ||
| GET tenant-identity/config | ||
| DELETE tenant-identity/config | ||
| ``` | ||
|
|
||
| ``` | ||
| PUT https://{nico-rest}/v2/org/{org-id}/nico/site/{site-id}/identity/config | ||
| PUT https://{nico-rest}/v2/org/{org-id}/nico/site/{site-id}/tenant-identity/config |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language hint to the fenced request block (MD040).
markdownlint flags the fence opened at line 558 (immediately following the bash-tagged block above) as missing a language identifier.
📝 Proposed fix
-```
+```bash
PUT https://{nico-rest}/v2/org/{org-id}/nico/site/{site-id}/tenant-identity/config📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| PUT tenant-identity/config | |
| GET tenant-identity/config | |
| DELETE tenant-identity/config | |
| ``` | |
| ``` | |
| PUT https://{nico-rest}/v2/org/{org-id}/nico/site/{site-id}/identity/config | |
| PUT https://{nico-rest}/v2/org/{org-id}/nico/site/{site-id}/tenant-identity/config |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 558-558: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/design/machine-identity/spiffe-svid-sdd.md` around lines 553 - 559, Add
the missing language identifier to the fenced request block following the
tenant-identity endpoint list, using the existing bash-tagged block style
(```bash) before the PUT command.
Source: Linters/SAST tools
Comment on lines
+1122
to
+1152
| workflowOptions := tclient.StartWorkflowOptions{ | ||
| ID: "tenant-identity-reencrypt-" + org + "-" + site.ID.String() + "-" + hash, | ||
| WorkflowExecutionTimeout: cutil.WorkflowExecutionTimeout, | ||
| TaskQueue: queue.SiteTaskQueue, | ||
| WorkflowIDConflictPolicy: temporalEnums.WORKFLOW_ID_CONFLICT_POLICY_USE_EXISTING, | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(ctx, cutil.WorkflowContextTimeout) | ||
| defer cancel() | ||
|
|
||
| we, err := temporalClient.ExecuteWorkflow(ctx, workflowOptions, "ReencryptTenantIdentitySecrets", protoRequest) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("failed to synchronously start Temporal workflow to reencrypt Tenant Identity secrets") | ||
| return cutil.NewAPIErrorResponse(c, http.StatusInternalServerError, "Failed to start workflow to reencrypt Tenant Identity secrets", nil) | ||
| } | ||
|
|
||
| wid := we.GetID() | ||
| logger.Info().Str("Workflow ID", wid).Msg("executed synchronous reencrypt Tenant Identity secrets workflow") | ||
|
|
||
| var protoResponse corev1.ReencryptTenantIdentitySecretsResponse | ||
| err = we.Get(ctx, &protoResponse) | ||
| if err != nil { | ||
| var timeoutErr *tp.TimeoutError | ||
| if errors.As(err, &timeoutErr) || err == context.DeadlineExceeded || ctx.Err() != nil { | ||
| return common.TerminateWorkflowOnTimeOut(c, logger, temporalClient, wid, err, "TenantIdentity", "ReencryptTenantIdentitySecrets") | ||
| } | ||
|
|
||
| code, unwrapped := common.UnwrapWorkflowError(err) | ||
| logger.Error().Err(unwrapped).Msg("failed to synchronously execute Temporal workflow to reencrypt Tenant Identity secrets") | ||
| return cutil.NewAPIErrorResponse(c, code, "Failed to reencrypt Tenant Identity secrets", nil) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -B2 -A2 'WorkflowContextTimeout\s*=|WorkflowExecutionTimeout\s*=' rest-api/apiRepository: NVIDIA/infra-controller
Length of output: 161
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the timeout constants and the tenant identity workflow/handler usage.
rg -n "WorkflowContextTimeout|WorkflowExecutionTimeout" rest-api -g '!**/vendor/**' || true
printf '\n---\n'
rg -n "ReencryptTenantIdentitySecrets|GetTenantIdentityReencryptActivityOptions|TerminateWorkflowOnTimeOut" rest-api -g '!**/vendor/**' || trueRepository: NVIDIA/infra-controller
Length of output: 50379
Use a longer timeout pair for tenant-identity reencrypt
cutil.WorkflowExecutionTimeout is only 1 minute and cutil.WorkflowContextTimeout is 50 seconds, while the reencrypt activity now runs with a 5-minute StartToCloseTimeout. This will make we.Get time out early and then terminate an otherwise healthy in-flight reencrypt. Give this endpoint a dedicated, larger timeout pair sized for the new activity and its retry budget.
🤖 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/handler/tenantidentity.go` around lines 1122 - 1152, Use
dedicated longer execution and context timeouts for the tenant-identity
reencrypt workflow instead of cutil.WorkflowExecutionTimeout and
cutil.WorkflowContextTimeout. Update the workflow options and context creation
in the reencrypt handler surrounding ExecuteWorkflow and we.Get, sizing both
values to cover the activity’s 5-minute StartToCloseTimeout plus its retry
budget.
Comment on lines
+294
to
+312
| type APIReencryptTenantIdentitySecretsRequest struct { | ||
| OrganizationID *string `json:"organizationId,omitempty"` | ||
| DryRun bool `json:"dryRun"` | ||
| } | ||
|
|
||
| // Validate enforces the REST-layer contract. Both fields are optional, so there is nothing | ||
| // to reject; the method exists to match the sibling requests' interface. | ||
| func (req APIReencryptTenantIdentitySecretsRequest) Validate() error { | ||
| return nil | ||
| } | ||
|
|
||
| // ToProto converts the request to its gRPC form. organizationId comes from the body | ||
| // (not the path), so no org argument is taken. | ||
| func (req APIReencryptTenantIdentitySecretsRequest) ToProto() *corev1.ReencryptTenantIdentitySecretsRequest { | ||
| return &corev1.ReencryptTenantIdentitySecretsRequest{ | ||
| OrganizationId: req.OrganizationID, | ||
| DryRun: req.DryRun, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Unvalidated OrganizationID is forwarded straight to gRPC.
Validate() is a no-op and ToProto() passes req.OrganizationID through unchecked. If a caller supplies a malformed value, it flows directly into the Core RPC as an opaque filter with no REST-layer feedback — the client would only discover the mistake indirectly (e.g., zero rows matched), rather than getting a clear 400. As per path instructions, "validation should prefer ozzo-validation built-in rules and composition over reinvented custom validation helpers" for this directory; consider validating the format of OrganizationID (e.g., is.UUID) when non-nil, if organization IDs in this codebase are UUIDs.
🛡️ Proposed fix using ozzo's built-in UUID rule
+import (
+ validation "github.com/go-ozzo/ozzo-validation/v4"
+ "github.com/go-ozzo/ozzo-validation/v4/is"
+)
+
func (req APIReencryptTenantIdentitySecretsRequest) Validate() error {
- return nil
+ return validation.ValidateStruct(&req,
+ validation.Field(&req.OrganizationID, is.UUID),
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type APIReencryptTenantIdentitySecretsRequest struct { | |
| OrganizationID *string `json:"organizationId,omitempty"` | |
| DryRun bool `json:"dryRun"` | |
| } | |
| // Validate enforces the REST-layer contract. Both fields are optional, so there is nothing | |
| // to reject; the method exists to match the sibling requests' interface. | |
| func (req APIReencryptTenantIdentitySecretsRequest) Validate() error { | |
| return nil | |
| } | |
| // ToProto converts the request to its gRPC form. organizationId comes from the body | |
| // (not the path), so no org argument is taken. | |
| func (req APIReencryptTenantIdentitySecretsRequest) ToProto() *corev1.ReencryptTenantIdentitySecretsRequest { | |
| return &corev1.ReencryptTenantIdentitySecretsRequest{ | |
| OrganizationId: req.OrganizationID, | |
| DryRun: req.DryRun, | |
| } | |
| } | |
| type APIReencryptTenantIdentitySecretsRequest struct { | |
| OrganizationID *string `json:"organizationId,omitempty"` | |
| DryRun bool `json:"dryRun"` | |
| } | |
| // Validate enforces the REST-layer contract. Both fields are optional, so there is nothing | |
| // to reject; the method exists to match the sibling requests' interface. | |
| func (req APIReencryptTenantIdentitySecretsRequest) Validate() error { | |
| return validation.ValidateStruct(&req, | |
| validation.Field(&req.OrganizationID, is.UUID), | |
| ) | |
| } | |
| // ToProto converts the request to its gRPC form. organizationId comes from the body | |
| // (not the path), so no org argument is taken. | |
| func (req APIReencryptTenantIdentitySecretsRequest) ToProto() *corev1.ReencryptTenantIdentitySecretsRequest { | |
| return &corev1.ReencryptTenantIdentitySecretsRequest{ | |
| OrganizationId: req.OrganizationID, | |
| DryRun: req.DryRun, | |
| } | |
| } |
🤖 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/tenantidentity.go` around lines 294 - 312,
Validate APIReencryptTenantIdentitySecretsRequest.OrganizationID in Validate
using ozzo-validation composition, applying is.UUID only when the optional
pointer is non-nil and preserving nil as valid; return the validation error so
malformed organization IDs produce a REST 400 before ToProto forwards the value.
Source: Path instructions
Comment on lines
+13377
to
+13415
| post: | ||
| summary: Reencrypt Tenant Identity Secrets | ||
| tags: | ||
| - Tenant Identity | ||
| operationId: reencrypt-tenant-identity-secrets | ||
| description: |- | ||
| Re-wrap stored `tenant_identity_config` ciphertext with the Site's | ||
| current master encryption key (KEK rotation). This is a | ||
| site-operator operation, not a per-tenant one. | ||
|
|
||
| User must have authorization role with `PROVIDER_ADMIN` suffix in the URL `{org}`. | ||
|
|
||
| Omit `organizationId` to re-wrap every org's secrets on the Site; | ||
| set it to scope the operation to a single org. Set `dryRun` to | ||
| decrypt and validate without writing any changes. | ||
| requestBody: | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/TenantIdentityReencryptSecretsRequest' | ||
| responses: | ||
| '200': | ||
| description: Reencryption completed; counters and any per-field failures are reported in the body. | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/TenantIdentityReencryptSecretsResponse' | ||
| '400': | ||
| $ref: '#/components/responses/ValidationError' | ||
| '403': | ||
| $ref: '#/components/responses/ForbiddenError' | ||
| '404': | ||
| $ref: '#/components/responses/NotFoundError' | ||
| '500': | ||
| description: Internal server error while loading the Tenant or Site, contacting the Core gRPC API, starting the reencrypt workflow, or after the workflow timed out and was terminated. | ||
| $ref: '#/components/responses/GenericHttpError' | ||
| '503': | ||
| $ref: '#/components/responses/TenantIdentityCoreUnavailable' |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document the 404 condition for this endpoint.
Every other Tenant Identity endpoint description in this file explicitly enumerates when 404 Not Found is returned (unknown site, no tenant allocation, etc.). This new endpoint's description explains the role requirement and organizationId/dryRun semantics but omits when 404 fires (e.g. unknown siteID, or unknown org when organizationId is supplied). As per path instructions, "When adding new tenant-identity-related endpoints, follow the existing documentation pattern: put role requirements and 4xx/5xx behavior in the endpoint description."
📝 Suggested description addition
description: |-
Re-wrap stored `tenant_identity_config` ciphertext with the Site's
current master encryption key (KEK rotation). This is a
site-operator operation, not a per-tenant one.
User must have authorization role with `PROVIDER_ADMIN` suffix in the URL `{org}`.
Omit `organizationId` to re-wrap every org's secrets on the Site;
set it to scope the operation to a single org. Set `dryRun` to
decrypt and validate without writing any changes.
+
+ Returns `404 Not Found` when the Site does not exist, or when
+ `organizationId` is supplied but has no matching tenant identity
+ configuration on this Site.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| post: | |
| summary: Reencrypt Tenant Identity Secrets | |
| tags: | |
| - Tenant Identity | |
| operationId: reencrypt-tenant-identity-secrets | |
| description: |- | |
| Re-wrap stored `tenant_identity_config` ciphertext with the Site's | |
| current master encryption key (KEK rotation). This is a | |
| site-operator operation, not a per-tenant one. | |
| User must have authorization role with `PROVIDER_ADMIN` suffix in the URL `{org}`. | |
| Omit `organizationId` to re-wrap every org's secrets on the Site; | |
| set it to scope the operation to a single org. Set `dryRun` to | |
| decrypt and validate without writing any changes. | |
| requestBody: | |
| required: true | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/TenantIdentityReencryptSecretsRequest' | |
| responses: | |
| '200': | |
| description: Reencryption completed; counters and any per-field failures are reported in the body. | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/TenantIdentityReencryptSecretsResponse' | |
| '400': | |
| $ref: '#/components/responses/ValidationError' | |
| '403': | |
| $ref: '#/components/responses/ForbiddenError' | |
| '404': | |
| $ref: '#/components/responses/NotFoundError' | |
| '500': | |
| description: Internal server error while loading the Tenant or Site, contacting the Core gRPC API, starting the reencrypt workflow, or after the workflow timed out and was terminated. | |
| $ref: '#/components/responses/GenericHttpError' | |
| '503': | |
| $ref: '#/components/responses/TenantIdentityCoreUnavailable' | |
| description: |- | |
| Re-wrap stored `tenant_identity_config` ciphertext with the Site's | |
| current master encryption key (KEK rotation). This is a | |
| site-operator operation, not a per-tenant one. | |
| User must have authorization role with `PROVIDER_ADMIN` suffix in the URL `{org}`. | |
| Omit `organizationId` to re-wrap every org's secrets on the Site; | |
| set it to scope the operation to a single org. Set `dryRun` to | |
| decrypt and validate without writing any changes. | |
| Returns `404 Not Found` when the Site does not exist, or when | |
| `organizationId` is supplied but has no matching tenant identity | |
| configuration on this Site. |
🤖 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 13377 - 13415, Update the
description for the reencrypt-tenant-identity-secrets operation to explicitly
document its 404 conditions, including an unknown siteID and an unknown
organization when organizationId is provided, while retaining the existing role
and parameter semantics.
Source: Path instructions
Comment on lines
+1744
to
+1748
| orgFilter := strings.TrimSpace(req.GetOrganizationId()) | ||
| for orgID, st := range f.identityState { | ||
| if st == nil || (orgFilter != "" && orgID != orgFilter) { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target file excerpt ==\n'
sed -n '1728,1768p' rest-api/site-workflow/pkg/grpc/server/nico_test_server.go
printf '\n== related test excerpt ==\n'
sed -n '120,170p' rest-api/site-workflow/pkg/grpc/server/nico_test_server_test.go
printf '\n== contract excerpt search ==\n'
rg -n "organizationId|must exist|RowsExamined|ReencryptTenantIdentitySecrets|Identity configuration not found" \
rest-api/site-workflow/pkg/grpc/server \
docs/design/machine-identity/spiffe-svid-sdd.mdRepository: NVIDIA/infra-controller
Length of output: 9043
Reject missing scoped orgs with NotFound. rest-api/site-workflow/pkg/grpc/server/nico_test_server.go:1744-1748
docs/design/machine-identity/spiffe-svid-sdd.md says a scoped organizationId “must exist,” but this path currently returns RowsExamined: 0 when the filter matches nothing. That turns a stale org id into a silent no-op instead of a missing-resource error.
Proposed fix
orgFilter := strings.TrimSpace(req.GetOrganizationId())
+ if orgFilter != "" {
+ if _, ok := f.identityState[orgFilter]; !ok {
+ return nil, status.Errorf(codes.NotFound, "Identity configuration not found for org %q", orgFilter)
+ }
+ }
for orgID, st := range f.identityState {
if st == nil || (orgFilter != "" && orgID != orgFilter) {
continue
}Update the matching test to expect codes.NotFound for a non-existent org.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| orgFilter := strings.TrimSpace(req.GetOrganizationId()) | |
| for orgID, st := range f.identityState { | |
| if st == nil || (orgFilter != "" && orgID != orgFilter) { | |
| continue | |
| } | |
| orgFilter := strings.TrimSpace(req.GetOrganizationId()) | |
| if orgFilter != "" { | |
| if _, ok := f.identityState[orgFilter]; !ok { | |
| return nil, status.Errorf(codes.NotFound, "Identity configuration not found for org %q", orgFilter) | |
| } | |
| } | |
| for orgID, st := range f.identityState { | |
| if st == nil || (orgFilter != "" && orgID != orgFilter) { | |
| continue | |
| } |
🤖 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/site-workflow/pkg/grpc/server/nico_test_server.go` around lines 1744
- 1748, When `GetOrganizationId()` supplies a non-empty filter in the
identity-state lookup, verify that the scoped organization exists and return a
gRPC `codes.NotFound` error when no matching `orgID` is found, instead of
returning an empty result. Update the relevant test to assert `codes.NotFound`
for a non-existent organization.
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.
feat(rest-api): add Tenant Identity secrets reencryption to rest-api
Expose Core's ReencryptTenantIdentitySecrets through NICo REST so provider admins can re-wrap tenant_identity_config ciphertext to the site's current master key after a KEK rotation, instead of dropping to raw grpcurl.
Adds POST /v2/org/{org}/nico/site/{siteID}/tenant-identity/reencrypt end to end: handler (provider-admin scoped, optional organizationId + dryRun), API models, site workflow + activity calling Core gRPC, RBAC grant for SiteAgent, and OpenAPI/SDK.
issue: 2520
Add - New feature or capability
This PR contains breaking changes
Unit tests added/updated
Manual testing performed