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

[bitnami/postgresql-repmgr] use passfile on primary_conninfo #73542

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

Conversation

yukha-dw
Copy link
Contributor

@yukha-dw yukha-dw commented Oct 23, 2024

Description of the change

Currently, postgresql.conf stores literal password on primary_conninfo, this could be a security issue

Benefits

Let user to hide their password on primary_conninfo on postgresql.conf (often visible to anyone, 644) using passfile

Possible drawbacks

I haven't checked much about POSTGRESQL_REPLICATION_ envs, POSTGRESQL_REPLICATION_USE_PASSFILE isn't used at all and I don't know its background

Applicable issues

Additional information

…sible

Signed-off-by: Yukha Dharmeswara <yukha.dw@samsung.com>
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Oct 24, 2024
@github-actions github-actions bot removed the triage Triage is needed label Oct 24, 2024
@github-actions github-actions bot removed the request for review from javsalgar October 24, 2024 10:05
@github-actions github-actions bot requested a review from fmulero October 24, 2024 10:05
Copy link
Collaborator

@fmulero fmulero left a comment

Choose a reason for hiding this comment

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

Thanks a lot @yukha-dw for your contribution!

I completely agree with your approach, this is a very interesting feature. I have some concerns that I'd like to discuss or suggest:

  • File associated to POSTGRESQL_REPLICATION_PASSFILE_PATH seems not created by default, Should users provide it?
  • I see an overlapping with REPMGR_USE_PASSFILE and REPMGR_PASSFILE_PATH env variables in lib. Not sure how they work together, have you seen this?
  • Could you add any documentation in the README file about the behaviour/use of the env variables?
  • Could you provide the steps you followed to test the solution?

@yukha-dw
Copy link
Contributor Author

From what I understand, POSTGRESQL_REPLICATION_* variables should be identic with REPMGR_*, except for POSTGRESQL_REPLICATION_PASSFILE_PATH which somehow we can overwrite even though we are using same username stated in REPMGR_USERNAME. This could be a remnant of short circuiting the configuration (also see POSTGRESQL_REPLICATION_USE_PASSFILE, it doesn't being used anywhere)

# PostgreSQL env var (Replication Manager)
export PGCONNECT_TIMEOUT="${PGCONNECT_TIMEOUT:-10}"
export POSTGRESQL_REPLICATION_USER="$REPMGR_USERNAME"
export POSTGRESQL_REPLICATION_PASSWORD="$REPMGR_PASSWORD"
export POSTGRESQL_REPLICATION_USE_PASSFILE="$REPMGR_USE_PASSFILE"
export POSTGRESQL_REPLICATION_PASSFILE_PATH="${POSTGRESQL_REPLICATION_PASSFILE_PATH:-$REPMGR_PASSFILE_PATH}"
export POSTGRESQL_MASTER_HOST="$REPMGR_PRIMARY_HOST"
export POSTGRESQL_MASTER_PORT_NUMBER="$REPMGR_PRIMARY_PORT"

File associated to POSTGRESQL_REPLICATION_PASSFILE_PATH seems not created by default, Should users provide it?

The file associated to POSTGRESQL_REPLICATION_PASSFILE_PATH must contain same content with REPMGR_PASSFILE_PATH. It's better to leave it alone. By default, when REPMGR_USE_PASSFILE is set to true, this container will create the required passfile when user hasn't provide one.

if [[ "$REPMGR_USE_PASSFILE" = "true" ]] && [[ ! -f "${REPMGR_PASSFILE_PATH}" ]]; then
echo "*:*:*:${REPMGR_USERNAME}:${REPMGR_PASSWORD}" >"${REPMGR_PASSFILE_PATH}"
chmod 600 "${REPMGR_PASSFILE_PATH}"
fi

I see an overlapping with REPMGR_USE_PASSFILE and REPMGR_PASSFILE_PATH env variables in lib. Not sure how they work together, have you seen this?

REPMGR_USE_PASSFILE is a boolean to determine if user want to use passfile or plain text password, while REPMGR_PASSFILE_PATH is the path of the passfile. User can provide its own passfile or let the container create one from them.

if [[ "$REPMGR_USE_PASSFILE" = "true" ]] && [[ ! -f "${REPMGR_PASSFILE_PATH}" ]]; then
echo "*:*:*:${REPMGR_USERNAME}:${REPMGR_PASSWORD}" >"${REPMGR_PASSFILE_PATH}"
chmod 600 "${REPMGR_PASSFILE_PATH}"
fi

Could you add any documentation in the README file about the behaviour/use of the env variables?

I am not sure how to do this without introducing any breaking change. Current README is quite coupled with bitnami/charts. My apology.

Could you provide the steps you followed to test the solution?

I simply test this by altering REPMGR_USE_PASSFILE value.

Before change,
REPMGR_USE_PASSFILE=false -> repmgr use PGPASSWORD, postgresql_configure_recovery use PGPASSWORD
REPMGR_USE_PASSFILE=true -> repmgr use PGPASSFILE, postgresql_configure_recovery use PGPASSWORD

After change,
REPMGR_USE_PASSFILE=false -> repmgr use PGPASSWORD, postgresql_configure_recovery use PGPASSWORD
REPMGR_USE_PASSFILE=true -> repmgr use PGPASSFILE, postgresql_configure_recovery use PGPASSFILE

…sible

Signed-off-by: Yukha Dharmeswara <yukha.dw@samsung.com>
@yukha-dw yukha-dw force-pushed the fix/postgresql-repmgr/primary-conninfo-passfile branch from f0ab664 to b35bd30 Compare October 29, 2024 07:11
@yukha-dw
Copy link
Contributor Author

I've added few changes here b35bd30:

  1. Update old postgresql versions (12, 13, 14, & 15)
  2. Misconfig prevention - Prevent user from overwriting POSTGRESQL_REPLICATION_PASSFILE_PATH. POSTGRESQL_REPLICATION_PASSFILE_PATH always will be alias of REPMGR_PASSFILE_PATH
  3. Update README - Move related variables to read-only section (as it is intended to)
  4. Update README - Add this notice:

NOTE: repmgr user is also used as PostgreSQL replication user.

@fmulero
Copy link
Collaborator

fmulero commented Oct 30, 2024

Thanks a lot @yukha-dw for such detailed explanation, I really appreciate it!

Your code is correct for postgresql-repmgr but I've been testing the solution in our environments and I've seen some problems in the postgresql container (some parts of the code are common between both containers) because in case of replication the slave postgresql container will try to use the passfile (POSTGRESQL_REPLICATION_PASSFILE_PATH) and it doesn't exist. To don't bother you with our dependencies I've opened an internal task to find a solution. I'll keep you posted on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress postgresql-repmgr verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants