You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "peterxcli (via GitHub)" <gi...@apache.org> on 2026/10/23 01:35:16 UTC
[PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
peterxcli opened a new pull request, #11707:
URL: https://github.com/apache/ozone/pull/11707
## What changes were proposed in this pull request?
### Background
The S3 Gateway serves all users with one `RpcClient`. Each request thread sets its own `S3Auth` as a thread-local, and `RpcClient` uses that S3 user as the owner of new keys, files, directories and multipart uploads.
### Problem
`RpcClient#setThreadLocalS3Auth` also stores the user in the field `s3gUgi`, which HDDS-7791 added. All request threads share this field, and `getRealUserInfo()` returns it whenever the calling thread has an `S3Auth`. OM stores the owner that the client sends.
Take two users, Alice and Bob, on the same S3 Gateway. Alice uploads an object. While the gateway handles the upload, Bob sends any request, for example ListObjects. Bob's request overwrites `s3gUgi`, so Alice's new key is stored with Bob as its owner. `ListObjects`, `ozone sh key info` and ofs then show Bob as the owner of Alice's key.
In a stress run on current master with one S3 Gateway, two users write from 8 threads each and a third user only lists objects. 330 of the 960 new keys get the wrong owner, and 236 of them get the user who only lists.
Access checks don't use the key owner, so this doesn't change who can read or write a key.
### Changes
- Remove the shared field `s3gUgi`.
- `getRealUserInfo()` creates the `UserGroupInformation` from the `S3Auth` of the calling thread. `createBucket()` and `getDEK()` already do the same.
- `setThreadLocalS3Auth()` no longer creates a `UserGroupInformation`. It used to do that once or twice for every S3 request. Now one is created only when a write needs the owner.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-17361
## How was this patch tested?
- New unit test `TestRpcClient#testKeyOwnerComesFromS3AuthOfCallingThread`. One thread sets the `S3Auth` of `alice`, a second thread then sets the `S3Auth` of `bob`, and the first thread creates a key. The test checks that the `CreateKey` request carries `alice` as the owner. Without the fix, it fails with `expected: <[alice]> but was: <[bob]>`.
- The existing owner tests `testKeyOwner` and `testMultipartUploadOwner` pass in `TestOzoneRpcClient` and in `TestSecureOzoneRpcClient` (block tokens and ACL checks on).
- With the fix, the stress run above (MiniOzoneCluster, one S3 Gateway, AWS SDK v1) gets no wrong owners in 960 keys. It is not part of this PR, because it depends on timing.
- `TestRpcClient`, `TestOzoneClient`, `TestOzoneECClient` and checkstyle pass locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #11707:
URL: https://github.com/apache/ozone/pull/11707#issuecomment-6393305460
Thanks @ivandika3 @smengcl!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #11707:
URL: https://github.com/apache/ozone/pull/11707#discussion_r4369321096
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -754,8 +750,9 @@ private UserGroupInformation getRealUserInfo() {
// After HDDS-5881 the user will not be different,
// as S3G uses single RpcClient. So we should be checking thread-local
// S3Auth and use it during proxy.
- if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- return s3gUgi;
+ S3Auth s3Auth = ozoneManagerClient.getThreadLocalS3Auth();
+ if (s3Auth != null) {
+ return UserGroupInformation.createRemoteUser(s3Auth.getUserPrincipal());
Review Comment:
> I think the change that addressed https://github.com/apache/ozone/pull/4188#discussion_r1550036354 is actually the cause of this bug. It uses a single s3gUgi field for all threads that use the RpcClient, so one user’s request can overwrite the owner of another user’s key.
Yes, I agreed that this is the root cause.
My thought is to have S3Auth contains the UGI instead of having an attribute of the UGI. However, you're right that the UGI is only needed for some cases and each AFAIK each S3G thread only calls `getRealUserInfo` at most once.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on code in PR #11707:
URL: https://github.com/apache/ozone/pull/11707#discussion_r4369321096
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -754,8 +750,9 @@ private UserGroupInformation getRealUserInfo() {
// After HDDS-5881 the user will not be different,
// as S3G uses single RpcClient. So we should be checking thread-local
// S3Auth and use it during proxy.
- if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- return s3gUgi;
+ S3Auth s3Auth = ozoneManagerClient.getThreadLocalS3Auth();
+ if (s3Auth != null) {
+ return UserGroupInformation.createRemoteUser(s3Auth.getUserPrincipal());
Review Comment:
> I think the change that addressed https://github.com/apache/ozone/pull/4188#discussion_r1550036354 is actually the cause of this bug. It uses a single s3gUgi field for all threads that use the RpcClient, so one user’s request can overwrite the owner of another user’s key.
Yes, I agreed that this is the root cause.
My thought is to have S3Auth contains the UGI instead of having an attribute of the UGI. However, you're right that the UGI is only needed for some cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "smengcl (via GitHub)" <gi...@apache.org>.
smengcl commented on code in PR #11707:
URL: https://github.com/apache/ozone/pull/11707#discussion_r4357378396
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/rpc/TestRpcClient.java:
##########
@@ -375,6 +375,40 @@ public void testListMultipartUploadsValidatesNames(String volume, String bucket,
}
}
+ /**
+ * S3 Gateway serves all users with one RpcClient. A request from bob on another thread must not change the owner
+ * of the key that alice creates.
+ */
+ @Test
+ public void testKeyOwnerComesFromS3AuthOfCallingThread() throws Exception {
+ List<String> owners = new ArrayList<>();
+ RpcClient client = createRpcClient(new MockOmTransport() {
+ @Override
+ public OMResponse submitRequest(OMRequest request) throws IOException {
+ if (request.getCmdType() == Type.CreateKey) {
+ owners.add(request.getCreateKeyRequest().getKeyArgs().getOwnerName());
+ }
+ return super.submitRequest(request);
+ }
+ });
+ try {
+ client.createVolume("vol1");
+ client.createBucket("vol1", "bucket1");
+ client.setThreadLocalS3Auth(new S3Auth("stringToSign", "signature", "alice", "alice"));
+ Thread otherRequest = new Thread(
+ () -> client.setThreadLocalS3Auth(new S3Auth("stringToSign", "signature", "bob", "bob")));
+ otherRequest.start();
+ otherRequest.join();
+
+ ReplicationConfig replication = RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.ONE);
+ client.createKey("vol1", "bucket1", "key1", 0, replication, Collections.emptyMap()).close();
+ assertEquals(Collections.singletonList("alice"), owners);
Review Comment:
nit: use AssertJ to give a clearer failure message here
```suggestion
assertThat(owners).containsExactly("alice");
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #11707:
URL: https://github.com/apache/ozone/pull/11707#discussion_r4368084303
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -3060,7 +3057,6 @@ public OzoneKey headS3Object(String bucketName, String keyName)
public void setThreadLocalS3Auth(
S3Auth ozoneSharedSecretAuth) {
ozoneManagerClient.setThreadLocalS3Auth(ozoneSharedSecretAuth);
- this.s3gUgi = UserGroupInformation.createRemoteUser(getThreadLocalS3Auth().getUserPrincipal());
Review Comment:
this removal actually help to reduce ugi creation, as `GET`, `HEAD`, `list` and `delete` request dont call `getRealUserInfo()`
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java:
##########
@@ -754,8 +750,9 @@ private UserGroupInformation getRealUserInfo() {
// After HDDS-5881 the user will not be different,
// as S3G uses single RpcClient. So we should be checking thread-local
// S3Auth and use it during proxy.
- if (ozoneManagerClient.getThreadLocalS3Auth() != null) {
- return s3gUgi;
+ S3Auth s3Auth = ozoneManagerClient.getThreadLocalS3Auth();
+ if (s3Auth != null) {
+ return UserGroupInformation.createRemoteUser(s3Auth.getUserPrincipal());
Review Comment:
> @peterxcli Thanks for the fix. Is it possible to make s3gUgi to be ThreadLocal instead? The argument of the review https://github.com/apache/ozone/pull/4188#discussion_r1550036354 is that UGI creation is wasteful.
@ivandika3 I think the change that addressed [apache/ozone#4188 (comment)](https://github.com/apache/ozone/pull/4188#discussion_r1550036354) is actually the cause of this bug. It uses a single s3gUgi field for all threads that use the RpcClient, so one user’s request can overwrite the owner of another user’s key.
Also, after this patch a request uses the UGI at most once, so caching it in a ThreadLocal won’t help. If we need it more than once in the future, we can add the ThreadLocal then.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-17361. S3 Gateway can set another user as the owner of a new key [ozone]
Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 merged PR #11707:
URL: https://github.com/apache/ozone/pull/11707
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org