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

fix(inputs.postgres*)!: Prevent leaking sensitive data in server tag #14829

Merged
merged 11 commits into from
Mar 1, 2024

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Feb 15, 2024

Summary

The current implementation of SanitizedAddress() does not handle quoted parameter values containing spaces or equal signs (e.g. host=localhost user=john password='a secret=good'). Additionally to those issues, the parsing of URI connection strings is completely broken for spaces and equal signs as values containing those need to be single-quoted which does not happen. Furthermore, that function escapes "special characters" (space, backslash and single-quote) which subsequently breaks the sanitize function.

This PR adds unit-tests for both of the mentioned functions. It also removes the useless escaping of spaces during URI handling and replaces this part by quoting the value. Finally, the PR improves the regular-expression used to remove sensitive data to correctly handle quoted values, equal-signs and other corner cases.

Checklist

  • No AI generated code was used in this PR

Related issues

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 15, 2024
@srebhan srebhan added area/postgresql security raise security concerns or improve the security of Telegraf labels Feb 15, 2024
@james-johnston-thumbtack

I am the person who originally reported this issue. I didn't look closely at the updated implementation, but glancing through the new test cases, it makes sense to me and seems to be working.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 15, 2024
@srebhan srebhan requested a review from jdstrand February 15, 2024 20:33
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

I am of course weary when we make these larger changes to fix an issue. Can you please add a note the change log about this change as it is breaking tags for some small percentage of users.

I would also like to land this in 1.30 and not 1.29.5. Agreed?

@srebhan srebhan changed the title fix(inputs.postgres*): Prevent leaking sensitive data in server tag fix!(inputs.postgres*): Prevent leaking sensitive data in server tag Feb 20, 2024
@srebhan srebhan changed the title fix!(inputs.postgres*): Prevent leaking sensitive data in server tag fix(inputs.postgres*)!: Prevent leaking sensitive data in server tag Feb 21, 2024
@powersj powersj removed their assignment Feb 22, 2024
@DStrand1 DStrand1 assigned srebhan and unassigned DStrand1 Feb 22, 2024
Copy link
Contributor

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

I think the PR looks fine overall. I didn't desk check the sanitizer but the existing tests show it is working better. Marking as 'request changes' since I'd like to see even more tests for this (there is an opportunity to refactor to make that better as well as a question on zero-ing out addr).

plugins/inputs/postgresql/service.go Show resolved Hide resolved
plugins/inputs/postgresql/postgresql_test.go Show resolved Hide resolved
@srebhan
Copy link
Member Author

srebhan commented Feb 28, 2024

@jdstrand I hopefully addressed your comments. If I got things wrong, please let me know!

@srebhan srebhan requested a review from jdstrand February 28, 2024 21:04
Copy link
Contributor

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! I have one separate comment about reverting 6f322d3 if you think it will cause a problem. Approving as is and feel free to commit if you revert 6f322d3.

@srebhan
Copy link
Member Author

srebhan commented Mar 1, 2024

@jdstrand I don't think using the temporary string is a problem, so let's try as-is.

@srebhan srebhan merged commit e1994a6 into influxdata:master Mar 1, 2024
26 checks passed
@srebhan srebhan deleted the postgres_sanitize branch March 1, 2024 08:38
@github-actions github-actions bot added this to the v1.30.0 milestone Mar 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/postgresql fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. security raise security concerns or improve the security of Telegraf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants