[RHELC-888] Make RHSM organization and username a secret #725

bocekm · 2023-02-03T15:27:28Z

Until now we've been recording and collecting the RHSM username and organization in their plain text form in the convert2rhel log files and the breadcrumbs files:

/var/log/convert2rhel/convert2rhel.log
/var/log/convert2rhel/archive/convert2rhel-*.log
/etc/migration-results
/etc/rhsm/facts/convert2rhel.facts

Users might perceive those values as sensitive or private information. Red Hat doesn't need this information for anything so it's better to not collect it at all.

Jira Issue: RHELC-888

Checklist

PR meets acceptance criteria specified in the Jira issue
PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-888] is part of the PR title
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending

codecov · 2023-02-03T15:30:07Z

Codecov Report

Base: 92.70% // Head: 92.70% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (5aeecb1) compared to base (e2c822e).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
- Coverage   92.70%   92.70%   -0.01%     
==========================================
  Files          24       24              
  Lines        3278     3275       -3     
  Branches      576      575       -1     
==========================================
- Hits         3039     3036       -3     
  Misses        172      172              
  Partials       67       67

Flag	Coverage Δ
centos-linux-7	`87.96% <100.00%> (-0.02%)`	⬇️
centos-linux-8	`89.09% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
convert2rhel/breadcrumbs.py	`98.05% <100.00%> (-0.02%)`	⬇️
convert2rhel/subscription.py	`92.72% <100.00%> (-0.04%)`	⬇️
convert2rhel/utils.py	`86.77% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

convert2rhel/utils.py

bocekm · 2023-02-03T15:29:19Z

convert2rhel/breadcrumbs.py

@@ -93,8 +93,7 @@ def _set_pkg_object(self):

    def _set_executed(self):
        """Set how was Convert2RHEL executed"""
-        cli_options_to_sanitize = frozenset(("--password", "-p", "--activationkey", "-k"))


No need to have the options to sanitize different for logs and breadcrumbs.
Better to keep the list just on one place.

bocekm · 2023-02-03T15:33:32Z

convert2rhel/utils.py

-    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.
+    Terminology:


For clarity I've included terminology that made it easier to name variables and form a meaningful debug message.

bocekm · 2023-02-03T15:35:36Z

convert2rhel/utils.py

@@ -718,16 +718,25 @@ def is_dir_empty(path):
            os.rmdir(path)


-def hide_secrets(args, secret_args=frozenset(("--password", "--activationkey", "--token", "-p", "-k"))):


I've removed the "--token" because convert2rhel does not accept such an option and I don't expect it to be accepted in the future either (I don't know about any kind of token that could be utilized during the conversion).

bocekm · 2023-02-03T20:22:55Z

TODO: Update related log messages after #681 gets merged (https://github.com/oamg/convert2rhel/pull/681/files#r1096154508).

EDIT: Done.

Venefilyn · 2023-02-06T16:54:45Z

#681 is now merged, please rebase

Venefilyn · 2023-02-08T21:51:20Z

convert2rhel/utils.py

-            "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]


LGTM bot is gone so can remove this

Suggested change

" secret parameter".format(sanitized_list[-1]) # lgtm[py/clear-text-logging-sensitive-data]

" secret parameter".format(sanitized_list[-1])

That change was not applied, @SpyTec do you want to proceed on merging this and removing the comment later?

Oh, I must have missed that. I can fix it, no problem. We still have a couple other PRs left to finish, so we don't need to merge this now.

Venefilyn

Failing integration tests unrelated. Can be merged

convert2rhel/unit_tests/utils_test.py

convert2rhel/utils.py

r0x0d

Looks good in my opinion, just some small comments that should not block the PR from getting merged.

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.

danmyway · 2023-02-15T19:08:06Z

tests/integration/tier0/test-check-rollback-handling/test_check_rollback_handling.py

@@ -78,7 +78,7 @@ def terminate_and_assert_good_rollback(c2r):
    # Verify the last step of the rollback is present in the log file
    with open("/var/log/convert2rhel/convert2rhel.log", "r") as logfile:
        for line in logfile:
-            assert "Rollback: Removing installed RHSM certificate" not in line
+            assert "Rollback: Remove installed RHSM certificate" not in line


leftover from #726

bocekm · 2023-02-15T21:48:56Z

The integration test update looks good to me, @danmyway, thanks 👍

r0x0d · 2023-02-16T00:38:17Z

Will give another round of review tomorrow if this was not merged before I start working tomorrow.

r0x0d

Still looks good, just one question to @SpyTec if he wants to merge even without his suggestion for removing the lgtm comments was not applied.

@bocekm one question, though: Should we do the same for the pool parameter?

Otherwise, feel free to merge whenever you feel like it.

* verify that the username and organization get obfuscated in log file and bredcrumbs Signed-off-by: Daniel Diblik <[email protected]>

bocekm · 2023-02-16T18:24:04Z

convert2rhel/utils.py


        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])


