Skip to content

Fix MongoClient leak in auto-encryption #1142

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jun 12, 2023
Merged

Fix MongoClient leak in auto-encryption #1142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 13 additions & 7 deletions ...er-reactive-streams/src/main/com/mongodb/reactivestreams/client/internal/crypt/Crypt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ public class Crypt implements Closeable {
private final KeyManagementService keyManagementService;
private final boolean bypassAutoEncryption;
@Nullable
private final MongoClient internalClient;
private final MongoClient collectionInfoRetrieverClient;
@Nullable
private final MongoClient keyVaultClient;

/**
* Create an instance to use for explicit encryption and decryption, and data key creation.
Expand All @@ -81,7 +83,7 @@ public class Crypt implements Closeable {
final Map<String, Map<String, Object>> kmsProviders,
final Map<String, Supplier<Map<String, Object>>> kmsProviderPropertySuppliers) {
this(mongoCrypt, keyRetriever, keyManagementService, kmsProviders, kmsProviderPropertySuppliers,
false, null, null, null);
false, null, null, null, null);
}

/**
Expand All @@ -95,7 +97,8 @@ public class Crypt implements Closeable {
* @param bypassAutoEncryption the bypass auto encryption flag
* @param collectionInfoRetriever the collection info retriever
* @param commandMarker the command marker
* @param internalClient the internal mongo client
* @param collectionInfoRetrieverClient the collection info retriever mongo client
* @param keyVaultClient the key vault mongo client
*/
Crypt(final MongoCrypt mongoCrypt,
final KeyRetriever keyRetriever,
Expand All @@ -105,7 +108,8 @@ public class Crypt implements Closeable {
final boolean bypassAutoEncryption,
@Nullable final CollectionInfoRetriever collectionInfoRetriever,
@Nullable final CommandMarker commandMarker,
@Nullable final MongoClient internalClient) {
@Nullable final MongoClient collectionInfoRetrieverClient,
@Nullable final MongoClient keyVaultClient) {
this.mongoCrypt = mongoCrypt;
this.keyRetriever = keyRetriever;
this.keyManagementService = keyManagementService;
Expand All @@ -114,7 +118,8 @@ public class Crypt implements Closeable {
this.bypassAutoEncryption = bypassAutoEncryption;
this.collectionInfoRetriever = collectionInfoRetriever;
this.commandMarker = commandMarker;
this.internalClient = internalClient;
this.collectionInfoRetrieverClient = collectionInfoRetrieverClient;
this.keyVaultClient = keyVaultClient;
}

/**
Expand Down Expand Up @@ -227,8 +232,9 @@ public void close() {
//noinspection EmptyTryBlock
try (MongoCrypt ignored = this.mongoCrypt;
CommandMarker ignored1 = this.commandMarker;
MongoClient ignored2 = this.internalClient;
KeyManagementService ignored3 = this.keyManagementService
MongoClient ignored2 = this.collectionInfoRetrieverClient;
Copy link
Collaborator Author

@jyemin jyemin Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Depending on configuration, these two MongoClient instances may either

  • both be null
  • one null and the other non-null
  • both be non-null and reference different instances
  • both be non-null and reference the same instance

MongoClient ignored3 = this.keyVaultClient;
KeyManagementService ignored4 = this.keyManagementService
Copy link
Member

Choose a reason for hiding this comment

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

This is not related to the PR changes, but it's not obvious why the async KeyManagementService uses TlsChannelStreamFactoryFactory (and needs to be closed), while its sync counterpart does not. Do you happen to know anything about it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's because with async I/O you need a thread pool to handle the I/O completion, and for the library that we're using for async TLS there is no "system" (i.e. global) thread pool that is used (unlike with the JDK's AsynchrounousSocketChannel)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

) {
// just using try-with-resources to ensure they all get closed, even in the case of exceptions
}
Expand Down
13 changes: 7 additions & 6 deletions ...r-reactive-streams/src/main/com/mongodb/reactivestreams/client/internal/crypt/Crypts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,17 +42,17 @@ private Crypts() {
}

public static Crypt createCrypt(final MongoClientImpl client, final AutoEncryptionSettings settings) {
MongoClient internalClient = null;
MongoClient sharedInternalClient = null;
MongoClientSettings keyVaultMongoClientSettings = settings.getKeyVaultMongoClientSettings();
if (keyVaultMongoClientSettings == null || !settings.isBypassAutoEncryption()) {
MongoClientSettings mongoClientSettings = MongoClientSettings.builder(client.getSettings())
MongoClientSettings defaultInternalMongoClientSettings = MongoClientSettings.builder(client.getSettings())
.applyToConnectionPoolSettings(builder -> builder.minSize(0))
.autoEncryptionSettings(null)
.build();
internalClient = MongoClients.create(mongoClientSettings);
sharedInternalClient = MongoClients.create(defaultInternalMongoClientSettings);
}
MongoClient keyVaultClient = keyVaultMongoClientSettings == null
? internalClient : MongoClients.create(keyVaultMongoClientSettings);
? sharedInternalClient : MongoClients.create(keyVaultMongoClientSettings);
MongoCrypt mongoCrypt = MongoCrypts.create(createMongoCryptOptions(settings));
return new Crypt(
mongoCrypt,
Expand All @@ -61,9 +61,10 @@ public static Crypt createCrypt(final MongoClientImpl client, final AutoEncrypti
settings.getKmsProviders(),
settings.getKmsProviderPropertySuppliers(),
settings.isBypassAutoEncryption(),
settings.isBypassAutoEncryption() ? null : new CollectionInfoRetriever(internalClient),
settings.isBypassAutoEncryption() ? null : new CollectionInfoRetriever(sharedInternalClient),
new CommandMarker(mongoCrypt, settings),
internalClient);
sharedInternalClient,
keyVaultClient);
}

public static Crypt create(final MongoClient keyVaultClient, final ClientEncryptionSettings settings) {
Expand Down
19 changes: 13 additions & 6 deletions driver-sync/src/main/com/mongodb/client/internal/Crypt.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ public class Crypt implements Closeable {
private final KeyRetriever keyRetriever;
private final KeyManagementService keyManagementService;
private final boolean bypassAutoEncryption;
private final MongoClient internalClient;
@Nullable
private final MongoClient collectionInfoRetrieverClient;
@Nullable
private final MongoClient keyVaultClient;


/**
Expand All @@ -81,7 +84,7 @@ public class Crypt implements Closeable {
final Map<String, Map<String, Object>> kmsProviders,
final Map<String, Supplier<Map<String, Object>>> kmsProviderPropertySuppliers) {
this(mongoCrypt, keyRetriever, keyManagementService, kmsProviders, kmsProviderPropertySuppliers,
false, null, null, null);
false, null, null, null, null);
}

/**
Expand All @@ -95,7 +98,8 @@ public class Crypt implements Closeable {
* @param bypassAutoEncryption the bypass auto encryption flag
* @param collectionInfoRetriever the collection info retriever
* @param commandMarker the command marker
* @param internalClient the internal mongo client
* @param collectionInfoRetrieverClient the collection info retriever mongo client
* @param keyVaultClient the key vault mongo client
*/
Crypt(final MongoCrypt mongoCrypt,
final KeyRetriever keyRetriever,
Expand All @@ -105,7 +109,8 @@ public class Crypt implements Closeable {
final boolean bypassAutoEncryption,
@Nullable final CollectionInfoRetriever collectionInfoRetriever,
@Nullable final CommandMarker commandMarker,
@Nullable final MongoClient internalClient) {
@Nullable final MongoClient collectionInfoRetrieverClient,
@Nullable final MongoClient keyVaultClient) {
this.mongoCrypt = mongoCrypt;
this.keyRetriever = keyRetriever;
this.keyManagementService = keyManagementService;
Expand All @@ -114,7 +119,8 @@ public class Crypt implements Closeable {
this.bypassAutoEncryption = bypassAutoEncryption;
this.collectionInfoRetriever = collectionInfoRetriever;
this.commandMarker = commandMarker;
this.internalClient = internalClient;
this.collectionInfoRetrieverClient = collectionInfoRetrieverClient;
this.keyVaultClient = keyVaultClient;
}

/**
Expand Down Expand Up @@ -260,7 +266,8 @@ public void close() {
//noinspection EmptyTryBlock
try (MongoCrypt ignored = this.mongoCrypt;
CommandMarker ignored1 = this.commandMarker;
MongoClient ignored2 = this.internalClient
MongoClient ignored2 = this.collectionInfoRetrieverClient;
MongoClient ignored3 = this.keyVaultClient
) {
// just using try-with-resources to ensure they all get closed, even in the case of exceptions
}
Expand Down
12 changes: 6 additions & 6 deletions driver-sync/src/main/com/mongodb/client/internal/Crypts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,17 @@
public final class Crypts {

public static Crypt createCrypt(final MongoClientImpl client, final AutoEncryptionSettings settings) {
MongoClient internalClient = null;
MongoClient sharedInternalClient = null;
MongoClientSettings keyVaultMongoClientSettings = settings.getKeyVaultMongoClientSettings();
if (keyVaultMongoClientSettings == null || !settings.isBypassAutoEncryption()) {
MongoClientSettings mongoClientSettings = MongoClientSettings.builder(client.getSettings())
MongoClientSettings defaultInternalMongoClientSettings = MongoClientSettings.builder(client.getSettings())
.applyToConnectionPoolSettings(builder -> builder.minSize(0))
.autoEncryptionSettings(null)
.build();
internalClient = MongoClients.create(mongoClientSettings);
sharedInternalClient = MongoClients.create(defaultInternalMongoClientSettings);
}
MongoClient keyVaultClient = keyVaultMongoClientSettings == null
? internalClient : MongoClients.create(keyVaultMongoClientSettings);
? sharedInternalClient : MongoClients.create(keyVaultMongoClientSettings);
MongoCrypt mongoCrypt = MongoCrypts.create(createMongoCryptOptions(settings));
return new Crypt(
mongoCrypt,
Expand All @@ -55,9 +55,9 @@ public static Crypt createCrypt(final MongoClientImpl client, final AutoEncrypti
settings.getKmsProviders(),
settings.getKmsProviderPropertySuppliers(),
settings.isBypassAutoEncryption(),
settings.isBypassAutoEncryption() ? null : new CollectionInfoRetriever(internalClient),
settings.isBypassAutoEncryption() ? null : new CollectionInfoRetriever(sharedInternalClient),
new CommandMarker(mongoCrypt, settings),
internalClient);
sharedInternalClient, keyVaultClient);
}

static Crypt create(final MongoClient keyVaultClient, final ClientEncryptionSettings settings) {
Expand Down