Hide RHSM org and username from logs & breadcrumbs

A username and organization, the RHSM registration credentials, might be
perceived as sensitive pieces of information. Having them in logs or
breadcrumbs adds no additional value so better to replace it with
asterisks.

convert2rhel/breadcrumbs.py

@@ -98,8 +98,7 @@ def _set_pkg_object(self):
 
     def _set_executed(self):
         """Set how was Convert2RHEL executed"""
-        cli_options_to_sanitize = frozenset(("--password", "-p", "--activationkey", "-k"))
-        self.executed = " ".join(utils.hide_secrets(args=sys.argv, secret_args=cli_options_to_sanitize))
+        self.executed = " ".join(utils.hide_secrets(args=sys.argv))
 
     def _set_nevra(self):
         """Set NEVRA of installed Convert2RHEL"""

convert2rhel/subscription.py

@@ -334,9 +334,6 @@ def from_tool_opts(cls, tool_opts):
                 loggerinst.info("    ... password detected")
                 password = tool_opts.password
             else:
-                if tool_opts.username:
-                    # Hint user for which username they need to enter pswd
-                    loggerinst.info("Username: %s", username)  # lgtm[py/clear-text-logging-sensitive-data]
                 password = ""
                 while not password:
                     password = utils.prompt_user("Password: ", password=True)
@@ -422,9 +419,9 @@ def __call__(self):
             try:
                 if self.password:
                     if self.org:
-                        loggerinst.info("Organization: %s", self.org)
-                    loggerinst.info("Username: %s", self.username)
-                    loggerinst.info("Password: %s", "*" * 5)
+                        loggerinst.info("Organization: %s", utils.OBFUSCATION_STRING)
+                    loggerinst.info("Username: %s", utils.OBFUSCATION_STRING)
+                    loggerinst.info("Password: %s", utils.OBFUSCATION_STRING)
                     loggerinst.info("Connection Options: %s", self.connection_opts)
                     loggerinst.info("Locale settings: %s", i18n.SUBSCRIPTION_MANAGER_LOCALE)
                     args = (
@@ -446,8 +443,8 @@ def __call__(self):
                     )
 
                 else:
-                    loggerinst.info("Organization: %s", self.org)
-                    loggerinst.info("Activation Key: %s", "*" * 5)
+                    loggerinst.info("Organization: %s", utils.OBFUSCATION_STRING)
+                    loggerinst.info("Activation Key: %s", utils.OBFUSCATION_STRING)
                     loggerinst.info("Connection Options: %s", self.connection_opts)
                     loggerinst.info("Locale settings: %s", i18n.SUBSCRIPTION_MANAGER_LOCALE)
                     args = (

convert2rhel/unit_tests/breadcrumbs_test.py

@@ -87,11 +87,11 @@ def test_finish_collection(pretend_os, success, monkeypatch):
                 "--username=test",
                 "--password=nicePassword",
             ],
-            "/usr/bin/convert2rhel --username=test --password=*****",
+            "/usr/bin/convert2rhel --username=***** --password=*****",
         ),
         (
             ["/usr/bin/convert2rhel", "-u=test", "-p=nicePassword"],
-            "/usr/bin/convert2rhel -u=test -p=*****",
+            "/usr/bin/convert2rhel -u=***** -p=*****",
         ),
         (
             [
@@ -100,11 +100,11 @@ def test_finish_collection(pretend_os, success, monkeypatch):
                 "--org=1234",
                 "-y",
             ],
-            "/usr/bin/convert2rhel --activationkey=***** --org=1234 -y",
+            "/usr/bin/convert2rhel --activationkey=***** --org=***** -y",
         ),
         (
             ["/usr/bin/convert2rhel", "-k=test", "-o=1234", "-y"],
-            "/usr/bin/convert2rhel -k=***** -o=1234 -y",
+            "/usr/bin/convert2rhel -k=***** -o=***** -y",
         ),
     ),
 )

