-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix TLS hostname validation bug and add configurable validation #7897
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
Fix TLS hostname validation bug and add configurable validation #7897
Conversation
…dotnet#7893) ## Summary This commit addresses GitHub issue akkadotnet#7893 by fixing a critical TLS hostname validation bug and introducing a configurable, type-safe validation system. ## Changes ### Bug Fix - **Fixed DotNettyTransport.cs:355** - TLS client was incorrectly validating against the client's own certificate DNS name instead of the remote server address - Changed from `certificate.GetNameInfo(X509NameType.DnsName, false)` to `remoteAddress.Host` ### New Configuration - Added `validate-certificate-hostname` config option to Remote.conf - Default: `false` (disabled for backward compatibility and mutual TLS flexibility) - When enabled: Traditional TLS hostname validation (CN/SAN must match target hostname) - When disabled: Only validates certificate chain, ignores hostname mismatches - Useful for: Mutual TLS with per-node certificates, IP-based connections, dynamic service discovery ### Type-Safe Validation System - Introduced enums to prevent primitive confusion in security-critical code: - `ChainValidationMode` enum (ValidateChain, IgnoreChainErrors) - `HostnameValidationMode` enum (ValidateHostname, IgnoreHostnameMismatch) - Created `TlsValidationCallbacks` static factory class with: - Main `Create()` method accepting enum parameters and logging adapter - Convenience methods: `ValidateFull()`, `ValidateChainOnly()`, `ValidateHostnameOnly()`, `AcceptAll()` - Detailed error logging with filtered SslPolicyErrors - Makes validation flags independent and composable - Replaces ~35 lines of inline callback code with 3 lines of self-documenting factory calls ### Updated SslSettings - Added `ValidateCertificateHostname` property - Updated all constructors to accept the new property - Updated `Create()` method to read from HOCON config ### Test Coverage - Extended `DotNettyMutualTlsSpec` with 4 new test cases: - `Hostname_validation_disabled_should_allow_different_certificates()` - Different certs work with validation disabled - `Hostname_validation_enabled_should_reject_different_certificates()` - Different certs fail with validation enabled - `Same_certificate_should_connect_in_mutual_tls()` - Typical mutual TLS scenario - `Hostname_validation_default_should_be_disabled()` - Verifies backward compatibility ## Technical Details Chain validation and hostname validation are now fully independent: - `suppressValidation=true` disables chain/CA validation (for self-signed certs) - `validateCertificateHostname=true/false` controls hostname matching (for per-node certs, IPs) This allows testing hostname validation with self-signed certificates by using `suppressValidation=true, validateCertificateHostname=true`. Fixes akkadotnet#7893
## Summary Extended `DotNettySslSetup` programmatic API to expose the full SSL/TLS configuration, including the newly added hostname validation setting and the existing RequireMutualAuthentication setting that was previously only available via HOCON. ## Changes ### DotNettySslSetup API - Added `RequireMutualAuthentication` property (was missing from programmatic API) - Added `ValidateCertificateHostname` property (new setting from akkadotnet#7893) - Added comprehensive XML documentation for all properties and constructors - Added backward-compatible constructors: - 2-parameter: Defaults to RequireMutualAuthentication=true, ValidateCertificateHostname=false - 3-parameter: Defaults to ValidateCertificateHostname=false - 4-parameter: Full control over all settings - Updated `Settings` property to pass all 4 parameters to `SslSettings` constructor ### Integration Tests - Added 3 integration tests in `DotNettySslSetupSpec`: - Verify 2-parameter setup configures effective DotNettyTransportSettings with expected defaults - Verify 3-parameter setup configures effective settings with specified RequireMutualAuth - Verify 4-parameter setup configures effective settings with all specified values - Tests verify the actual consumption path: ActorSystem → DotNettyTransportSettings.Create() → setup.Value.Settings - Tests validate that setup values correctly override HOCON defaults ## Backward Compatibility All existing code using the 2-parameter constructor continues to work with the same defaults: - RequireMutualAuthentication: true (matches previous HOCON-only behavior) - ValidateCertificateHostname: false (matches new HOCON default) The setup is properly consumed in `DotNettyTransportSettings.Create(ActorSystem)` which retrieves the setup via `system.Settings.Setup.Get<DotNettySslSetup>()` and calls `setup.Value.Settings` to get the fully configured `SslSettings` object.
## Changes - Explained the new independent validation system (chain vs hostname) - Added details about default certificate stores used (Windows, Linux, macOS) - Documented the `validate-certificate-hostname` setting with use cases - Added validation mode combination table - Included configuration examples for both P2P and client-server scenarios - Added comprehensive troubleshooting for hostname validation errors - Documented enhanced TLS error messages from v1.5.52 - Reduced emoji usage for more professional tone - Added links to Microsoft documentation and RFC specifications ## Key Documentation Updates ### New Sections - Certificate Validation: Independent Control - Hostname Validation setting explanation - Validation Mode Combinations table - Configuration with Hostname Validation Enabled - Enhanced error message examples ### Troubleshooting Additions - RemoteCertificateNameMismatch errors (hostname validation failures) - UntrustedRoot errors (chain validation failures) - Understanding TLS Error Messages section with real examples - Multiple fix options for each error scenario ### Technical Details - Explained which OS certificate stores are used by default - Referenced RFC 5280 and RFC 6125 for validation standards - Clarified that suppress-validation only controls chain validation - Clarified that hostname validation is separate and optional
- Add blank lines around all fenced code blocks - Add language specifiers to code blocks (text, hocon, bash, powershell) - Change dash lists to asterisk lists for consistency - Add 'hostnames' to spellcheck dictionary - Emphasize that hostname validation defaults to false (disabled)
- Remove technical setting names from headings - Use descriptive section titles instead - Change subheadings to 'Enabled/Disabled' pattern - Move technical details into content body - Fix title case linting issues This makes the documentation more scannable and separates conceptual sections from implementation details.
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.
Detailed my changes
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.
Cleaned up the docs and mentioned the new settings introduced in this PR, along with where certs are sourced from in the store / how they're used.
| } | ||
|
|
||
| [Fact(DisplayName = "Different certificates with hostname validation disabled should connect successfully")] | ||
| public async Task Hostname_validation_disabled_should_allow_different_certificates() |
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.
It's a tad difficult for us to robustly test this functionality without doing things like installing custom CAs, so we're doing this all with self-signed certificates with hostname validation and CA validation both disabled.
| }); | ||
| } | ||
|
|
||
| [Fact(DisplayName = "DotNettySslSetup with 2 parameters should configure effective DotNettyTransportSettings with defaults (RequireMutualAuth=true, ValidateHostname=false)")] |
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.
Updated the DotNettySslSetup so this stuff can get incorporated into that configuration path as well
| # - Service discovery with dynamic addresses | ||
| # | ||
| # Default: false (disabled for backward compatibility and mutual TLS flexibility) | ||
| validate-certificate-hostname = false |
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 setting, for validating the hostnames on the provided certificates. This should default to false so IP-based connections can still function correctly, but it can be enabled for zero-trust environments.
| IChannelHandler tlsHandler; | ||
|
|
||
| if (Settings.Ssl.SuppressValidation) | ||
| // Build validation callback using type-safe factory methods |
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.
Simplified the SSL callback construction using some enums to signify which behaviors we want to use - this should help reduce branching in here and make it easier to debug the SSL configuration.
| /// Factory for creating TLS certificate validation callbacks with different security policies. | ||
| /// Provides type-safe, self-documenting methods for configuring certificate validation behavior. | ||
| /// </summary> | ||
| internal static class TlsValidationCallbacks |
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.
Builder for creating the cert validation callbacks - covered with our tests
| /// Creates validation callback that validates chain but ignores hostname mismatches. | ||
| /// Use for: Mutual TLS with per-node certificates, IP-based connections, dynamic service discovery. | ||
| /// </summary> | ||
| public static RemoteCertificateValidationCallback ValidateChainOnly(ILoggingAdapter 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 the default setting
| var certificate = Settings.Ssl.Certificate; | ||
| var host = certificate.GetNameInfo(X509NameType.DnsName, false); | ||
| // Use the remote address host for TLS validation, not the client's certificate name | ||
| var host = remoteAddress.Host; |
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.
Another important fix - the host is determined by the address of the remote host, not the client's certificate name (because client != server host names, due to how Akka.NET mesh networking works.)
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.
LGTM
…dotnet#7897) * Fix TLS hostname validation bug and add configurable validation (akkadotnet#7893) ## Summary This commit addresses GitHub issue akkadotnet#7893 by fixing a critical TLS hostname validation bug and introducing a configurable, type-safe validation system. ## Changes ### Bug Fix - **Fixed DotNettyTransport.cs:355** - TLS client was incorrectly validating against the client's own certificate DNS name instead of the remote server address - Changed from `certificate.GetNameInfo(X509NameType.DnsName, false)` to `remoteAddress.Host` ### New Configuration - Added `validate-certificate-hostname` config option to Remote.conf - Default: `false` (disabled for backward compatibility and mutual TLS flexibility) - When enabled: Traditional TLS hostname validation (CN/SAN must match target hostname) - When disabled: Only validates certificate chain, ignores hostname mismatches - Useful for: Mutual TLS with per-node certificates, IP-based connections, dynamic service discovery ### Type-Safe Validation System - Introduced enums to prevent primitive confusion in security-critical code: - `ChainValidationMode` enum (ValidateChain, IgnoreChainErrors) - `HostnameValidationMode` enum (ValidateHostname, IgnoreHostnameMismatch) - Created `TlsValidationCallbacks` static factory class with: - Main `Create()` method accepting enum parameters and logging adapter - Convenience methods: `ValidateFull()`, `ValidateChainOnly()`, `ValidateHostnameOnly()`, `AcceptAll()` - Detailed error logging with filtered SslPolicyErrors - Makes validation flags independent and composable - Replaces ~35 lines of inline callback code with 3 lines of self-documenting factory calls ### Updated SslSettings - Added `ValidateCertificateHostname` property - Updated all constructors to accept the new property - Updated `Create()` method to read from HOCON config ### Test Coverage - Extended `DotNettyMutualTlsSpec` with 4 new test cases: - `Hostname_validation_disabled_should_allow_different_certificates()` - Different certs work with validation disabled - `Hostname_validation_enabled_should_reject_different_certificates()` - Different certs fail with validation enabled - `Same_certificate_should_connect_in_mutual_tls()` - Typical mutual TLS scenario - `Hostname_validation_default_should_be_disabled()` - Verifies backward compatibility ## Technical Details Chain validation and hostname validation are now fully independent: - `suppressValidation=true` disables chain/CA validation (for self-signed certs) - `validateCertificateHostname=true/false` controls hostname matching (for per-node certs, IPs) This allows testing hostname validation with self-signed certificates by using `suppressValidation=true, validateCertificateHostname=true`. Fixes akkadotnet#7893 * Extend DotNettySslSetup to expose all SSL/TLS configuration options ## Summary Extended `DotNettySslSetup` programmatic API to expose the full SSL/TLS configuration, including the newly added hostname validation setting and the existing RequireMutualAuthentication setting that was previously only available via HOCON. ## Changes ### DotNettySslSetup API - Added `RequireMutualAuthentication` property (was missing from programmatic API) - Added `ValidateCertificateHostname` property (new setting from akkadotnet#7893) - Added comprehensive XML documentation for all properties and constructors - Added backward-compatible constructors: - 2-parameter: Defaults to RequireMutualAuthentication=true, ValidateCertificateHostname=false - 3-parameter: Defaults to ValidateCertificateHostname=false - 4-parameter: Full control over all settings - Updated `Settings` property to pass all 4 parameters to `SslSettings` constructor ### Integration Tests - Added 3 integration tests in `DotNettySslSetupSpec`: - Verify 2-parameter setup configures effective DotNettyTransportSettings with expected defaults - Verify 3-parameter setup configures effective settings with specified RequireMutualAuth - Verify 4-parameter setup configures effective settings with all specified values - Tests verify the actual consumption path: ActorSystem → DotNettyTransportSettings.Create() → setup.Value.Settings - Tests validate that setup values correctly override HOCON defaults ## Backward Compatibility All existing code using the 2-parameter constructor continues to work with the same defaults: - RequireMutualAuthentication: true (matches previous HOCON-only behavior) - ValidateCertificateHostname: false (matches new HOCON default) The setup is properly consumed in `DotNettyTransportSettings.Create(ActorSystem)` which retrieves the setup via `system.Settings.Setup.Get<DotNettySslSetup>()` and calls `setup.Value.Settings` to get the fully configured `SslSettings` object. * Update security documentation for hostname validation feature ## Changes - Explained the new independent validation system (chain vs hostname) - Added details about default certificate stores used (Windows, Linux, macOS) - Documented the `validate-certificate-hostname` setting with use cases - Added validation mode combination table - Included configuration examples for both P2P and client-server scenarios - Added comprehensive troubleshooting for hostname validation errors - Documented enhanced TLS error messages from v1.5.52 - Reduced emoji usage for more professional tone - Added links to Microsoft documentation and RFC specifications ## Key Documentation Updates ### New Sections - Certificate Validation: Independent Control - Hostname Validation setting explanation - Validation Mode Combinations table - Configuration with Hostname Validation Enabled - Enhanced error message examples ### Troubleshooting Additions - RemoteCertificateNameMismatch errors (hostname validation failures) - UntrustedRoot errors (chain validation failures) - Understanding TLS Error Messages section with real examples - Multiple fix options for each error scenario ### Technical Details - Explained which OS certificate stores are used by default - Referenced RFC 5280 and RFC 6125 for validation standards - Clarified that suppress-validation only controls chain validation - Clarified that hostname validation is separate and optional * Fix markdown linting and spellcheck issues in security docs - Add blank lines around all fenced code blocks - Add language specifiers to code blocks (text, hocon, bash, powershell) - Change dash lists to asterisk lists for consistency - Add 'hostnames' to spellcheck dictionary - Emphasize that hostname validation defaults to false (disabled) * Improve documentation heading structure and title case - Remove technical setting names from headings - Use descriptive section titles instead - Change subheadings to 'Enabled/Disabled' pattern - Move technical details into content body - Fix title case linting issues This makes the documentation more scannable and separates conceptual sections from implementation details. * added api approvals
* Fix TLS hostname validation bug and add configurable validation (#7893) ## Summary This commit addresses GitHub issue #7893 by fixing a critical TLS hostname validation bug and introducing a configurable, type-safe validation system. ## Changes ### Bug Fix - **Fixed DotNettyTransport.cs:355** - TLS client was incorrectly validating against the client's own certificate DNS name instead of the remote server address - Changed from `certificate.GetNameInfo(X509NameType.DnsName, false)` to `remoteAddress.Host` ### New Configuration - Added `validate-certificate-hostname` config option to Remote.conf - Default: `false` (disabled for backward compatibility and mutual TLS flexibility) - When enabled: Traditional TLS hostname validation (CN/SAN must match target hostname) - When disabled: Only validates certificate chain, ignores hostname mismatches - Useful for: Mutual TLS with per-node certificates, IP-based connections, dynamic service discovery ### Type-Safe Validation System - Introduced enums to prevent primitive confusion in security-critical code: - `ChainValidationMode` enum (ValidateChain, IgnoreChainErrors) - `HostnameValidationMode` enum (ValidateHostname, IgnoreHostnameMismatch) - Created `TlsValidationCallbacks` static factory class with: - Main `Create()` method accepting enum parameters and logging adapter - Convenience methods: `ValidateFull()`, `ValidateChainOnly()`, `ValidateHostnameOnly()`, `AcceptAll()` - Detailed error logging with filtered SslPolicyErrors - Makes validation flags independent and composable - Replaces ~35 lines of inline callback code with 3 lines of self-documenting factory calls ### Updated SslSettings - Added `ValidateCertificateHostname` property - Updated all constructors to accept the new property - Updated `Create()` method to read from HOCON config ### Test Coverage - Extended `DotNettyMutualTlsSpec` with 4 new test cases: - `Hostname_validation_disabled_should_allow_different_certificates()` - Different certs work with validation disabled - `Hostname_validation_enabled_should_reject_different_certificates()` - Different certs fail with validation enabled - `Same_certificate_should_connect_in_mutual_tls()` - Typical mutual TLS scenario - `Hostname_validation_default_should_be_disabled()` - Verifies backward compatibility ## Technical Details Chain validation and hostname validation are now fully independent: - `suppressValidation=true` disables chain/CA validation (for self-signed certs) - `validateCertificateHostname=true/false` controls hostname matching (for per-node certs, IPs) This allows testing hostname validation with self-signed certificates by using `suppressValidation=true, validateCertificateHostname=true`. Fixes #7893 * Extend DotNettySslSetup to expose all SSL/TLS configuration options ## Summary Extended `DotNettySslSetup` programmatic API to expose the full SSL/TLS configuration, including the newly added hostname validation setting and the existing RequireMutualAuthentication setting that was previously only available via HOCON. ## Changes ### DotNettySslSetup API - Added `RequireMutualAuthentication` property (was missing from programmatic API) - Added `ValidateCertificateHostname` property (new setting from #7893) - Added comprehensive XML documentation for all properties and constructors - Added backward-compatible constructors: - 2-parameter: Defaults to RequireMutualAuthentication=true, ValidateCertificateHostname=false - 3-parameter: Defaults to ValidateCertificateHostname=false - 4-parameter: Full control over all settings - Updated `Settings` property to pass all 4 parameters to `SslSettings` constructor ### Integration Tests - Added 3 integration tests in `DotNettySslSetupSpec`: - Verify 2-parameter setup configures effective DotNettyTransportSettings with expected defaults - Verify 3-parameter setup configures effective settings with specified RequireMutualAuth - Verify 4-parameter setup configures effective settings with all specified values - Tests verify the actual consumption path: ActorSystem → DotNettyTransportSettings.Create() → setup.Value.Settings - Tests validate that setup values correctly override HOCON defaults ## Backward Compatibility All existing code using the 2-parameter constructor continues to work with the same defaults: - RequireMutualAuthentication: true (matches previous HOCON-only behavior) - ValidateCertificateHostname: false (matches new HOCON default) The setup is properly consumed in `DotNettyTransportSettings.Create(ActorSystem)` which retrieves the setup via `system.Settings.Setup.Get<DotNettySslSetup>()` and calls `setup.Value.Settings` to get the fully configured `SslSettings` object. * Update security documentation for hostname validation feature ## Changes - Explained the new independent validation system (chain vs hostname) - Added details about default certificate stores used (Windows, Linux, macOS) - Documented the `validate-certificate-hostname` setting with use cases - Added validation mode combination table - Included configuration examples for both P2P and client-server scenarios - Added comprehensive troubleshooting for hostname validation errors - Documented enhanced TLS error messages from v1.5.52 - Reduced emoji usage for more professional tone - Added links to Microsoft documentation and RFC specifications ## Key Documentation Updates ### New Sections - Certificate Validation: Independent Control - Hostname Validation setting explanation - Validation Mode Combinations table - Configuration with Hostname Validation Enabled - Enhanced error message examples ### Troubleshooting Additions - RemoteCertificateNameMismatch errors (hostname validation failures) - UntrustedRoot errors (chain validation failures) - Understanding TLS Error Messages section with real examples - Multiple fix options for each error scenario ### Technical Details - Explained which OS certificate stores are used by default - Referenced RFC 5280 and RFC 6125 for validation standards - Clarified that suppress-validation only controls chain validation - Clarified that hostname validation is separate and optional * Fix markdown linting and spellcheck issues in security docs - Add blank lines around all fenced code blocks - Add language specifiers to code blocks (text, hocon, bash, powershell) - Change dash lists to asterisk lists for consistency - Add 'hostnames' to spellcheck dictionary - Emphasize that hostname validation defaults to false (disabled) * Improve documentation heading structure and title case - Remove technical setting names from headings - Use descriptive section titles instead - Change subheadings to 'Enabled/Disabled' pattern - Move technical details into content body - Fix title case linting issues This makes the documentation more scannable and separates conceptual sections from implementation details. * added api approvals
Summary
This PR fixes a critical TLS hostname validation bug (issue #7893) and introduces a configurable, type-safe validation system for DotNetty SSL/TLS connections.
Changes
🐛 Bug Fix (DotNettyTransport.cs:355-356)
Problem: TLS client was validating against the client's own certificate DNS name instead of the remote server address.
Fix: Changed from
certificate.GetNameInfo(X509NameType.DnsName, false)toremoteAddress.HostThis bug caused incorrect hostname validation in TLS connections, particularly affecting mutual TLS scenarios.
⚙️ New Configuration Option
Added
validate-certificate-hostnamesetting toRemote.conf:false(disabled for backward compatibility)🔒 Type-Safe Validation System
Introduced enums to prevent primitive confusion in security-critical code:
ChainValidationModeenum (ValidateChain,IgnoreChainErrors)HostnameValidationModeenum (ValidateHostname,IgnoreHostnameMismatch)TlsValidationCallbacksstatic factory class with convenience methods:Create(chainValidation, hostnameValidation, log)- Main factoryValidateFull()- Full validation (chain + hostname)ValidateChainOnly()- Chain validation onlyValidateHostnameOnly()- Hostname validation only (for testing)AcceptAll()- Accept all certificates (dangerous, test-only)Benefits:
🔌 Programmatic API Enhancement
Extended
DotNettySslSetupto expose all SSL/TLS settings:RequireMutualAuthenticationproperty (was missing from programmatic API)ValidateCertificateHostnameproperty (new setting)✅ Test Coverage
DotNettyMutualTlsSpec - 4 new end-to-end tests:
DotNettySslSetupSpec - 3 integration tests:
All tests verify the actual consumption path through
DotNettyTransportSettings.Create().Technical Details
Chain validation and hostname validation are now fully independent:
suppressValidation=truedisables chain/CA validation (for self-signed certs)validateCertificateHostname=true/falsecontrols hostname matchingThis allows testing hostname validation with self-signed certificates using:
Breaking Changes
None - all defaults preserve existing behavior.
Related Issues
Fixes #7893
Testing
All new tests pass: