Opensearch repository-s3 plugin cannot read ServiceAccount token (ope…

…nsearch-project#6390) * Opensearch repository-s3 plugin cannot read ServiceAccount token Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> --------- Signed-off-by: Andriy Redko <[email protected]>
reta · Feb 21, 2023 · 5560ba4 · 5560ba4
1 parent cadba4d
commit 5560ba4
Show file tree

Hide file tree

Showing 12 changed files with 160 additions and 69 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -129,6 +129,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Fix weighted shard routing state across search requests([#6004](https://github.com/opensearch-project/OpenSearch/pull/6004))
 - [Segment Replication] Fix bug where inaccurate sequence numbers are sent during replication ([#6122](https://github.com/opensearch-project/OpenSearch/pull/6122))
 - Enable numeric sort optimisation for few numerical sort types ([#6321](https://github.com/opensearch-project/OpenSearch/pull/6321))
+- Fix Opensearch repository-s3 plugin cannot read ServiceAccount token ([#6390](https://github.com/opensearch-project/OpenSearch/pull/6390)
 
 ### Security
 

diff --git a/...c/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java b/...c/internalClusterTest/java/org/opensearch/repositories/s3/S3BlobStoreRepositoryTests.java
@@ -58,6 +58,7 @@
 import org.opensearch.threadpool.ThreadPool;
 
 import java.io.IOException;
+import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -149,8 +150,8 @@ protected Settings nodeSettings(int nodeOrdinal) {
      */
     public static class TestS3RepositoryPlugin extends S3RepositoryPlugin {
 
-        public TestS3RepositoryPlugin(final Settings settings) {
-            super(settings);
+        public TestS3RepositoryPlugin(final Settings settings, final Path configPath) {
+            super(settings, configPath);
         }
 
         @Override

diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3ClientSettings.java
@@ -35,6 +35,8 @@
 import com.amazonaws.ClientConfiguration;
 import com.amazonaws.Protocol;
 import org.opensearch.common.Strings;
+import org.opensearch.common.SuppressForbidden;
+import org.opensearch.common.io.PathUtils;
 import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.settings.SecureSetting;
 import org.opensearch.common.settings.SecureString;
@@ -46,6 +48,7 @@
 
 import java.net.InetAddress;
 import java.net.UnknownHostException;
+import java.nio.file.Path;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Locale;
@@ -348,16 +351,16 @@ S3ClientSettings refine(Settings repositorySettings) {
      *
      * Note this will always at least return a client named "default".
      */
-    static Map<String, S3ClientSettings> load(Settings settings) {
+    static Map<String, S3ClientSettings> load(final Settings settings, final Path configPath) {
         final Set<String> clientNames = settings.getGroups(PREFIX).keySet();
         final Map<String, S3ClientSettings> clients = new HashMap<>();
         for (final String clientName : clientNames) {
-            clients.put(clientName, getClientSettings(settings, clientName));
+            clients.put(clientName, getClientSettings(settings, clientName, configPath));
         }
         if (clients.containsKey("default") == false) {
             // this won't find any settings under the default client,
             // but it will pull all the fallback static settings
-            clients.put("default", getClientSettings(settings, "default"));
+            clients.put("default", getClientSettings(settings, "default", configPath));
         }
         return Collections.unmodifiableMap(clients);
     }
@@ -425,8 +428,17 @@ private static S3BasicCredentials loadCredentials(Settings settings, String clie
         }
     }
 
-    private static IrsaCredentials loadIrsaCredentials(Settings settings, String clientName) {
+    @SuppressForbidden(reason = "PathUtils#get")
+    private static IrsaCredentials loadIrsaCredentials(Settings settings, String clientName, Path configPath) {
         String identityTokenFile = getConfigValue(settings, clientName, IDENTITY_TOKEN_FILE_SETTING);
+        if (identityTokenFile.length() != 0) {
+            final Path identityTokenFilePath = PathUtils.get(identityTokenFile);
+            // If the path is not absolute, resolve it relatively to config path
+            if (!identityTokenFilePath.isAbsolute()) {
+                identityTokenFile = PathUtils.get(new Path[] { configPath }, identityTokenFile).toString();
+            }
+        }
+
         try (
             SecureString roleArn = getConfigValue(settings, clientName, ROLE_ARN_SETTING);
             SecureString roleSessionName = getConfigValue(settings, clientName, ROLE_SESSION_NAME_SETTING)
@@ -441,11 +453,11 @@ private static IrsaCredentials loadIrsaCredentials(Settings settings, String cli
 
     // pkg private for tests
     /** Parse settings for a single client. */
-    static S3ClientSettings getClientSettings(final Settings settings, final String clientName) {
+    static S3ClientSettings getClientSettings(final Settings settings, final String clientName, final Path configPath) {
         final Protocol awsProtocol = getConfigValue(settings, clientName, PROTOCOL_SETTING);
         return new S3ClientSettings(
             S3ClientSettings.loadCredentials(settings, clientName),
-            S3ClientSettings.loadIrsaCredentials(settings, clientName),
+            S3ClientSettings.loadIrsaCredentials(settings, clientName, configPath),
             getConfigValue(settings, clientName, ENDPOINT_SETTING),
             awsProtocol,
             Math.toIntExact(getConfigValue(settings, clientName, READ_TIMEOUT_SETTING).millis()),

diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3RepositoryPlugin.java
@@ -47,6 +47,7 @@
 import org.opensearch.repositories.Repository;
 
 import java.io.IOException;
+import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.Arrays;
@@ -77,15 +78,17 @@ public class S3RepositoryPlugin extends Plugin implements RepositoryPlugin, Relo
     }
 
     protected final S3Service service;
+    private final Path configPath;
 
-    public S3RepositoryPlugin(final Settings settings) {
-        this(settings, new S3Service());
+    public S3RepositoryPlugin(final Settings settings, final Path configPath) {
+        this(settings, configPath, new S3Service(configPath));
     }
 
-    S3RepositoryPlugin(final Settings settings, final S3Service service) {
+    S3RepositoryPlugin(final Settings settings, final Path configPath, final S3Service service) {
         this.service = Objects.requireNonNull(service, "S3 service must not be null");
+        this.configPath = configPath;
         // eagerly load client settings so that secure settings are read
-        final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings, configPath);
         this.service.refreshAndClearCache(clientsSettings);
     }
 
@@ -142,7 +145,7 @@ public List<Setting<?>> getSettings() {
     @Override
     public void reload(Settings settings) {
         // secure settings should be readable
-        final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> clientsSettings = S3ClientSettings.load(settings, configPath);
         service.refreshAndClearCache(clientsSettings);
     }
 

diff --git a/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java b/plugins/repository-s3/src/main/java/org/opensearch/repositories/s3/S3Service.java
@@ -72,6 +72,7 @@
 import java.net.PasswordAuthentication;
 import java.net.Proxy;
 import java.net.Socket;
+import java.nio.file.Path;
 import java.security.SecureRandom;
 import java.util.Map;
 
@@ -90,16 +91,20 @@ class S3Service implements Closeable {
     /**
      * Client settings calculated from static configuration and settings in the keystore.
      */
-    private volatile Map<String, S3ClientSettings> staticClientSettings = MapBuilder.<String, S3ClientSettings>newMapBuilder()
-        .put("default", S3ClientSettings.getClientSettings(Settings.EMPTY, "default"))
-        .immutableMap();
+    private volatile Map<String, S3ClientSettings> staticClientSettings;
 
     /**
      * Client settings derived from those in {@link #staticClientSettings} by combining them with settings
      * in the {@link RepositoryMetadata}.
      */
     private volatile Map<Settings, S3ClientSettings> derivedClientSettings = emptyMap();
 
+    S3Service(final Path configPath) {
+        staticClientSettings = MapBuilder.<String, S3ClientSettings>newMapBuilder()
+            .put("default", S3ClientSettings.getClientSettings(Settings.EMPTY, "default", configPath))
+            .immutableMap();
+    }
+
     /**
      * Refreshes the settings for the AmazonS3 clients and clears the cache of
      * existing clients. New clients will be build using these new settings. Old

diff --git a/...ins/repository-s3/src/test/java/org/opensearch/repositories/s3/AwsS3ServiceImplTests.java b/...ins/repository-s3/src/test/java/org/opensearch/repositories/s3/AwsS3ServiceImplTests.java
@@ -55,7 +55,7 @@
 import static org.opensearch.repositories.s3.S3ClientSettings.PROTOCOL_SETTING;
 import static org.opensearch.repositories.s3.S3ClientSettings.PROXY_TYPE_SETTING;
 
-public class AwsS3ServiceImplTests extends OpenSearchTestCase {
+public class AwsS3ServiceImplTests extends OpenSearchTestCase implements ConfigPathSupport {
     @AfterClass
     public static void shutdownIdleConnectionReaper() {
         // created by default STS client
@@ -64,7 +64,7 @@ public static void shutdownIdleConnectionReaper() {
 
     public void testAWSCredentialsDefaultToInstanceProviders() {
         final String inexistentClientName = randomAlphaOfLength(8).toLowerCase(Locale.ROOT);
-        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(Settings.EMPTY, inexistentClientName);
+        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(Settings.EMPTY, inexistentClientName, configPath());
         final AWSCredentialsProvider credentialsProvider = S3Service.buildCredentials(logger, clientSettings);
         assertThat(credentialsProvider, instanceOf(S3Service.PrivilegedInstanceProfileCredentialsProvider.class));
     }
@@ -79,7 +79,7 @@ public void testAWSCredentialsFromKeystore() {
             secureSettings.setString("s3.client." + clientName + ".secret_key", clientName + "_aws_secret_key");
         }
         final Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
-        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings, configPath());
         // no less, no more
         assertThat(allClientsSettings.size(), is(clientsCount + 1)); // including default
         for (int i = 0; i < clientsCount; i++) {
@@ -114,7 +114,7 @@ public void testCredentialsAndIrsaWithIdentityTokenFileCredentialsFromKeystore()
             plainSettings.put("s3.client." + clientName + ".identity_token_file", clientName + "_identity_token_file");
         }
         final Settings settings = Settings.builder().loadFromMap(plainSettings).setSecureSettings(secureSettings).build();
-        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings, configPath());
         // no less, no more
         assertThat(allClientsSettings.size(), is(clientsCount + 1)); // including default
         for (int i = 0; i < clientsCount; i++) {
@@ -148,7 +148,7 @@ public void testCredentialsAndIrsaCredentialsFromKeystore() throws IOException {
             plainSettings.put("s3.client." + clientName + ".region", "us-east1");
         }
         final Settings settings = Settings.builder().loadFromMap(plainSettings).setSecureSettings(secureSettings).build();
-        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings, configPath());
         // no less, no more
         assertThat(allClientsSettings.size(), is(clientsCount + 1)); // including default
         for (int i = 0; i < clientsCount; i++) {
@@ -175,7 +175,7 @@ public void testIrsaCredentialsFromKeystore() throws IOException {
             secureSettings.setString("s3.client." + clientName + ".role_session_name", clientName + "_role_session_name");
         }
         final Settings settings = Settings.builder().loadFromMap(plainSettings).setSecureSettings(secureSettings).build();
-        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings, configPath());
         // no less, no more
         assertThat(allClientsSettings.size(), is(clientsCount + 1)); // including default
         for (int i = 0; i < clientsCount; i++) {
@@ -198,7 +198,7 @@ public void testSetDefaultCredential() {
         secureSettings.setString("s3.client.default.access_key", awsAccessKey);
         secureSettings.setString("s3.client.default.secret_key", awsSecretKey);
         final Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
-        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings);
+        final Map<String, S3ClientSettings> allClientsSettings = S3ClientSettings.load(settings, configPath());
         assertThat(allClientsSettings.size(), is(1));
         // test default exists and is an Instance provider
         final S3ClientSettings defaultClientSettings = allClientsSettings.get("default");
@@ -218,7 +218,7 @@ public void testCredentialsIncomplete() {
             secureSettings.setString("s3.client." + clientName + ".secret_key", "aws_secret_key");
         }
         final Settings settings = Settings.builder().setSecureSettings(secureSettings).build();
-        final Exception e = expectThrows(IllegalArgumentException.class, () -> S3ClientSettings.load(settings));
+        final Exception e = expectThrows(IllegalArgumentException.class, () -> S3ClientSettings.load(settings, configPath()));
         if (missingOrMissing) {
             assertThat(e.getMessage(), containsString("Missing secret key for s3 client [" + clientName + "]"));
         } else {
@@ -308,7 +308,7 @@ public void testSocksProxyConfiguration() throws IOException {
             .put("s3.client.default.read_timeout", "10s")
             .build();
 
-        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default");
+        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default", configPath());
         final ClientConfiguration configuration = S3Service.buildConfiguration(clientSettings);
 
         assertEquals(Protocol.HTTPS, configuration.getProtocol());
@@ -342,7 +342,7 @@ private void launchAWSConfigurationTest(
         int expectedReadTimeout
     ) {
 
-        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default");
+        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, "default", configPath());
         final ClientConfiguration configuration = S3Service.buildConfiguration(clientSettings);
 
         assertThat(configuration.getResponseMetadataCacheSize(), is(0));
@@ -363,8 +363,7 @@ public void testEndpointSetting() {
 
     private void assertEndpoint(Settings repositorySettings, Settings settings, String expectedEndpoint) {
         final String configName = S3Repository.CLIENT_NAME.get(repositorySettings);
-        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, configName);
+        final S3ClientSettings clientSettings = S3ClientSettings.getClientSettings(settings, configName, configPath());
         assertThat(clientSettings.endpoint, is(expectedEndpoint));
     }
-
 }
diff --git a/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/ConfigPathSupport.java b/plugins/repository-s3/src/test/java/org/opensearch/repositories/s3/ConfigPathSupport.java
@@ -0,0 +1,22 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.repositories.s3;
+
+import org.opensearch.common.io.PathUtils;
+
+import java.nio.file.Path;
+
+/**
+ * The trait that adds the config path to the test cases
+ */
+interface ConfigPathSupport {
+    default Path configPath() {
+        return PathUtils.get("config");
+    }
+}
diff --git a/...epository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java b/...epository-s3/src/test/java/org/opensearch/repositories/s3/RepositoryCredentialsTests.java
@@ -57,6 +57,7 @@
 import org.opensearch.test.OpenSearchSingleNodeTestCase;
 import org.opensearch.test.rest.FakeRestRequest;
 
+import java.nio.file.Path;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.Collection;
@@ -284,8 +285,8 @@ private void createRepository(final String name, final Settings repositorySettin
      */
     public static final class ProxyS3RepositoryPlugin extends S3RepositoryPlugin {
 
-        public ProxyS3RepositoryPlugin(Settings settings) {
-            super(settings, new ProxyS3Service());
+        public ProxyS3RepositoryPlugin(Settings settings, Path configPath) {
+            super(settings, configPath, new ProxyS3Service(configPath));
         }
 
         @Override
@@ -316,6 +317,10 @@ public static final class ProxyS3Service extends S3Service {
 
             private static final Logger logger = LogManager.getLogger(ProxyS3Service.class);
 
+            ProxyS3Service(final Path configPath) {
+                super(configPath);
+            }
+
             @Override
             AmazonS3WithCredentials buildClient(final S3ClientSettings clientSettings) {
                 final AmazonS3WithCredentials client = super.buildClient(clientSettings);

diff --git a/...pository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java b/...pository-s3/src/test/java/org/opensearch/repositories/s3/S3BlobContainerRetriesTests.java
@@ -81,13 +81,13 @@
  * This class tests how a {@link S3BlobContainer} and its underlying AWS S3 client are retrying requests when reading or writing blobs.
  */
 @SuppressForbidden(reason = "use a http server")
-public class S3BlobContainerRetriesTests extends AbstractBlobContainerRetriesTestCase {
+public class S3BlobContainerRetriesTests extends AbstractBlobContainerRetriesTestCase implements ConfigPathSupport {
 
     private S3Service service;
 
     @Before
     public void setUp() throws Exception {
-        service = new S3Service();
+        service = new S3Service(configPath());
         super.setUp();
     }
 
@@ -140,7 +140,7 @@ protected BlobContainer createBlobContainer(
         secureSettings.setString(S3ClientSettings.ACCESS_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(), "access");
         secureSettings.setString(S3ClientSettings.SECRET_KEY_SETTING.getConcreteSettingForNamespace(clientName).getKey(), "secret");
         clientSettings.setSecureSettings(secureSettings);
-        service.refreshAndClearCache(S3ClientSettings.load(clientSettings.build()));
+        service.refreshAndClearCache(S3ClientSettings.load(clientSettings.build(), configPath()));
 
         final RepositoryMetadata repositoryMetadata = new RepositoryMetadata(
             "repository",