convert2rhel/unit_tests/subscription_test.py

@@ -781,7 +781,7 @@ def test_connection_opts(self, rhsm_hostname, rhsm_port, rhsm_prefix, expected):
                 None,
                 "RegisterWithActivationKeys",
                 "sasa{sv}a{sv}s",
-                "Organization: Local Organization",
+                "Organization: *****",
             ),
             (
                 "Local Organization",
@@ -790,7 +790,7 @@ def test_connection_opts(self, rhsm_hostname, rhsm_port, rhsm_prefix, expected):
                 "pass_word",
                 "Register",
                 "sssa{sv}a{sv}s",
-                "Organization: Local Organization",
+                "Organization: *****",
             ),
             (None, None, "user_name", "pass_word", "Register", "sssa{sv}a{sv}s", None),
         ),

convert2rhel/unit_tests/toolopts_test.py

@@ -479,17 +479,17 @@ def test_validate_serverurl_parsing(url_parts, message):
 def test__log_command_used(caplog, monkeypatch):
     obfuscation_string = "*" * 5
     input_command = mock_cli_arguments(
-        ["--username", "uname", "--password", "123", "--activationkey", "456", "--token", "789"]
+        ["--username", "uname", "--password", "123", "--activationkey", "456", "--org", "789"]
     )
     expected_command = mock_cli_arguments(
         [
             "--username",
-            "uname",
+            obfuscation_string,
             "--password",
             obfuscation_string,
             "--activationkey",
             obfuscation_string,
-            "--token",
+            "--org",
             obfuscation_string,
         ]
     )
