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

Added timeout for fetching windows keystores #445

Closed
wants to merge 12 commits into from
Closed

Added timeout for fetching windows keystores #445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7249e48
Added timeout and excluded keystore types
Hakky54 Jan 20, 2024
166a4b6
Make tests passing again
Hakky54 Jan 20, 2024
25e8a51
Make tests passing again
Hakky54 Jan 20, 2024
314a4bd
Increased time-out
Hakky54 Jan 20, 2024
70baaa6
Decreased time-out
Hakky54 Jan 20, 2024
9ab92fa
Decreased time-out
Hakky54 Jan 20, 2024
ddf967d
Added test
Hakky54 Jan 20, 2024
c7deca9
Fix sonar issue
Hakky54 Jan 20, 2024
230532e
Added parallel option and remove excluded keystore types
Hakky54 Jan 21, 2024
b8c4b7d
Removed parallel option
Hakky54 Jan 21, 2024
3db7fe8
Eliminated duplicate code
Hakky54 Jan 21, 2024
1950580
Improved coverage
Hakky54 Jan 21, 2024
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
26 changes: 17 additions & 9 deletions sslcontext-kickstart/src/main/java/nl/altindag/ssl/util/KeyStoreUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeUnit;
import java.util.function.UnaryOperator;
import java.util.stream.Stream;

