Skip to content

Conversation

@jchancojr
Copy link
Contributor

What kind of change does this PR introduce?

Feature- pgBackRest backup solution

What is the current behavior?

pgBackRest tasks and configs are absent

What is the new behavior?

pgBackRest tasks and configs are missing are present

Additional context

NA

@jchancojr jchancojr self-assigned this Oct 29, 2025
@jchancojr jchancojr added the enhancement New feature or request label Oct 29, 2025
- name: Sticky bit the pgBackRest binary
file:
path: /var/lib/pgbackrest/.nix-profile/bin/pgbackrest
mode: '4755'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Setting setuid won't work for multiple reasons:

  • /var/lib/pgbackrest/.nix-profile/bin/pgbackrest is a symlink to a binary in the store, and the kernel ignores setuid bits on symlinks.
  • We cannot modify the binary file in the Nix store as it is mounted with ro
  • Binaries with setuid in the Nix store won't work as it is mounted with nosuid

I see two possible paths:

  • Use sudo/doas
  • Create a wrapper in /usr/local/bin (or /usr/bin) that execs the original pgbackrest in the store and set the setuid bit on that wrapper

I find the sudo approach safer and more maintainable than using a setuid wrapper.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something like this could work

- name: Configure sudoers for pgBackRest
  lineinfile:
    path: /etc/sudoers.d/pgbackrest
    line: "postgres ALL=(pgbackrest) NOPASSWD: /var/lib/pgbackrest/.nix-profile/bin/pgbackrest"
    create: yes
    mode: '0440'
    validate: 'visudo -cf %s'

- name: Create pgBackRest wrapper script
  copy:
    content: |
      #!/bin/bash
      exec sudo -u pgbackrest /var/lib/pgbackrest/.nix-profile/bin/pgbackrest "$@"
    dest: /usr/bin/pgbackrest
    mode: '0755'
    owner: root
    group: root

if not root/root could be some user/group such that pgbackrest cannot change permissions of it's own script

Copy link
Contributor

Choose a reason for hiding this comment

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

added

Copy link
Collaborator

Choose a reason for hiding this comment

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

@staaldraad might want to take a peek at this just to make sure you're happy with this kind of approach too when you have a moment

Copy link
Member

Choose a reason for hiding this comment

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

there are a few arguments that look a little scary

  • --cmd
  • --ssh-cmd
  • --repo-host-cmd

The --config variants may also allow access to the above arguments.

The wrapper would feel safer, although less flexible, if we had fixed arguments per command in the wrapper, and then allow sudo on the wrapper.

Something like:

#!/bin/bash

if [[ "$1" == "archive-get" ]]; then
  exec /var/lib/pgbackrest/.nix-profile/bin/pgbackrest archive-get --stanza="$2"
else
  echo "Error: not allowed"
  exit 1
fi
- name: Configure sudoers for pgBackRest
  lineinfile:
    path: /etc/sudoers.d/pgbackrest
    line: "postgres ALL=(pgbackrest) NOPASSWD:  /usr/bin/pgbackrest"
    create: yes
    mode: '0440'
    validate: 'visudo -cf %s'

Copy link
Contributor

Choose a reason for hiding this comment

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

the backrest binary can only be called if you're already on the box, or by changing the archive_command in the postgresql.conf, which we disallow already. so it kinda feels like by the time someone could exploit those args, we'd already have bigger problems, no? @staaldraad

Copy link
Member

Choose a reason for hiding this comment

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

yes, in the ideal world. And that is what #1879 brings us closer to. Up until now, there was still avenue for superuser --> COPY (select 1) FROM PROGRAM '.../pgbackrest' (or first getting a more powerful shell and then having "local" access).

Fully agree that we are protecting against "bigger problems", however it is good to think of layered defense. As we start narrowing how big a problem local access is (if the likeliest route is to get shell as the unprivileged postgres user). With things like https://github.com/supabase/postgres/pull/1846/files , we really squeeze down what is possible to do, even if someone manages to change archive_command or get shell via postgres. Definitely trying to think in chains of small bugs here, in isolation one bug or protection might not be much, but chained together small bugs can be powerful. And the same goes for small protections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. This makes sense. Thanks very much will get this going

* origin/PSQL-773:
  fix(setup-pgbackrest.yml): errant indentation fix
  fix(setup-pgbackrest.yml): fix file module
  fix(setup-pgbackrest.yml): update nix install path
  feat(ansible): add pgBackRest tasks and configurations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants