Skip to content

Commit

Permalink
Fix IdentityClient security permissions, get rid of connection string…
Browse files Browse the repository at this point in the history 
… (since it is not applicable to managed identity configuration)

Signed-off-by: Andriy Redko <[email protected]>
  • Loading branch information
@reta @chengwushi-netapp
reta authored and chengwushi-netapp committed May 14, 2024
1 parent 3d34c8e commit d15d2fb
Show file tree
Hide file tree
Showing 6 changed files with 315 additions and 227 deletions.
2 changes: 1 addition & 1 deletion plugins/repository-azure/src/main/java/org/opensearch/repositories/azure/AzureBlobStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public BlobContainer blobContainer(BlobPath path) {
}

@Override
public void close() {
public void close() throws IOException {
service.close();
}

Expand Down
54 changes: 50 additions & 4 deletions ...repository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,19 @@
import org.opensearch.core.common.unit.ByteSizeUnit;
import org.opensearch.core.common.unit.ByteSizeValue;

import java.io.IOException;
import java.net.Authenticator;
import java.net.PasswordAuthentication;
import java.net.URISyntaxException;
import java.security.AccessController;
import java.security.InvalidKeyException;
import java.security.PrivilegedAction;
import java.time.Duration;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
Expand Down Expand Up @@ -98,6 +103,37 @@ public class AzureStorageService implements AutoCloseable {
// 'package' for testing
volatile Map<String, AzureStorageSettings> storageSettings = emptyMap();
private final Map<AzureStorageSettings, ClientState> clients = new ConcurrentHashMap<>();
private final ExecutorService executor;

private static final class IdentityClientThreadFactory implements ThreadFactory {
final ThreadGroup group;
final AtomicInteger threadNumber = new AtomicInteger(1);
final String namePrefix;

@SuppressWarnings("removal")
IdentityClientThreadFactory(String namePrefix) {
this.namePrefix = namePrefix;
SecurityManager s = System.getSecurityManager();
group = (s != null) ? s.getThreadGroup() : Thread.currentThread().getThreadGroup();
}

@Override
public Thread newThread(Runnable r) {
Thread t = new Thread(group, new Runnable() {
@SuppressWarnings("removal")
public void run() {
AccessController.doPrivileged(new PrivilegedAction<>() {
public Void run() {
r.run();
return null;
}
});
}
}, namePrefix + "[T#" + threadNumber.getAndIncrement() + "]", 0);
t.setDaemon(true);
return t;
}
}

static {
// See please:
Expand All @@ -111,6 +147,9 @@ public AzureStorageService(Settings settings) {
// eagerly load client settings so that secure settings are read
final Map<String, AzureStorageSettings> clientsSettings = AzureStorageSettings.load(settings);
refreshAndClearCache(clientsSettings);
executor = SocketAccess.doPrivilegedException(
() -> Executors.newCachedThreadPool(new IdentityClientThreadFactory("azure-identity-client"))
);
}

/**
Expand Down Expand Up @@ -244,9 +283,8 @@ private BlobServiceClientBuilder applyLocationMode(final BlobServiceClientBuilde
return builder;
}

private static BlobServiceClientBuilder createClientBuilder(AzureStorageSettings settings) throws InvalidKeyException,
URISyntaxException {
return SocketAccess.doPrivilegedException(() -> settings.configure(new BlobServiceClientBuilder()));
private BlobServiceClientBuilder createClientBuilder(AzureStorageSettings settings) throws InvalidKeyException, URISyntaxException {
return SocketAccess.doPrivilegedException(() -> settings.configure(new BlobServiceClientBuilder(), executor, logger));
}

/**
Expand Down Expand Up @@ -292,9 +330,17 @@ public Map<String, AzureStorageSettings> refreshAndClearCache(Map<String, AzureS
}

@Override
public void close() {
public void close() throws IOException {
this.clients.values().forEach(this::closeInternally);
this.clients.clear();
this.executor.shutdown();
try {
if (this.executor.awaitTermination(30, TimeUnit.SECONDS) == false) {
logger.warning("The executor was not shutdown gracefuly with 30 seconds");
}
} catch (final InterruptedException ex) {
throw new IOException(ex);
}
}

public Duration getBlobRequestTimeout(String clientName) {
Expand Down
76 changes: 43 additions & 33 deletions ...epository-azure/src/main/java/org/opensearch/repositories/azure/AzureStorageSettings.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,15 @@
package org.opensearch.repositories.azure;

import com.azure.core.util.logging.ClientLogger;
import com.azure.identity.ManagedIdentityCredential;
import com.azure.identity.ManagedIdentityCredentialBuilder;
import com.azure.identity.implementation.CredentialBuilderBaseHelper;
import com.azure.storage.blob.BlobServiceClientBuilder;
import com.azure.storage.common.implementation.Constants;
import com.azure.storage.common.implementation.connectionstring.StorageConnectionString;
import com.azure.storage.common.implementation.connectionstring.StorageEndpoint;
import org.opensearch.common.Nullable;
import org.opensearch.common.TriFunction;
import org.opensearch.common.collect.MapBuilder;
import org.opensearch.common.settings.SecureSetting;
import org.opensearch.common.settings.Setting;
Expand All @@ -57,6 +60,7 @@
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.ExecutorService;
import java.util.function.Function;

final class AzureStorageSettings {
Expand Down Expand Up @@ -90,7 +94,7 @@ final class AzureStorageSettings {
AZURE_CLIENT_PREFIX_KEY,
"token_credential_type",
key -> Setting.simpleString(key, value -> {
if (usesTokenCredential(value)) {
if (Strings.hasText(value) == true) {
TokenCredentialType.valueOfType(value);
}
}, Property.NodeScope),
Expand Down Expand Up @@ -214,9 +218,9 @@ final class AzureStorageSettings {
);

private final String account;
private final String connectString;
private final String tokenCredentialType;
private final Function<BlobServiceClientBuilder, BlobServiceClientBuilder> clientBuilder;
private final TriFunction<BlobServiceClientBuilder, ExecutorService, ClientLogger, BlobServiceClientBuilder> clientBuilder;
private final Function<ClientLogger, StorageEndpoint> endpointBuilder;
private final String endpointSuffix;
private final TimeValue timeout;
private final int maxRetries;
Expand All @@ -230,9 +234,9 @@ final class AzureStorageSettings {
// copy-constructor
private AzureStorageSettings(
String account,
String connectString,
String tokenCredentialType,
Function<BlobServiceClientBuilder, BlobServiceClientBuilder> clientBuilder,
TriFunction<BlobServiceClientBuilder, ExecutorService, ClientLogger, BlobServiceClientBuilder> clientBuilder,
Function<ClientLogger, StorageEndpoint> endpointBuilder,
String endpointSuffix,
TimeValue timeout,
int maxRetries,
Expand All @@ -244,9 +248,9 @@ private AzureStorageSettings(
ProxySettings proxySettings
) {
this.account = account;
this.connectString = connectString;
this.tokenCredentialType = tokenCredentialType;
this.clientBuilder = clientBuilder;
this.endpointBuilder = endpointBuilder;
this.endpointSuffix = endpointSuffix;
this.timeout = timeout;
this.maxRetries = maxRetries;
Expand Down Expand Up @@ -274,13 +278,35 @@ private AzureStorageSettings(
) {
this.account = account;
this.tokenCredentialType = tokenCredentialType;
if (usesTokenCredential(tokenCredentialType)) {
this.connectString = "";
this.clientBuilder = (builder) -> builder.credential(new ManagedIdentityCredentialBuilder().build())
.endpoint(getStorageEndpoint(null).getPrimaryUri());
if (Strings.hasText(tokenCredentialType) == true) {
this.endpointBuilder = (logger) -> {
String tokenCredentialEndpointSuffix = endpointSuffix;
if (Strings.hasText(tokenCredentialEndpointSuffix) == false) {
// Default to "core.windows.net".
tokenCredentialEndpointSuffix = Constants.ConnectionStringConstants.DEFAULT_DNS;
}
final URI primaryBlobEndpoint = URI.create("https://" + account + ".blob." + tokenCredentialEndpointSuffix);
final URI secondaryBlobEndpoint = URI.create("https://" + account + "-secondary.blob." + tokenCredentialEndpointSuffix);
return new StorageEndpoint(primaryBlobEndpoint, secondaryBlobEndpoint);
};

this.clientBuilder = (builder, executor, logger) -> builder.credential(new ManagedIdentityCredentialBuilder() {
@Override
public ManagedIdentityCredential build() {
// Use the privileged executor with IdentityClient instance
CredentialBuilderBaseHelper.getClientOptions(this).setExecutorService(executor);
return super.build();
}
}.build()).endpoint(endpointBuilder.apply(logger).getPrimaryUri());
} else {
this.connectString = buildConnectString(account, key, sasToken, endpointSuffix);
this.clientBuilder = (builder) -> builder.connectionString(connectString);
final String connectString = buildConnectString(account, key, sasToken, endpointSuffix);

this.endpointBuilder = (logger) -> {
final StorageConnectionString storageConnectionString = StorageConnectionString.create(connectString, logger);
return storageConnectionString.getBlobEndpoint();
};

this.clientBuilder = (builder, executor, logger) -> builder.connectionString(connectString);
}
this.endpointSuffix = endpointSuffix;
this.timeout = timeout;
Expand All @@ -297,24 +323,8 @@ public String getTokenCredentialType() {
return tokenCredentialType;
}

private static Boolean usesTokenCredential(String tokenCredential) {
return tokenCredential != null && !tokenCredential.isEmpty();
}

public StorageEndpoint getStorageEndpoint(@Nullable ClientLogger logger) {
if (usesTokenCredential(tokenCredentialType)) {
String tokenCredentialEndpointSuffix = endpointSuffix;
if (!Strings.hasText(tokenCredentialEndpointSuffix)) {
// Default to "core.windows.net".
tokenCredentialEndpointSuffix = Constants.ConnectionStringConstants.DEFAULT_DNS;
}
final URI primaryBlobEndpoint = URI.create("https://" + account + ".blob." + tokenCredentialEndpointSuffix);
final URI secondaryBlobEndpoint = URI.create("https://" + account + "-secondary.blob." + tokenCredentialEndpointSuffix);
return new StorageEndpoint(primaryBlobEndpoint, secondaryBlobEndpoint);
} else {
final StorageConnectionString storageConnectionString = StorageConnectionString.create(connectString, logger);
return storageConnectionString.getBlobEndpoint();
}
public StorageEndpoint getStorageEndpoint(ClientLogger logger) {
return endpointBuilder.apply(logger);
}

public String getEndpointSuffix() {
Expand Down Expand Up @@ -487,9 +497,9 @@ static Map<String, AzureStorageSettings> overrideLocationMode(
entry.getKey(),
new AzureStorageSettings(
entry.getValue().account,
entry.getValue().connectString,
entry.getValue().tokenCredentialType,
entry.getValue().clientBuilder,
entry.getValue().endpointBuilder,
entry.getValue().endpointSuffix,
entry.getValue().timeout,
entry.getValue().maxRetries,
Expand All @@ -505,7 +515,7 @@ static Map<String, AzureStorageSettings> overrideLocationMode(
return mapBuilder.immutableMap();
}

public BlobServiceClientBuilder configure(BlobServiceClientBuilder builder) {
return clientBuilder.apply(builder);
public BlobServiceClientBuilder configure(BlobServiceClientBuilder builder, ExecutorService executor, ClientLogger logger) {
return clientBuilder.apply(builder, executor, logger);
}
}
3 changes: 3 additions & 0 deletions plugins/repository-azure/src/main/plugin-metadata/plugin-security.policy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,4 +41,7 @@ grant {

// azure client set Authenticator for proxy username/password
permission java.net.NetPermission "setDefaultAuthenticator";

// azure identity
permission java.util.PropertyPermission "os.name", "read";
};
Loading

0 comments on commit d15d2fb

Please sign in to comment.