From e448282a320e9ed14e8f9e8c526f13fce393908b Mon Sep 17 00:00:00 2001 From: Will Tong Date: Mon, 1 Jun 2020 13:48:56 -0700 Subject: [PATCH 1/6] Add thread safe storage credential update --- .../includes/was/service_client.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/Microsoft.WindowsAzure.Storage/includes/was/service_client.h b/Microsoft.WindowsAzure.Storage/includes/was/service_client.h index 5202d49a..ff28c1af 100644 --- a/Microsoft.WindowsAzure.Storage/includes/was/service_client.h +++ b/Microsoft.WindowsAzure.Storage/includes/was/service_client.h @@ -74,11 +74,22 @@ namespace azure { namespace storage { /// Gets the storage account credentials for the service client. /// /// The storage account credentials for the service client. - const azure::storage::storage_credentials& credentials() const + const azure::storage::storage_credentials& credentials() { + pplx::extensibility::scoped_read_lock_t guard(m_mutex); return m_credentials; } + /// + /// Sets the storage credentials to use for the service client. + /// + /// The to use. + void set_storage_credentials(azure::storage::storage_credentials credentials) + { + pplx::extensibility::scoped_rw_lock_t guard(m_mutex); + m_credentials = std::move(credentials); + } + /// /// Gets the authentication scheme to use to sign HTTP requests for the service client. /// @@ -150,6 +161,7 @@ namespace azure { namespace storage { private: + pplx::extensibility::reader_writer_lock_t m_mutex; storage_uri m_base_uri; azure::storage::storage_credentials m_credentials; azure::storage::authentication_scheme m_authentication_scheme; From 4d2c7faf0371fc8a2c565a630b85a445cc4a3706 Mon Sep 17 00:00:00 2001 From: Will Tong Date: Mon, 8 Jun 2020 18:08:50 -0700 Subject: [PATCH 2/6] Revert "Add thread safe storage credential update" This reverts commit 4ce701a3832ccd0fde5f74d89b29e4a782004016. --- .../includes/was/service_client.h | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-) diff --git a/Microsoft.WindowsAzure.Storage/includes/was/service_client.h b/Microsoft.WindowsAzure.Storage/includes/was/service_client.h index ff28c1af..5202d49a 100644 --- a/Microsoft.WindowsAzure.Storage/includes/was/service_client.h +++ b/Microsoft.WindowsAzure.Storage/includes/was/service_client.h @@ -74,22 +74,11 @@ namespace azure { namespace storage { /// Gets the storage account credentials for the service client. /// /// The storage account credentials for the service client. - const azure::storage::storage_credentials& credentials() + const azure::storage::storage_credentials& credentials() const { - pplx::extensibility::scoped_read_lock_t guard(m_mutex); return m_credentials; } - /// - /// Sets the storage credentials to use for the service client. - /// - /// The to use. - void set_storage_credentials(azure::storage::storage_credentials credentials) - { - pplx::extensibility::scoped_rw_lock_t guard(m_mutex); - m_credentials = std::move(credentials); - } - /// /// Gets the authentication scheme to use to sign HTTP requests for the service client. /// @@ -161,7 +150,6 @@ namespace azure { namespace storage { private: - pplx::extensibility::reader_writer_lock_t m_mutex; storage_uri m_base_uri; azure::storage::storage_credentials m_credentials; azure::storage::authentication_scheme m_authentication_scheme; From d4c4495435ec46feeca5b6bd41b7a80f03169915 Mon Sep 17 00:00:00 2001 From: Will Tong Date: Thu, 11 Jun 2020 15:05:40 -0700 Subject: [PATCH 3/6] Allow setting of storage account key using shared pointer --- .../includes/was/core.h | 89 ++++++++++++++++--- 1 file changed, 78 insertions(+), 11 deletions(-) diff --git a/Microsoft.WindowsAzure.Storage/includes/was/core.h b/Microsoft.WindowsAzure.Storage/includes/was/core.h index 7e4d446a..76a849ae 100644 --- a/Microsoft.WindowsAzure.Storage/includes/was/core.h +++ b/Microsoft.WindowsAzure.Storage/includes/was/core.h @@ -212,14 +212,30 @@ namespace azure { namespace storage { { } + class account_key_credential + { + public: + account_key_credential(std::vector account_key = std::vector()) : m_account_key(std::move(account_key)) + { + } + + public: + std::vector m_account_key; + + private: + pplx::extensibility::reader_writer_lock_t m_mutex; + + friend class storage_credentials; + }; + /// /// Initializes a new instance of the class with the specified account name and key value. /// /// A string containing the name of the storage account. /// A string containing the Base64-encoded account access key. - storage_credentials(utility::string_t account_name, const utility::string_t& account_key) - : m_account_name(std::move(account_name)), m_account_key(utility::conversions::from_base64(account_key)) + storage_credentials(utility::string_t account_name, const utility::string_t& account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared()) { + m_account_key_credential->m_account_key = std::move(utility::conversions::from_base64(account_key)); } /// @@ -227,9 +243,9 @@ namespace azure { namespace storage { /// /// A string containing the name of the storage account. /// An array of bytes that represent the account access key. - storage_credentials(utility::string_t account_name, std::vector account_key) - : m_account_name(std::move(account_name)), m_account_key(std::move(account_key)) + storage_credentials(utility::string_t account_name, std::vector account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared()) { + m_account_key_credential->m_account_key = std::move(account_key); } class sas_credential @@ -339,9 +355,10 @@ namespace azure { namespace storage { m_sas_token = std::forward(other).m_sas_token; m_sas_token_with_api_version = std::forward(other).m_sas_token_with_api_version; m_account_name = std::forward(other).m_account_name; - m_account_key = std::forward(other).m_account_key; + std::atomic_store_explicit(&m_account_key_credential, std::atomic_load_explicit(&other.m_account_key_credential, std::memory_order_acquire), std::memory_order_release); + auto key_ptr = std::forward(other).m_account_key_credential; std::atomic_store_explicit(&m_bearer_token_credential, std::atomic_load_explicit(&other.m_bearer_token_credential, std::memory_order_acquire), std::memory_order_release); - auto ptr = std::forward(other).m_bearer_token_credential; + auto token_ptr = std::forward(other).m_bearer_token_credential; } return *this; } @@ -387,7 +404,43 @@ namespace azure { namespace storage { /// An array of bytes that contains the key. const std::vector& account_key() const { - return m_account_key; + auto account_key_ptr = std::atomic_load_explicit(&m_account_key_credential, std::memory_order_acquire); + pplx::extensibility::scoped_read_lock_t guard(account_key_ptr->m_mutex); + return account_key_ptr->m_account_key; + } + + /// + /// Sets the accounts for the credentials. + /// + /// A string containing the Base64-encoded account access key. + void set_account_key(const utility::string_t& account_key) + { + set_account_key(utility::conversions::from_base64(account_key)); + } + + /// + /// Sets the accounts for the credentials. + /// + /// An array of bytes that represent the account access key. + void set_account_key(std::vector account_key) + { + auto account_key_ptr = std::atomic_load_explicit(&m_account_key_credential, std::memory_order_acquire); + if (!account_key_ptr) + { + auto new_credential = std::make_shared(); + new_credential->m_account_key = std::move(account_key); + /* Compares m_account_key_credential and account_key_ptr(nullptr). + * If they are equivalent, assigns new_credential into m_account_key_credential and returns true. + * If they are not equivalent, assigns m_account_key_credential into m_account_key and returns false. + */ + bool set = std::atomic_compare_exchange_strong_explicit(&m_account_key_credential, &account_key_ptr, new_credential, std::memory_order_release, std::memory_order_acquire); + if (set) { + return; + } + account_key = std::move(new_credential->m_account_key); + } + pplx::extensibility::scoped_rw_lock_t guard(account_key_ptr->m_mutex); + account_key_ptr->m_account_key = std::move(account_key); } /// @@ -432,7 +485,7 @@ namespace azure { namespace storage { /// true if the credentials are for anonymous access; otherwise, false. bool is_anonymous() const { - return m_sas_token.empty() && m_account_key.empty() && !is_bearer_token(); + return m_sas_token.empty() && !is_account_key() && !is_bearer_token(); } /// @@ -441,7 +494,7 @@ namespace azure { namespace storage { /// true if the credentials are a shared access signature token; otherwise, false. bool is_sas() const { - return !m_sas_token.empty() && m_account_key.empty() && !is_bearer_token(); + return !m_sas_token.empty() && !is_account_key() && !is_bearer_token(); } /// @@ -450,7 +503,7 @@ namespace azure { namespace storage { /// true if the credentials are a shared key; otherwise, false. bool is_shared_key() const { - return m_sas_token.empty() && !m_account_key.empty() && !is_bearer_token(); + return m_sas_token.empty() && is_account_key() && !is_bearer_token(); } /// @@ -467,14 +520,28 @@ namespace azure { namespace storage { return !token_ptr->m_bearer_token.empty(); } + /// + /// Indicates whether the credentials are an account key. + /// + /// true if the credentials are an account key; otherwise false. + bool is_account_key() const { + auto account_key_ptr = std::atomic_load_explicit(&m_account_key_credential, std::memory_order_acquire); + if (!account_key_ptr) + { + return false; + } + pplx::extensibility::scoped_read_lock_t guard(account_key_ptr->m_mutex); + return !account_key_ptr->m_account_key.empty(); + } + private: utility::string_t m_sas_token; utility::string_t m_sas_token_with_api_version; utility::string_t m_account_name; - std::vector m_account_key; // We use std::atomic_{load/store/...} functions specialized for std::shared_ptr to access this member, since std::atomic> is not available until C++20. // These become deprecated since C++20, but still compile. + std::shared_ptr m_account_key_credential; std::shared_ptr m_bearer_token_credential; }; From 03a6d47c2f8c1dabd443883aad95f4f72fdce300 Mon Sep 17 00:00:00 2001 From: Will Tong Date: Mon, 15 Jun 2020 10:32:14 -0700 Subject: [PATCH 4/6] Resolve PR comments --- Microsoft.WindowsAzure.Storage/includes/was/core.h | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Microsoft.WindowsAzure.Storage/includes/was/core.h b/Microsoft.WindowsAzure.Storage/includes/was/core.h index 76a849ae..3c516a61 100644 --- a/Microsoft.WindowsAzure.Storage/includes/was/core.h +++ b/Microsoft.WindowsAzure.Storage/includes/was/core.h @@ -233,9 +233,8 @@ namespace azure { namespace storage { /// /// A string containing the name of the storage account. /// A string containing the Base64-encoded account access key. - storage_credentials(utility::string_t account_name, const utility::string_t& account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared()) + storage_credentials(utility::string_t account_name, const utility::string_t& account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared(utility::conversions::from_base64(account_key))) { - m_account_key_credential->m_account_key = std::move(utility::conversions::from_base64(account_key)); } /// @@ -243,9 +242,8 @@ namespace azure { namespace storage { /// /// A string containing the name of the storage account. /// An array of bytes that represent the account access key. - storage_credentials(utility::string_t account_name, std::vector account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared()) + storage_credentials(utility::string_t account_name, std::vector account_key) : m_account_name(std::move(account_name)), m_account_key_credential(std::make_shared(std::move(account_key))) { - m_account_key_credential->m_account_key = std::move(account_key); } class sas_credential @@ -410,7 +408,7 @@ namespace azure { namespace storage { } /// - /// Sets the accounts for the credentials. + /// Sets the account key for the credentials. /// /// A string containing the Base64-encoded account access key. void set_account_key(const utility::string_t& account_key) @@ -419,7 +417,7 @@ namespace azure { namespace storage { } /// - /// Sets the accounts for the credentials. + /// Sets the account key for the credentials. /// /// An array of bytes that represent the account access key. void set_account_key(std::vector account_key) From 30845b04eb0284fcb27780e88ece7b466e818d83 Mon Sep 17 00:00:00 2001 From: Will Tong Date: Mon, 15 Jun 2020 13:53:02 -0700 Subject: [PATCH 5/6] Fix failing storage account test --- .../tests/cloud_storage_account_test.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp b/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp index ce7b30af..a3625043 100644 --- a/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp +++ b/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp @@ -487,7 +487,7 @@ SUITE(Core) CHECK_UTF8_EQUAL(token, creds.sas_token()); CHECK(creds.account_name().empty()); - CHECK(creds.account_key().empty()); + CHECK(!creds.is_account_key()); CHECK(!creds.is_anonymous()); CHECK(creds.is_sas()); CHECK(!creds.is_shared_key()); @@ -502,7 +502,7 @@ SUITE(Core) CHECK_UTF8_EQUAL(token, creds.sas_token()); CHECK(creds.account_name().empty()); - CHECK(creds.account_key().empty()); + CHECK(!creds.is_account_key()); CHECK(!creds.is_anonymous()); CHECK(creds.is_sas()); CHECK(!creds.is_shared_key()); From e36de52f9d49d7cdc28357c7ffb29ff3b15b4be0 Mon Sep 17 00:00:00 2001 From: Will Tong Date: Tue, 23 Jun 2020 08:56:41 -0700 Subject: [PATCH 6/6] fix invalid account key access in test --- .../tests/cloud_storage_account_test.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp b/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp index a3625043..e5e73ab3 100644 --- a/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp +++ b/Microsoft.WindowsAzure.Storage/tests/cloud_storage_account_test.cpp @@ -42,7 +42,9 @@ void check_credentials_equal(const azure::storage::storage_credentials& a, const CHECK_EQUAL(a.is_sas(), b.is_sas()); CHECK_EQUAL(a.is_shared_key(), b.is_shared_key()); CHECK_UTF8_EQUAL(a.account_name(), b.account_name()); - CHECK_UTF8_EQUAL(utility::conversions::to_base64(a.account_key()), utility::conversions::to_base64(b.account_key())); + if (a.is_shared_key() && b.is_shared_key()) { + CHECK_UTF8_EQUAL(utility::conversions::to_base64(a.account_key()), utility::conversions::to_base64(b.account_key())); + } } void check_account_equal(azure::storage::cloud_storage_account& a, azure::storage::cloud_storage_account& b)