Expand Down Expand Up @@ -230,7 +232,7 @@ public static List<KeyStore> loadSystemKeyStores() {
}
case WINDOWS: {
Stream.of("Windows-ROOT", "Windows-ROOT-LOCALMACHINE", "Windows-ROOT-CURRENTUSER", "Windows-MY", "Windows-MY-CURRENTUSER", "Windows-MY-LOCALMACHINE")
.map(keystoreType -> createKeyStoreIfAvailable(keystoreType, null))
.map(keyStoreType -> createKeyStoreIfAvailable(keyStoreType, null))
.filter(Optional::isPresent)
.map(Optional::get)
.forEach(keyStores::add);
Expand All @@ -257,15 +259,21 @@ public static List<KeyStore> loadSystemKeyStores() {
@SuppressWarnings("SameParameterValue")
static Optional<KeyStore> createKeyStoreIfAvailable(String keyStoreType, char[] keyStorePassword) {
try {
KeyStore keyStore = createKeyStore(keyStoreType, keyStorePassword);

if (LOGGER.isDebugEnabled()) {
int totalTrustedCertificates = countAmountOfTrustMaterial(keyStore);
LOGGER.debug("Successfully loaded KeyStore of the type [{}] having [{}] entries", keyStoreType, totalTrustedCertificates);
return CompletableFuture.supplyAsync(() -> createKeyStore(keyStoreType, keyStorePassword))
.thenApply(keyStore -> {
if (LOGGER.isDebugEnabled()) {
int totalTrustedCertificates = countAmountOfTrustMaterial(keyStore);
LOGGER.debug("Successfully loaded KeyStore of the type [{}] having [{}] entries", keyStoreType, totalTrustedCertificates);
}
return keyStore;
})
.thenApply(Optional::of)
.get(500, TimeUnit.MILLISECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

The timeout will make the CompletableFuture complete exceptionally with a TimeoutException, but the underlying thread will probably stay blocked.
After multiple calls, it may create a thread leak (supplyAsync uses the ForkJoin pool, which is not limited in the number of threads AFAIR).

If you want more control, you should not use CompletableFuture, but your own thread, and attempt to interrupt it after the timeout. But I am not even sure it would work, because the stacktrace showed that the process was blocked in native code (sun.security.mscapi.CKeyStore.loadKeysOrCertificateChains) and I have no idea if this could be interrupted.

Copy link
Owner Author

@Hakky54 Hakky54 Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, you might be correct indeed. The underlying thread can still be active...
I will investigate whether the native code can be interrupted or else this PR will not resolve the issue at all.
Thank you for reviewing this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correcting myself :

The timeout will make the CompletableFuture complete exceptionally with a TimeoutException

this is inaccurate. get(timeout) will throw a TimeoutException if the time is exceeded, but the CompletableFuture will still be not completed.

You could be tempted to do:

   try {
      future.get(timeout);
   } catch (TimeoutException e) {
      future.cancel(true);
   }

The problem is that the "interruptIfRunning" flag is not used by the completable future framework. And the question about the actual effect of interrupting a native method call remains anyway.

} catch (Exception exception) {
if (exception instanceof InterruptedException) {
Thread.currentThread().interrupt();
}
return Optional.of(keyStore);
} catch (Exception ignored) {
LOGGER.debug("Failed to load KeyStore of the type [{}]", keyStoreType);
LOGGER.debug(String.format("Failed to load KeyStore of the type [%s]", keyStoreType), exception);
return Optional.empty();
}
}
Expand Down
44 changes: 44 additions & 0 deletions sslcontext-kickstart/src/test/java/nl/altindag/ssl/MockUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* Copyright 2019 Thunderberry.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package nl.altindag.ssl;

import org.mockito.MockedStatic;
import org.mockito.stubbing.Answer;

import java.util.concurrent.CompletableFuture;
import java.util.concurrent.Executor;
import java.util.function.Supplier;

import static org.mockito.ArgumentMatchers.any;

/**
* @author Hakan Altindag
*/
public final class MockUtils {

private MockUtils() {}

@SuppressWarnings("rawtypes")
public static void supplyAsyncOnCurrentThread(MockedStatic<CompletableFuture> mockCompletableFuture) {
mockCompletableFuture.when(() -> CompletableFuture.supplyAsync(any()))
.thenAnswer((Answer<CompletableFuture<?>>) invocation -> {
Executor currentThread = Runnable::run;
Supplier<?> supplier = invocation.getArgument(0);
return CompletableFuture.supplyAsync(supplier, currentThread);
});
}

}
33 changes: 19 additions & 14 deletions sslcontext-kickstart/src/test/java/nl/altindag/ssl/util/CertificateUtilsShould.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
package nl.altindag.ssl.util;

import nl.altindag.log.LogCaptor;
import nl.altindag.ssl.MockUtils;
import nl.altindag.ssl.TestConstants;
import nl.altindag.ssl.exception.GenericCertificateException;
import nl.altindag.ssl.exception.GenericIOException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.MockedConstruction;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.junit.jupiter.MockitoExtension;

Expand Down Expand Up @@ -50,6 +52,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.CompletableFuture;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

Expand Down Expand Up @@ -429,20 +432,22 @@ void getSystemTrustedCertificates() {

try (MockedStatic<MacCertificateUtils> macCertificateUtilsMockedStatic = mockStatic(MacCertificateUtils.class);
MockedStatic<KeyStoreUtils> keyStoreUtilsMockedStatic = mockStatic(KeyStoreUtils.class, invocation -> {
Method method = invocation.getMethod();
if ("createKeyStore".equals(method.getName())
&& method.getParameterCount() == 2
&& operatingSystem.contains("mac")) {
return KeyStoreUtils.loadKeyStore(KEYSTORE_LOCATION + TRUSTSTORE_FILE_NAME, TRUSTSTORE_PASSWORD);
} else if ("createTrustStore".equals(method.getName())
&& method.getParameterCount() == 1
&& method.getParameters()[0].getType().equals(List.class)
&& operatingSystem.contains("mac")) {
return KeyStoreUtils.loadKeyStore(KEYSTORE_LOCATION + "truststore-without-password.jks", null);
} else {
return invocation.callRealMethod();
}
})) {
Method method = invocation.getMethod();
if ("createKeyStore".equals(method.getName())
&& method.getParameterCount() == 2
&& operatingSystem.contains("mac")) {
return KeyStoreUtils.loadKeyStore(KEYSTORE_LOCATION + TRUSTSTORE_FILE_NAME, TRUSTSTORE_PASSWORD);
} else if ("createTrustStore".equals(method.getName())
&& method.getParameterCount() == 1
&& method.getParameters()[0].getType().equals(List.class)
&& operatingSystem.contains("mac")) {
return KeyStoreUtils.loadKeyStore(KEYSTORE_LOCATION + "truststore-without-password.jks", null);
} else {
return invocation.callRealMethod();
}
}); MockedStatic<CompletableFuture> mockCompletableFuture = mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS)) {
MockUtils.supplyAsyncOnCurrentThread(mockCompletableFuture);

List<X509Certificate> certificates = CertificateUtils.getSystemTrustedCertificates();
if (operatingSystem.contains("mac") || operatingSystem.contains("windows") || operatingSystem.contains("linux")) {
assertThat(certificates).isNotEmpty();
Expand Down
87 changes: 72 additions & 15 deletions sslcontext-kickstart/src/test/java/nl/altindag/ssl/util/KeyStoreUtilsShould.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,14 @@

import nl.altindag.log.LogCaptor;
import nl.altindag.ssl.IOTestUtils;
import nl.altindag.ssl.MockUtils;
import nl.altindag.ssl.SSLFactory;
import nl.altindag.ssl.TestConstants;
import nl.altindag.ssl.exception.GenericKeyStoreException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;

import javax.net.ssl.X509ExtendedTrustManager;
Expand All @@ -46,6 +48,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
Expand All @@ -63,6 +66,7 @@
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -156,24 +160,28 @@ void loadWindowsSystemKeyStore() {
Method method = invocation.getMethod();
if ("loadSystemKeyStores".equals(method.getName()) && method.getParameterCount() == 0) {
return invocation.callRealMethod();
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT".equals(invocation.getArgument(0))) {
return Optional.of(windowsRootKeyStore);
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY".equals(invocation.getArgument(0))) {
return Optional.of(windowsMyKeyStore);
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY-CURRENTUSER".equals(invocation.getArgument(0))) {
return Optional.of(windowsMyCurrentUserKeyStore);
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY-LOCALMACHINE".equals(invocation.getArgument(0))) {
return Optional.of(windowsMyLocalmachineKeyStore);
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT-LOCALMACHINE".equals(invocation.getArgument(0))) {
return Optional.of(windowsRootLocalmachineKeyStore);
} else if ("createKeyStoreIfAvailable".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT-CURRENTUSER".equals(invocation.getArgument(0))) {
return Optional.of(windowsRootCurrentUserKeyStore);
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT".equals(invocation.getArgument(0))) {
return windowsRootKeyStore;
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY".equals(invocation.getArgument(0))) {
return windowsMyKeyStore;
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY-CURRENTUSER".equals(invocation.getArgument(0))) {
return windowsMyCurrentUserKeyStore;
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-MY-LOCALMACHINE".equals(invocation.getArgument(0))) {
return windowsMyLocalmachineKeyStore;
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT-LOCALMACHINE".equals(invocation.getArgument(0))) {
return windowsRootLocalmachineKeyStore;
} else if ("createKeyStore".equals(method.getName()) && method.getParameterCount() == 2 && "Windows-ROOT-CURRENTUSER".equals(invocation.getArgument(0))) {
return windowsRootCurrentUserKeyStore;
} else if ("countAmountOfTrustMaterial".equals(method.getName())) {
return 2;
} else if ("createKeyStoreIfAvailable".equals(method.getName())) {
return invocation.callRealMethod();
} else {
return invocation.getMock();
}
})) {
}); MockedStatic<CompletableFuture> mockCompletableFuture = mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS)) {
MockUtils.supplyAsyncOnCurrentThread(mockCompletableFuture);

List<KeyStore> keyStores = KeyStoreUtils.loadSystemKeyStores();
assertThat(keyStores).containsExactlyInAnyOrder(windowsRootKeyStore, windowsMyKeyStore, windowsMyCurrentUserKeyStore, windowsMyLocalmachineKeyStore, windowsRootCurrentUserKeyStore, windowsRootLocalmachineKeyStore);
assertThat(logCaptor.getDebugLogs()).contains("Loaded [12] system trusted certificates");
Expand Down Expand Up @@ -679,6 +687,7 @@ void createKeyStoreIfAvailableReturnsEmptyForNonExistingKeyStoreType() {
}

@Test
@SuppressWarnings("rawtypes")
void createKeyStoreIfAvailableReturnsFilledKeyStore() {
LogCaptor logCaptor = LogCaptor.forClass(KeyStoreUtils.class);

Expand All @@ -693,14 +702,17 @@ void createKeyStoreIfAvailableReturnsFilledKeyStore() {
} else {
return invocation.callRealMethod();
}
})) {
}); MockedStatic<CompletableFuture> mockCompletableFuture = mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS)) {
MockUtils.supplyAsyncOnCurrentThread(mockCompletableFuture);

Optional<KeyStore> keyStore = KeyStoreUtils.createKeyStoreIfAvailable("Banana", null);
assertThat(keyStore).isPresent();
assertThat(logCaptor.getDebugLogs()).contains("Successfully loaded KeyStore of the type [Banana] having [2] entries");
}
}

