Skip to content

Ensure SSL_CERT_DIR messages are always shown and check for existing value #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tomerqodo wants to merge 6 commits into
base: claude_claude_vs_qodo_base_ensure_ssl_cert_dir_messages_are_always_shown_and_check_for_existing_value_pr3
Choose a base branch
from
Open

Ensure SSL_CERT_DIR messages are always shown and check for existing value #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fade7c7
Improve the SSL_CERT_DIR messaging on Unix systems.
danegsta Jan 7, 2026
60be2ab
Use critical for log level filter
danegsta Jan 7, 2026
9fd206b
Update src/Shared/CertificateGeneration/UnixCertificateManager.cs
danegsta Jan 7, 2026
35584d7
Update src/Shared/CertificateGeneration/CertificateManager.cs
danegsta Jan 7, 2026
c8ce09d
Treat invalid SSL_CERT_DIR as a partial success during trust
danegsta Jan 8, 2026
07d6e09
update pr
tomerqodo Jan 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/Shared/CertificateGeneration/CertificateManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -966,9 +966,6 @@ internal static bool TryFindCertificateInStore(X509Store store, X509Certificate2
return foundCertificate is not null;
}

/// <remarks>
/// Note that dotnet-dev-certs won't display any of these, regardless of level, unless --verbose is passed.
/// </remarks>
[EventSource(Name = "Dotnet-dev-certs")]
public sealed class CertificateManagerEventSource : EventSource
{
Expand Down Expand Up @@ -1303,7 +1300,7 @@ public sealed class CertificateManagerEventSource : EventSource
internal void UnixNotOverwritingCertificate(string certPath) => WriteEvent(109, certPath);

[Event(110, Level = EventLevel.LogAlways, Message = "For OpenSSL trust to take effect, '{0}' must be listed in the {2} environment variable. " +
"For example, `export SSL_CERT_DIR={0}:{1}`. " +
"For example, `export {2}=\"{0}:{1}\"`. " +
"See https://aka.ms/dev-certs-trust for more information.")]
internal void UnixSuggestSettingEnvironmentVariable(string certDir, string openSslDir, string envVarName) => WriteEvent(110, certDir, openSslDir, envVarName);

Comment on lines 1302 to 1306
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing bug: Event 111 (UnixSuggestSettingEnvironmentVariableWithoutExample) message uses {2} placeholder but the method only accepts 2 parameters (certDir={0}, envVarName={1}), so {2} has no corresponding argument. This will cause a FormatException at runtime when string.Format in ReporterEventListener.OnEventWritten tries to resolve {2} with only 2 payload items. The fix is to change {2} to {1} in the message string. This is a pre-existing issue not introduced by this PR, but it is directly adjacent to the modified Event 110.

Extended reasoning...

What the bug is

Event 111 (UnixSuggestSettingEnvironmentVariableWithoutExample) in CertificateManager.cs has a message string that references placeholder {2}:

[Event(111, ..., Message = "For OpenSSL trust to take effect, '{0}' must be listed in the {2} environment variable. ...")]
internal void UnixSuggestSettingEnvironmentVariableWithoutExample(string certDir, string envVarName) => WriteEvent(111, certDir, envVarName);

The method only has two parameters: certDir (index 0) and envVarName (index 1). There is no parameter at index 2.

How it manifests at runtime

In ReporterEventListener.OnEventWritten (ReporterEventListener.cs:33), the event message is formatted using:

var message = string.Format(CultureInfo.InvariantCulture, eventData.Message ?? "", eventData.Payload?.ToArray() ?? Array.Empty<object>());

When Event 111 fires, string.Format receives a format string containing {2} but only a 2-element payload array (indices 0 and 1). This causes a FormatException to be thrown at runtime.

Step-by-step proof

