Merge pull request ceph#61828 from clwluvw/datasync-useracl

cbodley · web-flow · commit c8c15bd4fdc4 · 2025-03-11T18:52:16.000-04:00
rgw: prevent data sync from replicating to buckets not owned by the user

Signed-off-by: Shilpa Jagannath &lt;smanjara@redhat.com&gt;
diff --git a/qa/tasks/rgw_multisite_tests.py b/qa/tasks/rgw_multisite_tests.py
@@ -78,8 +78,19 @@ def setup(self):
         user.create(master_zone, ['--display-name', 'TestUser',
                                   '--gen-access-key', '--gen-secret'])
 
+        # create non-account user
+        log.info('creating non-account user..')
+        non_account_user = multisite.User('rgw-multisite-test-non-account-user')
+        non_account_user.create(master_zone, ['--display-name', 'NonAccountUser',
+                                              '--gen-access-key', '--gen-secret'])
+        # create non-account alt user
+        log.info('creating non-account alt user..')
+        non_account_alt_user = multisite.User('rgw-multisite-test-non-account-alt-user')
+        non_account_alt_user.create(master_zone, ['--display-name', 'NonAccountAltUser',
+                                                  '--gen-access-key', '--gen-secret'])
+
         config = self.config.get('config', {})
-        tests.init_multi(realm, user, tests.Config(**config))
+        tests.init_multi(realm, user, non_account_user, non_account_alt_user, tests.Config(**config))
         tests.realm_meta_checkpoint(realm)
 
     def begin(self):
