Opened 4 months ago

Closed 3 months ago

#9544 defect closed fixed (fixed)

adbapi.ConnectionPool.connect logs password

Reported by: Nolan Prescott Owned by: Tom Most
Priority: normal Milestone:
Component: core Keywords:
Cc: Nolan Prescott Branch:
Author: 9544-adbapi-logging

Description

When noisy is set to True, connections are logged with connection arguments and keyword arguments, resulting in a log line similar to:

2018-10-02T17:08:02+0000 [-] adbapi connecting: psycopg2 {'database': 'database-name', 'host': 'database-host', 'user': 'my-username', 'password': 'leaked-password', 'port': ''}

This stems from adbapi.py:426 --

            if self.noisy:
                log.msg('adbapi connecting: %s %s%s' % (self.dbapiName,
                                                        self.connargs or '',
                                                        self.connkw or ''))

This was touched on in ticket #1806, where the noisy default was changed as a security risk. This was also fixed/removed entirely from the _close method as part of #871.

Change History (3)

comment:1 Changed 4 months ago by Nolan Prescott

Cc: Nolan Prescott added
Keywords: review added

https://github.com/twisted/twisted/pull/1074

The proposed fix duplicates the approach for #871 - removing the connection arguments and keyword arguments from the log message entirely.

I've included a second commit in the PR to fix an issue with the linter around a comparison to None, please let me know if this is out of band as the line in question was unchanged as part of my "fix" for this ticket.

comment:2 Changed 3 months ago by Tom Most

Author: 9544-adbapi-logging
Keywords: review removed
Owner: set to Tom Most
Status: newassigned

I posted a review on GitHub: https://github.com/twisted/twisted/pull/1074#pullrequestreview-164541672 +1

If the None comparison change merited all the overhead of its own ticket/newsfile/review then I'd ask for that, but I think it is fine to make such a trivial stylistic change when working on adjacent code. The tests still pass, after all. This is in line with our general approach of "fix linting issues where you find them, and add tests where there were none."

Thanks for contributing to Twisted!

Last edited 3 months ago by Tom Most (previous) (diff)

comment:3 Changed 3 months ago by Tom Most <twm@…>

Resolution: fixed
Status: assignedclosed

In 5a6a9b3:

Merge pull request #1074 from NPrescott/9544-NPrescott-adbapi-logging

Author: NPrescott
Reviewer: twm
Fixes: ticket:9544

Adjust adapi logging to avoid logging credentials.

Note: See TracTickets for help on using tickets.