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.

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

Jump to bottom

[6.8 backport] Fix keystore thread safety (#12233) #12237

Merged
merged 1 commit into from Sep 11, 2020
Merged

[6.8 backport] Fix keystore thread safety (#12233) #12237

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 17 additions & 21 deletions logstash-core/src/main/java/org/logstash/secret/store/backend/JavaKeyStore.java
View file
Open in desktop
Expand Up @@ -27,8 +27,7 @@
import java.security.cert.CertificateException;
import java.util.*;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.concurrent.locks.ReentrantLock;

import static org.logstash.secret.store.SecretStoreFactory.LOGSTASH_MARKER;

Expand All @@ -46,9 +45,8 @@ public final class JavaKeyStore implements SecretStore {
private char[] keyStorePass;
private Path keyStorePath;
private ProtectionParameter protectionParameter;
private Lock readLock;
private Lock lock;
private boolean useDefaultPass = false;
private Lock writeLock;
//package private for testing
static String filePermissions = "rw-r--r--";
private static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
Expand All @@ -69,7 +67,7 @@ public JavaKeyStore create(SecureConfig config) {
}
try {
init(config);
writeLock.lock();
lock.lock();
LOGGER.debug("Creating new keystore at {}.", keyStorePath.toAbsolutePath());
String keyStorePermissions = filePermissions;
//create the keystore on disk with a default entry to identify this as a logstash keystore
Expand Down Expand Up @@ -100,7 +98,7 @@ public JavaKeyStore create(SecureConfig config) {
} catch (Exception e) { //should never happen
throw new SecretStoreException.UnknownException("Error while trying to create the Logstash keystore. ", e);
} finally {
releaseLock(writeLock);
releaseLock(lock);
config.clearValues();
}
}
Expand All @@ -109,7 +107,7 @@ public JavaKeyStore create(SecureConfig config) {
public void delete(SecureConfig config) {
try {
initLocks();
writeLock.lock();
lock.lock();
if (exists(config)) {
Files.delete(Paths.get(new String(config.getPlainText(PATH_KEY))));
}
Expand All @@ -118,7 +116,7 @@ public void delete(SecureConfig config) {
} catch (Exception e) { //should never happen
throw new SecretStoreException.UnknownException("Error while trying to delete the Logstash keystore", e);
} finally {
releaseLock(writeLock);
releaseLock(lock);
config.clearValues();
}
}
Expand Down Expand Up @@ -214,16 +212,14 @@ private void init(SecureConfig config) throws IOException, KeyStoreException {
}

private void initLocks(){
ReadWriteLock readWriteLock = new ReentrantReadWriteLock();
readLock = readWriteLock.readLock();
writeLock = readWriteLock.writeLock();
lock = new ReentrantLock();
}

@Override
public Collection<SecretIdentifier> list() {
Set<SecretIdentifier> identifiers = new HashSet<>();
try {
readLock.lock();
lock.lock();
loadKeyStore();
Enumeration<String> aliases = keyStore.aliases();
while (aliases.hasMoreElements()) {
Expand All @@ -233,7 +229,7 @@ public Collection<SecretIdentifier> list() {
} catch (Exception e) {
throw new SecretStoreException.ListException(e);
} finally {
releaseLock(readLock);
releaseLock(lock);
}
return identifiers;
}
Expand All @@ -255,7 +251,7 @@ public JavaKeyStore load(SecureConfig config) {
}
try {
init(config);
readLock.lock();
lock.lock();
try (final InputStream is = Files.newInputStream(keyStorePath)) {
try {
keyStore.load(is, this.keyStorePass);
Expand All @@ -282,7 +278,7 @@ public JavaKeyStore load(SecureConfig config) {
} catch (Exception e) { //should never happen
throw new SecretStoreException.UnknownException("Error while trying to load the Logstash keystore", e);
} finally {
releaseLock(readLock);
releaseLock(lock);
config.clearValues();
}
}
Expand All @@ -299,7 +295,7 @@ private void loadKeyStore() throws CertificateException, NoSuchAlgorithmExceptio
@Override
public void persistSecret(SecretIdentifier identifier, byte[] secret) {
try {
writeLock.lock();
lock.lock();
loadKeyStore();
SecretKeyFactory factory = SecretKeyFactory.getInstance("PBE");
//PBEKey requires an ascii password, so base64 encode it
Expand All @@ -317,22 +313,22 @@ public void persistSecret(SecretIdentifier identifier, byte[] secret) {
} catch (Exception e) {
throw new SecretStoreException.PersistException(identifier, e);
} finally {
releaseLock(writeLock);
releaseLock(lock);
}
}

@Override
public void purgeSecret(SecretIdentifier identifier) {
try {
writeLock.lock();
lock.lock();
loadKeyStore();
keyStore.deleteEntry(identifier.toExternalForm());
saveKeyStore();
LOGGER.debug("purged secret {}", identifier.toExternalForm());
} catch (Exception e) {
throw new SecretStoreException.PurgeException(identifier, e);
} finally {
releaseLock(writeLock);
releaseLock(lock);
}
}

Expand All @@ -346,7 +342,7 @@ private void releaseLock(Lock lock) {
public byte[] retrieveSecret(SecretIdentifier identifier) {
if (identifier != null && identifier.getKey() != null && !identifier.getKey().isEmpty()) {
try {
readLock.lock();
lock.lock();
loadKeyStore();
SecretKeyFactory factory = SecretKeyFactory.getInstance("PBE");
KeyStore.SecretKeyEntry secretKeyEntry = (KeyStore.SecretKeyEntry) keyStore.getEntry(identifier.toExternalForm(), protectionParameter);
Expand All @@ -365,7 +361,7 @@ public byte[] retrieveSecret(SecretIdentifier identifier) {
} catch (Exception e) {
throw new SecretStoreException.RetrievalException(identifier, e);
} finally {
releaseLock(readLock);
releaseLock(lock);
}
}
return null;
Expand Down
32 changes: 31 additions & 1 deletion logstash-core/src/test/java/org/logstash/secret/store/backend/JavaKeyStoreTest.java
View file
Open in desktop
Expand Up @@ -26,15 +26,18 @@
import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermission;
import java.util.*;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import static java.nio.file.attribute.PosixFilePermission.*;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Fail.fail;
import static org.hamcrest.CoreMatchers.isA;
import static org.logstash.secret.store.SecretStoreFactory.LOGSTASH_MARKER;

/**
Expand Down Expand Up @@ -669,4 +672,31 @@ public void wrongPassword() throws Exception {
withDefinedPassConfig.add(SecretStoreFactory.KEYSTORE_ACCESS_KEY, "wrongpassword".toCharArray());
new JavaKeyStore().load(withDefinedPassConfig);
}

@Test(timeout = 40_000)
public void concurrentReadTest() throws Exception {

final int KEYSTORE_COUNT = 250;

final ExecutorService executorService = Executors.newFixedThreadPool(KEYSTORE_COUNT);
String password = "pAssW3rd!";
keyStore.persistSecret(new SecretIdentifier("password"), password.getBytes(StandardCharsets.UTF_8));
try{
Callable<byte[]> reader = () -> keyStore.retrieveSecret(new SecretIdentifier("password"));

List<Future<byte[]>> futures = new ArrayList<>();
for (int i = 0; i < KEYSTORE_COUNT; i++) {
futures.add(executorService.submit(reader));
}

for (Future<byte[]> future : futures) {
byte[] result = future.get();
assertThat(result).isNotNull();
assertThat(new String(result, StandardCharsets.UTF_8)).isEqualTo(password);
}
} finally {
executorService.shutdownNow();
executorService.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
}
}
}