Skip to content

Commit

Permalink
Fix to release system resource after reading JKWSet file (elastic#48666)
Browse files Browse the repository at this point in the history
When we load a JSON Web Key (JWKSet) from the specified
file using JWKSet.load it internally uses IOUtils.readFileToString
but the opened FileInputStream is never closed after usage.
https://bitbucket.org/connect2id/nimbus-jose-jwt/issues/342

This commit reads the file and parses the JWKSet from the string.

This also fixes an issue wherein if the underlying file changed,
for every change event it would add another file watcher. The
change is to only add the file watcher at the start.

Closes elastic#44942
  • Loading branch information
bizybot authored and Yogesh Gaikwad committed Oct 30, 2019
1 parent 4ce5c2d commit e155e8a
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@
import org.elasticsearch.common.CheckedRunnable;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.util.concurrent.EsExecutors;
import org.elasticsearch.common.util.concurrent.ListenableFuture;
Expand All @@ -95,10 +94,10 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.net.URL;

import java.net.URLEncoder;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.AccessController;
import java.security.PrivilegedAction;
Expand All @@ -111,8 +110,8 @@
import java.util.concurrent.atomic.AtomicReference;

import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.ALLOWED_CLOCK_SKEW;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECT_TIMEOUT;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECTION_READ_TIMEOUT;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_CONNECT_TIMEOUT;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_MAX_CONNECTIONS;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_MAX_ENDPOINT_CONNECTIONS;
import static org.elasticsearch.xpack.core.security.authc.oidc.OpenIdConnectRealmSettings.HTTP_SOCKET_TIMEOUT;
Expand Down Expand Up @@ -142,7 +141,7 @@ public OpenIdConnectAuthenticator(RealmConfig realmConfig, OpenIdConnectProvider
this.sslService = sslService;
this.httpClient = createHttpClient();
this.watcherService = watcherService;
this.idTokenValidator.set(createIdTokenValidator());
this.idTokenValidator.set(createIdTokenValidator(true));
}

// For testing
Expand Down Expand Up @@ -314,10 +313,11 @@ private void validateAccessToken(AccessToken accessToken, JWT idToken) {
* @throws ParseException if the file cannot be parsed
* @throws IOException if the file cannot be read
*/
@SuppressForbidden(reason = "uses toFile")
private JWKSet readJwkSetFromFile(String jwkSetPath) throws IOException, ParseException {
final Path path = realmConfig.env().configFile().resolve(jwkSetPath);
return JWKSet.load(path.toFile());
// avoid using JWKSet.loadFile() as it does not close FileInputStream internally
String jwkSet = new String(Files.readAllBytes(path), StandardCharsets.UTF_8);
return JWKSet.parse(jwkSet);
}

/**
Expand Down Expand Up @@ -589,7 +589,7 @@ private CloseableHttpAsyncClient createHttpClient() {
/*
* Creates an {@link IDTokenValidator} based on the current Relying Party configuration
*/
IDTokenValidator createIdTokenValidator() {
IDTokenValidator createIdTokenValidator(boolean addFileWatcherIfRequired) {
try {
final JWSAlgorithm requestedAlgorithm = rpConfig.getSignatureAlgorithm();
final int allowedClockSkew = Math.toIntExact(realmConfig.getSetting(ALLOWED_CLOCK_SKEW).getMillis());
Expand All @@ -600,12 +600,16 @@ IDTokenValidator createIdTokenValidator() {
new IDTokenValidator(opConfig.getIssuer(), rpConfig.getClientId(), requestedAlgorithm, clientSecret);
} else {
String jwkSetPath = opConfig.getJwkSetPath();
if (jwkSetPath.startsWith("https://")) {
if (jwkSetPath.startsWith("http://")) {
throw new IllegalArgumentException("The [http] protocol is not supported as it is insecure. Use [https] instead");
} else if (jwkSetPath.startsWith("https://")) {
final JWSVerificationKeySelector keySelector = new JWSVerificationKeySelector(requestedAlgorithm,
new ReloadableJWKSource(new URL(jwkSetPath)));
idTokenValidator = new IDTokenValidator(opConfig.getIssuer(), rpConfig.getClientId(), keySelector, null);
} else {
setMetadataFileWatcher(jwkSetPath);
if (addFileWatcherIfRequired) {
setMetadataFileWatcher(jwkSetPath);
}
final JWKSet jwkSet = readJwkSetFromFile(jwkSetPath);
idTokenValidator = new IDTokenValidator(opConfig.getIssuer(), rpConfig.getClientId(), requestedAlgorithm, jwkSet);
}
Expand All @@ -620,7 +624,7 @@ IDTokenValidator createIdTokenValidator() {
private void setMetadataFileWatcher(String jwkSetPath) throws IOException {
final Path path = realmConfig.env().configFile().resolve(jwkSetPath);
FileWatcher watcher = new FileWatcher(path);
watcher.addListener(new FileListener(LOGGER, () -> this.idTokenValidator.set(createIdTokenValidator())));
watcher.addListener(new FileListener(LOGGER, () -> this.idTokenValidator.set(createIdTokenValidator(false))));
watcherService.add(watcher, ResourceWatcherService.Frequency.MEDIUM);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.Constants;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.MockSecureSettings;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -128,7 +127,6 @@ protected boolean transportSSLEnabled() {
}

public void testClusterStarted() {
assumeFalse("https://github.com/elastic/elasticsearch/issues/44942", Constants.WINDOWS);
final AuthenticateRequest request = new AuthenticateRequest();
request.username(nodeClientUsername());
final AuthenticateResponse authenticate = client().execute(AuthenticateAction.INSTANCE, request).actionGet(10, TimeUnit.SECONDS);
Expand Down

0 comments on commit e155e8a

Please sign in to comment.