  1. User runs dotnet dev-certs https --trust on Linux without --verbose.
  2. OpenSSL trust succeeds but SSL_CERT_DIR is not set.
  3. TryGetOpenSslDirectory returns false (e.g., OpenSSL is not installed or returns an unexpected format).
  4. Code falls into the else branch in UnixCertificateManager.TrustCertificateCore, calling Log.UnixSuggestSettingEnvironmentVariableWithoutExample(prettyCertDir, OpenSslCertificateDirectoryVariableName).
  5. The ReporterEventListener receives Event 111 with Payload = [prettyCertDir, "SSL_CERT_DIR"] (2 items).
  6. string.Format tries to resolve {2} in the message but there is no index 2 in the payload array.
  7. A FormatException is thrown, crashing the event listener.

Why existing code does not prevent it

The EventSource infrastructure does not validate that format string placeholders match method parameter counts at compile time. The mismatch is only caught at runtime when the message is actually formatted. Previously this event was only delivered when --verbose was passed (the old code only created the listener in verbose mode), making this harder to hit. With this PR now enabling event listening in non-verbose mode at EventLevel.LogAlways, Event 111 (which is LogAlways level) will always be delivered to the listener, making this crash more likely to be exercised.

Impact

When a user trusts a dev certificate on Linux and TryGetOpenSslDirectory fails, the tool will crash with a FormatException instead of showing a helpful message about setting SSL_CERT_DIR.

Fix

Change {2} to {1} in the Event 111 message string, since envVarName is the second parameter (index 1). Compare with the sibling Event 110, where {2} correctly maps to the third parameter envVarName (which is the third of three parameters in that method).

Expand All @@ -1313,6 +1310,14 @@ public sealed class CertificateManagerEventSource : EventSource

[Event(112, Level = EventLevel.Warning, Message = "Directory '{0}' may be readable by other users.")]
internal void DirectoryPermissionsNotSecure(string directoryPath) => WriteEvent(112, directoryPath);

[Event(113, Level = EventLevel.Verbose, Message = "The certificate directory '{0}' is already included in the {1} environment variable.")]
internal void UnixOpenSslCertificateDirectoryAlreadyConfigured(string certDir, string envVarName) => WriteEvent(113, certDir, envVarName);

[Event(114, Level = EventLevel.LogAlways, Message = "For OpenSSL trust to take effect, '{0}' must be listed in the {1} environment variable. " +
"For example, `export {1}=\"{0}:${1}\"`. " +
"See https://aka.ms/dev-certs-trust for more information.")]
internal void UnixSuggestAppendingToEnvironmentVariable(string certDir, string envVarName) => WriteEvent(114, certDir, envVarName);
}

internal sealed class UserCancelledTrustException : Exception
Expand Down
60 changes: 56 additions & 4 deletions src/Shared/CertificateGeneration/UnixCertificateManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,14 +355,57 @@ protected override TrustLevel TrustCertificateCore(X509Certificate2 certificate)
? Path.Combine("$HOME", certDir[homeDirectoryWithSlash.Length..])
: certDir;

if (TryGetOpenSslDirectory(out var openSslDir))
var hasValidSslCertDir = false;

// Check if SSL_CERT_DIR is already set and if certDir is already included
Comment on lines +358 to +360
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Bug: Line 408 uses sawTrustFailure = !hasValidSslCertDir which unconditionally overwrites sawTrustFailure, erasing any prior failure state from dotnet trust, OpenSSL trust, or NSS DB trust steps. If the cert directory is already in SSL_CERT_DIR (hasValidSslCertDir is true), sawTrustFailure becomes false even though earlier steps failed, causing TrustLevel.Full to be returned instead of TrustLevel.Partial. Fix: use sawTrustFailure |= !hasValidSslCertDir to preserve earlier failures.

Extended reasoning...

What the bug is

In TrustCertificateCore, the variable sawTrustFailure tracks whether any trust step has failed. Multiple earlier code paths can set it to true: dotnet trust failure (line ~217), OpenSSL trust failure (line 295), NSS DB trust failures (lines 322, 328), and missing certutil (line 304). However, line 408 unconditionally assigns sawTrustFailure = !hasValidSslCertDir, which overwrites the accumulated failure state.

The specific code path that triggers it

Consider this scenario:

  1. OpenSSL trust succeeds (openSslTrustSucceeded = true)
  2. NSS DB trust fails for a browser (e.g., Firefox) -- sawTrustFailure is set to true at line 322 or 328
  3. The partial success check at lines 333-344 logs the warning but does NOT return (because sawTrustSuccess is also true)
  4. Execution enters the if (openSslTrustSucceeded) block at line 346
  5. SSL_CERT_DIR already contains the cert directory, so hasValidSslCertDir is set to true
  6. Line 408: sawTrustFailure = !hasValidSslCertDir becomes sawTrustFailure = false
  7. The return at line 411-413 evaluates sawTrustFailure as false and returns TrustLevel.Full

Why existing code does not prevent it