@Test
@SuppressWarnings("rawtypes")
void createKeyStoreIfAvailableReturnsFilledKeyStoreWithoutLoggingIfDebugIsDisabled() {
LogCaptor logCaptor = LogCaptor.forClass(KeyStoreUtils.class);
logCaptor.setLogLevelToInfo();
Expand All @@ -716,7 +728,9 @@ void createKeyStoreIfAvailableReturnsFilledKeyStoreWithoutLoggingIfDebugIsDisabl
} else {
return invocation.callRealMethod();
}
})) {
}); MockedStatic<CompletableFuture> mockCompletableFuture = mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS)) {
MockUtils.supplyAsyncOnCurrentThread(mockCompletableFuture);

Optional<KeyStore> keyStore = KeyStoreUtils.createKeyStoreIfAvailable("Banana", null);
assertThat(keyStore).isPresent();
assertThat(logCaptor.getDebugLogs()).isEmpty();
Expand Down Expand Up @@ -832,6 +846,49 @@ void throwGenericKeyStoreWhenIsCertificateEntryThrowsKeyStoreExceptionForMethodG
}
}

@Test
@SuppressWarnings("rawtypes")
void interruptCurrentThreadIfFutureGetsInterrupted() {
KeyStore mockedKeyStore = mock(KeyStore.class);
CompletableFuture<KeyStore> completableFuture = spy(CompletableFuture.completedFuture(mockedKeyStore));

try (MockedStatic<CompletableFuture> mockStatic = mockStatic(CompletableFuture.class, invocationOnMock -> {
Method method = invocationOnMock.getMethod();
if ("supplyAsync".equals(method.getName())) {
return completableFuture;
} else if ("reportGet".equalsIgnoreCase(method.getName())) {
throw new InterruptedException();
} else {
return invocationOnMock.callRealMethod();
}
})) {
Optional<KeyStore> keyStore = KeyStoreUtils.createKeyStoreIfAvailable("PKCS12", null);
assertThat(keyStore).isEmpty();
assertThat(Thread.interrupted()).isTrue();
}
}

