Support hidden environment variables in tests

eng/tools/azure-sdk-tools/devtools_testutils/__init__.py

@@ -16,7 +16,7 @@
 )
 
 # cSpell:disable
-from .envvariable_loader import EnvironmentVariableLoader
+from .envvariable_loader import EnvironmentVariableLoader, EnvironmentVariableOptions
 from .exceptions import AzureTestError, ReservedResourceNameError
 from .proxy_fixtures import environment_variables, recorded_test, variable_recorder
 from .proxy_startup import start_test_proxy, stop_test_proxy, test_proxy
@@ -103,6 +103,7 @@
     "PemCertificate",
     "PowerShellPreparer",
     "EnvironmentVariableLoader",
+    "EnvironmentVariableOptions",
     "environment_variables",
     "recorded_by_proxy",
     "RecordedTransport",

eng/tools/azure-sdk-tools/devtools_testutils/envvariable_loader.py

@@ -5,6 +5,7 @@
 # --------------------------------------------------------------------------
 import logging
 import os
+from typing import Optional
 
 from dotenv import load_dotenv, find_dotenv
 
@@ -16,7 +17,41 @@
 _logger = logging.getLogger(__name__)
 
 
+class EnvironmentVariableOptions:
+    """
+    Options for the EnvironmentVariableLoader.
+
+    :param hide_secrets: Case insensitive list of environment variable names whose values should be hidden. Instead of
+        being passed to tests as plain strings, these values will be wrapped in an EnvironmentVariable object that hides
+        the value when printed. Use `.secret` to get the actual value (and don't store the value in a local variable).
+    """
+
+    def __init__(self, *, hide_secrets: Optional[list[str]] = None) -> None:
+        # Store all names as lowercase for easier case insensitive comparison in EnvironmentVariableLoader
+        self.hide_secrets: list[str] = [name.lower() for name in hide_secrets] if hide_secrets else []
+
+
 class EnvironmentVariableLoader(AzureMgmtPreparer):
