Fb secretservice

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<FileLike> propFiles = propsDir.getChildren().stream().filter(f -> f.getName().endsWith(".properties")).toList();
+            {
+                _log.debug("no 'newinstall' file detected");
+            }
 
-        if (!propFiles.isEmpty())
-        {
-            List<FileLike> sortedPropFiles = propFiles.stream()
-                .sorted(Comparator.comparing(FileLike::getName).reversed())
-                .toList();
+            List<FileLike> 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<FileLike> 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<Object, Object> 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<Object, Object> 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

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

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<String> propertyNames);
+}

api/src/org/labkey/api/secrets/SecretProperty.java

@@ -0,0 +1,41 @@
+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.<propertyName>=<value>}
+ * Java property convention:  {@code -Plabkey.prop.secret.<propertyName>=<value>}
+ * Environment variable convention: {@code export <propertyName>=<value>}
+ */
+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;
+    }
+}

api/src/org/labkey/api/secrets/SecretService.java

@@ -0,0 +1,67 @@
+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.
+ *
+ * <p>Modules should:
+ * <ol>
+ *   <li>Declare each secret as a {@code static final SecretProperty} constant.</li>
+ *   <li>Call {@link #register} in their {@code doStartup()} method.</li>
+ *   <li>Call {@link #getSecret} wherever the value is needed.</li>
+ * </ol>
+ *
+ * <p>Startup property file convention: {@code secret.<propertyName>=<value>}
+ */
+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.
+     *
+     * <p><strong>Identity contract:</strong> 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
+     * 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);
+}

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();
             }

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.
+ *
+ * <p>Variables are removed (silently) if their name:
+ * <ul>
+ *   <li>matches any property name registered with {@link SecretService}, or</li>
+ *   <li>contains "secret" (case-insensitive), or</li>
+ *   <li>contains "password" (case-insensitive).</li>
+ * </ul>
+ *
+ * <p>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<String> command)
+    {
+        _pb = new ProcessBuilder(command);
+        sanitizeEnvironment();
+    }
+
+    public LabKeyProcessBuilder(String... command)
+    {
+        _pb = new ProcessBuilder(command);
+        sanitizeEnvironment();
+    }
+
+    public LabKeyProcessBuilder(File directory, List<String> command)
+    {
+        _pb = new ProcessBuilder(command);
+        _pb.directory(directory);
+        sanitizeEnvironment();
+    }
+
+    public List<String> command()
+    {
+        return _pb.command();
+    }
+
+    public Map<String, String> 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 -> {
+            String lc = name.toLowerCase();
+            return lc.contains("secret") || lc.contains("password") ||
+                   (secrets != null && secrets.isRegisteredSecret(name));
+        });
+    }
+}