-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(remote): custom certificate validation with single execution path - fixes mTLS asymmetry bug #7915
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.
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
feat(remote): custom certificate validation with single execution path - fixes mTLS asymmetry bug #7915
Conversation
…ateValidation helper factory - Define public CertificateValidationCallback delegate for custom certificate validation - Add CertificateValidation factory class with 7 helper methods: * ValidateChain() - CA chain validation * ValidateHostname() - CN/SAN matching * PinnedCertificate() - Certificate pinning by thumbprint * ValidateSubject() - Subject DN matching (with wildcard support) * ValidateIssuer() - Issuer DN matching * Combine() - Compose multiple validators * ChainPlusThen() - Chain validation + custom logic - Add CustomValidator property to DotNettySslSetup with overloaded constructors - Maintain full backward compatibility with existing config-based validation Relates to akkadotnet#7914
…ion with hostname validation asymmetry fix - Integrate custom certificate validators into DotNettyTransport pipelines (client and server) - Implement single execution path: compose validator from config when custom not provided - Add ComposeValidatorFromSettings() to build validators from SuppressValidation and ValidateCertificateHostname settings - Add CustomValidator property to SslSettings with updated constructors for seamless integration - Fix asymmetry bug: server-side now applies hostname validation like client-side - Replace dual-path logic (custom vs config-based) with unified composition pattern - Add hostname matching helper with reflection-based SAN support for multi-framework compatibility - Eliminates need for TlsValidationCallbacks on each pipeline setup call
… execution path - Revert to using proven TlsValidationCallbacks logic for configuration-based validation - This maintains compatibility with existing validation behavior while enabling single execution path - CertificateValidation helpers remain available for custom user validators - Reduces test failures from 9 to 2 by using well-tested validation logic
…idation When the server requires mutual TLS authentication (RequireMutualAuthentication=true), it must reject TLS handshakes where the client fails to provide a certificate. Previously, the validation callback would pass a null certificate to the composed validator without pre-checking it. This allowed connections from clients without certificates to succeed when they should fail. Now we explicitly check if the certificate is null when mutual auth is required and immediately reject the connection with a warning log message. This fixes the failing test: Mutual_TLS_should_fail_when_client_has_no_certificate Fixes: All 329 tests now pass (324 passed, 5 skipped, 0 failed)
…e security documentation Adds comprehensive programmatic certificate validation examples to TlsConfigurationSample: - ProgrammaticMutualTlsSetup: Basic mutual TLS with custom validators - CertificatePinningExample: Certificate pinning by thumbprint - CustomValidationLogicExample: Chain validation + custom business logic - HostnameValidationExample: Programmatic hostname validation setup - SubjectValidationExample: Subject DN validation Consolidates security.md documentation: - Merged "Hostname Validation" and "Mutual TLS Authentication" into unified "Validation Strategies: HOCON vs Programmatic" section with decision matrix - Added examples for both P2P clusters and client-server architectures - Cross-referenced sections to reduce duplication - Clarified when to use programmatic vs HOCON configuration Follows documentation guidelines (security.md:70): - Uses !code references with #region tags for live code examples - Organizes content for discoverability - Provides decision matrix for choosing validation strategy
…on examples - Add Akka.Remote reference to Akka.Docs.Tests project for DotNettySslSetup types - Wrap new programmatic examples with #if NET6_0_OR_GREATER for framework compatibility - Convert example methods to void to simplify documentation-only code - Fix API usage: use params instead of arrays, correct delegate signatures - Remove BootstrapSetup complexity from examples to focus on core TLS setup patterns
Arkatufus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions
…ntegration tests CRITICAL FIXES: - Fixed DotNettySslSetup.Settings to pass CustomValidator to SslSettings constructor (Line 114 was creating SslSettings without the CustomValidator parameter) - Removed unused ValidateCertificateHostnameMatch method (489-542) (Hostname validation is already handled by TlsValidationCallbacks.Create) NEW TESTS - CustomValidator Functionality: - CustomValidator_that_accepts_should_allow_connection * Verifies CustomValidator callback is invoked during TLS handshake * Verifies acceptance allows successful connection - CustomValidator_that_rejects_should_prevent_connection * Verifies CustomValidator rejection prevents connection * Verifies callback was invoked even when rejecting - DotNettySslSetup_should_pass_CustomValidator_to_SslSettings * Unit test verifying CustomValidator is wired through to SslSettings Addresses PR review comments akkadotnet#7915: - CustomValidator now properly wired to SslSettings - Removed dead code (ValidateCertificateHostnameMatch) - Added real integration tests that validate CustomValidator actually works
Review Comments AddressedThank you for catching these critical issues! I've pushed fixes in commit 29ec21c. Issue 1: CustomValidator not passed to SslSettings (Line 112)Fixed: Line 114 in internal SslSettings Settings => new SslSettings(Certificate, SuppressValidation, RequireMutualAuthentication, ValidateCertificateHostname, CustomValidator);Issue 2: ValidateCertificateHostnameMatch unused (Line 494)Fixed: Removed the unused method (lines 489-542 in DotNettyTransport.cs). Hostname validation is already handled by Issue 3: Tests don't actually test CustomValidator functionalityFixed: Added 3 comprehensive integration tests to
Test Results: All 3 new tests pass ✓ These tests actually validate that:
|
Concern: Mixing HOCON with DotNettySslSetupYou're right to be concerned about this. The current precedence rules are: DotNettySslSetup takes precedence over HOCON (fixed in PR #7918) However, this could be confusing for users. Here are a few options to clarify: Option 1: Add Warning LogsAdd a warning when both are configured: if (setup.HasValue && hoconConfig.HasCertificate)
{
log.Warning("Both DotNettySslSetup and HOCON SSL config detected. DotNettySslSetup takes precedence.");
}Option 2: Documentation ClarityAlready addressed in this PR - the security.md includes a decision matrix showing "Validation Strategies: HOCON vs Programmatic" with clear guidance on when to use each. Option 3: Make it Mutually ExclusiveThrow an exception if both are configured: if (setup.HasValue && hoconConfig.HasCertificate)
{
throw new ConfigurationException(
"Cannot configure both DotNettySslSetup and HOCON SSL settings. Choose one approach.");
}Recommendation: I'd suggest Option 2 (current approach) with maybe Option 1 (warning logs) added as a safety net. Making it mutually exclusive (Option 3) would be a breaking change and might be too strict. The precedence behavior (setup > HOCON) makes sense because:
What's your preference? I can add warning logs if that would help. |
…ig are present - Logs warning when DotNettySslSetup is used alongside explicit HOCON certificate configuration - Only warns when HOCON has actual certificate.path or certificate.thumbprint configured - Avoids false positives from default/empty config sections - Adds test verifying DotNettySslSetup precedence behavior - Addresses PR feedback: implement Option 1 from review comment
- All types used (X509Certificate2, DotNettySslSetup, CertificateValidation) are available in .NET Standard 2.0 - Conditional directives were added during troubleshooting but are not needed - Verified compilation on both net8.0 and net48 targets
Removes internal TlsValidationCallbacks class and related enums (~145 lines) and refactors ComposeValidatorFromSettings() to use the public CertificateValidation helpers instead. Changes: - Remove ChainValidationMode and HostnameValidationMode enums - Remove TlsValidationCallbacks internal class - Refactor ComposeValidatorFromSettings() to handle all 4 combinations of SuppressValidation and ValidateCertificateHostname flags: * suppressChain=true, validateHostname=false → Accept all * suppressChain=true, validateHostname=true → Validate hostname only * suppressChain=false, validateHostname=true → Chain + hostname * suppressChain=false, validateHostname=false → Chain only (default) Benefits: - Eliminates code duplication between internal and public APIs - Simplifies maintenance by having a single validation implementation - Makes the public CertificateValidation API the canonical approach - All 43 DotNetty tests pass including edge case validations
Aaronontheweb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First pass at a review - found a bunch of stuff to fix already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably need to simplify and clean this up in a separate PR, but the new cert validation strategies are documented here.
| namespace Akka.Remote.Transport.DotNetty | ||
| { | ||
| [System.Runtime.CompilerServices.NullableAttribute(0)] | ||
| public class static CertificateValidation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New set of helpers for defining certificate validation callbacks
| public DotNettySslSetup(System.Security.Cryptography.X509Certificates.X509Certificate2 certificate, bool suppressValidation) { } | ||
| public DotNettySslSetup(System.Security.Cryptography.X509Certificates.X509Certificate2 certificate, bool suppressValidation, bool requireMutualAuthentication) { } | ||
| public DotNettySslSetup(System.Security.Cryptography.X509Certificates.X509Certificate2 certificate, bool suppressValidation, bool requireMutualAuthentication, bool validateCertificateHostname) { } | ||
| public DotNettySslSetup(System.Security.Cryptography.X509Certificates.X509Certificate2 certificate, bool suppressValidation, bool requireMutualAuthentication, [System.Runtime.CompilerServices.NullableAttribute(2)] Akka.Remote.Transport.DotNetty.CertificateValidationCallback customValidator) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CTOR overload for adding the custom CertificationValidationCallabck to the DotNettySslSetup. This will need to be integrated into Akka.Hosting.
| /// Validate certificate chain against system CA store. | ||
| /// Use for: CA-signed certificates in production. | ||
| /// </summary> | ||
| public static CertificateValidationCallback ValidateChain( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
XML-DOC comment is accurate, but yes: validates the CA chain.
Added 11 new tests to achieve 100% coverage of previously untested CertificateValidation helper methods: PinnedCertificate tests: - Accept connections with matching thumbprint - Reject connections with non-matching thumbprint ValidateSubject tests: - Accept certificates with matching subject - Reject certificates with non-matching subject - Support wildcard pattern matching (CN=Akka-Node-*) ValidateIssuer tests: - Accept certificates with matching issuer Combine/ChainPlusThen tests: - Verify composability of validators CustomValidator precedence tests: - Verify CustomValidator overrides validateCertificateHostname setting Also removed obsolete Mono checks from all new tests per maintainer guidance (Mono is no longer supported). Test results: 18/18 passing (7 existing + 11 new)
Simplified two test cases that were unnecessarily wrapping single CertificateValidationCallback delegates in Combine(): - CustomValidator_that_accepts_should_allow_connection - CustomValidator_that_rejects_should_prevent_connection Changed from: var validator = CertificateValidation.Combine((cert, ...) => true); To cleaner direct delegate assignment: CertificateValidationCallback validator = (cert, ...) => true; Combine() is only needed when composing multiple validators. These tests verify single custom validators, so direct assignment is clearer. All tests still pass (4/4 CustomValidator tests verified).
- Removed #if !NET471 conditional compilation directives (10 instances) Project now targets net48, making NET471 conditionals meaningless - Removed if (IsMono) runtime checks (7 instances) Modern .NET uses CoreCLR cross-platform, not Mono - All SSL tests now run unconditionally on supported platforms - Tests verified passing: 27/27 on net8.0, 26/26 on net48
- Added explicit null checks to all CertificateValidation helper methods - PinnedCertificate: Check for null cert and filter empty thumbprints - ValidateSubject/ValidateIssuer: Check for null cert and empty values - ValidateHostname: Check for null cert before accessing properties - ValidateChain: Check for null cert before chain validation - Improved error messages to distinguish null cert from other failures - Added comprehensive unit test coverage for edge cases - Prevents potential NullReferenceException in TLS handshake scenarios
…on is safe Added detailed comment explaining: - Thumbprints are hexadecimal SHA hash representations - Hex values are inherently case-insensitive (2A8B == 2a8b) - Different tools display differently (Windows vs OpenSSL) - Case-insensitive comparison improves usability without compromising security
- Replaced ExpectMsg with EventFilter for proper log assertion pattern - EventFilter is the idiomatic way to assert log messages in Akka.NET tests - Added test for rejecting non-matching thumbprint with EventFilter - Updated Combine test to clearly document short-circuit behavior - All tests now properly verify both result AND expected log messages
Aaronontheweb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have another pass yet to go - hardening the mTLS end to end specs now.
| var certificate = new X509Certificate2("path/to/certificate.pfx", "password"); | ||
|
|
||
| // Create custom validator combining multiple validation strategies | ||
| var customValidator = CertificateValidation.Combine( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want stricter TLS validation, this is a good example of how to do it with certificate pinning.
| // </copyright> | ||
| //----------------------------------------------------------------------- | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enabled nullability here in order to catch errors in certification validation / config.
| // </copyright> | ||
| //----------------------------------------------------------------------- | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nullability enabled to catch SSL certificate errors, but caught some other potential nasties too.
| : ChainValidationMode.ValidateChain; | ||
| // Compose validator: either use custom validator or build from config settings | ||
| // This ensures a single execution path through validation logic | ||
| var validator = Settings.Ssl.CustomValidator ?? ComposeValidatorFromSettings(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use either the user-supplied certificate validation code or build one based on the SslSettings.
| } | ||
|
|
||
| [Fact(DisplayName = "PinnedCertificate should be case-insensitive for thumbprints")] | ||
| public void PinnedCertificate_should_be_case_insensitive() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment on the actual code being tested here, but there's no security issue due to thumbprints being HEX, so case sensitivity doesn't impact their underlying values
| } | ||
|
|
||
| [Fact(DisplayName = "Combine should short-circuit on first failure")] | ||
| public void Combine_should_short_circuit_on_first_failure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it simple and short-circuit. It's what ASP.NET Core et al do.
| var certificate = new X509Certificate2(ValidCertPath, Password, X509KeyStorageFlags.DefaultKeySet); | ||
|
|
||
| // Custom validator that accepts all certificates | ||
| CertificateValidationCallback customValidator = (cert, chain, peer, errors, log) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is like a hand-rolled equivalent of "suppress validation"
| var certificate = new X509Certificate2(ValidCertPath, Password, X509KeyStorageFlags.DefaultKeySet); | ||
|
|
||
| // Create validator that pins to a DIFFERENT thumbprint (connection should fail) | ||
| var validator = CertificateValidation.PinnedCertificate("0000000000000000000000000000000000000000"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-existent thumbprint
| <ProjectReference Include="..\Akka.Coordination.Tests\Akka.Coordination.Tests.csproj" /> | ||
| <ProjectReference Include="..\Akka\Akka.csproj" /> | ||
| <ProjectReference Include="..\Akka.Persistence\Akka.Persistence.csproj" /> | ||
| <ProjectReference Include="..\Akka.Remote\Akka.Remote.csproj" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not strictly necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handles a range of certificate validation edge cases to ensure that they're all caught.
… tests Updated SSL integration tests to use EventFilter for asserting specific validation errors instead of just checking connection failure. This provides better test precision by verifying the exact reason for connection failure. With mTLS enabled, validation errors occur on the server side (_sys2) when it validates the client certificate, since the client (Sys) has suppressValidation enabled. The EventFilter assertions are correctly targeted to the system where the validation errors occur. Changes: - Added EventFilter assertions to PinnedCertificate rejection test - Added EventFilter assertions to CustomValidator rejection test - Added EventFilter assertions to ValidateSubject rejection test - Modified custom validator to log error for EventFilter detection - Added comments explaining the mTLS validation flow
…r system tests" This reverts commit 2022d63.
Aaronontheweb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| if (Settings.Ssl.RequireMutualAuthentication) | ||
| { | ||
| // Mutual TLS requires a certificate to be configured | ||
| if (certificate == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a startup error / misconfiguration.
|
|
||
| return suppressChain switch | ||
| { | ||
| true when validateHostname => CertificateValidation.ValidateHostname(log: Log), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are suppressing the CA chain but are still validating on hostname, we will do that here.
| var hasCertPath = sslConfig.HasPath("certificate.path") && !string.IsNullOrWhiteSpace(sslConfig.GetString("certificate.path")); | ||
| var hasCertThumbprint = sslConfig.HasPath("certificate.thumbprint") && !string.IsNullOrWhiteSpace(sslConfig.GetString("certificate.thumbprint")); | ||
|
|
||
| if (hasCertPath || hasCertThumbprint) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue a warning if a user duped their configuration between HOCON and the DotNettySslSetup.
| ChainValidationMode chainValidation, | ||
| HostnameValidationMode hostnameValidation, | ||
| ILoggingAdapter log) | ||
| public static CertificateValidationCallback PinnedCertificate( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only accept certs with a given set of thumb prints.
| return false; | ||
| } | ||
|
|
||
| var hostname = expectedHostname ?? peer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fall back to validating our outbound peer's connect hostname if the user didn't provide one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is consistent with earlier behavior.
src/core/Akka.Remote/Transport/DotNetty/DotNettyTransportSettings.cs
Outdated
Show resolved
Hide resolved
Arkatufus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good to me, but I noticed something that we missed from the previous PR.
The condition `(errors & SslPolicyErrors.None) != SslPolicyErrors.None` was always false because SslPolicyErrors.None equals 0, and any value bitwise AND with 0 always results in 0. Changed to simple equality check `errors != SslPolicyErrors.None` to correctly detect when SSL policy errors are present. This bug prevented the TlsErrorMessageBuilder from ever building detailed error messages when SSL validation failed, making debugging harder.
…h - fixes mTLS asymmetry bug (akkadotnet#7915) * feat(remote): add CertificateValidationCallback delegate and CertificateValidation helper factory - Define public CertificateValidationCallback delegate for custom certificate validation - Add CertificateValidation factory class with 7 helper methods: * ValidateChain() - CA chain validation * ValidateHostname() - CN/SAN matching * PinnedCertificate() - Certificate pinning by thumbprint * ValidateSubject() - Subject DN matching (with wildcard support) * ValidateIssuer() - Issuer DN matching * Combine() - Compose multiple validators * ChainPlusThen() - Chain validation + custom logic - Add CustomValidator property to DotNettySslSetup with overloaded constructors - Maintain full backward compatibility with existing config-based validation Relates to akkadotnet#7914 * feat(remote): implement single execution path for certificate validation with hostname validation asymmetry fix - Integrate custom certificate validators into DotNettyTransport pipelines (client and server) - Implement single execution path: compose validator from config when custom not provided - Add ComposeValidatorFromSettings() to build validators from SuppressValidation and ValidateCertificateHostname settings - Add CustomValidator property to SslSettings with updated constructors for seamless integration - Fix asymmetry bug: server-side now applies hostname validation like client-side - Replace dual-path logic (custom vs config-based) with unified composition pattern - Add hostname matching helper with reflection-based SAN support for multi-framework compatibility - Eliminates need for TlsValidationCallbacks on each pipeline setup call * fix: use TlsValidationCallbacks for config-based validation in single execution path - Revert to using proven TlsValidationCallbacks logic for configuration-based validation - This maintains compatibility with existing validation behavior while enabling single execution path - CertificateValidation helpers remain available for custom user validators - Reduces test failures from 9 to 2 by using well-tested validation logic * fix: reject missing client certificates in server-side mutual TLS validation When the server requires mutual TLS authentication (RequireMutualAuthentication=true), it must reject TLS handshakes where the client fails to provide a certificate. Previously, the validation callback would pass a null certificate to the composed validator without pre-checking it. This allowed connections from clients without certificates to succeed when they should fail. Now we explicitly check if the certificate is null when mutual auth is required and immediately reject the connection with a warning log message. This fixes the failing test: Mutual_TLS_should_fail_when_client_has_no_certificate Fixes: All 329 tests now pass (324 passed, 5 skipped, 0 failed) * docs: add programmatic certificate validation examples and consolidate security documentation Adds comprehensive programmatic certificate validation examples to TlsConfigurationSample: - ProgrammaticMutualTlsSetup: Basic mutual TLS with custom validators - CertificatePinningExample: Certificate pinning by thumbprint - CustomValidationLogicExample: Chain validation + custom business logic - HostnameValidationExample: Programmatic hostname validation setup - SubjectValidationExample: Subject DN validation Consolidates security.md documentation: - Merged "Hostname Validation" and "Mutual TLS Authentication" into unified "Validation Strategies: HOCON vs Programmatic" section with decision matrix - Added examples for both P2P clusters and client-server architectures - Cross-referenced sections to reduce duplication - Clarified when to use programmatic vs HOCON configuration Follows documentation guidelines (security.md:70): - Uses !code references with #region tags for live code examples - Organizes content for discoverability - Provides decision matrix for choosing validation strategy * fix: correct compilation errors in TlsConfigurationSample documentation examples - Add Akka.Remote reference to Akka.Docs.Tests project for DotNettySslSetup types - Wrap new programmatic examples with #if NET6_0_OR_GREATER for framework compatibility - Convert example methods to void to simplify documentation-only code - Fix API usage: use params instead of arrays, correct delegate signatures - Remove BootstrapSetup complexity from examples to focus on core TLS setup patterns * fix: wire CustomValidator through SslSettings and add comprehensive integration tests CRITICAL FIXES: - Fixed DotNettySslSetup.Settings to pass CustomValidator to SslSettings constructor (Line 114 was creating SslSettings without the CustomValidator parameter) - Removed unused ValidateCertificateHostnameMatch method (489-542) (Hostname validation is already handled by TlsValidationCallbacks.Create) NEW TESTS - CustomValidator Functionality: - CustomValidator_that_accepts_should_allow_connection * Verifies CustomValidator callback is invoked during TLS handshake * Verifies acceptance allows successful connection - CustomValidator_that_rejects_should_prevent_connection * Verifies CustomValidator rejection prevents connection * Verifies callback was invoked even when rejecting - DotNettySslSetup_should_pass_CustomValidator_to_SslSettings * Unit test verifying CustomValidator is wired through to SslSettings Addresses PR review comments akkadotnet#7915: - CustomValidator now properly wired to SslSettings - Removed dead code (ValidateCertificateHostnameMatch) - Added real integration tests that validate CustomValidator actually works * Add warning when both DotNettySslSetup and HOCON SSL certificate config are present - Logs warning when DotNettySslSetup is used alongside explicit HOCON certificate configuration - Only warns when HOCON has actual certificate.path or certificate.thumbprint configured - Avoids false positives from default/empty config sections - Adds test verifying DotNettySslSetup precedence behavior - Addresses PR feedback: implement Option 1 from review comment * Remove unnecessary NET6_0_OR_GREATER conditional compilation directives - All types used (X509Certificate2, DotNettySslSetup, CertificateValidation) are available in .NET Standard 2.0 - Conditional directives were added during troubleshooting but are not needed - Verified compilation on both net8.0 and net48 targets * Consolidate TlsValidationCallbacks into public CertificateValidation API Removes internal TlsValidationCallbacks class and related enums (~145 lines) and refactors ComposeValidatorFromSettings() to use the public CertificateValidation helpers instead. Changes: - Remove ChainValidationMode and HostnameValidationMode enums - Remove TlsValidationCallbacks internal class - Refactor ComposeValidatorFromSettings() to handle all 4 combinations of SuppressValidation and ValidateCertificateHostname flags: * suppressChain=true, validateHostname=false → Accept all * suppressChain=true, validateHostname=true → Validate hostname only * suppressChain=false, validateHostname=true → Chain + hostname * suppressChain=false, validateHostname=false → Chain only (default) Benefits: - Eliminates code duplication between internal and public APIs - Simplifies maintenance by having a single validation implementation - Makes the public CertificateValidation API the canonical approach - All 43 DotNetty tests pass including edge case validations * cleaned up `CertificateValidation` composition code for default settings * Add comprehensive test coverage for CertificateValidation helpers Added 11 new tests to achieve 100% coverage of previously untested CertificateValidation helper methods: PinnedCertificate tests: - Accept connections with matching thumbprint - Reject connections with non-matching thumbprint ValidateSubject tests: - Accept certificates with matching subject - Reject certificates with non-matching subject - Support wildcard pattern matching (CN=Akka-Node-*) ValidateIssuer tests: - Accept certificates with matching issuer Combine/ChainPlusThen tests: - Verify composability of validators CustomValidator precedence tests: - Verify CustomValidator overrides validateCertificateHostname setting Also removed obsolete Mono checks from all new tests per maintainer guidance (Mono is no longer supported). Test results: 18/18 passing (7 existing + 11 new) * Remove unnecessary Combine() wrapper for single validators in tests Simplified two test cases that were unnecessarily wrapping single CertificateValidationCallback delegates in Combine(): - CustomValidator_that_accepts_should_allow_connection - CustomValidator_that_rejects_should_prevent_connection Changed from: var validator = CertificateValidation.Combine((cert, ...) => true); To cleaner direct delegate assignment: CertificateValidationCallback validator = (cert, ...) => true; Combine() is only needed when composing multiple validators. These tests verify single custom validators, so direct assignment is clearer. All tests still pass (4/4 CustomValidator tests verified). * added `nullability` annotations to DotNettyTransport * Remove obsolete Mono and NET471 workarounds from SSL tests - Removed #if !NET471 conditional compilation directives (10 instances) Project now targets net48, making NET471 conditionals meaningless - Removed if (IsMono) runtime checks (7 instances) Modern .NET uses CoreCLR cross-platform, not Mono - All SSL tests now run unconditionally on supported platforms - Tests verified passing: 27/27 on net8.0, 26/26 on net48 * Fix null certificate handling in SSL validation methods - Added explicit null checks to all CertificateValidation helper methods - PinnedCertificate: Check for null cert and filter empty thumbprints - ValidateSubject/ValidateIssuer: Check for null cert and empty values - ValidateHostname: Check for null cert before accessing properties - ValidateChain: Check for null cert before chain validation - Improved error messages to distinguish null cert from other failures - Added comprehensive unit test coverage for edge cases - Prevents potential NullReferenceException in TLS handshake scenarios * Add documentation explaining why case-insensitive thumbprint comparison is safe Added detailed comment explaining: - Thumbprints are hexadecimal SHA hash representations - Hex values are inherently case-insensitive (2A8B == 2a8b) - Different tools display differently (Windows vs OpenSSL) - Case-insensitive comparison improves usability without compromising security * Improve certificate validation tests with EventFilter - Replaced ExpectMsg with EventFilter for proper log assertion pattern - EventFilter is the idiomatic way to assert log messages in Akka.NET tests - Added test for rejecting non-matching thumbprint with EventFilter - Updated Combine test to clearly document short-circuit behavior - All tests now properly verify both result AND expected log messages * Use EventFilter to assert SSL validation errors in multi-actor system tests Updated SSL integration tests to use EventFilter for asserting specific validation errors instead of just checking connection failure. This provides better test precision by verifying the exact reason for connection failure. With mTLS enabled, validation errors occur on the server side (_sys2) when it validates the client certificate, since the client (Sys) has suppressValidation enabled. The EventFilter assertions are correctly targeted to the system where the validation errors occur. Changes: - Added EventFilter assertions to PinnedCertificate rejection test - Added EventFilter assertions to CustomValidator rejection test - Added EventFilter assertions to ValidateSubject rejection test - Modified custom validator to log error for EventFilter detection - Added comments explaining the mTLS validation flow * Revert "Use EventFilter to assert SSL validation errors in multi-actor system tests" This reverts commit 2022d63. * remove unnecessary project reference * added API approvals * Fix incorrect bitwise AND check with SslPolicyErrors.None The condition `(errors & SslPolicyErrors.None) != SslPolicyErrors.None` was always false because SslPolicyErrors.None equals 0, and any value bitwise AND with 0 always results in 0. Changed to simple equality check `errors != SslPolicyErrors.None` to correctly detect when SSL policy errors are present. This bug prevented the TlsErrorMessageBuilder from ever building detailed error messages when SSL validation failed, making debugging harder. --------- Co-authored-by: Gregorius Soedharmo <arkatufus@yahoo.com>
…h - fixes mTLS asymmetry bug (#7915) (#7921) * feat(remote): add CertificateValidationCallback delegate and CertificateValidation helper factory - Define public CertificateValidationCallback delegate for custom certificate validation - Add CertificateValidation factory class with 7 helper methods: * ValidateChain() - CA chain validation * ValidateHostname() - CN/SAN matching * PinnedCertificate() - Certificate pinning by thumbprint * ValidateSubject() - Subject DN matching (with wildcard support) * ValidateIssuer() - Issuer DN matching * Combine() - Compose multiple validators * ChainPlusThen() - Chain validation + custom logic - Add CustomValidator property to DotNettySslSetup with overloaded constructors - Maintain full backward compatibility with existing config-based validation Relates to #7914 * feat(remote): implement single execution path for certificate validation with hostname validation asymmetry fix - Integrate custom certificate validators into DotNettyTransport pipelines (client and server) - Implement single execution path: compose validator from config when custom not provided - Add ComposeValidatorFromSettings() to build validators from SuppressValidation and ValidateCertificateHostname settings - Add CustomValidator property to SslSettings with updated constructors for seamless integration - Fix asymmetry bug: server-side now applies hostname validation like client-side - Replace dual-path logic (custom vs config-based) with unified composition pattern - Add hostname matching helper with reflection-based SAN support for multi-framework compatibility - Eliminates need for TlsValidationCallbacks on each pipeline setup call * fix: use TlsValidationCallbacks for config-based validation in single execution path - Revert to using proven TlsValidationCallbacks logic for configuration-based validation - This maintains compatibility with existing validation behavior while enabling single execution path - CertificateValidation helpers remain available for custom user validators - Reduces test failures from 9 to 2 by using well-tested validation logic * fix: reject missing client certificates in server-side mutual TLS validation When the server requires mutual TLS authentication (RequireMutualAuthentication=true), it must reject TLS handshakes where the client fails to provide a certificate. Previously, the validation callback would pass a null certificate to the composed validator without pre-checking it. This allowed connections from clients without certificates to succeed when they should fail. Now we explicitly check if the certificate is null when mutual auth is required and immediately reject the connection with a warning log message. This fixes the failing test: Mutual_TLS_should_fail_when_client_has_no_certificate Fixes: All 329 tests now pass (324 passed, 5 skipped, 0 failed) * docs: add programmatic certificate validation examples and consolidate security documentation Adds comprehensive programmatic certificate validation examples to TlsConfigurationSample: - ProgrammaticMutualTlsSetup: Basic mutual TLS with custom validators - CertificatePinningExample: Certificate pinning by thumbprint - CustomValidationLogicExample: Chain validation + custom business logic - HostnameValidationExample: Programmatic hostname validation setup - SubjectValidationExample: Subject DN validation Consolidates security.md documentation: - Merged "Hostname Validation" and "Mutual TLS Authentication" into unified "Validation Strategies: HOCON vs Programmatic" section with decision matrix - Added examples for both P2P clusters and client-server architectures - Cross-referenced sections to reduce duplication - Clarified when to use programmatic vs HOCON configuration Follows documentation guidelines (security.md:70): - Uses !code references with #region tags for live code examples - Organizes content for discoverability - Provides decision matrix for choosing validation strategy * fix: correct compilation errors in TlsConfigurationSample documentation examples - Add Akka.Remote reference to Akka.Docs.Tests project for DotNettySslSetup types - Wrap new programmatic examples with #if NET6_0_OR_GREATER for framework compatibility - Convert example methods to void to simplify documentation-only code - Fix API usage: use params instead of arrays, correct delegate signatures - Remove BootstrapSetup complexity from examples to focus on core TLS setup patterns * fix: wire CustomValidator through SslSettings and add comprehensive integration tests CRITICAL FIXES: - Fixed DotNettySslSetup.Settings to pass CustomValidator to SslSettings constructor (Line 114 was creating SslSettings without the CustomValidator parameter) - Removed unused ValidateCertificateHostnameMatch method (489-542) (Hostname validation is already handled by TlsValidationCallbacks.Create) NEW TESTS - CustomValidator Functionality: - CustomValidator_that_accepts_should_allow_connection * Verifies CustomValidator callback is invoked during TLS handshake * Verifies acceptance allows successful connection - CustomValidator_that_rejects_should_prevent_connection * Verifies CustomValidator rejection prevents connection * Verifies callback was invoked even when rejecting - DotNettySslSetup_should_pass_CustomValidator_to_SslSettings * Unit test verifying CustomValidator is wired through to SslSettings Addresses PR review comments #7915: - CustomValidator now properly wired to SslSettings - Removed dead code (ValidateCertificateHostnameMatch) - Added real integration tests that validate CustomValidator actually works * Add warning when both DotNettySslSetup and HOCON SSL certificate config are present - Logs warning when DotNettySslSetup is used alongside explicit HOCON certificate configuration - Only warns when HOCON has actual certificate.path or certificate.thumbprint configured - Avoids false positives from default/empty config sections - Adds test verifying DotNettySslSetup precedence behavior - Addresses PR feedback: implement Option 1 from review comment * Remove unnecessary NET6_0_OR_GREATER conditional compilation directives - All types used (X509Certificate2, DotNettySslSetup, CertificateValidation) are available in .NET Standard 2.0 - Conditional directives were added during troubleshooting but are not needed - Verified compilation on both net8.0 and net48 targets * Consolidate TlsValidationCallbacks into public CertificateValidation API Removes internal TlsValidationCallbacks class and related enums (~145 lines) and refactors ComposeValidatorFromSettings() to use the public CertificateValidation helpers instead. Changes: - Remove ChainValidationMode and HostnameValidationMode enums - Remove TlsValidationCallbacks internal class - Refactor ComposeValidatorFromSettings() to handle all 4 combinations of SuppressValidation and ValidateCertificateHostname flags: * suppressChain=true, validateHostname=false → Accept all * suppressChain=true, validateHostname=true → Validate hostname only * suppressChain=false, validateHostname=true → Chain + hostname * suppressChain=false, validateHostname=false → Chain only (default) Benefits: - Eliminates code duplication between internal and public APIs - Simplifies maintenance by having a single validation implementation - Makes the public CertificateValidation API the canonical approach - All 43 DotNetty tests pass including edge case validations * cleaned up `CertificateValidation` composition code for default settings * Add comprehensive test coverage for CertificateValidation helpers Added 11 new tests to achieve 100% coverage of previously untested CertificateValidation helper methods: PinnedCertificate tests: - Accept connections with matching thumbprint - Reject connections with non-matching thumbprint ValidateSubject tests: - Accept certificates with matching subject - Reject certificates with non-matching subject - Support wildcard pattern matching (CN=Akka-Node-*) ValidateIssuer tests: - Accept certificates with matching issuer Combine/ChainPlusThen tests: - Verify composability of validators CustomValidator precedence tests: - Verify CustomValidator overrides validateCertificateHostname setting Also removed obsolete Mono checks from all new tests per maintainer guidance (Mono is no longer supported). Test results: 18/18 passing (7 existing + 11 new) * Remove unnecessary Combine() wrapper for single validators in tests Simplified two test cases that were unnecessarily wrapping single CertificateValidationCallback delegates in Combine(): - CustomValidator_that_accepts_should_allow_connection - CustomValidator_that_rejects_should_prevent_connection Changed from: var validator = CertificateValidation.Combine((cert, ...) => true); To cleaner direct delegate assignment: CertificateValidationCallback validator = (cert, ...) => true; Combine() is only needed when composing multiple validators. These tests verify single custom validators, so direct assignment is clearer. All tests still pass (4/4 CustomValidator tests verified). * added `nullability` annotations to DotNettyTransport * Remove obsolete Mono and NET471 workarounds from SSL tests - Removed #if !NET471 conditional compilation directives (10 instances) Project now targets net48, making NET471 conditionals meaningless - Removed if (IsMono) runtime checks (7 instances) Modern .NET uses CoreCLR cross-platform, not Mono - All SSL tests now run unconditionally on supported platforms - Tests verified passing: 27/27 on net8.0, 26/26 on net48 * Fix null certificate handling in SSL validation methods - Added explicit null checks to all CertificateValidation helper methods - PinnedCertificate: Check for null cert and filter empty thumbprints - ValidateSubject/ValidateIssuer: Check for null cert and empty values - ValidateHostname: Check for null cert before accessing properties - ValidateChain: Check for null cert before chain validation - Improved error messages to distinguish null cert from other failures - Added comprehensive unit test coverage for edge cases - Prevents potential NullReferenceException in TLS handshake scenarios * Add documentation explaining why case-insensitive thumbprint comparison is safe Added detailed comment explaining: - Thumbprints are hexadecimal SHA hash representations - Hex values are inherently case-insensitive (2A8B == 2a8b) - Different tools display differently (Windows vs OpenSSL) - Case-insensitive comparison improves usability without compromising security * Improve certificate validation tests with EventFilter - Replaced ExpectMsg with EventFilter for proper log assertion pattern - EventFilter is the idiomatic way to assert log messages in Akka.NET tests - Added test for rejecting non-matching thumbprint with EventFilter - Updated Combine test to clearly document short-circuit behavior - All tests now properly verify both result AND expected log messages * Use EventFilter to assert SSL validation errors in multi-actor system tests Updated SSL integration tests to use EventFilter for asserting specific validation errors instead of just checking connection failure. This provides better test precision by verifying the exact reason for connection failure. With mTLS enabled, validation errors occur on the server side (_sys2) when it validates the client certificate, since the client (Sys) has suppressValidation enabled. The EventFilter assertions are correctly targeted to the system where the validation errors occur. Changes: - Added EventFilter assertions to PinnedCertificate rejection test - Added EventFilter assertions to CustomValidator rejection test - Added EventFilter assertions to ValidateSubject rejection test - Modified custom validator to log error for EventFilter detection - Added comments explaining the mTLS validation flow * Revert "Use EventFilter to assert SSL validation errors in multi-actor system tests" This reverts commit 2022d63. * remove unnecessary project reference * added API approvals * Fix incorrect bitwise AND check with SslPolicyErrors.None The condition `(errors & SslPolicyErrors.None) != SslPolicyErrors.None` was always false because SslPolicyErrors.None equals 0, and any value bitwise AND with 0 always results in 0. Changed to simple equality check `errors != SslPolicyErrors.None` to correctly detect when SSL policy errors are present. This bug prevented the TlsErrorMessageBuilder from ever building detailed error messages when SSL validation failed, making debugging harder. --------- Co-authored-by: Gregorius Soedharmo <arkatufus@yahoo.com>
Summary
This PR implements a comprehensive programmatic certificate validation API for Akka.NET remote TLS transport, addressing the critical asymmetric hostname validation bug and providing high-level configuration flexibility.
Key Changes
6 commits total:
feat(remote):
CertificateValidationCallbackdelegate andCertificateValidationhelper factoryCertificateValidationCallbackdelegate for user-facing validation logicValidateChain,ValidateHostname,PinnedCertificate,ValidateSubject,ValidateIssuer,Combine,ChainPlusThenfix(remote): Single execution path for validation with hostname validation asymmetry fix
CustomValidator ?? ComposeValidatorFromSettings()pathfix(remote): Reject missing client certificates in server-side mutual TLS validation
Mutual_TLS_should_fail_when_client_has_no_certificatenow passesdocs: Programmatic certificate validation examples and consolidated security documentation
TlsConfigurationSample.cswith #region tagsfix: Corrected compilation errors in documentation examples
Akka.Remotereference toAkka.Docs.Testsproject#if NET6_0_OR_GREATERfor framework compatibilityfix: Wire CustomValidator through SslSettings and add comprehensive integration tests ⭐ CRITICAL
DotNettySslSetup.Settingsto passCustomValidatortoSslSettingsconstructorValidateCertificateHostnameMatchmethod (489-542 lines)Test Results
New Integration Tests (Commit 6)
CustomValidator_that_accepts_should_allow_connectionCustomValidator_that_rejects_should_prevent_connectionDotNettySslSetup_should_pass_CustomValidator_to_SslSettingsFiles Modified
DotNettyTransport.cs- Single execution path + null certificate rejection + removed unused methodDotNettyTransportSettings.cs- CertificateValidationCallback + CertificateValidation factoryDotNettySslSetup.cs- CustomValidator property + wiring to SslSettingsSslSettings.cs- CustomValidator propertyDotNettySslSetupSpec.cs- 3 new integration tests for CustomValidatorTlsConfigurationSample.cs- 5 programmatic examples (net6.0+)Akka.Docs.Tests.csproj- Akka.Remote referencesecurity.md- Consolidated validation strategies + decision matrixDocumentation
The new API provides:
CombineandChainPlusThenChainPlusThenfor complex scenariosBackwards Compatibility
✓ Fully backwards compatible - all changes are additive
✓ Existing HOCON-based configuration unaffected
✓ New API is optional; users can continue using HOCON
Test Plan