From 56107eab8796dbf86edbe0d7c2990a2e85ca4762 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 8 Apr 2019 14:28:30 -0400 Subject: [PATCH] Limiting system properties transmitted to the agent to those for which this has been explicitly indicated as safe. --- .../java/hudson/util/StreamTaskListener.java | 6 +- .../java/jenkins/util/SystemProperties.java | 85 +++++++------------ .../hudson/util/ArgumentListBuilder2Test.java | 5 +- 3 files changed, 42 insertions(+), 54 deletions(-) diff --git a/core/src/main/java/hudson/util/StreamTaskListener.java b/core/src/main/java/hudson/util/StreamTaskListener.java index 8136f0aaf2d3..931a0b99bba8 100644 --- a/core/src/main/java/hudson/util/StreamTaskListener.java +++ b/core/src/main/java/hudson/util/StreamTaskListener.java @@ -172,13 +172,17 @@ private void writeObject(ObjectOutputStream out) throws IOException { } } + private static final String KEY_AUTO_FLUSH = StreamTaskListener.class.getName() + ".AUTO_FLUSH"; + static { + SystemProperties.allowOnAgent(KEY_AUTO_FLUSH); + } /** * Restores eager remote flushing behavior. * By default, remote tasks are expected to call {@link PrintStream#flush} before exiting. * This flag will ensure that no output is lost from tasks which neglect to do so, * at the expense of heavier Remoting traffic and reduced performance. */ - private static /* not final */ boolean AUTO_FLUSH = SystemProperties.getBoolean(StreamTaskListener.class.getName() + ".AUTO_FLUSH"); + private static /* not final */ boolean AUTO_FLUSH = SystemProperties.getBoolean(KEY_AUTO_FLUSH); private void readObject(ObjectInputStream in) throws IOException, ClassNotFoundException { out = new PrintStream((OutputStream)in.readObject(), AUTO_FLUSH); diff --git a/core/src/main/java/jenkins/util/SystemProperties.java b/core/src/main/java/jenkins/util/SystemProperties.java index 349f2e7ef477..cf98b8e96bbf 100644 --- a/core/src/main/java/jenkins/util/SystemProperties.java +++ b/core/src/main/java/jenkins/util/SystemProperties.java @@ -34,10 +34,10 @@ import hudson.slaves.ComputerListener; import java.io.IOException; import java.util.Collections; -import java.util.Enumeration; import java.util.HashMap; +import java.util.HashSet; import java.util.Map; -import java.util.TreeMap; +import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.Nonnull; @@ -79,21 +79,12 @@ @Restricted(NoExternalUse.class) public class SystemProperties { + @FunctionalInterface private interface Handler { @CheckForNull String getString(String key); - @Nonnull Map getAllStrings(); } - private static final Handler NULL_HANDLER = new Handler() { - @Override - public String getString(String key) { - return null; - } - @Override - public Map getAllStrings() { - return Collections.emptyMap(); - } - }; + private static final Handler NULL_HANDLER = key -> null; private static @Nonnull Handler handler = NULL_HANDLER; @@ -106,32 +97,16 @@ public static final class Listener implements ServletContextListener, OnMaster { @Override public void contextInitialized(ServletContextEvent event) { ServletContext theContext = event.getServletContext(); - handler = new Handler() { - @Override - public String getString(String key) { - if (StringUtils.isNotBlank(key)) { - try { - return theContext.getInitParameter(key); - } catch (SecurityException ex) { - // Log exception and go on - LOGGER.log(Level.CONFIG, "Access to the property {0} is prohibited", key); - } + handler = key -> { + if (StringUtils.isNotBlank(key)) { + try { + return theContext.getInitParameter(key); + } catch (SecurityException ex) { + // Log exception and go on + LOGGER.log(Level.CONFIG, "Access to the property {0} is prohibited", key); } - return null; - } - @Override - public Map getAllStrings() { - Map values = new HashMap<>(); - Enumeration names = theContext.getInitParameterNames(); - while (names.hasMoreElements()) { - String key = names.nextElement(); - String value = getString(key); - if (value != null) { - values.put(key, value); - } - } - return values; } + return null; }; } @@ -142,6 +117,15 @@ public void contextDestroyed(ServletContextEvent event) { } + private static final Set ALLOW_ON_AGENT = Collections.synchronizedSet(new HashSet<>()); + + /** + * Mark a key whose value should be made accessible in agent JVMs. + */ + public static void allowOnAgent(String key) { + ALLOW_ON_AGENT.add(key); + } + @Extension public static final class AgentCopier extends ComputerListener { @Override @@ -150,32 +134,29 @@ public void preOnline(Computer c, Channel channel, FilePath root, TaskListener l } private static final class CopySystemProperties extends MasterToSlaveCallable { private static final long serialVersionUID = 1; - private final Map allStrings; - @SuppressWarnings("unchecked") + private final Map snapshot; CopySystemProperties() { - // Take a snapshot of all system properties and context variables available on the master at the time the agent starts. - allStrings = new HashMap<>(); - allStrings.putAll(handler.getAllStrings()); - allStrings.putAll((Map) System.getProperties()); // these take precedence + // Take a snapshot of those system properties and context variables available on the master at the time the agent starts which have been whitelisted for that purpose. + snapshot = new HashMap<>(); + for (String key : ALLOW_ON_AGENT) { + snapshot.put(key, getString(key)); + } + LOGGER.log(Level.FINE, "taking snapshot of {0}", snapshot); } @Override public Void call() throws RuntimeException { - handler = new CopiedHandler(allStrings); + handler = new CopiedHandler(snapshot); return null; } } private static final class CopiedHandler implements Handler { - private final Map allStrings; - CopiedHandler(Map allStrings) { - this.allStrings = allStrings; + private final Map snapshot; + CopiedHandler(Map snapshot) { + this.snapshot = snapshot; } @Override public String getString(String key) { - return allStrings.get(key); - } - @Override - public Map getAllStrings() { - return allStrings; + return snapshot.get(key); } } } diff --git a/test/src/test/java/hudson/util/ArgumentListBuilder2Test.java b/test/src/test/java/hudson/util/ArgumentListBuilder2Test.java index 29feb70424ff..3ce036ae69b3 100644 --- a/test/src/test/java/hudson/util/ArgumentListBuilder2Test.java +++ b/test/src/test/java/hudson/util/ArgumentListBuilder2Test.java @@ -47,6 +47,7 @@ import java.io.File; import java.io.StringWriter; import java.util.logging.Level; +import jenkins.util.SystemProperties; /** * @author Kohsuke Kawaguchi @@ -57,7 +58,9 @@ public class ArgumentListBuilder2Test { public JenkinsRule j = new JenkinsRule(); @Rule - public LoggerRule logging = new LoggerRule().record(StreamTaskListener.class, Level.FINE); + public LoggerRule logging = new LoggerRule(). + record(StreamTaskListener.class, Level.FINE). + record(SystemProperties.class, Level.FINE); /** * Makes sure {@link RemoteLauncher} properly masks arguments.