Skip to content

Commit

Permalink
Opensearch repository-s3 plugin cannot read ServiceAccount token
Browse files Browse the repository at this point in the history
Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
reta committed Feb 20, 2023
1 parent 70d25ef commit ac56ce8
Show file tree
Hide file tree
Showing 11 changed files with 160 additions and 62 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)
Expand All @@ -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()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@
import com.amazonaws.http.IdleConnectionReaper;

import org.junit.AfterClass;
import org.opensearch.common.io.PathUtils;
import org.opensearch.common.settings.MockSecureSettings;
import org.opensearch.common.settings.Settings;
import org.opensearch.test.OpenSearchTestCase;

import java.io.Closeable;
import java.io.IOException;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
Expand All @@ -64,7 +66,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));
}
Expand All @@ -79,7 +81,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++) {
Expand Down Expand Up @@ -114,7 +116,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++) {
Expand Down Expand Up @@ -148,7 +150,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++) {
Expand All @@ -175,7 +177,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++) {
Expand All @@ -198,7 +200,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");
Expand All @@ -218,7 +220,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 {
Expand Down Expand Up @@ -308,7 +310,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());
Expand Down Expand Up @@ -342,7 +344,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));
Expand All @@ -363,8 +365,11 @@ 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));
}

private Path configPath() {
return PathUtils.get("config");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.opensearch.common.blobstore.BlobContainer;
import org.opensearch.common.blobstore.BlobPath;
import org.opensearch.common.bytes.BytesReference;
import org.opensearch.common.io.PathUtils;
import org.opensearch.common.io.Streams;
import org.opensearch.common.lucene.store.ByteArrayIndexInput;
import org.opensearch.common.lucene.store.InputStreamIndexInput;
Expand All @@ -63,6 +64,7 @@
import java.net.InetSocketAddress;
import java.net.SocketTimeoutException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Path;
import java.util.Locale;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;
Expand All @@ -87,7 +89,7 @@ public class S3BlobContainerRetriesTests extends AbstractBlobContainerRetriesTes

@Before
public void setUp() throws Exception {
service = new S3Service();
service = new S3Service(configPath());
super.setUp();
}

Expand Down Expand Up @@ -140,7 +142,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",
Expand Down Expand Up @@ -401,4 +403,8 @@ public void close() throws IOException {
}
}
}

private Path configPath() {
return PathUtils.get("config");
}
}
Loading

0 comments on commit ac56ce8

Please sign in to comment.