@Test
void throwGenericKeyStoreWhenIsCertificateEntryThrowsKeyStoreExceptionForMethodCountAmountOfTrustMaterial() throws KeyStoreException {
KeyStore keyStore = mock(KeyStore.class);
doThrow(new KeyStoreException("some-alias")).when(keyStore).isCertificateEntry(anyString());
when(keyStore.aliases()).thenReturn(Collections.enumeration(Arrays.asList("some-alias")));

assertThatThrownBy(() -> KeyStoreUtils.countAmountOfTrustMaterial(keyStore))
.isInstanceOf(GenericKeyStoreException.class)
.hasMessageContaining("some-alias");
}

@Test
void throwGenericKeyStoreWhenGetCertificateAliasThrowsKeyStoreExceptionForMethodContainsCertificate() throws KeyStoreException {
KeyStore keyStore = mock(KeyStore.class);
doThrow(new KeyStoreException("KABOOOM")).when(keyStore).getCertificateAlias(any());

assertThatThrownBy(() -> KeyStoreUtils.containsCertificate(keyStore, null))
.isInstanceOf(GenericKeyStoreException.class)
.hasMessageContaining("KABOOOM");
}

private void resetOsName() {
System.setProperty("os.name", ORIGINAL_OS_NAME);
}
Expand Down
8 changes: 7 additions & 1 deletion sslcontext-kickstart/src/test/java/nl/altindag/ssl/util/TrustManagerUtilsShould.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package nl.altindag.ssl.util;

import nl.altindag.log.LogCaptor;
import nl.altindag.ssl.MockUtils;
import nl.altindag.ssl.exception.GenericSecurityException;
import nl.altindag.ssl.exception.GenericTrustManagerException;
import nl.altindag.ssl.trustmanager.CompositeX509ExtendedTrustManager;
Expand All @@ -32,6 +33,7 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.MockedStatic;
import org.mockito.Mockito;
import org.mockito.junit.jupiter.MockitoExtension;

import javax.net.ssl.CertPathTrustManagerParameters;
Expand Down Expand Up @@ -59,6 +61,7 @@
import java.util.EnumSet;
import java.util.List;
import java.util.Optional;
import java.util.concurrent.CompletableFuture;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
Expand Down Expand Up @@ -192,6 +195,7 @@ void createTrustManagerWithCertificates() {
assertThat((trustManager).getAcceptedIssuers()).hasSizeGreaterThan(10);
}

@SuppressWarnings("rawtypes")
@Test
void createTrustManagerWithSystemTrustedCertificate() {
String operatingSystem = System.getProperty("os.name").toLowerCase();
Expand All @@ -210,7 +214,9 @@ void createTrustManagerWithSystemTrustedCertificate() {
} else {
return invocation.callRealMethod();
}
})) {
}); MockedStatic<CompletableFuture> mockCompletableFuture = mockStatic(CompletableFuture.class, Mockito.CALLS_REAL_METHODS)) {
MockUtils.supplyAsyncOnCurrentThread(mockCompletableFuture);

Optional<X509ExtendedTrustManager> trustManager = TrustManagerUtils.createTrustManagerWithSystemTrustedCertificates();
if (operatingSystem.contains("mac") || operatingSystem.contains("windows") || operatingSystem.contains("linux")) {
assertThat(trustManager).isPresent();
Expand Down
Loading