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

[foreman] Obfuscate http_proxy passwords. PR-3878 improvement #3881

Merged
merged 1 commit into from
Dec 13, 2024
Merged

[foreman] Obfuscate http_proxy passwords. PR-3878 improvement #3881

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions sos/report/plugins/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1317,9 +1317,9 @@ def do_file_sub(self, srcpath, regexp, subst):
return replacements

def do_paths_http_sub(self, pathspecs):
""" Obfuscate credentials in *_PROXY variables in all files in the
given list. Proxy setting without protocol is ignored, since that
is not recommended setting and obfuscating that one can hit false
""" Obfuscate Basic_AUTH URL credentials in all files in the given
list. Proxy setting without protocol is ignored, since that is
not recommended setting and obfuscating that one can hit false
positives.

:param pathspecs: A filepath to obfuscate credentials in
Expand All @@ -1329,7 +1329,7 @@ def do_paths_http_sub(self, pathspecs):
pathspecs = [pathspecs]
for path in pathspecs:
self.do_path_regex_sub(
path, r"(http(s)?://)\S+:\S+(@.*)", r"\1******:******\3")
path, r"http(s)?://\S+:\S+@", r"http\1://******:******@")

def do_path_regex_sub(self, pathexp, regexp, subst):
"""Apply a regexp substituation to a set of files archived by
Expand Down
8 changes: 0 additions & 8 deletions sos/report/plugins/foreman.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,14 +331,6 @@ def postproc(self):
self.do_paths_http_sub([
'/var/log/foreman/production.log*',
])
Copy link
Member

Choose a reason for hiding this comment

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

Is is really necessary to remove this function? This obfuscates *_PROXY vars, and the change below doesn't do that, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do_paths_http_sub function replaces only one occurrence of r"(http(s)?://)\S+:\S+(@.*)", r"\1******:******\3").

In the foreman case there could be one or two occurrences on the same line. That's the reason to remove that function and improve the regex on foreman.py to capture one or multiple occurrences.

*_PROXY vars are being obfuscated at https://github.com/sosreport/sos/blob/main/sos/report/plugins/foreman.py#L299

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm. I think the description for do_paths_http_sub is not accurate:

  • Obfuscate credentials in *_PROXY variables...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the description is a historic artifact from some first PR draft and should be updated.

Copy link
Member

Choose a reason for hiding this comment

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

@pmoravec can you help with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, the do_paths_http_sub can match just one instance on a given line - that is why my #3878 already obfuscates the logfile(s) twice. Which is ridiculous and this PR aims to improve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you agree, we might modify it to something like "Obfuscate BASIC_AUTH credentials in URLs for all files in the...".
But again that function suffer the same issue. Only one match is being replaced /o\

Copy link
Contributor

Choose a reason for hiding this comment

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

I created #3882 for that. Usually, there is just one password on a line, so usually the method is sufficient, I think.

Or are there instances where we need to obfuscate a secret on the same line multiple times? This is generic question, not specific to the do_paths_http_sub method. As the method just calls something internal that is used everywhere, incl. by do_path_regex_sub method.

(i.e. should we utilize repeatedly option of do_path_regex_sub?)

# .. even those appearing TWICE in the logfile, in format (one-line):
# Setting (7) update event on value --- https://USER:PASS@foobar:443,\
# --- https://USER:PASS@foobar:3128
self.do_path_regex_sub(
'/var/log/foreman/production.log*',
r", --- (http(s)?://)\S+:\S+(@.*)",
r"\1******:******\3"
)
# hide proxy credentials from http_proxy setting
self.do_cmd_output_sub(
"from settings where",
Expand Down
Loading