-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix DotNettySslSetup being ignored when HOCON has valid SSL config #7918
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 DotNettySslSetup being ignored when HOCON has valid SSL config #7918
Conversation
…7917) Added test case that demonstrates DotNettySslSetup settings being ignored when HOCON has valid certificate configuration. The test configures: - HOCON with valid certificate path and settings - DotNettySslSetup with different certificate and settings Expected: DotNettySslSetup should take precedence (programmatic over config) Actual: HOCON certificate is used, DotNettySslSetup is completely ignored The bug occurs because CreateOrDefault() tries HOCON first and only uses the programmatic setup as an exception fallback. This test fails and will pass once the fix is applied to make programmatic setup take precedence. Existing tests didn't catch this because they only test the exception-based fallback path (HOCON with enable-ssl=true but no certificate path).
…kkadotnet#7917) Changed SSL settings initialization to prioritize programmatic DotNettySslSetup over HOCON configuration, fixing the precedence order bug. Changes: - Modified DotNettyTransportSettings.Create() to check sslSettings (from DotNettySslSetup) first before parsing HOCON configuration - Changed SslSettings.Create() from private to internal to enable direct usage - Previous behavior: HOCON always tried first, programmatic setup only used as exception fallback - New behavior: Programmatic setup takes precedence, HOCON used if not provided This ensures programmatic configuration properly overrides HOCON defaults, which is the expected behavior for Setup-based configuration in Akka.NET. The bug existed since DotNettySslSetup was introduced in July 2023 (commit 588d5d6). Existing tests passed only because they triggered the exception-based fallback path (HOCON with enable-ssl=true but no certificate path).
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
| const string setupCertPath = "Resources/akka-client-cert.pfx"; | ||
| var setupCert = new X509Certificate2(setupCertPath, Password, X509KeyStorageFlags.DefaultKeySet); | ||
|
|
||
| var sslSetup = new DotNettySslSetup(setupCert, suppressValidation: true, requireMutualAuthentication: false, validateCertificateHostname: true); |
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 two different certs - validate that the setup overrides the HOCON
| Assert.True(settings.EnableSsl); | ||
|
|
||
| // BUG: DotNettySslSetup should take precedence over HOCON, but currently HOCON wins | ||
| // because CreateOrDefault tries HOCON first, and only uses the setup as an exception fallback |
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 assertion failed before our fix.
| ClientSocketWorkerPoolSize: ComputeWorkerPoolSize(config.GetConfig("client-socket-worker-pool")), | ||
| MaxFrameSize: ToNullableInt(config.GetByteSize("maximum-frame-size", null)) ?? 128000, | ||
| Ssl: enableSsl ? SslSettings.CreateOrDefault(config.GetConfig("ssl"), sslSettings) : SslSettings.Empty, | ||
| Ssl: enableSsl ? (sslSettings ?? SslSettings.Create(config.GetConfig("ssl"))) : SslSettings.Empty, |
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.
Deprioritizes the default HOCON, in favor of what' specified in the DotNettySslSetup
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
…kkadotnet#7918) * Add failing test to expose DotNettySslSetup override bug (akkadotnet#7917) Added test case that demonstrates DotNettySslSetup settings being ignored when HOCON has valid certificate configuration. The test configures: - HOCON with valid certificate path and settings - DotNettySslSetup with different certificate and settings Expected: DotNettySslSetup should take precedence (programmatic over config) Actual: HOCON certificate is used, DotNettySslSetup is completely ignored The bug occurs because CreateOrDefault() tries HOCON first and only uses the programmatic setup as an exception fallback. This test fails and will pass once the fix is applied to make programmatic setup take precedence. Existing tests didn't catch this because they only test the exception-based fallback path (HOCON with enable-ssl=true but no certificate path). * Fix DotNettySslSetup being ignored when HOCON has valid SSL config (akkadotnet#7917) Changed SSL settings initialization to prioritize programmatic DotNettySslSetup over HOCON configuration, fixing the precedence order bug. Changes: - Modified DotNettyTransportSettings.Create() to check sslSettings (from DotNettySslSetup) first before parsing HOCON configuration - Changed SslSettings.Create() from private to internal to enable direct usage - Previous behavior: HOCON always tried first, programmatic setup only used as exception fallback - New behavior: Programmatic setup takes precedence, HOCON used if not provided This ensures programmatic configuration properly overrides HOCON defaults, which is the expected behavior for Setup-based configuration in Akka.NET. The bug existed since DotNettySslSetup was introduced in July 2023 (commit 588d5d6). Existing tests passed only because they triggered the exception-based fallback path (HOCON with enable-ssl=true but no certificate path).
…7918) (#7919) * Add failing test to expose DotNettySslSetup override bug (#7917) Added test case that demonstrates DotNettySslSetup settings being ignored when HOCON has valid certificate configuration. The test configures: - HOCON with valid certificate path and settings - DotNettySslSetup with different certificate and settings Expected: DotNettySslSetup should take precedence (programmatic over config) Actual: HOCON certificate is used, DotNettySslSetup is completely ignored The bug occurs because CreateOrDefault() tries HOCON first and only uses the programmatic setup as an exception fallback. This test fails and will pass once the fix is applied to make programmatic setup take precedence. Existing tests didn't catch this because they only test the exception-based fallback path (HOCON with enable-ssl=true but no certificate path). * Fix DotNettySslSetup being ignored when HOCON has valid SSL config (#7917) Changed SSL settings initialization to prioritize programmatic DotNettySslSetup over HOCON configuration, fixing the precedence order bug. Changes: - Modified DotNettyTransportSettings.Create() to check sslSettings (from DotNettySslSetup) first before parsing HOCON configuration - Changed SslSettings.Create() from private to internal to enable direct usage - Previous behavior: HOCON always tried first, programmatic setup only used as exception fallback - New behavior: Programmatic setup takes precedence, HOCON used if not provided This ensures programmatic configuration properly overrides HOCON defaults, which is the expected behavior for Setup-based configuration in Akka.NET. The bug existed since DotNettySslSetup was introduced in July 2023 (commit 588d5d6). Existing tests passed only because they triggered the exception-based fallback path (HOCON with enable-ssl=true but no certificate path).
Summary
Fixes #7917 -
DotNettySslSetupsettings were being ignored when HOCON configuration had valid SSL certificate configuration.Root Cause
The bug occurred in
DotNettyTransportSettings.Create()whereCreateOrDefault()tried parsing HOCON first and only used the programmaticDotNettySslSetupas an exception fallback. This meant:enable-ssl = trueAND valid certificate config → HOCON won, programmatic setup ignored ❌enable-ssl = trueBUT no/invalid certificate → Exception thrown, programmatic setup used ✅This was backwards from the expected behavior where programmatic configuration should take precedence over HOCON defaults.
Changes
DotNettySslSetup_should_override_HOCON_certificatethat exposes the bug by configuring both HOCON and programmatic SSL with different certificatessslSettings(fromDotNettySslSetup) first before parsing HOCONCreate()internal - Changed visibility fromprivatetointernalto enable direct usageTest Results
DotNettySslSetupSpectests passBackground
The bug existed since
DotNettySslSetupwas introduced in July 2023 (commit 588d5d6, PR #6854). Existing tests passed only because they triggered the exception-based fallback path (HOCON withenable-ssl=truebut no certificate path).