test: Enable TLS for all Integration Tests and run PQC against generated Client - #1005
robertvoinescu-work wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request configures the integration tests to run against the GAPIC Showcase server with TLS enabled by default. It refactors the Post-Quantum Cryptography (PQC) tests to inherit from ShowcaseTestBase, configures the gRPC adapter to bypass certificate validation, and updates the test runner script to export the Showcase self-signed certificate via SSL_CERT_FILE. Additionally, several resumable upload tests are temporarily skipped. Feedback is provided regarding a potential issue on Windows environments, where the SSL_CERT_FILE environment variable is ignored by SChannel, which will cause REST integration tests to fail unless the certificate is manually trusted or the tests are programmatically skipped on Windows.
| // RestGrpcAdapter does not forward GrpcChannelOptions.HttpHandler to its internal HttpClient, | ||
| // so REST tests require the SSL_CERT_FILE environment variable (set in runintegrationtests.sh) | ||
| // to trust Showcase's self-signed certificate. | ||
| string name when name.EndsWith("RestTest") => RestGrpcAdapter.Default, |
There was a problem hiding this comment.
On Windows, .NET uses SChannel (CryptoAPI) rather than OpenSSL, which means the SSL_CERT_FILE environment variable is ignored. As a result, REST integration tests running on Windows will fail with SSL/TLS trust errors unless the self-signed certificate is manually installed in the Windows Trusted Root Certification Authorities store.
To improve the local developer experience and support running tests directly from IDEs on Linux/macOS without requiring manual environment setup, consider programmatically detecting and setting SSL_CERT_FILE in the constructor or static constructor if it is not already set:
if (Environment.GetEnvironmentVariable("SSL_CERT_FILE") == null)
{
string currentDir = AppDomain.CurrentDomain.BaseDirectory;
while (currentDir != null)
{
string pemPath = System.IO.Path.Combine(currentDir, "showcase.pem");
if (System.IO.File.Exists(pemPath))
{
Environment.SetEnvironmentVariable("SSL_CERT_FILE", pemPath);
break;
}
currentDir = System.IO.Path.GetDirectoryName(currentDir);
}
}Additionally, consider documenting this Windows limitation or programmatically skipping REST tests on Windows if the certificate is not trusted.
There was a problem hiding this comment.
Not applicable anymore. To get around the windows limitation I've uniformly modified both handlers to accept any certificate without checking it is trusted (SSL_CERT_FILE or within the cert store). This allows us to avoid installing the cert to the root certificate store on windows.
a3fd511 to
59dd8ce
Compare
|
/gemini review |
59dd8ce to
97335a7
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the integration tests to run entirely over TLS against the GAPIC Showcase server. It refactors the Post-Quantum Cryptography (PQC) tests to share a common base class, updates ShowcaseTestBase to use secure SSL credentials, and introduces reflection-based certificate bypass logic. Additionally, several resumable upload tests are temporarily skipped. Feedback on the changes highlights that the reflection logic in AcceptAnyCertificate is fragile and prone to runtime exceptions if channel or invoker is null, suggesting defensive null checks to ensure robust execution.
97335a7 to
4eddf52
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the integration tests to run entirely over TLS against the GAPIC Showcase server. It updates ShowcaseTestBase to use secure SSL credentials and bypasses certificate validation via reflection on the internal HTTP handler. Additionally, PqcIntegrationTest is refactored into a base class with separate REST and gRPC test classes, and several resumable upload tests are temporarily skipped. Feedback on these changes highlights a compilation issue on .NET Framework due to the use of an unsupported string.Contains overload, and suggests improving error handling in the reflection-based certificate bypass logic to handle null handlers gracefully.
4eddf52 to
0d0f3b4
Compare
b/484351289
Validation