Skip to content

Commit 45d7b7b

Browse files
robot-clickhouserobot-clickhouse
committed
Backport #94617 to 25.11: Fix permission issues in BACKUP/RESTORE operations
1 parent 6c9708c commit 45d7b7b

File tree

4 files changed

+101
-9
lines changed

4 files changed

+101
-9
lines changed

‎src/Backups/BackupsWorker.cpp‎

Lines changed: 35 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ namespace DB
6161

6262
namespace Setting
6363
{
64+
extern const SettingsUInt64 readonly;
6465
extern const SettingsBool s3_disable_checksum;
6566
}
6667

@@ -71,6 +72,7 @@ namespace ServerSetting
7172

7273
namespace ErrorCodes
7374
{
75+
extern const int ACCESS_DENIED;
7476
extern const int BAD_ARGUMENTS;
7577
extern const int LOGICAL_ERROR;
7678
extern const int QUERY_WAS_CANCELLED;
@@ -390,6 +392,12 @@ struct BackupsWorker::BackupStarter
390392
backup_info = BackupInfo::fromAST(*backup_query->backup_name);
391393
backup_name_for_logging = backup_info.toStringForLogging();
392394
is_internal_backup = backup_settings.internal;
395+
396+
/// The "internal" option can only be used by a query that was initiated by another query (e.g., ON CLUSTER query).
397+
/// It should not be allowed for an initial query explicitly specified by a user.
398+
if (is_internal_backup && (query_context->getClientInfo().query_kind == ClientInfo::QueryKind::INITIAL_QUERY))
399+
throw Exception(ErrorCodes::ACCESS_DENIED, "Setting 'internal' cannot be set explicitly");
400+
393401
on_cluster = !backup_query->cluster.empty() || is_internal_backup;
394402

395403
if (!backup_settings.backup_uuid)
@@ -462,14 +470,25 @@ struct BackupsWorker::BackupStarter
462470
cluster = backup_context->getCluster(backup_query->cluster);
463471
backup_settings.cluster_host_ids = cluster->getHostIDs();
464472
}
473+
474+
/// Check access rights before opening the backup destination (e.g., S3).
475+
/// This ensures we fail fast with a proper ACCESS_DENIED error instead of trying to connect to external storage first.
476+
/// For ON CLUSTER queries, access rights are checked in executeDDLQueryOnCluster() before distributing the query.
477+
if (!on_cluster)
478+
{
479+
backup_query->setCurrentDatabase(backup_context->getCurrentDatabase());
480+
auto required_access = BackupUtils::getRequiredAccessToBackup(backup_query->elements);
481+
query_context->checkAccess(required_access);
482+
}
483+
465484
chassert(backup_settings.data_file_name_prefix_length);
466485
backup_coordination = backups_worker.makeBackupCoordination(on_cluster, backup_settings, backup_context);
467486
backup_coordination->startup();
468487

469488
chassert(!backup);
470489
backup = backups_worker.openBackupForWriting(backup_info, backup_settings, backup_coordination, backup_context);
471490

