feat(kas): emit INFO log of supported mechanisms at startup (DSPX-3456)#3564
Conversation
log supported kao mechanisms at info during startup
Adds a single `INFO msg="kas initialized" mechanisms=[...]` log line at service startup and on hot config reload, giving operators and the xtest integration suite an authoritative, low-noise way to discover which cryptographic mechanisms this KAS will serve. The mechanism list reflects what the registered trust KeyManagers are *capable* of serving (independent of currently provisioned keys) via a new optional `trust.AlgorithmAdvertiser` interface, then is filtered to match the gating in `rewrap.go` so the log only advertises algorithms rewrap would actually accept (drops `ec:*` unless ECTDFEnabled, drops `hpqt:*` unless HybridTDFEnabled — top-level or preview flag). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
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 enhances the KAS observability by emitting an INFO-level log entry at startup and configuration reload that explicitly lists the supported cryptographic mechanisms. This change provides operators and automated test suites with an authoritative, low-noise way to discover capabilities without needing to enable DEBUG logging or perform external HTTP probes. The implementation introduces a new capability interface for key managers and ensures that the advertised algorithm list respects current preview-feature gating. 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. 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. At startup the KAS takes a look, At algorithms written in the book. With an INFO log line, It makes the state shine, And helps all the tests off the hook. Footnotes
|
📝 WalkthroughWalkthroughThis PR introduces cryptographic algorithm advertisement throughout the KAS trust system. Key managers export canonical supported-algorithm lists; DelegatingKeyService aggregates them without instantiating managers; KAS logs mechanisms at startup, filtering by preview gates. Comprehensive test coverage validates deduplication, sorting, and nil-safety. ChangesAlgorithm Advertisement and KAS Mechanism Initialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Code Review
This pull request introduces the AlgorithmAdvertiser interface and implements it across various key managers to allow KAS to log its supported cryptographic mechanisms at the INFO level during startup. Feedback on the changes highlights a resource leak and redundant instantiation issue in DelegatingKeyService.SupportedAlgorithms, where calling getKeyManager caches a dummy manager indefinitely. It is recommended to instantiate the managers directly from factories and close them immediately to release resources.
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.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
service/internal/security/in_process_provider.go (1)
1-379:⚠️ Potential issue | 🟠 MajorFix Go lint failures and ensure gofumpt formatting for in_process_provider.go
gofumptcouldn’t be executed here (tool not available), sogofumpt -w service/internal/security/in_process_provider.goshould still be applied before finishing.golangci-lintis failing onservice/internal/security/...due togoconst(service/internal/security/in_process_provider.go:325has repeatedType: "PUBLIC KEY") andsloglint(service/internal/security/standard_crypto.go:252usesslog.Any("kasInfo", ...)which should be snake_case).go test ./service/internal/security/...passes.🤖 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/internal/security/in_process_provider.go` around lines 1 - 379, The linter failures are from a repeated PEM type string and a slog key style/usage; extract a constant (e.g., const publicKeyPEMType = "PUBLIC KEY") and replace the literal in DeriveKey (pem.Block{Type: "PUBLIC KEY", ...}) and any other occurrences in this package with that constant to satisfy goconst, and in standard_crypto.go change the slog call that uses slog.Any("kasInfo", ...) to use a snake_case key and an appropriate typed slog method (e.g., slog.String("kas_info", fmt.Sprintf("%v", kasInfo)) or slog.Any with "kas_info") so it conforms to sloglint; after edits run gofumpt -w on the file(s) and re-run golangci-lint and go test to verify all checks pass.service/internal/security/basic_manager.go (1)
1-307:⚠️ Potential issue | 🟠 MajorFix golangci-lint failures (and run gofumpt) for service/internal/security/basic_manager.go
gofumptwasn’t available in this environment—rungofumpt -w service/internal/security/basic_manager.go.golangci-lint run service/internal/security/basic_manager.gofails typecheck: undefinedAlgorithmRSA2048/AlgorithmRSA4096/AlgorithmECP256R1/AlgorithmECP384R1/AlgorithmECP521R1/AlgorithmHPQTXWing/AlgorithmHPQTSecp256r1MLKEM768/AlgorithmHPQTSecp384r1MLKEM1024(lines 33-40) and undefinedTDFSalt(lines 210, 236).go test ./service/internal/security/...passes.🤖 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/internal/security/basic_manager.go` around lines 1 - 307, The file fails golangci-lint typecheck because the algorithm constants and TDFSalt are undefined; replace the custom basicManagerSupportedAlgorithms entries with the corresponding ocrypto constants (e.g., use ocrypto.RSA2048Key, ocrypto.RSA4096Key, ocrypto.EC256Key, ocrypto.EC384Key, ocrypto.EC521Key, ocrypto.HybridXWingKey, ocrypto.HybridSecp256r1MLKEM768Key, ocrypto.HybridSecp384r1MLKEM1024Key) so they match the switch in Decrypt, and change calls to TDFSalt() to ocrypto.TDFSalt() (or import/implement the same symbol from the correct package if different); after making these symbol fixes, run gofumpt -w service/internal/security/basic_manager.go and re-run golangci-lint to verify the typecheck errors are resolved.
🤖 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 `@spec/DSPX-3456.md`:
- Around line 29-46: The spec currently contains placeholder headings ("Problem
/ Motivation", "Proposed Solution", "Inputs / Outputs / Contracts", "Edge Cases
& Constraints", "Out of Scope", "Acceptance Criteria") with no real content;
update DSPX-3456.md by either populating each section with concise, relevant
content (describe the user/business problem and success criteria under Problem /
Motivation, outline the implementation approach and key components under
Proposed Solution, enumerate function signatures/data shapes/API contracts under
Inputs / Outputs / Contracts, list boundary/error/performance/security
considerations under Edge Cases & Constraints, specify what will not be
delivered under Out of Scope, and provide clear, testable items under Acceptance
Criteria) or remove any section that does not apply so the spec contains only
meaningful sections.
- Around line 14-27: The Summary section is a dense, unformatted paragraph;
break it into distinct Problem, Proposed Solution, Why, and Acceptance Criteria
subsections, insert blank lines between paragraphs, wrap the JSON examples in
code blocks, and fix the "CriteriaKAS" spacing to "Criteria\n\nKAS" (or
"Criteria: KAS") so headings read correctly; update the headings "Summary",
"Proposed Solution", "Why", and "Acceptance Criteria" accordingly to improve
readability and ensure the JSON log examples are formatted as code blocks (the
JSON snippets shown after "kas config" and "kas initialized" should be fenced).
---
Outside diff comments:
In `@service/internal/security/basic_manager.go`:
- Around line 1-307: The file fails golangci-lint typecheck because the
algorithm constants and TDFSalt are undefined; replace the custom
basicManagerSupportedAlgorithms entries with the corresponding ocrypto constants
(e.g., use ocrypto.RSA2048Key, ocrypto.RSA4096Key, ocrypto.EC256Key,
ocrypto.EC384Key, ocrypto.EC521Key, ocrypto.HybridXWingKey,
ocrypto.HybridSecp256r1MLKEM768Key, ocrypto.HybridSecp384r1MLKEM1024Key) so they
match the switch in Decrypt, and change calls to TDFSalt() to ocrypto.TDFSalt()
(or import/implement the same symbol from the correct package if different);
after making these symbol fixes, run gofumpt -w
service/internal/security/basic_manager.go and re-run golangci-lint to verify
the typecheck errors are resolved.
In `@service/internal/security/in_process_provider.go`:
- Around line 1-379: The linter failures are from a repeated PEM type string and
a slog key style/usage; extract a constant (e.g., const publicKeyPEMType =
"PUBLIC KEY") and replace the literal in DeriveKey (pem.Block{Type: "PUBLIC
KEY", ...}) and any other occurrences in this package with that constant to
satisfy goconst, and in standard_crypto.go change the slog call that uses
slog.Any("kasInfo", ...) to use a snake_case key and an appropriate typed slog
method (e.g., slog.String("kas_info", fmt.Sprintf("%v", kasInfo)) or slog.Any
with "kas_info") so it conforms to sloglint; after edits run gofumpt -w on the
file(s) and re-run golangci-lint and go test to verify all checks pass.
🪄 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
Run ID: f3285421-d072-4209-a30b-1cec5edb439b
📒 Files selected for processing (10)
service/internal/security/basic_manager.goservice/internal/security/basic_manager_test.goservice/internal/security/in_process_provider.goservice/internal/security/in_process_provider_test.goservice/kas/kas.goservice/kas/kas_test.goservice/trust/delegating_key_service.goservice/trust/delegating_key_service_test.goservice/trust/key_manager.gospec/DSPX-3456.md
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/internal/security/basic_manager_test.go`:
- Around line 631-634: The test weakened validation of advertised algorithms:
restore an explicit check against the expected algorithm set (e.g., a hardcoded
slice like expectedAlgs := []string{"HS256", "RS256"} or whatever the supported
list should be) and assert equality (or set-equality ignoring order) with
bm.SupportedAlgorithms(); keep the copy behavior test by still mutating the
returned slice (algs[0] = "tampered") and asserting that
bm.SupportedAlgorithms()[0] is not "tampered" afterwards. Use the
bm.SupportedAlgorithms function name and the existing local variable algs to
locate where to add the explicit expected-set assertion.
In `@service/internal/security/in_process_provider_test.go`:
- Around line 237-240: Re-add an explicit assertion that the provider's
SupportedAlgorithms() returns the expected algorithm set in addition to the
non-empty and copy checks: call a.SupportedAlgorithms(), assert it contains (or
equals) the canonical algorithm(s) your implementation must advertise (e.g. the
known algorithm constant(s) used elsewhere in tests), then continue to mutate
the local copy and assert the provider's slice is unchanged; reference the
SupportedAlgorithms() method and the local variable a used in this test to
locate where to add the assertion.
🪄 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
Run ID: 8560eb22-5ded-457b-b81f-5b5763b767c7
📒 Files selected for processing (25)
service/entityresolution/keycloak/v2/entity_resolution.goservice/integration/main_test.goservice/internal/access/v2/evaluate.goservice/internal/auth/casbin.goservice/internal/fixtures/fixtures.goservice/internal/security/basic_manager.goservice/internal/security/basic_manager_test.goservice/internal/security/in_process_provider.goservice/internal/security/in_process_provider_test.goservice/internal/security/standard_crypto.goservice/kas/access/rewrap_test.goservice/kas/kas.goservice/kas/kas_test.goservice/logger/audit/utils.goservice/pkg/db/db.goservice/policy/keymanagement/key_management.goservice/policy/namespaces/namespaces.goservice/policy/obligations/obligations.goservice/policy/registeredresources/registered_resources.goservice/policy/resourcemapping/resource_mapping.goservice/policy/subjectmapping/subject_mapping.goservice/tracing/otel.goservice/trust/delegating_key_service.goservice/trust/delegating_key_service_test.goservice/trust/key_manager.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Replace the runtime AlgorithmAdvertiser interface with static algorithm metadata captured when a key manager is registered. SupportedAlgorithms now reads from the registration record and no longer instantiates any manager just to enumerate capabilities. - trust: add RegisterKeyManagerCtxWithAlgorithms; SupportedAlgorithms reads registration metadata only and never invokes a factory. - security: drop SupportedAlgorithms methods; export the package-level algorithm lists for registration sites. - kas: register basic and in-process managers with their algorithm sets. - tests: assert factories are never invoked during capability listing, that the registered slice is copied defensively, and switch a hand-rolled slices.Equal/t.Fatalf check to assert.Equal for consistency. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Dave Mihalcik <dmihalcik@virtru.com>
c0e640e
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
service/internal/security/in_process_provider.go (1)
20-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
InProcessSupportedAlgorithmsover-advertises EC support.
InProcessSupportedAlgorithmsincludesec:secp384r1andec:secp521r1, butDecryptonly handles the EC256 branch, so advertised mechanisms can still fail at runtime with"unsupported key algorithm". Align the list with actualDecryptcoverage (or expand the EC switch cases to match the list).Suggested minimal fix (advertise only what Decrypt currently supports)
var InProcessSupportedAlgorithms = []ocrypto.KeyType{ ocrypto.RSA2048Key, ocrypto.RSA4096Key, ocrypto.EC256Key, - ocrypto.EC384Key, - ocrypto.EC521Key, ocrypto.HybridXWingKey, ocrypto.HybridSecp256r1MLKEM768Key, ocrypto.HybridSecp384r1MLKEM1024Key, }Also applies to: 216-225
🤖 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/internal/security/in_process_provider.go` around lines 20 - 32, The supported-algorithms list advertises EC384/EC521 but Decrypt only implements the EC256 branch; update InProcessSupportedAlgorithms to match Decrypt’s actual coverage by removing ocrypto.EC384Key and ocrypto.EC521Key (and any duplicate list at the other occurrence) OR alternatively extend the Decrypt switch in Decrypt to handle ocrypto.EC384Key and ocrypto.EC521Key with the same pattern as the EC256 branch; touch the InProcessSupportedAlgorithms variable and the Decrypt method to keep both in sync (refer to the InProcessSupportedAlgorithms slice and the Decrypt function/switch cases).
🤖 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/kas/kas.go`:
- Line 32: The TraceContext call is logging the full kasCfg which may contain
secrets; change p.Logger.TraceContext(ctx, "kas config reloaded",
slog.Any("config", kasCfg)) to log only a safe/redacted subset or an explicit
safe shape: construct a sanitized struct or map (e.g., safeKasCfg or kasCfgSafe)
containing only non-secret fields (feature flags, booleans, durations) or redact
sensitive fields before passing to p.Logger.TraceContext, and apply the same
change for the other occurrence that logs kasCfg.
---
Duplicate comments:
In `@service/internal/security/in_process_provider.go`:
- Around line 20-32: The supported-algorithms list advertises EC384/EC521 but
Decrypt only implements the EC256 branch; update InProcessSupportedAlgorithms to
match Decrypt’s actual coverage by removing ocrypto.EC384Key and
ocrypto.EC521Key (and any duplicate list at the other occurrence) OR
alternatively extend the Decrypt switch in Decrypt to handle ocrypto.EC384Key
and ocrypto.EC521Key with the same pattern as the EC256 branch; touch the
InProcessSupportedAlgorithms variable and the Decrypt method to keep both in
sync (refer to the InProcessSupportedAlgorithms slice and the Decrypt
function/switch cases).
🪄 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
Run ID: 02ad105c-bd4e-4774-9a66-ba35f3ae5dde
📒 Files selected for processing (7)
service/internal/security/basic_manager.goservice/internal/security/in_process_provider.goservice/kas/kas.goservice/kas/kas_test.goservice/trust/delegating_key_service.goservice/trust/delegating_key_service_test.goservice/trust/key_manager.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a low-noise capability discovery log to KAS by aggregating the set of supported cryptographic mechanisms from registered trust KeyManager factories, then filtering that set using the same preview-feature gates enforced by rewrap.go. This helps operators (and integration tests) determine what the instance will actually accept without needing to probe request paths.
Changes:
- Extend KeyManager factory registration to optionally advertise supported algorithms, and aggregate/dedupe/sort them via
DelegatingKeyService.SupportedAlgorithms. - Emit an INFO log at startup and on hot config reload listing supported mechanisms, filtered by preview flags.
- Add unit tests for algorithm aggregation, preview filtering, and structured log emission.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| service/trust/key_manager.go | Adds optional SupportedAlgorithms metadata to named KeyManager ctx factories. |
| service/trust/delegating_key_service.go | Stores per-factory algorithm metadata; adds SupportedAlgorithms() union/dedupe/sort helper. |
| service/trust/delegating_key_service_test.go | Adds tests ensuring aggregation is correct and factories are not invoked. |
| service/kas/kas.go | Logs supported mechanisms at startup and config reload; filters by preview feature gates. |
| service/kas/kas_test.go | Adds tests for preview filtering and for the emitted INFO log record structure. |
| service/internal/security/in_process_provider.go | Introduces canonical supported-algorithm list and reuses it for listing keys. |
| service/internal/security/basic_manager.go | Introduces canonical supported-algorithm list for the BasicManager. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
This reverts commit 3cd2322.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
Summary
INFO msg="kas trust mechanisms initialized" mechanisms=[...]log line at KAS startup and on hot config reloadrewrap.o\acceptance logic (ec/hybrid types suppressed unless the corresponding flag is enabled)Implementation
Extended
NamedKeyManagerCtxFactorywith aSupportedAlgorithms []ocrypto.KeyTypefield (optional; empty means the manager contributes nothing to capability listings)🤖 Generated with Claude Code