[cleaner] ignores hostnames in SSL certificates #4093

pafernanr · 2025-07-29T14:55:52Z

Refer to 3919

New clean argument added --treat-certificates with next options:
- obfuscate: (Default) convert certificates to text and let the method obfuscate_report do its job.
- remove: Remove certificate files from the archive.
- keep: Don't touch certificate files and keep them as is in the archive.
Notes:
- Certificate files are identified by extension (.csr, .pem) and header:
  - r'-----BEGIN CERTIFICATE-----'
  - r'-----BEGIN [A-Z]+ PRIVATE KEY-----'
- If openssl is not available and obfuscate is selected. It will be replaced by remove after show a WARNING message.
- Certificate Key files are always removed.

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname [email protected]?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

sos/cleaner/archives/__init__.py

packit-as-a-service · 2025-07-29T14:58:19Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-4093
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

pmoravec · 2025-07-30T06:42:57Z

sos/cleaner/__init__.py

+            if self.opts.treat_certificates == "obfuscate":
+                self.ui_log.warning(
+                    "WARNING: certificate files that potentially contain "
+                    "sensitive information will be CONVERTED to text and "
+                    "OBFUSCATED in the final archive.\n"
+                )
+            elif self.opts.treat_certificates == "keep":
+                self.ui_log.warning(
+                    "WARNING: certificate files that potentially contain "
+                    "sensitive information will be KEPT in the final "
+                    "archive as is.\n"
+                )
+            elif self.opts.treat_certificates == "remove":
+                self.ui_log.warning(
+                    "WARNING: certificate files that potentially contain "
+                    "sensitive information will be REMOVED in the final "
+                    "archive.\n"
+                )


Just a thought / option to consider: do we really want a WARNING message (either one of those three) printed on every cleaner run?

pmoravec · 2025-07-30T06:44:20Z

sos/cleaner/__init__.py

                    continue
+                is_certificate = file_is_certificate(fname)
+                if is_certificate:
+                    if is_certificate == "certificatekey":


In theory, key files should have been removed by report already. But surely they can remain also here, makes sense to remove them despite they dont usually contain any cleaner-sensitive data.

pmoravec · 2025-07-30T06:48:01Z

sos/cleaner/__init__.py

+                        archive.remove_file(short_name)
+                        continue
+                    if self.opts.treat_certificates == "keep":
+                        continue
+                    if self.opts.treat_certificates == "remove":
+                        archive.remove_file(short_name)


Originally, I thought we should put some debug message here "removing certificate file" or similar.

But a better approach is updating remove_file method in sos/cleaner/archives/__init__.py to have extra argument filetype=binary and replace:

self.log_info(f"Removing binary file '{fname}' from archive")

to:
self.log_info(f"Removing {filetype} file '{fname}' from archive")

(and call the method from here with proper argument)

pmoravec · 2025-07-30T06:49:58Z

sos/cleaner/archives/__init__.py

+        sos_get_command_output(
+            f"openssl x509 -noout -text -in {str(fname)}",
+            to_file=f"{fname}.text")
+        shutil.move(f"{fname}.text", fname)


Just a thought / option to consider: should we keep original file's permissions and ownership? We are overwriting it here (which makes sense as well as we update the file content).

My only hangup here is actually dropping the .text suffix. I think if we're converting the cert file, then we should make that obvious, and I do like the idea of having a .text (or .txt if you prefer) appended to the filename.

I agree here, this behaviour can be confusing ("does the pem file really contain just text content, or..?").

Maybe worth appending the new filename to the log "Converting certificate .. above?

Gladly we do the change in cleaner and not in report where we would have to update filelist of collected files - here just removing the old file should be enough.

pmoravec · 2025-07-30T06:51:34Z

sos/cleaner/__init__.py

