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

Add roleEnableLogsAttr to prevent password leakage in logs #231

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MatthewPugliese
Copy link

@MatthewPugliese MatthewPugliese commented Jul 26, 2022

Currently, when a new role is created, the logs output the password
2022-07-25 18:53:15.069 UTC [49] LOG: statement: CREATE ROLE "my_role" WITH ENCRYPTED PASSWORD 'mypass' VALID UNTIL 'infinity' CONNECTION LIMIT -1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOBYPASSRLS NOREPLICATION

This new attribute, enable_logs, allows the user to determine whether or not they want the logs of any roles being created to be included or skipped, as shown below.

2022-07-25 18:55:13.616 UTC [51] LOG:  statement: CREATE ROLE "role_with_logs" WITH ENCRYPTED PASSWORD 'mypass' VALID UNTIL 'infinity' CONNECTION LIMIT -1 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOBYPASSRLS NOREPLICATION

2022-07-25 18:55:13.632 UTC [53] DETAIL:  parameters: $1 = 'role_with_logs'

2022-07-25 18:55:13.645 UTC [56] DETAIL:  parameters: $1 = 'role_with_no_logs'

The attribute is defaulted to false for improved security. For those who want to enable these logs, when defining a new role, include enable_logs = true besides the other included attributes.

resource "postgresql_role" "role_with_logs" {
    name         = "role_with_logs"
    login        = true
    password     = "mypass"
    enable_logs  = true
}

@MatthewPugliese
Copy link
Author

MatthewPugliese commented Jul 26, 2022

@cyrilgdn I have also tested all the above below automated tests on a local repository and everything passes.

@MatthewPugliese
Copy link
Author

@cyrilgdn Any updates on when you can take a look at this? Thanks

@MatthewPugliese
Copy link
Author

@cyrilgdn Hopefully we are all set to merge since all the checks passed :)

@BennettKnapek
Copy link

@cyrilgdn Are there any updates on this? I'm also being affected by the log leaks.

@omarabouelnour
Copy link

omarabouelnour commented Sep 1, 2022

@cyrilgdn This password leakage is dangerous, and should be controlled. Can you please take a look at this ASAP?

@pfnsec
Copy link

pfnsec commented Sep 12, 2022

We're seeing this behaviour as well - would be great to have this fixed.

@MatthewPugliese
Copy link
Author

Hey @cyrilgdn have you had a chance to review this?

1 similar comment
@MatthewPugliese
Copy link
Author

Hey @cyrilgdn have you had a chance to review this?

@MatthewPugliese MatthewPugliese force-pushed the add/enable-logs-attribute branch from 9de07a9 to cd56239 Compare October 11, 2023 13:21
@MatthewPugliese MatthewPugliese force-pushed the add/enable-logs-attribute branch from cd56239 to 451de68 Compare March 14, 2024 16:57
@MatthewPugliese
Copy link
Author

@cyrilgdn have you had a chance to review this yet?

Copy link
Owner

@cyrilgdn cyrilgdn left a comment

Choose a reason for hiding this comment

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

Hi @MatthewPugliese ,

Sorry for the lack of response over the years.

Apart the comment I posted on the code, I have a concern on the fact that when you disabled the logs, unless the transaction is rollback, the logs will be disabled for the life of the connection so potentially for other resources during the same apply.

Is it expected for you? I wonder if people would not complain later about this unexpected behavior.

I wonder if it should not be a provider settings instead 🤔

Let me know what do you think.

Comment on lines +296 to +301
areLogsEnabled := d.Get(roleEnableLogsAttr).(bool)
if areLogsEnabled {
sql := "SET log_statement TO 'none'; SET log_min_duration_statement TO -1; SET log_min_error_statement TO 'log'; SET pg_stat_statements.track_utility = 'off';"

if _, err := txn.Exec(sql); err != nil {
return fmt.Errorf("could not disable logs for %s: %w", roleName, err)
Copy link
Owner

Choose a reason for hiding this comment

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

I feel like you inverse the boolean no?

if areLogsEnabled is set to true, you disable the logs

@cyrilgdn
Copy link
Owner

I wonder if it should not be a provider settings instead 🤔

Another solution would be to get the current values of all settings you are changing and and reset to these values just after the create role actually.

@cyrilgdn cyrilgdn added the waiting-response Further information is requested label Sep 8, 2024
@cyrilgdn cyrilgdn force-pushed the main branch 3 times, most recently from f2c2e47 to dea1401 Compare September 8, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-response Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants