Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[cleaner] Add option to skip cleaning files #3520

Merged
merged 2 commits into from
Mar 11, 2024
Merged

[cleaner] Add option to skip cleaning files #3520

merged 2 commits into from
Mar 11, 2024

Conversation

pmoravec
Copy link
Contributor

A new option --skip-clean-files allows cleaner to skip cleaning files where the user is certain no sensitive information is present.

The option supports globs / wildcards.

Relevant: #3469
Closes: #3520

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@example.com?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@pmoravec
Copy link
Contributor Author

I need to add an avocado test before merging.

pmoravec added a commit to pmoravec/sos that referenced this pull request Feb 12, 2024
@pmoravec
[cleaner] Add option to skip cleaning files
c4a31ec 
A new option --skip-clean-files allows cleaner to skip cleaning files
where the user is certain no sensitive information is present.

The option supports globs / wildcards.

Relevant: sosreport#3469
Closes: sosreport#3520

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec force-pushed the sos-pmoravec-cleaner-skip-files-option branch from 1ccdcf8 to c4a31ec Compare February 12, 2024 07:34
@packit-as-a-service Packit-as-a-Service
Copy link

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-3520
  • And now you can install the packages.

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

pmoravec added a commit to pmoravec/sos that referenced this pull request Feb 12, 2024
@pmoravec
[cleaner] Add option to skip cleaning files
fdb4ec8 
A new option --skip-clean-files allows cleaner to skip cleaning files
where the user is certain no sensitive information is present.

The option supports globs / wildcards.

Relevant: sosreport#3469
Closes: sosreport#3520

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec force-pushed the sos-pmoravec-cleaner-skip-files-option branch from c4a31ec to fdb4ec8 Compare February 12, 2024 14:46
TurboTurtle
TurboTurtle reviewed Feb 12, 2024
View reviewed changes
Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, minor notes below.

man/en/sos-clean.1 Outdated
@@ -5,6 +5,7 @@ sos clean - Obfuscate sensitive data from one or more sosreports
.B sos clean TARGET [options]
[\-\-domains]
[\-\-disable-parsers]
[\-\-skip-clean-files]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps --skip-cleaning-files might be a bit clearer? We could also have an alias for --skip-masking-files for the downstreams that prefer mask.

sos/cleaner/__init__.py Outdated
clean_grp.add_argument('--skip-clean-files', action='extend',
default=[], dest='skip_clean_files',
help=('List of files to skip/ignore during '
'cleaning. Asterisks are supported.'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/Asterisks/Globs in my opinion, as it is more precise.

@pmoravec
[cleaner] Add option to skip cleaning files
50d90d9 
A new option --skip-cleaning-files / --skip-masking-files allows cleaner
to skip cleaning files where the user is certain no sensitive information
is present.

The option supports globs / wildcards.

Relevant: sosreport#3469
Closes: sosreport#3520

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec force-pushed the sos-pmoravec-cleaner-skip-files-option branch from fdb4ec8 to 50d90d9 Compare March 11, 2024 11:32
jcastill
jcastill approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@jcastill jcastill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@pmoravec
Copy link
Contributor Author

pmoravec commented Mar 11, 2024 •
edited
Loading

I was about to write a test for this when I discovered a problem: we can't easily tell cleaner not to obfuscate symbolic links.

Check:

sos report --clean -o filesys --keywords=sysfs --batch --build --skip-cleaning-files etc/mtab

Then, https://github.com/sosreport/sos/blob/main/sos/cleaner/__init__.py#L758 causes we skip the decision "shall we obfuscate the file or not?" for a symlink.

We can't easily add "else:" branch as the target file of the symlink can be already obfuscated.

I was thinking about this solution: anywere before https://github.com/sosreport/sos/blob/main/sos/cleaner/__init__.py#L676 , traverse the archive.get_file_list() for symbolic links. If either symlink matches a parser's skip_patterns, add the symlink's os.path.realpath target to the parser's skip_patterns.

It is bit ugly and lengthy, but I dont see an easier way..

Anyway, that is an independent issue I can deal afterwards.

@pmoravec
[tests] Add tests for cleaner's --skip-cleaning-files
2b207e5 
Closes: sosreport#3469

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec mentioned this pull request Mar 11, 2024
Open
@pmoravec
Copy link
Contributor Author

Avocado test added, symlinks issue raised as #3563 .

@TurboTurtle TurboTurtle merged commit cbecc16 into sosreport:main Mar 11, 2024
39 checks passed
TurboTurtle pushed a commit that referenced this pull request Mar 11, 2024
@pmoravec @TurboTurtle
[cleaner] Add option to skip cleaning files
ecda520 
A new option --skip-cleaning-files / --skip-masking-files allows cleaner
to skip cleaning files where the user is certain no sensitive information
is present.

The option supports globs / wildcards.

Relevant: #3469
Closes: #3520

Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
@pmoravec pmoravec deleted the sos-pmoravec-cleaner-skip-files-option branch July 17, 2024 14:34
@pmoravec pmoravec mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TurboTurtle TurboTurtle TurboTurtle approved these changes

@jcastill jcastill jcastill approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pmoravec @TurboTurtle @jcastill