Ensure SSL_CERT_DIR messages are always shown and check for existing value#18
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
WalkthroughThe changes enhance certificate handling and event logging across three files: adding SSL_CERT_DIR environment variable validation with conditional event logging in the Unix certificate manager, introducing two new event source methods for OpenSSL configuration feedback, and reorganizing event listener initialization and logging level configuration in the dev-certs CLI tool. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs`:
- Around line 358-408: The code currently calls Path.GetFullPath(prettyCertDir)
when computing certDirFullPath for comparison, but prettyCertDir can contain
unexpanded tokens like "$HOME"; replace uses of Path.GetFullPath(prettyCertDir)
with Path.GetFullPath(certDir) (or use certDir directly) when computing
certDirFullPath and performing the equality check inside the existingDirs.Any
lambda (keep Log calls using prettyCertDir for display), so isCertDirIncluded
compares actual filesystem paths (use symbols: prettyCertDir, certDir,
certDirFullPath, existingSslCertDir, existingDirs, isCertDirIncluded).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Shared/CertificateGeneration/CertificateManager.cssrc/Shared/CertificateGeneration/UnixCertificateManager.cssrc/Tools/dotnet-dev-certs/src/Program.cs
🧰 Additional context used
🧬 Code graph analysis (1)
src/Tools/dotnet-dev-certs/src/Program.cs (2)
src/Shared/CommandLineUtils/CommandLine/CommandOption.cs (1)
HasValue(93-96)src/Shared/CertificateGeneration/CertificateManager.cs (4)
CertificateManager(21-1356)CertificateManager(92-94)CertificateManager(97-100)CertificateManager(103-108)
🔇 Additional comments (5)
src/Shared/CertificateGeneration/CertificateManager.cs (2)
1302-1305: LGTM!The updated event message correctly uses
{2}placeholder for the environment variable name, and the parameter order in the method signature matches the message placeholders.
1313-1320: LGTM!The two new event methods are well-designed:
- Event 113 (
UnixOpenSslCertificateDirectoryAlreadyConfigured) appropriately usesVerboselevel since it's informational.- Event 114 (
UnixSuggestAppendingToEnvironmentVariable) correctly usesLogAlwaysto ensure the user sees the important guidance about appending to an existingSSL_CERT_DIR.The message format
export {1}=\"{0}:${1}\"correctly suggests appending the new directory while preserving the existing value via shell variable expansion.src/Shared/CertificateGeneration/UnixCertificateManager.cs (1)
994-1007: LGTM!The
NssDbclass refactor from primary constructor syntax to explicit field-based implementation is straightforward. The functionality is unchanged, and this style may be preferred for consistency with the rest of the codebase.src/Tools/dotnet-dev-certs/src/Program.cs (2)
127-135: LGTM!The change to unconditionally create
ReporterEventListenerand enable eitherVerboseorLogAlwayslevel is the correct approach to ensure SSL_CERT_DIR guidance messages (which useEventLevel.LogAlways) are always displayed to users, even when not running in verbose mode. This aligns well with the PR objective.
139-141: LGTM!Minor formatting adjustment; logic remains unchanged.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| var hasValidSslCertDir = false; | ||
|
|
||
| // Check if SSL_CERT_DIR is already set and if certDir is already included | ||
| var existingSslCertDir = Environment.GetEnvironmentVariable(OpenSslCertificateDirectoryVariableName); | ||
| if (!string.IsNullOrEmpty(existingSslCertDir)) | ||
| { | ||
| var existingDirs = existingSslCertDir.Split(Path.PathSeparator); | ||
| var certDirFullPath = Path.GetFullPath(prettyCertDir); | ||
| var isCertDirIncluded = existingDirs.Any(dir => | ||
| { | ||
| if (string.IsNullOrWhiteSpace(dir)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| try | ||
| { | ||
| return string.Equals(Path.GetFullPath(dir), certDirFullPath, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| catch | ||
| { | ||
| // Ignore invalid directory entries in SSL_CERT_DIR | ||
| return false; | ||
| } | ||
| }); | ||
|
|
||
| if (isCertDirIncluded) | ||
| { | ||
| // The certificate directory is already in SSL_CERT_DIR, no action needed | ||
| Log.UnixOpenSslCertificateDirectoryAlreadyConfigured(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = true; | ||
| } | ||
| else | ||
| { | ||
| // SSL_CERT_DIR is set but doesn't include our directory - suggest appending | ||
| Log.UnixSuggestAppendingToEnvironmentVariable(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
| } | ||
| else if (TryGetOpenSslDirectory(out var openSslDir)) | ||
| { | ||
| Log.UnixSuggestSettingEnvironmentVariable(prettyCertDir, Path.Combine(openSslDir, "certs"), OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
| else | ||
| { | ||
| Log.UnixSuggestSettingEnvironmentVariableWithoutExample(prettyCertDir, OpenSslCertificateDirectoryVariableName); | ||
| hasValidSslCertDir = false; | ||
| } | ||
|
|
||
| sawTrustFailure = !hasValidSslCertDir; |
There was a problem hiding this comment.
Bug: Path.GetFullPath on prettyCertDir may fail when it contains $HOME.
At line 365, Path.GetFullPath(prettyCertDir) is called, but prettyCertDir can contain the literal string $HOME (see lines 354-356). Path.GetFullPath does not perform shell variable expansion, so this will produce an incorrect path like "/<cwd>/$HOME/.aspnet/dev-certs/trust" instead of the actual home directory path.
You should use certDir (the actual filesystem path) instead of prettyCertDir (the display-friendly path) for the comparison:
🐛 Proposed fix
// Check if SSL_CERT_DIR is already set and if certDir is already included
var existingSslCertDir = Environment.GetEnvironmentVariable(OpenSslCertificateDirectoryVariableName);
if (!string.IsNullOrEmpty(existingSslCertDir))
{
var existingDirs = existingSslCertDir.Split(Path.PathSeparator);
- var certDirFullPath = Path.GetFullPath(prettyCertDir);
+ var certDirFullPath = Path.GetFullPath(certDir);
var isCertDirIncluded = existingDirs.Any(dir =>
{🤖 Prompt for AI Agents
In `@src/Shared/CertificateGeneration/UnixCertificateManager.cs` around lines 358
- 408, The code currently calls Path.GetFullPath(prettyCertDir) when computing
certDirFullPath for comparison, but prettyCertDir can contain unexpanded tokens
like "$HOME"; replace uses of Path.GetFullPath(prettyCertDir) with
Path.GetFullPath(certDir) (or use certDir directly) when computing
certDirFullPath and performing the equality check inside the existingDirs.Any
lambda (keep Log calls using prettyCertDir for display), so isCertDirIncluded
compares actual filesystem paths (use symbols: prettyCertDir, certDir,
certDirFullPath, existingSslCertDir, existingDirs, isCertDirIncluded).
2 participants
Benchmark PR from qodo-benchmark#82
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.