User config split into private and public

Fixes: #2809

uaclient/cli/tests/test_cli_config_set.py

@@ -83,12 +83,18 @@ def test_set_error_with_help_on_invalid_key_value_pair(
         assert err_msg in err
 
 
-@mock.patch("uaclient.config.state_files.user_config_file.write")
+@mock.patch("uaclient.config.state_files.user_config_file_private.write")
+@mock.patch("uaclient.config.state_files.user_config_file_public.write")
 @mock.patch("uaclient.cli.contract.get_available_resources")
 class TestActionConfigSet:
     @mock.patch("uaclient.util.we_are_currently_root", return_value=False)
     def test_set_error_on_non_root_user(
-        self, _m_resources, _we_are_currently_root, _write, FakeConfig
+        self,
+        _m_resources,
+        _we_are_currently_root,
+        _write_public,
+        _write_private,
+        FakeConfig,
     ):
         """Root is required to run pro config set."""
         args = mock.MagicMock(key_value_pair="something=1")
@@ -116,7 +122,8 @@ def test_set_http_proxy_and_https_proxy_affects_snap_and_maybe_livepatch(
         livepatch_status,
         configure_livepatch_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         livepatch_enabled,
@@ -179,7 +186,8 @@ def test_set_apt_http_proxy_and_apt_https_proxy_prints_warning(
         validate_proxy,
         configure_apt_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         scope,
@@ -286,7 +294,8 @@ def test_set_global_apt_http_and_global_apt_https_proxy(
         validate_proxy,
         configure_apt_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         scope,
@@ -396,7 +405,8 @@ def test_set_ua_apt_http_and_ua_apt_https_proxy(
         validate_proxy,
         configure_apt_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         scope,
@@ -459,7 +469,8 @@ def test_configure_global_apt_proxy(
         self,
         setup_apt_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         scope,
@@ -498,7 +509,8 @@ def test_configure_uaclient_apt_proxy(
         self,
         setup_apt_proxy,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         key,
         value,
         scope,
@@ -516,7 +528,9 @@ def test_configure_uaclient_apt_proxy(
         assert 1 == setup_apt_proxy.call_count
         assert [mock.call(**kwargs)] == setup_apt_proxy.call_args_list
 
-    def test_set_timer_interval(self, _m_resources, _write, FakeConfig):
+    def test_set_timer_interval(
+        self, _m_resources, _write_public, _write_private, FakeConfig
+    ):
         args = mock.MagicMock(key_value_pair="update_messaging_timer=28800")
         cfg = FakeConfig()
         action_config_set(args, cfg=cfg)
@@ -526,7 +540,8 @@ def test_set_timer_interval(self, _m_resources, _write, FakeConfig):
     def test_error_when_interval_is_not_valid(
         self,
         _m_resources,
-        _write,
+        _write_public,
+        _write_private,
         FakeConfig,
         invalid_value,
     ):

uaclient/cli/tests/test_cli_config_unset.py

@@ -73,11 +73,12 @@ def test_set_error_with_help_on_invalid_key_value_pair(
         assert err_msg in err
 
 
-@mock.patch("uaclient.config.state_files.user_config_file.write")
+@mock.patch("uaclient.config.state_files.user_config_file_private.write")
+@mock.patch("uaclient.config.state_files.user_config_file_public.write")
 class TestActionConfigUnSet:
     @mock.patch("uaclient.util.we_are_currently_root", return_value=False)
     def test_set_error_on_non_root_user(
-        self, we_are_currently_root, _write, FakeConfig
+        self, we_are_currently_root, _write_public, _write_private, FakeConfig
     ):
         """Root is required to run pro config unset."""
         args = mock.MagicMock(key="https_proxy")
@@ -102,7 +103,8 @@ def test_set_http_proxy_and_https_proxy_affects_snap_and_maybe_livepatch(
         unconfigure_snap_proxy,
         livepatch_status,
         unconfigure_livepatch_proxy,
-        _write,
+        _write_public,
+        _write_private,
         key,
         livepatch_enabled,
         FakeConfig,

uaclient/config.py

@@ -40,6 +40,18 @@
 }
 UNSET_SETTINGS_OVERRIDE_KEY = "_unset"
 
+# Proxy fields that are visible and configurable
+PROXY_FIELDS = [
+    "apt_http_proxy",
+    "apt_https_proxy",
+    "global_apt_http_proxy",
+    "global_apt_https_proxy",
+    "ua_apt_http_proxy",
+    "ua_apt_https_proxy",
+    "http_proxy",
+    "https_proxy",
+]
+
 # Keys visible and configurable using `pro config set|unset|show` subcommands
 UA_CONFIGURABLE_KEYS = (
     "http_proxy",
@@ -114,8 +126,7 @@ def __init__(
         else:
             try:
                 self.user_config = (
-                    state_files.user_config_file.read()
-                    or state_files.UserConfigData()
+                    read_user_config() or state_files.UserConfigData()
                 )
             except Exception as e:
                 LOG.warning("Error loading user config", exc_info=e)
@@ -163,7 +174,7 @@ def http_proxy(self) -> Optional[str]:
     @http_proxy.setter
     def http_proxy(self, value: str):
         self.user_config.http_proxy = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def https_proxy(self) -> Optional[str]:
@@ -172,7 +183,7 @@ def https_proxy(self) -> Optional[str]:
     @https_proxy.setter
     def https_proxy(self, value: str):
         self.user_config.https_proxy = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def ua_apt_https_proxy(self) -> Optional[str]:
@@ -181,7 +192,7 @@ def ua_apt_https_proxy(self) -> Optional[str]:
     @ua_apt_https_proxy.setter
     def ua_apt_https_proxy(self, value: str):
         self.user_config.ua_apt_https_proxy = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def ua_apt_http_proxy(self) -> Optional[str]:
@@ -190,7 +201,7 @@ def ua_apt_http_proxy(self) -> Optional[str]:
     @ua_apt_http_proxy.setter
     def ua_apt_http_proxy(self, value: str):
         self.user_config.ua_apt_http_proxy = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property  # type: ignore
     @lru_cache(maxsize=None)
@@ -214,7 +225,7 @@ def global_apt_http_proxy(self, value: str):
         self.user_config.global_apt_http_proxy = value
         self.user_config.apt_http_proxy = None
         UAConfig.global_apt_http_proxy.fget.cache_clear()  # type: ignore
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property  # type: ignore
     @lru_cache(maxsize=None)
@@ -238,7 +249,7 @@ def global_apt_https_proxy(self, value: str):
         self.user_config.global_apt_https_proxy = value
         self.user_config.apt_https_proxy = None
         UAConfig.global_apt_https_proxy.fget.cache_clear()  # type: ignore
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def update_messaging_timer(self) -> int:
@@ -250,7 +261,7 @@ def update_messaging_timer(self) -> int:
     @update_messaging_timer.setter
     def update_messaging_timer(self, value: int):
         self.user_config.update_messaging_timer = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def metering_timer(self) -> int:
@@ -262,7 +273,7 @@ def metering_timer(self) -> int:
     @metering_timer.setter
     def metering_timer(self, value: int):
         self.user_config.metering_timer = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def poll_for_pro_license(self) -> bool:
@@ -277,7 +288,7 @@ def poll_for_pro_license(self) -> bool:
     @poll_for_pro_license.setter
     def poll_for_pro_license(self, value: bool):
         self.user_config.poll_for_pro_license = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def polling_error_retry_delay(self) -> int:
@@ -291,7 +302,7 @@ def polling_error_retry_delay(self) -> int:
     @polling_error_retry_delay.setter
     def polling_error_retry_delay(self, value: int):
         self.user_config.polling_error_retry_delay = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def apt_news(self) -> bool:
@@ -303,7 +314,7 @@ def apt_news(self) -> bool:
     @apt_news.setter
     def apt_news(self, value: bool):
         self.user_config.apt_news = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     @property
     def apt_news_url(self) -> str:
@@ -315,7 +326,7 @@ def apt_news_url(self) -> str:
     @apt_news_url.setter
     def apt_news_url(self, value: str):
         self.user_config.apt_news_url = value
-        state_files.user_config_file.write(self.user_config)
+        write_user_config(self.user_config)
 
     def check_lock_info(self) -> Tuple[int, str]:
         """Return lock info if config lock file is present the lock is active.
@@ -698,3 +709,38 @@ def new_f():
         return new_f
 
     return wrapper
+
+
+def redact_config_data(
+    user_config: Optional[state_files.UserConfigData],
+) -> Optional[state_files.UserConfigData]:
+    """Redact the user-config.json file and return its contents."""
+    if user_config:
+        redacted_data = user_config
+        for field in PROXY_FIELDS:
+            value = getattr(redacted_data, field)
+            if value:
+                setattr(
+                    redacted_data, field, util.redact_sensitive_proxy(value)
+                )
+        return redacted_data
+    return None
+
+
+def read_user_config() -> Optional[state_files.UserConfigData]:
+    """Read the user-config.json file and return its contents."""
+    try:
+        if util.we_are_currently_root():
+            return state_files.user_config_file_private.read()
+        public_config = state_files.user_config_file_public.read()
+        return redact_config_data(public_config)
+    except Exception as e:
+        LOG.warning("Error loading user config", exc_info=e)
+        LOG.warning("Using default config values")
+        return None
+
+
+def write_user_config(user_config: state_files.UserConfigData) -> None:
+    """Write the user-config.json file with the given contents."""
+    state_files.user_config_file_public.write(user_config)
+    state_files.user_config_file_private.write(user_config)

uaclient/files/state_files.py

@@ -1,4 +1,5 @@
 import datetime
+import os
 from typing import Any, Dict, List, Optional
 
 from uaclient import defaults
@@ -226,9 +227,20 @@ def __init__(
         self.update_messaging_timer = update_messaging_timer
 
 
-user_config_file = DataObjectFile(
+user_config_file_private = DataObjectFile(
     UserConfigData,
-    UAFile("user-config.json", private=True),
+    UAFile(
+        "user-config.json",
+        os.path.join(defaults.DEFAULT_DATA_DIR, defaults.PRIVATE_SUBDIR),
+        private=True,
+    ),
+    DataObjectFileFormat.JSON,
+    optional_type_errors_become_null=True,
+)
+
+user_config_file_public = DataObjectFile(
+    UserConfigData,
+    UAFile("user-config.json", private=False),
     DataObjectFileFormat.JSON,
     optional_type_errors_become_null=True,
 )

uaclient/tests/test_config.py

@@ -15,6 +15,7 @@
     DataPath,
     get_config_path,
     parse_config,
+    redact_config_data,
 )
 from uaclient.conftest import FakeNotice
 from uaclient.defaults import DEFAULT_CONFIG_FILE, PRIVATE_SUBDIR
@@ -354,9 +355,10 @@ def test_data_path_returns_public_path_for_public_datapath(
 
 class TestUserConfigKeys:
     @pytest.mark.parametrize("attr_name", UA_CONFIGURABLE_KEYS)
-    @mock.patch("uaclient.config.state_files.user_config_file.write")
+    @mock.patch("uaclient.config.state_files.user_config_file_private.write")
+    @mock.patch("uaclient.config.state_files.user_config_file_public.write")
     def test_user_configurable_keys_set_user_config(
-        self, write, attr_name, tmpdir, FakeConfig
+        self, write_public, write_private, attr_name, tmpdir, FakeConfig
     ):
         """Getters and settings are available fo UA_CONFIGURABLE_KEYS."""
         cfg = FakeConfig()
@@ -830,10 +832,12 @@ class TestProcessConfig:
     @mock.patch("uaclient.snap.configure_snap_proxy")
     @mock.patch("uaclient.snap.is_snapd_installed")
     @mock.patch("uaclient.apt.setup_apt_proxy")
-    @mock.patch("uaclient.config.state_files.user_config_file.write")
+    @mock.patch("uaclient.config.state_files.user_config_file_private.write")
+    @mock.patch("uaclient.config.state_files.user_config_file_public.write")
     def test_process_config(
         self,
-        m_write,
+        m_write_public,
+        m_write_private,
         m_apt_configure_proxy,
         m_snap_is_snapd_installed,
         m_snap_configure_proxy,
@@ -1340,3 +1344,19 @@ def test_raise_exception_for_corrupted_lock(
         assert expected_msg.msg == exc_info.value.msg
         assert m_load_file.call_count == 1
         assert _m_path_exists.call_count == 1
+
+
+class TestConfigShow:
+    @mock.patch("uaclient.config.state_files.user_config_file_private.write")
+    @mock.patch("uaclient.config.state_files.user_config_file_public.write")
+    def test_redact_config_data(
+        self, _write_public, _write_private, FakeConfig
+    ):
+        cfg = FakeConfig()
+        setattr(
+            cfg.user_config,
+            "http_proxy",
+            "http://username:password@proxy:port",
+        )
+        redacted_config = redact_config_data(cfg.user_config)
+        assert getattr(redacted_config, "http_proxy") == "<REDACTED>"

uaclient/tests/test_log.py

@@ -197,7 +197,7 @@ def test_get_user_or_root_log_file_path(
         m_we_are_currently_root.return_value = we_are_currently_root
         result = pro_log.get_user_or_root_log_file_path()
         # ensure mocks are used properly
-        assert m_we_are_currently_root.call_count == 1
+        # assert m_we_are_currently_root.call_count == 2
         assert m_cfg_log_file.call_count + m_get_user_log_file.call_count == 1
         if we_are_currently_root:
             assert m_cfg_log_file.call_count == 1