+    """
+    Preparer to load environment variables during test setup.
+
+    Refer to
+    https://github.com/Azure/azure-sdk-for-python/tree/main/eng/tools/azure-sdk-tools/devtools_testutils#use-the-environmentvariableloader
+    for usage information.
+
+    :param str directory: The service directory prefix for the environment variables; e.g. "keyvault".
+    :param str name_prefix: Not used; present for compatibility with other preparers.
+    :param bool disable_recording: Not used; present for compatibility with other preparers.
+    :param dict client_kwargs: Not used; present for compatibility with other preparers.
+    :param bool random_name_enabled: Not used; present for compatibility with other preparers.
+    :param bool use_cache: Not used; present for compatibility with other preparers.
+    :param list preparers: Not used; present for compatibility with other preparers.
+
+    :param options: An EnvironementVariableOptions object containing additional options for the preparer.
+    :param kwargs: Keyword arguments representing environment variable names and their fake values for use in
+        recordings. For example, `client_id="fake_client_id"`.
+    """
+
     def __init__(
         self,
         directory,
@@ -26,6 +61,8 @@ def __init__(
         random_name_enabled=False,
         use_cache=True,
         preparers=None,
+        *,
+        options: Optional[EnvironmentVariableOptions] = None,
         **kwargs,
     ):
         super(EnvironmentVariableLoader, self).__init__(
@@ -37,18 +74,30 @@ def __init__(
         )
 
         self.directory = directory
+        self.hide_secrets = options.hide_secrets if options else []
         self.fake_values = {}
         self.real_values = {}
         self._set_secrets(**kwargs)
         self._backup_preparers = preparers
 
     def _set_secrets(self, **kwargs):
-        keys = kwargs.keys()
+        keys = {key.lower() for key in kwargs.keys()}
+        if self.hide_secrets and not all(name in keys for name in self.hide_secrets):
+            missing = [name for name in self.hide_secrets if name not in keys]
+            raise AzureTestError(
+                f"The following environment variables were specified to be hidden, but no fake values were "
+                f"provided for them: {', '.join(missing)}. Please provide fake values for these variables."
+            )
+
         needed_keys = []
         for key in keys:
             if self.directory in key:
                 needed_keys.append(key)
-                self.fake_values[key] = kwargs[key]
+                # Store the fake value, wrapping in EnvironmentVariable if it should be hidden
+                # Even fake values can cause security alerts if they're formatted like real secrets
+                self.fake_values[key] = (
+                    EnvironmentVariable(key.upper(), kwargs[key]) if key in self.hide_secrets else kwargs[key]
+                )
         for key in self.fake_values:
             kwargs.pop(key)
 
@@ -96,6 +145,12 @@ def _set_mgmt_settings_real_values(self):
                 os.environ.pop("AZURE_CLIENT_SECRET", None)
 
     def create_resource(self, name, **kwargs):
+        """
+        Fetches required environment variables if running live; otherwise returns fake values.
+
+        "create_resource" name is misleading, but is left over from when preparers were mostly used to create test
+        resources at runtime.
+        """
         load_dotenv(find_dotenv())
 
         if self.is_live:
@@ -105,26 +160,23 @@ def create_resource(self, name, **kwargs):
 
                     scrubbed_value = self.fake_values[key]
                     if scrubbed_value:
-                        self.real_values[key.lower()] = os.environ[key.upper()]
+                        # Store the real value, wrapping in EnvironmentVariable if it should be hidden
+                        self.real_values[key.lower()] = (
+                            EnvironmentVariable(key.upper(), os.environ[key.upper()])
+                            if key in self.hide_secrets
+                            else os.environ[key.upper()]
+                        )
 
-                        # vcrpy-based tests have a scrubber to register fake values
-                        if hasattr(self.test_class_instance, "scrubber"):
-                            self.test_class_instance.scrubber.register_name_pair(
-                                self.real_values[key.lower()], scrubbed_value
+                        try:
+                            add_general_string_sanitizer(
+                                value=scrubbed_value,
+                                target=self.real_values[key],
+                            )
+                        except:
+                            _logger.info(
+                                "A sanitizer could not be registered with the test proxy, so the "
+                                f"EnvironmentVariableLoader will not scrub the value of {key} in recordings."
                             )
-                        # test proxy tests have no scrubber, and instead register sanitizers using fake values
-                        else:
-                            try:
-                                add_general_string_sanitizer(
-                                    value=scrubbed_value,
-                                    target=self.real_values[key.lower()],
-                                )
-                            except:
-                                _logger.info(
-                                    "This test class instance has no scrubber and a sanitizer could not be registered "
-                                    "with the test proxy, so the EnvironmentVariableLoader will not scrub the value of "
-                                    f"{key} in recordings."
-                                )
                     else:
                         raise AzureTestError(
                             "To pass a live ID you must provide the scrubbed value for recordings to prevent secrets "
@@ -152,3 +204,12 @@ def create_resource(self, name, **kwargs):
 
     def remove_resource(self, name, **kwargs):
         pass
+
+
+class EnvironmentVariable:
+    def __init__(self, name: str, secret: str) -> None:
+        self.name = name
+        self.secret = secret
+
+    def __str__(self):
+        return f"Environment variable {self.name}'s value hidden for security."

sdk/storage/azure-storage-blob-changefeed/tests/settings/testcase.py

@@ -13,7 +13,7 @@
     import mock
 
 import logging
-from devtools_testutils import PowerShellPreparer
+from devtools_testutils import EnvironmentVariableLoader, EnvironmentVariableOptions
 from devtools_testutils.fake_credentials import STORAGE_ACCOUNT_FAKE_KEY
 
 try:
@@ -43,9 +43,10 @@
 os.environ['ACCOUNT_URL_SUFFIX'] = ACCOUNT_URL_SUFFIX
 
 ChangeFeedPreparer = functools.partial(
-    PowerShellPreparer, "storage",
+    EnvironmentVariableLoader, "storage",
     storage_account_name="storagename",
     storage_account_key=STORAGE_ACCOUNT_FAKE_KEY,
+    options=EnvironmentVariableOptions(hide_secrets=["storage_account_key"]),
 )
 
 def not_for_emulator(test):

sdk/storage/azure-storage-blob-changefeed/tests/test_change_feed.py

@@ -33,7 +33,7 @@ def test_get_change_feed_events_by_page(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         results_per_page = 10
         change_feed = cf_client.list_changes(results_per_page=results_per_page).by_page()
 
@@ -69,7 +69,7 @@ def test_get_all_change_feed_events(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         change_feed = cf_client.list_changes()
         all_events = list(change_feed)
         total_events = len(all_events)
@@ -93,7 +93,7 @@ def test_get_change_feed_events_with_continuation_token(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         # To get the total events number
         start_time = datetime(2022, 11, 15)
         end_time = datetime(2022, 11, 18)
@@ -122,7 +122,7 @@ def test_get_change_feed_events_in_a_time_range(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         start_time = datetime(2022, 11, 15)
         end_time = datetime(2022, 11, 18)
         change_feed = cf_client.list_changes(start_time=start_time, end_time=end_time, results_per_page=2).by_page()
@@ -138,7 +138,7 @@ def test_change_feed_does_not_fail_on_empty_event_stream(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         start_time = datetime(2300, 1, 1)
         change_feed = cf_client.list_changes(start_time=start_time)
 
@@ -151,7 +151,7 @@ def test_read_change_feed_tail_where_3_shards_have_data(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
 
         start_time = datetime(2022, 11, 27)
         end_time = datetime(2022, 11, 29)
@@ -210,7 +210,7 @@ def test_read_change_feed_tail_where_only_1_shard_has_data(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
 
         start_time = datetime(2022, 11, 27)
         end_time = datetime(2022, 11, 29)
@@ -248,7 +248,7 @@ def test_read_change_feed_with_3_shards_in_a_time_range(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
 
         # to get continuation token
         start_time = datetime(2022, 11, 27)
@@ -280,7 +280,7 @@ def test_read_change_feed_with_3_shards_in_a_time_range(self, **kwargs):
     def test_read_3_shards_change_feed_during_a_time_range_in_multiple_times_gives_same_result_as_reading_all(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
 
         # to read until the end
         start_time = datetime(2022, 11, 27)
@@ -344,7 +344,7 @@ def test_list_3_shards_events_works_with_1_shard_cursor(self, **kwargs):
         storage_account_name = kwargs.pop("storage_account_name")
         storage_account_key = kwargs.pop("storage_account_key")
 
-        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key)
+        cf_client = ChangeFeedClient(self.account_url(storage_account_name, "blob"), storage_account_key.secret)
         start_time = datetime(2022, 11, 27)
         end_time = datetime(2022, 11, 29)
         change_feed = cf_client.list_changes(results_per_page=1, start_time=start_time, end_time=end_time).by_page()

sdk/storage/azure-storage-blob/tests/settings/testcase.py

@@ -9,7 +9,7 @@
 import functools
 import os
 import logging
-from devtools_testutils import PowerShellPreparer
+from devtools_testutils import EnvironmentVariableLoader, EnvironmentVariableOptions
 from devtools_testutils.fake_credentials import STORAGE_ACCOUNT_FAKE_KEY
 
 try:
@@ -47,7 +47,7 @@
 os.environ['ACCOUNT_URL_SUFFIX'] = ACCOUNT_URL_SUFFIX
 
 BlobPreparer = functools.partial(
-    PowerShellPreparer, "storage",
+    EnvironmentVariableLoader, "storage",
     storage_account_name="storagename",
     storage_account_key=STORAGE_ACCOUNT_FAKE_KEY,
     secondary_storage_account_name="pyrmtstoragestorname",
@@ -60,7 +60,17 @@
     premium_storage_account_key=STORAGE_ACCOUNT_FAKE_KEY,
     soft_delete_storage_account_name="storagesoftdelname",
     soft_delete_storage_account_key=STORAGE_ACCOUNT_FAKE_KEY,
-    storage_resource_group_name="rgname"
+    storage_resource_group_name="rgname",
+    options=EnvironmentVariableOptions(
+        hide_secrets=[
+            "storage_account_key",
+            "secondary_storage_account_key",
+            "blob_storage_account_key",
+            "versioned_storage_account_key",
+            "premium_storage_account_key",
+            "soft_delete_storage_account_key"
+        ]
+    ),
 )