The check at lines 333-344 correctly detects the partial failure state and logs it, but it intentionally does not return early when there has been partial success (to allow for the SSL_CERT_DIR messaging). This is by design -- the method is supposed to continue and return Partial at line 411. However, the unconditional assignment at line 408 destroys the failure information that line 411 needs to make the correct decision.

Impact

When trust partially fails (e.g., NSS DB trust for Firefox fails, but OpenSSL and dotnet trust succeed) AND the certificate directory is already in SSL_CERT_DIR, the method incorrectly reports TrustLevel.Full. The caller (EnsureAspNetCoreHttpsDevelopmentCertificate) will then report complete success instead of PartiallyFailedToTrustTheCertificate, hiding the fact that the certificate is not trusted in some browsers.

How to fix

Change line 408 from:

sawTrustFailure = !hasValidSslCertDir;

to:

sawTrustFailure |= !hasValidSslCertDir;

The |= operator preserves any prior true value of sawTrustFailure while still setting it to true if hasValidSslCertDir is false.

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);
Comment on lines +365 to +375
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Path.GetFullPath(prettyCertDir) on line 365 resolves the literal string $HOME as a directory name (e.g. /cwd/$HOME/.aspnet/dev-certs/trust) because .NET does not expand shell variables, so the SSL_CERT_DIR comparison on line 375 always fails when the cert directory is under the home directory. This means the code always suggests appending to SSL_CERT_DIR even when it already contains the correct directory. Use certDir (the actual filesystem path) instead of prettyCertDir for the GetFullPath call, and change StringComparison.OrdinalIgnoreCase to StringComparison.Ordinal on line 375 since Unix paths are case-sensitive.

Extended reasoning...

Bug 1: Path.GetFullPath on prettyCertDir containing literal $HOME

On lines 354-355, prettyCertDir is set to Path.Combine("$HOME", certDir[homeDirectoryWithSlash.Length..]) when the cert directory starts with the home directory. This produces a display-friendly string like "$HOME/.aspnet/dev-certs/trust". On line 365, Path.GetFullPath(prettyCertDir) is called on this value.

Since Path.GetFullPath is a .NET API and does NOT expand shell environment variables, $HOME is treated as a literal directory name. The result is resolved relative to the current working directory, producing something like "/some/cwd/$HOME/.aspnet/dev-certs/trust". This bogus path is stored in certDirFullPath and used in the comparison on line 375.

Step-by-step proof:

  1. Suppose homeDirectory = /home/user, so homeDirectoryWithSlash = /home/user/
  2. certDir = /home/user/.aspnet/dev-certs/trust (starts with homeDirectoryWithSlash)
  3. prettyCertDir = Path.Combine("$HOME", ".aspnet/dev-certs/trust") = "$HOME/.aspnet/dev-certs/trust"
  4. certDirFullPath = Path.GetFullPath("$HOME/.aspnet/dev-certs/trust") = "/some/cwd/$HOME/.aspnet/dev-certs/trust" (bogus path)
  5. SSL_CERT_DIR contains /home/user/.aspnet/dev-certs/trust
  6. Path.GetFullPath("/home/user/.aspnet/dev-certs/trust") = /home/user/.aspnet/dev-certs/trust
  7. Comparison: "/home/user/.aspnet/dev-certs/trust" == "/some/cwd/$HOME/.aspnet/dev-certs/trust" → false
  8. Result: Code incorrectly reports the directory is not in SSL_CERT_DIR and suggests appending it

This is the common case since the default cert directory is always under the home directory. The fix is to use certDir (the actual filesystem path) instead of prettyCertDir for the Path.GetFullPath comparison, reserving prettyCertDir only for display/logging purposes.

Bug 2: OrdinalIgnoreCase comparison for Unix paths

On line 375, the comparison uses StringComparison.OrdinalIgnoreCase. UnixCertificateManager only runs on Linux, where file paths are case-sensitive. Using case-insensitive comparison could cause a false positive match where differently-cased paths (e.g. /home/user/.aspnet/Dev-Certs/Trust vs /home/user/.aspnet/dev-certs/trust) are incorrectly treated as equal. This is inconsistent with line 354, which correctly uses StringComparison.Ordinal. The comparison should use StringComparison.Ordinal to match Unix filesystem semantics.

While bug 2 has lower practical impact (case differences in paths are uncommon), it is a correctness issue in new code and should be fixed alongside bug 1.

}
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;
}

return sawTrustFailure
Expand Down Expand Up @@ -948,9 +991,18 @@ private static bool TryRehashOpenSslCertificates(string certificateDirectory)
return true;
}

private sealed class NssDb(string path, bool isFirefox)
private sealed class NssDb
{
public string Path => path;
public bool IsFirefox => isFirefox;
private readonly string _path;
private readonly bool _isFirefox;

public NssDb(string path, bool isFirefox)
{
_path = path;
_isFirefox = isFirefox;
}

public string Path => _path;
public bool IsFirefox => _isFirefox;
}
}
8 changes: 6 additions & 2 deletions src/Tools/dotnet-dev-certs/src/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,16 +124,20 @@ public static int Main(string[] args)
{
var reporter = new ConsoleReporter(PhysicalConsole.Singleton, verbose.HasValue(), quiet.HasValue());

var listener = new ReporterEventListener(reporter);
if (verbose.HasValue())
{
var listener = new ReporterEventListener(reporter);
listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.Verbose);
}
else
{
listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.LogAlways);
Copy link
Copy Markdown

@claude claude Bot Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 EventLevel.LogAlways (value 0) disables level filtering in EventSource, causing all Warning/Error events (NSS trust failures, OpenSSL issues, etc.) to be shown to users in non-verbose mode. This was correctly fixed to EventLevel.Critical in commit 60be2ab but accidentally reverted in this PR. Change EventLevel.LogAlways back to EventLevel.Critical on line 134.

Extended reasoning...

The Bug

On line 134 of Program.cs, the non-verbose code path calls listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.LogAlways). The intent is to only receive LogAlways-level events (the SSL_CERT_DIR suggestion messages, events 110, 111, 114). However, EventLevel.LogAlways has the numeric value 0, and in .NET's EventSource implementation, level 0 means no level filtering at all — every event passes through regardless of its level.

How EventSource Filtering Works

The EventSource.IsEnabled check works like this:

if ((int)m_level != 0) {
    if ((int)m_level < (int)level) return false;
}

When m_level is 0 (LogAlways), the entire filtering block is skipped, so all events are delivered. This is documented behavior: "LogAlways: No level filtering is done on the event."

Step-by-Step Proof

  1. User runs dotnet dev-certs https --trust without --verbose.
  2. Code enters the else branch at line 133 and calls EnableEvents with EventLevel.LogAlways (value 0).
  3. During trust operations, UnixCertificateManager.TrustCertificateCore encounters an NSS database issue and fires UnixNssDbTrustFailed (Event 90, EventLevel.Warning).
  4. Because level filtering is disabled (level = 0), this Warning event is delivered to ReporterEventListener.
  5. ReporterEventListener maps Warning events to _reporter.Warn(), which always writes to console (it is not gated by verbose mode).
  6. The user sees unexpected warning output about NSS databases in non-verbose mode.

Impact

The CertificateManagerEventSource has dozens of Warning and Error level events (events 34, 37, 41, 71, 74, 75, 76, 77, 82-86, 88, 90, 92-101, 103-106, 108, 109, 112, etc.). All of these will be delivered to the listener and displayed to users, producing noisy, confusing output during normal dotnet dev-certs https --trust operations.

Git History Confirms the Regression

Commit 60be2ab ("Use critical for log level filter") correctly changed EventLevel.LogAlways to EventLevel.Critical (value 1). With EventLevel.Critical, only LogAlways (0) and Critical (1) events pass the filter — exactly the intended behavior. This PR's commit 07d6e09 ("update pr") reverted that fix back to EventLevel.LogAlways.

Fix

Change EventLevel.LogAlways to EventLevel.Critical on line 134:

listener.EnableEvents(CertificateManager.Log, System.Diagnostics.Tracing.EventLevel.Critical);

This enables only LogAlways (value 0) and Critical (value 1) events, which is the correct filtering for non-verbose mode.

}

if (checkJsonOutput.HasValue())
{
if (exportPath.HasValue() || trust?.HasValue() == true || format.HasValue() || noPassword.HasValue() || check.HasValue() || clean.HasValue() ||
(!import.HasValue() && password.HasValue()) ||
(!import.HasValue() && password.HasValue()) ||
(import.HasValue() && !password.HasValue()))
{
reporter.Error(InvalidUsageErrorMessage);
Expand Down