Ensure necessary security context for s3 bulk deletions #108280

ywangd · 2024-05-04T13:10:08Z

This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Also added a new repository setting to configure max size for s3 deleteObjects request.

Fixes: #108049

This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Resolves: elastic#108049

github-actions · 2024-05-04T13:10:20Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-05-04T13:10:32Z

Hi @ywangd, I've created a changelog YAML for you.

elasticsearchmachine · 2024-05-04T13:10:33Z

Pinging @elastic/es-distributed (Team:Distributed)

ywangd · 2024-05-04T13:12:12Z

...aRestTest/java/org/elasticsearch/repositories/blobstore/testkit/S3SnapshotRepoTestKitIT.java

+            .put("client", "repo_test_kit")
+            .put("bucket", bucket)
+            .put("base_path", basePath)
+            .put("deletion_batch_size", between(1, 1000))


I took the easy way to piggyback on the existing repo analysis test. Please let me know if this works for you. Thanks!

👍 should be fine, but could you just confirm that this exposes the bug?

Yes I tested locally with real s3 by setting the properties listed here. I can confirm the test can fail without the fix.

A few notes:

It does not fail with the fixture. I wonder whether it has something to do with the fixture running on localhost.

When the size is 1, it will fail at repo verification without even start running the test.

I expected it to fail anytime the deletion is done inside the lambda but that is not always the case. I guess maybe if there is existing connection to perform the deletion request, it does not trigger the permission check. I did not look into it deeply since it feels sufficient to me that it does fail sometimes without the fix.

DaveCTurner

Couple of nits, overall looks great tho. WDYT about backporting all the way to 8.13? I think there'll be another release before 8.14, would be good to fix this ASAP.

DaveCTurner · 2024-05-05T19:03:44Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

+     * The batch size for bulk deletion
+     */
+    static final Setting<Integer> DELETION_BATCH_SIZE_SETTING = Setting.intSetting(
+        "deletion_batch_size",


Naming nit/suggestion: WDYT about delete_objects_max_size? The relevant S3 API is called DeleteObjects so I think mentioning that in the name would be helpful.

Sounds good to me. Updated it as you suggested 61255e5

DaveCTurner · 2024-05-05T19:04:07Z

...aRestTest/java/org/elasticsearch/repositories/blobstore/testkit/S3SnapshotRepoTestKitIT.java

+            .put("client", "repo_test_kit")
+            .put("bucket", bucket)
+            .put("base_path", basePath)
+            .put("deletion_batch_size", between(1, 1000))


👍 should be fine, but could you just confirm that this exposes the bug?

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

ywangd · 2024-05-06T02:50:01Z

WDYT about backporting all the way to 8.13? I think there'll be another release before 8.14, would be good to fix this ASAP.

I agree. Added v8.13.4. Thanks!

…ss-control

DaveCTurner

LGTM

elasticsearchmachine · 2024-05-06T04:52:58Z

Hi @ywangd, I've updated the changelog YAML for you.

elasticsearchmachine · 2024-05-06T04:57:36Z

Hi @ywangd, I've updated the changelog YAML for you.

arteam

LGTM2

ywangd · 2024-05-06T08:50:42Z

@elasticmachine update branch

This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Also added a new repository setting to configure max size for s3 deleteObjects request. Fixes: elastic#108049

elasticsearchmachine · 2024-05-06T10:03:35Z

💔 Backport failed

Status	Branch	Result
✅	8.14
❌	8.13	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 108280

ywangd · 2024-05-06T10:19:19Z

💚 All backports created successfully

Status	Branch	Result
✅	8.13

Questions ?

Please refer to the Backport tool documentation

This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Also added a new repository setting to configure max size for s3 deleteObjects request. Fixes: elastic#108049 (cherry picked from commit bcf4297) # Conflicts: # docs/reference/snapshot-restore/repository-s3.asciidoc # modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

…08298) This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Also added a new repository setting to configure max size for s3 deleteObjects request. Fixes: #108049

…08299) This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Also added a new repository setting to configure max size for s3 deleteObjects request. Fixes: #108049 (cherry picked from commit bcf4297) # Conflicts: # docs/reference/snapshot-restore/repository-s3.asciidoc # modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3BlobStore.java

