From 2fa873c4ffcf2e7cce86b934817becfb37c99cb4 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 17:56:41 -0500 Subject: [PATCH 1/2] fix(#15): harden transport config validation Warn when tls.hostnameVerification is off, treat empty resolved secrets and paths as missing with the field named, and reject credentials in opensearchUrl. Closes #15 --- README.md | 6 +- .../quicksearch/QuickSearchQBitConfig.java | 4 + .../quicksearch/QuickSearchTlsConfig.java | 5 +- .../OpenSearchTransportFactory.java | 51 ++++-- .../QuickSearchQBitConfigValidationTest.java | 12 ++ .../OpenSearchTransportFactoryTest.java | 157 ++++++++++++++++++ 6 files changed, 220 insertions(+), 15 deletions(-) create mode 100644 src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java diff --git a/README.md b/README.md index b8499f8..9e81e5a 100644 --- a/README.md +++ b/README.md @@ -137,13 +137,13 @@ Without `schedulerName` the processes exist but nothing runs them; the QBit logs | Field | Default | Description | |---|---|---| | `backendName` | required | QQQ backend for the operational tables | -| `opensearchUrl` | | `https://host[:port]`; preferred over `opensearchHost`, `opensearchPort`, `useSsl` | +| `opensearchUrl` | | `https://host[:port]`; preferred over `opensearchHost`, `opensearchPort`, `useSsl`; credentials in the URL are rejected | | `opensearchHost`, `opensearchPort`, `useSsl` | `useSsl=false` | Legacy connection fields | | `opensearchIndexName` | required | Alias name owned by this QBit; lowercase, OpenSearch naming rules | | `authMode` | inferred | `NONE`, `BASIC` (inferred when credentials are set), `AWS_SIGV4` | -| `opensearchUsername`, `opensearchPassword` | | BASIC credentials; `${env.X}` references recommended; refused over plain HTTP unless `allowPlaintextCredentials` | +| `opensearchUsername`, `opensearchPassword` | | BASIC credentials; `${env.X}` references recommended (an empty value counts as missing); refused over plain HTTP unless `allowPlaintextCredentials` | | `awsRegion`, `awsServiceName`, `awsAssumeRoleArn` | region from `AWS_REGION`; `es` | AWS_SIGV4 settings | -| `tls` | JVM defaults | `caCertificatePath` or `trustStorePath`/`trustStorePassword`/`trustStoreType`, `keyStorePath`/`keyStorePassword`/`keyStoreType` (mTLS), `hostnameVerification`, `insecureSkipVerify` (loopback only unless `allowInsecureInProduction`) | +| `tls` | JVM defaults | `caCertificatePath` or `trustStorePath`/`trustStorePassword`/`trustStoreType`, `keyStorePath`/`keyStorePassword`/`keyStoreType` (mTLS), `hostnameVerification` (`false` logs a warning), `insecureSkipVerify` (loopback only unless `allowInsecureInProduction`) | | `connectTimeoutMillis`, `responseTimeoutMillis`, `maxConnections` | 5000, 60000, 30 | Transport settings | | `transportCustomizer` | | `QCodeReference` to an `OpenSearchTransportCustomizer` (bearer tokens, API keys, interceptors) | | `startupMode` | `FAIL_FAST` | `FAIL_FAST` fails `produce()` when the cluster is unreachable; `DEGRADED` boots and retries on first use | diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfig.java b/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfig.java index c81d913..01b3da3 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfig.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfig.java @@ -166,6 +166,10 @@ public class QuickSearchQBitConfig implements QBitConfig { errors.add("opensearchUrl must include a host"); } + if(uri.getRawUserInfo() != null) + { + errors.add("opensearchUrl must not contain credentials; use opensearchUsername and opensearchPassword"); + } } catch(Exception e) { diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchTlsConfig.java b/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchTlsConfig.java index b57030c..5aba150 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchTlsConfig.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/QuickSearchTlsConfig.java @@ -29,9 +29,12 @@ import com.fasterxml.jackson.annotation.JsonIgnore; ** supply keyStorePath for client-certificate (mTLS) authentication. ** insecureSkipVerify disables certificate and hostname checks and is refused ** for non-loopback hosts unless allowInsecureInProduction is also set. + ** hostnameVerification=false keeps certificate checks but skips the host name + ** check; it is allowed for any host and logs a warning. ** ** Path and password values may be ${env.X} / ${prop.X} references; they are - ** interpreted when the transport is built, never stored resolved. + ** interpreted when the transport is built, never stored resolved. A path that + ** resolves to an empty value is treated as missing. *******************************************************************************/ public class QuickSearchTlsConfig implements Serializable { diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java index 8519c5d..806d40a 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java @@ -132,12 +132,8 @@ public class OpenSearchTransportFactory BasicCredentialsProvider credentialsProvider = null; if(config.getEffectiveAuthMode() == QuickSearchAuthMode.BASIC) { - String username = interpreter.interpret(config.getOpensearchUsername()); - String password = interpreter.interpret(config.getOpensearchPassword()); - if(username == null || password == null) - { - throw (new QException("BASIC authentication is configured but the username or password resolved to null")); - } + String username = requireResolved(interpreter, config.getOpensearchUsername(), "opensearchUsername"); + String password = requireResolved(interpreter, config.getOpensearchPassword(), "opensearchPassword"); credentialsProvider = new BasicCredentialsProvider(); credentialsProvider.setCredentials(new AuthScope(httpHost), new UsernamePasswordCredentials(username, password.toCharArray())); } @@ -262,14 +258,14 @@ public class OpenSearchTransportFactory } else if(StringUtils.hasContent(tls.getCaCertificatePath())) { - builder.loadTrustMaterial(loadPemTrustStore(interpreter.interpret(tls.getCaCertificatePath())), null); + builder.loadTrustMaterial(loadPemTrustStore(requireResolved(interpreter, tls.getCaCertificatePath(), "tls.caCertificatePath")), null); custom = true; } else if(StringUtils.hasContent(tls.getTrustStorePath())) { String password = interpreter.interpret(tls.getTrustStorePassword()); KeyStore trustStore = KeyStore.getInstance(StringUtils.hasContent(tls.getTrustStoreType()) ? tls.getTrustStoreType() : KeyStore.getDefaultType()); - try(InputStream inputStream = new FileInputStream(interpreter.interpret(tls.getTrustStorePath()))) + try(InputStream inputStream = new FileInputStream(requireResolved(interpreter, tls.getTrustStorePath(), "tls.trustStorePath"))) { trustStore.load(inputStream, password == null ? null : password.toCharArray()); } @@ -282,7 +278,7 @@ public class OpenSearchTransportFactory String password = interpreter.interpret(tls.getKeyStorePassword()); char[] chars = password == null ? new char[0] : password.toCharArray(); KeyStore keyStore = KeyStore.getInstance(StringUtils.hasContent(tls.getKeyStoreType()) ? tls.getKeyStoreType() : KeyStore.getDefaultType()); - try(InputStream inputStream = new FileInputStream(interpreter.interpret(tls.getKeyStorePath()))) + try(InputStream inputStream = new FileInputStream(requireResolved(interpreter, tls.getKeyStorePath(), "tls.keyStorePath"))) { keyStore.load(inputStream, chars); } @@ -296,19 +292,52 @@ public class OpenSearchTransportFactory /******************************************************************************* - ** + ** The no-op verifier when insecureSkipVerify (warned in buildSslContext) or + ** hostnameVerification=false is set, else null for the default verifier. *******************************************************************************/ static HostnameVerifier buildHostnameVerifier(QuickSearchTlsConfig tls) { - if(tls != null && (Boolean.TRUE.equals(tls.getInsecureSkipVerify()) || Boolean.FALSE.equals(tls.getHostnameVerification()))) + if(tls == null) + { + return (null); + } + + if(Boolean.TRUE.equals(tls.getInsecureSkipVerify())) { return (NoopHostnameVerifier.INSTANCE); } + + if(Boolean.FALSE.equals(tls.getHostnameVerification())) + { + LOG.warn("tls.hostnameVerification is disabled: OpenSearch server certificates are NOT checked against the host name"); + return (NoopHostnameVerifier.INSTANCE); + } + return (null); } + /******************************************************************************* + ** Interpret a value that must resolve to content; null, empty and blank all + ** count as missing. The error names the field and, for a ${...} reference, + ** the reference itself, never a resolved value. + *******************************************************************************/ + static String requireResolved(QMetaDataVariableInterpreter interpreter, String value, String fieldName) throws QException + { + String resolved = interpreter.interpret(value); + if(StringUtils.hasContent(resolved)) + { + return (resolved); + } + + String trimmed = value == null ? "" : value.trim(); + boolean isReference = trimmed.startsWith("${") && trimmed.endsWith("}"); + throw (new QException(fieldName + " is missing or empty" + (isReference ? " after resolving " + trimmed : ""))); + } + + + /******************************************************************************* ** Load one or more PEM certificates into an in-memory trust store. *******************************************************************************/ diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfigValidationTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfigValidationTest.java index 7436ac1..de957c0 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfigValidationTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/QuickSearchQBitConfigValidationTest.java @@ -108,6 +108,18 @@ class QuickSearchQBitConfigValidationTest + @Test + void testUrl_withUserInfo_isRejectedWithoutEchoingIt() + { + List errors = validate(baseConfig().withOpensearchHost(null).withOpensearchPort(null).withOpensearchUrl("https://admin:s3cret@search.example.com")); + assertThat(errors).anyMatch(e -> e.contains("opensearchUrl must not contain credentials")); + assertThat(errors).noneMatch(e -> e.contains("s3cret") || e.contains("admin")); + + assertThat(validate(baseConfig().withOpensearchUrl("https://admin@search.example.com"))).anyMatch(e -> e.contains("opensearchUrl must not contain credentials")); + } + + + @Test void testAuthMode_inferredFromCredentials() { diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java new file mode 100644 index 0000000..3c748e1 --- /dev/null +++ b/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java @@ -0,0 +1,157 @@ +/* + * Copyright 2024 Kingsrook, LLC + * + * 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 + * + * http://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 com.kingsrook.qbits.quicksearch.opensearch; + + +import com.kingsrook.qqq.backend.core.exceptions.QException; +import com.kingsrook.qqq.backend.core.logging.QCollectingLogger; +import com.kingsrook.qqq.backend.core.logging.QLogger; +import com.kingsrook.qbits.quicksearch.QuickSearchAuthMode; +import com.kingsrook.qbits.quicksearch.QuickSearchQBitConfig; +import com.kingsrook.qbits.quicksearch.QuickSearchTlsConfig; +import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; +import org.apache.logging.log4j.Level; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + + +/******************************************************************************* + ** Unit tests for OpenSearchTransportFactory: secret and path references that + ** resolve to nothing, and the hostname-verification warning. + *******************************************************************************/ +class OpenSearchTransportFactoryTest +{ + private static final String EMPTY_PROPERTY = "quickSearch.test.emptyValue"; + private static final String USER_PROPERTY = "quickSearch.test.userValue"; + private static final String UNSET_ENV = "${env.QUICK_SEARCH_TEST_UNSET_VARIABLE_15}"; + + + + /******************************************************************************* + ** + *******************************************************************************/ + @AfterEach + void afterEach() + { + System.clearProperty(EMPTY_PROPERTY); + System.clearProperty(USER_PROPERTY); + QLogger.deactivateCollectingLoggerForClass(OpenSearchTransportFactory.class); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private QuickSearchQBitConfig baseConfig() + { + return (new QuickSearchQBitConfig() + .withOpensearchUrl("https://localhost:9200") + .withOpensearchIndexName("app-search")); + } + + + + @Test + void testBasic_emptyPasswordReference_namesFieldNotSecrets() + { + System.setProperty(USER_PROPERTY, "secret-user-value"); + System.setProperty(EMPTY_PROPERTY, ""); + + QuickSearchQBitConfig config = baseConfig() + .withAuthMode(QuickSearchAuthMode.BASIC) + .withOpensearchUsername("${prop." + USER_PROPERTY + "}") + .withOpensearchPassword("${prop." + EMPTY_PROPERTY + "}"); + + assertThatThrownBy(() -> OpenSearchTransportFactory.build(config)) + .isInstanceOf(QException.class) + .hasMessageContaining("opensearchPassword") + .hasMessageContaining("${prop." + EMPTY_PROPERTY + "}") + .hasMessageNotContaining("secret-user-value"); + } + + + + @Test + void testBasic_blankOrUnsetReference_isTreatedAsMissing() + { + System.setProperty(EMPTY_PROPERTY, " "); + + QuickSearchQBitConfig blankPassword = baseConfig() + .withAuthMode(QuickSearchAuthMode.BASIC) + .withOpensearchUsername("admin") + .withOpensearchPassword("${prop." + EMPTY_PROPERTY + "}"); + assertThatThrownBy(() -> OpenSearchTransportFactory.build(blankPassword)) + .isInstanceOf(QException.class) + .hasMessageContaining("opensearchPassword"); + + QuickSearchQBitConfig unsetUsername = baseConfig() + .withAuthMode(QuickSearchAuthMode.BASIC) + .withOpensearchUsername(UNSET_ENV) + .withOpensearchPassword("not-in-the-message"); + assertThatThrownBy(() -> OpenSearchTransportFactory.build(unsetUsername)) + .isInstanceOf(QException.class) + .hasMessageContaining("opensearchUsername") + .hasMessageContaining(UNSET_ENV) + .hasMessageNotContaining("not-in-the-message"); + } + + + + @Test + void testTls_unresolvedPaths_nameTheField() + { + System.setProperty(EMPTY_PROPERTY, ""); + + assertPathError(new QuickSearchTlsConfig().withCaCertificatePath(UNSET_ENV), "tls.caCertificatePath", UNSET_ENV); + assertPathError(new QuickSearchTlsConfig().withTrustStorePath(UNSET_ENV), "tls.trustStorePath", UNSET_ENV); + assertPathError(new QuickSearchTlsConfig().withKeyStorePath("${prop." + EMPTY_PROPERTY + "}"), "tls.keyStorePath", "${prop." + EMPTY_PROPERTY + "}"); + } + + + + /******************************************************************************* + ** + *******************************************************************************/ + private void assertPathError(QuickSearchTlsConfig tls, String fieldName, String reference) + { + assertThatThrownBy(() -> OpenSearchTransportFactory.build(baseConfig().withTls(tls))) + .isInstanceOf(QException.class) + .hasMessageContaining(fieldName) + .hasMessageContaining(reference) + .hasMessageNotContaining(": null"); + } + + + + @Test + void testHostnameVerificationDisabled_logsWarning() + { + QCollectingLogger collectingLogger = QLogger.activateCollectingLoggerForClass(OpenSearchTransportFactory.class); + + assertThat(OpenSearchTransportFactory.buildHostnameVerifier(new QuickSearchTlsConfig().withHostnameVerification(false))).isSameAs(NoopHostnameVerifier.INSTANCE); + assertThat(collectingLogger.getCollectedMessages()).anyMatch(m -> Level.WARN.equals(m.getLevel()) && m.getMessage().contains("tls.hostnameVerification")); + + collectingLogger.clear(); + assertThat(OpenSearchTransportFactory.buildHostnameVerifier(new QuickSearchTlsConfig())).isNull(); + assertThat(OpenSearchTransportFactory.buildHostnameVerifier(null)).isNull(); + assertThat(collectingLogger.getCollectedMessages()).isEmpty(); + } + +} -- 2.54.0 From dc36ac68416c785f0d405616cd7fd751fb0c2622 Mon Sep 17 00:00:00 2001 From: James Maes Date: Tue, 6 Oct 2026 18:07:29 -0500 Subject: [PATCH 2/2] fix(#15): never echo literals from secret fallback chains Only a lone ${env.X} or ${prop.X} reference is named in resolution errors, and a happy-path test guards resolved BASIC credentials. Refs #15 --- .../OpenSearchTransportFactory.java | 14 ++++-- .../OpenSearchTransportFactoryTest.java | 44 +++++++++++++++++-- 2 files changed, 52 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java index 806d40a..1d6fe91 100644 --- a/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java +++ b/src/main/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactory.java @@ -23,6 +23,7 @@ import java.security.KeyStore; import java.security.cert.Certificate; import java.security.cert.CertificateFactory; import java.util.Collection; +import java.util.regex.Pattern; import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLContext; import com.fasterxml.jackson.databind.ObjectMapper; @@ -78,6 +79,12 @@ public class OpenSearchTransportFactory private static final String USER_AGENT = "qbit-quick-search/" + QuickSearchQBitProducer.getVersion(); + /////////////////////////////////////////////////////////////////////////// + // Only a lone ${env.X} or ${prop.X} is echoed in errors: a ?? chain may // + // hold a literal fallback secret between its references. // + /////////////////////////////////////////////////////////////////////////// + private static final Pattern SINGLE_REFERENCE = Pattern.compile("^\\$\\{(env|prop)\\.[A-Za-z0-9_.-]+\\}$"); + /******************************************************************************* @@ -320,8 +327,9 @@ public class OpenSearchTransportFactory /******************************************************************************* ** Interpret a value that must resolve to content; null, empty and blank all - ** count as missing. The error names the field and, for a ${...} reference, - ** the reference itself, never a resolved value. + ** count as missing. The error names the field and, for a single ${env.X} or + ** ${prop.X} reference, the reference itself; never a resolved value and + ** never any part of a ?? fallback chain. *******************************************************************************/ static String requireResolved(QMetaDataVariableInterpreter interpreter, String value, String fieldName) throws QException { @@ -332,7 +340,7 @@ public class OpenSearchTransportFactory } String trimmed = value == null ? "" : value.trim(); - boolean isReference = trimmed.startsWith("${") && trimmed.endsWith("}"); + boolean isReference = SINGLE_REFERENCE.matcher(trimmed).matches(); throw (new QException(fieldName + " is missing or empty" + (isReference ? " after resolving " + trimmed : ""))); } diff --git a/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java b/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java index 3c748e1..8aaa2d1 100644 --- a/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java +++ b/src/test/java/com/kingsrook/qbits/quicksearch/opensearch/OpenSearchTransportFactoryTest.java @@ -25,6 +25,7 @@ import com.kingsrook.qbits.quicksearch.QuickSearchQBitConfig; import com.kingsrook.qbits.quicksearch.QuickSearchTlsConfig; import org.apache.hc.client5.http.ssl.NoopHostnameVerifier; import org.apache.logging.log4j.Level; +import org.opensearch.client.transport.OpenSearchTransport; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; import static org.assertj.core.api.Assertions.assertThat; @@ -37,9 +38,10 @@ import static org.assertj.core.api.Assertions.assertThatThrownBy; *******************************************************************************/ class OpenSearchTransportFactoryTest { - private static final String EMPTY_PROPERTY = "quickSearch.test.emptyValue"; - private static final String USER_PROPERTY = "quickSearch.test.userValue"; - private static final String UNSET_ENV = "${env.QUICK_SEARCH_TEST_UNSET_VARIABLE_15}"; + private static final String EMPTY_PROPERTY = "quickSearch.test.emptyValue"; + private static final String USER_PROPERTY = "quickSearch.test.userValue"; + private static final String PASSWORD_PROPERTY = "quickSearch.test.passwordValue"; + private static final String UNSET_ENV = "${env.QUICK_SEARCH_TEST_UNSET_VARIABLE_15}"; @@ -51,6 +53,7 @@ class OpenSearchTransportFactoryTest { System.clearProperty(EMPTY_PROPERTY); System.clearProperty(USER_PROPERTY); + System.clearProperty(PASSWORD_PROPERTY); QLogger.deactivateCollectingLoggerForClass(OpenSearchTransportFactory.class); } @@ -114,6 +117,41 @@ class OpenSearchTransportFactoryTest + @Test + void testBasic_unresolvedFallbackChain_neverEchoesTheLiteral() + { + QuickSearchQBitConfig config = baseConfig() + .withAuthMode(QuickSearchAuthMode.BASIC) + .withOpensearchUsername("admin") + .withOpensearchPassword(UNSET_ENV + "??fallback-secret-literal??${env.QUICK_SEARCH_TEST_UNSET_VARIABLE_15B}"); + + assertThatThrownBy(() -> OpenSearchTransportFactory.build(config)) + .isInstanceOf(QException.class) + .hasMessageContaining("opensearchPassword") + .hasMessageNotContaining("fallback-secret-literal"); + } + + + + @Test + void testBasic_resolvedReferences_buildTransport() throws Exception + { + System.setProperty(USER_PROPERTY, "admin"); + System.setProperty(PASSWORD_PROPERTY, "admin-password"); + + QuickSearchQBitConfig config = baseConfig() + .withAuthMode(QuickSearchAuthMode.BASIC) + .withOpensearchUsername("${prop." + USER_PROPERTY + "}") + .withOpensearchPassword("${prop." + PASSWORD_PROPERTY + "}"); + + try(OpenSearchTransport transport = OpenSearchTransportFactory.build(config)) + { + assertThat(transport).isNotNull(); + } + } + + + @Test void testTls_unresolvedPaths_nameTheField() { -- 2.54.0