Skip to content

Commit 4213cc2

Browse files
Make cacheEntry.getIndexInput() privileged when fetching blobs from remote snapshot (opensearch-project#16544)
* Make cacheEntry.getIndexInput() privileged when fetching blobs from remote store Signed-off-by: Finn Carroll <carrofin@amazon.com> * Rebase Signed-off-by: Finn Carroll <carrofin@amazon.com> * Spotless apply Signed-off-by: Finn Carroll <carrofin@amazon.com> * Clean up doPrivileged calls Signed-off-by: Finn Carroll <carrofin@amazon.com> * Comment Signed-off-by: Finn Carroll <carrofin@amazon.com> * Move fetchBlob to PrivilegedExceptionAction. Catch and unwrap IOException. Signed-off-by: Finn Carroll <carrofin@amazon.com> * Unused import Signed-off-by: Finn Carroll <carrofin@amazon.com> * Update server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java Co-authored-by: Andriy Redko <drreta@gmail.com> Signed-off-by: Finn <finnegancarroll94@gmail.com> * Typo 'thrown'. Catch and throw unknown exception as IOException. Signed-off-by: Finn Carroll <carrofin@amazon.com> --------- Signed-off-by: Finn Carroll <carrofin@amazon.com> Signed-off-by: Finn <finnegancarroll94@gmail.com> Co-authored-by: Andriy Redko <drreta@gmail.com>
1 parent e07499a commit 4213cc2

File tree

2 files changed

+40
-25
lines changed

2 files changed

+40
-25
lines changed

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
3232
- Remove resource usages object from search response headers ([#16532](https://github.com/opensearch-project/OpenSearch/pull/16532))
3333
- Support retrieving doc values of unsigned long field ([#16543](https://github.com/opensearch-project/OpenSearch/pull/16543))
3434
- Fix rollover alias supports restored searchable snapshot index([#16483](https://github.com/opensearch-project/OpenSearch/pull/16483))
35+
- Fix permissions error on scripted query against remote snapshot ([#16544](https://github.com/opensearch-project/OpenSearch/pull/16544))
3536

3637
### Security
3738

server/src/main/java/org/opensearch/index/store/remote/utils/TransferManager.java

+39-25
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,8 @@
2424
import java.nio.file.Files;
2525
import java.nio.file.Path;
2626
import java.security.AccessController;
27-
import java.security.PrivilegedAction;
27+
import java.security.PrivilegedActionException;
28+
import java.security.PrivilegedExceptionAction;
2829
import java.util.concurrent.CompletableFuture;
2930
import java.util.concurrent.CompletionException;
3031
import java.util.concurrent.atomic.AtomicBoolean;
@@ -56,39 +57,52 @@ public TransferManager(final StreamReader streamReader, final FileCache fileCach
5657

5758
/**
5859
* Given a blobFetchRequestList, return it's corresponding IndexInput.
60+
*
61+
* Note: Scripted queries/aggs may trigger a blob fetch within a new security context.
62+
* As such the following operations require elevated permissions.
63+
*
64+
* cacheEntry.getIndexInput() downloads new blobs from the remote store to local fileCache.
65+
* fileCache.compute() as inserting into the local fileCache may trigger an eviction.
66+
*
5967
* @param blobFetchRequest to fetch
6068
* @return future of IndexInput augmented with internal caching maintenance tasks
6169
*/
6270
public IndexInput fetchBlob(BlobFetchRequest blobFetchRequest) throws IOException {
63-
6471
final Path key = blobFetchRequest.getFilePath();
6572
logger.trace("fetchBlob called for {}", key.toString());
6673

67-
// We need to do a privileged action here in order to fetch from remote
68-
// and write/evict from local file cache in case this is invoked as a side
69-
// effect of a plugin (such as a scripted search) that doesn't have the
70-
// necessary permissions.
71-
final CachedIndexInput cacheEntry = AccessController.doPrivileged((PrivilegedAction<CachedIndexInput>) () -> {
72-
return fileCache.compute(key, (path, cachedIndexInput) -> {
73-
if (cachedIndexInput == null || cachedIndexInput.isClosed()) {
74-
logger.trace("Transfer Manager - IndexInput closed or not in cache");
75-
// Doesn't exist or is closed, either way create a new one
76-
return new DelayedCreationCachedIndexInput(fileCache, streamReader, blobFetchRequest);
77-
} else {
78-
logger.trace("Transfer Manager - Already in cache");
79-
// already in the cache and ready to be used (open)
80-
return cachedIndexInput;
74+
try {
75+
return AccessController.doPrivileged((PrivilegedExceptionAction<IndexInput>) () -> {
76+
CachedIndexInput cacheEntry = fileCache.compute(key, (path, cachedIndexInput) -> {
77+
if (cachedIndexInput == null || cachedIndexInput.isClosed()) {
78+
logger.trace("Transfer Manager - IndexInput closed or not in cache");
79+
// Doesn't exist or is closed, either way create a new one
80+
return new DelayedCreationCachedIndexInput(fileCache, streamReader, blobFetchRequest);
81+
} else {
82+
logger.trace("Transfer Manager - Already in cache");
83+
// already in the cache and ready to be used (open)
84+
return cachedIndexInput;
85+
}
86+
});
87+
88+
// Cache entry was either retrieved from the cache or newly added, either
89+
// way the reference count has been incremented by one. We can only
90+
// decrement this reference _after_ creating the clone to be returned.
91+
try {
92+
return cacheEntry.getIndexInput().clone();
93+
} finally {
94+
fileCache.decRef(key);
8195
}
8296
});
83-
});
84-
85-
// Cache entry was either retrieved from the cache or newly added, either
86-
// way the reference count has been incremented by one. We can only
87-
// decrement this reference _after_ creating the clone to be returned.
88-
try {
89-
return cacheEntry.getIndexInput().clone();
90-
} finally {
91-
fileCache.decRef(key);
97+
} catch (PrivilegedActionException e) {
98+
final Exception cause = e.getException();
99+
if (cause instanceof IOException) {
100+
throw (IOException) cause;
101+
} else if (cause instanceof RuntimeException) {
102+
throw (RuntimeException) cause;
103+
} else {
104+
throw new IOException(cause);
105+
}
92106
}
93107
}
94108

0 commit comments

Comments
 (0)