Skip to content

Fix permission issues in BACKUP/RESTORE operations#94617

Merged
pamarcos merged 9 commits intoClickHouse:masterfrom
pamarcos:fix-backup-permission
Jan 22, 2026
Merged

Fix permission issues in BACKUP/RESTORE operations#94617
pamarcos merged 9 commits intoClickHouse:masterfrom
pamarcos:fix-backup-permission

Conversation

@pamarcos
Copy link
Copy Markdown
Member

@pamarcos pamarcos commented Jan 19, 2026

Fix permission issues in BACKUP/RESTORE operations

  1. Forbid RESTORE in readonly mode - RESTORE modifies data, so it should be blocked when readonly setting is enabled
  2. Forbid internal setting for initial queries - this setting is only meant for ON CLUSTER secondary queries, not user-specified
  3. Check BACKUP permissions before opening backup destination - prevents establishing S3/remote connections before access is verified

Closes https://github.com/ClickHouse/clickhouse-private/issues/45780

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix permission issues in BACKUP/RESTORE operations

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@pamarcos
Fix security issues in BACKUP/RESTORE operations
a253299 
1. Forbid RESTORE in readonly mode - RESTORE modifies data, so it
   should be blocked when `readonly` setting is enabled
2. Forbid `internal` setting for initial queries - this setting is
   only meant for ON CLUSTER secondary queries, not user-specified
3. Check BACKUP permissions before opening backup destination - prevents
   establishing S3/remote connections before access is verified
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jan 19, 2026 •
edited by pamarcos
Loading

Workflow [PR], commit [4737b78]

Summary: ❌

job_name test_name status info comment
Stateless tests (amd_debug, distributed plan, s3 storage, parallel) failure
01169_alter_partition_isolation_stress FAIL IGNORED
BuzzHouse (amd_debug) failure
Logical error: 'Inconsistent AST formatting: the query: (STID: 1941-1bfa) FAIL cidb, issue ISSUE EXISTS

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Jan 19, 2026
@pamarcos pamarcos marked this pull request as ready for review January 19, 2026 18:01
@pamarcos pamarcos requested a review from Copilot January 19, 2026 18:01
Copilot AI reviewed Jan 19, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses three critical security vulnerabilities in BACKUP/RESTORE operations by implementing proper permission checks and access controls before potentially expensive operations occur.

Changes:

  • Added validation to prevent user-specified internal setting for BACKUP/RESTORE queries (only allowed for ON CLUSTER secondary queries)
  • Added check to forbid RESTORE operations when readonly setting is enabled
  • Moved BACKUP permission verification before opening backup destination connections (e.g., S3)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Backups/BackupsWorker.cpp Implements security checks: validates internal setting for both BACKUP and RESTORE, blocks RESTORE in readonly mode, and verifies BACKUP permissions before opening connections
tests/queries/0_stateless/03799_backup_restore_security.sh Adds test cases covering readonly RESTORE blocking, internal setting validation, and permission check ordering
tests/queries/0_stateless/03799_backup_restore_security.reference Expected output for the security test

@jkartseva jkartseva self-assigned this Jan 19, 2026
@pamarcos
Fix test for fast test
8a90045
@pamarcos
Allow running test in prior version without crazy retries
df7a4a0
@pamarcos
Fix readonly check to only block readonly=1
0ce628a 
The previous check `if (readonly)` blocked RESTORE for both readonly=1
and readonly=2. However, readonly=2 is automatically set by the HTTP
interface for non-POST requests to protect against accidental writes.
This was causing legitimate RESTORE operations via HTTP sessions to fail.

Now we only block readonly=1 (strict read-only mode explicitly set by
the user) while allowing readonly=2 which is used by the HTTP interface
for its automatic protection mechanism.
@pamarcos
Merge branch 'master' into fix-backup-permission
f491a68
jkartseva
jkartseva reviewed Jan 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jkartseva jkartseva left a comment

Choose a reason for hiding this comment

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

The change LGTM, but the test requires a fix.

@pamarcos
Copy link
Copy Markdown
Member Author

pamarcos commented Jan 21, 2026

The change LGTM, but the test requires a fix.

@jkartseva what's wrong with the test? The only thing that failed was the Bugfix validation but I think it's a limitation of our CI that the job it's flagged as a failure even though the test works in the new version while it does not in the prior version. Or maybe there's something I'm missing, but I think it's a false negative.

jkartseva
jkartseva approved these changes Jan 21, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@jkartseva jkartseva left a comment

Choose a reason for hiding this comment

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

The CI was failing because of invalid access key – sorry, I did not save the link to the logs.
LGTM.

@pamarcos pamarcos added this pull request to the merge queue Jan 22, 2026
Merged via the queue into ClickHouse:master with commit cd03cf4 Jan 22, 2026
129 of 132 checks passed
@pamarcos pamarcos deleted the fix-backup-permission branch January 22, 2026 09:58
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jan 22, 2026
This was referenced Jan 22, 2026
Merged
Merged
Merged
Merged
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Merge pull request #94812 from ClickHouse/cherrypick/25.11/94617
4f2205a 
Cherry pick #94617 to 25.11: Fix permission issues in BACKUP/RESTORE operations
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Backport #94617 to 25.11: Fix permission issues in BACKUP/RESTORE ope…
45d7b7b 
…rations
This was referenced Jan 22, 2026
Merged
Merged
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Merge pull request #94814 from ClickHouse/cherrypick/25.12/94617
dfc4717 
Cherry pick #94617 to 25.12: Fix permission issues in BACKUP/RESTORE operations
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Backport #94617 to 25.12: Fix permission issues in BACKUP/RESTORE ope…
05bcf43 
…rations
@robot-clickhouse robot-clickhouse mentioned this pull request Jan 22, 2026
Merged
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94811 from ClickHouse/cherrypick/25.10/94617
9266c99 
Cherry pick #94617 to 25.10: Fix permission issues in BACKUP/RESTORE operations
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94810 from ClickHouse/cherrypick/25.8/94617
12b7d02 
Cherry pick #94617 to 25.8: Fix permission issues in BACKUP/RESTORE operations
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jan 22, 2026
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Backport #94617 to 25.8: Fix permission issues in BACKUP/RESTORE oper…
c2a9a3e 
…ations
@robot-ch-test-poll3 robot-ch-test-poll3 mentioned this pull request Jan 22, 2026
Merged
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Backport #94617 to 25.10: Fix permission issues in BACKUP/RESTORE ope…
90569ce 
…rations
@robot-ch-test-poll3 robot-ch-test-poll3 mentioned this pull request Jan 22, 2026
Merged
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94813 from ClickHouse/backport/25.11/94617
9a0c51a 
Backport #94617 to 25.11: Fix permission issues in BACKUP/RESTORE operations
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94815 from ClickHouse/backport/25.12/94617
aa3b660 
Backport #94617 to 25.12: Fix permission issues in BACKUP/RESTORE operations
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94809 from ClickHouse/cherrypick/25.3/94617
5af87a2 
Cherry pick #94617 to 25.3: Fix permission issues in BACKUP/RESTORE operations
robot-clickhouse added a commit that referenced this pull request Jan 22, 2026
@robot-clickhouse
Backport #94617 to 25.3: Fix permission issues in BACKUP/RESTORE oper…
b5d6977 
…ations
@robot-ch-test-poll2 robot-ch-test-poll2 mentioned this pull request Jan 22, 2026
Merged
clickhouse-gh bot added a commit that referenced this pull request Jan 22, 2026
@clickhouse-gh
Merge pull request #94819 from ClickHouse/backport/25.8/94617
7f820f2 
Backport #94617 to 25.8: Fix permission issues in BACKUP/RESTORE operations
@robot-clickhouse robot-clickhouse added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jan 22, 2026
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94820 from ClickHouse/backport/25.10/94617
90f2bb0 
Backport #94617 to 25.10: Fix permission issues in BACKUP/RESTORE operations
pamarcos added a commit that referenced this pull request Jan 22, 2026
@pamarcos
Merge pull request #94829 from ClickHouse/backport/25.3/94617
8444134 
Backport #94617 to 25.3: Fix permission issues in BACKUP/RESTORE operations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@jkartseva jkartseva jkartseva approved these changes

Assignees

@jkartseva jkartseva

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pamarcos @jkartseva @robot-clickhouse @robot-ch-test-poll1 @robot-clickhouse-ci-2