472-
backups_worker.doBackup(backup, backup_query, backup_id, backup_settings, backup_coordination, backup_context, query_context,
491+
backups_worker.doBackup(backup, backup_query, backup_id, backup_settings, backup_coordination, backup_context,
473492
on_cluster, cluster);
474493

475494
if (!is_internal_backup)
@@ -621,7 +640,6 @@ void BackupsWorker::doBackup(
621640
const BackupSettings & backup_settings,
622641
std::shared_ptr<IBackupCoordination> backup_coordination,
623642
ContextMutablePtr context,
624-
const ContextPtr & query_context,
625643
bool on_cluster,
626644
const ClusterPtr & cluster)
627645
{
@@ -636,17 +654,13 @@ void BackupsWorker::doBackup(
636654

637655
bool is_internal_backup = backup_settings.internal;
638656

639-
/// Checks access rights if this is not ON CLUSTER query.
640-
/// (If this is ON CLUSTER query executeDDLQueryOnCluster() will check access rights later.)
641-
auto required_access = BackupUtils::getRequiredAccessToBackup(backup_query->elements);
642-
if (!on_cluster)
643-
query_context->checkAccess(required_access);
644-
645657
maybeSleepForTesting();
646658

647659
/// Write the backup.
648660
if (on_cluster && !is_internal_backup)
649661
{
662+
auto required_access = BackupUtils::getRequiredAccessToBackup(backup_query->elements);
663+
650664
/// Send the BACKUP query to other hosts.
651665
backup_settings.copySettingsToQuery(*backup_query);
652666
sendQueryToOtherHosts(*backup_query, cluster, backup_settings.shard_num, backup_settings.replica_num,
@@ -847,6 +861,19 @@ struct BackupsWorker::RestoreStarter
847861
backup_info = BackupInfo::fromAST(*restore_query->backup_name);
848862
backup_name_for_logging = backup_info.toStringForLogging();
849863
is_internal_restore = restore_settings.internal;
864+
865+
/// The "internal" option can only be used by a query that was initiated by another query (e.g., ON CLUSTER query).
866+
/// It should not be allowed for an initial query explicitly specified by a user.
867+
if (is_internal_restore && (query_context->getClientInfo().query_kind == ClientInfo::QueryKind::INITIAL_QUERY))
868+
throw Exception(ErrorCodes::ACCESS_DENIED, "Setting 'internal' cannot be set explicitly");
869+
870+
/// RESTORE is a write operation, it should be forbidden in strict readonly mode (readonly=1).
871+
/// Note: readonly=2 allows changing settings but still restricts writes - however it's set automatically
872+
/// by the HTTP interface for GET requests (to protect against accidental writes), so we only block readonly=1
873+
/// which is explicitly set by the user to enforce read-only mode.
874+
if (query_context->getSettingsRef()[Setting::readonly] == 1)
875+
throw Exception(ErrorCodes::ACCESS_DENIED, "Cannot execute RESTORE in readonly mode");
876+
850877
on_cluster = !restore_query->cluster.empty() || is_internal_restore;
851878

852879
if (!restore_settings.restore_uuid)

‎src/Backups/BackupsWorker.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,6 @@ class BackupsWorker
8989
const BackupSettings & backup_settings,
9090
std::shared_ptr<IBackupCoordination> backup_coordination,
9191
ContextMutablePtr context,
92-
const ContextPtr & query_context,
9392
bool on_cluster,
9493
const ClusterPtr & cluster);
9594

‎tests/queries/0_stateless/03799_backup_restore_security.reference‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
Test 1 OK
2+
0

‎tests/queries/0_stateless/03799_backup_restore_security.sh‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
#!/usr/bin/env bash
2+
3+
# Test for security fixes related to BACKUP and RESTORE operations:
4+
# 1. RESTORE should be forbidden in readonly mode
5+
# 2. The 'internal' setting should not be allowed for initial queries
6+
# 3. Permission check should happen before backup destination is opened (e.g., S3 connection)
7+
#
8+
# All tests use fake S3 URLs with invalid credentials. Since all security checks happen
9+
# before any connection attempt, we should always get ACCESS_DENIED errors, not S3 errors.
10+
# We set backup_restore_s3_retry_attempts=0 to avoid retries on connection failure
11+
# when running against a version without the fix.
12+
13+
CURDIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
14+
# shellcheck source=../shell_config.sh
15+
. "$CURDIR"/../shell_config.sh
16+
17+
backup_name="${CLICKHOUSE_DATABASE}_03799_backup_security"
18+
user_name="test_03799_user_${CLICKHOUSE_DATABASE}"
19+
20+
function cleanup()
21+
{
22+
$CLICKHOUSE_CLIENT -q "DROP TABLE IF EXISTS test_backup_security"
23+
$CLICKHOUSE_CLIENT -q "DROP TABLE IF EXISTS test_restored"
24+
$CLICKHOUSE_CLIENT -q "DROP USER IF EXISTS $user_name"
25+
}
26+
trap cleanup EXIT
27+
28+
$CLICKHOUSE_CLIENT -q "
29+
DROP TABLE IF EXISTS test_backup_security;
30+
CREATE TABLE test_backup_security (id Int32) ENGINE=MergeTree() ORDER BY id;
31+
INSERT INTO test_backup_security VALUES (1), (2), (3);
32+
"
33+
34+
# Test 1: RESTORE should be forbidden in readonly mode
35+
# The readonly check happens before any backup destination is accessed.
36+
# We use CLICKHOUSE_CLIENT_BINARY directly to avoid test framework injecting settings that conflict with readonly mode.
37+
$CLICKHOUSE_CLIENT --readonly=1 --backup_restore_s3_retry_attempts=0 -q "RESTORE TABLE test_backup_security AS test_restored FROM S3('http://localhost:11111/test/backups/${backup_name}', 'INVALID_ACCESS_KEY', 'INVALID_SECRET')" 2>&1 | grep -q "ACCESS_DENIED" && echo "Test 1 OK" || echo "Test 1 FAIL"
38+
# Verify that the table was not created
39+
$CLICKHOUSE_CLIENT -q "SELECT count() FROM system.tables WHERE database = currentDatabase() AND name = 'test_restored'"
40+
41+
# Test 2: The 'internal' setting should not be allowed for initial BACKUP query
42+
# This check happens before any connection attempt.
43+
$CLICKHOUSE_CLIENT -m -q "
44+
BACKUP TABLE test_backup_security TO S3('http://localhost:11111/test/backups/${backup_name}_internal', 'INVALID_ACCESS_KEY', 'INVALID_SECRET') SETTINGS internal=1, backup_restore_s3_retry_attempts=0; -- { serverError ACCESS_DENIED }
45+
"
46+
47+
# Test 3: The 'internal' setting should not be allowed for initial RESTORE query
48+
# This check happens before any connection attempt.
49+
$CLICKHOUSE_CLIENT -m -q "
50+
RESTORE TABLE test_backup_security AS test_restored FROM S3('http://localhost:11111/test/backups/${backup_name}', 'INVALID_ACCESS_KEY', 'INVALID_SECRET') SETTINGS internal=1, backup_restore_s3_retry_attempts=0; -- { serverError ACCESS_DENIED }
51+
"
52+
53+
# Test 4: User without BACKUP permission should get ACCESS_DENIED (not S3_ERROR)
54+
# This tests that permission check happens before opening backup destination.
55+
# We use S3 with invalid credentials - if we get ACCESS_DENIED instead of S3_ERROR,
56+
# it proves the permission check happens before attempting to connect to S3.
57+
$CLICKHOUSE_CLIENT -q "DROP USER IF EXISTS $user_name"
58+
$CLICKHOUSE_CLIENT -q "CREATE USER $user_name"
59+
$CLICKHOUSE_CLIENT -q "GRANT SELECT ON ${CLICKHOUSE_DATABASE}.test_backup_security TO $user_name"
60+
# User has SELECT but not BACKUP permission - should get ACCESS_DENIED, not S3_ERROR
61+
$CLICKHOUSE_CLIENT --user=$user_name -m -q "
62+
BACKUP TABLE test_backup_security TO S3('http://localhost:11111/test/backups/${CLICKHOUSE_DATABASE}/no_permission_backup', 'INVALID_ACCESS_KEY', 'INVALID_SECRET') SETTINGS backup_restore_s3_retry_attempts=0; -- { serverError ACCESS_DENIED }
63+
"
64+
$CLICKHOUSE_CLIENT -q "DROP USER IF EXISTS $user_name"

0 commit comments

Comments
 (0)