Add PostgreSQL SSL configuration example and tests #1529
Conversation
Includes: - A new example demonstrating the use of the `WithSSLConfig` API for PostgreSQL with SSL/TLS. - SSL configuration options, server-side and mutual TLS examples. - Tests to validate SSL container setup and secure connection handling in PostgreSQL.
… and related tests for improved clarity.
✅ Deploy Preview for testcontainers-dotnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
…ty across examples, tests, and documentation.
…rt wait strategy - Use `TestContext.Current.CancellationToken` in async operations within SSL tests. - Replace `UntilPortIsAvailable` with `UntilInternalTcpPortIsAvailable` for more accurate wait logic.
HofmeisterAn
added
module
HofmeisterAn
force-pushed
the
develop
branch
3 times, most recently
from
4900ecd to
8fa5f1b
Compare
There was a problem hiding this comment.
Thanks for the PR and for adding SSL support to the PostgreSQL module!
I noticed there's quite a bit of code in the PR for setting up certificates. I've added test certificates to the common project, which all test projects reference (#1545). That should remove a lot of the code, and we'll be able to reuse the certificates for other tests too, something I've been wanting to do for a while.
Also, we usually don't add examples for module-specific features. Those are covered by tests and docs. It'd be great if we could just add a small section to the PostgreSQL docs explaining how to configure the API for SSL.
Could you update the PR accordingly? Then I'll be happy to continue reviewing and get it finalized 🙏.
- Enable conditional assembly signing based on CI or explicit request. - Separate `InternalsVisibleTo` configurations for signed and unsigned builds. - Refactor related MSBuild properties for clarity and maintainability.
Summary by CodeRabbitRelease Notes
WalkthroughThis PR introduces comprehensive SSL/TLS support for PostgreSQL containers in Testcontainers. Changes include conditional assembly signing in build configuration, new SSL configuration methods (WithSslMode, WithRootCertificate, WithClientCertificate, WithSSLSettings) to the PostgreSQL builder, an SslMode enum, extended configuration properties, and integration tests validating SSL setup and connectivity. Changes
Sequence DiagramssequenceDiagram
participant Test as Test Code
participant Builder as PostgreSqlBuilder
participant Config as PostgreSqlConfiguration
participant Container as PostgreSqlContainer
participant PgSQL as PostgreSQL Container
Test->>Builder: WithSSLSettings(caCert, serverCert, serverKey)
Builder->>Config: Create with SSL properties
Config->>Config: Store SSL file paths
Builder->>Builder: Register resource mappings
Builder->>Builder: Set custom entrypoint script
Builder->>Builder: Add startup commands for SSL
Builder->>Container: Build container
Container->>PgSQL: Start with SSL config
PgSQL->>PgSQL: Load CA, server cert, server key
PgSQL->>PgSQL: Enable SSL in postgres.conf
Container->>Container: Wait until ready (pg_isready)
Container-->>Test: Ready
Test->>Container: Connect with SslMode.Require
Container->>PgSQL: TLS handshake
PgSQL-->>Container: SSL connection established
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple interconnected components (builder, configuration, tests) with new SSL logic and comprehensive test coverage. Review effort is offset by consistent patterns across SSL methods, clear separation of concerns, and good test documentation. The conditional build changes and SSL setup logic require careful attention to ensure correct certificate handling and environment-variable semantics. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
Directory.Build.props (1)
29-31: Fail fast with a helpful message if the strong‑name key is missing (LFS).Add a compile‑time check so devs get a clear hint to run Git LFS.
Apply this diff:
<PropertyGroup Condition="'$(SignAssembly)' == 'true'"> <AssemblyOriginatorKeyFile>$(MSBuildThisFileDirectory)src/strongname.snk</AssemblyOriginatorKeyFile> </PropertyGroup> + <Target Name="EnsureStrongNameKey" + BeforeTargets="CoreCompile" + Condition="'$(SignAssembly)' == 'true' and !Exists('$(MSBuildThisFileDirectory)src/strongname.snk')"> + <Error Text="Strong name key 'src/strongname.snk' not found. Did you install Git LFS and run 'git lfs pull'?" /> + </Target>
🧹 Nitpick comments (5)
Directory.Build.props (1)
48-52: Trim public key when reading from file to avoid stray newline issues.Safer if the
.pubends with a newline.Apply this diff:
- <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo"> - <_Parameter1>Testcontainers.Tests, PublicKey=$([System.IO.File]::ReadAllText($(MSBuildThisFileDirectory)src/strongname.pub))</_Parameter1> - </AssemblyAttribute> + <AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo"> + <_Parameter1> + Testcontainers.Tests, PublicKey=$([System.String]::Copy($([System.IO.File]::ReadAllText('$(MSBuildThisFileDirectory)src/strongname.pub'))).Trim()) + </_Parameter1> + </AssemblyAttribute>src/Testcontainers.PostgreSql/SslMode.cs (1)
23-26: Nit: clarify VerifyFull docs.Say “matches the host name (Subject Alternative Name/subject)” instead of “common name.”
docs/modules/postgres.md (1)
48-60: Add a CA‑validated example (VerifyCA/VerifyFull) for production flows.Briefly show connecting with certificate validation instead of
TrustServerCertificate = true.src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs (2)
18-18: Consider consistency in certificate directory paths.The constant
DefaultCertificatesDirectoryis used byWithRootCertificateandWithClientCertificate, butWithSSLSettingsuses a hardcoded path/tmp/testcontainers-dotnet/postgres. While this might be intentional (client vs server certificate locations), consider whether these paths should be unified or if separate constants would improve clarity.
167-180: Consider consistency with client-side SSL methods.This method uses
WithResourceMapping(which reads file contents into memory), whileWithRootCertificateandWithClientCertificateuseWithBindMount. Both approaches work, but consistency would improve maintainability. TheWithResourceMappingapproach has the advantage of not requiring the host files to remain available after container creation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Directory.Build.props(2 hunks)docs/modules/postgres.md(1 hunks)src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs(5 hunks)src/Testcontainers.PostgreSql/PostgreSqlConfiguration.cs(3 hunks)src/Testcontainers.PostgreSql/PostgreSqlContainer.cs(1 hunks)src/Testcontainers.PostgreSql/SslMode.cs(1 hunks)tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs(1 hunks)tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs (3)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs (2)
Task(37-60)Task(187-268)src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs (15)
PostgreSqlContainer(183-192)PostgreSqlBuilder(23-27)PostgreSqlBuilder(33-37)PostgreSqlBuilder(47-51)PostgreSqlBuilder(58-62)PostgreSqlBuilder(69-73)PostgreSqlBuilder(80-84)PostgreSqlBuilder(91-96)PostgreSqlBuilder(104-114)PostgreSqlBuilder(130-180)PostgreSqlBuilder(195-207)PostgreSqlBuilder(220-223)PostgreSqlBuilder(226-229)PostgreSqlBuilder(232-235)Task(265-277)src/Testcontainers.PostgreSql/PostgreSqlContainer.cs (3)
PostgreSqlContainer(13-17)Task(40-49)GetConnectionString(23-32)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs (3)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs (4)
UsedImplicitly(12-141)Task(90-93)Task(95-140)Fact(46-64)src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs (15)
PostgreSqlContainer(183-192)Task(265-277)PostgreSqlBuilder(23-27)PostgreSqlBuilder(33-37)PostgreSqlBuilder(47-51)PostgreSqlBuilder(58-62)PostgreSqlBuilder(69-73)PostgreSqlBuilder(80-84)PostgreSqlBuilder(91-96)PostgreSqlBuilder(104-114)PostgreSqlBuilder(130-180)PostgreSqlBuilder(195-207)PostgreSqlBuilder(220-223)PostgreSqlBuilder(226-229)PostgreSqlBuilder(232-235)src/Testcontainers.PostgreSql/PostgreSqlContainer.cs (3)
PostgreSqlContainer(13-17)Task(40-49)GetConnectionString(23-32)
src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs (3)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlContainerTest.cs (1)
PostgreSqlBuilder(80-81)src/Testcontainers.PostgreSql/PostgreSqlContainer.cs (1)
PostgreSqlContainer(13-17)src/Testcontainers.PostgreSql/PostgreSqlConfiguration.cs (5)
PostgreSqlConfiguration(20-42)PostgreSqlConfiguration(48-52)PostgreSqlConfiguration(58-62)PostgreSqlConfiguration(68-72)PostgreSqlConfiguration(79-92)
src/Testcontainers.PostgreSql/PostgreSqlConfiguration.cs (1)
src/Testcontainers/Builders/BuildConfiguration.cs (1)
BuildConfiguration(13-150)
🪛 Gitleaks (8.28.0)
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs
[high] 133-135: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: ci (Testcontainers.Bigtable, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Cassandra, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.ActiveMq, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.ArangoDb, ubuntu-24.04)
- GitHub Check: ci (Testcontainers.Azurite, ubuntu-24.04)
- GitHub Check: analyze (csharp)
🔇 Additional comments (15)
Directory.Build.props (2)
25-26: Conditional signing logic looks good.Defaults to unsigned locally, signs in CI or when explicitly requested. No changes needed.
53-57: InternalsVisibleTo (unsigned) is correct.Matches the signed/unsigned split and keeps tests working.
docs/modules/postgres.md (1)
35-46: Server‑side SSL example reads well.Clear and minimal; the wrapper ensures permissions. LGTM.
src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs (3)
86-96: LGTM!The implementation correctly mounts the root certificate and sets the appropriate environment variable for client-side SSL verification.
98-114: LGTM!The client certificate configuration is correctly implemented with appropriate file mounts and environment variables.
249-253: LGTM!The reformatting improves readability without changing functionality.
src/Testcontainers.PostgreSql/PostgreSqlConfiguration.cs (4)
13-30: LGTM!The SSL-related constructor parameters are well-documented and follow the existing pattern. Making them optional ensures backward compatibility.
35-41: LGTM!SSL property initialization follows the established pattern in the constructor.
85-91: LGTM!The merge constructor correctly combines SSL properties using
BuildConfiguration.Combine, maintaining the immutable configuration pattern.
108-142: LGTM!The SSL property getters are well-documented and follow the immutable configuration pattern consistently.
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs (5)
79-93: LGTM!The test appropriately validates that the container starts with SSL settings and produces a valid connection string.
95-120: LGTM!Excellent test that not only connects but also verifies that SSL is actually being used through the
pg_stat_sslsystem view. The comment aboutTrustServerCertificatebeing for testing only is appropriate.
122-152: LGTM!The test validates that standard SQL operations work correctly over an SSL-enabled connection.
154-185: LGTM!The argument validation tests provide good coverage and appropriately verify both the exception type and parameter name for each required parameter.
187-268: LGTM!The certificate generation is properly implemented with appropriate X.509 extensions, Subject Alternative Names, and platform-specific permission handling for the private key. The PEM encoding and date calculations are correct.
| /// <summary> | ||
| /// Sets the PostgreSql SSL mode. | ||
| /// </summary> | ||
| /// <param name="sslMode">The PostgreSql SSL mode.</param> | ||
| /// <returns>A configured instance of <see cref="PostgreSqlBuilder" />.</returns> | ||
| public PostgreSqlBuilder WithSslMode(SslMode sslMode) | ||
| { | ||
| return Merge(DockerResourceConfiguration, new PostgreSqlConfiguration(sslMode: sslMode)) | ||
| .WithEnvironment("PGSSLMODE", sslMode.ToString().ToLowerInvariant()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify SslMode enum values match PostgreSQL expectations.
The implementation converts the SslMode enum to lowercase for the PGSSLMODE environment variable. Ensure that the enum values (e.g., Disable, Require, VerifyCa, VerifyFull) exactly match PostgreSQL's expected values when lowercased (e.g., disable, require, verify-ca, verify-full).
🏁 Script executed:
#!/bin/bash
# Verify SslMode enum definition matches PostgreSQL SSL mode values
ast-grep --pattern 'enum SslMode {
$$$
}'Length of output: 0
Let me search more broadly for the SslMode enum definition:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for SslMode enum definition using ripgrep
rg -n "enum SslMode" --type=cs
Length of output: 62
🏁 Script executed:
#!/bin/bash
# Also search for where SslMode is used/imported in the PostgreSqlBuilder file
rg -n "SslMode" src/Testcontainers.PostgreSql/PostgreSqlBuilder.csLength of output: 60
🏁 Script executed:
#!/bin/bash
# Find all C# files that might contain SslMode definition
fd -e cs --exec rg -l "SslMode" {} \;Length of output: 282
🏁 Script executed:
#!/bin/bash
# Read the SslMode enum definition
cat src/Testcontainers.PostgreSql/SslMode.csLength of output: 610
🌐 Web query:
PostgreSQL PGSSLMODE valid values allowed
💡 Result:
Valid PGSSLMODE (sslmode) values:
- disable, allow, prefer, require, verify-ca, verify-full. Default: prefer. Meanings: disable = no SSL; allow/prefer = fallbacks; require = encrypt but no cert validation; verify-ca = encrypt + CA validation; verify-full = encrypt + CA + hostname check. [1][2]
Fix enum value conversion for VerifyCa and VerifyFull SSL modes.
The current implementation has a critical issue: enum values VerifyCa and VerifyFull will convert to verifyca and verifyfull, but PostgreSQL expects verify-ca and verify-full with hyphens. The ToString().ToLowerInvariant() conversion removes the case-based word boundaries.
Solution: Either (1) rename the enum values to VerifyCaWithHyphens pattern, or (2) add an explicit mapping/switch statement to convert enum values to the correct PostgreSQL format with hyphens.
🤖 Prompt for AI Agents
In src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs around lines 75 to 84, the
current ToString().ToLowerInvariant() call produces "verifyca" and "verifyfull"
for SslMode.VerifyCa and SslMode.VerifyFull but PostgreSQL expects "verify-ca"
and "verify-full"; replace the direct ToString call with an explicit mapping
(e.g., switch or dictionary) that returns "verify-ca" for VerifyCa,
"verify-full" for VerifyFull, and falls back to ToString().ToLowerInvariant()
for other values, then pass that mapped string to WithEnvironment("PGSSLMODE",
mappedValue).
| var wrapperEntrypoint = @"#!/bin/sh | ||
| set -e | ||
| SSL_DIR=/tmp/testcontainers-dotnet/postgres | ||
| # Fix ownership and permissions for SSL key/cert before Postgres init runs | ||
| if [ -f ""$SSL_DIR/server.key"" ]; then | ||
| chown postgres:postgres ""$SSL_DIR/server.key"" || true | ||
| chmod 600 ""$SSL_DIR/server.key"" || true | ||
| fi | ||
| if [ -f ""$SSL_DIR/server.crt"" ]; then | ||
| chown postgres:postgres ""$SSL_DIR/server.crt"" || true | ||
| fi | ||
| if [ -f ""$SSL_DIR/ca_cert.pem"" ]; then | ||
| chown postgres:postgres ""$SSL_DIR/ca_cert.pem"" || true | ||
| fi | ||
| exec /usr/local/bin/docker-entrypoint.sh ""$@"" | ||
| "; |
There was a problem hiding this comment.
Consider the implications of suppressing permission errors.
The shell script uses || true to ignore failures from chown and chmod operations. While this may be necessary in some container environments where these operations fail, it could also hide legitimate permission problems that would cause PostgreSQL to fail startup with unclear errors.
Consider either:
- Logging when these operations fail (if the container environment supports it)
- Adding a comment explaining why failures are acceptable
- Testing that PostgreSQL startup will fail clearly if permissions are incorrect
| // Ensure PostgreSQL is actually ready to accept connections over TCP, not just that the container is running. | ||
| // Always append the pg_isready-based wait strategy by default so tests using the default fixture are stable. | ||
| var postgreSqlBuilder = | ||
| WithWaitStrategy(Wait.ForUnixContainer().AddCustomWaitStrategy(new WaitUntil(DockerResourceConfiguration))); | ||
| return new PostgreSqlContainer(postgreSqlBuilder.DockerResourceConfiguration); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify wait strategy behavior and verify non-breaking change.
The comment states "Always append the pg_isready-based wait strategy" but the code calls WithWaitStrategy(...), which may replace rather than append to existing wait strategies. Verify whether this is a breaking change for users who set custom wait strategies before calling Build().
🏁 Script executed:
#!/bin/bash
# Check how WithWaitStrategy works - does it append or replace?
ast-grep --pattern 'WithWaitStrategy($$$) {
$$$
}'Length of output: 0
I'll search for the WithWaitStrategy method implementation to verify its behavior:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for WithWaitStrategy method definition and implementation
rg -n "WithWaitStrategy" --type=cs -C5
Length of output: 115359
Add check to preserve user-provided wait strategies in PostgreSqlBuilder.Build().
The code unconditionally calls WithWaitStrategy(), which replaces any custom wait strategies users may have configured. Other database builders (MariaDb, MySql, MongoDb, etc.) check DockerResourceConfiguration.WaitStrategies.Count() > 1 to avoid overwriting user-provided strategies.
Additionally, the comment states "Always append" but the code replaces rather than appends. The PostgreSQL builder should:
- Only apply the pg_isready wait strategy if the user hasn't already configured a custom one
- Update the comment to accurately reflect the behavior
Apply the same pattern used in MariaDbBuilder.Build() at line 80:
var postgreSqlBuilder = DockerResourceConfiguration.WaitStrategies.Count() > 1
? this
: WithWaitStrategy(Wait.ForUnixContainer().AddCustomWaitStrategy(new WaitUntil(DockerResourceConfiguration)));🤖 Prompt for AI Agents
In src/Testcontainers.PostgreSql/PostgreSqlBuilder.cs around lines 187 to 192,
the Build() method unconditionally calls WithWaitStrategy which replaces any
user-provided wait strategies and contradicts the "Always append" comment;
change the logic to follow the MariaDbBuilder pattern by only applying the
pg_isready wait strategy when no custom wait strategies exist (i.e., when
DockerResourceConfiguration.WaitStrategies.Count() <= 1) so existing user
strategies are preserved, and update the preceding comment to accurately state
that the pg_isready wait strategy is appended only when the user hasn't
configured a custom wait strategy.
| return await ExecAsync(new[] { "psql", "--host", "localhost", "--username", _configuration.Username, "--dbname", _configuration.Database, "--file", scriptFilePath }, ct) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
Don’t force TCP localhost without providing credentials — risk of password prompt/failure.
With --host localhost, libpq won’t use the Unix socket/peer auth; if PGPASSWORD isn’t set in the container, psql may fail. Use a DSN embedding creds (escaped) or make TCP conditional on SSL mode.
Apply this diff:
- return await ExecAsync(new[] { "psql", "--host", "localhost", "--username", _configuration.Username, "--dbname", _configuration.Database, "--file", scriptFilePath }, ct)
+ var dsn = $"postgresql://{Uri.EscapeDataString(_configuration.Username)}:{Uri.EscapeDataString(_configuration.Password)}@localhost:{PostgreSqlBuilder.PostgreSqlPort}/{_configuration.Database}";
+ return await ExecAsync(new[] { "psql", dsn, "--file", scriptFilePath }, ct)
.ConfigureAwait(false);Alternative: only add --host localhost when SSL is enabled, otherwise keep the default socket path to preserve prior behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return await ExecAsync(new[] { "psql", "--host", "localhost", "--username", _configuration.Username, "--dbname", _configuration.Database, "--file", scriptFilePath }, ct) | |
| .ConfigureAwait(false); | |
| var dsn = $"postgresql://{Uri.EscapeDataString(_configuration.Username)}:{Uri.EscapeDataString(_configuration.Password)}@localhost:{PostgreSqlBuilder.PostgreSqlPort}/{_configuration.Database}"; | |
| return await ExecAsync(new[] { "psql", dsn, "--file", scriptFilePath }, ct) | |
| .ConfigureAwait(false); |
🤖 Prompt for AI Agents
In src/Testcontainers.PostgreSql/PostgreSqlContainer.cs around lines 47-48, the
call to psql currently forces TCP by passing "--host localhost", which prevents
using the Unix socket and can trigger a password prompt if PGPASSWORD is not
set; remove the hardcoded --host or make it conditional: either (A) omit "--host
localhost" to use the default socket when SSL is not required, or (B) when you
must force TCP (e.g. SSL enabled), build a proper connection string/DSN that
embeds and escapes username/password/host/dbname (or set PGPASSWORD in the
environment passed to ExecAsync) and pass that via "--dbname" (or a connection
URI), ensuring credentials are escaped to avoid shell injection. Ensure the code
branches so that --host is only added when TCP/SSL is required and credentials
are provided via a secure, escaped DSN or environment variable.
| public sealed class PostgreSqlSslConfigTest | ||
| { | ||
| private const string CaCertFileName = "ca_cert.pem"; | ||
| private const string ServerCertFileName = "server.crt"; | ||
| private const string ServerKeyFileName = "server.key"; | ||
| private const string ClientCertFileName = "client.crt"; | ||
| private const string ClientKeyFileName = "client.key"; | ||
|
|
||
| private readonly string _tempDir; | ||
| private readonly string _caCertPath; | ||
| private readonly string _serverCertPath; | ||
| private readonly string _serverKeyPath; | ||
|
|
||
| private PostgreSqlContainer? _postgreSqlContainer; | ||
|
|
||
| public PostgreSqlSslConfigTest() | ||
| { | ||
| _tempDir = Path.Combine(Path.GetTempPath(), "testcontainers-ssl-" + Guid.NewGuid().ToString("N")[..8]); | ||
| Directory.CreateDirectory(_tempDir); | ||
|
|
||
| _caCertPath = Path.Combine(_tempDir, CaCertFileName); | ||
| _serverCertPath = Path.Combine(_tempDir, ServerCertFileName); | ||
| _serverKeyPath = Path.Combine(_tempDir, ServerKeyFileName); | ||
| Path.Combine(_tempDir, ClientCertFileName); | ||
| Path.Combine(_tempDir, ClientKeyFileName); | ||
| } |
There was a problem hiding this comment.
Implement IAsyncLifetime to prevent resource leaks.
The test class creates containers and temporary directories but has no mechanism to clean them up automatically. The Cleanup() method exists but is never called, leading to resource leaks (running containers and temp directories).
Apply this diff to implement proper cleanup:
[UsedImplicitly]
-public sealed class PostgreSqlSslConfigTest
+public sealed class PostgreSqlSslConfigTest : IAsyncLifetime
{
// ... existing fields ...
+ public ValueTask InitializeAsync()
+ {
+ return ValueTask.CompletedTask;
+ }
+
+ public async ValueTask DisposeAsync()
+ {
+ if (_postgreSqlContainer != null)
+ {
+ await _postgreSqlContainer.DisposeAsync();
+ }
+
+ if (Directory.Exists(_tempDir))
+ {
+ Directory.Delete(_tempDir, true);
+ }
+ }Then remove the Cleanup() method as it's no longer needed.
🤖 Prompt for AI Agents
In tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs around lines
10 to 35, the test class lacks automated cleanup and should implement xUnit's
IAsyncLifetime to stop/dispose any created PostgreSqlContainer and remove the
temporary directory; add " : IAsyncLifetime" to the class declaration, implement
InitializeAsync() as a no-op (or move any setup there) and implement
DisposeAsync() to asynchronously stop and dispose _postgreSqlContainer if not
null and delete the _tempDir (handling exceptions), and then remove the existing
unused Cleanup() method.
| Path.Combine(_tempDir, ClientCertFileName); | ||
| Path.Combine(_tempDir, ClientKeyFileName); |
There was a problem hiding this comment.
Remove dead code.
Lines 33-34 compute paths for client certificate files but don't assign the results to any variables. These lines have no effect and should be removed.
Apply this diff:
_caCertPath = Path.Combine(_tempDir, CaCertFileName);
_serverCertPath = Path.Combine(_tempDir, ServerCertFileName);
_serverKeyPath = Path.Combine(_tempDir, ServerKeyFileName);
- Path.Combine(_tempDir, ClientCertFileName);
- Path.Combine(_tempDir, ClientKeyFileName);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Path.Combine(_tempDir, ClientCertFileName); | |
| Path.Combine(_tempDir, ClientKeyFileName); | |
| _caCertPath = Path.Combine(_tempDir, CaCertFileName); | |
| _serverCertPath = Path.Combine(_tempDir, ServerCertFileName); | |
| _serverKeyPath = Path.Combine(_tempDir, ServerKeyFileName); | |
| } |
🤖 Prompt for AI Agents
In tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs around lines
33 to 34, there are two Path.Combine calls whose return values are not assigned
and thus are dead code; remove these two lines so no unused expressions remain,
or if the intent was to set variables, assign the results to the appropriate
fields (e.g., client cert/key path variables) and use them; update any
references accordingly to eliminate the dead statements.
| _postgreSqlContainer?.DisposeAsync().AsTask().GetAwaiter().GetResult(); | ||
| } | ||
| catch | ||
| { | ||
| // ignore | ||
| } |
There was a problem hiding this comment.
Avoid silently suppressing all exceptions during cleanup.
Catching and ignoring all exceptions during container disposal could hide legitimate issues (e.g., network problems, Docker daemon issues). Consider either removing the try-catch or at least logging the exceptions.
🤖 Prompt for AI Agents
In tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs around lines
66 to 71, the code currently swallows all exceptions during _postgreSqlContainer
disposal; change the cleanup to avoid silently suppressing failures by at
minimum catching Exception ex and logging it (e.g., using the test
logger/TestOutputHelper or Console.WriteLine) before optionally swallowing or
rethrowing; alternatively remove the try-catch entirely so disposal exceptions
surface to the test runner.
| private static async Task WritePemAsync(string path, byte[] derBytes, string begin, string end) | ||
| { | ||
| await File.WriteAllTextAsync(path, $"{begin}\n{Convert.ToBase64String(derBytes, Base64FormattingOptions.InsertLineBreaks)}\n{end}\n"); | ||
| } |
There was a problem hiding this comment.
Use PemEncoding to generate PEM, avoid embedding PEM sentinels in source (gitleaks).
Refactor helper to write PEM without hardcoding “BEGIN …”/“END …” strings.
Apply this diff:
- private static async Task WritePemAsync(string path, byte[] derBytes, string begin, string end)
- {
- await File.WriteAllTextAsync(path, $"{begin}\n{Convert.ToBase64String(derBytes, Base64FormattingOptions.InsertLineBreaks)}\n{end}\n");
- }
+ private static async Task WritePemAsync(string path, ReadOnlySpan<char> label, ReadOnlySpan<byte> derBytes)
+ {
+ var pem = System.Security.Cryptography.PemEncoding.Write(label, derBytes);
+ await File.WriteAllTextAsync(path, pem);
+ }Based on static analysis hints.
🤖 Prompt for AI Agents
In tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs around lines 90 to
93, the helper writes PEM by embedding "BEGIN/END" sentinel strings which
triggers gitleaks; replace it to use System.Security.Cryptography.PemEncoding.
Change the method to accept a PEM label (e.g. "CERTIFICATE" or "PRIVATE KEY")
and call PemEncoding.Write(label, derBytes) then write the resulting bytes to
disk with File.WriteAllBytesAsync(path, pemBytes) (or await
File.WriteAllBytesAsync(path, PemEncoding.Write(label, derBytes))). This removes
hardcoded sentinel strings and uses the platform API to emit correct PEM
formatting.
| await WritePemAsync(_caCertPath, caCert.RawData, "-----BEGIN CERTIFICATE-----", "-----END CERTIFICATE-----"); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Apply new PEM helper to certs and key; removes “BEGIN PRIVATE KEY” literal from code.
Prevents false positives and reduces manual base64 handling.
Apply this diff:
- await WritePemAsync(_caCertPath, caCert.RawData, "-----BEGIN CERTIFICATE-----", "-----END CERTIFICATE-----");
+ await WritePemAsync(_caCertPath, "CERTIFICATE".AsSpan(), caCert.RawData);
...
- await WritePemAsync(_serverCertPath, serverCert.RawData, "-----BEGIN CERTIFICATE-----", "-----END CERTIFICATE-----");
- await File.WriteAllTextAsync(_serverKeyPath, "-----BEGIN PRIVATE KEY-----\n" +
- Convert.ToBase64String(serverRsa.ExportPkcs8PrivateKey(), Base64FormattingOptions.InsertLineBreaks) +
- "\n-----END PRIVATE KEY-----\n");
+ await WritePemAsync(_serverCertPath, "CERTIFICATE".AsSpan(), serverCert.RawData);
+ var keyBytes = serverRsa.ExportPkcs8PrivateKey();
+ await WritePemAsync(_serverKeyPath, "PRIVATE KEY".AsSpan(), keyBytes);Run to verify no PEM sentinels remain in source:
Also applies to: 132-136
🏁 Script executed:
#!/bin/bash
rg -n --hidden -C0 -g '!**/bin/**' -g '!**/obj/**' \
-e '-----BEGIN PRIVATE KEY-----' \
-e '-----BEGIN CERTIFICATE-----'Length of output: 1334
I need to search the codebase directly for the WritePemAsync method and examine the implementation.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find WritePemAsync method definition
rg -n "WritePemAsync" --type csharp -B2 -A5Length of output: 2900
Perfect! I found PemEncoding.WriteString(ReadOnlySpan label, ReadOnlySpan data) which is the new PEM helper the review comment refers to. Now let me check if the new WritePemAsync overload exists and verify the scope of changes needed.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for all WritePemAsync definitions and implementations
rg -n "WritePemAsync" --type csharp -A8 | head -100Length of output: 3350
Now let me check PostgreSqlSSLConfigTest.cs to see the context of those PEM sentinels and whether they should also be refactored:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check PostgreSqlSSLConfigTest.cs for PEM sentinels and WritePemAsync usage
rg -n "WritePemAsync\|-----BEGIN" tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs -B2 -A2Length of output: 0
Let me examine the actual lines where PEM sentinels appear in PostgreSqlSSLConfigTest.cs:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check lines around 213, 253, 259 in PostgreSqlSSLConfigTest.cs
sed -n '210,220p' tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs
echo "---"
sed -n '250,260p' tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs
echo "---"
sed -n '256,266p' tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.csLength of output: 1521
Create new WritePemAsync overload using PemEncoding.WriteString and apply it to eliminate PEM sentinels from test files.
The refactoring has not been applied yet. Two locations require updates:
-
PostgreSqlSslTest.cs (lines 110-111, 132-136): Create a new
WritePemAsyncoverload with signatureWritePemAsync(string path, ReadOnlySpan<char> label, byte[] derBytes)that usesPemEncoding.WriteString(label, derBytes). Replace the 4-parameter calls and manualFile.WriteAllTextAsyncwith this new method. -
PostgreSqlSSLConfigTest.cs (lines 213-215, 253-256, 259-261): Apply the same refactoring to replace the manual
File.WriteAllTextAsynccalls with the new helper. This requires either adding the helper method to this file or moving it to a shared location.
The current hardcoded PEM markers ("-----BEGIN CERTIFICATE-----", "-----BEGIN PRIVATE KEY-----") remain in both files. Using PemEncoding.WriteString centralizes PEM formatting logic and removes sentinel strings from source code.
🤖 Prompt for AI Agents
In tests/Testcontainers.PostgreSql.Tests/PostgreSqlSslTest.cs around lines
110-111 and 132-136 and in
tests/Testcontainers.PostgreSql.Tests/PostgreSqlSSLConfigTest.cs around lines
213-215, 253-256 and 259-261, the test files still write PEM files by composing
sentinel strings and calling File.WriteAllTextAsync; create a new overload
WritePemAsync(string path, ReadOnlySpan<char> label, byte[] derBytes) that calls
PemEncoding.WriteString(label, derBytes) and writes that result to disk
asynchronously, then replace the existing 4-parameter WritePemAsync calls and
any File.WriteAllTextAsync PEM writes in both files to call this new overload
(either add the helper to each file or move it to a shared test helper) so
sentinel strings are removed and PEM formatting is centralized.
2 participants
Includes:
WithSSLSettingsAPI for PostgreSQL with SSL/TLS.What does this PR do?
deliver extra ssl cinfig for dotnet API
Why is it important?
Go module had this , better to have it in dotnet too