PR#3203: Hide sensitive values in urrlib3 exceptions - koji

koji

#3203 Hide sensitive values in urrlib3 exceptions

Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.

tkopecek/koji issue2801 into master

Hide sensitive values in urrlib3 exceptions

Tomas Kopecek • 2 years ago

06b5f7c

koji/__init__.py

file modified

+10

		`@@ -65,6 +65,7 @@`
		`import six.moves.urllib`
		`from requests.adapters import HTTPAdapter`
		`from requests.packages.urllib3.util.retry import Retry`
		`+ from requests.packages.urllib3.exceptions import MaxRetryError`
		`from six.moves import range, zip`

		`from koji.tasks import parse_task_params`
		`@@ -2713,6 +2714,15 @@`
		`try:`
		`return self._sendOneCall(handler, headers, request)`
		`except Exception as e:`
		`+ if isinstance(e, requests.exceptions.ConnectionError):`
		`+ # mask sensitive values, ConnectionError could contain underlying`
		`+ # urllib3 exception which displays full URL with session-id, etc.`
		`+ if e.args and isinstance(e.args[0], MaxRetryError):`
		`+ mre = e.args[0]`
		`+ s = mre.args[0]`
		`+ s = re.sub(r'session-id=\d+', 'session-id=XXXXX', s)`
		`+ s = re.sub(r'session-key=[^&]+', 'session-key=XXXXX', s)`
		`+ mre.args = (s, mre.args[1:])`
		`if i or not is_conn_error(e):`
		`raise`
		`self.logger.debug("Connection Error: %s", e)`

tkopecek commented 2 years ago

Fixes: https://pagure.io/koji/issue/2801

ktdreyer commented on line 13 of koji/__init__.py 2 years ago

Let's move this out of the if ... not is_conn_error(e): condition, so we sanitize what goes into the raise statement and the self.logger.debug() statement.

tkopecek commented 2 years ago

Let's move this out of the if ... not is_conn_error(e): condition, so we sanitize what goes into the raise statement and the self.logger.debug() statement.

Ah, you're right. My thought was that we (maybe) want these data in server's log (e.g. for kojid). But it is sadly logged also in client, so it is less harm to sanitize it everywhere.

rebased onto 8b0a0bb92ad0482695e432f3c7b50fd890742d43

2 years ago

tkopecek commented 2 years ago

What I don't like about this is that it is extremely fragile. It solves only one case when urllib3 exception is directly part of the requests exception. I'm not sure if there are no other possible variants (more nested exceptions, etc.). Theoretically I can do separate sanitize function which will iterate over all args and try to recursively check all of them whose inherit from Exception.

ktdreyer commented 2 years ago

Agreed, I think we should write a general method to sanitize this from all .args... with unit tests to demonstrate it's working

tkopecek commented 2 years ago

I'll merge this one for current release with the followup issue #3246

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @tkopecek:
- Pull-request untagged with: testing-ready

2 years ago

rebased onto 06b5f7c

2 years ago

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago