[SECURITY-49] Deprecating Jenkins.getSecretKey()

We are replacing it by the ConfidentialStore class and the
ConfidentialKey class, which provides purpose-specific confidential
information that are separated from each other.

In this way, not all eggs are in one basket, and in case of a
compromise, the impact will contained.

Also replaced several insecure use of digest(secret|messsage) or
digest(message|secret) by HMAC.

core/pom.xml

@@ -768,6 +768,35 @@ THE SOFTWARE.
           </archive>
         </configuration>
       </plugin>
+      <plugin><!-- run unit test in src/test/java -->
+        <groupId>org.kohsuke.gmaven</groupId>
+        <artifactId>gmaven-plugin</artifactId>
+        <!-- version specified in grandparent pom -->
+        <executions>
+          <execution>
+            <goals>
+              <goal>testCompile</goal>
+            </goals>
+            <configuration>
+              <sources>
+                <fileset>
+                  <directory>${project.basedir}/src/test/java</directory>
+                  <includes>
+                    <include>**/*.groovy</include>
+                  </includes>
+                </fileset>
+              </sources>
+            </configuration>
+          </execution>
+        </executions>
+        <dependencies>
+          <dependency>
+            <groupId>org.codehaus.groovy</groupId>
+            <artifactId>groovy-all</artifactId>
+            <version>1.8.5</version>
+          </dependency>
+        </dependencies>
+      </plugin>
     </plugins>
   </build>
 

core/src/main/java/hudson/DNSMultiCast.java

@@ -46,7 +46,7 @@ public class DNSMultiCast implements Closeable {
                     if (tal!=null)
                         props.put("slave-port",String.valueOf(tal.getPort()));
 
-                    props.put("server-id", Util.getDigestOf(jenkins.getSecretKey()));
+                    props.put("server-id", jenkins.getLegacyInstanceId());
 
                     URL jenkins_url = new URL(rootURL);
                     int jenkins_port = jenkins_url.getPort();

core/src/main/java/hudson/UDPBroadcastThread.java

@@ -86,7 +86,7 @@ public class UDPBroadcastThread extends Thread {
                 StringBuilder rsp = new StringBuilder("<hudson>");
                 tag(rsp,"version", Jenkins.VERSION);
                 tag(rsp,"url", jenkins.getRootUrl());
-                tag(rsp,"server-id", Util.getDigestOf(jenkins.getSecretKey()));
+                tag(rsp,"server-id", jenkins.getLegacyInstanceId());
                 tag(rsp,"slave-port",tal==null?null:tal.getPort());
 
                 for (UDPBroadcastFragment f : UDPBroadcastFragment.all())

core/src/main/java/hudson/console/AnnotatedLargeText.java

@@ -29,6 +29,7 @@ import hudson.remoting.ObjectInputStreamEx;
 import hudson.util.IOException2;
 import hudson.util.Secret;
 import hudson.util.TimeUnit2;
+import jenkins.security.CryptoConfidentialKey;
 import org.apache.commons.io.output.ByteArrayOutputStream;
 import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
@@ -116,8 +117,7 @@ public class AnnotatedLargeText<T> extends LargeText {
         try {
             String base64 = req!=null ? req.getHeader("X-ConsoleAnnotator") : null;
             if (base64!=null) {
-                Cipher sym = Secret.getCipher("AES");
-                sym.init(Cipher.DECRYPT_MODE, Jenkins.getInstance().getSecretKeyAsAES128());
+                Cipher sym = PASSING_ANNOTATOR.decrypt();
 
                 ObjectInputStream ois = new ObjectInputStreamEx(new GZIPInputStream(
                         new CipherInputStream(new ByteArrayInputStream(Base64.decode(base64.toCharArray())),sym)),
@@ -131,8 +131,6 @@ public class AnnotatedLargeText<T> extends LargeText {
                     ois.close();
                 }
             }
-        } catch (GeneralSecurityException e) {
-            throw new IOException2(e);
         } catch (ClassNotFoundException e) {
             throw new IOException2(e);
         }
@@ -158,21 +156,20 @@ public class AnnotatedLargeText<T> extends LargeText {
                 w, createAnnotator(Stapler.getCurrentRequest()), context, charset);
         long r = super.writeLogTo(start,caw);
 
-        try {
-            ByteArrayOutputStream baos = new ByteArrayOutputStream();
-            Cipher sym = Secret.getCipher("AES");
-            sym.init(Cipher.ENCRYPT_MODE, Jenkins.getInstance().getSecretKeyAsAES128());
-            ObjectOutputStream oos = new ObjectOutputStream(new GZIPOutputStream(new CipherOutputStream(baos,sym)));
-            oos.writeLong(System.currentTimeMillis()); // send timestamp to prevent a replay attack
-            oos.writeObject(caw.getConsoleAnnotator());
-            oos.close();
-            StaplerResponse rsp = Stapler.getCurrentResponse();
-            if (rsp!=null)
-                rsp.setHeader("X-ConsoleAnnotator", new String(Base64.encode(baos.toByteArray())));
-        } catch (GeneralSecurityException e) {
-            throw new IOException2(e);
-        }
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        Cipher sym = PASSING_ANNOTATOR.encrypt();
+        ObjectOutputStream oos = new ObjectOutputStream(new GZIPOutputStream(new CipherOutputStream(baos,sym)));
+        oos.writeLong(System.currentTimeMillis()); // send timestamp to prevent a replay attack
+        oos.writeObject(caw.getConsoleAnnotator());
+        oos.close();
+        StaplerResponse rsp = Stapler.getCurrentResponse();
+        if (rsp!=null)
+            rsp.setHeader("X-ConsoleAnnotator", new String(Base64.encode(baos.toByteArray())));
         return r;
     }
 
+    /**
+     * Used for sending the state of ConsoleAnnotator to the client, because we are deserializing this object later.
+     */
+    private static final CryptoConfidentialKey PASSING_ANNOTATOR = new CryptoConfidentialKey(AnnotatedLargeText.class,"consoleAnnotator");
 }

core/src/main/java/hudson/model/Job.java

@@ -62,6 +62,7 @@ import hudson.widgets.HistoryWidget.Adapter;
 import hudson.widgets.Widget;
 import jenkins.model.Jenkins;
 import jenkins.model.ProjectNamingStrategy;
+import jenkins.security.HexStringConfidentialKey;
 import jenkins.util.io.OnMaster;
 import net.sf.json.JSONException;
 import net.sf.json.JSONObject;
@@ -308,8 +309,8 @@ public abstract class Job<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
      */
     public EnvVars getCharacteristicEnvVars() {
         EnvVars env = new EnvVars();
-        env.put("JENKINS_SERVER_COOKIE",Util.getDigestOf("ServerID:"+ Jenkins.getInstance().getSecretKey()));
-        env.put("HUDSON_SERVER_COOKIE",Util.getDigestOf("ServerID:"+ Jenkins.getInstance().getSecretKey())); // Legacy compatibility
+        env.put("JENKINS_SERVER_COOKIE",SERVER_COOKIE.get());
+        env.put("HUDSON_SERVER_COOKIE",SERVER_COOKIE.get()); // Legacy compatibility
         env.put("JOB_NAME",getFullName());
         return env;
     }
@@ -1313,4 +1314,6 @@ public abstract class Job<JobT extends Job<JobT, RunT>, RunT extends Run<JobT, R
     public BuildTimelineWidget getTimeline() {
         return new BuildTimelineWidget(getBuilds());
     }
