User config split into private and public

Fixes: #2809
canonical · Feb 23, 2024 · 380d026 · 380d026
1 parent 3ac3da6
commit 380d026
Show file tree

Hide file tree

Showing 8 changed files with 180 additions and 34 deletions.
diff --git a/features/config.feature b/features/config.feature
@@ -50,3 +50,39 @@ Feature: pro config sub-command
             | xenial  | lxd-container |
             | jammy   | lxd-container |
             | mantic  | lxd-container |
+
+    Scenario Outline: Proxy information is redacted when using pro config show
+            Given a `<release>` `<machine_type>` machine with ubuntu-advantage-tools installed
+            Given a `focal` `lxd-container` machine named `proxy`
+            When I apt update on the `proxy` machine
+            And I apt install `squid apache2-utils` on the `proxy` machine
+            And I run `htpasswd -bc /etc/squid/passwordfile someuser somepassword` `with sudo` on the `proxy` machine
+            And I add this text on `/etc/squid/squid.conf` on `proxy` above `http_access deny all`:
+            """
+            dns_v4_first on\nauth_param basic program \/usr\/lib\/squid\/basic_ncsa_auth \/etc\/squid\/passwordfile\nacl topsecret proxy_auth REQUIRED\nhttp_access allow topsecret
+            """
+            And I run `systemctl restart squid.service` `with sudo` on the `proxy` machine
+            When I create the file `/var/lib/ubuntu-advantage/user-config.json` with the following:
+            """
+            {
+            "http_proxy": "http://someuser:somepassword@$behave_var{machine-ip proxy}:3128",
+            "https_proxy": "http://someuser:somepassword@$behave_var{machine-ip proxy}:3128"
+            }
+            """
+            When I run `pro config set http_proxy=http://someuser:somepassword@$behave_var{machine-ip proxy}:3128` with sudo
+            When I run `pro config show` with sudo
+            Then stdout matches regexp:
+            """
+            http_proxy              http://someuser:somepassword@$behave_var{machine-ip proxy}:3128
+            """
+            When I run `pro config show` as non-root
+            Then stdout matches regexp:
+            """
+            http_proxy              <REDACTED>
+            """
+            Examples: ubuntu release
+            | release | machine_type  |
+            | xenial  | lxd-container |
+            | bionic  | lxd-container |
+            | focal   | lxd-container |
+            | jammy   | lxd-container |
diff --git a/uaclient/cli/tests/test_cli_config_set.py b/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,
     ):

diff --git a/uaclient/cli/tests/test_cli_config_unset.py b/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,

diff --git a/uaclient/config.py b/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)
diff --git a/uaclient/files/state_files.py b/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,
 )