+        clean_grp.add_argument('--treat-certificates', default='obfuscate',
+                               choices=['obfuscate', 'keep', 'remove'],
+                               dest='treat_certificates',
+                               help=('How to treat certificate files.'


nitpick: Extra dot at the end. Same applies on two equivalent places later on.

pmoravec · 2025-07-30T06:52:22Z

sos/utilities.py

+                return "certificate"
+            if re.search(r'-----BEGIN [A-Z]+ PRIVATE KEY-----', f.read()):
+                return "certificatekey"
+        return None


Just a thought / option to consider: isnt this return redundant?

Yes, its is :) was added just to make lint happy

pmoravec · 2025-07-30T06:53:36Z

I added several comments. Those starting "Just a thought / option to consider:" are really "just a thought" and nothing else - in those cases I am OK with the current change but maybe others can see the mentioned option as a better one?

pmoravec · 2025-07-30T06:57:11Z

The foreman tests failing is foreman infra issue: they have expired GPG key:

# curl https://archivedeb.theforeman.org/foreman.asc | gpg
gpg: WARNING: no command supplied.  Trying to guess what you mean ...
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  2460  100  2460    0     0  25696      0 --:--:-- --:--:-- --:--:-- 25894
pub   rsa4096 2021-07-23 [SCEA] [expired: 2025-07-24]
      5B7C3E5A735BCB4D615829DC0BDDA991FD7AAC8A
uid           Foreman Automatic Signing Key (2021) <[email protected]>
#

I am reporting this to the foreman team.

ekohl · 2025-07-30T08:43:47Z

The foreman tests failing is foreman infra issue: they have expired GPG key:

I opened theforeman/foreman-infra#2280 for this.

sos/cleaner/__init__.py

sos/utilities.py

sos/report/__init__.py

sos/cleaner/archives/__init__.py

pmoravec

ACK (up to the doubt about CodeQL warning).

Thanks Pablo for applying repetitive feedback! :)

pafernanr · 2025-08-01T13:47:59Z

@pmoravec all checks passed.

openssl command modified to "openssl storeutl -noout -text -certs /tmp/BUNDLE_FILE.crt"
That CodeQL warning didn't arise ¿? but "with open" code remains the same :)

TurboTurtle

Ack up to my comment on the filename. I'd like to see us keep the .text/.txt, but willing to hear why that might not be a good idea.

TurboTurtle · 2025-08-08T18:04:53Z

sos/cleaner/archives/__init__.py

+        sos_get_command_output(
+            f"openssl x509 -noout -text -in {str(fname)}",
+            to_file=f"{fname}.text")
+        shutil.move(f"{fname}.text", fname)


My only hangup here is actually dropping the .text suffix. I think if we're converting the cert file, then we should make that obvious, and I do like the idea of having a .text (or .txt if you prefer) appended to the filename.

pmoravec

Sorry @pafernanr for yet another nitpick change requested. This slipped down among the reviews..

pafernanr · 2025-09-15T07:06:39Z

Hi all, last commit behaves as you requested. .pem files are renamed to .pem.text when using sos report --clean --treat-certificates obfuscate

pmoravec · 2025-09-15T08:10:12Z

sos/utilities.py

+    :returns:   The type of the certificate or ``None``
+    :rtype:     ``string`` or ``None``
+    """
+    if fname[-4:] in [".csr", ".pem"]:


Can we check against .crt suffix as well? E.g. various Satellite-related certificates (though afaik neither of them being collected by sos, in neither of plenty locations) have that suffix.

(tiny update: even sosreport from Satellite can contain ./etc/pki/pulp/qpid/ca.crt until Qpid broker is deprecated in Sat)

pmoravec · 2025-09-15T08:10:51Z

Apart of the .crt suffix, two nitpicks:

Please merge the two commits
The WARNING messages shall be wrapped to 80chars like e.g. print_disclaimer in sos/cleaner/__init__.py

sos/cleaner/__init__.py

Signed-off-by: Pablo Fernández Rodríguez <[email protected]>

TurboTurtle · 2025-09-19T15:31:07Z

LGTM, however if you could please rebase on current main to pull in the temporary disabling of Fedora tests in our suite, that will let the rest of the CI run, which does include some sanity testing for cleaner.

TurboTurtle · 2025-09-30T22:07:08Z

Friendly reminder ping to please rebase on current main and repush so we can get a sanity run of the test suite.

pmoravec

ACK, we just should drop the internal merge commit when we will do merge into upstream.

github-advanced-security bot found potential problems Jul 29, 2025

View reviewed changes

sos/cleaner/archives/__init__.py Fixed Show fixed Hide fixed

pafernanr mentioned this pull request Jul 29, 2025

Cleaner ignores hostnames in SSL certificates #3919

Open

pafernanr force-pushed the 3919_certificates_cleaner branch from 568d426 to 3474c83 Compare July 29, 2025 15:04

pmoravec reviewed Jul 30, 2025

View reviewed changes

pmoravec mentioned this pull request Jul 30, 2025

Drop or update foreman specific tests #4094

Closed

pafernanr closed this Jul 30, 2025

pafernanr force-pushed the 3919_certificates_cleaner branch from cb4ddde to 7a1463f Compare July 30, 2025 15:27

pafernanr reopened this Jul 30, 2025

github-advanced-security bot found potential problems Jul 30, 2025

View reviewed changes

sos/cleaner/__init__.py Fixed Show fixed Hide fixed

sos/utilities.py Fixed Show fixed Hide fixed

pafernanr closed this Jul 30, 2025

pafernanr force-pushed the 3919_certificates_cleaner branch from 3b5ee03 to 7a1463f Compare July 30, 2025 17:05

pafernanr reopened this Jul 30, 2025

github-advanced-security bot found potential problems Jul 30, 2025

View reviewed changes

sos/report/__init__.py Fixed Show fixed Hide fixed

pmoravec reviewed Aug 1, 2025

View reviewed changes

sos/cleaner/archives/__init__.py Outdated Show resolved Hide resolved

pafernanr force-pushed the 3919_certificates_cleaner branch from f3fa9f7 to faab870 Compare August 1, 2025 12:25

pmoravec approved these changes Aug 1, 2025

View reviewed changes

pmoravec added Kind/Enhancement Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Kind/cleaner cleaner component of sos labels Aug 1, 2025

pafernanr force-pushed the 3919_certificates_cleaner branch from ee8f7f0 to 1c04560 Compare August 1, 2025 15:51

pafernanr requested a review from bmr-cymru August 1, 2025 20:01

pmoravec approved these changes Aug 3, 2025

View reviewed changes

TurboTurtle reviewed Aug 8, 2025

View reviewed changes

pmoravec requested changes Sep 12, 2025

View reviewed changes

pafernanr force-pushed the 3919_certificates_cleaner branch from 475e389 to 3c96a3c Compare September 15, 2025 07:01

pmoravec reviewed Sep 15, 2025

View reviewed changes

sos/cleaner/__init__.py Outdated Show resolved Hide resolved

pafernanr force-pushed the 3919_certificates_cleaner branch from 4126e80 to 803e6fe Compare September 15, 2025 08:22

[cleaner] ignores hostnames in SSL certificates

e506c7f

Signed-off-by: Pablo Fernández Rodríguez <[email protected]>

pafernanr force-pushed the 3919_certificates_cleaner branch from 803e6fe to e506c7f Compare September 15, 2025 08:35

pmoravec approved these changes Sep 15, 2025

View reviewed changes

TurboTurtle approved these changes Sep 19, 2025

View reviewed changes

TurboTurtle added Reviewed/Ready for Merge Has been reviewed, ready for merge and removed Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer labels Sep 19, 2025

pafernanr force-pushed the 3919_certificates_cleaner branch from 748e8ad to e506c7f Compare October 2, 2025 07:33

Merge branch 'sosreport:main' into 3919_certificates_cleaner

3ad45ca

pmoravec approved these changes Oct 5, 2025

View reviewed changes

[cleaner] ignores hostnames in SSL certificates #4093

Are you sure you want to change the base?

[cleaner] ignores hostnames in SSL certificates #4093

Uh oh!

Conversation

pafernanr commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

packit-as-a-service bot commented Jul 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec commented Jul 30, 2025

Uh oh!

pmoravec commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekohl commented Jul 30, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

pafernanr commented Aug 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TurboTurtle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

pafernanr commented Sep 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmoravec commented Sep 15, 2025

Uh oh!

Uh oh!

TurboTurtle commented Sep 19, 2025

Uh oh!

TurboTurtle commented Sep 30, 2025

Uh oh!

pmoravec left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pafernanr commented Jul 29, 2025 •

edited

Loading

pmoravec commented Jul 30, 2025 •

edited

Loading

pafernanr commented Aug 1, 2025 •

edited

Loading