From c59da11c25e7b5de43eef613344e16aa6a780dea Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Thu, 14 May 2026 18:51:42 -0700 Subject: [PATCH 1/4] checkpoint initial implementation of service interface SecretService --- .../org/labkey/api/module/ModuleLoader.java | 84 ++++---- .../api/reports/ExternalScriptEngine.java | 5 +- .../api/secrets/ExternalSecretProvider.java | 20 ++ .../labkey/api/secrets/SecretProperty.java | 40 ++++ .../org/labkey/api/secrets/SecretService.java | 58 ++++++ api/src/org/labkey/api/util/GUID.java | 4 +- .../labkey/api/util/LabKeyProcessBuilder.java | 96 ++++++++++ core/src/org/labkey/core/CoreModule.java | 8 + .../labkey/core/admin/AdminController.java | 9 +- .../core/secrets/SecretServiceImpl.java | 180 ++++++++++++++++++ .../org/labkey/core/vcs/VcsServiceImpl.java | 3 +- .../pipeline/analysis/CommandTaskImpl.java | 7 +- .../pipeline/cluster/ClusterStartup.java | 3 +- 13 files changed, 464 insertions(+), 53 deletions(-) create mode 100644 api/src/org/labkey/api/secrets/ExternalSecretProvider.java create mode 100644 api/src/org/labkey/api/secrets/SecretProperty.java create mode 100644 api/src/org/labkey/api/secrets/SecretService.java create mode 100644 api/src/org/labkey/api/util/LabKeyProcessBuilder.java create mode 100644 core/src/org/labkey/core/secrets/SecretServiceImpl.java diff --git a/api/src/org/labkey/api/module/ModuleLoader.java b/api/src/org/labkey/api/module/ModuleLoader.java index 69dadae1fdd..2e11c36b188 100644 --- a/api/src/org/labkey/api/module/ModuleLoader.java +++ b/api/src/org/labkey/api/module/ModuleLoader.java @@ -2561,67 +2561,65 @@ public FileLike getStartupPropDirectory() private void loadStartupProps() { FileLike propsDir = getStartupPropDirectory(); - if (null == propsDir) - return; - - if (!propsDir.isDirectory()) - return; - FileLike newinstall = propsDir.resolveChild("newinstall"); - if (newinstall.isFile()) + if (null != propsDir && propsDir.isDirectory()) { - _log.debug("'newinstall' file detected: {}", newinstall.toNioPathForRead()); + FileLike newinstall = propsDir.resolveChild("newinstall"); + if (newinstall.isFile()) + { + _log.debug("'newinstall' file detected: {}", newinstall.toNioPathForRead()); - _newInstall = true; + _newInstall = true; - // propsDir is readonly, so we need to cheat to get a File - var newInstallFile = newinstall.toNioPathForRead().toFile(); - if (newInstallFile.canWrite()) - newInstallFile.delete(); + // propsDir is readonly, so we need to cheat to get a File + var newInstallFile = newinstall.toNioPathForRead().toFile(); + if (newInstallFile.canWrite()) + newInstallFile.delete(); + else + throw new ConfigurationException("file 'newinstall' exists, but is not writeable: " + newinstall.toNioPathForRead()); + } else - throw new ConfigurationException("file 'newinstall' exists, but is not writeable: " + newinstall.toNioPathForRead()); - } - else - { - _log.debug("no 'newinstall' file detected"); - } - - List propFiles = propsDir.getChildren().stream().filter(f -> f.getName().endsWith(".properties")).toList(); + { + _log.debug("no 'newinstall' file detected"); + } - if (!propFiles.isEmpty()) - { - List sortedPropFiles = propFiles.stream() - .sorted(Comparator.comparing(FileLike::getName).reversed()) - .toList(); + List propFiles = propsDir.getChildren().stream().filter(f -> f.getName().endsWith(".properties")).toList(); - for (FileLike propFile : sortedPropFiles) + if (!propFiles.isEmpty()) { - _log.debug("loading propsFile: {}", propFile.toNioPathForRead()); + List sortedPropFiles = propFiles.stream() + .sorted(Comparator.comparing(FileLike::getName).reversed()) + .toList(); - try (InputStream in = propFile.openInputStream()) + for (FileLike propFile : sortedPropFiles) { - Properties props = new Properties(); - props.load(in); + _log.debug("loading propsFile: {}", propFile.toNioPathForRead()); - for (Map.Entry entry : props.entrySet()) + try (InputStream in = propFile.openInputStream()) { - if (entry.getKey() instanceof String && entry.getValue() instanceof String) + Properties props = new Properties(); + props.load(in); + + for (Map.Entry entry : props.entrySet()) { - _log.trace("property '{}' resolved to value: '{}'", entry.getKey(), entry.getValue()); + if (entry.getKey() instanceof String && entry.getValue() instanceof String) + { + _log.trace("property '{}' resolved to value: '{}'", entry.getKey(), entry.getValue()); - addStartupPropertyEntry(entry.getKey().toString(), entry.getValue().toString()); + addStartupPropertyEntry(entry.getKey().toString(), entry.getValue().toString()); + } } } - } - catch (Exception e) - { - _log.error("Error parsing startup config properties file '{}'", propFile.toNioPathForRead(), e); + catch (Exception e) + { + _log.error("Error parsing startup config properties file '{}'", propFile.toNioPathForRead(), e); + } } } - } - else - { - _log.debug("no propFiles to load"); + else + { + _log.debug("no propFiles to load"); + } } // load any system properties with the labkey prop prefix diff --git a/api/src/org/labkey/api/reports/ExternalScriptEngine.java b/api/src/org/labkey/api/reports/ExternalScriptEngine.java index f31b0ff68d3..ed46777b4f5 100644 --- a/api/src/org/labkey/api/reports/ExternalScriptEngine.java +++ b/api/src/org/labkey/api/reports/ExternalScriptEngine.java @@ -25,6 +25,7 @@ import org.labkey.api.reader.Readers; import org.labkey.api.reports.report.r.ParamReplacementSvc; import org.labkey.api.util.ExceptionUtil; +import org.labkey.api.util.LabKeyProcessBuilder; import org.labkey.api.util.FileUtil; import org.labkey.api.util.QuietCloser; import org.labkey.api.util.URIUtil; @@ -126,7 +127,7 @@ public Object eval(String script, ScriptContext context) throws ScriptException protected Object eval(FileLike scriptFile, ScriptContext context) throws ScriptException { String[] params = formatCommand(scriptFile, context); - ProcessBuilder pb = new ProcessBuilder(params); + LabKeyProcessBuilder pb = new LabKeyProcessBuilder(params); pb = pb.directory(getWorkingDir(context).toNioPathForRead().toFile()); final long timeout = getTimeout(context); @@ -318,7 +319,7 @@ else if (value.startsWith("\"")) * Execute the external script engine in separate process * @return the exit code for the invocation - 0 if the process completed successfully. */ - protected int runProcess(ScriptContext context, ProcessBuilder pb, StringBuffer output, long timeout, TimeUnit timeoutUnit) + protected int runProcess(ScriptContext context, LabKeyProcessBuilder pb, StringBuffer output, long timeout, TimeUnit timeoutUnit) { Process proc; try diff --git a/api/src/org/labkey/api/secrets/ExternalSecretProvider.java b/api/src/org/labkey/api/secrets/ExternalSecretProvider.java new file mode 100644 index 00000000000..e03e1c630e3 --- /dev/null +++ b/api/src/org/labkey/api/secrets/ExternalSecretProvider.java @@ -0,0 +1,20 @@ +package org.labkey.api.secrets; + +import org.jetbrains.annotations.Nullable; + +import java.util.Collection; + +/** + * SPI for external secret stores (e.g., AWS SSM Parameter Store). + * Implementations are registered with {@link SecretService} and consulted at higher + * priority than startup-property secrets. The provider is responsible for caching; + * {@link #refreshAll} is called periodically by a timer managed by {@link SecretService}. + */ +public interface ExternalSecretProvider +{ + /** Returns the secret for the given property name, or null if not available in this provider. */ + @Nullable String getSecret(String propertyName); + + /** Refresh cached values for all registered property names. Called on a timer. */ + void refreshAll(Collection propertyNames); +} diff --git a/api/src/org/labkey/api/secrets/SecretProperty.java b/api/src/org/labkey/api/secrets/SecretProperty.java new file mode 100644 index 00000000000..a6294ea0e5c --- /dev/null +++ b/api/src/org/labkey/api/secrets/SecretProperty.java @@ -0,0 +1,40 @@ +package org.labkey.api.secrets; + +import org.labkey.api.settings.StartupProperty; + +/** + * Describes a named secret that a module needs to access. Register instances with + * {@link SecretService#register} during module startup; retrieve values via + * {@link SecretService#getSecret}. + * + * Startup property file convention: {@code secret.=} + * Environment variable convention: {@code labkey.prop.secret.=} + */ +public class SecretProperty implements StartupProperty +{ + private final String _name; + private final String _description; + + public SecretProperty(String name) + { + this(name, "Secret: " + name); + } + + public SecretProperty(String name, String description) + { + _name = name; + _description = description; + } + + @Override + public String getPropertyName() + { + return _name; + } + + @Override + public String getDescription() + { + return _description; + } +} diff --git a/api/src/org/labkey/api/secrets/SecretService.java b/api/src/org/labkey/api/secrets/SecretService.java new file mode 100644 index 00000000000..652577febea --- /dev/null +++ b/api/src/org/labkey/api/secrets/SecretService.java @@ -0,0 +1,58 @@ +package org.labkey.api.secrets; + +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.labkey.api.services.ServiceRegistry; + +/** + * Internal service that provides access to secrets (API keys, passwords, etc.) without + * requiring callers to know where the secret is stored. Secrets may come from startup + * property files, process environment variables, or an external store such as AWS SSM. + * + *

Modules should: + *

    + *
  1. Declare each secret as a {@code static final SecretProperty} constant.
    2. + *
  2. Call {@link #register} in their {@code doStartup()} method.
    3. + *
  3. Call {@link #getSecret} wherever the value is needed.
    4. + *
+ * + *

Startup property file convention: {@code secret.=} + */ +public interface SecretService +{ + static @NotNull SecretService get() + { + SecretService svc = ServiceRegistry.get().getService(SecretService.class); + if (svc == null) + throw new IllegalStateException("SecretService has not been initialized"); + return svc; + } + + static void setInstance(SecretService service) + { + ServiceRegistry.get().registerService(SecretService.class, service); + } + + /** + * Declare that the calling module may request the named secret. Should be called + * from {@code Module.doStartup()}. Registration is for documentation and filtering + * (e.g., admin env-var page redaction); it does not affect whether a value is returned. + */ + void register(@NotNull SecretProperty property); + + /** + * Retrieve the value of a secret. Returns {@code null} if the secret has not been + * configured in any source. Never logs or caches the returned value. + */ + @Nullable String getSecret(@NotNull SecretProperty property); + + /** + * Register an {@link ExternalSecretProvider} (e.g., AWS SSM). The provider is + * consulted at higher priority than startup-property secrets. Should be called after + * server startup is complete so that startup properties are loaded first. + * + * TODO This is a total place holder. Secrets may need to be available very eary, so we + * TODO need to consider a typical registerProvider() interface will work well. + */ + void setExternalProvider(@NotNull ExternalSecretProvider provider); +} diff --git a/api/src/org/labkey/api/util/GUID.java b/api/src/org/labkey/api/util/GUID.java index c245e355944..914a391b31d 100644 --- a/api/src/org/labkey/api/util/GUID.java +++ b/api/src/org/labkey/api/util/GUID.java @@ -126,7 +126,7 @@ private static String networkIdentifier() try { - ProcessBuilder cmd = new ProcessBuilder("ipconfig.exe", "/all"); + LabKeyProcessBuilder cmd = new LabKeyProcessBuilder("ipconfig.exe", "/all"); cmd.redirectErrorStream(true); p = cmd.start(); } @@ -135,7 +135,7 @@ private static String networkIdentifier() { try { - ProcessBuilder cmd = new ProcessBuilder("ifconfig", "-a"); + LabKeyProcessBuilder cmd = new LabKeyProcessBuilder("ifconfig", "-a"); cmd.redirectErrorStream(true); p = cmd.start(); } diff --git a/api/src/org/labkey/api/util/LabKeyProcessBuilder.java b/api/src/org/labkey/api/util/LabKeyProcessBuilder.java new file mode 100644 index 00000000000..cbba925b032 --- /dev/null +++ b/api/src/org/labkey/api/util/LabKeyProcessBuilder.java @@ -0,0 +1,96 @@ +package org.labkey.api.util; + +import org.labkey.api.secrets.SecretService; +import org.labkey.api.services.ServiceRegistry; + +import java.io.File; +import java.io.IOException; +import java.util.List; +import java.util.Map; + +/** + * Wrapper for {@link ProcessBuilder} that removes secret-named environment variables + * from the subprocess environment before the process starts. This prevents secrets from leaking + * to untrusted child processes. + * + *

Variables are removed (silently) if their name: + *

    + *
  • matches any property name registered with {@link SecretService}, or
    • + *
  • contains "secret" (case-insensitive), or
    • + *
  • contains "password" (case-insensitive).
    • + *
+ * + *

Use this class wherever {@link ProcessBuilder} would otherwise be used. A Checkstyle rule + * flags direct instantiation of {@code java.lang.ProcessBuilder} as a reminder. + */ +public class LabKeyProcessBuilder +{ + private final ProcessBuilder _pb; + + public LabKeyProcessBuilder(List command) + { + _pb = new ProcessBuilder(command); + sanitizeEnvironment(); + } + + public LabKeyProcessBuilder(String... command) + { + _pb = new ProcessBuilder(command); + sanitizeEnvironment(); + } + + public LabKeyProcessBuilder(File directory, List command) + { + _pb = new ProcessBuilder(command); + _pb.directory(directory); + sanitizeEnvironment(); + } + + public List command() + { + return _pb.command(); + } + + public Map environment() + { + return _pb.environment(); + } + + public File directory() + { + return _pb.directory(); + } + + public LabKeyProcessBuilder directory(File directory) + { + _pb.directory(directory); + return this; + } + + public LabKeyProcessBuilder redirectErrorStream(boolean redirectErrorStream) + { + _pb.redirectErrorStream(redirectErrorStream); + return this; + } + + public Process start() throws IOException + { + return _pb.start(); + } + + /** Returns the underlying {@link ProcessBuilder} for APIs that require it directly. */ + public ProcessBuilder processBuilder() + { + return _pb; + } + + private void sanitizeEnvironment() + { + SecretService secrets = ServiceRegistry.get().getService(SecretService.class); + + _pb.environment().keySet().removeIf(name -> + name.toLowerCase().contains("secret") || + name.toLowerCase().contains("password") + ); + } +} diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index a10324eec79..c7572cbe0e1 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -158,6 +158,7 @@ import org.labkey.api.settings.OptionalFeatureFlag; import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.settings.OptionalFeatureService.FeatureType; +import org.labkey.api.secrets.SecretService; import org.labkey.api.settings.ProductConfiguration; import org.labkey.api.settings.StandardStartupPropertyHandler; import org.labkey.api.settings.StartupPropertyEntry; @@ -295,6 +296,7 @@ import org.labkey.core.thumbnail.ThumbnailServiceImpl; import org.labkey.core.user.LimitActiveUsersSettings; import org.labkey.core.user.UserController; +import org.labkey.core.secrets.SecretServiceImpl; import org.labkey.core.vcs.VcsServiceImpl; import org.labkey.core.view.ShortURLServiceImpl; import org.labkey.core.view.TableViewFormTestCase; @@ -418,6 +420,11 @@ public boolean hasScripts() @Override protected void init() { + SecretServiceImpl secretService = new SecretServiceImpl(); + secretService.handleStartupProperties(); + SecretService.setInstance(secretService); + ContextListener.addShutdownListener(ShutdownListener.of("SecretService", null, secretService::shutdown)); + ContainerService.setInstance(new ContainerServiceImpl()); FolderSerializationRegistry.setInstance(new FolderSerializationRegistryImpl()); ExternalToolsViewService.setInstance(new ExternalToolsViewServiceImpl()); @@ -1495,6 +1502,7 @@ public TabDisplayMode getTabDisplayMode() OutOfRangeDisplayColumn.TestCase.class, PostgreSqlVersion.TestCase.class, ScriptEngineManagerImpl.TestCase.class, + SecretServiceImpl.TestCase.class, StatsServiceImpl.TestCase.class, diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index d0907560eed..bffe51b1e1a 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -3396,7 +3396,14 @@ public class EnvironmentVariablesAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) { - return new JspView<>("/org/labkey/core/admin/properties.jsp", System.getenv()); + Map env = new LinkedHashMap<>(System.getenv()); + env.replaceAll((name, value) -> { + String lc = name.toLowerCase(); + if (lc.contains("secret") || lc.contains("password")) + return "[REDACTED]"; + return value; + }); + return new JspView<>("/org/labkey/core/admin/properties.jsp", env); } @Override diff --git a/core/src/org/labkey/core/secrets/SecretServiceImpl.java b/core/src/org/labkey/core/secrets/SecretServiceImpl.java new file mode 100644 index 00000000000..874efd3d658 --- /dev/null +++ b/core/src/org/labkey/core/secrets/SecretServiceImpl.java @@ -0,0 +1,180 @@ +package org.labkey.core.secrets; + +import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; +import org.labkey.api.module.ModuleLoader; +import org.labkey.api.secrets.ExternalSecretProvider; +import org.labkey.api.secrets.SecretProperty; +import org.labkey.api.secrets.SecretService; +import org.labkey.api.settings.LenientStartupPropertyHandler; +import org.labkey.api.settings.StartupPropertyEntry; +import org.labkey.api.util.logging.LogHelper; + +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; + +public class SecretServiceImpl implements SecretService +{ + private static final Logger LOG = LogHelper.getLogger(SecretServiceImpl.class, "Secret service"); + static final String STARTUP_PROPERTY_SCOPE = "secret"; + + // Secrets loaded from startup properties / environment variables + private final Map _startupSecrets = new ConcurrentHashMap<>(); + // Names of all registered SecretProperty instances (for documentation and env filtering) + private final Set _registeredNames = Collections.synchronizedSet(new HashSet<>()); + + private volatile ExternalSecretProvider _externalProvider = null; + + // Refresh timer — used when an ExternalSecretProvider is registered + private final ScheduledExecutorService _scheduler = Executors.newSingleThreadScheduledExecutor(r -> { + Thread t = new Thread(r, "SecretService-refresh"); + t.setDaemon(true); + return t; + }); + private volatile ScheduledFuture _refreshFuture = null; + + /** + * Register a LenientStartupPropertyHandler to capture all "secret.*" entries from + * startup property files and their corresponding environment variables. Call this + * from CoreModule.init() after constructing and registering the service. + */ + public void handleStartupProperties() + { + ModuleLoader.getInstance().handleStartupProperties( + new LenientStartupPropertyHandler<>(STARTUP_PROPERTY_SCOPE, _SecretDocProperty.INSTANCE) + { + @Override + public void handle(Collection entries) + { + entries.forEach(entry -> _startupSecrets.put(entry.getName(), entry.getValue())); + } + }); + } + + @Override + public void register(@NotNull SecretProperty property) + { + _registeredNames.add(property.getPropertyName()); + } + + @Override + public @Nullable String getSecret(@NotNull SecretProperty property) + { + String name = property.getPropertyName(); + + // External provider (e.g., SSM) has highest priority + ExternalSecretProvider provider = _externalProvider; + if (provider != null) + { + String value = provider.getSecret(name); + if (value != null) + { + LOG.debug("Secret '{}' resolved from external provider", name); + return value; + } + } + + String value = _startupSecrets.get(name); + if (value != null) + LOG.debug("Secret '{}' resolved from startup properties", name); + return value; + } + + @Override + public void setExternalProvider(@NotNull ExternalSecretProvider provider) + { + _externalProvider = provider; + scheduleRefresh(); + } + + /** Cancel the refresh timer on server shutdown. */ + public void shutdown() + { + if (_refreshFuture != null) + _refreshFuture.cancel(false); + _scheduler.shutdown(); + } + + private void scheduleRefresh() + { + if (_refreshFuture != null) + _refreshFuture.cancel(false); + + _refreshFuture = _scheduler.scheduleAtFixedRate(() -> { + ExternalSecretProvider p = _externalProvider; + if (p != null) + p.refreshAll(Collections.unmodifiableSet(_registeredNames)); + }, 5, 5, TimeUnit.MINUTES); + } + + // Singleton SecretProperty used as the documentation entry for the "secret" scope on the + // Startup Properties admin page. The scope name is "secret" and the property name is "*" + // to indicate that any property name is accepted under this scope. + private static class _SecretDocProperty extends SecretProperty + { + static final _SecretDocProperty INSTANCE = new _SecretDocProperty(); + + private _SecretDocProperty() + { + super(STARTUP_PROPERTY_SCOPE, "Any secret registered via SecretService.register(). " + + "Provide as: secret.="); + } + } + + public static class TestCase extends Assert + { + @Test + public void testStartupPropertyLoading() + { + SecretServiceImpl svc = new SecretServiceImpl(); + svc._startupSecrets.put("test.API_KEY", "abc123"); + + SecretProperty prop = new SecretProperty("test.API_KEY", "Test API Key"); + svc.register(prop); + + assertEquals("abc123", svc.getSecret(prop)); + } + + @Test + public void testMissingSecret() + { + SecretServiceImpl svc = new SecretServiceImpl(); + SecretProperty prop = new SecretProperty("nonexistent.KEY"); + assertNull(svc.getSecret(prop)); + } + + @Test + public void testExternalProviderPriority() + { + SecretServiceImpl svc = new SecretServiceImpl(); + svc._startupSecrets.put("my.KEY", "from-startup"); + + svc.setExternalProvider(new ExternalSecretProvider() + { + @Override + public @Nullable String getSecret(String propertyName) + { + return "my.KEY".equals(propertyName) ? "from-external" : null; + } + + @Override + public void refreshAll(Collection propertyNames) {} + }); + + SecretProperty prop = new SecretProperty("my.KEY"); + assertEquals("from-external", svc.getSecret(prop)); + svc.shutdown(); + } + } +} diff --git a/core/src/org/labkey/core/vcs/VcsServiceImpl.java b/core/src/org/labkey/core/vcs/VcsServiceImpl.java index 287ffebc112..d0b774cc75f 100644 --- a/core/src/org/labkey/core/vcs/VcsServiceImpl.java +++ b/core/src/org/labkey/core/vcs/VcsServiceImpl.java @@ -4,6 +4,7 @@ import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; import org.labkey.api.util.FileUtil; +import org.labkey.api.util.LabKeyProcessBuilder; import org.labkey.api.vcs.Vcs; import org.labkey.api.vcs.VcsService; @@ -103,7 +104,7 @@ private void execute(String... command) { String cl = log(_rootDirectory, command); - ProcessBuilder builder = new ProcessBuilder(command); + LabKeyProcessBuilder builder = new LabKeyProcessBuilder(command); builder.directory(_rootDirectory); try { diff --git a/pipeline/src/org/labkey/pipeline/analysis/CommandTaskImpl.java b/pipeline/src/org/labkey/pipeline/analysis/CommandTaskImpl.java index 864c736bb1b..460727f4254 100644 --- a/pipeline/src/org/labkey/pipeline/analysis/CommandTaskImpl.java +++ b/pipeline/src/org/labkey/pipeline/analysis/CommandTaskImpl.java @@ -48,6 +48,7 @@ import org.labkey.api.security.SecurityManager.TransformSession; import org.labkey.api.settings.AppProps; import org.labkey.api.util.FileType; +import org.labkey.api.util.LabKeyProcessBuilder; import org.labkey.api.util.NetworkDrive; import org.labkey.api.util.Path; import org.labkey.vfs.FileLike; @@ -690,7 +691,7 @@ protected boolean runCommand(RecordedAction action, @Nullable String apiKey, @Nu { Map replacements = container == null ? Collections.emptyMap() : createReplacements(null, apiKey, container); - ProcessBuilder pb = new ProcessBuilder(_factory.toArgs(this, replacements)); + LabKeyProcessBuilder pb = new LabKeyProcessBuilder(_factory.toArgs(this, replacements)); applyEnvironment(pb); List args = pb.command(); @@ -732,7 +733,7 @@ protected boolean runCommand(RecordedAction action, @Nullable String apiKey, @Nu action.setStartTime(new Date()); action.addParameter(RecordedAction.COMMAND_LINE_PARAM, commandLine); int timeout = _factory._timeout != null ? _factory._timeout : 0; - getJob().runSubProcess(pb, _wd.getDir(), fileOutput, lineInterval, false, timeout, TimeUnit.SECONDS); + getJob().runSubProcess(pb.processBuilder(), _wd.getDir(), fileOutput, lineInterval, false, timeout, TimeUnit.SECONDS); action.setEndTime(new Date()); return true; } @@ -777,7 +778,7 @@ public void testSubstitution() } } - private void applyEnvironment(ProcessBuilder pb) + private void applyEnvironment(LabKeyProcessBuilder pb) { Map originalEnvironment = new HashMap<>(pb.environment()); for (Map.Entry entry : _factory._environment.entrySet()) diff --git a/pipeline/src/org/labkey/pipeline/cluster/ClusterStartup.java b/pipeline/src/org/labkey/pipeline/cluster/ClusterStartup.java index 3fc7dafa737..d6a1df84bf9 100644 --- a/pipeline/src/org/labkey/pipeline/cluster/ClusterStartup.java +++ b/pipeline/src/org/labkey/pipeline/cluster/ClusterStartup.java @@ -29,6 +29,7 @@ import org.labkey.api.reader.Readers; import org.labkey.api.test.TestWhen; import org.labkey.api.util.ContextListener; +import org.labkey.api.util.LabKeyProcessBuilder; import org.labkey.api.util.FileUtil; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.PageFlowUtil; @@ -225,7 +226,7 @@ public void testBadPath() throws IOException, InterruptedException protected String executeJobRemote(List args, int expectedExitCode) throws IOException, InterruptedException { - ProcessBuilder pb = new ProcessBuilder(args); + LabKeyProcessBuilder pb = new LabKeyProcessBuilder(args); pb.directory(_tempDir); pb.redirectErrorStream(true); Process proc = pb.start(); From e49b0861e64bcc45445278e4f12046ce3888acb2 Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Fri, 15 May 2026 09:56:52 -0700 Subject: [PATCH 2/4] comment out the time stuff for now environment variable implementation startup ordering --- core/src/org/labkey/core/CoreModule.java | 2 +- .../core/secrets/SecretServiceImpl.java | 63 ++++++++++--------- 2 files changed, 36 insertions(+), 29 deletions(-) diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index c7572cbe0e1..540caec21fb 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -421,7 +421,6 @@ public boolean hasScripts() protected void init() { SecretServiceImpl secretService = new SecretServiceImpl(); - secretService.handleStartupProperties(); SecretService.setInstance(secretService); ContextListener.addShutdownListener(ShutdownListener.of("SecretService", null, secretService::shutdown)); @@ -973,6 +972,7 @@ public void startupAfterSpringConfig(ModuleContext moduleContext) checkForMissingDbViews(); + ((SecretServiceImpl)SecretService.get()).handleStartupProperties(); ProductConfiguration.handleStartupProperties(); // This listener deletes all properties; make sure it executes after most of the other listeners diff --git a/core/src/org/labkey/core/secrets/SecretServiceImpl.java b/core/src/org/labkey/core/secrets/SecretServiceImpl.java index 874efd3d658..798a428201a 100644 --- a/core/src/org/labkey/core/secrets/SecretServiceImpl.java +++ b/core/src/org/labkey/core/secrets/SecretServiceImpl.java @@ -1,5 +1,6 @@ package org.labkey.core.secrets; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -14,10 +15,7 @@ import org.labkey.api.util.logging.LogHelper; import java.util.Collection; -import java.util.Collections; -import java.util.HashSet; import java.util.Map; -import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; @@ -31,18 +29,18 @@ public class SecretServiceImpl implements SecretService // Secrets loaded from startup properties / environment variables private final Map _startupSecrets = new ConcurrentHashMap<>(); - // Names of all registered SecretProperty instances (for documentation and env filtering) - private final Set _registeredNames = Collections.synchronizedSet(new HashSet<>()); + // Registered SecretProperty instances keyed by property name (for documentation and env filtering) + private final Map _registeredSecrets = new ConcurrentHashMap<>(); private volatile ExternalSecretProvider _externalProvider = null; - // Refresh timer — used when an ExternalSecretProvider is registered - private final ScheduledExecutorService _scheduler = Executors.newSingleThreadScheduledExecutor(r -> { - Thread t = new Thread(r, "SecretService-refresh"); - t.setDaemon(true); - return t; - }); - private volatile ScheduledFuture _refreshFuture = null; +// // Refresh timer — used when an ExternalSecretProvider is registered +// private final ScheduledExecutorService _scheduler = Executors.newSingleThreadScheduledExecutor(r -> { +// Thread t = new Thread(r, "SecretService-refresh"); +// t.setDaemon(true); +// return t; +// }); +// private volatile ScheduledFuture _refreshFuture = null; /** * Register a LenientStartupPropertyHandler to capture all "secret.*" entries from @@ -58,6 +56,12 @@ public void handleStartupProperties() public void handle(Collection entries) { entries.forEach(entry -> _startupSecrets.put(entry.getName(), entry.getValue())); + for (var name : _registeredSecrets.keySet()) + { + var s = System.getenv(name); + if (StringUtils.isNotBlank(s)) + _startupSecrets.put(name,s); + } } }); } @@ -65,13 +69,16 @@ public void handle(Collection entries) @Override public void register(@NotNull SecretProperty property) { - _registeredNames.add(property.getPropertyName()); + _registeredSecrets.put(property.getPropertyName(), property); } @Override public @Nullable String getSecret(@NotNull SecretProperty property) { String name = property.getPropertyName(); + var registered = _registeredSecrets.get(name); + if (registered != property) + return null; // External provider (e.g., SSM) has highest priority ExternalSecretProvider provider = _externalProvider; @@ -95,28 +102,28 @@ public void register(@NotNull SecretProperty property) public void setExternalProvider(@NotNull ExternalSecretProvider provider) { _externalProvider = provider; - scheduleRefresh(); +// scheduleRefresh(); } /** Cancel the refresh timer on server shutdown. */ public void shutdown() { - if (_refreshFuture != null) - _refreshFuture.cancel(false); - _scheduler.shutdown(); +// if (_refreshFuture != null) +// _refreshFuture.cancel(false); +// _scheduler.shutdown(); } - private void scheduleRefresh() - { - if (_refreshFuture != null) - _refreshFuture.cancel(false); - - _refreshFuture = _scheduler.scheduleAtFixedRate(() -> { - ExternalSecretProvider p = _externalProvider; - if (p != null) - p.refreshAll(Collections.unmodifiableSet(_registeredNames)); - }, 5, 5, TimeUnit.MINUTES); - } +// private void scheduleRefresh() +// { +// if (_refreshFuture != null) +// _refreshFuture.cancel(false); +// +// _refreshFuture = _scheduler.scheduleAtFixedRate(() -> { +// ExternalSecretProvider p = _externalProvider; +// if (p != null) +// p.refreshAll(_registeredSecrets.keySet()); +// }, 5, 5, TimeUnit.MINUTES); +// } // Singleton SecretProperty used as the documentation entry for the "secret" scope on the // Startup Properties admin page. The scope name is "secret" and the property name is "*" From d612904201ac33fd76ff72d192db13e03834c445 Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Fri, 15 May 2026 10:02:45 -0700 Subject: [PATCH 3/4] duplicate test --- core/src/org/labkey/core/secrets/SecretServiceImpl.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/org/labkey/core/secrets/SecretServiceImpl.java b/core/src/org/labkey/core/secrets/SecretServiceImpl.java index 798a428201a..b4cbb61bb12 100644 --- a/core/src/org/labkey/core/secrets/SecretServiceImpl.java +++ b/core/src/org/labkey/core/secrets/SecretServiceImpl.java @@ -12,6 +12,7 @@ import org.labkey.api.secrets.SecretService; import org.labkey.api.settings.LenientStartupPropertyHandler; import org.labkey.api.settings.StartupPropertyEntry; +import org.labkey.api.util.ConfigurationException; import org.labkey.api.util.logging.LogHelper; import java.util.Collection; @@ -69,7 +70,9 @@ public void handle(Collection entries) @Override public void register(@NotNull SecretProperty property) { - _registeredSecrets.put(property.getPropertyName(), property); + var prev = _registeredSecrets.put(property.getPropertyName(), property); + if (null != prev) + throw new ConfigurationException("Duplicate secret registered: " + property.getPropertyName()); } @Override From 1dd9bf219ebf62dc6c439ea7e2d59c9a8694589b Mon Sep 17 00:00:00 2001 From: labkey-matthewb Date: Fri, 15 May 2026 13:15:42 -0700 Subject: [PATCH 4/4] fix mcp init --- .../labkey/api/secrets/SecretProperty.java | 3 ++- .../org/labkey/api/secrets/SecretService.java | 9 ++++++++ .../labkey/api/util/LabKeyProcessBuilder.java | 10 ++++----- .../labkey/core/admin/AdminController.java | 4 +++- .../core/secrets/SecretServiceImpl.java | 21 ++++++++++++++++++- 5 files changed, 39 insertions(+), 8 deletions(-) diff --git a/api/src/org/labkey/api/secrets/SecretProperty.java b/api/src/org/labkey/api/secrets/SecretProperty.java index a6294ea0e5c..7d87136f4c4 100644 --- a/api/src/org/labkey/api/secrets/SecretProperty.java +++ b/api/src/org/labkey/api/secrets/SecretProperty.java @@ -8,7 +8,8 @@ * {@link SecretService#getSecret}. * * Startup property file convention: {@code secret.=} - * Environment variable convention: {@code labkey.prop.secret.=} + * Java property convention: {@code -Plabkey.prop.secret.=} + * Environment variable convention: {@code export =} */ public class SecretProperty implements StartupProperty { diff --git a/api/src/org/labkey/api/secrets/SecretService.java b/api/src/org/labkey/api/secrets/SecretService.java index 652577febea..a3c062e4796 100644 --- a/api/src/org/labkey/api/secrets/SecretService.java +++ b/api/src/org/labkey/api/secrets/SecretService.java @@ -43,9 +43,18 @@ static void setInstance(SecretService service) /** * Retrieve the value of a secret. Returns {@code null} if the secret has not been * configured in any source. Never logs or caches the returned value. + * + *

Identity contract: the {@code property} argument must be the exact + * {@code static final} instance that was passed to {@link #register}. A freshly constructed + * {@code new SecretProperty("SOME_KEY")} will always return {@code null}, even if a secret + * with that name is configured. This prevents unregistered callers from reading secrets + * they did not declare. */ @Nullable String getSecret(@NotNull SecretProperty property); + /** Returns true if the given property name has been registered via {@link #register}. */ + boolean isRegisteredSecret(@NotNull String name); + /** * Register an {@link ExternalSecretProvider} (e.g., AWS SSM). The provider is * consulted at higher priority than startup-property secrets. Should be called after diff --git a/api/src/org/labkey/api/util/LabKeyProcessBuilder.java b/api/src/org/labkey/api/util/LabKeyProcessBuilder.java index cbba925b032..6a01703318c 100644 --- a/api/src/org/labkey/api/util/LabKeyProcessBuilder.java +++ b/api/src/org/labkey/api/util/LabKeyProcessBuilder.java @@ -87,10 +87,10 @@ public ProcessBuilder processBuilder() private void sanitizeEnvironment() { SecretService secrets = ServiceRegistry.get().getService(SecretService.class); - - _pb.environment().keySet().removeIf(name -> - name.toLowerCase().contains("secret") || - name.toLowerCase().contains("password") - ); + _pb.environment().keySet().removeIf(name -> { + String lc = name.toLowerCase(); + return lc.contains("secret") || lc.contains("password") || + (secrets != null && secrets.isRegisteredSecret(name)); + }); } } diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index bffe51b1e1a..464852dddf0 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -188,6 +188,7 @@ import org.labkey.api.reports.ExternalScriptEngineDefinition; import org.labkey.api.reports.LabKeyScriptEngineManager; import org.labkey.api.search.SearchService; +import org.labkey.api.secrets.SecretService; import org.labkey.api.security.ActionNames; import org.labkey.api.security.AdminConsoleAction; import org.labkey.api.security.CSRF; @@ -3396,10 +3397,11 @@ public class EnvironmentVariablesAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) { + SecretService secrets = SecretService.get(); Map env = new LinkedHashMap<>(System.getenv()); env.replaceAll((name, value) -> { String lc = name.toLowerCase(); - if (lc.contains("secret") || lc.contains("password")) + if (lc.contains("secret") || lc.contains("password") || secrets.isRegisteredSecret(name)) return "[REDACTED]"; return value; }); diff --git a/core/src/org/labkey/core/secrets/SecretServiceImpl.java b/core/src/org/labkey/core/secrets/SecretServiceImpl.java index b4cbb61bb12..b64ec722993 100644 --- a/core/src/org/labkey/core/secrets/SecretServiceImpl.java +++ b/core/src/org/labkey/core/secrets/SecretServiceImpl.java @@ -101,6 +101,12 @@ public void register(@NotNull SecretProperty property) return value; } + @Override + public boolean isRegisteredSecret(@NotNull String name) + { + return _registeredSecrets.containsKey(name); + } + @Override public void setExternalProvider(@NotNull ExternalSecretProvider provider) { @@ -157,10 +163,22 @@ public void testStartupPropertyLoading() } @Test - public void testMissingSecret() + public void testUnregisteredSecretReturnsNull() + { + // getSecret() requires the same instance passed to register(); a fresh instance with + // the same name must not be able to retrieve a secret it didn't declare. + SecretServiceImpl svc = new SecretServiceImpl(); + SecretProperty prop = new SecretProperty("some.KEY"); + svc._startupSecrets.put("some.KEY", "value"); + assertNull("unregistered property must not retrieve a secret", svc.getSecret(prop)); + } + + @Test + public void testRegisteredSecretWithNoValueReturnsNull() { SecretServiceImpl svc = new SecretServiceImpl(); SecretProperty prop = new SecretProperty("nonexistent.KEY"); + svc.register(prop); assertNull(svc.getSecret(prop)); } @@ -183,6 +201,7 @@ public void refreshAll(Collection propertyNames) {} }); SecretProperty prop = new SecretProperty("my.KEY"); + svc.register(prop); assertEquals("from-external", svc.getSecret(prop)); svc.shutdown(); }