From e100d392a0ed4a22d8544d25299df2366e727d4c Mon Sep 17 00:00:00 2001 From: Yuval Lifshitz Date: Sun, 26 Mar 2023 10:02:17 +0000 Subject: [PATCH] rgw/notifications: support bucket notification with bucket policy following policy should be used to allow any user to get, put and delete bucket notification on a bucket called "my-bucket": { "Version": "2012-10-17", "Statement": [ { "Sid": "Statement", "Effect": "Allow", "Principal": "*", "Action": ["s3:GetBucketNotification", "s3:PutBucketNotification"], "Resource": "arn:aws:s3:::my-bucket" } ] } note that notification deletion uses the "PUT" permission. Fixes: https://tracker.ceph.com/issues/59136 Signed-off-by: Yuval Lifshitz --- src/rgw/rgw_rest_pubsub.cc | 165 +++++++++----------- src/test/rgw/bucket_notification/test_bn.py | 120 +++++++++++--- 2 files changed, 175 insertions(+), 110 deletions(-) diff --git a/src/rgw/rgw_rest_pubsub.cc b/src/rgw/rgw_rest_pubsub.cc index b68c3b5996675..5ab22e63775ef 100644 --- a/src/rgw/rgw_rest_pubsub.cc +++ b/src/rgw/rgw_rest_pubsub.cc @@ -565,12 +565,7 @@ int delete_all_notifications(const DoutPrefixProvider *dpp, const rgw_pubsub_buc // a "notification" and a subscription will be auto-generated // actual configuration is XML encoded in the body of the message class RGWPSCreateNotifOp : public RGWDefaultResponseOp { - private: - std::string bucket_name; - std::unique_ptr bucket; - rgw_pubsub_s3_notifications configurations; - - int get_params() { + int verify_params() override { bool exists; const auto no_value = s->info.args.get("notification", &exists); if (!exists) { @@ -585,22 +580,10 @@ class RGWPSCreateNotifOp : public RGWDefaultResponseOp { ldpp_dout(this, 1) << "request must be on a bucket" << dendl; return -EINVAL; } - bucket_name = s->bucket_name; return 0; } - public: - int verify_permission(optional_yield y) override; - - void pre_exec() override { - rgw_bucket_object_pre_exec(s); - } - - const char* name() const override { return "pubsub_notification_create_s3"; } - RGWOpType get_type() override { return RGW_OP_PUBSUB_NOTIF_CREATE; } - uint32_t op_mask() override { return RGW_OP_TYPE_WRITE; } - - int get_params_from_body() { + int get_params_from_body(rgw_pubsub_s3_notifications& configurations) { const auto max_size = s->cct->_conf->rgw_max_put_param_size; int r; bufferlist data; @@ -635,16 +618,41 @@ class RGWPSCreateNotifOp : public RGWDefaultResponseOp { } return 0; } +public: + int verify_permission(optional_yield y) override; + + void pre_exec() override { + rgw_bucket_object_pre_exec(s); + } + + const char* name() const override { return "pubsub_notification_create_s3"; } + RGWOpType get_type() override { return RGW_OP_PUBSUB_NOTIF_CREATE; } + uint32_t op_mask() override { return RGW_OP_TYPE_WRITE; } + void execute(optional_yield) override; }; void RGWPSCreateNotifOp::execute(optional_yield y) { - op_ret = get_params_from_body(); + op_ret = verify_params(); if (op_ret < 0) { return; } + rgw_pubsub_s3_notifications configurations; + op_ret = get_params_from_body(configurations); + if (op_ret < 0) { + return; + } + + std::unique_ptr user = driver->get_user(s->owner.get_id()); + std::unique_ptr bucket; + op_ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, s->bucket_name, &bucket, y); + if (op_ret < 0) { + ldpp_dout(this, 1) << "failed to get bucket '" << s->bucket_name << "' info, ret = " << op_ret << dendl; + return; + } + const RGWPubSub ps(driver, s->owner.get_id().tenant); const RGWPubSub::Bucket b(ps, bucket.get()); @@ -653,7 +661,7 @@ void RGWPSCreateNotifOp::execute(optional_yield y) { rgw_pubsub_bucket_topics bucket_topics; op_ret = b.get_topics(this, bucket_topics, y); if (op_ret < 0) { - ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << bucket_name << "', ret=" << op_ret << dendl; + ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << s->bucket_name << "', ret=" << op_ret << dendl; return; } @@ -728,44 +736,16 @@ void RGWPSCreateNotifOp::execute(optional_yield y) { } int RGWPSCreateNotifOp::verify_permission(optional_yield y) { - int ret = get_params(); - if (ret < 0) { - return ret; - } - - std::unique_ptr user = driver->get_user(s->owner.get_id()); - ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, bucket_name, &bucket, y); - if (ret < 0) { - ldpp_dout(this, 1) << "failed to get bucket info, cannot verify ownership" << dendl; - return ret; + if (!verify_bucket_permission(this, s, rgw::IAM::s3PutBucketNotification)) { + return -EACCES; } - if (bucket->get_info().owner != s->owner.get_id()) { - ldpp_dout(this, 1) << "user doesn't own bucket, not allowed to create notification" << dendl; - return -EPERM; - } return 0; } // command (extension to S3): DELETE /bucket?notification[=] class RGWPSDeleteNotifOp : public RGWDefaultResponseOp { - private: - std::string bucket_name; - std::unique_ptr bucket; - std::string notif_name; - - public: - int verify_permission(optional_yield y) override; - - void pre_exec() override { - rgw_bucket_object_pre_exec(s); - } - - const char* name() const override { return "pubsub_notification_delete_s3"; } - RGWOpType get_type() override { return RGW_OP_PUBSUB_NOTIF_DELETE; } - uint32_t op_mask() override { return RGW_OP_TYPE_DELETE; } - - int get_params() { + int get_params(std::string& notif_name) const { bool exists; notif_name = s->info.args.get("notification", &exists); if (!exists) { @@ -776,19 +756,38 @@ class RGWPSDeleteNotifOp : public RGWDefaultResponseOp { ldpp_dout(this, 1) << "request must be on a bucket" << dendl; return -EINVAL; } - bucket_name = s->bucket_name; return 0; } +public: + int verify_permission(optional_yield y) override; + + void pre_exec() override { + rgw_bucket_object_pre_exec(s); + } + + const char* name() const override { return "pubsub_notification_delete_s3"; } + RGWOpType get_type() override { return RGW_OP_PUBSUB_NOTIF_DELETE; } + uint32_t op_mask() override { return RGW_OP_TYPE_DELETE; } + void execute(optional_yield y) override; }; void RGWPSDeleteNotifOp::execute(optional_yield y) { - op_ret = get_params(); + std::string notif_name; + op_ret = get_params(notif_name); if (op_ret < 0) { return; } + std::unique_ptr user = driver->get_user(s->owner.get_id()); + std::unique_ptr bucket; + op_ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, s->bucket_name, &bucket, y); + if (op_ret < 0) { + ldpp_dout(this, 1) << "failed to get bucket '" << s->bucket_name << "' info, ret = " << op_ret << dendl; + return; + } + const RGWPubSub ps(driver, s->owner.get_id().tenant); const RGWPubSub::Bucket b(ps, bucket.get()); @@ -796,7 +795,7 @@ void RGWPSDeleteNotifOp::execute(optional_yield y) { rgw_pubsub_bucket_topics bucket_topics; op_ret = b.get_topics(this, bucket_topics, y); if (op_ret < 0) { - ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << bucket_name << "', ret=" << op_ret << dendl; + ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << s->bucket_name << "', ret=" << op_ret << dendl; return; } @@ -817,33 +816,18 @@ void RGWPSDeleteNotifOp::execute(optional_yield y) { } int RGWPSDeleteNotifOp::verify_permission(optional_yield y) { - int ret = get_params(); - if (ret < 0) { - return ret; + if (!verify_bucket_permission(this, s, rgw::IAM::s3PutBucketNotification)) { + return -EACCES; } - std::unique_ptr user = driver->get_user(s->owner.get_id()); - ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, bucket_name, &bucket, y); - if (ret < 0) { - return ret; - } - - if (bucket->get_info().owner != s->owner.get_id()) { - ldpp_dout(this, 1) << "user doesn't own bucket, cannot remove notification" << dendl; - return -EPERM; - } return 0; } // command (S3 compliant): GET /bucket?notification[=] class RGWPSListNotifsOp : public RGWOp { -private: - std::string bucket_name; - std::unique_ptr bucket; - std::string notif_name; rgw_pubsub_s3_notifications notifications; - int get_params() { + int get_params(std::string& notif_name) const { bool exists; notif_name = s->info.args.get("notification", &exists); if (!exists) { @@ -854,11 +838,10 @@ private: ldpp_dout(this, 1) << "request must be on a bucket" << dendl; return -EINVAL; } - bucket_name = s->bucket_name; return 0; } - public: +public: int verify_permission(optional_yield y) override; void pre_exec() override { @@ -886,6 +869,20 @@ private: }; void RGWPSListNotifsOp::execute(optional_yield y) { + std::string notif_name; + op_ret = get_params(notif_name); + if (op_ret < 0) { + return; + } + + std::unique_ptr user = driver->get_user(s->owner.get_id()); + std::unique_ptr bucket; + op_ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, s->bucket_name, &bucket, y); + if (op_ret < 0) { + ldpp_dout(this, 1) << "failed to get bucket '" << s->bucket_name << "' info, ret = " << op_ret << dendl; + return; + } + const RGWPubSub ps(driver, s->owner.get_id().tenant); const RGWPubSub::Bucket b(ps, bucket.get()); @@ -893,7 +890,7 @@ void RGWPSListNotifsOp::execute(optional_yield y) { rgw_pubsub_bucket_topics bucket_topics; op_ret = b.get_topics(this, bucket_topics, y); if (op_ret < 0) { - ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << bucket_name << "', ret=" << op_ret << dendl; + ldpp_dout(this, 1) << "failed to get list of topics from bucket '" << s->bucket_name << "', ret=" << op_ret << dendl; return; } if (!notif_name.empty()) { @@ -918,20 +915,8 @@ void RGWPSListNotifsOp::execute(optional_yield y) { } int RGWPSListNotifsOp::verify_permission(optional_yield y) { - int ret = get_params(); - if (ret < 0) { - return ret; - } - - std::unique_ptr user = driver->get_user(s->owner.get_id()); - ret = driver->get_bucket(this, user.get(), s->owner.get_id().tenant, bucket_name, &bucket, y); - if (ret < 0) { - return ret; - } - - if (bucket->get_info().owner != s->owner.get_id()) { - ldpp_dout(this, 1) << "user doesn't own bucket, cannot get notification list" << dendl; - return -EPERM; + if (!verify_bucket_permission(this, s, rgw::IAM::s3GetBucketNotification)) { + return -EACCES; } return 0; diff --git a/src/test/rgw/bucket_notification/test_bn.py b/src/test/rgw/bucket_notification/test_bn.py index d3d77ba1a2f66..66ee673cbed6a 100644 --- a/src/test/rgw/bucket_notification/test_bn.py +++ b/src/test/rgw/bucket_notification/test_bn.py @@ -9,6 +9,7 @@ import time import os import string import boto +from botocore.exceptions import ClientError from http import server as http_server from random import randint import hashlib @@ -515,6 +516,22 @@ def connection2(): return conn +def another_user(tenant=None): + access_key = str(time.time()) + secret_key = str(time.time()) + uid = 'superman' + str(time.time()) + if tenant: + _, result = admin(['user', 'create', '--uid', uid, '--tenant', tenant, '--access-key', access_key, '--secret-key', secret_key, '--display-name', '"Super Man"']) + else: + _, result = admin(['user', 'create', '--uid', uid, '--access-key', access_key, '--secret-key', secret_key, '--display-name', '"Super Man"']) + + assert_equal(result, 0) + conn = S3Connection(aws_access_key_id=access_key, + aws_secret_access_key=secret_key, + is_secure=False, port=get_config_port(), host=get_config_host(), + calling_format='boto.s3.connection.OrdinaryCallingFormat') + return conn + ############## # bucket notifications tests ############## @@ -523,17 +540,8 @@ def connection2(): @attr('basic_test') def test_ps_s3_topic_on_master(): """ test s3 topics set/get/delete on master """ - - access_key = str(time.time()) - secret_key = str(time.time()) - uid = 'superman' + str(time.time()) tenant = 'kaboom' - _, result = admin(['user', 'create', '--uid', uid, '--tenant', tenant, '--access-key', access_key, '--secret-key', secret_key, '--display-name', '"Super Man"']) - assert_equal(result, 0) - conn = S3Connection(aws_access_key_id=access_key, - aws_secret_access_key=secret_key, - is_secure=False, port=get_config_port(), host=get_config_host(), - calling_format='boto.s3.connection.OrdinaryCallingFormat') + conn = another_user(tenant) zonegroup = 'default' bucket_name = gen_bucket_name() topic_name = bucket_name + TOPIC_SUFFIX @@ -604,17 +612,8 @@ def test_ps_s3_topic_on_master(): @attr('basic_test') def test_ps_s3_topic_admin_on_master(): """ test s3 topics set/get/delete on master """ - - access_key = str(time.time()) - secret_key = str(time.time()) - uid = 'superman' + str(time.time()) tenant = 'kaboom' - _, result = admin(['user', 'create', '--uid', uid, '--tenant', tenant, '--access-key', access_key, '--secret-key', secret_key, '--display-name', '"Super Man"']) - assert_equal(result, 0) - conn = S3Connection(aws_access_key_id=access_key, - aws_secret_access_key=secret_key, - is_secure=False, port=get_config_port(), host=get_config_host(), - calling_format='boto.s3.connection.OrdinaryCallingFormat') + conn = another_user(tenant) zonegroup = 'default' bucket_name = gen_bucket_name() topic_name = bucket_name + TOPIC_SUFFIX @@ -1124,6 +1123,87 @@ def test_ps_s3_notification_errors_on_master(): # delete the bucket conn.delete_bucket(bucket_name) +@attr('basic_test') +def test_ps_s3_notification_permissions(): + """ test s3 notification set/get/delete permissions """ + conn1 = connection() + conn2 = another_user() + zonegroup = 'default' + bucket_name = gen_bucket_name() + # create bucket + bucket = conn1.create_bucket(bucket_name) + topic_name = bucket_name + TOPIC_SUFFIX + # create s3 topic + endpoint_address = 'amqp://127.0.0.1:7001' + endpoint_args = 'push-endpoint='+endpoint_address+'&amqp-exchange=amqp.direct&amqp-ack-level=none' + topic_conf = PSTopicS3(conn1, topic_name, zonegroup, endpoint_args=endpoint_args) + topic_arn = topic_conf.set_config() + + # one user create a notification + notification_name = bucket_name + NOTIFICATION_SUFFIX + topic_conf_list = [{'Id': notification_name, + 'TopicArn': topic_arn, + 'Events': [] + }] + s3_notification_conf1 = PSNotificationS3(conn1, bucket_name, topic_conf_list) + _, status = s3_notification_conf1.set_config() + assert_equal(status, 200) + # another user try to fetch it + s3_notification_conf2 = PSNotificationS3(conn2, bucket_name, topic_conf_list) + try: + _, _ = s3_notification_conf2.get_config() + assert False, "'AccessDenied' error is expected" + except ClientError as error: + assert_equal(error.response['Error']['Code'], 'AccessDenied') + # other user try to delete the notification + _, status = s3_notification_conf2.del_config() + assert_equal(status, 403) + + # bucket policy is added by the 1st user + client = boto3.client('s3', + endpoint_url='http://'+conn1.host+':'+str(conn1.port), + aws_access_key_id=conn1.aws_access_key_id, + aws_secret_access_key=conn1.aws_secret_access_key) + bucket_policy = json.dumps({ + "Version": "2012-10-17", + "Statement": [ + { + "Sid": "Statement", + "Effect": "Allow", + "Principal": "*", + "Action": ["s3:GetBucketNotification", "s3:PutBucketNotification"], + "Resource": f"arn:aws:s3:::{bucket_name}" + } + ] + }) + response = client.put_bucket_policy(Bucket=bucket_name, Policy=bucket_policy) + assert_equal(int(response['ResponseMetadata']['HTTPStatusCode']/100), 2) + result = client.get_bucket_policy(Bucket=bucket_name) + print(result['Policy']) + + # 2nd user try to fetch it again + _, status = s3_notification_conf2.get_config() + assert_equal(status, 200) + + # 2nd user try to delete it again + result, status = s3_notification_conf2.del_config() + assert_equal(status, 200) + + # 2nd user try to add another notification + topic_conf_list = [{'Id': notification_name+"2", + 'TopicArn': topic_arn, + 'Events': [] + }] + s3_notification_conf2 = PSNotificationS3(conn2, bucket_name, topic_conf_list) + result, status = s3_notification_conf2.set_config() + assert_equal(status, 200) + + # cleanup + s3_notification_conf1.del_config() + s3_notification_conf2.del_config() + topic_conf.del_config() + # delete the bucket + conn1.delete_bucket(bucket_name) @attr('amqp_test') def test_ps_s3_notification_push_amqp_on_master(): -- 2.39.5