Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Opensearch repository-s3 plugin cannot read ServiceAccount token #6390

Merged
merged 2 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If anyone else reading this is wondering how we can change a public constructor without changing any calling code, the answer is that the PluginsService expects one of three different constructor signatures and this one of them.

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 @@ -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
Expand All @@ -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));
}
Expand All @@ -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++) {
Expand Down Expand Up @@ -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++) {
Expand Down Expand Up @@ -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++) {
Expand All @@ -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++) {
Expand All @@ -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");
Expand All @@ -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 {
Expand Down Expand Up @@ -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());
Expand Down Expand Up @@ -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));
Expand All @@ -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));
}

}
Original file line number Diff line number Diff line change
@@ -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");
}
}
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 @@ -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();
}

Expand Down Expand Up @@ -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",
Expand Down
Loading