+
+    private final static HexStringConfidentialKey SERVER_COOKIE = new HexStringConfidentialKey(Job.class,"serverCookie",16);
 }

core/src/main/java/hudson/model/UsageStatistics.java

@@ -121,31 +121,31 @@ public class UsageStatistics extends PageDecorator {
      * Gets the encrypted usage stat data to be sent to the Hudson server.
      */
     public String getStatData() throws IOException {
-        Jenkins h = Jenkins.getInstance();
+        Jenkins j = Jenkins.getInstance();
 
         JSONObject o = new JSONObject();
         o.put("stat",1);
-        o.put("install", Util.getDigestOf(h.getSecretKey()));
-        o.put("servletContainer",h.servletContext.getServerInfo());
+        o.put("install", j.getLegacyInstanceId());
+        o.put("servletContainer", j.servletContext.getServerInfo());
         o.put("version", Jenkins.VERSION);
 
         List<JSONObject> nodes = new ArrayList<JSONObject>();
-        for( Computer c : h.getComputers() ) {
+        for( Computer c : j.getComputers() ) {
             JSONObject  n = new JSONObject();
-            if(c.getNode()==h) {
+            if(c.getNode()==j) {
                 n.put("master",true);
                 n.put("jvm-vendor", System.getProperty("java.vm.vendor"));
                 n.put("jvm-version", System.getProperty("java.version"));
             }
             n.put("executors",c.getNumExecutors());
-            DescriptorImpl descriptor = h.getDescriptorByType(DescriptorImpl.class);
+            DescriptorImpl descriptor = j.getDescriptorByType(DescriptorImpl.class);
             n.put("os", descriptor.get(c));
             nodes.add(n);
         }
         o.put("nodes",nodes);
 
         List<JSONObject> plugins = new ArrayList<JSONObject>();
-        for( PluginWrapper pw : h.getPluginManager().getPlugins() ) {
+        for( PluginWrapper pw : j.getPluginManager().getPlugins() ) {
             if(!pw.isActive())  continue;   // treat disabled plugins as if they are uninstalled
             JSONObject p = new JSONObject();
             p.put("name",pw.getShortName());
@@ -155,7 +155,7 @@ public class UsageStatistics extends PageDecorator {
         o.put("plugins",plugins);
 
         JSONObject jobs = new JSONObject();
-        List<TopLevelItem> items = h.getItems();
+        List<TopLevelItem> items = j.getItems();
         for (TopLevelItemDescriptor d : Items.all()) {
             int cnt=0;
             for (TopLevelItem item : items) {

core/src/main/java/hudson/security/RememberMeServicesProxy.java

@@ -23,6 +23,8 @@
  */
 package hudson.security;
 
+import jenkins.model.Jenkins;
+import jenkins.security.ConfidentialStore;
 import org.acegisecurity.ui.rememberme.RememberMeServices;
 import org.acegisecurity.Authentication;
 
@@ -33,9 +35,9 @@ import javax.servlet.http.HttpServletResponse;
  * {@link RememberMeServices} proxy.
  *
  * <p>
- * In Hudson, we need {@link jenkins.model.Jenkins} instance to perform remember-me service,
- * because it relies on {@link jenkins.model.Jenkins#getSecretKey()}. However, security
- * filters can be initialized before Hudson is initialized.
+ * In Jenkins, we need {@link Jenkins} instance to perform remember-me service,
+ * because it relies on {@link ConfidentialStore}. However, security
+ * filters can be initialized before Jenkins is initialized.
  * (See #1210 for example.)
  *
  * <p>

core/src/main/java/hudson/security/SecurityRealm.java

@@ -516,10 +516,21 @@ public abstract class SecurityRealm extends AbstractDescribableImpl<SecurityReal
             this.rememberMe = rememberMe;
         }
 
+        @SuppressWarnings("deprecation")
         private static RememberMeServices createRememberMeService(UserDetailsService uds) {
             // create our default TokenBasedRememberMeServices, which depends on the availability of the secret key
             TokenBasedRememberMeServices2 rms = new TokenBasedRememberMeServices2();
             rms.setUserDetailsService(uds);
+            /*
+                TokenBasedRememberMeServices needs to be used in conjunction with RememberMeAuthenticationProvider,
+                and both needs to use the same key (this is a reflection of a poor design in AcgeiSecurity, if you ask me)
+                and various security plugins have its own groovy script that configures them.
+
+                So if we change this, it creates a painful situation for those plugins by forcing them to choose
+                to work with earlier version of Jenkins or newer version of Jenkins, and not both.
+
+                So we keep this here.
+             */
             rms.setKey(Jenkins.getInstance().getSecretKey());
             rms.setParameter("remember_me"); // this is the form field name in login.jelly
             return rms;

core/src/main/java/hudson/security/TokenBasedRememberMeServices2.java

@@ -23,10 +23,10 @@
  */
 package hudson.security;
 
+import jenkins.security.HMACConfidentialKey;
 import org.acegisecurity.ui.rememberme.TokenBasedRememberMeServices;
 import org.acegisecurity.userdetails.UserDetails;
 import org.acegisecurity.Authentication;
-import org.apache.commons.codec.digest.DigestUtils;
 
 /**
  * {@link TokenBasedRememberMeServices} with modification so as not to rely
@@ -41,7 +41,7 @@ import org.apache.commons.codec.digest.DigestUtils;
 public class TokenBasedRememberMeServices2 extends TokenBasedRememberMeServices {
     @Override
     protected String makeTokenSignature(long tokenExpiryTime, UserDetails userDetails) {
-        String expectedTokenSignature = DigestUtils.md5Hex(userDetails.getUsername() + ":" + tokenExpiryTime + ":"
+        String expectedTokenSignature = MAC.mac(userDetails.getUsername() + ":" + tokenExpiryTime + ":"
                 + "N/A" + ":" + getKey());
         return expectedTokenSignature;
     }
@@ -50,4 +50,9 @@ public class TokenBasedRememberMeServices2 extends TokenBasedRememberMeServices
     protected String retrievePassword(Authentication successfulAuthentication) {
         return "N/A";
     }
+
+    /**
+     * Used to compute the token signature securely.
+     */
+    private static final HMACConfidentialKey MAC = new HMACConfidentialKey(TokenBasedRememberMeServices.class,"mac");
 }

core/src/main/java/hudson/security/csrf/DefaultCrumbIssuer.java

@@ -118,7 +118,8 @@ public class DefaultCrumbIssuer extends CrumbIssuer {
     public static final class DescriptorImpl extends CrumbIssuerDescriptor<DefaultCrumbIssuer> implements ModelObject {
 
         public DescriptorImpl() {
-            super(Jenkins.getInstance().getSecretKey(), System.getProperty("hudson.security.csrf.requestfield", ".crumb"));
+            // salt just needs to be unique, and it doesn't have to be a secret
+            super(Jenkins.getInstance().getLegacyInstanceId(), System.getProperty("hudson.security.csrf.requestfield", ".crumb"));
             load();
         }
 

core/src/main/java/hudson/slaves/SlaveComputer.java

@@ -59,6 +59,7 @@ import java.security.Security;
 
 import hudson.util.io.ReopenableFileOutputStream;
 import jenkins.model.Jenkins;
+import jenkins.slaves.JnlpSlaveAgentProtocol;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
 import org.kohsuke.stapler.QueryParameter;
@@ -128,6 +129,10 @@ public class SlaveComputer extends Computer {
         return acceptingTasks;
     }
 
+    public String getJnlpMac() {
+        return JnlpSlaveAgentProtocol.SLAVE_SECRET.mac(getName());
+    }
+
     /**
      * Allows a {@linkplain hudson.slaves.ComputerLauncher} or a {@linkplain hudson.slaves.RetentionStrategy} to
      * suspend tasks being accepted by the slave computer.

core/src/main/java/hudson/util/Secret.java

@@ -31,8 +31,11 @@ import com.thoughtworks.xstream.io.HierarchicalStreamWriter;
 import com.trilead.ssh2.crypto.Base64;
 import jenkins.model.Jenkins;
 import hudson.Util;
+import jenkins.security.CryptoConfidentialKey;
 import org.kohsuke.stapler.Stapler;
 
+import javax.crypto.BadPaddingException;
+import javax.crypto.IllegalBlockSizeException;
 import javax.crypto.SecretKey;
 import javax.crypto.Cipher;
 import java.io.Serializable;
@@ -44,8 +47,8 @@ import java.security.GeneralSecurityException;
  * Glorified {@link String} that uses encryption in the persisted form, to avoid accidental exposure of a secret.
  *
  * <p>
- * Note that since the cryptography relies on {@link jenkins.model.Jenkins#getSecretKey()}, this is not meant as a protection
- * against code running in the same VM, nor against an attacker who has local file system access. 
+ * This is not meant as a protection against code running in the same VM, nor against an attacker
+ * who has local file system access on Jenkins master.
  *
  * <p>
  * {@link Secret}s can correctly read-in plain text password, so this allows the existing
@@ -96,9 +99,13 @@ public final class Secret implements Serializable {
     }
 
     /**
-     * Turns {@link jenkins.model.Jenkins#getSecretKey()} into an AES key.
+     * Turns {@link Jenkins#getSecretKey()} into an AES key.
+     *
+     * @deprecated
+     * This is no longer the key we use to encrypt new information, but we still need this
+     * to be able to decrypt what's already persisted.
      */
-    private static SecretKey getKey() throws UnsupportedEncodingException, GeneralSecurityException {
+    /*package*/ static SecretKey getLegacyKey() throws UnsupportedEncodingException, GeneralSecurityException {
         String secret = SECRET;
         if(secret==null)    return Jenkins.getInstance().getSecretKeyAsAES128();
         return Util.toAes128Key(secret);
@@ -111,8 +118,7 @@ public final class Secret implements Serializable {
      */
     public String getEncryptedValue() {
         try {
-            Cipher cipher = getCipher("AES");
-            cipher.init(Cipher.ENCRYPT_MODE, getKey());
+            Cipher cipher = KEY.encrypt();
             // add the magic suffix which works like a check sum.
             return new String(Base64.encode(cipher.doFinal((value+MAGIC).getBytes("UTF-8"))));
         } catch (GeneralSecurityException e) {
@@ -129,12 +135,14 @@ public final class Secret implements Serializable {
     public static Secret decrypt(String data) {
         if(data==null)      return null;
         try {
+            byte[] in = Base64.decode(data.toCharArray());
+            Secret s = tryDecrypt(KEY.decrypt(), in);
+            if (s!=null)    return s;
+
+            // try our historical key for backward compatibility
             Cipher cipher = getCipher("AES");
-            cipher.init(Cipher.DECRYPT_MODE, getKey());
-            String plainText = new String(cipher.doFinal(Base64.decode(data.toCharArray())), "UTF-8");
-            if(plainText.endsWith(MAGIC))
-                return new Secret(plainText.substring(0,plainText.length()-MAGIC.length()));
-            return null;
+            cipher.init(Cipher.DECRYPT_MODE, getLegacyKey());
+            return tryDecrypt(cipher, in);
         } catch (GeneralSecurityException e) {
             return null;
         } catch (UnsupportedEncodingException e) {
@@ -144,6 +152,17 @@ public final class Secret implements Serializable {
         }
     }
 
+    private static Secret tryDecrypt(Cipher cipher, byte[] in) throws UnsupportedEncodingException {
+        try {
+            String plainText = new String(cipher.doFinal(in), "UTF-8");
+            if(plainText.endsWith(MAGIC))
+                return new Secret(plainText.substring(0,plainText.length()-MAGIC.length()));
+            return null;
+        } catch (GeneralSecurityException e) {
+            return null;
+        }
+    }
+
     /**
      * Workaround for JENKINS-6459 / http://java.net/jira/browse/GLASSFISH-11862
      * This method uses specific provider selected via hudson.util.Secret.provider system property
@@ -207,10 +226,15 @@ public final class Secret implements Serializable {
     private static final String PROVIDER = System.getProperty(Secret.class.getName()+".provider");
 
     /**
-     * For testing only. Override the secret key so that we can test this class without {@link jenkins.model.Jenkins}.
+     * For testing only. Override the secret key so that we can test this class without {@link Jenkins}.
      */
     /*package*/ static String SECRET = null;
 
+    /**
+     * The key that encrypts the data on disk.
+     */
+    private static final CryptoConfidentialKey KEY = new CryptoConfidentialKey(Secret.class.getName());
+
     private static final long serialVersionUID = 1L;
 
     static {

core/src/main/java/jenkins/model/Jenkins.java

@@ -196,6 +196,8 @@ import jenkins.ExtensionComponentSet;
 import jenkins.ExtensionRefreshException;
 import jenkins.InitReactorRunner;
 import jenkins.model.ProjectNamingStrategy.DefaultProjectNamingStrategy;
+import jenkins.security.ConfidentialKey;
+import jenkins.security.ConfidentialStore;
 import net.sf.json.JSONObject;
 import org.acegisecurity.AccessDeniedException;
 import org.acegisecurity.AcegiSecurityException;
@@ -994,6 +996,10 @@ public class Jenkins extends AbstractCIBase implements ModifiableTopLevelItemGro
      * Returns a secret key that survives across container start/stop.
      * <p>
      * This value is useful for implementing some of the security features.
+     *
+     * @deprecated
+     *      Due to the past security advisory, this value should not be used any more to protect sensitive information.
+     *      See {@link ConfidentialStore} and {@link ConfidentialKey} for how to store secrets.
      */
     public String getSecretKey() {
         return secretKey;
@@ -1002,11 +1008,29 @@ public class Jenkins extends AbstractCIBase implements ModifiableTopLevelItemGro
     /**
      * Gets {@linkplain #getSecretKey() the secret key} as a key for AES-128.
      * @since 1.308
+     * @deprecated
+     *       See {@link #getSecretKey()}.
      */
     public SecretKey getSecretKeyAsAES128() {
         return Util.toAes128Key(secretKey);
     }
 
+    /**
+     * Returns the unique identifier of this Jenkins that has been historically used to identify
+     * this Jenkins to the outside world.
+     *
+     * <p>
+     * This form of identifier is weak in that it can be impersonated by others. See
+     * https://wiki.jenkins-ci.org/display/JENKINS/Instance+Identity for more modern form of instance ID
+     * that can be challenged and verified.
+     *
+     * @since 1.498
+     */
+    @SuppressWarnings("deprecation")
+    public String getLegacyInstanceId() {
+        return Util.getDigestOf(getSecretKey());
+    }
+
     /**
      * Gets the SCM descriptor by name. Primarily used for making them web-visible.
      */

core/src/main/java/jenkins/security/ApiTokenProperty.java

@@ -31,7 +31,6 @@ import hudson.model.UserProperty;
 import hudson.model.UserPropertyDescriptor;
 import hudson.util.HttpResponses;
 import hudson.util.Secret;
-import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
 import org.kohsuke.stapler.AncestorInPath;
 import org.kohsuke.stapler.DataBoundConstructor;
@@ -103,10 +102,10 @@ public class ApiTokenProperty extends UserProperty {
          * because there's no guarantee that the property is saved.
          *
          * But we also need to make sure that an attacker won't be able to guess
-         * the initial API token value. So we take the seed by hasing the instance secret key + user ID.
+         * the initial API token value. So we take the seed by hashing the secret + user ID.
          */
         public ApiTokenProperty newInstance(User user) {
-            return new ApiTokenProperty(Util.getDigestOf(Jenkins.getInstance().getSecretKey() + ":" + user.getId()));
+            return new ApiTokenProperty(API_KEY_SEED.mac(user.getId()));
         }
 
         public HttpResponse doChangeToken(@AncestorInPath User u, StaplerResponse rsp) throws IOException {
@@ -123,4 +122,9 @@ public class ApiTokenProperty extends UserProperty {
     }
 
     private static final SecureRandom RANDOM = new SecureRandom();
+
+    /**
+     * We don't want an API key that's too long, so cut the length to 16 (which produces 32-letter MAC code in hexdump)
+     */
+    private static final HMACConfidentialKey API_KEY_SEED = new HMACConfidentialKey(ApiTokenProperty.class,"seed",16);
 }

core/src/main/java/jenkins/security/ConfidentialKey.java

+package jenkins.security;
+
+import hudson.scm.SCM;
+import hudson.tasks.Builder;
+import hudson.util.Secret;
+import jenkins.slaves.JnlpSlaveAgentProtocol;
+
+import javax.annotation.CheckForNull;
+import java.io.IOException;
+
+/**
+ * Confidential information that gets stored as a singleton in Jenkins, mostly some random token value.
+ *
+ * <p>
+ * The actual value is persisted via {@link ConfidentialStore}, but each use case that requires
+ * a secret like this should use a separate {@link ConfidentialKey} instance so that one compromised
+ * {@link ConfidentialKey} (say through incorrect usage and failure to protect it) shouldn't compromise
+ * all the others.
+ *
+ * <p>
+ * {@link ConfidentialKey} is ultimately a sequence of bytes,
+ * but for convenience, a family of subclasses are provided to represent the secret in different formats.
+ * See {@link HexStringConfidentialKey} and {@link HMACConfidentialKey} for example. In addition to the programming
+ * ease, these use case specific subtypes make it harder for vulnerability to creep in by making it harder
+ * for the secret to leak.
+ *
+ * <p>
+ * The {@link ConfidentialKey} subtypes are expected to be used as a singleton, like {@link JnlpSlaveAgentProtocol#SLAVE_SECRET}.
+ * For code that relies on XStream for persistence (such as {@link Builder}s, {@link SCM}s, and other fragment objects
+ * around builds and jobs), {@link Secret} provides more convenient way of storing secrets.
+ *
+ * @author Kohsuke Kawaguchi
+ * @see Secret
+ * @since 1.498
+ */
+public abstract class ConfidentialKey {
+    /**
+     * Name of the key. This is used as the file name.
+     */
+    private final String id;
+
+    protected ConfidentialKey(String id) {
+        this.id = id;
+    }
+
+    protected @CheckForNull byte[] load() throws IOException {
+        return ConfidentialStore.get().load(this);
+    }
+
+    protected void store(byte[] payload) throws IOException {
+        ConfidentialStore.get().store(this,payload);
+    }
+
+    public String getId() {
+        return id;
+    }
+}

core/src/main/java/jenkins/security/ConfidentialStore.java

+package jenkins.security;
+
+import hudson.Extension;
+import jenkins.model.Jenkins;
+
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnull;
+import java.io.IOException;
+import java.security.SecureRandom;
+
+/**
+ * The actual storage for the data held by {@link ConfidentialKey}s, and the holder
+ * of the master secret.
+ *
+ * <p>
+ * This class is only relevant for the implementers of {@link ConfidentialKey}s.
+ * Most plugin code should interact with {@link ConfidentialKey}s.
+ *
+ * <p>
+ * OEM distributions of Jenkins can provide a custom {@link ConfidentialStore} implementation
+ * by writing a subclass, mark it with {@link Extension} annotation, package it as a Jenkins module,
+ * and bundling it with the war file.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.498
+ */
+public abstract class ConfidentialStore {
+    /**
+     * Persists the payload of {@link ConfidentialKey} to a persisted storage (such as disk.)
+     * The expectation is that the persisted form is secure.
+     */
+    protected abstract void store(ConfidentialKey key, byte[] payload) throws IOException;
+
+    /**
+     * Reverse operation of {@link #store(ConfidentialKey, byte[])}
+     *
+     * @return
+     *      null the data has not been previously persisted, or if the data was tampered.
+     */
+    protected abstract @CheckForNull byte[] load(ConfidentialKey key) throws IOException;
+
+    /**
+     * Works like {@link SecureRandom#nextBytes(byte[])}.
+     *
+     * This enables implementations to consult other entropy sources, if it's available.
+     */
+    public abstract byte[] randomBytes(int size);
+
+    /**
+     * Retrieves the currently active singleton instance of {@link ConfidentialStore}.
+     */
+    public static @Nonnull ConfidentialStore get() {
+        if (TEST!=null) return TEST.get();
+        return Jenkins.getInstance().getExtensionList(ConfidentialStore.class).get(0);
+    }
+
+    /**
+     * Testing only. Used for testing {@link ConfidentialKey} without {@link Jenkins}
+     */
+    /*package*/ static ThreadLocal<ConfidentialStore> TEST = null;
+}

core/src/main/java/jenkins/security/CryptoConfidentialKey.java

+package jenkins.security;
+
+import hudson.util.Secret;
+
+import javax.crypto.Cipher;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+
+/**
+ * {@link ConfidentialKey} that stores a {@link SecretKey} for shared-secret cryptography (AES).
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.498
+ */
+public class CryptoConfidentialKey extends ConfidentialKey {
+    private volatile SecretKey secret;
+    public CryptoConfidentialKey(String id) {
+        super(id);
+    }
+
+    public CryptoConfidentialKey(Class owner, String shortName) {
+        this(owner.getName()+'.'+shortName);
+    }
+
+    private SecretKey getKey() {
+        try {
+            if (secret==null) {
+                synchronized (this) {
+                    if (secret==null) {
+                        byte[] payload = load();
+                        if (payload==null) {
+                            payload = ConfidentialStore.get().randomBytes(256);
+                            store(payload);
+                        }
+                        // Due to the stupid US export restriction JDK only ships 128bit version.
+                        secret = new SecretKeySpec(payload,0,128/8, ALGORITHM);
+                    }
+                }
+            }
+            return secret;
+        } catch (IOException e) {
+            throw new Error("Failed to load the key: "+getId(),e);
+        }
+    }
+
+    /**
+     * Returns a {@link Cipher} object for encrypting with this key.
+     */
+    public Cipher encrypt() {
+        try {
+            Cipher cipher = Secret.getCipher(ALGORITHM);
+            cipher.init(Cipher.ENCRYPT_MODE, getKey());
+            return cipher;
+        } catch (GeneralSecurityException e) {
+            throw new AssertionError(e);
+        }
+    }
+
+    /**
+     * Returns a {@link Cipher} object for decrypting with this key.
+     */
+    public Cipher decrypt() {
+        try {
+            Cipher cipher = Secret.getCipher(ALGORITHM);
+            cipher.init(Cipher.DECRYPT_MODE, getKey());
+            return cipher;
+        } catch (GeneralSecurityException e) {
+            throw new AssertionError(e);
+        }
+    }
+
+
+    private static final String ALGORITHM = "AES";
+}

core/src/main/java/jenkins/security/DefaultConfidentialStore.java

+package jenkins.security;
+
+import hudson.Extension;
+import hudson.FilePath;
+import hudson.Util;
+import hudson.util.IOException2;
+import hudson.util.IOUtils;
+import hudson.util.Secret;
+import hudson.util.TextFile;
+import jenkins.model.Jenkins;
+
+import javax.crypto.Cipher;
+import javax.crypto.CipherInputStream;
+import javax.crypto.CipherOutputStream;
+import javax.crypto.SecretKey;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+import java.security.SecureRandom;
+
+/**
+ * Default portable implementation of {@link ConfidentialStore} that uses
+ * a directory inside $JENKINS_HOME.
+ *
+ * The master key is also stored in this same directory.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+@Extension(ordinal=-99999) // small ordinal value to allow other higher ones to take over
+public class DefaultConfidentialStore extends ConfidentialStore {
+    private final SecureRandom sr = new SecureRandom();
+
+    /**
+     * Directory that stores individual keys.
+     */
+    private final File rootDir;
+
+    /**
+     * The master key.
+     *
+     * The sole purpose of the master key is to encrypt individual keys on the disk.
+     * Because leaking this master key compromises all the individual keys, we must not let
+     * this master key used for any other purpose, hence the protected access.
+     */
+    private final SecretKey masterKey;
+
+    public DefaultConfidentialStore() throws IOException, InterruptedException {
+        this(new File(Jenkins.getInstance().getRootDir(),"secrets"));
+    }
+
+    public DefaultConfidentialStore(File rootDir) throws IOException, InterruptedException {
+        this.rootDir = rootDir;
+        if (rootDir.mkdirs()) {
+            // protect this directory. but don't change the permission of the existing directory
+            // in case the administrator changed this.
+            new FilePath(rootDir).chmod(0700);
+        }
+
+        TextFile masterSecret = new TextFile(new File(rootDir,"master.key"));
+        if (!masterSecret.exists()) {
+            // we are only going to use small number of bits (since export control limits AES key length)
+            // but let's generate a long enough key anyway
+            masterSecret.write(Util.toHexString(randomBytes(128)));
+        }
+        this.masterKey = Util.toAes128Key(masterSecret.readTrim());
+    }
+
+    /**
+     * Persists the payload of {@link ConfidentialKey} to the disk.
+     */
+    @Override
+    protected void store(ConfidentialKey key, byte[] payload) throws IOException {
+        CipherOutputStream cos=null;
+        FileOutputStream fos=null;
+        try {
+            Cipher sym = Secret.getCipher("AES");
+            sym.init(Cipher.ENCRYPT_MODE, masterKey);
+            cos = new CipherOutputStream(fos=new FileOutputStream(getFileFor(key)), sym);
+            cos.write(payload);
+            cos.write(MAGIC);
+        } catch (GeneralSecurityException e) {
+            throw new IOException2("Failed to persist the key: "+key.getId(),e);
+        } finally {
+            IOUtils.closeQuietly(cos);
+            IOUtils.closeQuietly(fos);
+        }
+    }
+
+    /**
+     * Reverse operation of {@link #store(ConfidentialKey, byte[])}
+     *
+     * @return
+     *      null the data has not been previously persisted.
+     */
+    @Override
+    protected byte[] load(ConfidentialKey key) throws IOException {
+        CipherInputStream cis=null;
+        FileInputStream fis=null;
+        try {
+            File f = getFileFor(key);
+            if (!f.exists())    return null;
+
+            Cipher sym = Secret.getCipher("AES");
+            sym.init(Cipher.DECRYPT_MODE, masterKey);
+            cis = new CipherInputStream(fis=new FileInputStream(f), sym);
+            byte[] bytes = IOUtils.toByteArray(cis);
+            return verifyMagic(bytes);
+        } catch (GeneralSecurityException e) {
+            throw new IOException2("Failed to persist the key: "+key.getId(),e);
+        } finally {
+            IOUtils.closeQuietly(cis);
+            IOUtils.closeQuietly(fis);
+        }
+    }
+
+    /**
+     * Verifies that the given byte[] has the MAGIC trailer, to verify the integrity of the decryption process.
+     */
+    private byte[] verifyMagic(byte[] payload) {
+        int payloadLen = payload.length-MAGIC.length;
+        if (payloadLen<0)   return null;    // obviously broken
+
+        for (int i=0; i<MAGIC.length; i++) {
+            if (payload[payloadLen+i]!=MAGIC[i])
+                return null;    // broken
+        }
+        byte[] truncated = new byte[payloadLen];
+        System.arraycopy(payload,0,truncated,0,truncated.length);
+        return truncated;
+    }
+
+    private File getFileFor(ConfidentialKey key) {
+        return new File(rootDir, key.getId());
+    }
+
+    public byte[] randomBytes(int size) {
+        byte[] random = new byte[size];
+        sr.nextBytes(random);
+        return random;
+    }
+
+    private static final byte[] MAGIC = "::::MAGIC::::".getBytes();
+}

core/src/main/java/jenkins/security/HMACConfidentialKey.java

+package jenkins.security;
+
+import hudson.Util;
+
+import javax.crypto.KeyGenerator;
+import javax.crypto.Mac;
+import javax.crypto.SecretKey;
+import javax.crypto.spec.SecretKeySpec;
+import java.io.IOException;
+import java.io.UnsupportedEncodingException;
+import java.security.GeneralSecurityException;
+import java.security.NoSuchAlgorithmException;
+import java.util.Arrays;
+
+/**
+ * {@link ConfidentialKey} that's used for creating a token by hashing some information with secret
+ * (such as <tt>hash(msg|secret)</tt>).
+ *
+ * <p>
+ * This provides more secure version of it by using HMAC.
+ * See http://rdist.root.org/2009/10/29/stop-using-unsafe-keyed-hashes-use-hmac/ for background.
+ * This implementation also never leaks the secret value to outside, so it makes it impossible
+ * for the careless caller to misuse the key (thus protecting ourselves from our own stupidity!)
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.498
+ */
+public class HMACConfidentialKey extends ConfidentialKey {
+    private volatile SecretKey key;
+    private final int length;
+
+    /**
+     * @param length
+     *      Byte length of the HMAC code.
+     *      By default we use HMAC-SHA256, which produces 256bit (=32bytes) HMAC,
+     *      but if different use cases requires a shorter HMAC, specify the desired length here.
+     *      Note that when using {@link #mac(String)}, string encoding causes the length to double.
+     *      So if you want to get 16-letter HMAC, you specify 8 here.
+     */
+    public HMACConfidentialKey(String id, int length) {
+        super(id);
+        this.length = length;
+    }
+
+    /**
+     * Calls into {@link #HMACConfidentialKey(String, int)} with the longest possible HMAC length.
+     */
+    public HMACConfidentialKey(String id) {
+        this(id,Integer.MAX_VALUE);
+    }
+
+    /**
+     * Calls into {@link #HMACConfidentialKey(String, int)} by combining the class name and the shortName
+     * as the ID.
+     */
+    public HMACConfidentialKey(Class owner, String shortName, int length) {
+        this(owner.getName()+'.'+shortName,length);
+    }
+
+    public HMACConfidentialKey(Class owner, String shortName) {
+        this(owner,shortName,Integer.MAX_VALUE);
+    }
+
+
+    /**
+     * Computes the message authentication code for the specified byte sequence.
+     */
+    public byte[] mac(byte[] message) {
+        return chop(createMac().doFinal(message));
+    }
+
+    /**
+     * Convenience method for verifying the MAC code.
+     */
+    public boolean checkMac(byte[] message, byte[] mac) {
+        return Arrays.equals(mac(message),mac);
+    }
+
+    /**
+     * Computes the message authentication code and return it as a string.
+     * While redundant, often convenient.
+     */
+    public String mac(String message) {
+        try {
+            return Util.toHexString(mac(message.getBytes("UTF-8")));
+        } catch (UnsupportedEncodingException e) {
+            throw new AssertionError(e);
+        }
+    }
+
+    /**
+     * Verifies MAC constructed from {@link #mac(String)}
+     */
+    public boolean checkMac(String message, String mac) {
+        return mac(message).equals(mac);
+    }
+
+    private byte[] chop(byte[] mac) {
+        if (mac.length<=length)  return mac; // already too short
+
+        byte[] b = new byte[length];
+        System.arraycopy(mac,0,b,0,b.length);
+        return b;
+    }
+
+    /**
+     * Creates a new {@link Mac} object.
+     */
+    public Mac createMac() {
+        try {
+            Mac mac = Mac.getInstance(ALGORITHM);
+            mac.init(getKey());
+            return mac;
+        } catch (GeneralSecurityException e) {
+            // Javadoc says HmacSHA256 must be supported by every Java implementation.
+            throw new Error(ALGORITHM+" not supported?",e);
+        }
+    }
+
+    private SecretKey getKey() {
+        if (key==null) {
+            synchronized (this) {
+                if (key==null) {
+                    try {
+                        byte[] encoded = load();
+                        if (encoded==null) {
+                            KeyGenerator kg = KeyGenerator.getInstance(ALGORITHM);
+                            SecretKey key = kg.generateKey();
+                            store(encoded=key.getEncoded());
+                        }
+                        key = new SecretKeySpec(encoded,ALGORITHM);
+                    } catch (IOException e) {
+                        throw new Error("Failed to load the key: "+getId(),e);
+                    } catch (NoSuchAlgorithmException e) {
+                        throw new Error("Failed to load the key: "+getId(),e);
+                    }
+                }
+            }
+        }
+        return key;
+    }
+
+    private static final String ALGORITHM = "HmacSHA256";
+}

core/src/main/java/jenkins/security/HexStringConfidentialKey.java

+package jenkins.security;
+
+import hudson.Util;
+
+import java.io.IOException;
+
+/**
+ * {@link ConfidentialKey} that is the random hexadecimal string of length N.
+ *
+ * <p>
+ * This is typically used as a unique ID, as a hex dump is suitable for printing, copy-pasting,
+ * as well as use as an identifier.
+ *
+ * @author Kohsuke Kawaguchi
+ * @since 1.498
+ */
+public class HexStringConfidentialKey extends ConfidentialKey {
+    private final int length;
+
+    private volatile String secret;
+
+    /**
+     * @param length
+     *      Length of the hexadecimal string.
+     */
+    public HexStringConfidentialKey(String id, int length) {
+        super(id);
+        if (length%2!=0)
+            throw new IllegalArgumentException("length must be even: "+length);
+        this.length = length;
+    }
+
+    public HexStringConfidentialKey(Class owner, String shortName, int length) {
+        this(owner.getName()+'.'+shortName,length);
+    }
+
+    /**
+     * Returns the persisted hex string value.
+     *
+     * If the value isn't persisted, a new random value is created.
+     *
+     * @throws Error
+     *      If the secret fails to load. Not throwing a checked exception is for the convenience
+     *      of the caller.
+     */
+    public String get() {
+        try {
+            if (secret==null) {
+                synchronized (this) {
+                    if (secret==null) {
+                        byte[] payload = load();
+                        if (payload==null) {
+                            payload = ConfidentialStore.get().randomBytes(length/2);
+                            store(payload);
+                        }
+                        secret = Util.toHexString(payload).substring(0,length);
+                    }
+                }
+            }
+            return secret;
+        } catch (IOException e) {
+            throw new Error("Failed to load the key: "+getId(),e);
+        }
+    }
+}

core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol.java

@@ -8,6 +8,7 @@ import hudson.remoting.Engine;
 import hudson.slaves.SlaveComputer;
 import jenkins.AgentProtocol;
 import jenkins.model.Jenkins;
+import jenkins.security.HMACConfidentialKey;
 
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
@@ -31,10 +32,10 @@ import java.util.logging.Logger;
  * unauthorized remote slaves.
  *
  * <p>
- * The approach here is to have {@link Jenkins#getSecretKey() a secret key} on the master.
- * This key is sent to the slave inside the <tt>.jnlp</tt> file
+ * We do this by computing HMAC of the slave name.
+ * This code is sent to the slave inside the <tt>.jnlp</tt> file
  * (this file itself is protected by HTTP form-based authentication that
- * we use everywhere else in Hudson), and the slave sends this
+ * we use everywhere else in Jenkins), and the slave sends this
  * token back when it connects to the master.
  * Unauthorized slaves can't access the protected <tt>.jnlp</tt> file,
  * so it can't impersonate a valid slave.
@@ -84,12 +85,15 @@ public class JnlpSlaveAgentProtocol extends AgentProtocol {
         }
 
         protected void run() throws IOException, InterruptedException {
-            if(!getSecretKey().equals(in.readUTF())) {
+            final String secret = in.readUTF();
+            final String nodeName = in.readUTF();
+
+            if(!SLAVE_SECRET.mac(nodeName).equals(secret)) {
                 error(out, "Unauthorized access");
                 return;
             }
 
-            final String nodeName = in.readUTF();
+
             SlaveComputer computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);
             if(computer==null) {
                 error(out, "No such slave: "+nodeName);
@@ -138,10 +142,6 @@ public class JnlpSlaveAgentProtocol extends AgentProtocol {
             }
         }
 
-        protected String getSecretKey() {
-            return Jenkins.getInstance().getSecretKey();
-        }
-
         protected void error(PrintWriter out, String msg) throws IOException {
             out.println(msg);
             LOGGER.log(Level.WARNING,Thread.currentThread().getName()+" is aborted: "+msg);
@@ -150,4 +150,9 @@ public class JnlpSlaveAgentProtocol extends AgentProtocol {
     }
 
     private static final Logger LOGGER = Logger.getLogger(JnlpSlaveAgentProtocol.class.getName());
+
+    /**
+     * This secret value is used as a seed for slaves.
+     */
+    public static final HMACConfidentialKey SLAVE_SECRET = new HMACConfidentialKey(JnlpSlaveAgentProtocol.class,"secret");
 }

core/src/main/java/jenkins/slaves/JnlpSlaveAgentProtocol2.java

@@ -57,12 +57,13 @@ public class JnlpSlaveAgentProtocol2 extends JnlpSlaveAgentProtocol {
             Properties request = new Properties();
             request.load(new ByteArrayInputStream(in.readUTF().getBytes("UTF-8")));
 
-            if(!getSecretKey().equals(request.getProperty("Secret-Key"))) {
+            final String nodeName = request.getProperty("Node-Name");
+
+            if(!SLAVE_SECRET.mac(nodeName).equals(request.getProperty("Secret-Key"))) {
                 error(out, "Unauthorized access");
                 return;
             }
 
-            final String nodeName = request.getProperty("Node-Name");
             SlaveComputer computer = (SlaveComputer) Jenkins.getInstance().getComputer(nodeName);
             if(computer==null) {
                 error(out, "No such slave: "+nodeName);

core/src/main/resources/hudson/slaves/SlaveComputer/slave-agent.jnlp.jelly

@@ -31,7 +31,9 @@ THE SOFTWARE.
   <!--
     See http://www.dallaway.com/acad/webstart/ for obtaining the certificate.
   -->
-  <l:isAdminOrTest test="true">
+  <j:getStatic var="connect" className="hudson.model.Computer" field="CONNECT"/>
+  ${it.checkPermission(connect)}
+
     <!-- See http://java.sun.com/j2se/1.5.0/docs/guide/javaws/developersguide/syntax.html for the syntax -->
     <jnlp spec="1.0+"
         codebase="${rootURL}computer/${h.encode(it.node.nodeName)}/">
@@ -61,7 +63,7 @@ THE SOFTWARE.
       </resources>
 
       <application-desc main-class="hudson.remoting.jnlp.Main">
-        <argument>${app.secretKey}</argument>
+        <argument>${it.jnlpMac}</argument>
         <argument>${it.node.nodeName}</argument>
         <j:if test="${it.launcher.tunnel!=null}">
           <argument>-tunnel</argument>
@@ -87,5 +89,4 @@ THE SOFTWARE.
         </j:if>
       </application-desc>
     </jnlp>
-  </l:isAdminOrTest>
 </j:jelly>

core/src/test/java/hudson/util/SecretTest.java → core/src/test/java/hudson/util/SecretTest.groovy

@@ -21,11 +21,17 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
-package hudson.util;
+package hudson.util
 
-import jenkins.model.Jenkins;
-import junit.framework.TestCase;
+import com.trilead.ssh2.crypto.Base64;
+import jenkins.model.Jenkins
+import jenkins.security.ConfidentialStoreRule;
+import org.junit.After
+import org.junit.Before
+import org.junit.Rule
+import org.junit.Test
 
+import javax.crypto.Cipher;
 import java.security.SecureRandom;
 
 import hudson.Util;
@@ -33,42 +39,50 @@ import hudson.Util;
 /**
  * @author Kohsuke Kawaguchi
  */
-public class SecretTest extends TestCase {
-    @Override protected void setUp() throws Exception {
-        SecureRandom sr = new SecureRandom();
+public class SecretTest {
+    @Rule
+    public ConfidentialStoreRule confidentialStore = new ConfidentialStoreRule()
+
+    @Test @Before
+    void setUp() {
+        def sr = new SecureRandom();
         byte[] random = new byte[32];
         sr.nextBytes(random);
         Secret.SECRET = Util.toHexString(random);
-
     }
 
-    @Override protected void tearDown() throws Exception {
+    @Test @After
+    void tearDown() {
         Secret.SECRET = null;
     }
 
-    public void testEncrypt() {
-        Secret secret = Secret.fromString("abc");
-        assertEquals("abc",secret.getPlainText());
+    @Test
+    void testEncrypt() {
+        def secret = Secret.fromString("abc");
+        assert "abc"==secret.plainText;
 
         // make sure we got some encryption going
-        System.out.println(secret.getEncryptedValue());
-        assertTrue(!"abc".equals(secret.getEncryptedValue()));
+        println secret.encryptedValue;
+        assert !"abc".equals(secret.encryptedValue);
 
         // can we round trip?
-        assertEquals(secret,Secret.fromString(secret.getEncryptedValue()));
+        assert secret==Secret.fromString(secret.encryptedValue);
     }
 
-    public void testDecrypt() {
-        assertEquals("abc",Secret.toString(Secret.fromString("abc")));
+    @Test
+    void testDecrypt() {
+        assert "abc"==Secret.toString(Secret.fromString("abc"))
     }
 
-    public void testSerialization() {
-        Secret s = Secret.fromString("Mr.Hudson");
-        String xml = Jenkins.XSTREAM.toXML(s);
-        assertTrue(xml, !xml.contains(s.getPlainText()));
-        assertTrue(xml, xml.contains(s.getEncryptedValue()));
-        Object o = Jenkins.XSTREAM.fromXML(xml);
-        assertEquals(xml, o, s);
+    @Test
+    void testSerialization() {
+        def s = Secret.fromString("Mr.Jenkins");
+        def xml = Jenkins.XSTREAM.toXML(s);
+        assert !xml.contains(s.plainText)
+        assert xml.contains(s.encryptedValue)
+
+        def o = Jenkins.XSTREAM.fromXML(xml);
+        assert o==s : xml;
     }
 
     public static class Foo {
@@ -78,11 +92,28 @@ public class SecretTest extends TestCase {
     /**
      * Makes sure the serialization form is backward compatible with String.
      */
-    public void testCompatibilityFromString() {
-        String tagName = Foo.class.getName().replace("$","_-");
-        String xml = "<"+tagName+"><password>secret</password></"+tagName+">";
-        Foo foo = new Foo();
+    @Test
+    void testCompatibilityFromString() {
+        def tagName = Foo.class.name.replace("\$","_-");
+        def xml = "<$tagName><password>secret</password></$tagName>";
+        def foo = new Foo();
         Jenkins.XSTREAM.fromXML(xml, foo);
-        assertEquals("secret",Secret.toString(foo.password));
+        assert "secret"==Secret.toString(foo.password)
+    }
+
+    /**
+     * Secret persisted with Jenkins.getSecretKey() should still decrypt OK.
+     */
+    @Test
+    void migrationFromLegacyKeyToConfidentialStore() {
+        def legacy = Secret.legacyKey
+        ["Hello world","","\u0000unprintable"].each { str ->
+            def cipher = Secret.getCipher("AES");
+            cipher.init(Cipher.ENCRYPT_MODE, legacy);
+            def old = new String(Base64.encode(cipher.doFinal((str + Secret.MAGIC).getBytes("UTF-8"))))
+            def s = Secret.fromString(old)
+            assert s.plainText==str : "secret by the old key should decrypt"
+            assert s.encryptedValue!=old : "but when encrypting, ConfidentialKey should be in use"
+        }
     }
 }

core/src/test/java/jenkins/security/ConfidentialStoreRule.java

+package jenkins.security;
+
+import hudson.Util;
+import org.junit.rules.ExternalResource;
+
+import java.io.File;
+import java.io.IOException;
+
+/**
+ * Test rule that injects a temporary {@link DefaultConfidentialStore}
+ * @author Kohsuke Kawaguchi
+ */
+public class ConfidentialStoreRule extends ExternalResource {
+    public ConfidentialStore store;
+    public File tmp;
+
+    @Override
+    protected void before() throws Throwable {
+        tmp = Util.createTempDir();
+        store = new DefaultConfidentialStore(tmp);
+        ConfidentialStore.TEST.set(store);
+    }
+
+    @Override
+    protected void after() {
+        ConfidentialStore.TEST.set(null);
+        try {
+            Util.deleteRecursive(tmp);
+        } catch (IOException e) {
+            throw new Error(e);
+        }
+    }
+
+    static {
+        ConfidentialStore.TEST = new ThreadLocal<ConfidentialStore>();
+    }
+}

core/src/test/java/jenkins/security/CryptoConfidentialKeyTest.groovy

+package jenkins.security
+
+import org.junit.Rule
+import org.junit.Test
+
+/**
+ *
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class CryptoConfidentialKeyTest {
+    @Rule
+    public ConfidentialStoreRule store = new ConfidentialStoreRule()
+
+    def key = new CryptoConfidentialKey("test")
+
+    @Test
+    void decryptGetsPlainTextBack() {
+        ["Hello world","","\u0000"].each { str ->
+            assert key.decrypt().doFinal(key.encrypt().doFinal(str.bytes))==str.bytes
+        }
+    }
+
+    @Test
+    void multipleEncryptsAreIdempotent() {
+        def str = "Hello world".bytes
+        assert key.encrypt().doFinal(str)==key.encrypt().doFinal(str)
+    }
+
+    @Test
+    void loadingExistingKey() {
+        def key2 = new CryptoConfidentialKey("test") // this will cause the key to be loaded from the disk
+        ["Hello world","","\u0000"].each { str ->
+            assert key2.decrypt().doFinal(key.encrypt().doFinal(str.bytes))==str.bytes
+        }
+    }
+}

core/src/test/java/jenkins/security/DefaultConfidentialStoreTest.groovy

+package jenkins.security
+
+import hudson.FilePath
+import hudson.Functions
+import hudson.Util
+import org.junit.After
+import org.junit.Before
+import org.junit.Test
+
+/**
+ * @author Kohsuke Kawaguchi
+ */
+public class DefaultConfidentialStoreTest {
+
+    def tmp;
+
+    @Before
+    void setup() {
+        tmp = Util.createTempDir()
+    }
+
+    @After
+    void tearDown() {
+        Util.deleteRecursive(tmp)
+    }
+
+    @Test
+    void roundtrip() {
+        tmp.deleteDir()   // let ConfidentialStore create a directory
+
+        def store = new DefaultConfidentialStore(tmp);
+        def key = new ConfidentialKey("test") {};
+
+        // basic roundtrip
+        def str = "Hello world!"
+        store.store(key, str.bytes)
+        assert new String(store.load(key))==str
+
+        // data storage should have some stuff
+        assert new File(tmp,"test").exists()
+        assert new File(tmp,"master.key").exists()
+
+        assert !new File(tmp,"test").text.contains("Hello") // the data shouldn't be a plain text, obviously
+
+        if (!Functions.isWindows())
+            assert (new FilePath(tmp).mode()&0777) == 0700 // should be read only
+
+        // if the master key changes, we should gracefully fail to load the store
+        new File(tmp,"master.key").delete()
+        def store2 = new DefaultConfidentialStore(tmp)
+        assert new File(tmp,"master.key").exists()  // we should have a new key now
+        assert store2.load(key)==null;
+    }
+}

core/src/test/java/jenkins/security/HMACConfidentialKeyTest.groovy

+package jenkins.security
+
+import org.junit.Rule
+import org.junit.Test
+
+/**
+ *
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class HMACConfidentialKeyTest {
+    @Rule
+    public ConfidentialStoreRule store = new ConfidentialStoreRule()
+
+    def key = new HMACConfidentialKey("test",16)
+
+    @Test
+    void basics() {
+        def unique = [] as TreeSet
+        ["Hello world","","\u0000"].each { str ->
+            def mac = key.mac(str)
+            unique.add(mac)
+            assert mac =~ /[0-9A-Fa-f]{32}/
+            assert key.checkMac(str,mac)
+            assert !key.checkMac("garbage",mac)
+        }
+
+        assert unique.size()==3 // make sure all 3 MAC are different
+    }
+
+    @Test
+    void loadingExistingKey() {
+        // this second key of the same ID will cause it to load the key from the disk
+        def key2 = new HMACConfidentialKey("test",16)
+        ["Hello world","","\u0000"].each { str ->
+            assert key.mac(str)==key2.mac(str)
+        }
+    }
+}

core/src/test/java/jenkins/security/HexStringConfidentialKeyTest.groovy

+package jenkins.security
+
+import org.junit.Rule
+import org.junit.Test
+
+/**
+ *
+ *
+ * @author Kohsuke Kawaguchi
+ */
+class HexStringConfidentialKeyTest {
+    @Rule
+    public ConfidentialStoreRule store = new ConfidentialStoreRule()
+
+    @Test
+    void hexStringShouldProduceHexString() {
+        def key = new HexStringConfidentialKey("test",8)
+        assert key.get() =~ /[A-Fa-f0-9]{8}/
+    }
+
+    @Test
+    void multipleGetsAreIdempotent() {
+        def key = new HexStringConfidentialKey("test",8)
+        assert key.get()==key.get()
+    }
+
+    @Test
+    void specifyLengthAndMakeSureItTakesEffect() {
+        [8,16,32,256].each { n ->
+            new HexStringConfidentialKey("test"+n,n).get().length()==n
+        }
+    }
+
+    @Test
+    void loadingExistingKey() {
+        def key1 = new HexStringConfidentialKey("test",8)
+        key1.get() // this causes the ke to be generated
+
+        // this second key of the same ID will cause it to load the key from the disk
+        def key2 = new HexStringConfidentialKey("test",8)
+        assert key1.get()==key2.get()
+    }
+}

pom.xml

@@ -452,7 +452,7 @@ THE SOFTWARE.
           <!-- see http://maven-junit-plugin.kenai.com/ for more info -->
           <groupId>org.kohsuke</groupId>
           <artifactId>maven-junit-plugin</artifactId>
-          <version>1.10</version>
+          <version>1.11</version>
          </plugin>
         <plugin>
           <groupId>org.jvnet.localizer</groupId>

test/src/test/java/hudson/bugs/JnlpAccessWithSecuredHudsonTest.java

@@ -23,6 +23,7 @@
  */
 package hudson.bugs;
 
+import com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException;
 import com.gargoylesoftware.htmlunit.Page;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import com.gargoylesoftware.htmlunit.xml.XmlPage;
@@ -39,6 +40,7 @@ import org.jvnet.hudson.test.HudsonTestCase;
 import org.jvnet.hudson.test.recipes.PresetData;
 import org.jvnet.hudson.test.recipes.PresetData.DataSet;
 
+import java.net.HttpURLConnection;
 import java.net.URL;
 import java.util.Collections;
 import java.util.List;
@@ -58,7 +60,7 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase {
         return new DumbSlave(name,"",System.getProperty("java.io.tmpdir")+'/'+name,"2", Mode.NORMAL, "", new JNLPLauncher(), RetentionStrategy.INSTANCE, Collections.EMPTY_LIST);
     }
 
-    @PresetData(DataSet.NO_ANONYMOUS_READACCESS)
+    @PresetData(DataSet.ANONYMOUS_READONLY)
     @Email("http://www.nabble.com/Launching-slave-by-JNLP-with-Active-Directory-plugin-and-matrix-security-problem-td18980323.html")
     public void test() throws Exception {
         jenkins.setNodes(Collections.singletonList(createNewJnlpSlave("test")));
@@ -80,5 +82,13 @@ public class JnlpAccessWithSecuredHudsonTest extends HudsonTestCase {
             Page jarResource = jnlpAgent.getPage(url);
             assertTrue(jarResource.getWebResponse().getContentType().toLowerCase(Locale.ENGLISH).startsWith("application/"));
         }
+
+
+        try {
+            jnlp = (XmlPage) jnlpAgent.goTo("computer/test/slave-agent.jnlp", "application/x-java-jnlp-file");
+            fail("anonymous users must not be able to get secrets");
+        } catch (FailingHttpStatusCodeException x) {
+            assertEquals(HttpURLConnection.HTTP_FORBIDDEN, x.getStatusCode());
+        }
     }
 }