Ensure necessary security context for s3 bulk deletions

6ad4aa1

This PR moves the doPrivileged wrapper closer to the actual deletion request to ensure the necesary security context is established at all times. Resolves: elastic#108049

ywangd added >bug :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v8.14.0 labels May 4, 2024

ywangd requested review from arteam and DaveCTurner May 4, 2024 13:10

Update docs/changelog/108280.yaml

bd2ed4e

elasticsearchmachine added the Team:Distributed Meta label for distributed team label May 4, 2024

elasticsearchmachine added the v8.15.0 label May 4, 2024

ywangd commented May 4, 2024

View reviewed changes

DaveCTurner reviewed May 5, 2024

View reviewed changes

review comments

61255e5

ywangd added v8.13.4 auto-backport-and-merge Automatically create backport pull requests and merge when ready labels May 6, 2024

ywangd added 2 commits May 6, 2024 12:50

Merge remote-tracking branch 'origin/main' into es-108049-fix-s3-acce…

056d69f

…ss-control

update changelog

9d80756

ywangd requested a review from DaveCTurner May 6, 2024 02:51

DaveCTurner approved these changes May 6, 2024

View reviewed changes

Update docs/changelog/108280.yaml

78187c0

update changelog

0862279

ywangd added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 6, 2024

Update docs/changelog/108280.yaml

41fc54c

ywangd removed the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 6, 2024

arteam approved these changes May 6, 2024

View reviewed changes

DaveCTurner added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 6, 2024

changelog

b8ec609

Merge branch 'main' into es-108049-fix-s3-access-control

4d7201f

elasticsearchmachine merged commit bcf4297 into elastic:main May 6, 2024
15 checks passed

ywangd deleted the es-108049-fix-s3-access-control branch May 6, 2024 10:02

ywangd mentioned this pull request May 6, 2024

[8.14] Ensure necessary security context for s3 bulk deletions (#108280) #108298

Merged

elasticsearchmachine added the backport pending label May 6, 2024

ywangd mentioned this pull request May 6, 2024

[8.13] Ensure necessary security context for s3 bulk deletions (#108280) #108299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure necessary security context for s3 bulk deletions #108280

Ensure necessary security context for s3 bulk deletions #108280

ywangd commented May 4, 2024 •

edited

github-actions bot commented May 4, 2024

elasticsearchmachine commented May 4, 2024

elasticsearchmachine commented May 4, 2024

ywangd May 4, 2024

DaveCTurner May 5, 2024

ywangd May 6, 2024

DaveCTurner left a comment

DaveCTurner May 5, 2024

ywangd May 6, 2024

DaveCTurner May 5, 2024

ywangd commented May 6, 2024

DaveCTurner left a comment

elasticsearchmachine commented May 6, 2024

elasticsearchmachine commented May 6, 2024

arteam left a comment

ywangd commented May 6, 2024

elasticsearchmachine commented May 6, 2024

ywangd commented May 6, 2024

Ensure necessary security context for s3 bulk deletions #108280

Ensure necessary security context for s3 bulk deletions #108280

Conversation

ywangd commented May 4, 2024 • edited

github-actions bot commented May 4, 2024

elasticsearchmachine commented May 4, 2024

elasticsearchmachine commented May 4, 2024

ywangd May 4, 2024

Choose a reason for hiding this comment

DaveCTurner May 5, 2024

Choose a reason for hiding this comment

ywangd May 6, 2024

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner May 5, 2024

Choose a reason for hiding this comment

ywangd May 6, 2024

Choose a reason for hiding this comment

DaveCTurner May 5, 2024

Choose a reason for hiding this comment

ywangd commented May 6, 2024

DaveCTurner left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented May 6, 2024

elasticsearchmachine commented May 6, 2024

arteam left a comment

Choose a reason for hiding this comment

ywangd commented May 6, 2024

elasticsearchmachine commented May 6, 2024

💔 Backport failed

ywangd commented May 6, 2024

💚 All backports created successfully

Questions ?

ywangd commented May 4, 2024 •

edited