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

[plugins] Obfuscate proxy credentials in env. settings #3789

Closed
wants to merge 1 commit into from
Closed

[plugins] Obfuscate proxy credentials in env. settings #3789

wants to merge 1 commit into from

Conversation

pmoravec
Copy link
Contributor

Obfuscate credentials in env.variable settings for e.g. HTTP_PROXY or similar.

Inspired by: #3788
Resolves: #3789

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?

@pmoravec
Copy link
Contributor Author

Trivial reproducer:

echo HTTP_PROXY=http://foouser:barpassword@proxyurl:8080 >> /etc/environment
sos report -o system --batch --build

I am not sure if whole /etc/sysconfig or /etc/default needs such scrubbing - various services can define the proxy there so it is imho better to cover all. BUT if either directory is password-less every time, I am happy to remove it from the postproc.

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

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

@pafernanr pafernanr mentioned this pull request Sep 25, 2024
Closed
@pmoravec
Copy link
Contributor Author

OK, the /etc/sysconfig or /etc/default could also collect the env.variables, worth processing them as well.

pmoravec added a commit to pmoravec/sos that referenced this pull request Sep 26, 2024
@pmoravec
[plugins] Obfuscate *_PROXY credentials
100071d 
HTTP_PROXY or similar env.variables can contain credentials we must
scrub. The variables or directly credentials can be specified in a few
places the commit deals with.

Resolves: sosreport#3789

Signed-off-by: Pavel Moravec <[email protected]>
@pmoravec pmoravec force-pushed the sos-pmoravec-proxy-setting-obfuscate-credentials branch from ee3acd2 to 100071d Compare September 26, 2024 17:21
pmoravec
pmoravec commented Sep 26, 2024
View reviewed changes
sos/report/plugins/anaconda.py
"/var/log/anaconda.*",
"/root/install.log",
"/root/install.log.syslog"
]

self.add_copy_spec(paths)
self.add_copy_spec(self.copypaths)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The credentials can be in either anaconda log or in the *ks.cfg file - hence apply postproc to all copyspecs.

@jcastill
Copy link
Member

The pylint failure is solved already but checking out this pr doesn't show the commit, so I think you'll have to resync.

arif-ali
arif-ali approved these changes Sep 26, 2024
View reviewed changes
Copy link
Member

@arif-ali arif-ali left a comment

Choose a reason for hiding this comment

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

looks good to me, it would be good to do the same in apt.py as well, but if you prefer, I can do a seperate PR for that later (as that is already obfuscating proxy)

pmoravec added a commit to pmoravec/sos that referenced this pull request Sep 26, 2024
@pmoravec
[plugins] Obfuscate *_PROXY credentials
824cc48 
HTTP_PROXY or similar env.variables can contain credentials we must
scrub. The variables or directly credentials can be specified in a few
places the commit deals with.

Futher, update apt plugin to use the new do_paths_httpproxy_sub method.

Resolves: sosreport#3789

Signed-off-by: Pavel Moravec <[email protected]>
@pmoravec pmoravec force-pushed the sos-pmoravec-proxy-setting-obfuscate-credentials branch from 100071d to 824cc48 Compare September 26, 2024 21:11
@pmoravec pmoravec changed the title [system] Obfuscate proxy credentials in env. settings [plugins] Obfuscate proxy credentials in env. settings Sep 26, 2024
@pmoravec
Copy link
Contributor Author

Good point wrt Apt plugin where I in fact copied the RE from - I just updated the PR accordingly.

TurboTurtle
TurboTurtle approved these changes Sep 26, 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.

This LGTM, however I should note that what this does and what it says it does are slightly different.

The obfuscation isn't actually looking for *_PROXY variables, just urls with username and passwords in them, which so happen to be the convention for proxy authentication.

I have a slight preference to rename to do_path_http_sub() or something similar, but it is not a big item for me and I'm fine if you want to merge as is.

@pmoravec
[plugins] Obfuscate http URL credentials
7eb20e7 
HTTP_PROXY or similar env.variables can contain credentials we must
scrub. The variables or directly credentials of a http(s) URL can be
specified in several places the commit deals with.

Futher, update apt plugin to use the new do_paths_httpp_sub method.

Resolves: sosreport#3789

Signed-off-by: Pavel Moravec <[email protected]>
@pmoravec pmoravec force-pushed the sos-pmoravec-proxy-setting-obfuscate-credentials branch from 824cc48 to 7eb20e7 Compare September 27, 2024 06:01
@pmoravec pmoravec closed this in 60356d6 Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arif-ali arif-ali arif-ali approved these changes

@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.
4 participants
@pmoravec @jcastill @arif-ali @TurboTurtle