I dismissed the alert, choosing that it is False positive.
Hopefully it's enough so that others won't see this failure in other PRs.

bocekm · 2023-02-16T18:28:13Z

@bocekm one question, though: Should we do the same for the pool parameter?

No, because Pool IDs are random alphanumerical strings from which no one can infer anything about the subscription, and knowing the string doesn't give those without account credentials the ability to use it in any way.

What we should do though is allowing to pass the username and organization through the new config file. I'll create a task for that (EDIT: here it is https://issues.redhat.com/browse/RHELC-901).

danmyway · 2023-02-20T17:04:52Z

/packit retest-failed

bocekm · 2023-02-20T20:08:42Z

@danmyway, telling Packit to retest-failed brings an unintended consequence these days - all tests are re-run, not just the failed (packit/packit-service#1886). That can lead to a situation where the ones that previously passed suddenly end up failing :)

Better to re-run the failed tests manually one by one under the Checks tab. That's what I've done now for three failed jobs.

danmyway · 2023-02-21T07:37:01Z

@danmyway, telling Packit to retest-failed brings an unintended consequence these days - all tests are re-run, not just the failed (packit/packit-service#1886). That can lead to a situation where the ones that previously passed suddenly end up failing :)

Better to re-run the failed tests manually one by one under the Checks tab. That's what I've done now for three failed jobs.

Thanks for the heads-up @bocekm, noted!

bocekm · 2023-02-21T16:31:52Z

@danmyway, FYI I've re-run testing-farm:oraclelinux-8.6-x86_64:tier1. There was one test that failed due to flaky RHSM.

danmyway · 2023-02-21T18:34:43Z

@danmyway, FYI I've re-run testing-farm:oraclelinux-8.6-x86_64:tier1. There was one test that failed due to flaky RHSM.

I'd say waive and merge if that happens again.
Code proved working.

bocekm requested review from abadger and r0x0d February 3, 2023 15:27

bocekm changed the title ~~Make org a secret~~ Make RHSM organization and username a secret Feb 3, 2023

bocekm changed the title ~~Make RHSM organization and username a secret~~ [RHELC-888] Make RHSM organization and username a secret Feb 3, 2023

github-advanced-security bot found potential problems Feb 3, 2023

View reviewed changes

convert2rhel/utils.py Fixed Show fixed Hide fixed

bocekm commented Feb 3, 2023

View reviewed changes

bocekm mentioned this pull request Feb 3, 2023

[RHELC-723] Remove RegistrationCommand.args property #681

Merged

7 tasks

bocekm marked this pull request as draft February 3, 2023 20:23

Venefilyn added the kind/security Anything which improves security or fixes vulnerabilities label Feb 7, 2023

bocekm force-pushed the make-org-a-secret branch 2 times, most recently from 875fb4c to 7d3c004 Compare February 8, 2023 21:11

bocekm marked this pull request as ready for review February 8, 2023 21:12

Venefilyn reviewed Feb 8, 2023

View reviewed changes

Venefilyn force-pushed the make-org-a-secret branch from 7d3c004 to 4d3aab6 Compare February 9, 2023 22:33

Venefilyn approved these changes Feb 10, 2023

View reviewed changes