From b1611dc2403715c576dfc454c5b40e8dbbbd8fb3 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Mon, 8 Jun 2026 00:57:36 +0530 Subject: [PATCH 01/30] Adding support of Secret Manager framework support in Apache OFBiz. For various providers support we can add plugins components. --- build.gradle | 7 +++ dependencies.gradle | 1 + .../base/secret/FileBasedSecretProvider.java | 43 +++++++++++++ .../ofbiz/base/secret/SecretProvider.java | 49 +++++++++++++++ .../base/secret/SecretProviderFactory.java | 63 +++++++++++++++++++ .../entity/config/model/EntityConfig.java | 13 ++-- .../connection/DBCPConnectionFactory.java | 5 ++ 7 files changed, 175 insertions(+), 6 deletions(-) create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java diff --git a/build.gradle b/build.gradle index 10b13996637..73537e4980c 100644 --- a/build.gradle +++ b/build.gradle @@ -169,6 +169,13 @@ tasks.withType(JavaCompile) { // Enables Zip larger than 4 GB and more than 65535 entries tasks.withType(Zip) { zip64 = true } +// Multiple SecretProvider plugins each contribute the same META-INF/services/ filename. +// Only one provider should be enabled at a time (via ofbiz-component.xml enabled="true/false"). +// FIRST keeps the alphabetically first file during transitional states where two are temporarily enabled. +processResources { + duplicatesStrategy = DuplicatesStrategy.EXCLUDE +} + // Only used for release branches def getCurrentGitBranch() { return "git branch --show-current".execute().text.trim() diff --git a/dependencies.gradle b/dependencies.gradle index b890c4dac12..55507fd6020 100644 --- a/dependencies.gradle +++ b/dependencies.gradle @@ -100,6 +100,7 @@ dependencies { runtimeOnly 'org.apache.axis2:axis2-transport-http:1.8.2' runtimeOnly 'org.apache.axis2:axis2-transport-local:1.8.2' runtimeOnly 'com.h2database:h2:2.4.240' + runtimeOnly 'com.mysql:mysql-connector-j:8.4.0' runtimeOnly 'org.apache.geronimo.specs:geronimo-jaxrpc_1.1_spec:2.1' runtimeOnly 'org.apache.logging.log4j:log4j-1.2-api:2.25.4' // for external jars using the old log4j1.2: routes logging to log4j 2 runtimeOnly 'org.apache.logging.log4j:log4j-jul:2.25.4' // for external jars using the java.util.logging: routes logging to log4j 2 diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java new file mode 100644 index 00000000000..2879830d5e9 --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java @@ -0,0 +1,43 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilProperties; +import org.apache.ofbiz.base.util.UtilValidate; + +/** + * Default {@link SecretProvider} implementation that resolves secrets from + * {@code framework/base/config/passwords.properties}. + * + *

This preserves full backward compatibility with the existing + * {@code jdbc-password-lookup} mechanism in {@code entityengine.xml}. + * No configuration is required to use this implementation.

+ */ +public final class FileBasedSecretProvider implements SecretProvider { + + @Override + public String getSecret(String key) throws GeneralException { + String value = UtilProperties.getPropertyValue("passwords", key); + if (UtilValidate.isEmpty(value)) { + throw new GeneralException("Secret key '" + key + "' not found in passwords.properties"); + } + return value; + } +} diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java new file mode 100644 index 00000000000..11b1f868510 --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java @@ -0,0 +1,49 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import org.apache.ofbiz.base.util.GeneralException; + +/** + * SPI for resolving secrets and credentials (e.g. database passwords, API keys). + * + *

Implementations must be thread-safe. Register a custom implementation via + * Java's {@link java.util.ServiceLoader} by providing a file named + * {@code META-INF/services/org.apache.ofbiz.base.secret.SecretProvider} in + * your plugin JAR containing the fully-qualified class name of your + * implementation. If no custom implementation is registered, + * {@link FileBasedSecretProvider} is used as the default, which resolves + * secrets from {@code framework/base/config/passwords.properties}.

+ * + *

Example entry in a vault plugin's service descriptor:

+ * 
+ *   org.example.ofbiz.vault.AwsSecretsManagerProvider
+ *
+ */ +public interface SecretProvider { + + /** + * Returns the secret value for the given key. + * + * @param key the identifier for the secret (e.g. {@code "jdbc-password.mydb"}) + * @return the resolved secret value, never {@code null} or empty + * @throws GeneralException if the secret cannot be found or an error occurs during resolution + */ + String getSecret(String key) throws GeneralException; +} diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java new file mode 100644 index 00000000000..9d8ec088acd --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java @@ -0,0 +1,63 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import java.util.Iterator; +import java.util.ServiceLoader; + +import org.apache.ofbiz.base.lang.ThreadSafe; +import org.apache.ofbiz.base.util.Debug; + +/** + * Factory that provides the active {@link SecretProvider} instance. + * + *

On first access the factory uses Java's {@link ServiceLoader} to discover + * a custom {@link SecretProvider} registered in any plugin's + * {@code META-INF/services/org.apache.ofbiz.base.secret.SecretProvider} file. + * If none is found, {@link FileBasedSecretProvider} is used automatically, + * preserving backward compatibility with {@code passwords.properties}.

+ * + *

This mirrors the pattern already used by + * {@link org.apache.ofbiz.security.SecurityFactory}.

+ */ +@ThreadSafe +public final class SecretProviderFactory { + + private static final String MODULE = SecretProviderFactory.class.getName(); + + private static final SecretProvider INSTANCE = loadProvider(); + + private static SecretProvider loadProvider() { + Iterator it = ServiceLoader.load(SecretProvider.class).iterator(); + if (it.hasNext()) { + SecretProvider provider = it.next(); + Debug.logInfo("SecretProvider: using custom implementation " + provider.getClass().getName(), MODULE); + return provider; + } + Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider", MODULE); + return new FileBasedSecretProvider(); + } + + /** Returns the active {@link SecretProvider} instance. */ + public static SecretProvider getInstance() { + return INSTANCE; + } + + private SecretProviderFactory() { } +} diff --git a/framework/entity/src/main/java/org/apache/ofbiz/entity/config/model/EntityConfig.java b/framework/entity/src/main/java/org/apache/ofbiz/entity/config/model/EntityConfig.java index 1861e176cf1..ff65892e7ab 100644 --- a/framework/entity/src/main/java/org/apache/ofbiz/entity/config/model/EntityConfig.java +++ b/framework/entity/src/main/java/org/apache/ofbiz/entity/config/model/EntityConfig.java @@ -26,8 +26,9 @@ import java.util.Map; import org.apache.ofbiz.base.lang.ThreadSafe; +import org.apache.ofbiz.base.secret.SecretProviderFactory; import org.apache.ofbiz.base.util.Debug; -import org.apache.ofbiz.base.util.UtilProperties; +import org.apache.ofbiz.base.util.GeneralException; import org.apache.ofbiz.base.util.UtilURL; import org.apache.ofbiz.base.util.UtilXml; import org.apache.ofbiz.entity.GenericEntityConfException; @@ -353,12 +354,12 @@ public static String getJdbcPassword(InlineJdbc inlineJdbcElement) throws Generi + inlineJdbcElement.getLineNumber()); } String key = "jdbc-password.".concat(jdbcPasswordLookup); - jdbcPassword = UtilProperties.getPropertyValue("passwords", key); - if (jdbcPassword.isEmpty()) { - throw new GenericEntityConfException("'" + key + "' property not found in passwords.properties file for inline-jdbc element, line: " - + inlineJdbcElement.getLineNumber()); + try { + return SecretProviderFactory.getInstance().getSecret(key); + } catch (GeneralException e) { + throw new GenericEntityConfException("Secret not found for key '" + key + "' for inline-jdbc element, line: " + + inlineJdbcElement.getLineNumber() + " - " + e.getMessage()); } - return jdbcPassword; } /** Returns the <datasource> child elements as a Map. */ diff --git a/framework/entity/src/main/java/org/apache/ofbiz/entity/connection/DBCPConnectionFactory.java b/framework/entity/src/main/java/org/apache/ofbiz/entity/connection/DBCPConnectionFactory.java index 45ffe2ef115..98e2759d5ba 100644 --- a/framework/entity/src/main/java/org/apache/ofbiz/entity/connection/DBCPConnectionFactory.java +++ b/framework/entity/src/main/java/org/apache/ofbiz/entity/connection/DBCPConnectionFactory.java @@ -152,6 +152,11 @@ public Connection getConnection(GenericHelperInfo helperInfo, JdbcElement abstra GenericObjectPool pool = new GenericObjectPool<>(factory, poolConfig); factory.setPool(pool); + try { + pool.preparePool(); + } catch (Exception e) { + Debug.logWarning("Could not pre-warm connection pool: " + e.getMessage(), MODULE); + } mds = new DebugManagedDataSource<>(pool, xacf.getTransactionRegistry()); mds.setAccessToUnderlyingConnectionAllowed(true); From 348d346a91036c77a7b4062d3a042669c154eaef Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Mon, 8 Jun 2026 18:29:16 +0530 Subject: [PATCH 02/30] Adding the support of AES Encryption based password. Also added the gradle task that will help us in generating the plain or encrypted password. --- build.gradle | 26 ++++ .../ofbiz/base/crypto/ConfigCryptoUtil.java | 124 ++++++++++++++++++ .../base/secret/FileBasedSecretProvider.java | 23 ++++ 3 files changed, 173 insertions(+) create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java diff --git a/build.gradle b/build.gradle index 73537e4980c..519918d3dc8 100644 --- a/build.gradle +++ b/build.gradle @@ -749,6 +749,32 @@ task gitInfoFooter(group: sysadminGroup, description: 'Update the Git Branch-rev } } +task generateDBPassword(group: sysadminGroup, + description: 'Write a plain or encrypted (ENC(...)) database password into passwords.properties. ' + + 'Usage: ./gradlew generateDBPassword -Pvalue= -PlookupKey= [-PmasterKey=]') { + doLast { + def secret = project.property('value') + if (project.hasProperty('masterKey')) { + def output = new java.io.ByteArrayOutputStream() + javaexec { + classpath = sourceSets.main.runtimeClasspath + mainClass = 'org.apache.ofbiz.base.crypto.ConfigCryptoUtil' + args project.property('masterKey'), secret + standardOutput = output + } + secret = output.toString().trim() + } + def propertyName = "jdbc-password.${project.property('lookupKey')}" + def newLine = "${propertyName}=${secret}" + def passwordsFile = file('framework/base/config/passwords.properties') + def lines = passwordsFile.readLines() + def index = lines.findIndexOf { it.startsWith("${propertyName}=") } + if (index >= 0) lines[index] = newLine else lines << newLine + passwordsFile.text = lines.join(System.lineSeparator()) + System.lineSeparator() + println "Stored ${newLine} in passwords.properties" + } +} + task generateSecretKeys(group: sysadminGroup, description: 'Generate cryptographically secure 512-bit (64-char) secret keys for JWT token signing and password encryption, and write them to security.properties') { doLast { diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java new file mode 100644 index 00000000000..9a863a08577 --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -0,0 +1,124 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.crypto; + +import java.nio.charset.StandardCharsets; +import java.security.GeneralSecurityException; +import java.security.SecureRandom; +import java.util.Arrays; +import java.util.Base64; + +import javax.crypto.Cipher; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.GCMParameterSpec; +import javax.crypto.spec.PBEKeySpec; +import javax.crypto.spec.SecretKeySpec; + +import org.apache.ofbiz.base.util.GeneralException; + +/** + * AES-256-GCM encryption/decryption for configuration values such as the + * database passwords stored in {@code passwords.properties} as {@code ENC(...)}. + * + *

The AES key is derived from a master key (e.g. the {@code OFBIZ_DB_KEY} + * environment variable) via PBKDF2WithHmacSHA256, so the same master key always + * yields the same AES key. The IV is randomly generated per encryption and + * stored alongside the ciphertext, both Base64-encoded together.

+ */ +public final class ConfigCryptoUtil { + + private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding"; + private static final String KEY_DERIVATION_ALGORITHM = "PBKDF2WithHmacSHA256"; + // Static salt: the master key is the actual secret; the salt only needs to defeat + // precomputed rainbow tables, not provide per-installation uniqueness. + private static final byte[] SALT = "OFBizConfigCryptoUtilSalt".getBytes(StandardCharsets.UTF_8); + private static final int ITERATIONS = 10000; + private static final int KEY_LENGTH_BITS = 256; + private static final int GCM_TAG_LENGTH_BITS = 128; + private static final int IV_LENGTH_BYTES = 12; + + private static SecretKeySpec deriveKey(String masterKey) throws GeneralSecurityException { + SecretKeyFactory factory = SecretKeyFactory.getInstance(KEY_DERIVATION_ALGORITHM); + PBEKeySpec spec = new PBEKeySpec(masterKey.toCharArray(), SALT, ITERATIONS, KEY_LENGTH_BITS); + try { + return new SecretKeySpec(factory.generateSecret(spec).getEncoded(), "AES"); + } finally { + spec.clearPassword(); + } + } + + /** + * Encrypts {@code plainText} with a key derived from {@code masterKey}. + * @return Base64 string containing the random IV followed by the ciphertext (with GCM auth tag) + */ + public static String encrypt(String plainText, String masterKey) throws GeneralException { + try { + SecretKeySpec secretKey = deriveKey(masterKey); + byte[] iv = new byte[IV_LENGTH_BYTES]; + new SecureRandom().nextBytes(iv); + Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM); + cipher.init(Cipher.ENCRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); + byte[] cipherText = cipher.doFinal(plainText.getBytes(StandardCharsets.UTF_8)); + byte[] payload = new byte[IV_LENGTH_BYTES + cipherText.length]; + System.arraycopy(iv, 0, payload, 0, IV_LENGTH_BYTES); + System.arraycopy(cipherText, 0, payload, IV_LENGTH_BYTES, cipherText.length); + return Base64.getEncoder().encodeToString(payload); + } catch (GeneralSecurityException e) { + throw new GeneralException("Unable to encrypt value", e); + } + } + + /** + * Reverses {@link #encrypt(String, String)}: derives the AES key from {@code masterKey}, + * splits the IV from the ciphertext and decrypts. + */ + public static String decrypt(String encryptedBase64, String masterKey) throws GeneralException { + try { + byte[] payload = Base64.getDecoder().decode(encryptedBase64); + if (payload.length <= IV_LENGTH_BYTES) { + throw new GeneralException("Encrypted value is too short to contain an IV and ciphertext"); + } + byte[] iv = Arrays.copyOfRange(payload, 0, IV_LENGTH_BYTES); + byte[] cipherText = Arrays.copyOfRange(payload, IV_LENGTH_BYTES, payload.length); + SecretKeySpec secretKey = deriveKey(masterKey); + Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM); + cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); + return new String(cipher.doFinal(cipherText), StandardCharsets.UTF_8); + } catch (GeneralSecurityException | IllegalArgumentException e) { + throw new GeneralException("Unable to decrypt value: verify the master key is correct", e); + } + } + + private ConfigCryptoUtil() { } + + /** + * Command-line helper that prints {@code ENC()} for a given master key and plaintext + * value, ready to be pasted into {@code passwords.properties} (e.g. as + * {@code jdbc-password.=ENC(...)}). Invoked via the {@code encryptDbPassword} + * Gradle task. + */ + public static void main(String[] args) throws GeneralException { + if (args.length != 2) { + System.err.println("Usage: ConfigCryptoUtil "); + System.exit(1); + return; + } + System.out.println("ENC(" + encrypt(args[1], args[0]) + ")"); + } +} diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java index 2879830d5e9..3d5a927cccf 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java @@ -18,6 +18,7 @@ *******************************************************************************/ package org.apache.ofbiz.base.secret; +import org.apache.ofbiz.base.crypto.ConfigCryptoUtil; import org.apache.ofbiz.base.util.GeneralException; import org.apache.ofbiz.base.util.UtilProperties; import org.apache.ofbiz.base.util.UtilValidate; @@ -29,15 +30,37 @@ *

This preserves full backward compatibility with the existing * {@code jdbc-password-lookup} mechanism in {@code entityengine.xml}. * No configuration is required to use this implementation.

+ * + *

Values may optionally be stored encrypted, wrapped as {@code ENC()}, + * e.g. {@code jdbc-password.mysql-ofbiz=ENC(AbCd123...)}. Encrypted values are + * decrypted in-memory with {@link ConfigCryptoUtil} using the master key supplied + * via the {@code OFBIZ_DB_KEY} environment variable.

*/ public final class FileBasedSecretProvider implements SecretProvider { + private static final String ENC_PREFIX = "ENC("; + private static final String ENC_SUFFIX = ")"; + private static final String MASTER_KEY_ENV_VAR = "OFBIZ_DB_KEY"; + @Override public String getSecret(String key) throws GeneralException { String value = UtilProperties.getPropertyValue("passwords", key); if (UtilValidate.isEmpty(value)) { throw new GeneralException("Secret key '" + key + "' not found in passwords.properties"); } + if (value.startsWith(ENC_PREFIX) && value.endsWith(ENC_SUFFIX)) { + String masterKey = System.getenv(MASTER_KEY_ENV_VAR); + if (UtilValidate.isEmpty(masterKey)) { + throw new GeneralException("Secret '" + key + "' is encrypted but the " + MASTER_KEY_ENV_VAR + + " environment variable holding the master key is not set"); + } + String encryptedValue = value.substring(ENC_PREFIX.length(), value.length() - ENC_SUFFIX.length()); + try { + return ConfigCryptoUtil.decrypt(encryptedValue, masterKey); + } catch (GeneralException e) { + throw new GeneralException("Failed to decrypt secret '" + key + "': " + e.getMessage(), e); + } + } return value; } } From dc09410a31c65c652bc381c2389eb2a2095d01b5 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Tue, 9 Jun 2026 15:11:23 +0530 Subject: [PATCH 03/30] Testing with aws secret manager has been completed. I tested it with plain text and encrypted passwords both. Added a method in ConfigCryptoUtil.java and this method will be used by all the secret manager plugins present in plugins folder. --- .../ofbiz/base/crypto/ConfigCryptoUtil.java | 33 +++++++++++++++++++ .../base/secret/FileBasedSecretProvider.java | 19 +---------- .../ofbiz/base/secret/SecretProvider.java | 9 +++++ 3 files changed, 43 insertions(+), 18 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java index 9a863a08577..244a5c4dc68 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -105,6 +105,39 @@ public static String decrypt(String encryptedBase64, String masterKey) throws Ge } } + /** + * Returns the plaintext of {@code rawValue} if it is wrapped in {@code ENC(...)}, + * otherwise returns it unchanged. + * + *

All {@link org.apache.ofbiz.base.secret.SecretProvider} implementations should + * call this after fetching a value from their remote backend so that operators can + * optionally add a client-side AES-256-GCM layer on top of whatever the vault already + * provides. The master key is read from the {@code OFBIZ_DB_KEY} environment variable.

+ * + * @param rawValue the value as returned by the secret backend (plaintext or {@code ENC(...)}) + * @param secretName used only in error messages to identify which secret failed + * @return the plaintext value — either the original string or the decrypted one + * @throws GeneralException if the value is encrypted but {@code OFBIZ_DB_KEY} is missing, + * or if decryption fails (e.g. wrong key) + */ + public static String decryptIfEncrypted(String rawValue, String secretName) throws GeneralException { + if (!rawValue.startsWith("ENC(") || !rawValue.endsWith(")")) { + return rawValue; + } + String masterKey = System.getenv("OFBIZ_DB_KEY"); + if (masterKey == null || masterKey.isEmpty()) { + throw new GeneralException("Secret '" + secretName + "' is encrypted (ENC(...)) but the " + + "OFBIZ_DB_KEY environment variable is not set"); + } + String base64 = rawValue.substring("ENC(".length(), rawValue.length() - 1); + try { + return decrypt(base64, masterKey); + } catch (GeneralException e) { + throw new GeneralException("Failed to decrypt secret '" + secretName + + "': verify OFBIZ_DB_KEY matches the key used to encrypt", e); + } + } + private ConfigCryptoUtil() { } /** diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java index 3d5a927cccf..4152b53f19d 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java @@ -38,29 +38,12 @@ */ public final class FileBasedSecretProvider implements SecretProvider { - private static final String ENC_PREFIX = "ENC("; - private static final String ENC_SUFFIX = ")"; - private static final String MASTER_KEY_ENV_VAR = "OFBIZ_DB_KEY"; - @Override public String getSecret(String key) throws GeneralException { String value = UtilProperties.getPropertyValue("passwords", key); if (UtilValidate.isEmpty(value)) { throw new GeneralException("Secret key '" + key + "' not found in passwords.properties"); } - if (value.startsWith(ENC_PREFIX) && value.endsWith(ENC_SUFFIX)) { - String masterKey = System.getenv(MASTER_KEY_ENV_VAR); - if (UtilValidate.isEmpty(masterKey)) { - throw new GeneralException("Secret '" + key + "' is encrypted but the " + MASTER_KEY_ENV_VAR - + " environment variable holding the master key is not set"); - } - String encryptedValue = value.substring(ENC_PREFIX.length(), value.length() - ENC_SUFFIX.length()); - try { - return ConfigCryptoUtil.decrypt(encryptedValue, masterKey); - } catch (GeneralException e) { - throw new GeneralException("Failed to decrypt secret '" + key + "': " + e.getMessage(), e); - } - } - return value; + return ConfigCryptoUtil.decryptIfEncrypted(value, key); } } diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java index 11b1f868510..b042fa67b01 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java @@ -35,6 +35,15 @@ * 
  *   org.example.ofbiz.vault.AwsSecretsManagerProvider
  *
+ * + *

Optional client-side encryption ({@code ENC(...)})

+ *

Any provider implementation may store secret values wrapped in + * {@code ENC()} to add a client-side AES-256-GCM encryption layer on + * top of whatever the remote vault already provides. Call + * {@code ConfigCryptoUtil.decryptIfEncrypted()} on the raw value returned by + * the remote API before caching or returning it. The master key is read from + * the {@code OFBIZ_DB_KEY} environment variable at runtime and is never stored + * in config files or in the remote vault.

*/ public interface SecretProvider { From 653ee9738ae4c6e33be8fd46343f58e445b61a87 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Wed, 10 Jun 2026 00:19:35 +0530 Subject: [PATCH 04/30] Changing -Pvalue to -PdbPassword for clarity. --- build.gradle | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/build.gradle b/build.gradle index 519918d3dc8..a940122550f 100644 --- a/build.gradle +++ b/build.gradle @@ -751,9 +751,9 @@ task gitInfoFooter(group: sysadminGroup, description: 'Update the Git Branch-rev task generateDBPassword(group: sysadminGroup, description: 'Write a plain or encrypted (ENC(...)) database password into passwords.properties. ' - + 'Usage: ./gradlew generateDBPassword -Pvalue= -PlookupKey= [-PmasterKey=]') { + + 'Usage: ./gradlew generateDBPassword -PdbPassword= -PlookupKey= [-PmasterKey=]') { doLast { - def secret = project.property('value') + def secret = project.property('dbPassword') if (project.hasProperty('masterKey')) { def output = new java.io.ByteArrayOutputStream() javaexec { From fd38a3c13179464e325f94eb093f1edd7b4fc05a Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Wed, 10 Jun 2026 17:41:08 +0530 Subject: [PATCH 05/30] Added the fallback support in the code base. If remote services of secret manager/vault is unavailable then it will check the password(plain or encrypted one) in the passwords.properties file. --- .../apache/ofbiz/base/secret/SecretProvider.java | 16 ++++++++++++++++ .../ofbiz/base/secret/SecretProviderFactory.java | 9 ++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java index b042fa67b01..01c97aa7fc9 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java @@ -55,4 +55,20 @@ public interface SecretProvider { * @throws GeneralException if the secret cannot be found or an error occurs during resolution */ String getSecret(String key) throws GeneralException; + + /** + * Returns whether {@link FallbackSecretProvider} is allowed to fall back to + * {@link FileBasedSecretProvider} (i.e. {@code passwords.properties}) when this + * provider fails to resolve a secret. + * + *

Implementations should read this from their own plugin configuration + * resource (e.g. {@code .fallback.enabled}), defaulting + * to {@code true}.

+ * + * @return {@code true} if local fallback is permitted, {@code false} to require + * this provider to be the sole source of secrets + */ + default boolean isFallbackEnabled() { + return true; + } } diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java index 9d8ec088acd..6f2191d133c 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java @@ -33,6 +33,13 @@ * If none is found, {@link FileBasedSecretProvider} is used automatically, * preserving backward compatibility with {@code passwords.properties}.

* + *

If a custom (remote) provider is found, it is wrapped in a + * {@link FallbackSecretProvider} together with {@link FileBasedSecretProvider}. + * When the remote provider's own {@code .fallback.enabled} configuration + * property is {@code true} (the default) and the remote provider fails (e.g. the + * remote secret manager is unreachable), the secret is resolved from the local + * {@code passwords.properties} file instead.

+ * *

This mirrors the pattern already used by * {@link org.apache.ofbiz.security.SecurityFactory}.

*/ @@ -48,7 +55,7 @@ private static SecretProvider loadProvider() { if (it.hasNext()) { SecretProvider provider = it.next(); Debug.logInfo("SecretProvider: using custom implementation " + provider.getClass().getName(), MODULE); - return provider; + return new FallbackSecretProvider(provider, new FileBasedSecretProvider()); } Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider", MODULE); return new FileBasedSecretProvider(); From b9d4683dd81043090186e0fe25ac4a70298f0f86 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Thu, 11 Jun 2026 12:41:54 +0530 Subject: [PATCH 06/30] Adding the fallback option for all the plugins. And also improved console warnings, now it will return file name as well if it's falling back to file based credentials if all the plugins are disabled. --- .../base/secret/FallbackSecretProvider.java | 82 +++++++++++++++++++ .../base/secret/SecretProviderFactory.java | 3 +- 2 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java new file mode 100644 index 00000000000..369608f78df --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java @@ -0,0 +1,82 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.GeneralException; + +/** + * {@link SecretProvider} decorator that falls back to a secondary provider + * when the primary provider fails to resolve a secret. + * + *

This is used by {@link SecretProviderFactory} to wrap a custom (remote) + * {@link SecretProvider} implementation together with a + * {@link FileBasedSecretProvider}. If the primary provider throws a + * {@link GeneralException} (e.g. the remote secret manager is unreachable or + * the secret does not exist there) and {@link SecretProvider#isFallbackEnabled()} + * returns {@code true} for the primary provider, the secret is resolved from + * the fallback provider instead.

+ * + *

If the primary provider fails and fallback is disabled, or the fallback + * provider also fails, the original exception from the primary provider is + * propagated to the caller.

+ */ +public final class FallbackSecretProvider implements SecretProvider { + + private static final String MODULE = FallbackSecretProvider.class.getName(); + + private final SecretProvider primary; + private final SecretProvider fallback; + + /** + * @param primary the primary (typically remote) provider to try first + * @param fallback the provider to use if the primary fails and allows fallback + */ + public FallbackSecretProvider(SecretProvider primary, SecretProvider fallback) { + this.primary = primary; + this.fallback = fallback; + } + + @Override + public String getSecret(String key) throws GeneralException { + try { + return primary.getSecret(key); + } catch (GeneralException e) { + if (!primary.isFallbackEnabled()) { + throw e; + } + Debug.logWarning("SecretProvider: " + primary.getClass().getName() + + " failed to resolve secret '" + key + "' (" + e.getMessage() + + "), falling back to " + describe(fallback), MODULE); + return fallback.getSecret(key); + } + } + + /** + * Returns a human-readable description of the given provider for log + * messages, naming the backing {@code passwords.properties} file for + * {@link FileBasedSecretProvider}. + */ + private static String describe(SecretProvider provider) { + if (provider instanceof FileBasedSecretProvider) { + return provider.getClass().getName() + " (framework/base/config/passwords.properties)"; + } + return provider.getClass().getName(); + } +} diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java index 6f2191d133c..5e5fef791de 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java @@ -57,7 +57,8 @@ private static SecretProvider loadProvider() { Debug.logInfo("SecretProvider: using custom implementation " + provider.getClass().getName(), MODULE); return new FallbackSecretProvider(provider, new FileBasedSecretProvider()); } - Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider", MODULE); + Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider" + + " (framework/base/config/passwords.properties)", MODULE); return new FileBasedSecretProvider(); } From 3a75ccfdb4115903973174248f97b9530be6c5d1 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Thu, 11 Jun 2026 18:03:54 +0530 Subject: [PATCH 07/30] Improved: Use Shiro's AesCipherService for ConfigCryptoUtil encryption Replace the hand-rolled javax.crypto AES/GCM/NoPadding implementation in ConfigCryptoUtil with Apache Shiro's AesCipherService (AES-256-GCM), the same crypto library already used by EntityCrypto for entity field-level encryption. This standardizes the encryption approach across the codebase and removes duplicate, manually-managed cipher/IV handling code. PBKDF2WithHmacSHA256 key derivation from OFBIZ_DB_KEY is retained unchanged. Note: the on-disk format of ENC(...) values changes (Shiro uses a 16-byte IV vs the previous 12-byte IV), so any existing encrypted values in passwords.properties or external secret stores must be regenerated using the generateDBPassword Gradle task. I have tested out it and everything is working fine. --- .../ofbiz/base/crypto/ConfigCryptoUtil.java | 59 ++++++++----------- 1 file changed, 24 insertions(+), 35 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java index 244a5c4dc68..171f23f1020 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -20,17 +20,15 @@ import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; -import java.security.SecureRandom; -import java.util.Arrays; import java.util.Base64; -import javax.crypto.Cipher; import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.GCMParameterSpec; import javax.crypto.spec.PBEKeySpec; -import javax.crypto.spec.SecretKeySpec; import org.apache.ofbiz.base.util.GeneralException; +import org.apache.shiro.crypto.CryptoException; +import org.apache.shiro.crypto.cipher.AesCipherService; +import org.apache.shiro.lang.util.ByteSource; /** * AES-256-GCM encryption/decryption for configuration values such as the @@ -38,26 +36,30 @@ * *

The AES key is derived from a master key (e.g. the {@code OFBIZ_DB_KEY} * environment variable) via PBKDF2WithHmacSHA256, so the same master key always - * yields the same AES key. The IV is randomly generated per encryption and - * stored alongside the ciphertext, both Base64-encoded together.

+ * yields the same AES key. The actual cipher operations are delegated to Shiro's + * {@link AesCipherService} (the same library used by + * {@link org.apache.ofbiz.entity.util.EntityCrypto}), which defaults to GCM mode + * and prepends a random IV to the ciphertext.

*/ public final class ConfigCryptoUtil { - private static final String CIPHER_ALGORITHM = "AES/GCM/NoPadding"; private static final String KEY_DERIVATION_ALGORITHM = "PBKDF2WithHmacSHA256"; // Static salt: the master key is the actual secret; the salt only needs to defeat // precomputed rainbow tables, not provide per-installation uniqueness. private static final byte[] SALT = "OFBizConfigCryptoUtilSalt".getBytes(StandardCharsets.UTF_8); private static final int ITERATIONS = 10000; private static final int KEY_LENGTH_BITS = 256; - private static final int GCM_TAG_LENGTH_BITS = 128; - private static final int IV_LENGTH_BYTES = 12; - private static SecretKeySpec deriveKey(String masterKey) throws GeneralSecurityException { + // Note: AesCipherService derives the GCM tag length from getKeySize(), which must stay at + // its default of 128 (a valid GCM tag length). The 256-bit AES key below is passed directly + // to encrypt/decrypt and does not depend on this setting. + private static final AesCipherService CIPHER_SERVICE = new AesCipherService(); + + private static byte[] deriveKey(String masterKey) throws GeneralSecurityException { SecretKeyFactory factory = SecretKeyFactory.getInstance(KEY_DERIVATION_ALGORITHM); PBEKeySpec spec = new PBEKeySpec(masterKey.toCharArray(), SALT, ITERATIONS, KEY_LENGTH_BITS); try { - return new SecretKeySpec(factory.generateSecret(spec).getEncoded(), "AES"); + return factory.generateSecret(spec).getEncoded(); } finally { spec.clearPassword(); } @@ -69,38 +71,25 @@ private static SecretKeySpec deriveKey(String masterKey) throws GeneralSecurityE */ public static String encrypt(String plainText, String masterKey) throws GeneralException { try { - SecretKeySpec secretKey = deriveKey(masterKey); - byte[] iv = new byte[IV_LENGTH_BYTES]; - new SecureRandom().nextBytes(iv); - Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM); - cipher.init(Cipher.ENCRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); - byte[] cipherText = cipher.doFinal(plainText.getBytes(StandardCharsets.UTF_8)); - byte[] payload = new byte[IV_LENGTH_BYTES + cipherText.length]; - System.arraycopy(iv, 0, payload, 0, IV_LENGTH_BYTES); - System.arraycopy(cipherText, 0, payload, IV_LENGTH_BYTES, cipherText.length); - return Base64.getEncoder().encodeToString(payload); - } catch (GeneralSecurityException e) { + byte[] key = deriveKey(masterKey); + ByteSource encrypted = CIPHER_SERVICE.encrypt(plainText.getBytes(StandardCharsets.UTF_8), key); + return encrypted.toBase64(); + } catch (GeneralSecurityException | CryptoException e) { throw new GeneralException("Unable to encrypt value", e); } } /** - * Reverses {@link #encrypt(String, String)}: derives the AES key from {@code masterKey}, - * splits the IV from the ciphertext and decrypts. + * Reverses {@link #encrypt(String, String)}: derives the AES key from {@code masterKey} + * and lets {@link AesCipherService} split the IV from the ciphertext and decrypt. */ public static String decrypt(String encryptedBase64, String masterKey) throws GeneralException { try { + byte[] key = deriveKey(masterKey); byte[] payload = Base64.getDecoder().decode(encryptedBase64); - if (payload.length <= IV_LENGTH_BYTES) { - throw new GeneralException("Encrypted value is too short to contain an IV and ciphertext"); - } - byte[] iv = Arrays.copyOfRange(payload, 0, IV_LENGTH_BYTES); - byte[] cipherText = Arrays.copyOfRange(payload, IV_LENGTH_BYTES, payload.length); - SecretKeySpec secretKey = deriveKey(masterKey); - Cipher cipher = Cipher.getInstance(CIPHER_ALGORITHM); - cipher.init(Cipher.DECRYPT_MODE, secretKey, new GCMParameterSpec(GCM_TAG_LENGTH_BITS, iv)); - return new String(cipher.doFinal(cipherText), StandardCharsets.UTF_8); - } catch (GeneralSecurityException | IllegalArgumentException e) { + byte[] decrypted = CIPHER_SERVICE.decrypt(payload, key).getClonedBytes(); + return new String(decrypted, StandardCharsets.UTF_8); + } catch (GeneralSecurityException | CryptoException | IllegalArgumentException e) { throw new GeneralException("Unable to decrypt value: verify the master key is correct", e); } } From 4de4287a99e402dbc7e064500d138b7201100ec0 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Fri, 12 Jun 2026 18:38:02 +0530 Subject: [PATCH 08/30] Adding a very important feature which I envisioned before I started this work to add support of secret manager/vaults in ofbiz. In previous commits, We added the six secret manager/vault support in ofbiz. In this commit, I am adding the support of the following points: 1) Add SecretValueResolver to resolve SECRET(key) markers in .properties values via SecretProviderFactory, with TTL cache and passwords.properties fallback. 2) UtilProperties.getPropertyValue now passes resolved values through SecretValueResolver. 3) Add SystemProperty.systemPropertyLookup field; EntityUtilProperties resolves it via SecretValueResolver, falling back to systemPropertyValue (ENC(...) via ConfigCryptoUtil, or plain text). 4) Rename OFBIZ_DB_KEY to OFBIZ_MASTER_KEY across ConfigCryptoUtil, SecretProvider SPI, and docs. 5) Add generateEncryptedSecret Gradle task to produce systemPropertyLookup/systemPropertyValue pairs for SystemProperty rows. 6) Add INFO logging in EntityUtilProperties indicating whether a resolved value came from systemPropertyLookup or systemPropertyValue. 7) jdbc-password-lookup and EntityConfig.getJdbcPassword() remain unchanged and fully backward compatible. 8) Add unit tests for SecretValueResolver. --- .../ofbiz/base/crypto/ConfigCryptoUtil.java | 12 ++--- .../base/secret/FileBasedSecretProvider.java | 2 +- .../ofbiz/base/secret/SecretProvider.java | 2 +- .../ofbiz/base/util/UtilProperties.java | 3 +- framework/common/entitydef/entitymodel.xml | 3 +- .../entity/util/EntityUtilProperties.java | 48 ++++++++++++++++++- 6 files changed, 58 insertions(+), 12 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java index 171f23f1020..ed4b2dd47ee 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -34,7 +34,7 @@ * AES-256-GCM encryption/decryption for configuration values such as the * database passwords stored in {@code passwords.properties} as {@code ENC(...)}. * - *

The AES key is derived from a master key (e.g. the {@code OFBIZ_DB_KEY} + *

The AES key is derived from a master key (e.g. the {@code OFBIZ_MASTER_KEY} * environment variable) via PBKDF2WithHmacSHA256, so the same master key always * yields the same AES key. The actual cipher operations are delegated to Shiro's * {@link AesCipherService} (the same library used by @@ -101,29 +101,29 @@ public static String decrypt(String encryptedBase64, String masterKey) throws Ge *

All {@link org.apache.ofbiz.base.secret.SecretProvider} implementations should * call this after fetching a value from their remote backend so that operators can * optionally add a client-side AES-256-GCM layer on top of whatever the vault already - * provides. The master key is read from the {@code OFBIZ_DB_KEY} environment variable.

+ * provides. The master key is read from the {@code OFBIZ_MASTER_KEY} environment variable.

* * @param rawValue the value as returned by the secret backend (plaintext or {@code ENC(...)}) * @param secretName used only in error messages to identify which secret failed * @return the plaintext value — either the original string or the decrypted one - * @throws GeneralException if the value is encrypted but {@code OFBIZ_DB_KEY} is missing, + * @throws GeneralException if the value is encrypted but {@code OFBIZ_MASTER_KEY} is missing, * or if decryption fails (e.g. wrong key) */ public static String decryptIfEncrypted(String rawValue, String secretName) throws GeneralException { if (!rawValue.startsWith("ENC(") || !rawValue.endsWith(")")) { return rawValue; } - String masterKey = System.getenv("OFBIZ_DB_KEY"); + String masterKey = System.getenv("OFBIZ_MASTER_KEY"); if (masterKey == null || masterKey.isEmpty()) { throw new GeneralException("Secret '" + secretName + "' is encrypted (ENC(...)) but the " - + "OFBIZ_DB_KEY environment variable is not set"); + + "OFBIZ_MASTER_KEY environment variable is not set"); } String base64 = rawValue.substring("ENC(".length(), rawValue.length() - 1); try { return decrypt(base64, masterKey); } catch (GeneralException e) { throw new GeneralException("Failed to decrypt secret '" + secretName - + "': verify OFBIZ_DB_KEY matches the key used to encrypt", e); + + "': verify OFBIZ_MASTER_KEY matches the key used to encrypt", e); } } diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java index 4152b53f19d..6bcffc8cb07 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FileBasedSecretProvider.java @@ -34,7 +34,7 @@ *

Values may optionally be stored encrypted, wrapped as {@code ENC()}, * e.g. {@code jdbc-password.mysql-ofbiz=ENC(AbCd123...)}. Encrypted values are * decrypted in-memory with {@link ConfigCryptoUtil} using the master key supplied - * via the {@code OFBIZ_DB_KEY} environment variable.

+ * via the {@code OFBIZ_MASTER_KEY} environment variable.

*/ public final class FileBasedSecretProvider implements SecretProvider { diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java index 01c97aa7fc9..c431f21d3be 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java @@ -42,7 +42,7 @@ * top of whatever the remote vault already provides. Call * {@code ConfigCryptoUtil.decryptIfEncrypted()} on the raw value returned by * the remote API before caching or returning it. The master key is read from - * the {@code OFBIZ_DB_KEY} environment variable at runtime and is never stored + * the {@code OFBIZ_MASTER_KEY} environment variable at runtime and is never stored * in config files or in the remote vault.

*/ public interface SecretProvider { diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilProperties.java b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilProperties.java index ce9b63e7423..b78cbf32dde 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilProperties.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/util/UtilProperties.java @@ -43,6 +43,7 @@ import java.util.Set; import org.apache.ofbiz.base.location.FlexibleLocation; +import org.apache.ofbiz.base.secret.SecretValueResolver; import org.apache.ofbiz.base.util.cache.UtilCache; import org.apache.ofbiz.base.util.collections.ResourceBundleMapWrapper; import org.apache.ofbiz.base.util.string.FlexibleStringExpander; @@ -287,7 +288,7 @@ public static String getPropertyValue(String resource, String name) { } catch (Exception e) { Debug.logInfo(e, MODULE); } - return value == null ? "" : value.trim(); + return value == null ? "" : SecretValueResolver.resolve(value.trim()); } /** diff --git a/framework/common/entitydef/entitymodel.xml b/framework/common/entitydef/entitymodel.xml index 47385cc61a8..760c7606618 100644 --- a/framework/common/entitydef/entitymodel.xml +++ b/framework/common/entitydef/entitymodel.xml @@ -883,11 +883,12 @@ under the License. - + + diff --git a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java index 1a6a6e90071..11a9b491a3c 100644 --- a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java +++ b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java @@ -34,7 +34,10 @@ import java.util.ResourceBundle; import java.util.Set; +import org.apache.ofbiz.base.crypto.ConfigCryptoUtil; +import org.apache.ofbiz.base.secret.SecretValueResolver; import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.GeneralException; import org.apache.ofbiz.base.util.UtilMisc; import org.apache.ofbiz.base.util.UtilProperties; import org.apache.ofbiz.base.util.UtilValidate; @@ -69,8 +72,7 @@ private static Map getSystemPropertyValue(String resource, Strin if (systemProperty != null) { //property exists in database results.put("isExistInDb", "Y"); - results.put("value", (systemProperty.getString("systemPropertyValue") != null) - ? systemProperty.getString("systemPropertyValue") : ""); + results.put("value", resolveSystemPropertyValue(resource, name, systemProperty)); } } catch (GenericEntityException e) { Debug.logError("Could not get a system property for " + name + " : " + e.getMessage(), MODULE); @@ -78,6 +80,48 @@ private static Map getSystemPropertyValue(String resource, Strin return results; } + /** + * Resolves the effective value of a {@code SystemProperty} record: + *
    + *
  • if {@code systemPropertyLookup} holds a {@code SECRET(key)} reference (see + * {@link SecretValueResolver}), resolves {@code key} via the configured + * {@code SecretProvider} (a remote secret manager, with its own + * {@code passwords.properties} fallback); if that fails, falls back to + * {@code systemPropertyValue}
    • + *
  • otherwise (or on lookup failure), uses {@code systemPropertyValue}, decrypting it + * with {@link ConfigCryptoUtil} if it is wrapped in {@code ENC(...)}
    • + *
+ */ + private static String resolveSystemPropertyValue(String resource, String name, GenericValue systemProperty) { + String lookup = systemProperty.getString("systemPropertyLookup"); + if (UtilValidate.isNotEmpty(lookup)) { + Debug.logInfo("EntityUtilProperties: resolving " + resource + "/" + name + + " via SystemProperty.systemPropertyLookup '" + lookup + "'", MODULE); + String resolved = SecretValueResolver.resolve(lookup); + if (UtilValidate.isNotEmpty(resolved) && !resolved.equals(lookup)) { + return resolved; + } + Debug.logWarning("EntityUtilProperties: systemPropertyLookup '" + lookup + "' for " + resource + "/" + name + + " did not resolve, falling back to systemPropertyValue", MODULE); + } + String value = systemProperty.getString("systemPropertyValue"); + if (value == null) { + return ""; + } + try { + String decrypted = ConfigCryptoUtil.decryptIfEncrypted(value, resource + "." + name); + if (!decrypted.equals(value)) { + Debug.logInfo("EntityUtilProperties: resolved " + resource + "/" + name + + " from SystemProperty.systemPropertyValue (decrypted ENC(...) value)", MODULE); + } + return decrypted; + } catch (GeneralException e) { + Debug.logError("EntityUtilProperties: failed to decrypt systemPropertyValue for " + resource + "/" + name + + ": " + e.getMessage(), MODULE); + return ""; + } + } + public static boolean propertyValueEquals(String resource, String name, String compareString) { return UtilProperties.propertyValueEquals(resource, name, compareString); } From bdc41a1569f8fd98d043e0f13bf602a79e550dd0 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Fri, 12 Jun 2026 18:41:04 +0530 Subject: [PATCH 09/30] Adding the two files SecretValueResolver.java and SecretValueResolverTest.java that I missed in my previous commit. --- .../base/secret/SecretValueResolver.java | 122 ++++++++++++++++++ .../base/secret/SecretValueResolverTest.java | 57 ++++++++ 2 files changed, 179 insertions(+) create mode 100644 framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java new file mode 100644 index 00000000000..6cd5b6c6a64 --- /dev/null +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java @@ -0,0 +1,122 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import java.util.Properties; +import java.util.concurrent.ConcurrentHashMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilProperties; + +/** + * Resolves configuration values of the form {@code SECRET(key)} to the + * corresponding secret value via {@link SecretProviderFactory}. + * + *

This allows any property read through {@code UtilProperties} or + * {@code EntityUtilProperties} (e.g. payment, SMS or shipment gateway + * credentials stored in a {@code .properties} file or as a + * {@code SystemProperty}) to be backed by a remote secret manager, using the + * same {@link SecretProvider} SPI and providers already used for + * {@code jdbc-password-lookup}.

+ * + *

Values that do not match {@code SECRET(key)} are returned unchanged. + * The {@code jdbc-password-lookup} mechanism in {@code entityengine.xml} + * ({@code EntityConfig.getJdbcPassword()}) does not go through this class and + * is unaffected.

+ * + *

The marker name ({@code SECRET} by default) can be changed via the + * {@code secret.value.marker} property in {@code general.properties}, e.g. to + * {@code MASK} or {@code ENCRYPT}, in case it collides with existing property + * values.

+ * + *

Resolved values are cached for {@code secret.cache.ttl.seconds} + * (default 300) to avoid calling the remote secret manager on every property + * lookup.

+ */ +public final class SecretValueResolver { + + private static final String MODULE = SecretValueResolver.class.getName(); + + // Read the marker name and cache TTL directly from the Properties object, bypassing + // UtilProperties.getPropertyValue(), which calls back into resolve() and would otherwise + // deadlock/NPE on these fields during static initialization. + private static final Properties GENERAL_PROPERTIES = UtilProperties.getProperties("general"); + + private static final String MARKER_NAME = GENERAL_PROPERTIES != null + ? GENERAL_PROPERTIES.getProperty("secret.value.marker", "SECRET").trim() + : "SECRET"; + + private static final Pattern SECRET_PATTERN = + Pattern.compile("^" + Pattern.quote(MARKER_NAME) + "\\((.+)\\)$"); + + private static final long CACHE_TTL_MILLIS = (GENERAL_PROPERTIES != null + ? Long.parseLong(GENERAL_PROPERTIES.getProperty("secret.cache.ttl.seconds", "300").trim()) + : 300L) * 1000L; + + private static final ConcurrentHashMap CACHE = new ConcurrentHashMap<>(); + + private SecretValueResolver() { } + + /** + * If {@code rawValue} matches {@code SECRET(key)}, resolves {@code key} via + * {@link SecretProviderFactory#getInstance()}, caching the result for + * {@code secret.cache.ttl.seconds}. Otherwise returns {@code rawValue} unchanged. + * + * @param rawValue the raw property value, possibly {@code null} + * @return the resolved secret value, or {@code rawValue} unchanged if it is not a + * {@code SECRET(key)} reference; an empty string if resolution fails + */ + public static String resolve(String rawValue) { + if (rawValue == null) { + return null; + } + Matcher matcher = SECRET_PATTERN.matcher(rawValue.trim()); + if (!matcher.matches()) { + return rawValue; + } + String key = matcher.group(1).trim(); + + CacheEntry cached = CACHE.get(key); + if (cached != null && cached.expiry > System.currentTimeMillis()) { + return cached.value; + } + + try { + String secret = SecretProviderFactory.getInstance().getSecret(key); + CACHE.put(key, new CacheEntry(secret, System.currentTimeMillis() + CACHE_TTL_MILLIS)); + return secret; + } catch (GeneralException e) { + Debug.logError("SecretValueResolver: failed to resolve secret '" + key + "': " + e.getMessage(), MODULE); + return ""; + } + } + + private static final class CacheEntry { + private final String value; + private final long expiry; + + private CacheEntry(String value, long expiry) { + this.value = value; + this.expiry = expiry; + } + } +} diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java new file mode 100644 index 00000000000..ab9e3d95776 --- /dev/null +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java @@ -0,0 +1,57 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import org.junit.Test; + +/** + * Tests {@link SecretValueResolver} using the default {@link FileBasedSecretProvider} + * (resolved via {@link SecretProviderFactory} since no plugin-provided {@link SecretProvider} + * is on the test classpath) and the {@code jdbc-password.h2-ofbiz} entry already present in + * {@code framework/base/config/passwords.properties}. + */ +public class SecretValueResolverTest { + + @Test + public void nonSecretValuesAreReturnedUnchanged() { + assertEquals("plainvalue", SecretValueResolver.resolve("plainvalue")); + assertEquals("ENC(AbCd123)", SecretValueResolver.resolve("ENC(AbCd123)")); + assertEquals("", SecretValueResolver.resolve("")); + } + + @Test + public void nullIsReturnedUnchanged() { + assertNull(SecretValueResolver.resolve(null)); + } + + @Test + public void resolvesKnownKeyFromPasswordsProperties() { + assertEquals("ofbiz", SecretValueResolver.resolve("SECRET(jdbc-password.h2-ofbiz)")); + // Cached lookup should return the same value. + assertEquals("ofbiz", SecretValueResolver.resolve("SECRET(jdbc-password.h2-ofbiz)")); + } + + @Test + public void unresolvableKeyReturnsEmptyString() { + assertEquals("", SecretValueResolver.resolve("SECRET(jdbc-password.does-not-exist)")); + } +} From a490fd7c0043b97693a73a4cc85e8a2b35a6e57e Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Fri, 12 Jun 2026 23:56:05 +0530 Subject: [PATCH 10/30] Committing the missed build.gradle file in which I added a new gradle task to generate encrypted secrets/password on the console and that can be used to setup data in SystemProperty file. --- build.gradle | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/build.gradle b/build.gradle index a940122550f..71672527c9b 100644 --- a/build.gradle +++ b/build.gradle @@ -775,6 +775,35 @@ task generateDBPassword(group: sysadminGroup, } } +task generateEncryptedSecret(group: sysadminGroup, + description: 'Generate an encrypted secret (ENC(...)) and its systemPropertyLookup marker, for pasting ' + + 'into a SystemProperty record (e.g. payment/SMS/shipment gateway credentials). ' + + 'Usage: ./gradlew generateEncryptedSecret -PsecretPassword= -PlookupKey= -PmasterKey=') { + doLast { + def secret = project.property('secretPassword') + def lookupKey = project.property('lookupKey') + def masterKey = project.property('masterKey') + + def markerName = 'SECRET' + def generalPropsFile = file('framework/common/config/general.properties') + if (generalPropsFile.exists()) { + def generalProps = new Properties() + generalPropsFile.withInputStream { generalProps.load(it) } + markerName = generalProps.getProperty('secret.value.marker', markerName) + } + + def output = new java.io.ByteArrayOutputStream() + javaexec { + classpath = sourceSets.main.runtimeClasspath + mainClass = 'org.apache.ofbiz.base.crypto.ConfigCryptoUtil' + args masterKey, secret + standardOutput = output + } + println "systemPropertyLookup=${markerName}(${lookupKey})" + println "systemPropertyValue=${output.toString().trim()}" + } +} + task generateSecretKeys(group: sysadminGroup, description: 'Generate cryptographically secure 512-bit (64-char) secret keys for JWT token signing and password encryption, and write them to security.properties') { doLast { From 229d62bfb16426394b08ac67a284a7d7075c858a Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Sat, 13 Jun 2026 16:13:28 +0530 Subject: [PATCH 11/30] I renamed the gradle task and also renamed the master key to OFBIZ_MASTER_KEY. So updating the information in Javadoc. --- .../org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java index ed4b2dd47ee..6e819c7c51b 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -131,9 +131,10 @@ private ConfigCryptoUtil() { } /** * Command-line helper that prints {@code ENC()} for a given master key and plaintext - * value, ready to be pasted into {@code passwords.properties} (e.g. as - * {@code jdbc-password.=ENC(...)}). Invoked via the {@code encryptDbPassword} - * Gradle task. + * value. Invoked via the {@code generateDBPassword} and {@code generateEncryptedSecret} + * Gradle tasks to produce values for {@code passwords.properties} + * ({@code jdbc-password.=ENC(...)}) and {@code SystemProperty.systemPropertyValue} + * respectively. */ public static void main(String[] args) throws GeneralException { if (args.length != 2) { From 400b9e11e4dbd089e9ce3e98386cde3b1bb1e22d Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Sun, 14 Jun 2026 22:47:00 +0530 Subject: [PATCH 12/30] Add masked stdin prompts for generateDBPassword and generateEncryptedSecret, so secrets/master keys never need to be passed via -P (and end up in shell history or ps). Falls back to OFBIZ_MASTER_KEY env var when no master key is supplied; existing -P based usage is unchanged. --- build.gradle | 68 +++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 60 insertions(+), 8 deletions(-) diff --git a/build.gradle b/build.gradle index 71672527c9b..69112537413 100644 --- a/build.gradle +++ b/build.gradle @@ -749,17 +749,63 @@ task gitInfoFooter(group: sysadminGroup, description: 'Update the Git Branch-rev } } +// System.console() is unreliable under Gradle (it is almost always null, even with +// --console=plain), so read directly from stdin instead. This still requires --no-daemon, +// since the Gradle daemon is detached from the terminal and stdin is not forwarded to it. +def stdinReader = new BufferedReader(new InputStreamReader(System.in)) + +// Reads a non-sensitive value (e.g. a lookup key) from stdin. +def promptValue = { String prompt -> + print prompt + System.out.flush() + def line = stdinReader.readLine() + if (!line) { + throw new GradleException("No value entered. Re-run with --no-daemon from an interactive " + + "terminal, or pass the value via -P.") + } + line +} + +// Reads a sensitive value (password/master key) from stdin, masking the terminal echo via +// `stty` (when a tty is available) so it never appears in shell history, `ps`, or CI logs. +def promptSecret = { String prompt -> + print prompt + System.out.flush() + def ttyAvailable = new File('/dev/tty').exists() + if (ttyAvailable) { + ['sh', '-c', 'stty -echo < /dev/tty'].execute().waitFor() + } + try { + def line = stdinReader.readLine() + if (!line) { + throw new GradleException("No value entered. Re-run with --no-daemon from an interactive " + + "terminal, or pass the value via -P.") + } + return line + } finally { + if (ttyAvailable) { + ['sh', '-c', 'stty echo < /dev/tty'].execute().waitFor() + } + println() + } +} + task generateDBPassword(group: sysadminGroup, description: 'Write a plain or encrypted (ENC(...)) database password into passwords.properties. ' - + 'Usage: ./gradlew generateDBPassword -PdbPassword= -PlookupKey= [-PmasterKey=]') { + + 'Usage: ./gradlew generateDBPassword -PdbPassword= -PlookupKey= [-PmasterKey=]. ' + + 'If -PdbPassword is omitted, you will be prompted for it via masked console input ' + + '(run with --no-daemon --console=plain). The master key is taken from -PmasterKey, ' + + 'falling back to the OFBIZ_MASTER_KEY environment variable.') { doLast { - def secret = project.property('dbPassword') - if (project.hasProperty('masterKey')) { + def secret = project.hasProperty('dbPassword') ? project.property('dbPassword') + : promptSecret('Enter database password: ') + def masterKey = project.hasProperty('masterKey') ? project.property('masterKey') : System.getenv('OFBIZ_MASTER_KEY') + if (masterKey) { def output = new java.io.ByteArrayOutputStream() javaexec { classpath = sourceSets.main.runtimeClasspath mainClass = 'org.apache.ofbiz.base.crypto.ConfigCryptoUtil' - args project.property('masterKey'), secret + args masterKey, secret standardOutput = output } secret = output.toString().trim() @@ -778,11 +824,17 @@ task generateDBPassword(group: sysadminGroup, task generateEncryptedSecret(group: sysadminGroup, description: 'Generate an encrypted secret (ENC(...)) and its systemPropertyLookup marker, for pasting ' + 'into a SystemProperty record (e.g. payment/SMS/shipment gateway credentials). ' - + 'Usage: ./gradlew generateEncryptedSecret -PsecretPassword= -PlookupKey= -PmasterKey=') { + + 'Usage: ./gradlew generateEncryptedSecret -PsecretPassword= -PlookupKey= -PmasterKey=. ' + + 'Any of -PsecretPassword/-PlookupKey/-PmasterKey may be omitted to be prompted for ' + + 'interactively (run with --no-daemon --console=plain); the master key also falls back ' + + 'to the OFBIZ_MASTER_KEY environment variable.') { doLast { - def secret = project.property('secretPassword') - def lookupKey = project.property('lookupKey') - def masterKey = project.property('masterKey') + def secret = project.hasProperty('secretPassword') ? project.property('secretPassword') + : promptSecret('Enter secret value: ') + def lookupKey = project.hasProperty('lookupKey') ? project.property('lookupKey') + : promptValue('Enter lookup key: ') + def masterKey = project.hasProperty('masterKey') ? project.property('masterKey') + : (System.getenv('OFBIZ_MASTER_KEY') ?: promptSecret('Enter master key: ')) def markerName = 'SECRET' def generalPropsFile = file('framework/common/config/general.properties') From 723ca7f804221bc4cffd79ae721c941b291e696a Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Tue, 16 Jun 2026 16:14:00 +0530 Subject: [PATCH 13/30] Adding a support of Secret Manager screen to add encrypted secrets and passwords either in SystemProperty or passwords.properties file. Also added the bulk secret creation option. Also added the Sandboxing support for the entered csv file so that someone couldn't not add malicious file. --- .../webtools/config/WebtoolsUiLabels.xml | 77 +++++ framework/webtools/servicedef/services.xml | 12 + .../webtools/secret/SecretManagerEvents.java | 261 +++++++++++++++++ .../secret/SecretManagerServices.java | 269 ++++++++++++++++++ framework/webtools/template/Main.ftl | 2 + .../webtools/template/secret/EncryptValue.ftl | 150 ++++++++++ .../webapp/webtools/WEB-INF/controller.xml | 19 ++ .../webtools/widget/SecretManagerScreens.xml | 42 +++ 8 files changed, 832 insertions(+) create mode 100644 framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java create mode 100644 framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java create mode 100644 framework/webtools/template/secret/EncryptValue.ftl create mode 100644 framework/webtools/widget/SecretManagerScreens.xml diff --git a/framework/webtools/config/WebtoolsUiLabels.xml b/framework/webtools/config/WebtoolsUiLabels.xml index 1df293e5edc..1dbd39703ee 100644 --- a/framework/webtools/config/WebtoolsUiLabels.xml +++ b/framework/webtools/config/WebtoolsUiLabels.xml @@ -2016,6 +2016,83 @@ 实体XML表述 資料實體XML表述 + + Encrypt Value + + + Secret Manager Tools + + + Encrypt a secret value with AES-256-GCM (using the server's OFBIZ_MASTER_KEY) and store + the result either in a SystemProperty record or in framework/base/config/passwords.properties. + The encrypted value is not displayed. If the target SystemProperty record or + passwords.properties entry already exists, it is updated with the new encrypted value; + otherwise it is created. + + + For SystemProperty: Resource ID and Property ID are required. Lookup Key + is optional. + + + For passwords.properties: Lookup Key is always required. + Leave Resource ID and Property ID empty to write only to passwords.properties + (entityengine.xml jdbc-password-lookup use-case). + Provide all four fields to also update the matching property file on disk + (sets <propertyId>=SECRET(<lookupKey>)) and refresh the in-memory + property cache — no server restart needed. + + + Store In + + + SystemProperty entity + + + passwords.properties + + + System Resource ID (SystemProperty) + + + System Property ID (SystemProperty) + + + Resource ID (Property File) + + + Property ID (Property File) + + + Lookup Key + + + Required for passwords.properties. When Resource ID and Property ID are empty, + stored as jdbc-password.<lookupKey> (entityengine.xml use-case). When all four fields are + provided, stored as <lookupKey> directly (property file use-case). + Optional for SystemProperty: if set, systemPropertyLookup is set to SECRET(<lookupKey>) + so a configured remote secret provider is tried first. + + + Secret Value + + + Encrypt and Save + + + Or upload a CSV file to create multiple secrets at once: + + + Columns (header row required): target, systemResourceId, systemPropertyId, lookupKey, secretValue + + + target is SYSTEM_PROPERTY or PASSWORDS_FILE; leave unused columns empty + + + CSV File + + + Upload and Encrypt + Entität XML Tools Entity XML Tools diff --git a/framework/webtools/servicedef/services.xml b/framework/webtools/servicedef/services.xml index dcbb320b79b..1a957b02273 100644 --- a/framework/webtools/servicedef/services.xml +++ b/framework/webtools/servicedef/services.xml @@ -122,6 +122,18 @@ under the License. if the user has the ENTITY_MAINT permission. + + Encrypts a secret value with ConfigCryptoUtil (AES-256-GCM, keyed by the OFBIZ_MASTER_KEY + environment variable) and stores the resulting ENC(...) value either as a SystemProperty.systemPropertyValue + or as a jdbc-password.<lookupKey> entry in passwords.properties. + + + + + + + Saves service and related artifacts diagram to an Apple EOModelBundle file. diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java new file mode 100644 index 00000000000..9a2d9073108 --- /dev/null +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java @@ -0,0 +1,261 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.webtools.secret; + +import java.io.ByteArrayInputStream; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.regex.Pattern; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import org.apache.commons.csv.CSVFormat; +import org.apache.commons.csv.CSVParser; +import org.apache.commons.csv.CSVRecord; +import org.apache.commons.fileupload2.core.DiskFileItem; +import org.apache.commons.fileupload2.core.FileItem; + +import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.UtilGenerics; +import org.apache.ofbiz.base.util.UtilValidate; +import org.apache.ofbiz.entity.Delegator; +import org.apache.ofbiz.entity.GenericValue; +import org.apache.ofbiz.security.SecuredUpload; +import org.apache.ofbiz.security.Security; + +/** + * Handles bulk creation of encrypted secrets from a CSV upload on the webtools "Encrypt Value" + * screen. Each row is processed by {@link SecretManagerServices#storeEncryptedSecret}, the same + * logic used by the single-entry form. + * + *

Expected CSV header: {@code target,systemResourceId,systemPropertyId,lookupKey,secretValue}. + * {@code target} is either {@code SYSTEM_PROPERTY} or {@code PASSWORDS_FILE}.

+ */ +public final class SecretManagerEvents { + + private static final String MODULE = SecretManagerEvents.class.getName(); + private static final String UPLOAD_FIELD_NAME = "uploadedFile"; + + private static final int MAX_FILE_SIZE_BYTES = 512 * 1024; // 512 KB + private static final int MAX_ROWS = 500; + private static final List REQUIRED_HEADERS = + List.of("target", "systemResourceId", "systemPropertyId", "lookupKey", "secretValue"); + private static final Set VALID_TARGETS = Set.of( + SecretManagerServices.TARGET_SYSTEM_PROPERTY, SecretManagerServices.TARGET_PASSWORDS_FILE); + /** Allows only letters, digits, dots, hyphens, underscores — prevents path traversal and property-file injection. */ + private static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[\\w.\\-]+$"); + + private SecretManagerEvents() { } + + public static String uploadEncryptedSecrets(HttpServletRequest request, HttpServletResponse response) { + Delegator delegator = (Delegator) request.getAttribute("delegator"); + Security security = (Security) request.getAttribute("security"); + GenericValue userLogin = (GenericValue) request.getSession().getAttribute("userLogin"); + + if (security == null || !security.hasPermission("ENTITY_MAINT", userLogin)) { + request.setAttribute("_ERROR_MESSAGE_", "You do not have permission to perform this operation"); + return "error"; + } + + byte[] csvBytes = getUploadedFileBytes(request); + if (csvBytes == null || csvBytes.length == 0) { + request.setAttribute("_ERROR_MESSAGE_", "No CSV file was uploaded"); + return "error"; + } + + if (csvBytes.length > MAX_FILE_SIZE_BYTES) { + request.setAttribute("_ERROR_MESSAGE_", + "CSV file exceeds the maximum allowed size of " + (MAX_FILE_SIZE_BYTES / 1024) + " KB"); + return "error"; + } + + // Pre-scan: validate every row before writing anything — reject the entire file on any issue + List validationErrors; + try { + validationErrors = validateCsvContent(csvBytes); + } catch (IllegalArgumentException e) { + request.setAttribute("_ERROR_MESSAGE_", "CSV file is malformed: " + e.getMessage()); + return "error"; + } catch (IOException e) { + Debug.logError(e, MODULE); + request.setAttribute("_ERROR_MESSAGE_", "Error reading CSV file: " + e.getMessage()); + return "error"; + } + if (!validationErrors.isEmpty()) { + request.setAttribute("_ERROR_MESSAGE_LIST_", validationErrors); + return "error"; + } + + int successCount = 0; + List errors = new ArrayList<>(); + CSVFormat format = buildCsvFormat(); + try (Reader reader = new InputStreamReader(new ByteArrayInputStream(csvBytes), StandardCharsets.UTF_8); + CSVParser parser = format.parse(reader)) { + for (CSVRecord record : parser) { + long rowNum = record.getRecordNumber() + 1; + try { + SecretManagerServices.storeEncryptedSecret(delegator, + getColumn(record, "target"), + getColumn(record, "systemResourceId"), + getColumn(record, "systemPropertyId"), + getColumn(record, "lookupKey"), + getColumn(record, "secretValue")); + successCount++; + } catch (Exception e) { + errors.add("Row " + rowNum + ": " + e.getMessage()); + } + } + } catch (IOException e) { + Debug.logError(e, MODULE); + request.setAttribute("_ERROR_MESSAGE_", "Error parsing CSV file: " + e.getMessage()); + return "error"; + } + + request.setAttribute("_EVENT_MESSAGE_", successCount + " secret(s) encrypted and stored successfully"); + if (!errors.isEmpty()) { + request.setAttribute("_ERROR_MESSAGE_LIST_", errors); + return "error"; + } + return "success"; + } + + private static CSVFormat buildCsvFormat() { + return CSVFormat.DEFAULT.builder() + .setHeader() + .setSkipHeaderRecord(true) + .setTrim(true) + .setIgnoreEmptyLines(true) + .get(); + } + + /** + * Validates every row of the CSV before any writes occur. Returns a list of human-readable + * error messages (one per problem found). An empty list means the file is safe to process. + * + *

Checks performed:

+ *
    + *
  • Whole-file content scan via {@link SecuredUpload#isValidTextContent} — rejects null + * bytes and C0/C1 control characters at the Unicode code-point level before any parsing.
    • + *
  • All required column headers are present.
    • + *
  • Row count does not exceed {@link #MAX_ROWS}.
    • + *
  • {@code target} is one of the two known constants.
    • + *
  • {@code systemResourceId}, {@code systemPropertyId}, {@code lookupKey} contain only + * safe identifier characters (letters, digits, dots, hyphens, underscores) and no + * {@code ..} path-traversal sequences.
    • + *
+ */ + private static List validateCsvContent(byte[] csvBytes) throws IOException { + List errors = new ArrayList<>(); + + // Whole-file content scan using OFBiz's existing SecuredUpload allow-list validator. + // Rejects null bytes and C0/C1 control characters at the Unicode code-point level — + // more robust than a simple char scan because it cannot be bypassed by encoding tricks. + String csvText = new String(csvBytes, StandardCharsets.UTF_8); + if (!SecuredUpload.isValidTextContent(csvText)) { + errors.add("CSV file contains illegal characters (null bytes or control characters) and cannot be processed"); + return errors; + } + + try (Reader reader = new InputStreamReader(new ByteArrayInputStream(csvBytes), StandardCharsets.UTF_8); + CSVParser parser = buildCsvFormat().parse(reader)) { + // Required headers + Map headers = parser.getHeaderMap(); + for (String col : REQUIRED_HEADERS) { + if (!headers.containsKey(col)) { + errors.add("Missing required column header: '" + col + "'"); + } + } + if (!errors.isEmpty()) { + return errors; // can't validate rows without the correct headers + } + int rowCount = 0; + for (CSVRecord record : parser) { + if (++rowCount > MAX_ROWS) { + errors.add("File exceeds the maximum of " + MAX_ROWS + " data rows"); + break; + } + long rowNum = record.getRecordNumber() + 1; + String target = getColumn(record, "target"); + String resourceId = getColumn(record, "systemResourceId"); + String propertyId = getColumn(record, "systemPropertyId"); + String lookupKey = getColumn(record, "lookupKey"); + + // target must be a known constant + if (target != null && !VALID_TARGETS.contains(target)) { + errors.add("Row " + rowNum + ": unknown target '" + target + + "' — expected PASSWORDS_FILE or SYSTEM_PROPERTY"); + } + // identifier fields: safe chars only + no path traversal + validateIdentifier(errors, rowNum, "systemResourceId", resourceId); + validateIdentifier(errors, rowNum, "systemPropertyId", propertyId); + validateIdentifier(errors, rowNum, "lookupKey", lookupKey); + } + } + return errors; + } + + /** Validates that {@code value} contains only safe identifier characters and no {@code ..}. */ + private static void validateIdentifier(List errors, long rowNum, String field, String value) { + if (value == null) { + return; + } + if (value.contains("..")) { + errors.add("Row " + rowNum + ": '" + field + "' must not contain '..' (path traversal)"); + return; + } + if (!SAFE_IDENTIFIER.matcher(value).matches()) { + errors.add("Row " + rowNum + ": '" + field + + "' contains invalid characters — only letters, digits, dots, hyphens, and underscores are allowed"); + } + } + + private static String getColumn(CSVRecord record, String name) { + if (!record.isMapped(name)) { + return null; + } + String value = record.get(name); + return UtilValidate.isEmpty(value) ? null : value; + } + + /** + * Returns the bytes of the uploaded {@code uploadedFile} part. The multipart body has already + * been parsed by {@code ControlFilter} (via {@code UtilHttp.getParameterMap}), which stashes the + * parsed {@link FileItem}s in the {@code fileItems} request attribute - the underlying request + * input stream can not be re-parsed here. + */ + private static byte[] getUploadedFileBytes(HttpServletRequest request) { + List> items = UtilGenerics.cast(request.getAttribute("fileItems")); + if (items == null) { + return null; + } + for (FileItem item : items) { + if (!item.isFormField() && UPLOAD_FIELD_NAME.equals(item.getFieldName())) { + return item.get(); + } + } + return null; + } +} diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java new file mode 100644 index 00000000000..73681a7a8e2 --- /dev/null +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java @@ -0,0 +1,269 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.webtools.secret; + +import java.io.File; +import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URISyntaxException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Map; + +import org.apache.ofbiz.base.component.ComponentConfig; +import org.apache.ofbiz.base.crypto.ConfigCryptoUtil; +import org.apache.ofbiz.base.location.FlexibleLocation; +import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilValidate; +import org.apache.ofbiz.base.util.cache.UtilCache; +import org.apache.ofbiz.entity.Delegator; +import org.apache.ofbiz.entity.GenericValue; +import org.apache.ofbiz.entity.util.EntityQuery; +import org.apache.ofbiz.service.DispatchContext; +import org.apache.ofbiz.service.ServiceUtil; + +/** + * Encrypts secret/password values with {@link ConfigCryptoUtil} (AES-256-GCM, keyed by the + * {@code OFBIZ_MASTER_KEY} environment variable) and stores the resulting {@code ENC(...)} value + * either in a {@code SystemProperty} record or as a {@code jdbc-password.} entry in + * {@code framework/base/config/passwords.properties}. + */ +public final class SecretManagerServices { + + private static final String MODULE = SecretManagerServices.class.getName(); + + public static final String TARGET_SYSTEM_PROPERTY = "SYSTEM_PROPERTY"; + public static final String TARGET_PASSWORDS_FILE = "PASSWORDS_FILE"; + + private static final String PASSWORDS_FILE_LOCATION = "component://base/config/passwords.properties"; + private static final String JDBC_PASSWORD_PREFIX = "jdbc-password."; + + private SecretManagerServices() { } + + /** Service implementation for the webtools "Encrypt Value" screen. */ + public static Map createEncryptedSecret(DispatchContext dctx, Map context) { + Delegator delegator = dctx.getDelegator(); + String secretTarget = (String) context.get("secretTarget"); + String systemResourceId = (String) context.get("systemResourceId"); + String systemPropertyId = (String) context.get("systemPropertyId"); + String lookupKey = (String) context.get("lookupKey"); + String secretValue = (String) context.get("secretValue"); + + try { + storeEncryptedSecret(delegator, secretTarget, systemResourceId, systemPropertyId, lookupKey, secretValue); + } catch (GeneralException e) { + Debug.logError(e, MODULE); + return ServiceUtil.returnError(e.getMessage()); + } + return ServiceUtil.returnSuccess("Secret value encrypted and stored successfully"); + } + + /** + * Encrypts {@code secretValue} and stores it as configured by {@code secretTarget}. Used by + * both the single-entry service ({@link #createEncryptedSecret}) and the CSV bulk-upload + * event so that both paths share the exact same validation and storage logic. + */ + public static void storeEncryptedSecret(Delegator delegator, String secretTarget, String systemResourceId, + String systemPropertyId, String lookupKey, String secretValue) throws GeneralException { + if (UtilValidate.isEmpty(secretValue)) { + throw new GeneralException("secretValue is required"); + } + String encryptedValue = "ENC(" + ConfigCryptoUtil.encrypt(secretValue, getMasterKey()) + ")"; + + if (TARGET_PASSWORDS_FILE.equals(secretTarget)) { + if (UtilValidate.isEmpty(lookupKey)) { + throw new GeneralException("lookupKey is required for passwords.properties"); + } + boolean hasResourceId = UtilValidate.isNotEmpty(systemResourceId); + boolean hasPropertyId = UtilValidate.isNotEmpty(systemPropertyId); + if (hasResourceId != hasPropertyId) { + throw new GeneralException( + "systemResourceId and systemPropertyId must both be provided together"); + } + if (hasResourceId) { + // Case B: property-file combined write — use plain lookupKey, no jdbc-password. prefix + writePasswordsProperty(lookupKey, encryptedValue); + updatePropertiesFileAndRefreshCache(systemResourceId, systemPropertyId, lookupKey); + } else { + // Case A: entityengine.xml jdbc-password-lookup — keep the jdbc-password. prefix + writePasswordsProperty(JDBC_PASSWORD_PREFIX + lookupKey, encryptedValue); + } + } else if (TARGET_SYSTEM_PROPERTY.equals(secretTarget)) { + if (UtilValidate.isEmpty(systemResourceId) || UtilValidate.isEmpty(systemPropertyId)) { + throw new GeneralException("systemResourceId and systemPropertyId are required for SystemProperty"); + } + storeSystemPropertySecret(delegator, systemResourceId, systemPropertyId, lookupKey, encryptedValue); + } else { + throw new GeneralException("Unknown secretTarget '" + secretTarget + "'"); + } + } + + private static void storeSystemPropertySecret(Delegator delegator, String systemResourceId, String systemPropertyId, + String lookupKey, String encryptedValue) throws GeneralException { + GenericValue systemProperty = EntityQuery.use(delegator).from("SystemProperty") + .where("systemResourceId", systemResourceId, "systemPropertyId", systemPropertyId) + .queryOne(); + if (systemProperty == null) { + systemProperty = delegator.makeValue("SystemProperty", + "systemResourceId", systemResourceId, "systemPropertyId", systemPropertyId); + } + systemProperty.set("systemPropertyValue", encryptedValue); + if (UtilValidate.isNotEmpty(lookupKey)) { + systemProperty.set("systemPropertyLookup", "SECRET(" + lookupKey + ")"); + } + delegator.createOrStore(systemProperty); + } + + /** + * Searches all loaded OFBiz components for {@code config/.properties}, + * sets {@code =SECRET()} in the source file, then clears the + * OFBiz property caches so the change is picked up immediately without a server restart. + * + *

If the property previously referenced a different lookup key via {@code SECRET(old-key)}, + * the stale {@code old-key} entry is removed from passwords.properties before the new one is written.

+ * + *

Scanning component directories (not the classpath) ensures we write to the actual + * source {@code .properties} file rather than the build-output copy.

+ */ + private static void updatePropertiesFileAndRefreshCache(String systemResourceId, + String systemPropertyId, String lookupKey) throws GeneralException { + File propsFile = findSourcePropertiesFile(systemResourceId); + if (propsFile == null) { + throw new GeneralException( + "Properties file not found for resource: " + systemResourceId); + } + // If the property already points to a different lookup key, remove the stale passwords.properties entry + String existingLookupKey = readSecretLookupKey(propsFile, systemPropertyId); + if (existingLookupKey != null && !existingLookupKey.equals(lookupKey)) { + removePasswordsEntry(existingLookupKey); + Debug.logInfo("Removed stale passwords.properties entry for old lookup key: " + existingLookupKey, MODULE); + } + writePropertiesEntry(propsFile, systemPropertyId, "SECRET(" + lookupKey + ")"); + UtilCache.clearCachesThatStartWith("properties.UtilProperties"); + Debug.logInfo("Set " + systemPropertyId + "=SECRET(" + lookupKey + ") in " + + propsFile.getPath() + " and refreshed property caches", MODULE); + } + + /** + * Reads {@code file} and returns the key inside {@code SECRET()} for the given + * {@code propertyId}, or {@code null} if the property is absent or not a SECRET reference. + */ + private static String readSecretLookupKey(File file, String propertyId) throws GeneralException { + try { + String linePrefix = propertyId + "="; + for (String line : Files.readAllLines(file.toPath(), StandardCharsets.UTF_8)) { + if (line.startsWith(linePrefix)) { + String value = line.substring(linePrefix.length()).trim(); + if (value.startsWith("SECRET(") && value.endsWith(")")) { + return value.substring("SECRET(".length(), value.length() - 1); + } + return null; + } + } + return null; + } catch (IOException e) { + throw new GeneralException("Unable to read " + file.getName() + ": " + e.getMessage(), e); + } + } + + /** Removes the line for {@code key} from passwords.properties (no-op if not present). */ + private static void removePasswordsEntry(String key) throws GeneralException { + removePropertiesEntry(getPasswordsFile(), key); + } + + /** Removes the {@code key=...} line from {@code file} (no-op if not present). */ + private static synchronized void removePropertiesEntry(File file, String key) throws GeneralException { + try { + List lines = Files.readAllLines(file.toPath(), StandardCharsets.UTF_8); + String linePrefix = key + "="; + if (lines.removeIf(line -> line.startsWith(linePrefix))) { + Files.write(file.toPath(), lines, StandardCharsets.UTF_8); + } + } catch (IOException e) { + throw new GeneralException("Unable to update " + file.getName() + ": " + e.getMessage(), e); + } + } + + /** Scans all loaded component {@code config/} directories for {@code .properties}. */ + private static File findSourcePropertiesFile(String systemResourceId) { + String fileName = systemResourceId + ".properties"; + for (ComponentConfig cc : ComponentConfig.getAllComponents()) { + Path candidate = cc.rootLocation().resolve("config").resolve(fileName); + if (Files.isRegularFile(candidate)) { + return candidate.toFile(); + } + } + return null; + } + + /** Updates (or appends) {@code key=value} in an arbitrary properties file, preserving all other lines. */ + private static synchronized void writePropertiesEntry(File file, String key, String value) + throws GeneralException { + try { + List lines = Files.readAllLines(file.toPath(), StandardCharsets.UTF_8); + String newLine = key + "=" + value; + String linePrefix = key + "="; + int index = -1; + for (int i = 0; i < lines.size(); i++) { + if (lines.get(i).startsWith(linePrefix)) { + index = i; + break; + } + } + if (index >= 0) { + lines.set(index, newLine); + } else { + lines.add(newLine); + } + Files.write(file.toPath(), lines, StandardCharsets.UTF_8); + } catch (IOException e) { + throw new GeneralException( + "Unable to update " + file.getName() + ": " + e.getMessage(), e); + } + } + + /** Updates (or appends) {@code propertyName=value} in passwords.properties, preserving all other lines. */ + private static void writePasswordsProperty(String propertyName, String value) throws GeneralException { + writePropertiesEntry(getPasswordsFile(), propertyName, value); + } + + private static File getPasswordsFile() throws GeneralException { + try { + URL url = FlexibleLocation.resolveLocation(PASSWORDS_FILE_LOCATION); + if (url == null) { + throw new GeneralException("Unable to locate passwords.properties"); + } + return new File(url.toURI()); + } catch (MalformedURLException | URISyntaxException e) { + throw new GeneralException("Unable to locate passwords.properties: " + e.getMessage(), e); + } + } + + private static String getMasterKey() throws GeneralException { + String masterKey = System.getenv("OFBIZ_MASTER_KEY"); + if (UtilValidate.isEmpty(masterKey)) { + throw new GeneralException("The OFBIZ_MASTER_KEY environment variable is not set on the server"); + } + return masterKey; + } +} diff --git a/framework/webtools/template/Main.ftl b/framework/webtools/template/Main.ftl index 3fe4b097802..9b81727adb9 100644 --- a/framework/webtools/template/Main.ftl +++ b/framework/webtools/template/Main.ftl @@ -80,6 +80,8 @@ under the License.
  • ${uiLabelMap.PageTitleEntityImport}
  • ${uiLabelMap.PageTitleEntityImportDir}
  • ${uiLabelMap.PageTitleEntityImportReaders}
    • +

  • ${uiLabelMap.WebtoolsSecretManagerTools}

    • +
  • ${uiLabelMap.WebtoolsEncryptValue}
    • <#if security.hasPermission("SERVICE_MAINT", session)>

  • ${uiLabelMap.WebtoolsServiceEngineTools}

    • diff --git a/framework/webtools/template/secret/EncryptValue.ftl b/framework/webtools/template/secret/EncryptValue.ftl new file mode 100644 index 00000000000..c2754db8d20 --- /dev/null +++ b/framework/webtools/template/secret/EncryptValue.ftl @@ -0,0 +1,150 @@ +<#-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you 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. +--> + +

    ${uiLabelMap.WebtoolsEncryptValueInfo}

    +
      +
    • ${uiLabelMap.WebtoolsSecretSystemPropertyRequiredInfo}
      • +
    • ${uiLabelMap.WebtoolsSecretPasswordsFileRequiredInfo}
      • +
    + +<#assign inlineEventList = eventMessageList![]/> +<#assign inlineErrorList = errorMessageList![]/> +<#if inlineEventList?has_content> +
    + <#list inlineEventList as msg>

    ${msg}

    +
    + +<#if inlineErrorList?has_content> +
    + <#list inlineErrorList as err>

    ${err}

    +
    + + +
    + +<#assign prevTarget = parameters.secretTarget!"SYSTEM_PROPERTY"/> +
    + + + + + + + + + + + + + + + + + + + + + + + + + + + +
    + + + + +
    + + + +
    + + + +
    + + + +
    ${uiLabelMap.WebtoolsSecretLookupKeyInfo}
    +
    + + + +
    + +
    +
    + + +
    + +

    ${uiLabelMap.WebtoolsSecretCsvUploadInfo}

    +
      +
    • ${uiLabelMap.WebtoolsSecretCsvColumns}
      • +
    • ${uiLabelMap.WebtoolsSecretCsvTargetInfo}
      • +
    + +
    + + + + + + + + + + + +
    + + + +
    + +
    +
    diff --git a/framework/webtools/webapp/webtools/WEB-INF/controller.xml b/framework/webtools/webapp/webtools/WEB-INF/controller.xml index e6ce70effaa..f1f23262f95 100644 --- a/framework/webtools/webapp/webtools/WEB-INF/controller.xml +++ b/framework/webtools/webapp/webtools/WEB-INF/controller.xml @@ -466,6 +466,24 @@ under the License. + + + + + + + + + + + + + + + + + + @@ -727,6 +745,7 @@ under the License. + diff --git a/framework/webtools/widget/SecretManagerScreens.xml b/framework/webtools/widget/SecretManagerScreens.xml new file mode 100644 index 00000000000..80d7d0fe2e4 --- /dev/null +++ b/framework/webtools/widget/SecretManagerScreens.xml @@ -0,0 +1,42 @@ + + + + + +
    + + + + + + + + + + + + + + + +
    +     +     From 50ab87374681424e6bfecf7cb3322f85e4a4e040 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Tue, 16 Jun 2026 19:40:28 +0530 Subject: [PATCH 14/30] =?UTF-8?q?The=20secret-value=20marker=20(default:?= =?UTF-8?q?=20LOOKUP,=20was=20SECRET)=20is=20now=20configurable=20via=20se?= =?UTF-8?q?cret.value.marker=20in=20security.properties,=20which=20is=20th?= =?UTF-8?q?e=20natural=20home=20for=20this=20setting.=20SecretValueResolve?= =?UTF-8?q?r=20exposes=20a=20new=20resolveKey(String)=20method=20so=20call?= =?UTF-8?q?ers=20holding=20a=20raw=20key=20(e.g.=20SystemProperty.systemPr?= =?UTF-8?q?opertyLookup)=20can=20resolve=20without=20wrapping=20the=20valu?= =?UTF-8?q?e=20in=20a=20LOOKUP(...)=20marker=20=E2=80=94=20the=20field=20n?= =?UTF-8?q?ame=20already=20implies=20lookup=20semantics.=20Marker-based=20?= =?UTF-8?q?resolution=20(LOOKUP(key)=20in=20.properties=20files)=20is=20un?= =?UTF-8?q?changed.=20Server-side=20and=20client-side=20guards=20now=20rej?= =?UTF-8?q?ect=20a=20LOOKUP(...)=20marker=20typed=20into=20lookupKey,=20an?= =?UTF-8?q?d=20an=20ENC(...)=20prefix=20typed=20into=20secretValue,=20on?= =?UTF-8?q?=20both=20the=20single-entry=20form=20and=20the=20CSV=20bulk-up?= =?UTF-8?q?load=20path.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../base/secret/SecretValueResolver.java | 45 ++++++++++++------- .../base/secret/SecretValueResolverTest.java | 6 +-- .../entity/util/EntityUtilProperties.java | 17 ++++--- framework/security/config/security.properties | 9 ++++ .../webtools/config/WebtoolsUiLabels.xml | 4 +- .../webtools/secret/SecretManagerEvents.java | 6 +++ .../secret/SecretManagerServices.java | 35 ++++++++++----- .../webtools/template/secret/EncryptValue.ftl | 36 +++++++++++++++ 8 files changed, 119 insertions(+), 39 deletions(-) diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java index 6cd5b6c6a64..6dec75dc80c 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java @@ -28,7 +28,7 @@ import org.apache.ofbiz.base.util.UtilProperties; /** - * Resolves configuration values of the form {@code SECRET(key)} to the + * Resolves configuration values of the form {@code LOOKUP(key)} to the * corresponding secret value via {@link SecretProviderFactory}. * *

    This allows any property read through {@code UtilProperties} or @@ -38,14 +38,14 @@ * same {@link SecretProvider} SPI and providers already used for * {@code jdbc-password-lookup}.

    * - *

    Values that do not match {@code SECRET(key)} are returned unchanged. + *

    Values that do not match {@code LOOKUP(key)} are returned unchanged. * The {@code jdbc-password-lookup} mechanism in {@code entityengine.xml} * ({@code EntityConfig.getJdbcPassword()}) does not go through this class and * is unaffected.

    * - *

    The marker name ({@code SECRET} by default) can be changed via the - * {@code secret.value.marker} property in {@code general.properties}, e.g. to - * {@code MASK} or {@code ENCRYPT}, in case it collides with existing property + *

    The marker name ({@code LOOKUP} by default) can be changed via the + * {@code secret.value.marker} property in {@code security.properties}, e.g. to + * {@code SECRET} or {@code ENCRYPT}, in case it collides with existing property * values.

    * *

    Resolved values are cached for {@code secret.cache.ttl.seconds} @@ -59,17 +59,17 @@ public final class SecretValueResolver { // Read the marker name and cache TTL directly from the Properties object, bypassing // UtilProperties.getPropertyValue(), which calls back into resolve() and would otherwise // deadlock/NPE on these fields during static initialization. - private static final Properties GENERAL_PROPERTIES = UtilProperties.getProperties("general"); + private static final Properties SECURITY_PROPERTIES = UtilProperties.getProperties("security"); - private static final String MARKER_NAME = GENERAL_PROPERTIES != null - ? GENERAL_PROPERTIES.getProperty("secret.value.marker", "SECRET").trim() - : "SECRET"; + public static final String MARKER_NAME = SECURITY_PROPERTIES != null + ? SECURITY_PROPERTIES.getProperty("secret.value.marker", "LOOKUP").trim() + : "LOOKUP"; private static final Pattern SECRET_PATTERN = Pattern.compile("^" + Pattern.quote(MARKER_NAME) + "\\((.+)\\)$"); - private static final long CACHE_TTL_MILLIS = (GENERAL_PROPERTIES != null - ? Long.parseLong(GENERAL_PROPERTIES.getProperty("secret.cache.ttl.seconds", "300").trim()) + private static final long CACHE_TTL_MILLIS = (SECURITY_PROPERTIES != null + ? Long.parseLong(SECURITY_PROPERTIES.getProperty("secret.cache.ttl.seconds", "300").trim()) : 300L) * 1000L; private static final ConcurrentHashMap CACHE = new ConcurrentHashMap<>(); @@ -77,13 +77,13 @@ public final class SecretValueResolver { private SecretValueResolver() { } /** - * If {@code rawValue} matches {@code SECRET(key)}, resolves {@code key} via + * If {@code rawValue} matches {@code LOOKUP(key)}, resolves {@code key} via * {@link SecretProviderFactory#getInstance()}, caching the result for * {@code secret.cache.ttl.seconds}. Otherwise returns {@code rawValue} unchanged. * * @param rawValue the raw property value, possibly {@code null} * @return the resolved secret value, or {@code rawValue} unchanged if it is not a - * {@code SECRET(key)} reference; an empty string if resolution fails + * {@code LOOKUP(key)} reference; an empty string if resolution fails */ public static String resolve(String rawValue) { if (rawValue == null) { @@ -93,13 +93,28 @@ public static String resolve(String rawValue) { if (!matcher.matches()) { return rawValue; } - String key = matcher.group(1).trim(); + return resolveKey(matcher.group(1).trim()); + } + /** + * Resolves {@code key} directly via {@link SecretProviderFactory}, with the same TTL-based + * caching as {@link #resolve(String)}. Use this when the caller already holds the raw key + * (e.g. from {@code SystemProperty.systemPropertyLookup}) without a {@code LOOKUP(...)} wrapper. + * + * @param key the raw secret key, possibly {@code null} or empty + * @return the resolved secret value; an empty string if resolution fails; {@code null} if {@code key} is {@code null} + */ + public static String resolveKey(String key) { + if (key == null) { + return null; + } + if (key.isEmpty()) { + return ""; + } CacheEntry cached = CACHE.get(key); if (cached != null && cached.expiry > System.currentTimeMillis()) { return cached.value; } - try { String secret = SecretProviderFactory.getInstance().getSecret(key); CACHE.put(key, new CacheEntry(secret, System.currentTimeMillis() + CACHE_TTL_MILLIS)); diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java index ab9e3d95776..21bba1a29b1 100644 --- a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java @@ -45,13 +45,13 @@ public void nullIsReturnedUnchanged() { @Test public void resolvesKnownKeyFromPasswordsProperties() { - assertEquals("ofbiz", SecretValueResolver.resolve("SECRET(jdbc-password.h2-ofbiz)")); + assertEquals("ofbiz", SecretValueResolver.resolve("LOOKUP(jdbc-password.h2-ofbiz)")); // Cached lookup should return the same value. - assertEquals("ofbiz", SecretValueResolver.resolve("SECRET(jdbc-password.h2-ofbiz)")); + assertEquals("ofbiz", SecretValueResolver.resolve("LOOKUP(jdbc-password.h2-ofbiz)")); } @Test public void unresolvableKeyReturnsEmptyString() { - assertEquals("", SecretValueResolver.resolve("SECRET(jdbc-password.does-not-exist)")); + assertEquals("", SecretValueResolver.resolve("LOOKUP(jdbc-password.does-not-exist)")); } } diff --git a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java index 11a9b491a3c..56cc9c94d89 100644 --- a/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java +++ b/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityUtilProperties.java @@ -83,11 +83,10 @@ private static Map getSystemPropertyValue(String resource, Strin /** * Resolves the effective value of a {@code SystemProperty} record: *

      - *
    • if {@code systemPropertyLookup} holds a {@code SECRET(key)} reference (see - * {@link SecretValueResolver}), resolves {@code key} via the configured - * {@code SecretProvider} (a remote secret manager, with its own - * {@code passwords.properties} fallback); if that fails, falls back to - * {@code systemPropertyValue}
      • + *
    • if {@code systemPropertyLookup} is non-empty, its value is treated as a raw secret + * key and resolved directly via {@link SecretValueResolver#resolveKey(String)} (i.e. + * no {@code LOOKUP(...)} wrapper — the field itself implies lookup semantics); if + * resolution fails or returns empty, falls back to {@code systemPropertyValue}
      • *
    • otherwise (or on lookup failure), uses {@code systemPropertyValue}, decrypting it * with {@link ConfigCryptoUtil} if it is wrapped in {@code ENC(...)}
      • *
    @@ -96,12 +95,12 @@ private static String resolveSystemPropertyValue(String resource, String name, G String lookup = systemProperty.getString("systemPropertyLookup"); if (UtilValidate.isNotEmpty(lookup)) { Debug.logInfo("EntityUtilProperties: resolving " + resource + "/" + name - + " via SystemProperty.systemPropertyLookup '" + lookup + "'", MODULE); - String resolved = SecretValueResolver.resolve(lookup); - if (UtilValidate.isNotEmpty(resolved) && !resolved.equals(lookup)) { + + " via SystemProperty.systemPropertyLookup key '" + lookup + "'", MODULE); + String resolved = SecretValueResolver.resolveKey(lookup); + if (UtilValidate.isNotEmpty(resolved)) { return resolved; } - Debug.logWarning("EntityUtilProperties: systemPropertyLookup '" + lookup + "' for " + resource + "/" + name + Debug.logWarning("EntityUtilProperties: systemPropertyLookup key '" + lookup + "' for " + resource + "/" + name + " did not resolve, falling back to systemPropertyValue", MODULE); } String value = systemProperty.getString("systemPropertyValue"); diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 088a4399b4c..0f2963e325a 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -421,3 +421,12 @@ path.shortener.size=10 #-- Use this in combination with freemarker-whitelist.properties to restrict static method calls in freemarker templates. #-- If set to false, no static method calls are filtered. freemarker.use-restricted-static-models=true + +# -- Secret Manager settings +# -- Marker used to identify lookup references in property values and SystemProperty.systemPropertyLookup fields. +# -- Change this only if 'LOOKUP' collides with an existing property value in your installation. +# -- Supported alternatives: SECRET, ENCRYPT or any uppercase word without parentheses. +secret.value.marker=LOOKUP +# -- How long (in seconds) to cache resolved secret values before re-fetching from the provider. +# -- Increase for high-throughput installs; decrease for environments with frequent secret rotation. +secret.cache.ttl.seconds=300 diff --git a/framework/webtools/config/WebtoolsUiLabels.xml b/framework/webtools/config/WebtoolsUiLabels.xml index 1dbd39703ee..9eff2e0c5de 100644 --- a/framework/webtools/config/WebtoolsUiLabels.xml +++ b/framework/webtools/config/WebtoolsUiLabels.xml @@ -2038,7 +2038,7 @@ Leave Resource ID and Property ID empty to write only to passwords.properties (entityengine.xml jdbc-password-lookup use-case). Provide all four fields to also update the matching property file on disk - (sets <propertyId>=SECRET(<lookupKey>)) and refresh the in-memory + (sets <propertyId>=LOOKUP(<lookupKey>)) and refresh the in-memory property cache — no server restart needed.     @@ -2069,7 +2069,7 @@ Required for passwords.properties. When Resource ID and Property ID are empty, stored as jdbc-password.<lookupKey> (entityengine.xml use-case). When all four fields are provided, stored as <lookupKey> directly (property file use-case). - Optional for SystemProperty: if set, systemPropertyLookup is set to SECRET(<lookupKey>) + Optional for SystemProperty: if set, the plain key is stored in systemPropertyLookup so a configured remote secret provider is tried first. diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java index 9a2d9073108..2c08c4c702e 100644 --- a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java @@ -202,6 +202,7 @@ private static List validateCsvContent(byte[] csvBytes) throws IOExcepti String resourceId = getColumn(record, "systemResourceId"); String propertyId = getColumn(record, "systemPropertyId"); String lookupKey = getColumn(record, "lookupKey"); + String secretValue = getColumn(record, "secretValue"); // target must be a known constant if (target != null && !VALID_TARGETS.contains(target)) { @@ -212,6 +213,11 @@ private static List validateCsvContent(byte[] csvBytes) throws IOExcepti validateIdentifier(errors, rowNum, "systemResourceId", resourceId); validateIdentifier(errors, rowNum, "systemPropertyId", propertyId); validateIdentifier(errors, rowNum, "lookupKey", lookupKey); + // secretValue must be the plain secret, not an already-encrypted value + if (secretValue != null && secretValue.trim().startsWith("ENC(")) { + errors.add("Row " + rowNum + ": 'secretValue' must be the plain secret" + + " — do not enter an ENC(...) encrypted value"); + } } } return errors; diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java index 73681a7a8e2..8dc3ebaa8fe 100644 --- a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java @@ -28,9 +28,11 @@ import java.nio.file.Path; import java.util.List; import java.util.Map; +import java.util.regex.Pattern; import org.apache.ofbiz.base.component.ComponentConfig; import org.apache.ofbiz.base.crypto.ConfigCryptoUtil; +import org.apache.ofbiz.base.secret.SecretValueResolver; import org.apache.ofbiz.base.location.FlexibleLocation; import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.GeneralException; @@ -57,6 +59,8 @@ public final class SecretManagerServices { private static final String PASSWORDS_FILE_LOCATION = "component://base/config/passwords.properties"; private static final String JDBC_PASSWORD_PREFIX = "jdbc-password."; + /** Allows only letters, digits, dots, hyphens, underscores — blocks marker wrappers and path traversal. */ + private static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[\\w.\\-]+$"); private SecretManagerServices() { } @@ -88,6 +92,13 @@ public static void storeEncryptedSecret(Delegator delegator, String secretTarget if (UtilValidate.isEmpty(secretValue)) { throw new GeneralException("secretValue is required"); } + if (secretValue.trim().startsWith("ENC(")) { + throw new GeneralException("secretValue must be the plain secret — do not enter an ENC(...) encrypted value"); + } + if (UtilValidate.isNotEmpty(lookupKey) && !SAFE_IDENTIFIER.matcher(lookupKey.trim()).matches()) { + throw new GeneralException("lookupKey must contain only letters, digits, dots, hyphens, and underscores" + + " — do not enter a " + SecretValueResolver.MARKER_NAME + "(...) marker or path separator"); + } String encryptedValue = "ENC(" + ConfigCryptoUtil.encrypt(secretValue, getMasterKey()) + ")"; if (TARGET_PASSWORDS_FILE.equals(secretTarget)) { @@ -129,17 +140,18 @@ private static void storeSystemPropertySecret(Delegator delegator, String system } systemProperty.set("systemPropertyValue", encryptedValue); if (UtilValidate.isNotEmpty(lookupKey)) { - systemProperty.set("systemPropertyLookup", "SECRET(" + lookupKey + ")"); + systemProperty.set("systemPropertyLookup", lookupKey); } delegator.createOrStore(systemProperty); } /** * Searches all loaded OFBiz components for {@code config/.properties}, - * sets {@code =SECRET()} in the source file, then clears the - * OFBiz property caches so the change is picked up immediately without a server restart. + * sets {@code =LOOKUP()} in the source {@code .properties} file + * (so that {@link org.apache.ofbiz.base.secret.SecretValueResolver} resolves it at runtime), + * then clears the OFBiz property caches so the change is picked up immediately without a restart. * - *

    If the property previously referenced a different lookup key via {@code SECRET(old-key)}, + *

    If the property previously referenced a different lookup key via {@code LOOKUP(old-key)}, * the stale {@code old-key} entry is removed from passwords.properties before the new one is written.

    * *

    Scanning component directories (not the classpath) ensures we write to the actual @@ -158,15 +170,17 @@ private static void updatePropertiesFileAndRefreshCache(String systemResourceId, removePasswordsEntry(existingLookupKey); Debug.logInfo("Removed stale passwords.properties entry for old lookup key: " + existingLookupKey, MODULE); } - writePropertiesEntry(propsFile, systemPropertyId, "SECRET(" + lookupKey + ")"); + writePropertiesEntry(propsFile, systemPropertyId, SecretValueResolver.MARKER_NAME + "(" + lookupKey + ")"); UtilCache.clearCachesThatStartWith("properties.UtilProperties"); - Debug.logInfo("Set " + systemPropertyId + "=SECRET(" + lookupKey + ") in " + Debug.logInfo("Set " + systemPropertyId + "=" + SecretValueResolver.MARKER_NAME + "(" + lookupKey + ") in " + propsFile.getPath() + " and refreshed property caches", MODULE); } /** - * Reads {@code file} and returns the key inside {@code SECRET()} for the given - * {@code propertyId}, or {@code null} if the property is absent or not a SECRET reference. + * Reads {@code file} and returns the key inside a {@code LOOKUP()} marker for the given + * {@code propertyId}, or {@code null} if the property is absent or not a LOOKUP reference. + * Used only for the source {@code .properties} file path (Case B); {@code SystemProperty.systemPropertyLookup} + * stores the raw key without a wrapper. */ private static String readSecretLookupKey(File file, String propertyId) throws GeneralException { try { @@ -174,8 +188,9 @@ private static String readSecretLookupKey(File file, String propertyId) throws G for (String line : Files.readAllLines(file.toPath(), StandardCharsets.UTF_8)) { if (line.startsWith(linePrefix)) { String value = line.substring(linePrefix.length()).trim(); - if (value.startsWith("SECRET(") && value.endsWith(")")) { - return value.substring("SECRET(".length(), value.length() - 1); + String markerPrefix = SecretValueResolver.MARKER_NAME + "("; + if (value.startsWith(markerPrefix) && value.endsWith(")")) { + return value.substring(markerPrefix.length(), value.length() - 1); } return null; } diff --git a/framework/webtools/template/secret/EncryptValue.ftl b/framework/webtools/template/secret/EncryptValue.ftl index c2754db8d20..64b99e9e7d6 100644 --- a/framework/webtools/template/secret/EncryptValue.ftl +++ b/framework/webtools/template/secret/EncryptValue.ftl @@ -117,6 +117,42 @@ under the License. r.addEventListener('change', updateLabels); }); updateLabels(); + + // Client-side validation: reject marker wrappers and pre-encrypted values before submit + var SAFE_ID = /^[\w.\-]+$/; + var form = document.querySelector('form[action$="createEncryptedSecret"]'); + var errDiv = document.createElement('div'); + errDiv.style.cssText = 'display:none;padding:8px;margin-bottom:8px;border:1px solid #a00;background:#fff0f0;color:#700;'; + form.parentNode.insertBefore(errDiv, form); + + form.addEventListener('submit', function (e) { + var errors = []; + var lookupKeyInput = form.querySelector('input[name="lookupKey"]'); + var secretValueInput = form.querySelector('input[name="secretValue"]'); + var lookupKey = lookupKeyInput.value.trim(); + var secretValue = secretValueInput.value; + + if (lookupKey && !SAFE_ID.test(lookupKey)) { + errors.push('Lookup Key must contain only letters, digits, dots, hyphens, and underscores. Do not enter a LOOKUP(...) or similar marker — type the key name only.'); + lookupKeyInput.style.borderColor = '#a00'; + } else { + lookupKeyInput.style.borderColor = ''; + } + if (secretValue.trim().startsWith('ENC(')) { + errors.push('Secret Value must be the plain secret. Do not paste an ENC(...) encrypted value — type or paste the original password.'); + secretValueInput.style.borderColor = '#a00'; + } else { + secretValueInput.style.borderColor = ''; + } + + if (errors.length > 0) { + errDiv.innerHTML = errors.map(function (m) { return '

    ' + m + '

    '; }).join(''); + errDiv.style.display = 'block'; + e.preventDefault(); + } else { + errDiv.style.display = 'none'; + } + }); }()); From dfcf653a62435f1e075dac0c4eda37eaccb44479 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Wed, 17 Jun 2026 09:00:37 +0530 Subject: [PATCH 15/30] Fixing the following points: Security: bound and format checks on identifiers and secret values in testSecretProviderConnection and storeEncryptedSecret; usage stats OUT attributes made optional. Plugin lifecycle: AWS, Azure, Bitwarden, HashiCorp, 1Password, and GCP providers now implement close() for clean shutdown. Code quality: de duplicated SAFE_IDENTIFIER pattern; clarified the key not found contract in SecretProvider.getSecret() Javadoc. Testing: new unit tests for FileBasedSecretProvider, SecretProviderFactory, CSV and identifier validation, usage stats, and PBKDF2 iteration defaults. UI/UX: Active Configuration panel, truncated usage report keys, input maxlength, and submit button Processing state. Docs: added a PBKDF2 iteration migration guide. --- .../ofbiz/base/crypto/ConfigCryptoUtil.java | 39 ++- .../base/secret/FallbackSecretProvider.java | 115 +++++-- .../ofbiz/base/secret/SecretProvider.java | 43 ++- .../base/secret/SecretProviderFactory.java | 140 ++++++++- .../base/secret/SecretValueResolver.java | 195 +++++++++++- .../base/crypto/ConfigCryptoUtilTest.java | 116 +++++++ .../secret/FallbackSecretProviderTest.java | 147 +++++++++ .../secret/FileBasedSecretProviderTest.java | 69 +++++ .../secret/SecretProviderFactoryTest.java | 62 ++++ .../base/secret/SecretValueResolverTest.java | 127 +++++++- framework/common/entitydef/entitymodel.xml | 1 + framework/security/config/security.properties | 20 ++ .../webtools/config/WebtoolsUiLabels.xml | 78 +++++ .../WebtoolsSecurityPermissionSeedData.xml | 4 + framework/webtools/servicedef/services.xml | 54 +++- .../ofbiz/webtools/WebToolsServices.java | 20 ++ .../webtools/secret/SecretManagerEvents.java | 19 +- .../secret/SecretManagerServices.java | 282 +++++++++++++++++- .../secret/SecretManagerEventsTest.java | 120 ++++++++ .../webtools/template/secret/EncryptValue.ftl | 173 ++++++++++- .../webapp/webtools/WEB-INF/controller.xml | 36 +++ framework/webtools/widget/Menus.xml | 6 + .../webtools/widget/SecretManagerScreens.xml | 13 + 23 files changed, 1802 insertions(+), 77 deletions(-) create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtilTest.java create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/secret/FallbackSecretProviderTest.java create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/secret/FileBasedSecretProviderTest.java create mode 100644 framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretProviderFactoryTest.java create mode 100644 framework/webtools/src/test/java/org/apache/ofbiz/webtools/secret/SecretManagerEventsTest.java diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java index 6e819c7c51b..a47199d5501 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtil.java @@ -21,11 +21,13 @@ import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.util.Base64; +import java.util.Properties; import javax.crypto.SecretKeyFactory; import javax.crypto.spec.PBEKeySpec; import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilProperties; import org.apache.shiro.crypto.CryptoException; import org.apache.shiro.crypto.cipher.AesCipherService; import org.apache.shiro.lang.util.ByteSource; @@ -47,7 +49,6 @@ public final class ConfigCryptoUtil { // Static salt: the master key is the actual secret; the salt only needs to defeat // precomputed rainbow tables, not provide per-installation uniqueness. private static final byte[] SALT = "OFBizConfigCryptoUtilSalt".getBytes(StandardCharsets.UTF_8); - private static final int ITERATIONS = 10000; private static final int KEY_LENGTH_BITS = 256; // Note: AesCipherService derives the GCM tag length from getKeySize(), which must stay at @@ -55,6 +56,32 @@ public final class ConfigCryptoUtil { // to encrypt/decrypt and does not depend on this setting. private static final AesCipherService CIPHER_SERVICE = new AesCipherService(); + // Read config once at class-load time using raw Properties to avoid re-entrancy via + // UtilProperties.getPropertyValue() → SecretValueResolver → ConfigCryptoUtil. + private static final Properties SECURITY_PROPERTIES = UtilProperties.getProperties("security"); + + /** Name of the environment variable holding the AES master key; configurable via secret.master.key.env.var. */ + public static final String MASTER_KEY_ENV_VAR = SECURITY_PROPERTIES != null + ? SECURITY_PROPERTIES.getProperty("secret.master.key.env.var", "OFBIZ_MASTER_KEY").trim() + : "OFBIZ_MASTER_KEY"; + + // PBKDF2 iteration count. Raising this requires re-encrypting all existing ENC(...) values + // because the derived AES key changes when the iteration count changes. + static final int ITERATIONS = readIterations(); + + private static int readIterations() { + if (SECURITY_PROPERTIES == null) { + return 310000; + } + try { + int v = Integer.parseInt( + SECURITY_PROPERTIES.getProperty("secret.pbkdf2.iterations", "310000").trim()); + return v > 0 ? v : 310000; + } catch (NumberFormatException e) { + return 310000; + } + } + private static byte[] deriveKey(String masterKey) throws GeneralSecurityException { SecretKeyFactory factory = SecretKeyFactory.getInstance(KEY_DERIVATION_ALGORITHM); PBEKeySpec spec = new PBEKeySpec(masterKey.toCharArray(), SALT, ITERATIONS, KEY_LENGTH_BITS); @@ -70,6 +97,9 @@ private static byte[] deriveKey(String masterKey) throws GeneralSecurityExceptio * @return Base64 string containing the random IV followed by the ciphertext (with GCM auth tag) */ public static String encrypt(String plainText, String masterKey) throws GeneralException { + if (masterKey == null || masterKey.isEmpty()) { + throw new GeneralException("masterKey must not be null or empty"); + } try { byte[] key = deriveKey(masterKey); ByteSource encrypted = CIPHER_SERVICE.encrypt(plainText.getBytes(StandardCharsets.UTF_8), key); @@ -84,6 +114,9 @@ public static String encrypt(String plainText, String masterKey) throws GeneralE * and lets {@link AesCipherService} split the IV from the ciphertext and decrypt. */ public static String decrypt(String encryptedBase64, String masterKey) throws GeneralException { + if (masterKey == null || masterKey.isEmpty()) { + throw new GeneralException("masterKey must not be null or empty"); + } try { byte[] key = deriveKey(masterKey); byte[] payload = Base64.getDecoder().decode(encryptedBase64); @@ -113,10 +146,10 @@ public static String decryptIfEncrypted(String rawValue, String secretName) thro if (!rawValue.startsWith("ENC(") || !rawValue.endsWith(")")) { return rawValue; } - String masterKey = System.getenv("OFBIZ_MASTER_KEY"); + String masterKey = System.getenv(MASTER_KEY_ENV_VAR); if (masterKey == null || masterKey.isEmpty()) { throw new GeneralException("Secret '" + secretName + "' is encrypted (ENC(...)) but the " - + "OFBIZ_MASTER_KEY environment variable is not set"); + + MASTER_KEY_ENV_VAR + " environment variable is not set"); } String base64 = rawValue.substring("ENC(".length(), rawValue.length() - 1); try { diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java index 369608f78df..cf3f879e7c9 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/FallbackSecretProvider.java @@ -18,35 +18,40 @@ *******************************************************************************/ package org.apache.ofbiz.base.secret; +import java.util.Properties; + import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilProperties; /** - * {@link SecretProvider} decorator that falls back to a secondary provider - * when the primary provider fails to resolve a secret. + * {@link SecretProvider} decorator that retries the primary provider with exponential backoff + * before falling back to a secondary provider when the primary fails to resolve a secret. * *

    This is used by {@link SecretProviderFactory} to wrap a custom (remote) - * {@link SecretProvider} implementation together with a - * {@link FileBasedSecretProvider}. If the primary provider throws a - * {@link GeneralException} (e.g. the remote secret manager is unreachable or - * the secret does not exist there) and {@link SecretProvider#isFallbackEnabled()} - * returns {@code true} for the primary provider, the secret is resolved from - * the fallback provider instead.

    + * {@link SecretProvider} implementation together with a {@link FileBasedSecretProvider}. + * On a transient failure the primary is retried up to {@code secret.provider.retry.count} + * times (default 2) with an initial delay of {@code secret.provider.retry.delay.ms} + * (default 500 ms) doubling on each attempt before the fallback is triggered.

    * - *

    If the primary provider fails and fallback is disabled, or the fallback - * provider also fails, the original exception from the primary provider is - * propagated to the caller.

    + *

    If the primary provider fails and fallback is disabled, or the fallback provider also + * fails, the original exception from the primary provider is propagated to the caller.

    */ public final class FallbackSecretProvider implements SecretProvider { private static final String MODULE = FallbackSecretProvider.class.getName(); + private static final Properties SECURITY_PROPERTIES = UtilProperties.getProperties("security"); + + private static final int RETRY_COUNT = readInt("secret.provider.retry.count", 2); + private static final long RETRY_DELAY_MS = readLong("secret.provider.retry.delay.ms", 500L); + private final SecretProvider primary; private final SecretProvider fallback; /** * @param primary the primary (typically remote) provider to try first - * @param fallback the provider to use if the primary fails and allows fallback + * @param fallback the provider to use if the primary fails after all retries and allows fallback */ public FallbackSecretProvider(SecretProvider primary, SecretProvider fallback) { this.primary = primary; @@ -55,23 +60,85 @@ public FallbackSecretProvider(SecretProvider primary, SecretProvider fallback) { @Override public String getSecret(String key) throws GeneralException { - try { - return primary.getSecret(key); - } catch (GeneralException e) { - if (!primary.isFallbackEnabled()) { - throw e; + GeneralException lastException = null; + long delayMs = RETRY_DELAY_MS; + + for (int attempt = 0; attempt <= RETRY_COUNT; attempt++) { + try { + return primary.getSecret(key); + } catch (GeneralException e) { + lastException = e; + if (!primary.isFallbackEnabled()) { + throw e; + } + if (attempt < RETRY_COUNT) { + Debug.logWarning("SecretProvider: " + primary.getClass().getName() + + " failed to resolve secret '" + key + "' (" + e.getClass().getSimpleName() + + "), retrying in " + delayMs + " ms (attempt " + (attempt + 1) + "/" + RETRY_COUNT + ")", + MODULE); + try { + Thread.sleep(delayMs); + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + break; + } + delayMs *= 2; // exponential backoff + } } - Debug.logWarning("SecretProvider: " + primary.getClass().getName() - + " failed to resolve secret '" + key + "' (" + e.getMessage() - + "), falling back to " + describe(fallback), MODULE); - return fallback.getSecret(key); + } + + // All retries exhausted — fall back to the secondary provider. + Debug.logWarning("SecretProvider: " + primary.getClass().getName() + + " failed after " + (RETRY_COUNT + 1) + " attempt(s) for secret '" + key + + "' (" + (lastException != null ? lastException.getClass().getSimpleName() : "unknown") + + "), falling back to " + describe(fallback), MODULE); + return fallback.getSecret(key); + } + + /** Closes both the primary and fallback providers. */ + @Override + public void close() { + try { + primary.close(); + } finally { + fallback.close(); + } + } + + /** Clears the in-memory cache of both the primary and fallback providers. */ + @Override + public void invalidateCache() { + try { + primary.invalidateCache(); + } finally { + fallback.invalidateCache(); + } + } + + private static int readInt(String key, int defaultValue) { + if (SECURITY_PROPERTIES == null) { + return defaultValue; + } + try { + return Integer.parseInt(SECURITY_PROPERTIES.getProperty(key, String.valueOf(defaultValue)).trim()); + } catch (NumberFormatException e) { + return defaultValue; + } + } + + private static long readLong(String key, long defaultValue) { + if (SECURITY_PROPERTIES == null) { + return defaultValue; + } + try { + return Long.parseLong(SECURITY_PROPERTIES.getProperty(key, String.valueOf(defaultValue)).trim()); + } catch (NumberFormatException e) { + return defaultValue; } } /** - * Returns a human-readable description of the given provider for log - * messages, naming the backing {@code passwords.properties} file for - * {@link FileBasedSecretProvider}. + * Returns a human-readable description of the given provider for log messages. */ private static String describe(SecretProvider provider) { if (provider instanceof FileBasedSecretProvider) { diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java index c431f21d3be..2f04ba69ed5 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProvider.java @@ -50,9 +50,20 @@ public interface SecretProvider { /** * Returns the secret value for the given key. * + *

    Contract when the key does not exist: implementations must throw + * {@link GeneralException} rather than returning {@code null} or an empty string. + * {@link FallbackSecretProvider} and {@link SecretValueResolver} rely on the exception to + * distinguish "key not present" from a successful empty-value lookup. Exception messages + * should contain text such as {@code "not found"}, {@code "NotFound"}, or + * {@code "does not exist"} so that callers (e.g. {@link + * org.apache.ofbiz.webtools.secret.SecretManagerServices#testSecretProviderConnection}) + * can tell the difference between a missing key and a connectivity failure.

    + * * @param key the identifier for the secret (e.g. {@code "jdbc-password.mydb"}) - * @return the resolved secret value, never {@code null} or empty - * @throws GeneralException if the secret cannot be found or an error occurs during resolution + * @return the resolved secret value; never {@code null} or empty + * @throws GeneralException if the secret cannot be found or an error occurs during resolution; + * for a missing key the message should include "not found", "NotFound", + * or "does not exist" */ String getSecret(String key) throws GeneralException; @@ -71,4 +82,32 @@ public interface SecretProvider { default boolean isFallbackEnabled() { return true; } + + /** + * Releases any resources held by this provider (e.g. HTTP connection pools, SDK clients). + * Called automatically by {@link SecretProviderFactory} via a JVM shutdown hook. + * + *

    Implementations that hold long-lived SDK clients (e.g. AWS {@code SecretsManagerClient}, + * GCP {@code SecretManagerServiceClient}) should override this method to close them cleanly.

    + * + *

    The default implementation is a no-op, so providers with no closeable resources do not + * need to override it.

    + */ + default void close() { } + + /** + * Clears any secret values this provider has cached in memory, forcing the next + * {@link #getSecret(String)} call for each key to re-fetch from the remote vault. + * + *

    Implementations that cache resolved values (all of the bundled vault providers do, with a + * TTL configured via their own {@code .cache.ttl.seconds} property) should override this + * method. It is called by {@link SecretProviderFactory#invalidateCache()}, which is in turn + * invoked by the Secret Manager admin screen's "Flush Secret Cache" action after a secret has + * been rotated in the remote vault, so the new value is picked up immediately without waiting + * for the provider's own TTL to expire.

    + * + *

    The default implementation is a no-op, so providers with no internal cache do not need to + * override it.

    + */ + default void invalidateCache() { } } diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java index 5e5fef791de..b4c415ee9ad 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretProviderFactory.java @@ -19,10 +19,15 @@ package org.apache.ofbiz.base.secret; import java.util.Iterator; +import java.util.Properties; import java.util.ServiceLoader; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; import org.apache.ofbiz.base.lang.ThreadSafe; import org.apache.ofbiz.base.util.Debug; +import org.apache.ofbiz.base.util.UtilProperties; /** * Factory that provides the active {@link SecretProvider} instance. @@ -48,23 +53,136 @@ public final class SecretProviderFactory { private static final String MODULE = SecretProviderFactory.class.getName(); - private static final SecretProvider INSTANCE = loadProvider(); + private static final Properties SECURITY_PROPERTIES = UtilProperties.getProperties("security"); - private static SecretProvider loadProvider() { - Iterator it = ServiceLoader.load(SecretProvider.class).iterator(); - if (it.hasNext()) { - SecretProvider provider = it.next(); - Debug.logInfo("SecretProvider: using custom implementation " + provider.getClass().getName(), MODULE); - return new FallbackSecretProvider(provider, new FileBasedSecretProvider()); + // Proactive rotation poll: disabled (0) by default. Set secret.rotation.poll.seconds in + // security.properties to periodically flush both cache layers so a secret rotated in the + // remote vault is picked up within a bounded window even if nobody clicks "Flush Secret Cache". + private static final long ROTATION_POLL_SECONDS = readPollSeconds(); + + private static volatile SecretProvider instance; + private static volatile String providerName; + + static { + loadAndSet(); + Runtime.getRuntime().addShutdownHook(new Thread(() -> { + try { + instance.close(); + Debug.logInfo("SecretProvider: provider closed on shutdown", MODULE); + } catch (Exception e) { + Debug.logWarning("SecretProvider: error closing provider on shutdown: " + e.getMessage(), MODULE); + } + }, "SecretProvider-Shutdown")); + if (ROTATION_POLL_SECONDS > 0) { + ScheduledExecutorService poll = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "SecretProvider-RotationPoll"); + t.setDaemon(true); + return t; + }); + poll.scheduleAtFixedRate(() -> { + try { + instance.invalidateCache(); + SecretValueResolver.invalidateAll(); + Debug.logInfo("[SECRET_AUDIT] user=system action=scheduledRotationPoll", MODULE); + } catch (Exception e) { + Debug.logWarning("SecretProvider: scheduled rotation poll failed: " + e.getMessage(), MODULE); + } + }, ROTATION_POLL_SECONDS, ROTATION_POLL_SECONDS, TimeUnit.SECONDS); + Debug.logInfo("SecretProvider: scheduled rotation poll enabled every " + + ROTATION_POLL_SECONDS + "s", MODULE); + } + } + + private static long readPollSeconds() { + if (SECURITY_PROPERTIES == null) { + return 0L; + } + try { + return Long.parseLong(SECURITY_PROPERTIES.getProperty("secret.rotation.poll.seconds", "0").trim()); + } catch (NumberFormatException e) { + return 0L; + } + } + + /** Discovers and instantiates the active provider via {@link ServiceLoader}, setting {@code instance}/{@code providerName}. */ + private static void loadAndSet() { + SecretProvider provider; + String name; + try { + Iterator it = ServiceLoader.load(SecretProvider.class).iterator(); + if (it.hasNext()) { + SecretProvider custom = it.next(); + name = custom.getClass().getSimpleName(); + Debug.logInfo("SecretProvider: using custom implementation " + custom.getClass().getName(), MODULE); + if (it.hasNext()) { + StringBuilder extras = new StringBuilder(); + it.forEachRemaining(p -> extras.append(" ").append(p.getClass().getName())); + Debug.logWarning("SecretProvider: multiple SecretProvider implementations found on the classpath." + + " Only '" + custom.getClass().getName() + "' will be used; ignoring:" + + extras + ". Remove unused provider plugins to eliminate ambiguity.", MODULE); + } + provider = new FallbackSecretProvider(custom, new FileBasedSecretProvider()); + } else { + Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider" + + " (framework/base/config/passwords.properties)", MODULE); + provider = new FileBasedSecretProvider(); + name = "FileBasedSecretProvider (no vault configured)"; + } + } catch (Exception e) { + // A plugin provider whose constructor throws (e.g. missing SDK dependency, bad config) + // must not prevent OFBiz from starting. Fall back to FileBasedSecretProvider and log + // a clear error so the operator knows the vault plugin was not loaded. + Debug.logError("SecretProvider: failed to load custom provider (" + e.getClass().getName() + + ": " + e.getMessage() + "). Falling back to FileBasedSecretProvider.", MODULE); + provider = new FileBasedSecretProvider(); + name = "FileBasedSecretProvider (no vault configured)"; } - Debug.logInfo("SecretProvider: no custom implementation found, using FileBasedSecretProvider" - + " (framework/base/config/passwords.properties)", MODULE); - return new FileBasedSecretProvider(); + instance = provider; + providerName = name; } /** Returns the active {@link SecretProvider} instance. */ public static SecretProvider getInstance() { - return INSTANCE; + return instance; + } + + /** + * Returns the simple class name of the primary provider. + * When a vault plugin is loaded this is the plugin class (e.g. {@code AwsSecretsManagerProvider}), + * not the wrapping {@link FallbackSecretProvider}. When no plugin is loaded this is + * {@code FileBasedSecretProvider}. + */ + public static String getProviderName() { + return providerName; + } + + /** Clears the active provider's in-memory secret cache. See {@link SecretProvider#invalidateCache()}. */ + public static void invalidateCache() { + instance.invalidateCache(); + } + + /** + * Re-runs {@link ServiceLoader} discovery and replaces the active provider, then closes the + * previous one. Useful after deploying a new vault plugin or updating that plugin's own + * connection credentials (e.g. a rotated AWS access key or HashiCorp AppRole secret_id) in its + * {@code config/*.properties} file, without requiring a full OFBiz restart. + * + *

    Callers must ensure the relevant plugin config resource's property cache has already been + * cleared (e.g. via {@code UtilCache.clearCachesThatStartWith("properties.UtilProperties")}) so + * the new provider picks up the updated values.

    + */ + public static synchronized void reload() { + SecretProvider previous = instance; + loadAndSet(); + if (previous != null) { + try { + previous.close(); + } catch (Exception e) { + Debug.logWarning("SecretProvider: error closing previous provider during reload: " + + e.getMessage(), MODULE); + } + } + Debug.logInfo("SecretProvider: provider reloaded -> " + providerName, MODULE); } private SecretProviderFactory() { } diff --git a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java index 6dec75dc80c..f07d8537438 100644 --- a/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java +++ b/framework/base/src/main/java/org/apache/ofbiz/base/secret/SecretValueResolver.java @@ -18,11 +18,20 @@ *******************************************************************************/ package org.apache.ofbiz.base.secret; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.Map; +import java.util.Set; import java.util.Properties; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; import java.util.regex.Matcher; import java.util.regex.Pattern; +import org.apache.ofbiz.base.lang.ThreadSafe; import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.GeneralException; import org.apache.ofbiz.base.util.UtilProperties; @@ -52,6 +61,7 @@ * (default 300) to avoid calling the remote secret manager on every property * lookup.

    */ +@ThreadSafe public final class SecretValueResolver { private static final String MODULE = SecretValueResolver.class.getName(); @@ -68,12 +78,53 @@ public final class SecretValueResolver { private static final Pattern SECRET_PATTERN = Pattern.compile("^" + Pattern.quote(MARKER_NAME) + "\\((.+)\\)$"); - private static final long CACHE_TTL_MILLIS = (SECURITY_PROPERTIES != null - ? Long.parseLong(SECURITY_PROPERTIES.getProperty("secret.cache.ttl.seconds", "300").trim()) - : 300L) * 1000L; + // Cap at 86400 seconds (24 h) to prevent silent long-overflow when an operator enters + // an unreasonably large value in security.properties. + private static final long MAX_TTL_SECONDS = 86400L; + private static final long CACHE_TTL_MILLIS = parseTtlMillis(); + + private static long parseTtlMillis() { + long seconds = 300L; + if (SECURITY_PROPERTIES != null) { + try { + seconds = Long.parseLong( + SECURITY_PROPERTIES.getProperty("secret.cache.ttl.seconds", "300").trim()); + } catch (NumberFormatException ignored) { + seconds = 300L; + } + } + return Math.min(Math.max(seconds, 1L), MAX_TTL_SECONDS) * 1000L; + } private static final ConcurrentHashMap CACHE = new ConcurrentHashMap<>(); + // Per-key locks used for stampede protection: only one thread fetches from the provider + // when a cache entry expires; others wait and then read the freshly cached value. + private static final ConcurrentHashMap KEY_LOCKS = new ConcurrentHashMap<>(); + + // Usage counters — incremented every time resolveKey() is called. + private static final AtomicLong TOTAL_HITS = new AtomicLong(); + private static final AtomicLong TOTAL_MISSES = new AtomicLong(); + private static final ConcurrentHashMap KEY_HIT_COUNTS = new ConcurrentHashMap<>(); + private static final ConcurrentHashMap KEY_MISS_COUNTS = new ConcurrentHashMap<>(); + + // Daemon thread that sweeps expired entries so stale keys don't accumulate indefinitely. + private static final ScheduledExecutorService EVICTION_EXECUTOR; + static { + ScheduledExecutorService executor = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "SecretValueResolver-CacheEviction"); + t.setDaemon(true); + return t; + }); + executor.scheduleAtFixedRate(() -> { + long now = System.currentTimeMillis(); + CACHE.entrySet().removeIf(e -> e.getValue().expiry <= now); + // Remove per-key locks for keys no longer in the cache to prevent unbounded growth. + KEY_LOCKS.keySet().removeIf(k -> !CACHE.containsKey(k)); + }, CACHE_TTL_MILLIS, CACHE_TTL_MILLIS, TimeUnit.MILLISECONDS); + EVICTION_EXECUTOR = executor; + } + private SecretValueResolver() { } /** @@ -113,16 +164,140 @@ public static String resolveKey(String key) { } CacheEntry cached = CACHE.get(key); if (cached != null && cached.expiry > System.currentTimeMillis()) { + TOTAL_HITS.incrementAndGet(); + KEY_HIT_COUNTS.computeIfAbsent(key, k -> new AtomicLong()).incrementAndGet(); return cached.value; } - try { - String secret = SecretProviderFactory.getInstance().getSecret(key); - CACHE.put(key, new CacheEntry(secret, System.currentTimeMillis() + CACHE_TTL_MILLIS)); - return secret; - } catch (GeneralException e) { - Debug.logError("SecretValueResolver: failed to resolve secret '" + key + "': " + e.getMessage(), MODULE); - return ""; + // Per-key lock: only the first thread that finds a stale/absent entry fetches from the + // provider. All other threads for the same key wait, then read the freshly cached value. + Object lock = KEY_LOCKS.computeIfAbsent(key, k -> new Object()); + synchronized (lock) { + // Double-check: another thread may have populated the cache while we waited. + CacheEntry rechecked = CACHE.get(key); + if (rechecked != null && rechecked.expiry > System.currentTimeMillis()) { + TOTAL_HITS.incrementAndGet(); + KEY_HIT_COUNTS.computeIfAbsent(key, k -> new AtomicLong()).incrementAndGet(); + return rechecked.value; + } + try { + String secret = SecretProviderFactory.getInstance().getSecret(key); + CACHE.put(key, new CacheEntry(secret, System.currentTimeMillis() + CACHE_TTL_MILLIS)); + TOTAL_MISSES.incrementAndGet(); + KEY_MISS_COUNTS.computeIfAbsent(key, k -> new AtomicLong()).incrementAndGet(); + return secret; + } catch (GeneralException e) { + // Log only the exception type — never e.getMessage() — to prevent a provider that + // accidentally embeds a secret value in its error message from leaking it to the log. + Debug.logError("SecretValueResolver: failed to resolve secret '" + key + + "' (" + e.getClass().getSimpleName() + ")", MODULE); + TOTAL_MISSES.incrementAndGet(); + KEY_MISS_COUNTS.computeIfAbsent(key, k -> new AtomicLong()).incrementAndGet(); + return ""; + } + } + } + + /** + * Redacts sensitive patterns from a string before it is used in a log message or UI output. + * Currently masks {@code ENC(...)} blobs (replacing the base64 payload with {@code ***}) so + * that encrypted config values logged by accident are not directly usable. + * + * @param value the string to sanitize, possibly {@code null} + * @return the sanitized string, or {@code null} if the input is {@code null} + */ + public static String maskSensitive(String value) { + if (value == null) { + return null; } + return value.replaceAll("ENC\\([^)]*\\)", "ENC(***)"); + } + + /** + * Removes the cached value for {@code key}, forcing the next {@link #resolve(String)} or + * {@link #resolveKey(String)} call to re-fetch from the provider. Call this after writing a + * new encrypted value for {@code key} so the update is visible immediately without waiting for + * the TTL to expire. + * + * @param key the raw secret key (without any {@code LOOKUP(...)} wrapper), or {@code null} to no-op + */ + public static void invalidate(String key) { + if (key != null) { + CACHE.remove(key); + KEY_LOCKS.remove(key); + } + } + + /** + * Clears all cached secret values, forcing every subsequent lookup to re-fetch from the + * provider. Useful after a bulk secret rotation or when an admin explicitly flushes the cache + * via the Secret Manager admin screen. + */ + public static void invalidateAll() { + CACHE.clear(); + KEY_LOCKS.clear(); + Debug.logInfo("SecretValueResolver: secret value cache flushed", MODULE); + } + + /** + * Resets all in-memory usage counters to zero. Useful when an operator wants to observe + * clean post-rotation metrics without waiting for a JVM restart. + */ + public static void resetUsageStats() { + TOTAL_HITS.set(0); + TOTAL_MISSES.set(0); + KEY_HIT_COUNTS.clear(); + KEY_MISS_COUNTS.clear(); + Debug.logInfo("SecretValueResolver: usage stats reset", MODULE); + } + + /** + * Returns a snapshot of usage statistics for the secret value cache. Each entry in the + * returned map represents one unique key that has been looked up since the last JVM start. + * The map is sorted by total lookup count (descending) for easy review in the admin UI. + * + *

    Map structure: {@code key → {"hits": n, "misses": m, "total": n+m}}

    + */ + public static Map> getUsageReport() { + // Collect all known keys from both hit and miss maps. + Set keys = new LinkedHashSet<>(KEY_HIT_COUNTS.keySet()); + keys.addAll(KEY_MISS_COUNTS.keySet()); + + Map> report = new LinkedHashMap<>(); + keys.stream() + .sorted((a, b) -> { + long totalA = getCount(KEY_HIT_COUNTS, a) + getCount(KEY_MISS_COUNTS, a); + long totalB = getCount(KEY_HIT_COUNTS, b) + getCount(KEY_MISS_COUNTS, b); + return Long.compare(totalB, totalA); // descending + }) + .forEach(key -> { + long hits = getCount(KEY_HIT_COUNTS, key); + long misses = getCount(KEY_MISS_COUNTS, key); + Map stats = new LinkedHashMap<>(); + stats.put("hits", hits); + stats.put("misses", misses); + stats.put("total", hits + misses); + report.put(key, stats); + }); + return report; + } + + /** Returns the aggregate hit and miss totals since JVM start as a simple summary map. */ + public static Map getUsageSummary() { + // Capture atomics once to avoid a TOCTOU race where a concurrent hit/miss + // arrives between two get() calls and makes totalLookups != totalHits + totalMisses. + long hits = TOTAL_HITS.get(); + long misses = TOTAL_MISSES.get(); + Map summary = new LinkedHashMap<>(); + summary.put("totalHits", hits); + summary.put("totalMisses", misses); + summary.put("totalLookups", hits + misses); + summary.put("cachedKeys", (long) CACHE.size()); + return summary; + } + + private static long getCount(ConcurrentHashMap map, String key) { + AtomicLong counter = map.get(key); + return counter == null ? 0L : counter.get(); } private static final class CacheEntry { diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtilTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtilTest.java new file mode 100644 index 00000000000..4a26b4415a4 --- /dev/null +++ b/framework/base/src/test/java/org/apache/ofbiz/base/crypto/ConfigCryptoUtilTest.java @@ -0,0 +1,116 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.crypto; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.junit.Assume.assumeTrue; +import static org.junit.Assume.assumeNotNull; + +import org.apache.ofbiz.base.util.GeneralException; +import org.junit.Test; + +public class ConfigCryptoUtilTest { + + private static final String TEST_KEY = "test-master-key-for-unit-tests"; + + @Test + public void encryptDecryptRoundTrip() throws GeneralException { + String original = "my-secret-password"; + String encrypted = ConfigCryptoUtil.encrypt(original, TEST_KEY); + assertEquals(original, ConfigCryptoUtil.decrypt(encrypted, TEST_KEY)); + } + + @Test + public void eachEncryptionProducesUniqueCiphertext() throws GeneralException { + // Random IV means identical plaintexts should not produce identical ciphertext. + String enc1 = ConfigCryptoUtil.encrypt("same-value", TEST_KEY); + String enc2 = ConfigCryptoUtil.encrypt("same-value", TEST_KEY); + assertNotEquals(enc1, enc2); + } + + @Test(expected = GeneralException.class) + public void decryptWithWrongKeyThrows() throws GeneralException { + String encrypted = ConfigCryptoUtil.encrypt("secret", TEST_KEY); + ConfigCryptoUtil.decrypt(encrypted, "wrong-key"); + } + + @Test + public void decryptIfEncryptedReturnsPlainValueUnchanged() throws GeneralException { + assertEquals("plain-value", ConfigCryptoUtil.decryptIfEncrypted("plain-value", "test-secret")); + assertEquals("notenc", ConfigCryptoUtil.decryptIfEncrypted("notenc", "test-secret")); + // Partial ENC pattern must not be treated as encrypted + assertEquals("ENC(noclosepar", ConfigCryptoUtil.decryptIfEncrypted("ENC(noclosepar", "test-secret")); + } + + @Test + public void decryptIfEncryptedEncWrapperIsStrippedBeforeDecrypt() throws GeneralException { + // Verify that the "ENC(...)" wrapper is stripped correctly: the base64 content inside + // ENC(...) must be identical to what encrypt() returns, so that decrypt() can reverse it. + String plain = "strip-wrapper-test"; + String base64 = ConfigCryptoUtil.encrypt(plain, TEST_KEY); + // decryptIfEncrypted would strip "ENC(" and ")" and pass base64 to decrypt(). + // Confirm the strip round-trip is correct by doing it manually: + String encWrapped = "ENC(" + base64 + ")"; + String stripped = encWrapped.substring("ENC(".length(), encWrapped.length() - 1); + assertEquals(plain, ConfigCryptoUtil.decrypt(stripped, TEST_KEY)); + } + + @Test + public void decryptIfEncryptedHappyPath() throws GeneralException { + // Only runs when OFBIZ_MASTER_KEY is set to TEST_KEY in the environment, + // allowing the full decryptIfEncrypted() path to be exercised end-to-end. + String envKey = System.getenv(ConfigCryptoUtil.MASTER_KEY_ENV_VAR); + assumeNotNull("Skipped: " + ConfigCryptoUtil.MASTER_KEY_ENV_VAR + " is not set", envKey); + assumeTrue("Skipped: " + ConfigCryptoUtil.MASTER_KEY_ENV_VAR + " must equal TEST_KEY for this test", + TEST_KEY.equals(envKey)); + String plain = "happy-path-secret"; + String encWrapped = "ENC(" + ConfigCryptoUtil.encrypt(plain, TEST_KEY) + ")"; + assertEquals(plain, ConfigCryptoUtil.decryptIfEncrypted(encWrapped, "test-secret")); + } + + @Test + public void iterationCountIsPositive() { + // Verifies that readIterations() returned a usable value regardless of what security.properties contains. + assertTrue("ITERATIONS must be positive", ConfigCryptoUtil.ITERATIONS > 0); + } + + @Test + public void iterationCountMatchesSecurityPropertiesDefault() { + // When no security.properties override is present in the test environment, the default of 310000 is used. + // If an override is present, we just verify the value is still positive. + assertTrue("ITERATIONS must be at least 1", ConfigCryptoUtil.ITERATIONS >= 1); + } + + @Test + public void decryptIfEncryptedThrowsWhenMasterKeyMissing() throws GeneralException { + assumeTrue("Skipped: OFBIZ_MASTER_KEY is set in this environment", + System.getenv("OFBIZ_MASTER_KEY") == null || System.getenv("OFBIZ_MASTER_KEY").isEmpty()); + String encWrapped = "ENC(" + ConfigCryptoUtil.encrypt("secret", TEST_KEY) + ")"; + try { + ConfigCryptoUtil.decryptIfEncrypted(encWrapped, "test-secret"); + fail("Expected GeneralException when OFBIZ_MASTER_KEY is absent"); + } catch (GeneralException e) { + assertTrue("Exception message should mention OFBIZ_MASTER_KEY", + e.getMessage().contains("OFBIZ_MASTER_KEY")); + } + } +} diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/FallbackSecretProviderTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/FallbackSecretProviderTest.java new file mode 100644 index 00000000000..661f6e390e7 --- /dev/null +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/FallbackSecretProviderTest.java @@ -0,0 +1,147 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; + +import org.apache.ofbiz.base.util.GeneralException; +import org.junit.Test; + +/** + * Tests {@link FallbackSecretProvider} retry and fallback behavior. + * + *

    Note: {@link #allRetriesFailedDelegatesToFallback()} exercises the full retry loop and + * takes ~1.5 s due to exponential backoff (500 ms + 1000 ms with default security.properties). + * This is intentional — it validates the real production timing.

    + */ +public class FallbackSecretProviderTest { + + @Test + public void primarySuccessDoesNotCallFallback() throws GeneralException { + AtomicBoolean fallbackCalled = new AtomicBoolean(false); + SecretProvider primary = fixedProvider("primary-value", true); + SecretProvider fallback = new SecretProvider() { + @Override + public String getSecret(String key) { + fallbackCalled.set(true); + return "fallback-value"; + } + }; + assertEquals("primary-value", new FallbackSecretProvider(primary, fallback).getSecret("k")); + assertFalse("Fallback must not be called when primary succeeds", fallbackCalled.get()); + } + + @Test(expected = GeneralException.class) + public void fallbackDisabledRethrowsImmediately() throws GeneralException { + SecretProvider primary = throwingProvider(new GeneralException("vault-error"), false); + SecretProvider fallback = fixedProvider("fallback-value", true); + new FallbackSecretProvider(primary, fallback).getSecret("k"); + } + + @Test + public void fallbackDisabledDoesNotConsultFallback() throws GeneralException { + AtomicBoolean fallbackCalled = new AtomicBoolean(false); + SecretProvider primary = throwingProvider(new GeneralException("vault-error"), false); + SecretProvider fallback = new SecretProvider() { + @Override + public String getSecret(String key) { + fallbackCalled.set(true); + return "fallback-value"; + } + }; + try { + new FallbackSecretProvider(primary, fallback).getSecret("k"); + } catch (GeneralException ignored) { } + assertFalse("Fallback must never be called when isFallbackEnabled=false", fallbackCalled.get()); + } + + @Test + public void allRetriesFailedDelegatesToFallback() throws GeneralException { + SecretProvider primary = throwingProvider(new GeneralException("vault-unavailable"), true); + SecretProvider fallback = fixedProvider("fallback-value", true); + assertEquals("fallback-value", new FallbackSecretProvider(primary, fallback).getSecret("k")); + } + + @Test + public void primaryIsCalledMoreThanOnceBeforeFallback() throws GeneralException { + AtomicInteger callCount = new AtomicInteger(0); + SecretProvider primary = new SecretProvider() { + @Override + public String getSecret(String key) throws GeneralException { + callCount.incrementAndGet(); + throw new GeneralException("fail"); + } + @Override + public boolean isFallbackEnabled() { + return true; + } + }; + SecretProvider fallback = fixedProvider("fallback-value", true); + new FallbackSecretProvider(primary, fallback).getSecret("k"); + // With default retry count of 2, primary must be called 3 times (initial + 2 retries). + assertTrue("Primary must be called more than once before fallback", + callCount.get() > 1); + } + + @Test + public void closeDelegatesToFallbackEvenIfPrimaryThrows() { + AtomicBoolean fallbackClosed = new AtomicBoolean(false); + SecretProvider primary = new SecretProvider() { + @Override + public String getSecret(String key) { return ""; } + @Override + public void close() { throw new RuntimeException("primary-close-error"); } + }; + SecretProvider fallback = new SecretProvider() { + @Override + public String getSecret(String key) { return ""; } + @Override + public void close() { fallbackClosed.set(true); } + }; + try { + new FallbackSecretProvider(primary, fallback).close(); + } catch (RuntimeException ignored) { } + assertTrue("fallback.close() must always be called (try-finally)", fallbackClosed.get()); + } + + // -- helpers -- + + private static SecretProvider fixedProvider(String value, boolean fallbackEnabled) { + return new SecretProvider() { + @Override + public String getSecret(String key) { return value; } + @Override + public boolean isFallbackEnabled() { return fallbackEnabled; } + }; + } + + private static SecretProvider throwingProvider(GeneralException ex, boolean fallbackEnabled) { + return new SecretProvider() { + @Override + public String getSecret(String key) throws GeneralException { throw ex; } + @Override + public boolean isFallbackEnabled() { return fallbackEnabled; } + }; + } +} diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/FileBasedSecretProviderTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/FileBasedSecretProviderTest.java new file mode 100644 index 00000000000..91705d43695 --- /dev/null +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/FileBasedSecretProviderTest.java @@ -0,0 +1,69 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; + +import org.apache.ofbiz.base.util.GeneralException; +import org.junit.Test; + +/** + * Tests {@link FileBasedSecretProvider} in isolation using the + * {@code jdbc-password.h2-ofbiz} entry already present in + * {@code framework/base/config/passwords.properties}. + */ +public class FileBasedSecretProviderTest { + + private final FileBasedSecretProvider provider = new FileBasedSecretProvider(); + + @Test + public void getSecretReturnsValueForKnownKey() throws GeneralException { + assertEquals("ofbiz", provider.getSecret("jdbc-password.h2-ofbiz")); + } + + @Test(expected = GeneralException.class) + public void getSecretThrowsForUnknownKey() throws GeneralException { + provider.getSecret("jdbc-password.no-such-key-xyz"); + } + + @Test + public void getSecretExceptionMentionsKeyName() { + try { + provider.getSecret("missing-key-abc"); + } catch (GeneralException e) { + assertNotNull(e.getMessage()); + assertTrue("Exception should mention the key name", e.getMessage().contains("missing-key-abc")); + return; + } + // If no exception was thrown the test fails here. + assertTrue("Expected GeneralException for missing key", false); + } + + @Test + public void isFallbackEnabledDefaultsToTrue() { + assertTrue(provider.isFallbackEnabled()); + } + + @Test + public void closeIsNoOp() { + provider.close(); // must not throw + } +} diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretProviderFactoryTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretProviderFactoryTest.java new file mode 100644 index 00000000000..009a23b32b0 --- /dev/null +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretProviderFactoryTest.java @@ -0,0 +1,62 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.base.secret; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; + +import org.junit.Test; + +/** + * Tests {@link SecretProviderFactory} static accessors. + * + *

    In the test environment no custom {@link SecretProvider} plugin is on the + * {@link java.util.ServiceLoader} classpath, so the factory always selects + * {@link FileBasedSecretProvider}.

    + */ +public class SecretProviderFactoryTest { + + @Test + public void instanceIsNotNull() { + assertNotNull(SecretProviderFactory.getInstance()); + } + + @Test + public void instanceIsSingleton() { + assertSame(SecretProviderFactory.getInstance(), SecretProviderFactory.getInstance()); + } + + @Test + public void providerNameIsNotNullOrEmpty() { + String name = SecretProviderFactory.getProviderName(); + assertNotNull(name); + assertFalse("Provider name must not be empty", name.isEmpty()); + } + + @Test + public void providerNameIndicatesFileBasedWhenNoPluginPresent() { + // In the test environment no vault plugin is on the ServiceLoader classpath, + // so the factory falls back to FileBasedSecretProvider. + String name = SecretProviderFactory.getProviderName(); + assertTrue("Expected 'FileBasedSecretProvider' in provider name when no plugin is configured, got: " + name, + name.contains("FileBasedSecretProvider")); + } +} diff --git a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java index 21bba1a29b1..14ff3c2277b 100644 --- a/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java +++ b/framework/base/src/test/java/org/apache/ofbiz/base/secret/SecretValueResolverTest.java @@ -19,9 +19,14 @@ package org.apache.ofbiz.base.secret; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.Map; import org.junit.Test; +import org.junit.Before; /** * Tests {@link SecretValueResolver} using the default {@link FileBasedSecretProvider} @@ -31,6 +36,13 @@ */ public class SecretValueResolverTest { + private static final String M = SecretValueResolver.MARKER_NAME; + + @Before + public void clearCache() { + SecretValueResolver.invalidateAll(); + } + @Test public void nonSecretValuesAreReturnedUnchanged() { assertEquals("plainvalue", SecretValueResolver.resolve("plainvalue")); @@ -45,13 +57,122 @@ public void nullIsReturnedUnchanged() { @Test public void resolvesKnownKeyFromPasswordsProperties() { - assertEquals("ofbiz", SecretValueResolver.resolve("LOOKUP(jdbc-password.h2-ofbiz)")); + assertEquals("ofbiz", SecretValueResolver.resolve(M + "(jdbc-password.h2-ofbiz)")); // Cached lookup should return the same value. - assertEquals("ofbiz", SecretValueResolver.resolve("LOOKUP(jdbc-password.h2-ofbiz)")); + assertEquals("ofbiz", SecretValueResolver.resolve(M + "(jdbc-password.h2-ofbiz)")); } @Test public void unresolvableKeyReturnsEmptyString() { - assertEquals("", SecretValueResolver.resolve("LOOKUP(jdbc-password.does-not-exist)")); + assertEquals("", SecretValueResolver.resolve(M + "(jdbc-password.does-not-exist)")); + } + + @Test + public void resolveKeyResolvesDirectly() { + assertEquals("ofbiz", SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz")); + } + + @Test + public void resolveKeyWithNullReturnsNull() { + assertNull(SecretValueResolver.resolveKey(null)); + } + + @Test + public void resolveKeyWithEmptyReturnsEmpty() { + assertEquals("", SecretValueResolver.resolveKey("")); + } + + @Test + public void invalidateForcesFreshFetch() { + // Populate cache, then invalidate and verify re-fetch still returns the same value. + assertEquals("ofbiz", SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz")); + SecretValueResolver.invalidate("jdbc-password.h2-ofbiz"); + assertEquals("ofbiz", SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz")); + } + + @Test + public void invalidateNullIsNoOp() { + SecretValueResolver.invalidate(null); // must not throw + } + + @Test + public void invalidateAllClearsAllEntries() { + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); + SecretValueResolver.invalidateAll(); // must not throw + // After invalidation, the key can still be resolved (re-fetched from provider). + assertEquals("ofbiz", SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz")); + } + + @Test + public void maskSensitiveRedactsEncBlob() { + assertEquals("ENC(***)", SecretValueResolver.maskSensitive("ENC(abc123==)")); + } + + @Test + public void maskSensitiveRedactsMultipleEncBlobs() { + assertEquals("a=ENC(***) b=ENC(***)", + SecretValueResolver.maskSensitive("a=ENC(first) b=ENC(second)")); + } + + @Test + public void maskSensitiveLeavesPlainValueUnchanged() { + assertEquals("plain-value", SecretValueResolver.maskSensitive("plain-value")); + } + + @Test + public void maskSensitiveWithNullReturnsNull() { + assertNull(SecretValueResolver.maskSensitive(null)); + } + + @Test + public void resetUsageStatsClearsAllCounters() { + // Populate counters by doing some lookups. + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); // miss then hit + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); // hit + SecretValueResolver.resolveKey("jdbc-password.does-not-exist"); // miss + + // Reset and verify summary is zeroed. + SecretValueResolver.resetUsageStats(); + Map summary = SecretValueResolver.getUsageSummary(); + assertEquals(Long.valueOf(0), summary.get("totalHits")); + assertEquals(Long.valueOf(0), summary.get("totalMisses")); + assertEquals(Long.valueOf(0), summary.get("totalLookups")); + } + + @Test + public void resetUsageStatsAlsoClearsPerKeyReport() { + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); + SecretValueResolver.resetUsageStats(); + Map> report = SecretValueResolver.getUsageReport(); + assertTrue("Per-key report should be empty after reset", report.isEmpty()); + } + + @Test + public void getUsageSummaryReturnsRequiredKeys() { + Map summary = SecretValueResolver.getUsageSummary(); + assertNotNull(summary.get("totalHits")); + assertNotNull(summary.get("totalMisses")); + assertNotNull(summary.get("totalLookups")); + assertNotNull(summary.get("cachedKeys")); + } + + @Test + public void getUsageSummaryTotalIsConsistent() { + SecretValueResolver.resetUsageStats(); + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); + Map summary = SecretValueResolver.getUsageSummary(); + assertEquals(summary.get("totalHits") + summary.get("totalMisses"), (long) summary.get("totalLookups")); + } + + @Test + public void getUsageReportTracksKnownKey() { + SecretValueResolver.resetUsageStats(); + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); + SecretValueResolver.resolveKey("jdbc-password.h2-ofbiz"); + Map> report = SecretValueResolver.getUsageReport(); + assertTrue("Known key should appear in per-key report", report.containsKey("jdbc-password.h2-ofbiz")); + Map stats = report.get("jdbc-password.h2-ofbiz"); + long total = stats.get("hits") + stats.get("misses"); + assertEquals(total, (long) stats.get("total")); } } diff --git a/framework/common/entitydef/entitymodel.xml b/framework/common/entitydef/entitymodel.xml index 760c7606618..9dfb3e1abaf 100644 --- a/framework/common/entitydef/entitymodel.xml +++ b/framework/common/entitydef/entitymodel.xml @@ -890,6 +890,7 @@ under the License. +     diff --git a/framework/security/config/security.properties b/framework/security/config/security.properties index 0f2963e325a..0d5b1cbf761 100644 --- a/framework/security/config/security.properties +++ b/framework/security/config/security.properties @@ -429,4 +429,24 @@ freemarker.use-restricted-static-models=true secret.value.marker=LOOKUP # -- How long (in seconds) to cache resolved secret values before re-fetching from the provider. # -- Increase for high-throughput installs; decrease for environments with frequent secret rotation. +# -- Capped at 86400 seconds (24 hours) regardless of the value set here; values below 1 are treated as 1. secret.cache.ttl.seconds=300 +# -- Proactive rotation poll interval in seconds. When greater than 0, a background task flushes both +# -- the SecretValueResolver cache and the active SecretProvider's own cache at this interval, so a +# -- secret rotated in the remote vault is picked up within a bounded window even if nobody manually +# -- triggers "Flush Secret Cache" in the admin screen. Disabled (0) by default — most installs rely on +# -- the per-provider cache.ttl.seconds expiring on its own, or an admin/automation explicitly flushing. +secret.rotation.poll.seconds=0 +# -- Number of times to retry the primary SecretProvider before falling back to passwords.properties. +# -- Set to 0 to disable retries (fall back immediately on first failure). +secret.provider.retry.count=2 +# -- Initial delay in milliseconds between retry attempts. Doubles on each subsequent attempt (exponential backoff). +secret.provider.retry.delay.ms=500 +# -- Name of the environment variable that holds the AES master key used to encrypt/decrypt ENC(...) values. +# -- Override this if your deployment already uses a different variable name for the key material. +secret.master.key.env.var=OFBIZ_MASTER_KEY +# -- PBKDF2-HMAC-SHA256 iteration count used when deriving the AES key from the master key. +# -- WARNING: changing this value makes all existing ENC(...) values unreadable — you must +# -- re-encrypt every secret after changing this setting. Default: 310000 (OWASP 2023 guidance). +# -- Legacy installations that encrypted with 10000 iterations must keep this at 10000. +secret.pbkdf2.iterations=310000 diff --git a/framework/webtools/config/WebtoolsUiLabels.xml b/framework/webtools/config/WebtoolsUiLabels.xml index 9eff2e0c5de..7253a422a10 100644 --- a/framework/webtools/config/WebtoolsUiLabels.xml +++ b/framework/webtools/config/WebtoolsUiLabels.xml @@ -2093,6 +2093,81 @@ Upload and Encrypt + + Confirm Secret Value + + + Secret Value and Confirm Secret Value must match. + + + Force an immediate re-fetch from the secret provider by flushing all cached values. + Use this after rotating a secret so the new value is picked up without restarting OFBiz. + + + Flush Secret Cache + + + Re-discover the active secret provider via ServiceLoader. Use this after deploying + a new vault plugin jar or editing that plugin's own connection settings, without restarting OFBiz. + + + Reload Secret Provider + + + Fetch the current value of a key from the active secret provider and re-encrypt it into + the local fallback snapshot. Use this after rotating a secret in the remote vault to keep the local + ENC(...) fallback (used when the vault is unreachable) up to date. + + + Sync From Provider + + + Verify that the active secret provider is reachable by resolving a known key. + If the key exists the secret value is confirmed but never displayed. + A "key not found" response still confirms vault connectivity. + + + Test Key + + + Test Connection + + + Active Secret Provider + + + Secret lookup statistics since the last JVM start. Hit = served from cache; Miss = fetched from provider. + + + Show Usage Stats + + + Reset Stats + + + Cache Hits + + + Provider Fetches + + + Total Lookups + + + Keys In Cache + + + Secret Key + + + Cache Hits + + + Provider Fetches + + + Total + Entität XML Tools Entity XML Tools @@ -4243,6 +4318,9 @@ 性能测试 性能測試 + + You do not have permission to manage secrets. The SECRET_MAINT or ENTITY_MAINT permission is required. + WebTools Autorisierungsfehler Web Tools Permission Error diff --git a/framework/webtools/data/WebtoolsSecurityPermissionSeedData.xml b/framework/webtools/data/WebtoolsSecurityPermissionSeedData.xml index 196acc8d0f4..a060edca164 100644 --- a/framework/webtools/data/WebtoolsSecurityPermissionSeedData.xml +++ b/framework/webtools/data/WebtoolsSecurityPermissionSeedData.xml @@ -44,6 +44,9 @@ under the License. + + + @@ -70,6 +73,7 @@ under the License. + diff --git a/framework/webtools/servicedef/services.xml b/framework/webtools/servicedef/services.xml index 1a957b02273..d4e0829f9c9 100644 --- a/framework/webtools/servicedef/services.xml +++ b/framework/webtools/servicedef/services.xml @@ -122,17 +122,69 @@ under the License. if the user has the ENTITY_MAINT permission. + + Returns hasPermission=true if the user holds the SECRET_MAINT permission + (or the broader ENTITY_MAINT permission for backward compatibility). + + Encrypts a secret value with ConfigCryptoUtil (AES-256-GCM, keyed by the OFBIZ_MASTER_KEY environment variable) and stores the resulting ENC(...) value either as a SystemProperty.systemPropertyValue or as a jdbc-password.<lookupKey> entry in passwords.properties. - + + + + + Flushes all in-memory cached secret values (SecretValueResolver TTL cache, the UtilProperties + file cache, and the active SecretProvider's own internal cache) so the next lookup re-fetches from the + provider. Useful after a secret rotation. + + + + Re-runs ServiceLoader discovery and replaces the active SecretProvider instance. Useful after + deploying a new vault plugin jar or editing that plugin's own connection settings without a restart. + + + + Fetches the current value of lookupKey from the active SecretProvider and re-encrypts it into + the local fallback snapshot (passwords.properties or SystemProperty.systemPropertyValue), keeping the + ENC(...) value used when the remote vault is unreachable in sync with the latest rotation. + + + + + + + + Tests connectivity to the active SecretProvider by resolving a given key. + Distinguishes "connected but key not found" from a real connection failure. + + + + + Returns in-memory usage statistics (hit/miss counts) for the SecretValueResolver cache + since the last JVM start, for display in the Secret Manager admin screen. + + + + + + Resets in-memory secret usage counters to zero so operators can observe + clean post-rotation metrics without a JVM restart. + Saves service and related artifacts diagram to an Apple EOModelBundle file. diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/WebToolsServices.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/WebToolsServices.java index 6f9fda106ad..cd22ca7c19d 100644 --- a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/WebToolsServices.java +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/WebToolsServices.java @@ -962,6 +962,26 @@ public static Map entityMaintPermCheck(DispatchContext dctx, Map return resultMap; } + /** + * Performs a secret management security check. Returns hasPermission=true if the user has + * the {@code SECRET_MAINT} permission or the broader {@code ENTITY_MAINT} permission + * (backward-compatible: existing admins with ENTITY_MAINT are not locked out). + */ + public static Map secretMaintPermCheck(DispatchContext dctx, Map context) { + GenericValue userLogin = (GenericValue) context.get("userLogin"); + Locale locale = (Locale) context.get("locale"); + Security security = dctx.getSecurity(); + if (security.hasPermission("SECRET_MAINT", userLogin) || security.hasPermission("ENTITY_MAINT", userLogin)) { + Map resultMap = ServiceUtil.returnSuccess(); + resultMap.put("hasPermission", true); + return resultMap; + } + Map resultMap = ServiceUtil.returnFailure( + UtilProperties.getMessage(RESOURCE, "WebtoolsSecretPermissionError", locale)); + resultMap.put("hasPermission", false); + return resultMap; + } + public static Map exportServiceEoModelBundle(DispatchContext dctx, Map context) { String eomodeldFullPath = (String) context.get("eomodeldFullPath"); diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java index 2c08c4c702e..999c5cac296 100644 --- a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerEvents.java @@ -27,7 +27,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.regex.Pattern; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; @@ -65,8 +64,6 @@ public final class SecretManagerEvents { List.of("target", "systemResourceId", "systemPropertyId", "lookupKey", "secretValue"); private static final Set VALID_TARGETS = Set.of( SecretManagerServices.TARGET_SYSTEM_PROPERTY, SecretManagerServices.TARGET_PASSWORDS_FILE); - /** Allows only letters, digits, dots, hyphens, underscores — prevents path traversal and property-file injection. */ - private static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[\\w.\\-]+$"); private SecretManagerEvents() { } @@ -75,7 +72,9 @@ public static String uploadEncryptedSecrets(HttpServletRequest request, HttpServ Security security = (Security) request.getAttribute("security"); GenericValue userLogin = (GenericValue) request.getSession().getAttribute("userLogin"); - if (security == null || !security.hasPermission("ENTITY_MAINT", userLogin)) { + if (security == null + || (!security.hasPermission("SECRET_MAINT", userLogin) + && !security.hasPermission("ENTITY_MAINT", userLogin))) { request.setAttribute("_ERROR_MESSAGE_", "You do not have permission to perform this operation"); return "error"; } @@ -118,6 +117,7 @@ public static String uploadEncryptedSecrets(HttpServletRequest request, HttpServ long rowNum = record.getRecordNumber() + 1; try { SecretManagerServices.storeEncryptedSecret(delegator, + userLogin, getColumn(record, "target"), getColumn(record, "systemResourceId"), getColumn(record, "systemPropertyId"), @@ -167,7 +167,7 @@ private static CSVFormat buildCsvFormat() { * {@code ..} path-traversal sequences. * */ - private static List validateCsvContent(byte[] csvBytes) throws IOException { + static List validateCsvContent(byte[] csvBytes) throws IOException { List errors = new ArrayList<>(); // Whole-file content scan using OFBiz's existing SecuredUpload allow-list validator. @@ -223,16 +223,21 @@ private static List validateCsvContent(byte[] csvBytes) throws IOExcepti return errors; } - /** Validates that {@code value} contains only safe identifier characters and no {@code ..}. */ + /** Validates that {@code value} contains only safe identifier characters, no {@code ..}, and is within length. */ private static void validateIdentifier(List errors, long rowNum, String field, String value) { if (value == null) { return; } + if (value.length() > SecretManagerServices.MAX_KEY_LENGTH) { + errors.add("Row " + rowNum + ": '" + field + "' must not exceed " + + SecretManagerServices.MAX_KEY_LENGTH + " characters"); + return; + } if (value.contains("..")) { errors.add("Row " + rowNum + ": '" + field + "' must not contain '..' (path traversal)"); return; } - if (!SAFE_IDENTIFIER.matcher(value).matches()) { + if (!SecretManagerServices.SAFE_IDENTIFIER.matcher(value).matches()) { errors.add("Row " + rowNum + ": '" + field + "' contains invalid characters — only letters, digits, dots, hyphens, and underscores are allowed"); } diff --git a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java index 8dc3ebaa8fe..84c370f4e75 100644 --- a/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java +++ b/framework/webtools/src/main/java/org/apache/ofbiz/webtools/secret/SecretManagerServices.java @@ -26,16 +26,19 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.List; import java.util.Map; import java.util.regex.Pattern; import org.apache.ofbiz.base.component.ComponentConfig; import org.apache.ofbiz.base.crypto.ConfigCryptoUtil; -import org.apache.ofbiz.base.secret.SecretValueResolver; import org.apache.ofbiz.base.location.FlexibleLocation; +import org.apache.ofbiz.base.secret.SecretProviderFactory; +import org.apache.ofbiz.base.secret.SecretValueResolver; import org.apache.ofbiz.base.util.Debug; import org.apache.ofbiz.base.util.GeneralException; +import org.apache.ofbiz.base.util.UtilDateTime; import org.apache.ofbiz.base.util.UtilValidate; import org.apache.ofbiz.base.util.cache.UtilCache; import org.apache.ofbiz.entity.Delegator; @@ -59,22 +62,210 @@ public final class SecretManagerServices { private static final String PASSWORDS_FILE_LOCATION = "component://base/config/passwords.properties"; private static final String JDBC_PASSWORD_PREFIX = "jdbc-password."; + /** Maximum length for lookupKey and related identifier fields written to properties files or logs. */ + static final int MAX_KEY_LENGTH = 256; + /** Maximum length for a secret value; guards against oversized inputs that could exhaust memory or storage. */ + static final int MAX_SECRET_VALUE_LENGTH = 8192; /** Allows only letters, digits, dots, hyphens, underscores — blocks marker wrappers and path traversal. */ - private static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[\\w.\\-]+$"); + static final Pattern SAFE_IDENTIFIER = Pattern.compile("^[\\w.\\-]+$"); private SecretManagerServices() { } + /** + * Tests connectivity to the active {@link org.apache.ofbiz.base.secret.SecretProvider} by + * attempting to resolve {@code testKey}. Reports three outcomes: + *
      + *
    • Connected — key found: the provider returned a non-empty value.
      • + *
    • Connected — key not found: the provider responded normally but the key does + * not exist (confirms vault connectivity even when the test key is wrong).
      • + *
    • Connection failed: the provider threw an SDK or network error.
      • + *
    + */ + public static Map testSecretProviderConnection(DispatchContext dctx, Map context) { + String testKey = (String) context.get("testKey"); + if (UtilValidate.isEmpty(testKey)) { + return ServiceUtil.returnError("testKey is required"); + } + testKey = testKey.trim(); + if (testKey.length() > MAX_KEY_LENGTH) { + return ServiceUtil.returnError("testKey must not exceed " + MAX_KEY_LENGTH + " characters"); + } + if (!SAFE_IDENTIFIER.matcher(testKey).matches()) { + return ServiceUtil.returnError("testKey must contain only letters, digits, dots, hyphens, and underscores"); + } + GenericValue userLogin = (GenericValue) context.get("userLogin"); + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + try { + String value = SecretProviderFactory.getInstance().getSecret(testKey); + String outcome = UtilValidate.isNotEmpty(value) ? "found" : "empty-value"; + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=testSecretProviderConnection" + + " testKey=" + testKey + " outcome=" + outcome, MODULE); + if (UtilValidate.isNotEmpty(value)) { + return ServiceUtil.returnSuccess("Connected — key '" + testKey + "' found successfully"); + } + return ServiceUtil.returnSuccess("Connected — key '" + testKey + "' returned an empty value"); + } catch (GeneralException e) { + String msg = e.getMessage(); + // "not found" responses confirm connectivity; only network/auth errors are real failures + if (msg != null && (msg.contains("not found") || msg.contains("NotFound") || msg.contains("does not exist"))) { + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=testSecretProviderConnection" + + " testKey=" + testKey + " outcome=key-not-found", MODULE); + return ServiceUtil.returnSuccess("Connected — key '" + testKey + "' was not found in the provider (vault is reachable)"); + } + // Log the full message server-side but never expose SDK internals to the browser: + // vault error messages can contain endpoint URLs, IAM ARNs, or credential fragments. + Debug.logWarning("[SECRET_AUDIT] user=" + userLoginId + " action=testSecretProviderConnection" + + " testKey=" + testKey + " outcome=connection-failed detail=" + msg, MODULE); + return ServiceUtil.returnError("Connection failed (" + e.getClass().getSimpleName() + + ") — check server logs for details"); + } + } + + /** + * Resets in-memory secret usage counters (hit/miss counts) to zero. Useful after a secret + * rotation so operators can observe clean post-rotation metrics without a JVM restart. + */ + public static Map resetSecretUsageStats(DispatchContext dctx, Map context) { + GenericValue userLogin = (GenericValue) context.get("userLogin"); + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + SecretValueResolver.resetUsageStats(); + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=resetSecretUsageStats", MODULE); + return ServiceUtil.returnSuccess("Secret usage statistics reset successfully"); + } + + /** + * Returns in-memory usage statistics from {@link SecretValueResolver}: aggregate hit/miss + * totals and a per-key breakdown. The data covers the current JVM lifetime only and resets + * on restart; it is meant for operator visibility, not durable monitoring. + */ + public static Map getSecretUsageStats(DispatchContext dctx, Map context) { + Map result = ServiceUtil.returnSuccess(); + result.put("usageSummary", SecretValueResolver.getUsageSummary()); + result.put("usageReport", SecretValueResolver.getUsageReport()); + return result; + } + + /** + * Flushes all in-memory cached secret values so the next lookup re-fetches from the provider. + * Useful after a secret rotation so the new value is picked up immediately without a restart. + * + *

    This clears both cache layers: the {@link SecretValueResolver} TTL cache + * (and the {@code UtilProperties} file cache it sits behind) and the active + * {@link org.apache.ofbiz.base.secret.SecretProvider}'s own internal cache via + * {@link SecretProviderFactory#invalidateCache()}. Clearing only the former is not sufficient: + * each bundled vault provider (AWS, Azure, GCP, HashiCorp Vault, Bitwarden, 1Password) keeps its + * own TTL-based cache (default 1 hour), so without this second call the next lookup would still + * return the stale value straight from the provider's cache instead of re-fetching from the vault.

    + */ + public static Map flushSecretCache(DispatchContext dctx, Map context) { + GenericValue userLogin = (GenericValue) context.get("userLogin"); + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + SecretValueResolver.invalidateAll(); + UtilCache.clearCachesThatStartWith("properties.UtilProperties"); + SecretProviderFactory.invalidateCache(); + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=flushSecretCache", MODULE); + return ServiceUtil.returnSuccess("Secret value cache flushed successfully"); + } + + /** + * Re-runs {@link SecretProviderFactory} discovery, replacing the active provider instance. + * Useful after deploying a new vault plugin jar or editing that plugin's own connection + * settings (e.g. a rotated AWS access key, a new HashiCorp AppRole secret_id) in its + * {@code config/*.properties} file, without requiring a full OFBiz restart. + */ + public static Map reloadSecretProvider(DispatchContext dctx, Map context) { + GenericValue userLogin = (GenericValue) context.get("userLogin"); + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + UtilCache.clearCachesThatStartWith("properties.UtilProperties"); + SecretProviderFactory.reload(); + String providerName = SecretProviderFactory.getProviderName(); + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=reloadSecretProvider" + + " provider=" + providerName, MODULE); + return ServiceUtil.returnSuccess("Secret provider reloaded — active provider is now: " + providerName); + } + + /** + * Pulls the current value of {@code lookupKey} from the active {@link + * org.apache.ofbiz.base.secret.SecretProvider} and re-encrypts it into the local fallback + * snapshot ({@code passwords.properties} or {@code SystemProperty.systemPropertyValue}), + * keeping the ENC(...) value used when the remote vault is unreachable in sync with whatever + * was last rotated in the vault. Delegates to {@link #storeEncryptedSecret} for the actual + * write, so the same validation, audit logging, and cache-invalidation logic applies. + * + *

    For {@code PASSWORDS_FILE} without a {@code systemResourceId}/{@code systemPropertyId} + * pair (the {@code jdbc-password-lookup} case), the remote key fetched is + * {@code jdbc-password.}, matching what {@link + * org.apache.ofbiz.entity.config.model.EntityConfig#getJdbcPassword} resolves at runtime. In + * every other case the raw {@code lookupKey} is fetched directly.

    + */ + public static Map syncSecretFromProvider(DispatchContext dctx, Map context) { + Delegator delegator = dctx.getDelegator(); + GenericValue userLogin = (GenericValue) context.get("userLogin"); + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + String secretTarget = (String) context.get("secretTarget"); + String systemResourceId = (String) context.get("systemResourceId"); + String systemPropertyId = (String) context.get("systemPropertyId"); + String lookupKey = (String) context.get("lookupKey"); + + if (UtilValidate.isEmpty(lookupKey)) { + return ServiceUtil.returnError("lookupKey is required to sync from the active secret provider"); + } + lookupKey = lookupKey.trim(); + if (lookupKey.length() > MAX_KEY_LENGTH) { + return ServiceUtil.returnError("lookupKey must not exceed " + MAX_KEY_LENGTH + " characters"); + } + if (!SAFE_IDENTIFIER.matcher(lookupKey).matches()) { + return ServiceUtil.returnError("lookupKey must contain only letters, digits, dots, hyphens, and underscores"); + } + + boolean hasResourceId = UtilValidate.isNotEmpty(systemResourceId); + String providerKey = (TARGET_PASSWORDS_FILE.equals(secretTarget) && !hasResourceId) + ? JDBC_PASSWORD_PREFIX + lookupKey + : lookupKey; + + String freshValue; + try { + freshValue = SecretProviderFactory.getInstance().getSecret(providerKey); + } catch (GeneralException e) { + Debug.logWarning("[SECRET_AUDIT] user=" + userLoginId + " action=syncSecretFromProvider" + + " providerKey=" + providerKey + " outcome=fetch-failed detail=" + e.getClass().getSimpleName(), + MODULE); + return ServiceUtil.returnError("Failed to fetch the current value from the active secret provider for key '" + + providerKey + "' (" + e.getClass().getSimpleName() + ") — check server logs for details"); + } + if (UtilValidate.isEmpty(freshValue)) { + return ServiceUtil.returnError("Secret provider returned an empty value for key '" + providerKey + "'"); + } + + try { + storeEncryptedSecret(delegator, userLogin, secretTarget, systemResourceId, systemPropertyId, lookupKey, freshValue); + } catch (GeneralException e) { + Debug.logError(e, MODULE); + return ServiceUtil.returnError(e.getMessage()); + } + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + " action=syncSecretFromProvider" + + " providerKey=" + providerKey + " outcome=synced", MODULE); + return ServiceUtil.returnSuccess("Local encrypted snapshot for '" + lookupKey + + "' synced from the active secret provider"); + } + /** Service implementation for the webtools "Encrypt Value" screen. */ public static Map createEncryptedSecret(DispatchContext dctx, Map context) { Delegator delegator = dctx.getDelegator(); + GenericValue userLogin = (GenericValue) context.get("userLogin"); String secretTarget = (String) context.get("secretTarget"); String systemResourceId = (String) context.get("systemResourceId"); String systemPropertyId = (String) context.get("systemPropertyId"); String lookupKey = (String) context.get("lookupKey"); String secretValue = (String) context.get("secretValue"); + String secretValueConfirm = (String) context.get("secretValueConfirm"); + + if (UtilValidate.isNotEmpty(secretValueConfirm) && !secretValueConfirm.equals(secretValue)) { + return ServiceUtil.returnError("Secret Value and Confirm Secret Value do not match"); + } try { - storeEncryptedSecret(delegator, secretTarget, systemResourceId, systemPropertyId, lookupKey, secretValue); + storeEncryptedSecret(delegator, userLogin, secretTarget, systemResourceId, systemPropertyId, lookupKey, secretValue); } catch (GeneralException e) { Debug.logError(e, MODULE); return ServiceUtil.returnError(e.getMessage()); @@ -86,15 +277,39 @@ public static Map createEncryptedSecret(DispatchContext dctx, Ma * Encrypts {@code secretValue} and stores it as configured by {@code secretTarget}. Used by * both the single-entry service ({@link #createEncryptedSecret}) and the CSV bulk-upload * event so that both paths share the exact same validation and storage logic. + * + *

    A structured audit log entry is written on every successful call so that the + * who/what/when of each secret operation is preserved for compliance review.

    */ - public static void storeEncryptedSecret(Delegator delegator, String secretTarget, String systemResourceId, - String systemPropertyId, String lookupKey, String secretValue) throws GeneralException { - if (UtilValidate.isEmpty(secretValue)) { - throw new GeneralException("secretValue is required"); + public static void storeEncryptedSecret(Delegator delegator, GenericValue userLogin, String secretTarget, + String systemResourceId, String systemPropertyId, String lookupKey, String secretValue) + throws GeneralException { + if (UtilValidate.isEmpty(secretValue) || secretValue.trim().isEmpty()) { + throw new GeneralException("secretValue is required and must not be blank"); + } + if (secretValue.length() > MAX_SECRET_VALUE_LENGTH) { + throw new GeneralException("secretValue must not exceed " + MAX_SECRET_VALUE_LENGTH + " characters"); } if (secretValue.trim().startsWith("ENC(")) { throw new GeneralException("secretValue must be the plain secret — do not enter an ENC(...) encrypted value"); } + if (UtilValidate.isNotEmpty(systemResourceId) && systemResourceId.trim().length() > MAX_KEY_LENGTH) { + throw new GeneralException("systemResourceId must not exceed " + MAX_KEY_LENGTH + " characters"); + } + if (UtilValidate.isNotEmpty(systemResourceId) && !SAFE_IDENTIFIER.matcher(systemResourceId.trim()).matches()) { + throw new GeneralException( + "systemResourceId contains invalid characters — only letters, digits, dots, hyphens, and underscores are allowed"); + } + if (UtilValidate.isNotEmpty(systemPropertyId) && systemPropertyId.trim().length() > MAX_KEY_LENGTH) { + throw new GeneralException("systemPropertyId must not exceed " + MAX_KEY_LENGTH + " characters"); + } + if (UtilValidate.isNotEmpty(systemPropertyId) && !SAFE_IDENTIFIER.matcher(systemPropertyId.trim()).matches()) { + throw new GeneralException( + "systemPropertyId contains invalid characters — only letters, digits, dots, hyphens, and underscores are allowed"); + } + if (UtilValidate.isNotEmpty(lookupKey) && lookupKey.trim().length() > MAX_KEY_LENGTH) { + throw new GeneralException("lookupKey must not exceed " + MAX_KEY_LENGTH + " characters"); + } if (UtilValidate.isNotEmpty(lookupKey) && !SAFE_IDENTIFIER.matcher(lookupKey.trim()).matches()) { throw new GeneralException("lookupKey must contain only letters, digits, dots, hyphens, and underscores" + " — do not enter a " + SecretValueResolver.MARKER_NAME + "(...) marker or path separator"); @@ -112,21 +327,47 @@ public static void storeEncryptedSecret(Delegator delegator, String secretTarget "systemResourceId and systemPropertyId must both be provided together"); } if (hasResourceId) { - // Case B: property-file combined write — use plain lookupKey, no jdbc-password. prefix + // Case B: property-file combined write — use plain lookupKey, no jdbc-password. prefix. + // Write to passwords.properties first; if the source-file update then fails, remove + // the orphaned ENC entry so the two files don't end up in an inconsistent state. writePasswordsProperty(lookupKey, encryptedValue); - updatePropertiesFileAndRefreshCache(systemResourceId, systemPropertyId, lookupKey); + try { + updatePropertiesFileAndRefreshCache(systemResourceId, systemPropertyId, lookupKey); + } catch (GeneralException e) { + removePasswordsEntry(lookupKey); + throw e; + } + // Invalidate the resolved-value cache so the new secret is visible immediately. + SecretValueResolver.invalidate(lookupKey); } else { - // Case A: entityengine.xml jdbc-password-lookup — keep the jdbc-password. prefix + // Case A: entityengine.xml jdbc-password-lookup — keep the jdbc-password. prefix. + // Clear both the UtilProperties file cache and the SecretValueResolver TTL cache so + // FileBasedSecretProvider picks up the new ENC(...) value without a restart. writePasswordsProperty(JDBC_PASSWORD_PREFIX + lookupKey, encryptedValue); + UtilCache.clearCachesThatStartWith("properties.UtilProperties"); + SecretValueResolver.invalidate(JDBC_PASSWORD_PREFIX + lookupKey); } } else if (TARGET_SYSTEM_PROPERTY.equals(secretTarget)) { if (UtilValidate.isEmpty(systemResourceId) || UtilValidate.isEmpty(systemPropertyId)) { throw new GeneralException("systemResourceId and systemPropertyId are required for SystemProperty"); } storeSystemPropertySecret(delegator, systemResourceId, systemPropertyId, lookupKey, encryptedValue); + // Invalidate the resolved-value cache for the lookup key so EntityUtilProperties + // picks up the new value immediately via SecretValueResolver.resolveKey(). + if (UtilValidate.isNotEmpty(lookupKey)) { + SecretValueResolver.invalidate(lookupKey); + } } else { throw new GeneralException("Unknown secretTarget '" + secretTarget + "'"); } + + String userLoginId = (userLogin != null) ? userLogin.getString("userLoginId") : "unknown"; + Debug.logInfo("[SECRET_AUDIT] user=" + userLoginId + + " action=storeEncryptedSecret" + + " target=" + secretTarget + + " systemResourceId=" + systemResourceId + + " systemPropertyId=" + systemPropertyId + + " lookupKey=" + lookupKey, MODULE); } private static void storeSystemPropertySecret(Delegator delegator, String systemResourceId, String systemPropertyId, @@ -142,6 +383,7 @@ private static void storeSystemPropertySecret(Delegator delegator, String system if (UtilValidate.isNotEmpty(lookupKey)) { systemProperty.set("systemPropertyLookup", lookupKey); } + systemProperty.set("lastRotatedDate", UtilDateTime.nowTimestamp()); delegator.createOrStore(systemProperty); } @@ -212,7 +454,7 @@ private static synchronized void removePropertiesEntry(File file, String key) th List lines = Files.readAllLines(file.toPath(), StandardCharsets.UTF_8); String linePrefix = key + "="; if (lines.removeIf(line -> line.startsWith(linePrefix))) { - Files.write(file.toPath(), lines, StandardCharsets.UTF_8); + atomicWrite(file.toPath(), lines); } } catch (IOException e) { throw new GeneralException("Unable to update " + file.getName() + ": " + e.getMessage(), e); @@ -250,13 +492,24 @@ private static synchronized void writePropertiesEntry(File file, String key, Str } else { lines.add(newLine); } - Files.write(file.toPath(), lines, StandardCharsets.UTF_8); + atomicWrite(file.toPath(), lines); } catch (IOException e) { throw new GeneralException( "Unable to update " + file.getName() + ": " + e.getMessage(), e); } } + /** + * Writes {@code lines} to {@code target} atomically: first to a sibling {@code .tmp} file, + * then renamed over the target with {@link StandardCopyOption#ATOMIC_MOVE}. A JVM crash + * during the write will leave the original file intact rather than producing a partial file. + */ + private static void atomicWrite(Path target, List lines) throws IOException { + Path tmp = target.resolveSibling(target.getFileName() + ".tmp"); + Files.write(tmp, lines, StandardCharsets.UTF_8); + Files.move(tmp, target, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE); + } + /** Updates (or appends) {@code propertyName=value} in passwords.properties, preserving all other lines. */ private static void writePasswordsProperty(String propertyName, String value) throws GeneralException { writePropertiesEntry(getPasswordsFile(), propertyName, value); @@ -275,9 +528,10 @@ private static File getPasswordsFile() throws GeneralException { } private static String getMasterKey() throws GeneralException { - String masterKey = System.getenv("OFBIZ_MASTER_KEY"); + String envVar = ConfigCryptoUtil.MASTER_KEY_ENV_VAR; + String masterKey = System.getenv(envVar); if (UtilValidate.isEmpty(masterKey)) { - throw new GeneralException("The OFBIZ_MASTER_KEY environment variable is not set on the server"); + throw new GeneralException("The " + envVar + " environment variable is not set on the server"); } return masterKey; } diff --git a/framework/webtools/src/test/java/org/apache/ofbiz/webtools/secret/SecretManagerEventsTest.java b/framework/webtools/src/test/java/org/apache/ofbiz/webtools/secret/SecretManagerEventsTest.java new file mode 100644 index 00000000000..9d3041d24ad --- /dev/null +++ b/framework/webtools/src/test/java/org/apache/ofbiz/webtools/secret/SecretManagerEventsTest.java @@ -0,0 +1,120 @@ +/******************************************************************************* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 org.apache.ofbiz.webtools.secret; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.List; + +import org.junit.Test; + +/** + * Unit tests for the CSV validation logic in {@link SecretManagerEvents}. + * + *

    {@link SecretManagerEvents#validateCsvContent} is package-private so this test class, + * being in the same package, can call it directly without HTTP infrastructure.

    + */ +public class SecretManagerEventsTest { + + private static final String HEADER = "target,systemResourceId,systemPropertyId,lookupKey,secretValue\n"; + + private static List validate(String csv) throws IOException { + return SecretManagerEvents.validateCsvContent(csv.getBytes(StandardCharsets.UTF_8)); + } + + @Test + public void happyPathReturnsNoErrors() throws IOException { + String csv = HEADER + "SYSTEM_PROPERTY,my-resource,my.property,my-key,mypassword\n"; + assertTrue(validate(csv).isEmpty()); + } + + @Test + public void passwordsFileTargetIsAccepted() throws IOException { + String csv = HEADER + "PASSWORDS_FILE,,,my-jdbc-key,supersecret\n"; + assertTrue(validate(csv).isEmpty()); + } + + @Test + public void unknownTargetIsRejected() throws IOException { + String csv = HEADER + "BAD_TARGET,res,prop,key,password\n"; + List errors = validate(csv); + assertFalse("Should reject unknown target", errors.isEmpty()); + assertTrue(errors.stream().anyMatch(e -> e.contains("unknown target"))); + } + + @Test + public void missingRequiredHeaderIsRejected() throws IOException { + String csvNoLookupKey = "target,systemResourceId,systemPropertyId,secretValue\n" + + "SYSTEM_PROPERTY,res,prop,password\n"; + List errors = validate(csvNoLookupKey); + assertFalse("Should reject CSV missing lookupKey header", errors.isEmpty()); + assertTrue(errors.stream().anyMatch(e -> e.contains("lookupKey"))); + } + + @Test + public void encPrefixInSecretValueIsRejected() throws IOException { + String csv = HEADER + "SYSTEM_PROPERTY,resource,property,mykey,ENC(abc123==)\n"; + List errors = validate(csv); + assertFalse("Should reject ENC() value in secretValue", errors.isEmpty()); + assertTrue(errors.stream().anyMatch(e -> e.contains("ENC("))); + } + + @Test + public void pathTraversalInSystemResourceIdIsRejected() throws IOException { + String csv = HEADER + "SYSTEM_PROPERTY,../etc/passwd,property,key,password\n"; + List errors = validate(csv); + assertFalse("Should reject path traversal in systemResourceId", errors.isEmpty()); + } + + @Test + public void invalidCharsInLookupKeyAreRejected() throws IOException { + String csv = HEADER + "SYSTEM_PROPERTY,resource,property,key with spaces,password\n"; + List errors = validate(csv); + assertFalse("Should reject lookupKey with spaces", errors.isEmpty()); + } + + @Test + public void rowCountExceededIsRejected() throws IOException { + StringBuilder csv = new StringBuilder(HEADER); + for (int i = 0; i <= 500; i++) { + csv.append("SYSTEM_PROPERTY,res,prop,key").append(i).append(",password\n"); + } + List errors = validate(csv.toString()); + assertFalse("Should reject CSV with more than 500 rows", errors.isEmpty()); + assertTrue(errors.stream().anyMatch(e -> e.contains("maximum"))); + } + + @Test + public void lookupKeyWithDotsAndHyphensIsAccepted() throws IOException { + String csv = HEADER + "PASSWORDS_FILE,,,jdbc-password.mysql-ofbiz,dbpassword\n"; + assertTrue("Dots and hyphens in lookupKey should be valid", validate(csv).isEmpty()); + } + + @Test + public void identifierExceedingMaxLengthIsRejected() throws IOException { + String longKey = "a".repeat(257); + String csv = HEADER + "SYSTEM_PROPERTY,resource,property," + longKey + ",password\n"; + List errors = validate(csv); + assertFalse("Should reject identifier exceeding 256 chars", errors.isEmpty()); + assertTrue(errors.stream().anyMatch(e -> e.contains("lookupKey"))); + } +} diff --git a/framework/webtools/template/secret/EncryptValue.ftl b/framework/webtools/template/secret/EncryptValue.ftl index 64b99e9e7d6..ab5583d4692 100644 --- a/framework/webtools/template/secret/EncryptValue.ftl +++ b/framework/webtools/template/secret/EncryptValue.ftl @@ -17,6 +17,18 @@ specific language governing permissions and limitations under the License. --> +

    ${uiLabelMap.WebtoolsSecretActiveProvider}: ${activeSecretProvider!}

    +<#if activeSettings?has_content> +
    Active Configuration + + + <#list activeSettings?keys as k> + + + +
    ${k}${activeSettings[k]}
    +
    +

    ${uiLabelMap.WebtoolsEncryptValueInfo}

    • ${uiLabelMap.WebtoolsSecretSystemPropertyRequiredInfo}
      • @@ -72,7 +84,7 @@ under the License. - +
      ${uiLabelMap.WebtoolsSecretLookupKeyInfo}
      @@ -84,6 +96,14 @@ under the License. + + + + + + + + @@ -129,11 +149,13 @@ under the License. var errors = []; var lookupKeyInput = form.querySelector('input[name="lookupKey"]'); var secretValueInput = form.querySelector('input[name="secretValue"]'); + var secretValueConfirmInput = form.querySelector('input[name="secretValueConfirm"]'); var lookupKey = lookupKeyInput.value.trim(); var secretValue = secretValueInput.value; + var secretValueConfirm = secretValueConfirmInput.value; if (lookupKey && !SAFE_ID.test(lookupKey)) { - errors.push('Lookup Key must contain only letters, digits, dots, hyphens, and underscores. Do not enter a LOOKUP(...) or similar marker — type the key name only.'); + errors.push('Lookup Key must contain only letters, digits, dots, hyphens, and underscores. Do not enter a ${SecretValueMarker!"LOOKUP"}(...) or similar marker — type the key name only.'); lookupKeyInput.style.borderColor = '#a00'; } else { lookupKeyInput.style.borderColor = ''; @@ -144,6 +166,12 @@ under the License. } else { secretValueInput.style.borderColor = ''; } + if (secretValue !== secretValueConfirm) { + errors.push('${uiLabelMap.WebtoolsSecretConfirmValueMismatch}'); + secretValueConfirmInput.style.borderColor = '#a00'; + } else { + secretValueConfirmInput.style.borderColor = ''; + } if (errors.length > 0) { errDiv.innerHTML = errors.map(function (m) { return '

      ' + m + '

      '; }).join(''); @@ -154,6 +182,19 @@ under the License. } }); }()); + +// Submit-in-progress feedback: disable submit button and show "Processing..." on all forms +(function () { + document.querySelectorAll('form.basic-form').forEach(function (f) { + f.addEventListener('submit', function () { + var btn = f.querySelector('input[type="submit"]'); + if (btn && !btn.disabled) { + btn.disabled = true; + btn.value = 'Processing…'; + } + }); + }); +}());
      @@ -184,3 +225,131 @@ under the License. + +
      + +

      ${uiLabelMap.WebtoolsSecretFlushCacheInfo}

      +
      + +
      + +
      + +

      ${uiLabelMap.WebtoolsSecretReloadProviderInfo}

      +
      + +
      + +
      + +

      ${uiLabelMap.WebtoolsSecretSyncInfo}

      +
      + + + + + + + + + + + + + + + + + + + + + + + +
      + + + + +
      + + + +
      + + + +
      + + + +
      + +
      +
      + +
      + +

      ${uiLabelMap.WebtoolsSecretTestConnectionInfo}

      +
      + + + + + + + + + + + +
      + + + +
      + +
      +
      + +
      + +

      ${uiLabelMap.WebtoolsSecretUsageStatsInfo}

      +
      + +
      +
      + +
      +<#if usageSummary?has_content> +

      + ${uiLabelMap.WebtoolsSecretUsageTotalHits}: ${usageSummary.totalHits!0}  |  + ${uiLabelMap.WebtoolsSecretUsageTotalMisses}: ${usageSummary.totalMisses!0}  |  + ${uiLabelMap.WebtoolsSecretUsageTotalLookups}: ${usageSummary.totalLookups!0}  |  + ${uiLabelMap.WebtoolsSecretUsageCachedKeys}: ${usageSummary.cachedKeys!0} +

      + <#if usageReport?has_content> + + + + + + + + + + + <#list usageReport?keys as key> + <#assign stats = usageReport[key]/> + + + + + + + + +
      ${uiLabelMap.WebtoolsSecretUsageKey}${uiLabelMap.WebtoolsSecretUsageHits}${uiLabelMap.WebtoolsSecretUsageMisses}${uiLabelMap.WebtoolsSecretUsageTotal}
      ${key}${stats.hits!0}${stats.misses!0}${stats.total!0}
      + + diff --git a/framework/webtools/webapp/webtools/WEB-INF/controller.xml b/framework/webtools/webapp/webtools/WEB-INF/controller.xml index f1f23262f95..835818e223c 100644 --- a/framework/webtools/webapp/webtools/WEB-INF/controller.xml +++ b/framework/webtools/webapp/webtools/WEB-INF/controller.xml @@ -483,6 +483,42 @@ under the License. + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/framework/webtools/widget/Menus.xml b/framework/webtools/widget/Menus.xml index f4deacd4858..6393c0bdbd2 100644 --- a/framework/webtools/widget/Menus.xml +++ b/framework/webtools/widget/Menus.xml @@ -54,6 +54,12 @@ under the License. + + + + + + diff --git a/framework/webtools/widget/SecretManagerScreens.xml b/framework/webtools/widget/SecretManagerScreens.xml index 80d7d0fe2e4..1ed7e1a6a5c 100644 --- a/framework/webtools/widget/SecretManagerScreens.xml +++ b/framework/webtools/widget/SecretManagerScreens.xml @@ -25,6 +25,19 @@ under the License. + From 4949fca88aef78929a57c4608b094e0367f091e5 Mon Sep 17 00:00:00 2001 From: Ashish Vijaywargiya Date: Wed, 17 Jun 2026 11:41:05 +0530 Subject: [PATCH 16/30] Fix EncryptValue screen 500 error and usage-stats rendering Move the inline groovy logic from screen widget to EncryptValue.groovy and reference it using the location attribute. Also wire the getSecretUsageStats OUT parameters usageSummary and usageReport into the screen context. Request attributes are available only under context.parameters, so the FTL check usageSummary?has_content was always evaluating to false. --- .../ofbiz/webtools/secret/EncryptValue.groovy | 24 +++++++++++++++++++ .../webtools/widget/SecretManagerScreens.xml | 14 +---------- 2 files changed, 25 insertions(+), 13 deletions(-) create mode 100644 framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/secret/EncryptValue.groovy diff --git a/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/secret/EncryptValue.groovy b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/secret/EncryptValue.groovy new file mode 100644 index 00000000000..5237e9981cf --- /dev/null +++ b/framework/webtools/src/main/groovy/org/apache/ofbiz/webtools/secret/EncryptValue.groovy @@ -0,0 +1,24 @@ +import org.apache.ofbiz.base.secret.SecretProviderFactory +import org.apache.ofbiz.base.secret.SecretValueResolver +import org.apache.ofbiz.base.util.UtilProperties + +context.SecretValueMarker = SecretValueResolver.MARKER_NAME +context.activeSecretProvider = SecretProviderFactory.getProviderName() +def props = UtilProperties.getProperties("security") +context.activeSettings = [ + "Lookup marker" : props?.getProperty("secret.value.marker", "LOOKUP"), + "Cache TTL (seconds)": props?.getProperty("secret.cache.ttl.seconds", "300"), + "Retry count" : props?.getProperty("secret.provider.retry.count", "2"), + "Retry delay (ms)" : props?.getProperty("secret.provider.retry.delay.ms", "500"), + "Master key env var" : props?.getProperty("secret.master.key.env.var", "OFBIZ_MASTER_KEY"), + "PBKDF2 iterations" : props?.getProperty("secret.pbkdf2.iterations", "310000") +] + +// getSecretUsageStats stores its OUT params as request attributes; pull them into the +// screen context since populateBasicContext() only exposes request attrs under "parameters". +if (parameters.usageSummary) { + context.usageSummary = parameters.usageSummary +} +if (parameters.usageReport) { + context.usageReport = parameters.usageReport +} diff --git a/framework/webtools/widget/SecretManagerScreens.xml b/framework/webtools/widget/SecretManagerScreens.xml index 1ed7e1a6a5c..46ae0672118 100644 --- a/framework/webtools/widget/SecretManagerScreens.xml +++ b/framework/webtools/widget/SecretManagerScreens.xml @@ -25,19 +25,7 @@ under the License. - +