feat(cli): Unsafe update key cli#3733
Conversation
Signed-off-by: Chris Reed <creed@virtru.com>
Signed-off-by: Chris Reed <creed@virtru.com>
Signed-off-by: Chris Reed <creed@virtru.com>
Signed-off-by: Chris Reed <creed@virtru.com>
github-actions
Bot
added
comp:db
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request adds support for unsafely updating KAS registry keys via a new CLI command and service RPC. It enables changing key modes between 'remote' and 'public_key' and updating provider configurations for existing remote keys, while ensuring necessary safety checks and validation are in place. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. The keys were set in stone and cold, But now a change is brave and bold. With unsafe flags and careful hand, We update keys across the land. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an unsafe key update feature (UnsafeUpdateKey) allowing administrators to switch key modes between REMOTE and PUBLIC_KEY_ONLY or update the provider configuration for existing remote keys. The changes span Protobuf definitions, database queries, service-layer logic, CLI commands, and comprehensive tests. The review feedback suggests several defensive programming enhancements to prevent potential nil pointer dereferences when handling key objects and responses, as well as adding a default case to a key mode switch statement to ensure robust validation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if existing.GetKey() == nil { | ||
| return nil, errors.New("existing key is nil") | ||
| } |
There was a problem hiding this comment.
Defensive programming: existing could be nil. Checking both existing and existing.GetKey() for nil prevents a potential nil pointer dereference panic in the service layer.
| if existing.GetKey() == nil { | |
| return nil, errors.New("existing key is nil") | |
| } | |
| if existing == nil || existing.GetKey() == nil { | |
| return nil, errors.New("existing key is nil") | |
| } |
| switch r.GetKeyMode() { | ||
| case policy.KeyMode_KEY_MODE_REMOTE: | ||
| if !newProviderConfiguration.Valid { | ||
| return params, db.ErrUnsafeUpdateKeyProviderConfigRequired | ||
| } | ||
| params.KeyMode = newKeyMode | ||
| params.ProviderConfigID = newProviderConfiguration | ||
| case policy.KeyMode_KEY_MODE_PUBLIC_KEY_ONLY: | ||
| if r.GetProviderConfigId() != "" { | ||
| return params, db.ErrUnsafeUpdateKeyProviderConfigNotAllowed | ||
| } | ||
| params.KeyMode = newKeyMode | ||
| case policy.KeyMode_KEY_MODE_UNSPECIFIED: | ||
| if existingMode != policy.KeyMode_KEY_MODE_REMOTE { | ||
| return params, db.ErrUnsafeUpdateKeyProviderConfigExistingMode | ||
| } | ||
| if !newProviderConfiguration.Valid { | ||
| return params, db.ErrUnsafeUpdateKeyProviderConfigRequired | ||
| } | ||
| params.ProviderConfigID = newProviderConfiguration | ||
| } |
There was a problem hiding this comment.
The switch statement on r.GetKeyMode() does not have a default case. If an unsupported key mode is passed, it will fall through and return params, nil without validation, which could lead to unexpected database updates. Adding a default case to return db.ErrEnumValueInvalid ensures robust validation.
switch r.GetKeyMode() {
case policy.KeyMode_KEY_MODE_REMOTE:
if !newProviderConfiguration.Valid {
return params, db.ErrUnsafeUpdateKeyProviderConfigRequired
}
params.KeyMode = newKeyMode
params.ProviderConfigID = newProviderConfiguration
case policy.KeyMode_KEY_MODE_PUBLIC_KEY_ONLY:
if r.GetProviderConfigId() != "" {
return params, db.ErrUnsafeUpdateKeyProviderConfigNotAllowed
}
params.KeyMode = newKeyMode
case policy.KeyMode_KEY_MODE_UNSPECIFIED:
if existingMode != policy.KeyMode_KEY_MODE_REMOTE {
return params, db.ErrUnsafeUpdateKeyProviderConfigExistingMode
}
if !newProviderConfiguration.Valid {
return params, db.ErrUnsafeUpdateKeyProviderConfigRequired
}
params.ProviderConfigID = newProviderConfiguration
default:
return params, db.ErrEnumValueInvalid
}| if kasKey.GetKey() == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Defensive programming: kasKey could be nil. Checking both kasKey and kasKey.GetKey() for nil prevents a potential nil pointer dereference panic during audit logging.
| if kasKey.GetKey() == nil { | |
| return nil | |
| } | |
| if kasKey == nil || kasKey.GetKey() == nil { | |
| return nil | |
| } |
| cli.ExitWithError("Failed to get kas key", err) | ||
| } | ||
|
|
||
| existingKey := existingKasKey.GetKey() |
There was a problem hiding this comment.
Defensive programming: existingKasKey or existingKasKey.GetKey() could be nil. Checking for nil before calling GetKey() prevents a potential nil pointer dereference panic.
if existingKasKey == nil || existingKasKey.GetKey() == nil {
cli.ExitWithError("Failed to get kas key", fmt.Errorf("key details are missing"))
}
existingKey := existingKasKey.GetKey()| return nil, err | ||
| } | ||
|
|
||
| return resp.GetKey(), nil |
There was a problem hiding this comment.
Defensive programming: resp could be nil if the underlying SDK call returns a nil response without an error (e.g., in mock/test environments or certain client configurations). Checking for nil prevents a nil pointer dereference panic when calling resp.GetKey().
if resp == nil {
return nil, fmt.Errorf("received empty response from unsafe update key service")
}
return resp.GetKey(), nil
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
1 participant
Summary
Adds CLI support for unsafely updating KAS registry keys through the new
UnsafeUpdateKeyservice RPC.This introduces:
otdfctl policy kas-registry key unsafe updateremoteandpublic_keyremotekeyCLI Behavior
The new command accepts:
--id: system key ID to update--mode: target key mode, limited client-side toremoteorpublic_key--provider-config-id: provider config ID forremotemode or remote provider config updates--force: bypasses interactive unsafe confirmationRequest validation for provider config combinations is intentionally handled by the service. The CLI now validates only CLI-level concerns like required
flags, UUID parsing, and unsupported mode strings.
Testing
Updated
otdfctl/e2e/kas-keys.batsto cover:Depends On
feat(policy): Add UnsafeUpdateKey rpc.feat(policy): Add UnsafeUpdateKey implementation