From cb96bf8d96d948e72f88b64a3183e2a80b8bb030 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Szczerbi=C5=84ski?= Date: Tue, 5 Nov 2024 14:45:05 +0100 Subject: [PATCH] Fix windows credential cache --- CMakeLists.txt | 2 +- cpp/lib/CredentialCache.cpp | 10 +++---- cpp/platform/SecureStorage.cpp | 18 +++++++----- cpp/platform/SecureStorageWin.cpp | 44 ++++++++++++++++------------ tests/CMakeLists.txt | 4 ++- tests/test_manual_connect.c | 7 ++++- tests/test_unit_credential_cache.cpp | 29 ++++++++++++++++++ 7 files changed, 80 insertions(+), 34 deletions(-) create mode 100644 tests/test_unit_credential_cache.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 01bf80da91..4065672c18 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -333,7 +333,7 @@ if (WIN32) set(VSDIR "vs15" CACHE STRING "Used to specify visual studio version of libsnowflakeclient dependecies") add_definitions(-D_CRT_SECURE_NO_DEPRECATE) find_library(OOB_LIB libtelemetry_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/oob/lib/ REQUIRED NO_DEFAULT_PATH) -# find_library(CURL_LIB libcurl_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/curl/lib/ REQUIRED NO_DEFAULT_PATH) + find_library(CURL_LIB libcurl_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/curl/lib/ REQUIRED NO_DEFAULT_PATH) find_library(SSL_LIB libssl_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/openssl/lib/ REQUIRED NO_DEFAULT_PATH) find_library(CRYPTO_LIB libcrypto_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/openssl/lib/ REQUIRED NO_DEFAULT_PATH) find_library(ZLIB_LIB zlib_a.lib PATHS deps-build/${PLATFORM}/${VSDIR}/${CMAKE_BUILD_TYPE}/zlib/lib/ REQUIRED NO_DEFAULT_PATH) diff --git a/cpp/lib/CredentialCache.cpp b/cpp/lib/CredentialCache.cpp index 4688ed95d4..74104b9dbd 100644 --- a/cpp/lib/CredentialCache.cpp +++ b/cpp/lib/CredentialCache.cpp @@ -125,7 +125,7 @@ namespace sf { }; CredentialCache *CredentialCache::make() { -#if defined(__WIN32) || defined(__APPLE__) +#if defined(_WIN32) || defined(__APPLE__) return new SecureStorageCredentialCache(); #else return new FileCredentialCache(std::string("file.txt")); @@ -144,14 +144,14 @@ cred_cache_ptr cred_cache_init() { char* cred_cache_get_credential(cred_cache_ptr tc, const char* account, const char* host, const char* user, CredentialType type) { - sf::CredentialKey key = { .account = account, .host = host, .user = user, .type = type }; + sf::CredentialKey key = { account, host, user, type }; auto tokenOpt = reinterpret_cast(tc)->get(key); if (!tokenOpt) { return nullptr; } size_t result_size = tokenOpt->size() + 1; char* result = new char[result_size]; - strncpy(result, tokenOpt->c_str(), result_size + 1); + strncpy(result, tokenOpt->c_str(), result_size); return result; } @@ -161,13 +161,13 @@ void cred_cache_free_credential(char* cred) { void cred_cache_save_credential(cred_cache_ptr tc, const char* account, const char* host, const char* user, CredentialType type, const char *cred) { - sf::CredentialKey key = { .account = account, .host = host, .user = user, .type = type }; + sf::CredentialKey key = { account, host, user, type }; reinterpret_cast(tc)->save(key, std::string(cred)); } void cred_cache_remove_credential(cred_cache_ptr tc, const char* account, const char* host, const char* user, CredentialType type) { - sf::CredentialKey key = { .account = account, .host = host, .user = user, .type = type }; + sf::CredentialKey key = { account, host, user, type }; reinterpret_cast(tc)->remove(key); } diff --git a/cpp/platform/SecureStorage.cpp b/cpp/platform/SecureStorage.cpp index d2f793ec35..97a2f18742 100644 --- a/cpp/platform/SecureStorage.cpp +++ b/cpp/platform/SecureStorage.cpp @@ -11,6 +11,8 @@ namespace sf { + using Snowflake::Client::SFLogger; + bool SecureStorage::storeToken(const std::string& host, const std::string& username, const std::string& credType, @@ -19,11 +21,11 @@ namespace sf SecureStorageImpl secStorage; if (secStorage.storeToken(host, username, credType, token) != SecureStorageStatus::Success) { - log_error("Failed to store secure token%s", ""); + CXX_LOG_ERROR("Failed to store secure token"); return false; } - log_debug("Successfully stored secure token%s", ""); + CXX_LOG_DEBUG("Successfully stored secure token"); return true; } @@ -35,11 +37,11 @@ namespace sf std::string result; if (secStorage.retrieveToken(host, username, credType, result) != SecureStorageStatus::Success) { - log_error("Failed to retrieve secure token%s", ""); + CXX_LOG_ERROR("Failed to retrieve secure token"); return {}; } - log_debug("Successfully retrieved secure tokeni%s", ""); + CXX_LOG_DEBUG("Successfully retrieved secure token"); return result; } @@ -51,11 +53,11 @@ namespace sf SecureStorageImpl secStorage; if ( secStorage.updateToken(host, username, credType, token) != SecureStorageStatus::Success) { - log_error("Failed to update secure token%s", ""); + CXX_LOG_ERROR("Failed to update secure token"); return false; } - log_debug("Successfully updated secure token%s", ""); + CXX_LOG_DEBUG("Successfully updated secure token"); return true; } @@ -66,10 +68,10 @@ namespace sf SecureStorageImpl secStorage; if ( secStorage.removeToken(host, username, credType) != SecureStorageStatus::Success) { - log_error("Failed to remove secure token%s", ""); + CXX_LOG_ERROR("Failed to remove secure token"); return false; } - log_debug("Successfully removed secure token%s", ""); + CXX_LOG_DEBUG("Successfully removed secure token"); return true; } diff --git a/cpp/platform/SecureStorageWin.cpp b/cpp/platform/SecureStorageWin.cpp index 06a262ec0e..3df8a1c0d9 100644 --- a/cpp/platform/SecureStorageWin.cpp +++ b/cpp/platform/SecureStorageWin.cpp @@ -37,11 +37,12 @@ namespace sf const std::string& token) { std::string target = convertTarget(host, username, credType); - CREDENTIALW creds = { 0 }; + std::wstring wide_target = std::wstring(target.begin(), target.end()); - creds.TargetName = target_wcs; - creds.CredentialBlobSize = strlen(token); - creds.CredentialBlob = (LPBYTE)token; + CREDENTIALW creds = { 0 }; + creds.TargetName = wide_target.data(); + creds.CredentialBlobSize = token.size(); + creds.CredentialBlob = (LPBYTE)token.c_str(); creds.Persist = CRED_PERSIST_LOCAL_MACHINE; creds.Type = CRED_TYPE_GENERIC; @@ -62,27 +63,33 @@ namespace sf const std::string& credType, std::string& token) { - std::string target = convertTarget(host, username, credType, target_wcs, max_len); + std::string target = convertTarget(host, username, credType); + std::wstring wide_target = std::wstring(target.begin(), target.end()); + PCREDENTIALW retcreds = nullptr; - if (!CredReadW(target_wcs, CRED_TYPE_GENERIC, 0, &retcreds)) + if (!CredReadW(wide_target.data(), CRED_TYPE_GENERIC, 0, &retcreds)) { CXX_LOG_ERROR("Failed to read target or could not find it"); return SecureStorageStatus::Error; } - else + + CXX_LOG_DEBUG("Read the token now copying it"); + + DWORD blobSize = retcreds->CredentialBlobSize; + if (!blobSize) { - CXX_LOG_DEBUG("Read the token now copying it"); - - blobSize = retcreds->CredentialBlobSize; - if (!blobSize) - { - return SecureStorageStatus::Error; - } - strncpy_s(token, MAX_TOKEN_LEN, (char *)retcreds->CredentialBlob, size_t(blobSize)+1); - CXX_LOG_DEBUG("Read the token, copied it will return now."); + return SecureStorageStatus::Error; } - *token_len = size_t(blobSize); + token = ""; + std::copy( + retcreds->CredentialBlob, + retcreds->CredentialBlob + blobSize, + std::back_inserter(token) + ); + + CXX_LOG_DEBUG("Copied token"); + CredFree(retcreds); return SecureStorageStatus::Success; } @@ -100,8 +107,9 @@ namespace sf const std::string& credType) { std::string target = convertTarget(host, username, credType); + std::wstring wide_target = std::wstring(target.begin(), target.end()); - if (!CredDeleteW(target_wcs, CRED_TYPE_GENERIC, 0)) + if (!CredDeleteW(wide_target.data(), CRED_TYPE_GENERIC, 0)) { return SecureStorageStatus::Error; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 67b0eecec1..5500c082c8 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -73,7 +73,9 @@ SET(TESTS_CXX test_unit_snowflake_types_to_string test_unit_azure_client test_unit_query_context_cache - test_unit_sfurl) + test_unit_sfurl + test_unit_credential_cache +) SET(TESTS_PUTGET test_include_aws diff --git a/tests/test_manual_connect.c b/tests/test_manual_connect.c index 6a2df3e920..030a0e5b03 100644 --- a/tests/test_manual_connect.c +++ b/tests/test_manual_connect.c @@ -162,6 +162,11 @@ void test_mfa_connect_with_duo_passcodeInPassword(void** unused) void test_mfa_connect_with_mfa_cache(void** unused) { + /* + * Should trigger mfa push notification at most once. + * Make sure ALLOW_CLIENT_MFA_CACHING is set to true + * For more details refer to: https://docs.snowflake.com/en/user-guide/security-mfa#using-mfa-token-caching-to-minimize-the-number-of-prompts-during-authentication-optional + */ for (int i = 0; i < 2; i++) { SF_CONNECT *sf = snowflake_init(); snowflake_set_attribute(sf, SF_CON_APPLICATION_NAME, "ODBC"); @@ -205,7 +210,7 @@ void test_none(void** unused) {} int main(void) { - initialize_test(SF_BOOLEAN_FALSE); + initialize_test(SF_BOOLEAN_TRUE); struct CMUnitTest tests[1] = { cmocka_unit_test(test_none) }; diff --git a/tests/test_unit_credential_cache.cpp b/tests/test_unit_credential_cache.cpp new file mode 100644 index 0000000000..01145713f8 --- /dev/null +++ b/tests/test_unit_credential_cache.cpp @@ -0,0 +1,29 @@ +// +// Created by Jakub Szczerbinski on 05.11.24. +// +#include "lib/CredentialCache.hpp" +#include "utils/test_setup.h" + +void test_credential_cache(void **unused) +{ + std::unique_ptr cache{sf::CredentialCache::make()}; + sf::CredentialKey key { "account", "host", "user", CredentialType::MFA_TOKEN }; + + std::string token = "example_token"; + assert_true(cache->save(key, token)); + assert_true(cache->get(key).value() == token); + + assert_true(cache->remove(key)); + assert_false(cache->get(key).has_value()); +} + +int main(void) { + /* Testing only file based credential cache */ +#ifndef __linux__ + return 0; +#endif + const struct CMUnitTest tests[] = { + cmocka_unit_test(test_credential_cache), + }; + return cmocka_run_group_tests(tests, NULL, NULL); +}