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.

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

#780: do not log any connection string above DEBUG level #781

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mulder999
Copy link

#780: do not log connection string above DEBUG level

Remark

  • This is a quick fix to solve vulnerability issue
  • Further enhancement should attempt to sanitize information passed to unable to connect and unable to ping messages in order to still give valuable hints to admin

* Do not log connection string above DEBUG level
@gonzalemario
Copy link
Contributor

@mulder999 do you still need this? Despite it might pass any requirements you might have by removing the conninfo entirely, I don't think we should remove everything. When you've got multiple node and just one it's failing, knowing in the log which one it is, might be really helpful.

I was checking how psql handles this in libpq:

psql "host=localhost user=barman password=PASSWORD"
psql: error: connection to server at "localhost" (127.0.0.1), port 5432 failed: FATAL:  password authentication failed for user "barman"
connection to server at "localhost" (127.0.0.1), port 5432 failed: FATAL:  password authentication failed for user "barman"

@martinmarques
Copy link
Collaborator

The solution here is to anonymize the secrets instead of removing the whole conninfo string

@mulder999
Copy link
Author

Thank you for getting back to me. As I mentioned, this was intended to be a quick fix for a high risk security issue. Masking the password is perfectly acceptable and precisely the enhancement I was suggesting.

@gonzalemario
Copy link
Contributor

To be honest, I personally don't think a quick fix would go into upstream. We'll discuss idea how we can mask the password but as long as you don't update the patch to provide this masking or similar, we might need to close this PR.

@mulder999
Copy link
Author

Thank you for the feedback. I understand the priority for a robust, upstream-worthy solution, especially for something as sensitive as password masking. My concern is that this security risk has been open for almost two years, and I worry that delaying further could continue to expose some users to potential vulnerabilities.

While I’m unable to update the patch right now, I believe implementing a temporary solution might mitigate the risk until a more comprehensive fix can be developed. I’d be happy to help with any discussions around this if it’s helpful.

@martinmarques
Copy link
Collaborator

I think we should close this PR and open an issue for development on password masking. We are planning on releasing PG17 support, plus some low hanging fruit, and this work would delay that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants