impl(bq_driver): Implementing Configuration Properties- UseSystemTrustStore by Khushikathuria008 · Pull Request #1337 · googleapis/cpp-bigquery-odbc

Khushikathuria008 · 2025-12-12T06:02:55Z

No description provided.

shivamd-gpartner · 2025-12-12T06:57:56Z

  auto conn = std::make_shared<ODBCHandles>();
-  EXPECT_EQ(Connect(kDefaultConnectionString, conn), SQL_SUCCESS);
+  std::string connection_string =
+      kDefaultConnectionString + ";UseSystemTrustStore=1;";


this is windows only feature , please add a flag

shivamd-gpartner · 2025-12-12T07:04:44Z


 struct SslCredentials {
  std::string pem_root_certs;
+  bool use_system_trust_store = false;


let's add this under windows only flag as well

shivamd-gpartner · 2025-12-15T06:48:24Z

 namespace {

+#ifdef _WIN32
+std::string ExportWindowsSystemCertsToPem() {


please write a unit test case for this

shivamd-gpartner · 2025-12-15T06:48:51Z

@Khushikathuria008 approved, please write unit test case for the function

shivamd-gpartner · 2025-12-15T08:18:14Z

 }
+
+#ifdef _WIN32
+TEST(ExportWindowsSystemCertsToPemTest, ReturnsValidPemFilePath) {


we can combine all these three test cases in one as they are all doing basically the same thing

sachinpro · 2025-12-17T09:15:21Z

+  // -----------------------------
+  // Create a REAL temp .pem file directly
+  // -----------------------------
+  char temp_path[MAX_PATH];


Don't spend much time on this but verify once if client libraries have a way of setting directly the file contents in binary.

sachinpro · 2025-12-17T09:23:32Z

+#ifdef _WIN32
+  // UseSystemTrustStore is only valid on Windows.
+  std::string connection_string =
+      kDefaultConnectionString + ";UseSystemTrustStore=1;";


This is not validating the behaviour since removing this line will still make the tast succeed.

Please modify an HTAPI test(SQLExecDirect_htapi_basictypes let's say) to disable using the packaged pem file, validate the error, set UseSystemTrustStore . There should be no errors finally.

Done
Made a separate testcase as this change is only valid for windows.
Existing driver gives error while we do a driver connect whereas our driver gives error when we run a query.

sachinpro · 2025-12-19T00:17:01Z


-namespace {
+#ifdef _WIN32
+std::string ExportWindowsSystemCertsToPem() {


Rather than returning empty strings, this function should return StatusRecordOr to handle failures.

sachinpro · 2025-12-19T00:22:22Z

+#endif
+
+namespace {
 google::cloud::ProxyConfig CreateProxyConfig(std::string hostname,


Although not related to this change, please remove this function. I cannot find it being used anywhere else . ref.

sachinpro · 2025-12-19T00:24:41Z

+           *(unsigned long long*)(guid.Data4 + 2));
+
+  // Build final *.pem path
+  std::string pem_file = std::string(temp_path) + "bqca_" + guid_str + ".pem";


We cannot use guid in the file name since it will create a new file every time ExportWindowsSystemCertsToPem is called with ODBCBQClient::CreateBQClient(.

sachinpro · 2025-12-19T02:48:25Z

+  std::string pem_path;
+  if (use_system_trust_store == true) {
+    pem_path = ExportWindowsSystemCertsToPem();
+    options.set<google::cloud::CARootsFilePathOption>(pem_path);


Why do we need to set it here when we are setting it with read_options also?

This is not needed for REST, right?

sachinpro

I think there is a way to directly use the certificates string. It would be a better implementation if it works.

Let me test it out. I can fix my comments myself alongwith those changes, so no need to update this PR right now.

…tStore

sachinpro

We need to find a way to have a test to verify this through integration tests.
Currently, UseSystemTrustStore is an optional property. No errors are thrown in either case.

Khushikathuria008 temporarily deployed to internal December 12, 2025 06:03 — with GitHub Actions Inactive

Khushikathuria008 temporarily deployed to internal December 12, 2025 06:17 — with GitHub Actions Inactive

Khushikathuria008 force-pushed the system_trust_store_impl branch from f78e8ca to 25bde8b Compare December 12, 2025 06:29

Khushikathuria008 temporarily deployed to internal December 12, 2025 06:30 — with GitHub Actions Inactive

Khushikathuria008 force-pushed the system_trust_store_impl branch from 25bde8b to e86eb3f Compare December 12, 2025 06:35

Khushikathuria008 temporarily deployed to internal December 12, 2025 06:36 — with GitHub Actions Inactive

shivamd-gpartner reviewed Dec 12, 2025

View reviewed changes

Khushikathuria008 temporarily deployed to internal December 15, 2025 05:27 — with GitHub Actions Inactive

Khushikathuria008 force-pushed the system_trust_store_impl branch from 82ac389 to c1da543 Compare December 15, 2025 05:31

Khushikathuria008 temporarily deployed to internal December 15, 2025 05:32 — with GitHub Actions Inactive

Khushikathuria008 force-pushed the system_trust_store_impl branch from c1da543 to ce0946c Compare December 15, 2025 06:13

Khushikathuria008 temporarily deployed to internal December 15, 2025 06:13 — with GitHub Actions Inactive

Khushikathuria008 temporarily deployed to internal December 15, 2025 06:18 — with GitHub Actions Inactive

shivamd-gpartner approved these changes Dec 15, 2025

View reviewed changes

shivamd-gpartner marked this pull request as ready for review December 15, 2025 06:47

shivamd-gpartner requested review from a team and sachinpro December 15, 2025 06:47

shivamd-gpartner reviewed Dec 15, 2025

View reviewed changes

Khushikathuria008 temporarily deployed to internal December 15, 2025 07:16 — with GitHub Actions Inactive

Khushikathuria008 removed the request for review from sachinpro December 15, 2025 07:47

Khushikathuria008 marked this pull request as draft December 15, 2025 07:47

Khushikathuria008 temporarily deployed to internal December 15, 2025 07:51 — with GitHub Actions Inactive

shivamd-gpartner reviewed Dec 15, 2025

View reviewed changes

Khushikathuria008 force-pushed the system_trust_store_impl branch from 436e046 to df80bf2 Compare December 15, 2025 08:20

Khushikathuria008 temporarily deployed to internal December 15, 2025 08:21 — with GitHub Actions Inactive

Khushikathuria008 marked this pull request as ready for review December 15, 2025 08:44

Khushikathuria008 requested a review from sachinpro December 15, 2025 08:44

Khushikathuria008 temporarily deployed to internal December 15, 2025 10:23 — with GitHub Actions Inactive

sachinpro changed the title ~~impl(bq_driver): Implementing Configuration Properties- UseSystemTrus…~~ impl(bq_driver): Implementing Configuration Properties- UseSystemTrustStore Dec 17, 2025

sachinpro reviewed Dec 17, 2025

View reviewed changes

Khushikathuria008 force-pushed the system_trust_store_impl branch from df80bf2 to a594b6d Compare December 18, 2025 10:11

sachinpro reviewed Dec 19, 2025

View reviewed changes

sachinpro requested changes Dec 19, 2025

View reviewed changes

sachinpro force-pushed the system_trust_store_impl branch from a594b6d to af739f1 Compare January 21, 2026 16:02

sachinpro temporarily deployed to internal January 21, 2026 16:02 — with GitHub Actions Inactive

sachinpro force-pushed the system_trust_store_impl branch from af739f1 to 2040107 Compare January 21, 2026 16:15

sachinpro temporarily deployed to internal January 21, 2026 16:16 — with GitHub Actions Inactive

sachinpro force-pushed the system_trust_store_impl branch from 2040107 to ef92796 Compare January 21, 2026 16:26

sachinpro temporarily deployed to internal January 21, 2026 16:26 — with GitHub Actions Inactive

sachinpro force-pushed the system_trust_store_impl branch from ef92796 to 38ac556 Compare January 21, 2026 16:41

sachinpro temporarily deployed to internal January 21, 2026 16:42 — with GitHub Actions Inactive

impl(bq_driver): Implementing Configuration Properties- UseSystemTrus…

be920af

…tStore

sachinpro force-pushed the system_trust_store_impl branch from 38ac556 to 9aa5b0a Compare May 12, 2026 13:30

sachinpro requested a review from a team as a code owner May 12, 2026 13:30

sachinpro temporarily deployed to internal May 12, 2026 13:31 — with GitHub Actions Inactive

sachinpro force-pushed the system_trust_store_impl branch from 9aa5b0a to 09a2b1a Compare May 12, 2026 14:03

sachinpro temporarily deployed to internal May 12, 2026 14:05 — with GitHub Actions Inactive

fix: modified the logic to create a single temp file for certs

25bb9d6

sachinpro force-pushed the system_trust_store_impl branch from 09a2b1a to 25bb9d6 Compare May 13, 2026 13:01

sachinpro temporarily deployed to internal May 13, 2026 13:02 — with GitHub Actions Inactive

sachinpro approved these changes May 13, 2026

View reviewed changes

sachinpro merged commit 063e2c1 into main May 13, 2026
21 of 23 checks passed

sachinpro deleted the system_trust_store_impl branch May 13, 2026 13:21

Conversation

Khushikathuria008 commented Dec 12, 2025 • edited by sachinpro Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shivamd-gpartner commented Dec 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinpro Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sachinpro left a comment

Choose a reason for hiding this comment

Uh oh!

sachinpro left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Khushikathuria008 commented Dec 12, 2025 •

edited by sachinpro

Loading

sachinpro Dec 19, 2025 •

edited

Loading