BUG: System DSNs decryptable by system users (#24)

One feature blocking the ODBC driver from working on the Power BI Gateway server was that System DSNs were usable by anyone on the system, but the client secret decryption was only possible by the user who created the DSN. This meant that when the Power BI Gateway Server tried to use the DSN via its service account, the client secret could not be decrypted.

With this commit in place, I have observed queries running via the Power BI Gateway server both for on-demand as well as scheduled data refreshes.

Author: rileymcdowell

src/driver/config/configDSN.cpp

@@ -12,15 +12,46 @@
 #include "dsnConfigForm.hpp"
 #include "profileReader.hpp"
 
+bool isSystemDSN() {
+  /*
+  System DSN secrets need to be decryptable for anyone on the system.
+  User DSN secrets only need to be decryptable for the current user.
+  To tell them apart, we need to use SQLGetConfigMode.
+
+  There exists a ODBC_BOTH_DSN option, which we will treat the same
+  as a system DSN in terms of encryption/decryption of secrets.
+  */
+  UWORD configMode;
+  SQLGetConfigMode(&configMode);
+
+  bool isSystemDSN = configMode != ODBC_USER_DSN;
+  WriteLog(LL_INFO, "System DSN: " + std::to_string(isSystemDSN));
+  return isSystemDSN;
+}
+
 void writeToProfile(DriverConfig result,
                     std::string section,
                     std::string value) {
   std::string dsn = result.getDSN();
   if (section == "clientSecret") {
+    std::function<std::string(const std::string&)> encryptionFunc = nullptr;
+    std::string encryptionLevel;
+    if (isSystemDSN()) {
+      encryptionFunc  = systemEncryptString;
+      encryptionLevel = "system";
+    } else {
+      encryptionFunc  = userEncryptString;
+      encryptionLevel = "user";
+    }
     SQLWritePrivateProfileString(dsn.c_str(),
                                  "encryptedClientSecret",
-                                 encryptString(value).c_str(),
+                                 encryptionFunc(value).c_str(),
+                                 "ODBC.INI");
+    SQLWritePrivateProfileString(dsn.c_str(),
+                                 "secretEncryptionLevel",
+                                 encryptionLevel.c_str(),
                                  "ODBC.INI");
+
   } else {
     SQLWritePrivateProfileString(
         dsn.c_str(), section.c_str(), value.c_str(), "ODBC.INI");

src/driver/config/driverConfig.cpp

@@ -56,6 +56,7 @@ std::map<std::string, std::string> DRIVER_CONFIG_DEFAULT_VALUES = {
     std::make_pair("clientId", ""),
     std::make_pair("clientSecret", ""),
     std::make_pair("oidcScope", ""),
+    std::make_pair("secretEncryptionLevel", "user"),
 };
 
 // DSN

src/driver/config/profileReader.cpp

@@ -43,9 +43,20 @@ DriverConfig readDriverConfigFromProfile(std::string dsn) {
   config.setClientId(readFromPrivateProfile(dsn, "clientId"));
   config.setOidcScope(readFromPrivateProfile(dsn, "oidcScope"));
 
-  // Handle the encrypted data
+  std::string secretEncryptionLevel =
+      readFromPrivateProfile(dsn, "secretEncryptionLevel");
   std::string encryptedClientSecret =
       readFromPrivateProfile(dsn, "encryptedClientSecret");
-  config.setClientSecret(decryptString(encryptedClientSecret));
+
+  // Handle the encrypted data
+  if (secretEncryptionLevel == "user") {
+    config.setClientSecret(userDecryptString(encryptedClientSecret));
+  } else if (secretEncryptionLevel == "system") {
+    config.setClientSecret(systemDecryptString(encryptedClientSecret));
+  } else {
+    throw std::runtime_error("Unknown secret encryption level: " +
+                             secretEncryptionLevel);
+  }
+
   return config;
 }

src/trinoAPIWrapper/authProvider/tokens/tokenCache.cpp

@@ -137,8 +137,8 @@ TokenCacheEntry readTokenCache(const std::string& tokenId) {
       tokenData.value("encryptedAccessToken", "");
   std::string encryptedRefreshToken =
       tokenData.value("encryptedRefreshToken", "");
-  std::string accessToken  = decryptString(encryptedAccessToken);
-  std::string refreshToken = decryptString(encryptedRefreshToken);
+  std::string accessToken  = userDecryptString(encryptedAccessToken);
+  std::string refreshToken = userDecryptString(encryptedRefreshToken);
 
   // Return a cache entry for these.
   WriteLog(LL_TRACE, "  Token cache read successfully");
@@ -167,9 +167,9 @@ void writeTokenCache(TokenCacheEntry cacheEntry) {
   std::string tokenId    = cacheEntry.getTokenId();
   inputJsonData[tokenId] = json::object();
   inputJsonData[tokenId]["encryptedAccessToken"] =
-      encryptString(cacheEntry.getAccessToken());
+      userEncryptString(cacheEntry.getAccessToken());
   inputJsonData[tokenId]["encryptedRefreshToken"] =
-      encryptString(cacheEntry.getRefreshToken());
+      userEncryptString(cacheEntry.getRefreshToken());
 
   // Last, modify it and write it back with an update token cache.
   std::ofstream outputFile(filePath);

src/util/cryptUtils.cpp

@@ -65,14 +65,13 @@ std::vector<BYTE> fromBase64(const std::string& data) {
   return decodedData;
 }
 
-std::string encryptString(const std::string& text) {
-  /* Encrypt a string using the user's login data.
+std::string encryptString(const std::string& text, DWORD flags) {
+  /*
    * Based on the API documentation for DPAPI CryptProtectData function
    * https://learn.microsoft.com/en-us/windows/win32/api/dpapi/nf-dpapi-cryptprotectdata
    * Adds base64 encoding to the output to avoid returning non printing
    * characters or other tricky stuff in the response.
    */
-
   if (text.empty()) {
     return "";
   }
@@ -85,7 +84,7 @@ std::string encryptString(const std::string& text) {
   dataIn.pbData = pbDataInput;
   dataIn.cbData = cbDataInput;
 
-  if (!CryptProtectData(&dataIn, NULL, NULL, NULL, NULL, 0, &dataOut)) {
+  if (!CryptProtectData(&dataIn, NULL, NULL, NULL, NULL, flags, &dataOut)) {
     throw std::runtime_error("Encryption Failed");
   }
   std::vector<BYTE> encryptedData(dataOut.pbData,
@@ -95,8 +94,8 @@ std::string encryptString(const std::string& text) {
   return toBase64(encryptedData);
 }
 
-std::string decryptString(const std::string& text) {
-  /* Decrypt a string using the user's login data.
+std::string decryptString(const std::string& text, DWORD flags) {
+  /*
    * Based on the API documentation for DPAPI CryptUnprotectData function
    * https://learn.microsoft.com/en-us/windows/win32/api/dpapi/nf-dpapi-cryptunprotectdata
    * Adds base64 decoding to the input to match the encryptString
@@ -124,3 +123,23 @@ std::string decryptString(const std::string& text) {
 
   return decryptedString;
 }
+
+std::string userEncryptString(const std::string& text) {
+  // Encrypt a string using the user's login data.
+  return encryptString(text, 0);
+}
+
+std::string systemEncryptString(const std::string& text) {
+  // Encrypt a string using the current computer as context.
+  return encryptString(text, CRYPTPROTECT_LOCAL_MACHINE);
+}
+
+std::string userDecryptString(const std::string& text) {
+  // Decrypt a string using the user's login data.
+  return decryptString(text, 0);
+}
+
+std::string systemDecryptString(const std::string& text) {
+  // Decrypt a string using the user's login data.
+  return decryptString(text, 0);
+}

src/util/cryptUtils.hpp

@@ -2,5 +2,7 @@
 
 #include <string>
 
-std::string encryptString(const std::string& text);
-std::string decryptString(const std::string& text);
+std::string userEncryptString(const std::string& text);
+std::string userDecryptString(const std::string& text);
+std::string systemEncryptString(const std::string& text);
+std::string systemDecryptString(const std::string& text);

test/unit/util/cryptUtilsTest.cpp

@@ -3,18 +3,53 @@
 
 #include "../../../src/util/cryptUtils.hpp"
 
-TEST(CryptUtilsTest, RoundTripText) {
+TEST(CryptUtilsTest, RoundTripUserText) {
   std::string secret    = "thisIsASecret";
-  std::string encrypted = encryptString(secret);
-  std::string decrypted = decryptString(encrypted);
+  std::string encrypted = userEncryptString(secret);
+  std::string decrypted = userDecryptString(encrypted);
 
   EXPECT_EQ(secret, decrypted);
 }
 
-TEST(CryptUtilsTest, RoundTripEmptyString) {
+TEST(CryptUtilsTest, RoundTripUsesrEmptyString) {
   std::string secret    = "";
-  std::string encrypted = encryptString(secret);
-  std::string decrypted = decryptString(encrypted);
+  std::string encrypted = userEncryptString(secret);
+  std::string decrypted = userDecryptString(encrypted);
 
   EXPECT_EQ(secret, decrypted);
 }
+
+TEST(CryptUtilsTest, RoundTripSystemText) {
+  std::string secret    = "thisIsASecret";
+  std::string encrypted = systemEncryptString(secret);
+  std::string decrypted = systemDecryptString(encrypted);
+
+  EXPECT_EQ(secret, decrypted);
+}
+
+TEST(CryptUtilsTest, RoundTripSystemEmptyString) {
+  std::string secret    = "";
+  std::string encrypted = systemEncryptString(secret);
+  std::string decrypted = systemDecryptString(encrypted);
+
+  EXPECT_EQ(secret, decrypted);
+}
+
+TEST(CryptUtilsTest, UserAndSystemSecretsAreDifferent) {
+  std::string secret          = "hello";
+  std::string userEncrypted   = userEncryptString(secret);
+  std::string systemEncrypted = systemEncryptString(secret);
+
+  // The secrets should be encrypted differently depending on
+  // whether they are user secrets or system secrets.
+  EXPECT_NE(userEncrypted, systemEncrypted);
+
+  std::string userDecrypted   = userDecryptString(userEncrypted);
+  std::string systemDecrypted = systemDecryptString(systemEncrypted);
+
+  // After decryption, the values should all be the same again
+  // since we started from the same secret string.
+  EXPECT_EQ(userDecrypted, systemDecrypted);
+  EXPECT_EQ(userDecrypted, secret);
+  EXPECT_EQ(systemDecrypted, secret);
+}