@@ -503,7 +503,7 @@ def test__log_command_used(caplog, monkeypatch):
     ("argv", "message"),
     (
         # The message is a log of used command
-        (mock_cli_arguments(["-o", "org", "-k", "key"]), "-o org -k *****"),
+        (mock_cli_arguments(["-o", "org", "-k", "key"]), "-o ***** -k *****"),
         (
             mock_cli_arguments(["-o", "org"]),
             "Either the --organization or the --activationkey option is missing. You can't use one without the other.",

convert2rhel/unit_tests/utils_test.py

@@ -601,7 +601,7 @@ def test_remove_orphan_folders(path_exists, list_dir, expected, tmpdir, monkeypa
 
 
 @pytest.mark.parametrize(
-    ("arguments", "secret_args", "expected"),
+    ("arguments", "secret_options", "expected"),
     (
         # No sanitization is being used here
         (["-h"], frozenset(), ["-h"]),
@@ -619,37 +619,30 @@ def test_remove_orphan_folders(path_exists, list_dir, expected, tmpdir, monkeypa
             frozenset(),
             ["--argument=with space in it", "--another"],
         ),
-        # Single parameter being passed
+        # Single option being passed
         (
             ["--activationkey=123"],
             frozenset(("--activationkey",)),
             ["--activationkey=*****"],
         ),
-        # Multiple parameters and hide the secrets only for activation key
+        # Hide the secrets in the short form of the options
         (
-            ["--activationkey=123", "--org=1234", "-y"],
+            ["-u=test", "-p=Super@Secret@Password", "-k=123", "-o=1234"],
             frozenset(
-                ("--activationkey",),
+                (
+                    "-u",
+                    "-p",
+                    "-k",
+                    "-o",
+                )
             ),
-            ["--activationkey=*****", "--org=1234", "-y"],
-        ),
-        # Hide the secrets for activationkey in it's short form
-        (
-            ["-k=123"],
-            frozenset(("-k",)),
-            ["-k=*****"],
-        ),
-        # Hide the secrets for password only
-        (
-            ["--username=test", "--password=Super@Secret@Password"],
-            frozenset(("--password",)),
-            ["--username=test", "--password=*****"],
+            ["-u=*****", "-p=*****", "-k=*****", "-o=*****"],
         ),
         # Multiple sanitizations should occur in the next test
         (
             ["--username=test", "--password=Super@Secret@Password", "--activationkey=123", "--org=1234", "-y"],
-            frozenset(("--password", "--activationkey")),
-            ["--username=test", "--password=*****", "--activationkey=*****", "--org=1234", "-y"],
+            frozenset(("--username", "--password", "--activationkey", "--org")),
+            ["--username=*****", "--password=*****", "--activationkey=*****", "--org=*****", "-y"],
         ),
         # Test the same sanitization but without the equal sign ("=") in the arguments
         (
@@ -664,8 +657,8 @@ def test_remove_orphan_folders(path_exists, list_dir, expected, tmpdir, monkeypa
                 "1234",
                 "-y",
             ],
-            frozenset(("--password", "--activationkey")),
-            ["--username", "test", "--password", "*****", "--activationkey", "*****", "--org", "1234", "-y"],
+            frozenset(("--username", "--password", "--activationkey", "--org")),
+            ["--username", "*****", "--password", "*****", "--activationkey", "*****", "--org", "*****", "-y"],
         ),
         # A real world example of how the tool would be used
         (
@@ -678,26 +671,24 @@ def test_remove_orphan_folders(path_exists, list_dir, expected, tmpdir, monkeypa
                 "--debug",
                 "-y",
             ],
-            frozenset(("--password", "-p", "--activationkey", "-k")),
+            frozenset(
+                (
+                    "--username",
+                    "--password",
+                    "--activationkey",
+                )
+            ),
             [
                 "/usr/bin/convert2rhel",
-                "--username=test",
+                "--username=*****",
                 "--password=*****",
                 "--pool=e6e3f4ca-342f-11ed-b5eb-6c9466263bdf",
                 "--no-rpm-va",
                 "--debug",
                 "-y",
             ],
         ),
-        # Test password with special strings
-        (
-            ["--password", "\\)(*&^%f %##@^%&*&^("],
-            frozenset(("--password",)),
-            [
-                "--password",
-                "*****",
-            ],
-        ),
+        # Test replacement of parameters with special characters
         (
             ["--password", " "],
             frozenset(
@@ -724,13 +715,13 @@ def test_remove_orphan_folders(path_exists, list_dir, expected, tmpdir, monkeypa
         ),
     ),
 )
-def test_hide_secrets(arguments, secret_args, expected):
-    sanitazed_cmd = utils.hide_secrets(arguments, secret_args=secret_args)
+def test_hide_secrets(arguments, secret_options, expected):
+    sanitazed_cmd = utils.hide_secrets(arguments, secret_options=secret_options)
     assert sanitazed_cmd == expected
 
 
 def test_hide_secrets_default():
-    """Test that the default secret_args are sound."""
+    """Test that the default secret_options cover all known secrets."""
     test_cmd = [
         "register",
         "--force",
@@ -748,15 +739,15 @@ def test_hide_secrets_default():
     assert sanitized_cmd == [
         "register",
         "--force",
-        "--username=jdoe",
+        "--username=*****",
         "--password=*****",
         "-p=*****",
         "--activationkey=*****",
         "-k=*****",
         "--pool=e6e3f4ca-342f-11ed-b5eb-6c9466263bdf",
         "--no-rpm-va",
         "--debug",
-        "--org=0123",
+        "--org=*****",
     ]
 
 
@@ -765,15 +756,15 @@ def test_hide_secrets_no_secrets():
     test_cmd = [
         "register",
         "--force",
-        "--username=jdoe",
-        "--org=0123",
+        "--no-rpm-va",
+        "-y",
     ]
     sanitized_cmd = utils.hide_secrets(test_cmd)
     assert sanitized_cmd == [
         "register",
         "--force",
-        "--username=jdoe",
-        "--org=0123",
+        "--no-rpm-va",
+        "-y",
     ]
 
 
@@ -794,13 +785,13 @@ def test_hide_secret_unexpected_input(caplog):
         "register",
         "--force",
         "--password=*****",
-        "--username=jdoe",
-        "--org=0123",
+        "--username=*****",
+        "--org=*****",
         "--activationkey",
     ]
     assert len(caplog.records) == 1
     assert caplog.records[-1].levelname == "DEBUG"
-    assert "Passed arguments had unexpected secret argument," " '--activationkey', without a secret" in caplog.text
+    assert "Passed arguments had an option, '--activationkey', without an expected secret parameter" in caplog.text
 
 
 @pytest.mark.parametrize(

convert2rhel/utils.py

@@ -38,6 +38,9 @@
 
 loggerinst = logging.getLogger(__name__)
 
+# A string we're using to replace sensitive information (like an RHSM password) in logs, terminal output, etc.
+OBFUSCATION_STRING = "*" * 5
+
 
 class ImportGPGKeyError(Exception):
     """Raised for failures during the rpm import of gpg keys."""
@@ -702,44 +705,51 @@ def is_dir_empty(path):
             os.rmdir(path)
 
 
-def hide_secrets(args, secret_args=frozenset(("--password", "--activationkey", "--token", "-p", "-k"))):
+def hide_secrets(
+    args, secret_options=frozenset(("--username", "--password", "--activationkey", "--org", "-u", "-p", "-k", "-o"))
+):
     """
-    Replace secret values with asterisks.
+    Replace secret values passed through a command line with asterisks.
 
-    This function takes a list of arguments which will be passed
-    in a transformation process where we will replace any secret values
-    with an fixed size of asterisks (*) and returns a new list containing
-     the arguments with this transformation.
+    This function takes a list of command line arguments and returns a new list containing where any secret value is
+    replaced with a fixed size of asterisks (*).
 
-    :arg args: An argument list which may contain secret values.
+    Terminology:
+     - an argument is one of whitespace delimited strings of an executed cli command
+       Example: `ls -a -w 10 dir` ... four arguments (-a, -w, 10, dir)
+     - an option is a subset of arguments that start with - or -- and modify the behavior of a cli command
+       Example: `ls -a -w 10 dir` ... two options (-a, -w)
+     - an option parameter is the argument following an option if the option requires a value
+       Example: `ls -a -w 10 dir` ... one option parameter (10)
+
+    :param: args: A list of command line arguments which may contain secret values.
+    :param: secret_options: A set of command line options requiring sensitive or private information.
     :returns: A new list of arguments with secret values hidden.
     """
-    obfuscation_string = "*" * 5
-
     sanitized_list = []
     hide_next = False
     for arg in args:
         if hide_next:
-            # Second part of a two part secret argument (like --password *SECRET*)
-            arg = obfuscation_string
+            # This is a parameter of a secret option
+            arg = OBFUSCATION_STRING
             hide_next = False
 
-        elif arg in secret_args:
-            # First part of a two part secret argument (like *--password* SECRET)
+        elif arg in secret_options:
+            # This is a secret option => mark its parameter (the following argument) to be obfuscated
             hide_next = True
 
         else:
-            # A secret argument in one part (like --password=SECRET)
-            for problem_arg in secret_args:
-                if arg.startswith(problem_arg + "="):
-                    arg = "{0}={1}".format(problem_arg, obfuscation_string)
+            # Handle the case where the secret option and its parameter are both in one argument ("--password=SECRET")
+            for option in secret_options:
+                if arg.startswith(option + "="):
+                    arg = "{0}={1}".format(option, OBFUSCATION_STRING)
 
         sanitized_list.append(arg)
 
     if hide_next:
         loggerinst.debug(
-            "Passed arguments had unexpected secret argument,"
-            " '{0}', without a secret".format(sanitized_list[-1])  # lgtm[py/clear-text-logging-sensitive-data]
+            "Passed arguments had an option, '{0}', without an expected"
+            " secret parameter".format(sanitized_list[-1])  # lgtm[py/clear-text-logging-sensitive-data]
         )
 
     return sanitized_list