Skip to content

Using HeadBucket for backups is dangerous in multi-tenant S3 buckets#1717

Open
DimitriosLisenko wants to merge 1 commit into
percona:trunkfrom
DimitriosLisenko:no-head-bucket
Open

Using HeadBucket for backups is dangerous in multi-tenant S3 buckets#1717
DimitriosLisenko wants to merge 1 commit into
percona:trunkfrom
DimitriosLisenko:no-head-bucket

Conversation

@DimitriosLisenko

@DimitriosLisenko DimitriosLisenko commented Dec 19, 2025

Copy link
Copy Markdown

Proposal

This project uses HeadBucket to determine the signature version to use, as well as whether the bucket exists before creating backups. The problem with this is that in IAM, there is literally no way to let a user call HeadBucket while also preventing them from listing objects inside that bucket. This is because HeadBucket requires the s3:ListBucket permission.

There are zero air-tight IAM workarounds that would prevent this permission from also allowing users to list objects inside the bucket. Here are some examples:

  1. Grant s3:ListBucket conditionally on the "" prefix - does not allow HeadBucket because HeadBucket does not pass a prefix parameter, and so it is denied.
  2. Grant s3:ListBucket conditionally on a specific directory - same as above.
  3. Grant s3:ListBucket conditionally on "Null": {"s3:prefix": "true"} - this actually still allows listing all objects in the bucket, just not specifying a prefix.
  4. Grant s3:ListBucket conditionally on everything except */* - can still specify a prefix without a slash and list objects.

There has been extensive discussion around this issue as it relates to the CNPG plugin:
EnterpriseDB/barman#929
EnterpriseDB/barman#1101
hashicorp/terraform-provider-aws#17195

The initial suggestion was to use ListObjectsV2 with a prefix rather than HeadBucket - this works in terms of security, but it is more expensive in terms of AWS API calls, and so was rejected.

The final solution as it relates to CNPG was to completely remove cloud connectivity + bucket existence checks before running backups. I propose the same here, which would require to remove the signature checks and require that it is provider; or instead to use HeadObject, which can be scoped to a directory.

Use-Case

The use case is for multi-tenant S3 buckets. Using HeadBucket in this scenario is a non-starter because it would allow users to see backups of other users.

Anything else?

This is a requirement to resolve percona/percona-xtradb-cluster-operator#2251, because that operator calls HeadBucket in itself, as well as by virtue of calling xbcloud.

@it-percona-cla

it-percona-cla commented Dec 19, 2025 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@DimitriosLisenko DimitriosLisenko mentioned this pull request Dec 19, 2025
Closed
@DimitriosLisenko DimitriosLisenko changed the title Don't use HeadBucket for pre-flight checks Using HeadBucket for backups is dangerous in multi-tenant S3 buckets Dec 19, 2025
@DimitriosLisenko DimitriosLisenko mentioned this pull request Jan 12, 2026
Closed
@DimitriosLisenko

DimitriosLisenko commented Jan 12, 2026

Copy link
Copy Markdown
Author

This PR is required for HeadBucket to be removed from the backup flow, and also for the operator to stop using HeadBucket. The operator work is currently at percona/percona-xtradb-cluster-operator#2326 - are the same people able to review this item as well?

@egegunes

egegunes commented Feb 5, 2026

Copy link
Copy Markdown

@satya-bodapati could you please take a look when you have the chance?

@DimitriosLisenko DimitriosLisenko mentioned this pull request Apr 24, 2026
Merged
@satya-bodapati

satya-bodapati commented May 21, 2026 •
edited by atlassian Bot
Loading

Copy link
Copy Markdown
Contributor

Hi @DimitriosLisenko,

I tested this against a few IAM shapes against MinIO before pulling it into our 8.4 backport. Full reproduction and tests are on PXB-3654.

The first one is the push-only uploader: a user with s3:ListBucket and s3:PutObject only. This works today. After the patch, the new HeadObject probe asks for s3:GetObject on bucket/<backup_name>, gets 403 AccessDenied, and xbcloud bails out with Probe failed before any chunk is uploaded.

The delete-only retention case is the same story. s3:ListBucket and s3:DeleteObject are enough today, xbcloud delete works. After the patch, no GetObject means the probe fails the same way.

The prefix-scoped tenant is more subtle. Their IAM grants Get/Put/Delete on bucket/tenant1/* only, and they happen to take backups named tenant1. The new probe sends HeadObject to bucket/tenant1, which does not match the IAM resource bucket/tenant1/* (the * requires at least one character after the slash), so the probe fails even though every actual chunk would land at bucket/tenant1/<chunk> and would succeed.

To be fair: the case your PR is built around, the multi-tenant tenant with prefix-conditioned s3:ListBucket, does start passing after the patch. The bug you set out to fix is real. It is just that the chosen fix trades one broken IAM pattern for three others.

I have a candidate fix in commit #4 of PXB-3654-xbcloud-pr1717-regression-tests that keeps your xbcloud_put change (the bucket pre-check removal, which is the part that actually unblocks the multi-tenant case end-to-end) but reverts the probe to HeadBucket and makes it tolerant of 403 AccessDenied. The reasoning is that the probe loop's only useful job is auto-detecting signature version and bucket lookup style; once the endpoint accepts our signature, the probe is done, and whether the caller has ListBucket should not be its business. With that change all four IAM shapes pass, including yours. The 403-tolerance bit is a heuristic and I am not 100% sure at the moment.

A couple of things I have not tested. GCS goes through the same S3_object_store code path so the fix should apply, but I did not point xbcloud at a real GCS bucket. Azure uses its own probe so neither your patch nor my other patch touches it. Your xbcloud_put change does affect Azure and Swift though, since the auto-create is in the common path; users on those backends who relied on xbcloud creating the container for them will need to pre-create it. Worth a changelog mention if it is not in there already.

satya-bodapati added a commit to satya-bodapati/percona-xtrabackup that referenced this pull request May 21, 2026
@satya-bodapati
PXB-3654 : xbcloud: multi-tenant motivating-case test for PR percona#…
93321b4 
…1717 https://perconadev.atlassian.net/browse/PXB-3654

Adds permission_multi_tenant.sh (R4), which exercises the exact scenario
PR percona#1717 ("Using HeadBucket for backups is dangerous in multi-tenant S3
buckets") was filed to enable: a tenant of a shared S3 bucket whose IAM
policy is

  s3:ListBucket  on bucket
    Condition: StringLike s3:prefix = tenant1/*
  s3:GetObject / s3:PutObject / s3:DeleteObject on bucket/tenant1/*

i.e. prefix-conditioned ListBucket only (so HeadBucket is denied because
it carries no s3:prefix in the request) plus full object permissions
inside the tenant's own prefix.

backup_name is chosen to live inside the tenant's prefix
(tenant1/backup_<ts>_<pid>) so all the runtime object keys match the
tenant's IAM Resource and Condition.

R4 is intentionally complementary to R1 / R1b / R2 in the previous
commit:

  R1, R1b, R2  user shapes that currently work and PR percona#1717 breaks
               (regressions caused by replacing HeadBucket with
               HeadObject on bucket/<backup_name>)

  R4           user shape that currently does NOT work and PR percona#1717
               aims to enable (the motivating use case)

Expected results at each step of the branch sequence:

  rel-8406 HEAD (no fix)         R1 R1b R2 pass, R4 fails  (today's state)
  rel-8406 + PR percona#1717            R1 R1b R2 fail, R4 passes (the trade-off)
  rel-8406 + PR percona#1717 + our delta  all four pass            (full coverage)

The two-commit "test-only baseline" (R1/R1b/R2 then R4) is in place
before any code change, so reviewers can apply PR percona#1717 verbatim and
observe the trade-off via test results, then apply our delta and
observe full coverage.
satya-bodapati added a commit to satya-bodapati/percona-xtrabackup that referenced this pull request May 21, 2026
@satya-bodapati
PXB-3654 : xbcloud: permission-tolerant probe on top of PR percona#1717
4eee0bc 
https://perconadev.atlassian.net/browse/PXB-3654

PR percona#1717 ("Don't use HeadBucket for pre-flight checks, instead use
HeadObject") fixes the multi-tenant bucket case (R4) but introduces
regressions for three currently-working user shapes (see R1, R1b and
R2 in the regression-tests commit earlier in this branch):

  - users with s3:ListBucket + s3:PutObject only (push-only credentials)
  - users with s3:ListBucket + s3:DeleteObject only (delete-only
    retention credentials)
  - users with s3:ListBucket + prefix-scoped Get/Put/Delete where the
    backup name equals the prefix (HeadObject ARN does not match the
    IAM Resource)

All three fail at "Probe failed" after PR percona#1717 because the new
HeadObject-on-bucket/<backup_name> probe requires s3:GetObject, which
those users do not have.

This commit replaces PR percona#1717's HeadObject probe with a permission-
tolerant HeadBucket probe (probe_endpoint, declared private in s3.h)
while keeping PR percona#1717's xbcloud_put change (the removal of the
container_exists / create_container pre-checks).

probe_endpoint preserves PR percona#1717's intent - tenants that lack
unconditional s3:ListBucket no longer need it to pass the probe -
because we accept any well-formed S3 response, including 403
AccessDenied, as evidence that the request signature was honoured.
The probe only fails over to the next (lookup, signature-version)
combination on transport errors or on the specific 403s that mean the
signature was rejected (SignatureDoesNotMatch, InvalidAccessKeyId).
See the implementation comment in s3.cc for the full contract.

Concretely, the changes vs PR percona#1717 are:

  - probe_api_version_and_lookup goes back to taking only `bucket`.
  - object_exists() (added by PR percona#1717) is removed - it was only used
    by the probe and is no longer needed.
  - probe_endpoint() is added as a new private helper on S3_client.
  - xbcloud-t.cc test fixtures revert to the single-arg probe call.
  - xbcloud.cc call sites revert to the single-arg probe call.
  - xbcloud_put's container_exists / create_container removal is left
    unchanged from PR percona#1717.

Combined with the preceding commits, this branch:

  test commits  R1, R1b, R2, R4 in place
  PR percona#1717 cherry-pick   R1, R1b, R2 fail; R4 passes (PR percona#1717's trade-off)
  this commit            R1, R1b, R2, R4 all pass

The change matches the behaviour of AWS CLI and rclone, which do not
pre-flight bucket existence either and rely on the first real request
to surface NoSuchBucket.
@satya-bodapati

satya-bodapati commented May 22, 2026

Copy link
Copy Markdown
Contributor

Another issue with removing the probe: https://perconadev.atlassian.net/browse/PXB-3654?focusedCommentId=463526

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@satya-bodapati satya-bodapati Awaiting requested review from satya-bodapati

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using HeadBucket for backups is dangerous in multi-tenant S3 buckets

4 participants

@DimitriosLisenko @it-percona-cla @egegunes @satya-bodapati