Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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);

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
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
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 =>
Comment on lines +364 to +366

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

4. $home used for comparison 🐞 Bug ✓ Correctness

• The new SSL_CERT_DIR inclusion check compares Path.GetFullPath(prettyCertDir) even though
  prettyCertDir may contain the literal $HOME prefix for display.
• This makes the comparison against real filesystem paths in SSL_CERT_DIR fail, causing false “not
  configured” guidance and forcing TrustLevel.Partial even when the env var is correctly set.
Agent prompt
### Issue description
The SSL_CERT_DIR validation uses `prettyCertDir` (which may contain a literal `$HOME` for display) for filesystem comparisons, causing false negatives.

### Issue Context
`prettyCertDir` is designed for user-friendly output; it should not be fed into `Path.GetFullPath` for equality checks.

### Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[354-366]
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[373-377]

### Suggested change
- Compute comparison target from the real path:
  - `var certDirFullPath = Path.GetFullPath(certDir);`
- Keep logging/UX using `prettyCertDir`.
- (Optional) On Unix, consider using `StringComparison.Ordinal` rather than `OrdinalIgnoreCase` for path equality.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

{
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;
}
Comment on lines +373 to +381

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remediation recommended

2. Bare catch ignores exceptions 📘 Rule violation ⛯ Reliability

• Parsing SSL_CERT_DIR entries uses a bare catch that silently ignores all exceptions from
  Path.GetFullPath(dir).
• This can mask unexpected failures (beyond just malformed entries) and removes actionable context
  needed to diagnose trust issues.
• Catch specific exceptions and/or log at Verbose with safe context (e.g., exception type and the
  offending entry) to preserve debuggability.
Agent prompt
## Issue description
A bare `catch` swallows all exceptions when processing entries from `SSL_CERT_DIR`, which can hide unexpected issues and violates the robust error-handling requirement.

## Issue Context
The code is attempting to ignore malformed directory entries, but it currently suppresses every exception type without any diagnostic trail.

## Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[360-382]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

});

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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

3. Trust flag overwritten 🐞 Bug ✓ Correctness

TrustCertificateCore now reassigns sawTrustFailure based only on SSL_CERT_DIR validity,
  overwriting earlier failures from dotnet store / NSS trust steps.
• This can incorrectly return TrustLevel.Full (and avoid non-zero exit) even when earlier trust
  steps failed, misleading users about actual trust state.
Agent prompt
### Issue description
`UnixCertificateManager.TrustCertificateCore` overwrites `sawTrustFailure` with `!hasValidSslCertDir`, losing earlier failures from other trust steps and potentially returning `TrustLevel.Full` incorrectly.

### Issue Context
`sawTrustFailure` is an accumulator across dotnet/OpenSSL/NSS trust operations and drives the final `TrustLevel`.

### Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[408-413]

### Suggested change
- Replace `sawTrustFailure = !hasValidSslCertDir;` with `sawTrustFailure |= !hasValidSslCertDir;` (or `if (!hasValidSslCertDir) sawTrustFailure = true;`).
- Consider keeping a separate variable for “OpenSSL trust effective for external clients” if you want distinct reporting, but don’t lose prior failures.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}

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;
Comment on lines +994 to +1006

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. nssdb avoids primary constructor 📘 Rule violation ✓ Correctness

NssDb was changed from a C# 12 primary constructor to a traditional constructor with repetitive
  parameter-to-field assignment.
• This adds boilerplate and diverges from the required modern DI/boilerplate-reduction convention,
  making the code less concise and consistent.
• Reverting to a primary constructor would satisfy the requirement and keep intent clear.
Agent prompt
## Issue description
`NssDb` was rewritten to use a traditional constructor and backing fields, which violates the requirement to use C# primary constructors where the class is a simple parameter-to-field mapping.

## Issue Context
This is a small private sealed helper type that previously fit the primary-constructor pattern.

## Fix Focus Areas
- src/Shared/CertificateGeneration/UnixCertificateManager.cs[994-1006]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

}
}
8 changes: 6 additions & 2 deletions src/Tools/dotnet-dev-certs/src/Program.cs
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);
}
Comment on lines +127 to +135

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

5. Logalways output can crash 🐞 Bug ⛯ Reliability

dotnet-dev-certs now enables CertificateManager.Log at EventLevel.LogAlways even when not
  verbose.
• Event 111’s message uses a {2} placeholder but only writes two payload values;
  ReporterEventListener formats messages with string.Format without handling FormatException,
  which can crash the tool when that event is emitted.
Agent prompt
### Issue description
Event 111 uses `{2}` in its message but only supplies two payload arguments, and `ReporterEventListener` calls `string.Format` without catching `FormatException`.

### Issue Context
This becomes more likely to surface because Program now enables LogAlways events even when not `--verbose`.

### Fix Focus Areas
- src/Shared/CertificateGeneration/CertificateManager.cs[1307-1309]
- src/Tools/dotnet-dev-certs/src/ReporterEventListener.cs[21-35]

### Suggested change
- Correct Event 111 message to use `{1}` (envVarName) instead of `{2}`.
- (Optional hardening) Wrap the `string.Format(...)` call in a try/catch and fall back to the raw message/payload dump to avoid crashing on malformed event templates.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


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