Opened 7 months ago

Closed 6 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 7 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 6 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.

Thanks for contributing to Twisted!

Version 0, edited 6 months ago by Tom Most (next)

comment:3 Changed 6 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.