From 98858d8728a8208f10243cc4de453dfa47174ebe Mon Sep 17 00:00:00 2001 From: Finn Carroll Date: Fri, 1 Nov 2024 14:20:25 -0700 Subject: [PATCH] Make cacheEntry.getIndexInput() privileged when fetching blobs from remote store Signed-off-by: Finn Carroll --- .../store/remote/utils/TransferManager.java | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java index 94c25202ac90c..9a62dfa0cf0b6 100644 --- a/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java +++ b/server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java @@ -56,6 +56,13 @@ public TransferManager(final StreamReader streamReader, final FileCache fileCach /** * Given a blobFetchRequestList, return it's corresponding IndexInput. + * + * Note: Scripted queries/aggs may trigger a blob fetch within a new security context. + * As such the following operations require elevated permissions. + * + * cacheEntry.getIndexInput() downloads new blobs from the remote store to local disk. + * fileCache.compute() because inserting into the fileCache can trigger an eviction of on disk cache. + * * @param blobFetchRequest to fetch * @return future of IndexInput augmented with internal caching maintenance tasks */ @@ -64,10 +71,6 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio final Path key = blobFetchRequest.getFilePath(); logger.trace("fetchBlob called for {}", key.toString()); - // We need to do a privileged action here in order to fetch from remote - // and write/evict from local file cache in case this is invoked as a side - // effect of a plugin (such as a scripted search) that doesn't have the - // necessary permissions. final CachedIndexInput cacheEntry = AccessController.doPrivileged((PrivilegedAction) () -> { return fileCache.compute(key, (path, cachedIndexInput) -> { if (cachedIndexInput == null || cachedIndexInput.isClosed()) { @@ -82,14 +85,18 @@ public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOExceptio }); }); - // Cache entry was either retrieved from the cache or newly added, either - // way the reference count has been incremented by one. We can only - // decrement this reference _after_ creating the clone to be returned. - try { - return cacheEntry.getIndexInput().clone(); - } finally { - fileCache.decRef(key); - } + return AccessController.doPrivileged((PrivilegedAction) () -> { + // Cache entry was either retrieved from the cache or newly added, either + // way the reference count has been incremented by one. We can only + // decrement this reference _after_ creating the clone to be returned. + try { + return cacheEntry.getIndexInput().clone(); + } catch (IOException e) { + throw new RuntimeException(e); + } finally { + fileCache.decRef(key); + } + }); } @SuppressWarnings("removal")