jenkins test make check

very cool

my only concern is that this might break existing "user-mode" replication policies where the destination bucket relied on s3:PutObject. but you've only recently enabled the policy evaluation here, right? if we don't have to worry about existing reef/squid users, then a release note should suffice

separately, objects that fail to sync due to these permission checks will still show x-amz-replication-status: COMPLETED after #57060. ideally they would be FAILED, but i don't think we have a good way to communicate that back to the source zone

my only concern is that this might break existing "user-mode" replication policies where the destination bucket relied on s3:PutObject. but you've only recently enabled the policy evaluation here, right? if we don't have to worry about existing reef/squid users, then a release note should suffice

right, I have added a release note for that.

separately, objects that fail to sync due to these permission checks will still show x-amz-replication-status: COMPLETED after #57060. ideally they would be FAILED, but i don't think we have a good way to communicate that back to the source zone

true, the only thing I can think of is as of offloading perm check for the source object to the source zone (a3f40b4) we can perhaps update the attr if the permission was denied. this could also help to implement s3:GetObjectVersionForReplication (and perhaps an RGW-specific one s3:GetObjectForReplication - as we don't enforce versioning). but that requires a good identifier on the request so RGW can assume this is a replication request, not a normal system request issued by a human.
I have tried one in #60227 (comment) through rgwx-zonegroup existence.

separately, objects that fail to sync due to these permission checks will still show x-amz-replication-status: COMPLETED after #57060. ideally they would be FAILED, but i don't think we have a good way to communicate that back to the source zone

true, the only thing I can think of is as of offloading perm check for the source object to the source zone (a3f40b4) we can perhaps update the attr if the permission was denied. this could also help to implement s3:GetObjectVersionForReplication (and perhaps an RGW-specific one s3:GetObjectForReplication - as we don't enforce versioning).

oh wow, i had no idea that aws requires that "Both source and destination buckets must have versioning enabled."

but that requires a good identifier on the request so RGW can assume this is a replication request, not a normal system request issued by a human. I have tried one in #60227 through rgwx-zonegroup existence.

maybe the requests from FetchRemoteObj could include the rgwx-uid param that we use for rgw_forward_request_to_master() and, if present in RGWGetObj::verify_permission(), use that identity and the s3:GetObjectVersionForReplication action for verify_object_permission(). if that fails, we'd consider updating the replication status header. but because it's still a system request, we'd ignore errors from verify_permission() with "overriding permissions due to system operation"

separately, objects that fail to sync due to these permission checks will still show x-amz-replication-status: COMPLETED after #57060. ideally they would be FAILED, but i don't think we have a good way to communicate that back to the source zone

true, the only thing I can think of is as of offloading perm check for the source object to the source zone (a3f40b4) we can perhaps update the attr if the permission was denied. this could also help to implement s3:GetObjectVersionForReplication (and perhaps an RGW-specific one s3:GetObjectForReplication - as we don't enforce versioning).

oh wow, i had no idea that aws requires that "Both source and destination buckets must have versioning enabled."

Yeah, I had a thread on Slack a while ago trying to discuss why AWS enforces this requirement and whether we should follow suit. Unfortunately, I can't retrieve messages older than 90 days in Slack. The general consensus seems to be that versioning helps maintain the immutability of the replicated objects. This can be useful in cases where replication lags behind and the replication policy changes in the meantime. For instance, if a policy initially specifies replicating objects with the prefix "A," but later changes to "B," objects with the prefix "A" that were uploaded earlier might not be replicated, as the new policy would be applied at the destination and it will get dropped, or if it was based on tags which here perhaps versioning can play a role.
Not sure what is the right thing to do in these cases but one of my ideas about the log_zonegroup in #61862 (comment) was to ensure consistent replication of objects based on the state at the time of upload, regardless of policy changes. so no matter what, the destination just replicates it as at that time where the object has been uploaded the criteria matched the object - although I guess this can't be true for permission stuff as if the policy is dropped anytime and so the replication should be stopped.

On the other hand, from an Object Storage perspective, Apparantly GCS doesn't require you to enable versioning (https://cloud.google.com/storage-transfer/docs/cross-bucket-replication - at least I couldn't find a link saying it's required) - so not sure if this should be something up to the user whether to enable or not.

Some discussions around versioning required by AWS for replication:

• https://repost.aws/questions/QUgcwxF8jAQqaaM7ujgjRCVA/why-do-we-need-s3-bucket-versioning-enabled-in-order-to-do-replication
• https://www.reddit.com/r/aws/comments/4kl8dt/s3_from_a_code_perspective_why_does_versioning/?rdt=33757

maybe the requests from FetchRemoteObj could include the rgwx-uid param

This is already happening in the commit I mentioned (a3f40b4) 👍

if present in RGWGetObj::verify_permission(), use that identity and the s3:GetObjectVersionForReplication action for verify_object_permission(). if that fails, we'd consider updating the replication status header.

yeah if you also agree, we can do so. but perhaps in another PR to make it separate. Although they have dependencies and perhaps new PR could be difficult as this one requires changes from #61828.
Although there is also an edge case here where if there are multiple destinations for an object and only one destination fails due to permission stuff, should we mark it FAILED or COMPLETED? If it's FAILED when should be changed it to COMPLETED? we don't know whether it was a new upload or not (here maybe forcing versioning can help) but also be tricky for full sync scenarios which we perhaps need to implement differently for cross-zonegroup replication as they should come from a different "API" like batch replication job in AWS.

Some discussions around versioning required by AWS for replication:

thanks. i can definitely see why immutability helps with async replication. but rgw's default multisite replication for DR has to handle the non-versioned case anyway, so i can also see why Yehuda didn't impose that requirement on bucket replication policy. that said, it would make sad to need more rgw extensions like s3:GetObjectForReplication to make the feature sensible for rgw

but because it's still a system request, we'd ignore errors from verify_permission() with "overriding permissions due to system operation"

I didn't get this part, I have something like this in my mind:

@@ -1100,7 +1108,17 @@ int RGWGetObj::verify_permission(optional_yield y)
     if (has_s3_existing_tag || has_s3_resource_tag)
       rgw_iam_add_objtags(this, s, has_s3_existing_tag, has_s3_resource_tag);
 
-  if (get_torrent) {
+  // if it's system request and the identity is not the acl owner
+  // then we assume rgwx-uid was passed by the request and so it's a replication request.
+  bool is_replication_request = s->system_request && !s->identity->is_owner_of(s->owner);
+
+  if (is_replication_request) {
+    if (s->object->get_instance().empty()) {
+      action = rgw::IAM::s3GetObjectForReplication;
+    } else {
+      action = rgw::IAM::s3GetObjectVersionForReplication;
+    }
+  } else if (get_torrent) {
     if (s->object->get_instance().empty()) {
       action = rgw::IAM::s3GetObjectTorrent;
     } else {
@@ -1115,7 +1133,18 @@ int RGWGetObj::verify_permission(optional_yield y)
   }
 
   if (!verify_object_permission(this, s, action)) {
-    return -EACCES;
+    if (is_replication_request) {
+      // fallback to s3:GetObject(Version) if s3:GetObject(Version)ForReplication is not allowed
+      action = s->object->get_instance().empty() ? rgw::IAM::s3GetObject : rgw::IAM::s3GetObjectVersion;
+      if (!verify_object_permission(this, s, action)) {
+        // update the replication status of the object to FAILED
+        set_replication_status_header(this, s->object.get(), y, "FAILED");
+
+        return -EACCES;
+      }
+    } else {
+      return -EACCES;
+    }
   }

but why do we need to not return -EACCES as usual and continue?

btw this only covers object fetching, the destination permissions are being evaluated by the destination zone so perhaps we can implement an API letting destination zones update the status on the source object. not sure how efficient this is going to be.

but why do we need to not return -EACCES as usual and continue?

sorry, the problem is here: https://github.com/ceph/ceph/blob/7211a69/src/rgw/rgw_process.cc#L226-L231

even if RGWGetObj::verify_permission() returns an error, admin/system requests assume they should have access regardless, so continue processing the request as normal. so we'd either need to change how these overrides work, or stop using the system user for these user-mode requests

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

no but it adds a bool is_system_request to RoleApplier where i think we'd rather pass bool is_impersonating to SysReqApplier

@cbodley - I'm just unsure about rgwx-uid param parse skipping when we have is_impersonating true (c0c4834#diff-7aeff97f1eb1e873092d521f455a855328376f71b4339dc679527092e9f7a1d8R309-R312). Do you know how does the forwarded requests for STS look like? Could we impersonate and have rgwx-uid still effective?

no but it adds a bool is_system_request to RoleApplier where i think we'd rather pass bool is_impersonating to SysReqApplier

@cbodley - I'm just unsure about rgwx-uid param parse skipping when we have is_impersonating true (c0c4834#diff-7aeff97f1eb1e873092d521f455a855328376f71b4339dc679527092e9f7a1d8R309-R312). Do you know how does the forwarded requests for STS look like? Could we impersonate and have rgwx-uid still effective?

i thought the only purpose of rgwx-uid was to preserve the original ACLOwner. if we're impersonating a role, i'd expect the RoleApplier to handle ownership the same way it did on the original zone. so i think it's fine to pass is_impersonating=true for that case to ignore rgwx-uid

cc @pritha-srivastava

no but it adds a bool is_system_request to RoleApplier where i think we'd rather pass bool is_impersonating to SysReqApplier

@cbodley - I'm just unsure about rgwx-uid param parse skipping when we have is_impersonating true (c0c4834#diff-7aeff97f1eb1e873092d521f455a855328376f71b4339dc679527092e9f7a1d8R309-R312). Do you know how does the forwarded requests for STS look like? Could we impersonate and have rgwx-uid still effective?

i thought the only purpose of rgwx-uid was to preserve the original ACLOwner. if we're impersonating a role, i'd expect the RoleApplier to handle ownership the same way it did on the original zone. so i think it's fine to pass is_impersonating=true for that case to ignore rgwx-uid

cc @pritha-srivastava

right, thanks. as the token is passed by header, it will act like rgwx-perm-check-uid to derive the right role here (as we create the role applier from the token, so is_impersonating is what exactly has to happen).

https://jenkins.ceph.com/job/ceph-pull-requests-arm64/73264/

32 - run-rbd-unit-tests-61.sh (Failed)

jenkins test make check arm64

"utilize is_impersonating for forwarded sts requests" commit looks great

passed qa in https://pulpito.ceph.com/cbodley-2025-04-29_02:19:58-rgw-wip-70093-distro-default-smithi/

Yay! 😍

tentacle backport bundled with #63043