fix(dev-mode): avoid certificate store rescans#421
Conversation
PR SummaryMedium Risk Overview Certificate listing on Linux (and other non-Windows/non-macOS platforms) skips Unix dev cert persistence falls back to an ephemeral key set when saving to the user store fails with home-directory access errors ( The parameterless Reviewed by Cursor Bugbot for commit 2a8a450. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds a configurable ChangesConfigurable dev certificate path support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Program
participant DevCertificateFile
participant CertificateManager
participant DevCertificateProvider
Program->>DevCertificateFile: TryLoad(DevCertPath)
alt certificate loaded
DevCertificateFile-->>Program: certificate
else load failed or no path
Program->>CertificateManager: EnsureDevelopmentCertificate(devCertPath)
CertificateManager-->>Program: EnsureCertificateResult, certificate
Program->>DevCertificateFile: WritePublicCertificate(certificate, DevCertPath)
end
Program->>CertificateManager: TrustCertificate (Windows only)
Program->>DevCertificateProvider: construct with devCertificate
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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a54c232. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/EventStore.Common/DevCertificates/UnixCertificateManager.cs (1)
39-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLog the fallback to
EphemeralKeySet.When a home-directory access error triggers the fallback, the certificate is reloaded in-memory and, notably, is not re-added to the store, so it is not persisted. This happens silently. A warning here would make it clear (during debugging of "why does my dev cert keep regenerating?") that the store write was skipped and persistence now depends on
--dev-cert-path.♻️ Suggested log on fallback
catch (Exception ex) when (IsHomeDirectoryAccessError(ex)) { + Log.SaveCertificateInStoreError(ex.ToString()); certificate.Dispose(); certificate = X509CertificateLoader.LoadPkcs12(export, "", X509KeyStorageFlags.EphemeralKeySet | X509KeyStorageFlags.Exportable); }🤖 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 `@src/EventStore.Common/DevCertificates/UnixCertificateManager.cs` around lines 39 - 44, Handle the home-directory access fallback in UnixCertificateManager by adding a warning in the catch block around the EphemeralKeySet reload so it is clear the certificate was reloaded in memory and not re-added to the store. Use the existing IsHomeDirectoryAccessError path and the certificate reload logic in UnixCertificateManager to log that persistence was skipped and that future persistence depends on --dev-cert-path, so the fallback is visible during debugging.
🤖 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 `@src/EventStore.ClusterNode/Program.cs`:
- Around line 202-212: The trust-check branch in Program.Main is conflating two
different cases: an already-trusted certificate on Windows and an unsupported
platform. Update the conditional around manager.IsTrusted(devCertificate) and
RuntimeInformation.IsWindows so the Windows path only logs and calls
TrustCertificate when the cert is not trusted, and add a separate branch for the
already-trusted case that avoids the warning. Keep the unsupported-platform
warning only for non-Windows platforms, using the existing manager and
devCertificate symbols to make the split clear.
In `@src/EventStore.Common/DevCertificates/DevCertificateFile.cs`:
- Around line 11-40: The blanket catch in DevCertificateFile.TryLoad hides real
certificate load failures, so distinguish the expected missing-file case from
unexpected load errors. Keep the current null return behavior for
File.Exists(path) and invalid certificate checks, but in the
X509CertificateLoader.LoadPkcs12FromFile try/catch path log the caught exception
with context before returning null, using the TryLoad method and
CertificateManager.IsHttpsDevelopmentCertificate path to locate the fix. Ensure
the log captures the path and exception details so misconfigured mounts are
diagnosable.
- Around line 20-23: DevCertificateFile.TryLoad currently loads the PKCS#12
certificate with Exportable only, which can still depend on a writable profile;
update the X509CertificateLoader.LoadPkcs12FromFile call to match the Unix
import behavior by using X509KeyStorageFlags.EphemeralKeySet |
X509KeyStorageFlags.Exportable, or add the same home-directory fallback used in
the other import path. Keep the change localized to DevCertificateFile and its
certificate-loading logic so the import works without requiring a writable user
profile.
---
Nitpick comments:
In `@src/EventStore.Common/DevCertificates/UnixCertificateManager.cs`:
- Around line 39-44: Handle the home-directory access fallback in
UnixCertificateManager by adding a warning in the catch block around the
EphemeralKeySet reload so it is clear the certificate was reloaded in memory and
not re-added to the store. Use the existing IsHomeDirectoryAccessError path and
the certificate reload logic in UnixCertificateManager to log that persistence
was skipped and that future persistence depends on --dev-cert-path, so the
fallback is visible during debugging.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5469b3ff-6fb7-4f16-bffe-20de86e9d02d
📒 Files selected for processing (7)
src/EventStore.ClusterNode/Program.cssrc/EventStore.Common/DevCertificates/CertificateManager.cssrc/EventStore.Common/DevCertificates/DevCertificateFile.cssrc/EventStore.Common/DevCertificates/UnixCertificateManager.cssrc/EventStore.Core.XUnit.Tests/Certificates/DevCertificateFileTests.cssrc/EventStore.Core.XUnit.Tests/Configuration/ClusterVNodeOptionsTests.cssrc/EventStore.Core/Configuration/ClusterVNodeOptions.cs
yordis
changed the title
yordis
force-pushed
the
yordis/fix-dev-certificate-startup
branch
from
a54c232 to
6d82e5f
Compare
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
yordis/fix-dev-certificate-startup
branch
from
6d82e5f to
2a8a450
Compare
1 participant
Uh oh!
There was an error while loading. Please reload this page.