diff --git a/src/rgw/driver/rados/rgw_data_sync.cc b/src/rgw/driver/rados/rgw_data_sync.cc
@@ -2694,8 +2694,8 @@ class RGWUserPermHandler {
 
       ret = RGWUserPermHandler::policy_from_attrs(
           sync_env->cct, user->get_attrs(), &info->user_acl);
-      if (ret == -ENOENT) {
-        info->user_acl.create_default(uid, user->get_display_name());
+      if (ret < 0 && ret != -ENOENT) {
+        return ret;
       }
 
       return 0;
diff --git a/src/test/rgw/rgw_multi/conn.py b/src/test/rgw/rgw_multi/conn.py
@@ -5,15 +5,16 @@
 
 def get_gateway_connection(gateway, credentials):
     """ connect to the given gateway """
+    # Always create a new connection to the gateway to ensure each set of credentials gets its own connection
+    conn = boto.connect_s3(aws_access_key_id = credentials.access_key,
+                           aws_secret_access_key = credentials.secret,
+                           host = gateway.host,
+                           port = gateway.port,
+                           is_secure = False,
+                           calling_format = boto.s3.connection.OrdinaryCallingFormat())
     if gateway.connection is None:
-        gateway.connection = boto.connect_s3(
-                aws_access_key_id = credentials.access_key,
-                aws_secret_access_key = credentials.secret,
-                host = gateway.host,
-                port = gateway.port,
-                is_secure = False,
-                calling_format = boto.s3.connection.OrdinaryCallingFormat())
-    return gateway.connection
+        gateway.connection = conn
+    return conn
 
 def get_gateway_secure_connection(gateway, credentials):
     """ secure connect to the given gateway """
@@ -47,13 +48,15 @@ def get_gateway_iam_connection(gateway, credentials, region):
 
 def get_gateway_s3_client(gateway, credentials, region):
   """ connect to boto3 s3 client api of the given gateway """
+  # Always create a new connection to the gateway to ensure each set of credentials gets its own connection
+  s3_client = boto3.client('s3',
+                           endpoint_url='http://' + gateway.host + ':' + str(gateway.port),
+                           aws_access_key_id=credentials.access_key,
+                           aws_secret_access_key=credentials.secret,
+                           region_name=region)
   if gateway.s3_client is None:
-      gateway.s3_client = boto3.client('s3',
-                                        endpoint_url='http://' + gateway.host + ':' + str(gateway.port),
-                                        aws_access_key_id=credentials.access_key,
-                                        aws_secret_access_key=credentials.secret,
-                                        region_name=region)
-  return gateway.s3_client
+      gateway.s3_client = s3_client
+  return s3_client
 
 
 def get_gateway_sns_client(gateway, credentials, region):
diff --git a/src/test/rgw/rgw_multi/tests.py b/src/test/rgw/rgw_multi/tests.py
@@ -42,12 +42,18 @@ def __init__(self, **kwargs):
 # implementations of these interfaces by calling init_multi()
 realm = None
 user = None
+non_account_user = None
+non_account_alt_user = None
 config = None
-def init_multi(_realm, _user, _config=None):
+def init_multi(_realm, _user, _non_account_user, _non_account_alt_user, _config=None):
     global realm
     realm = _realm
     global user
     user = _user
+    global non_account_user
+    non_account_user = _non_account_user
+    global non_account_alt_user
+    non_account_alt_user = _non_account_alt_user
     global config
     config = _config or Config()
     realm_meta_checkpoint(realm)
@@ -468,17 +474,31 @@ class ZonegroupConns:
     def __init__(self, zonegroup):
         self.zonegroup = zonegroup
         self.zones = []
+        self.non_account_zones = []
+        self.non_account_alt_zones = []
         self.ro_zones = []
+        self.non_account_ro_zones = []
+        self.non_account_alt_ro_zones = []
         self.rw_zones = []
+        self.non_account_rw_zones = []
+        self.non_account_alt_rw_zones = []
         self.master_zone = None
 
         for z in zonegroup.zones:
             zone_conn = z.get_conn(user.credentials)
+            non_account_zone_conn = z.get_conn(non_account_user.credentials)
+            non_account_alt_zone_conn = z.get_conn(non_account_alt_user.credentials)
             self.zones.append(zone_conn)
+            self.non_account_zones.append(non_account_zone_conn)
+            self.non_account_alt_zones.append(non_account_alt_zone_conn)
             if z.is_read_only():
                 self.ro_zones.append(zone_conn)
+                self.non_account_ro_zones.append(non_account_zone_conn)
+                self.non_account_alt_ro_zones.append(non_account_alt_zone_conn)
             else:
                 self.rw_zones.append(zone_conn)
+                self.non_account_rw_zones.append(non_account_zone_conn)
+                self.non_account_alt_rw_zones.append(non_account_alt_zone_conn)
 
             if z == zonegroup.master_zone:
                 self.master_zone = zone_conn
@@ -3703,3 +3723,165 @@ def test_bucket_create_location_constraint():
                                     Bucket=bucket_name,
                                     CreateBucketConfiguration={'LocationConstraint': zg.name})
                 assert e.response['ResponseMetadata']['HTTPStatusCode'] == 400
+
+def allow_bucket_replication(function):
+    def wrapper(*args, **kwargs):
+        zonegroup = realm.master_zonegroup()
+        if len(zonegroup.zones) < 2:
+            raise SkipTest("More than one zone needed in any one or multiple zone(s).")
+        
+        zones = ",".join([z.name for z in zonegroup.zones])
+        z = zonegroup.zones[0]
+        c = z.cluster
+
+        create_sync_policy_group(c, "sync-group")
+        create_sync_group_flow_symmetrical(c, "sync-group", "sync-flow", zones)
+        create_sync_group_pipe(c, "sync-group", "sync-pipe", zones, zones)
+
+        zonegroup.period.update(z, commit=True)
+        realm_meta_checkpoint(realm)
+
+        try:
+            function(*args, **kwargs)
+        finally:
+            remove_sync_group_pipe(c, "sync-group", "sync-pipe")
+            remove_sync_group_flow_symmetrical(c, "sync-group", "sync-flow")
+            remove_sync_policy_group(c, "sync-group")
+
+            zonegroup.period.update(z, commit=True)
+            realm_meta_checkpoint(realm)
+
+    return wrapper
+
+@allow_bucket_replication
+def test_bucket_replication_normal():
+    zonegroup = realm.master_zonegroup()
+    zonegroup_conns = ZonegroupConns(zonegroup)
+    
+    source = zonegroup_conns.non_account_rw_zones[0]
+    dest = zonegroup_conns.non_account_rw_zones[1]
+
+    source_bucket = source.create_bucket(gen_bucket_name())
+    dest_bucket = dest.create_bucket(gen_bucket_name())
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # create replication configuration
+    response = source.s3_client.put_bucket_replication(
+        Bucket=source_bucket.name,
+        ReplicationConfiguration={
+            'Role': '',
+            'Rules': [{
+                'ID': 'rule1',
+                'Status': 'Enabled',
+                'Destination': {
+                    'Bucket': f'arn:aws:s3:::{dest_bucket.name}',
+                }
+            }]
+        }
+    )
+    assert response['ResponseMetadata']['HTTPStatusCode'] == 200
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # upload an object and wait for sync.
+    objname = 'dummy'
+    k = new_key(source, source_bucket, objname)
+    k.set_contents_from_string('foo')
+    zone_data_checkpoint(dest.zone, source.zone)
+
+    # check that object exists in destination bucket
+    k = get_key(dest, dest_bucket, objname)
+    assert_equal(k.get_contents_as_string().decode('utf-8'), 'foo')
+
+@allow_bucket_replication
+def test_bucket_replication_alt_user_forbidden():
+    zonegroup = realm.master_zonegroup()
+    zonegroup_conns = ZonegroupConns(zonegroup)
+
+    source = zonegroup_conns.non_account_rw_zones[0]
+    dest = zonegroup_conns.non_account_alt_rw_zones[1]
+
+    source_bucket = source.create_bucket(gen_bucket_name())
+    dest_bucket = dest.create_bucket(gen_bucket_name())
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # create replication configuration
+    response = source.s3_client.put_bucket_replication(
+        Bucket=source_bucket.name,
+        ReplicationConfiguration={
+            'Role': '',
+            'Rules': [{
+                'ID': 'rule1',
+                'Status': 'Enabled',
+                'Destination': {
+                    'Bucket': f'arn:aws:s3:::{dest_bucket.name}',
+                }
+            }]
+        }
+    )
+    assert response['ResponseMetadata']['HTTPStatusCode'] == 200
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # upload an object and wait for sync.
+    objname = 'dummy'
+    k = new_key(source, source_bucket, objname)
+    k.set_contents_from_string('foo')
+    zone_data_checkpoint(dest.zone, source.zone)
+
+    # check that object does not exist in destination bucket
+    e = assert_raises(ClientError, dest.s3_client.get_object, Bucket=dest_bucket.name, Key=objname)
+    assert e.response['Error']['Code'] == 'NoSuchKey'
+
+@allow_bucket_replication
+def test_bucket_replication_alt_user():
+    zonegroup = realm.master_zonegroup()
+    zonegroup_conns = ZonegroupConns(zonegroup)
+
+    source = zonegroup_conns.non_account_rw_zones[0]
+    dest = zonegroup_conns.non_account_alt_rw_zones[1]
+
+    source_bucket = source.create_bucket(gen_bucket_name())
+    dest_bucket = dest.create_bucket(gen_bucket_name())
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # create replication configuration
+    response = source.s3_client.put_bucket_replication(
+        Bucket=source_bucket.name,
+        ReplicationConfiguration={
+            'Role': '',
+            'Rules': [{
+                'ID': 'rule1',
+                'Status': 'Enabled',
+                'Destination': {
+                    'Bucket': f'arn:aws:s3:::{dest_bucket.name}',
+                    'AccessControlTranslation': {
+                        'Owner': non_account_alt_user.id,
+                    },
+                }
+            }]
+        }
+    )
+    assert response['ResponseMetadata']['HTTPStatusCode'] == 200
+    # give access to user to write to alt user's bucket
+    dest.s3_client.put_bucket_policy(
+        Bucket=dest_bucket.name,
+        Policy=json.dumps({
+            'Version': '2012-10-17',
+            'Statement': [{
+                'Effect': 'Allow',
+                'Principal': {'AWS': [f"arn:aws:iam:::user/{user.id}"]},
+                'Action': 's3:PutObject',
+                'Resource': f'arn:aws:s3:::{dest_bucket.name}/*',
+            }]
+        })
+    )
+    zonegroup_meta_checkpoint(zonegroup)
+
+    # upload an object and wait for sync.
+    objname = 'dummy'
+    k = new_key(source, source_bucket, objname)
+    k.set_contents_from_string('foo')
+    zone_data_checkpoint(dest.zone, source.zone)
+
+    # check that object exists in destination bucket
+    k = get_key(dest, dest_bucket, objname)
+    assert_equal(k.get_contents_as_string().decode('utf-8'), 'foo')
diff --git a/src/test/rgw/test_multi.py b/src/test/rgw/test_multi.py
@@ -248,6 +248,12 @@ def init(parse_args):
     user_creds = gen_credentials()
     user = multisite.User('tester', tenant=args.tenant, account='RGW11111111111111111')
 
+    non_account_user_creds = gen_credentials()
+    non_account_user = multisite.User('nonaccounttester', tenant=args.tenant)
+
+    non_account_alt_user_creds = gen_credentials()
+    non_account_alt_user = multisite.User('nonaccountalttester', tenant=args.tenant)
+
     realm = multisite.Realm('r')
     if bootstrap:
         # create the realm on c1
@@ -388,13 +394,24 @@ def init(parse_args):
                     arg = ['--display-name', 'TestUser']
                     arg += user_creds.credential_args()
                     user.create(zone, arg)
+                    # create non-account test user
+                    arg = ['--display-name', 'NonAccountTestUser']
+                    arg += non_account_user_creds.credential_args()
+                    non_account_user.create(zone, arg)
+                    # create non-account alt test user
+                    arg = ['--display-name', 'NonAccountAltTestUser']
+                    arg += non_account_alt_user_creds.credential_args()
+                    non_account_alt_user.create(zone, arg)
                 else:
                     # read users and update keys
                     admin_user.info(zone)
                     admin_creds = admin_user.credentials[0]
-                    arg = []
-                    user.info(zone, arg)
+                    user.info(zone)
                     user_creds = user.credentials[0]
+                    non_account_user.info(zone)
+                    non_account_user_creds = non_account_user.credentials[0]
+                    non_account_alt_user.info(zone)
+                    non_account_alt_user_creds = non_account_alt_user.credentials[0]
 
     if not bootstrap:
         period.get(c1)
@@ -403,7 +420,7 @@ def init(parse_args):
                     checkpoint_delay=args.checkpoint_delay,
                     reconfigure_delay=args.reconfigure_delay,
                     tenant=args.tenant)
-    init_multi(realm, user, config)
+    init_multi(realm, user, non_account_user, non_account_alt_user, config)
 
 def setup_module():
     init(False)

Original file line number	Diff line number	Diff line change
`@@ -2694,8 +2694,8 @@ class RGWUserPermHandler {`
`2694`	`2694`
`2695`	`2695`	`ret = RGWUserPermHandler::policy_from_attrs(`
`2696`	`2696`	`sync_env->cct, user->get_attrs(), &info->user_acl);`
`2697`		`- if (ret == -ENOENT) {`
`2698`		`- info->user_acl.create_default(uid, user->get_display_name());`
	`2697`	`+ if (ret < 0 && ret != -ENOENT) {`
	`2698`	`+ return ret;`
`2699`	`2699`	`}`
`2700`	`2700`
`2701`	`2701`	`return 0;`