fix(sdk,policy): skip grant table entry when mapped key has empty PEM/KID - #3827
fix(sdk,policy): skip grant table entry when mapped key has empty PEM/KID#3827timdumm wants to merge 1 commit into
Conversation
github-actions
Bot
added
comp:db
📝 WalkthroughWalkthroughMapped KAS keys with incomplete public-key data or grant creation failures are now logged and skipped. Valid keys continue through grant mapping. The nil public-key test now expects successful mapping without an error. ChangesMapped KAS grant handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
service/policy/db/grant_mappings_test.go (1)
137-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression cases for each missing-material branch.
This test covers only
PublicKey == nil. Add separate cases for empty PEM and empty KID. Add a mixed case with one invalid key and one valid key to verify that invalid keys are skipped and valid keys still create grants.🤖 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 `@service/policy/db/grant_mappings_test.go` around lines 137 - 143, Add regression cases alongside the existing “key with nil public key” case for an empty PEM and an empty KID. Also add a mixed-key case containing one invalid key and one valid key, asserting the invalid key is skipped while the valid key still produces its grant and no error is returned.
🤖 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 `@sdk/granter.go`:
- Around line 340-345: In the mapped-key handling flow around addMappedKey, move
the r.typ and result = mappedFound assignments to after addMappedKey returns
successfully. Ensure failed additions continue without marking mappedFound,
allowing the subsequent ag.GetGrants() fallback to run when no mapped key was
added.
---
Outside diff comments:
In `@service/policy/db/grant_mappings_test.go`:
- Around line 137-143: Add regression cases alongside the existing “key with nil
public key” case for an empty PEM and an empty KID. Also add a mixed-key case
containing one invalid key and one valid key, asserting the invalid key is
skipped while the valid key still produces its grant and no error is returned.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fdbea4be-793c-4fb3-8a8d-17c1969b5d30
📒 Files selected for processing (3)
sdk/granter.goservice/policy/db/grant_mappings.goservice/policy/db/grant_mappings_test.go
…/KID When addMappedKey() rejects a SimpleKasKey due to missing PEM or KID, the KAS URI was still unconditionally added to the grant table. This caused prepareManifest() to attempt fetching the public key from the KAS URI, which for partner keys is a logical identifier (e.g. https://partner.example.com) not a reachable endpoint — resulting in DNS failures and dropped attachments. SDK fix (granter.go): - Skip grant table addition when addMappedKey() fails - Upgrade log from Debug to Warn for visibility Platform fix (grant_mappings.go): - Validate PEM and KID are non-empty before including in SimpleKasKey - Skip keys with missing material instead of aborting all grant mapping Fixes: PEP-5140, PEP-4333 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e680ed5 to
e49dc1c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
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 `@service/policy/db/grant_mappings_test.go`:
- Line 143: Add table-driven cases in the relevant grant-mapping test alongside
the existing PublicKey == nil case for empty PEM and empty KID inputs. Assert
wantErr is false and that no grant is generated for each case, reusing the
existing test fixtures and assertion pattern.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 80e7852e-4254-4c3d-af7a-2c9e604da025
📒 Files selected for processing (3)
sdk/granter.goservice/policy/db/grant_mappings.goservice/policy/db/grant_mappings_test.go
| expectedGrants: []*policy.KeyAccessServer{}, | ||
| wantErr: true, | ||
| errContains: "kas key info is nil for a key with kas uri http://kas1.example.com", | ||
| wantErr: false, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for empty PEM and empty KID.
This case covers only PublicKey == nil. Add one table case for an empty PEM and one for an empty KID. Assert no error and no generated grant in both cases.
🤖 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 `@service/policy/db/grant_mappings_test.go` at line 143, Add table-driven cases
in the relevant grant-mapping test alongside the existing PublicKey == nil case
for empty PEM and empty KID inputs. Assert wantErr is false and that no grant is
generated for each case, reusing the existing test fixtures and assertion
pattern.
2 participants
Summary
addMappedKey()rejects aSimpleKasKeydue to missing PEM or KID, skip adding the KAS URI to the grant table instead of adding it unconditionally. This preventsprepareManifest()from falling back to dialing unreachable partner KAS URIs.SimpleKasKeyresponses. Skip keys with missing material instead of aborting all grant mapping.Context
When encrypting with RSA 4096 partner KAS keys, the clib SDK falls back to fetching the public key from the partner KAS URI (e.g.
https://partner.example.com), which is a logical identifier not a reachable endpoint. This causes DNS failures and dropped email attachments in the Exchange MTA.Root cause:
addAllGrants()unconditionally added the KAS URI to the grant table even afteraddMappedKey()rejected the key. Later,prepareManifest()saw the URI with no cached key and tried to dial it.Fixes: PEP-5140, PEP-4333
Test plan
go test ./sdk/... -run "TestReasoner|TestConfiguration|TestAttribute"— PASSgo test ./service/policy/db/... -run TestMapKasKeysToGrants— PASS (updated test expectation for nil public key: skip instead of error)🤖 Generated with Claude Code
Summary by CodeRabbit