[SECURITY-376] Remove backup directory for RekeySecretAdminMonitor.

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

@@ -2,7 +2,6 @@ package hudson.util;
 
 import com.trilead.ssh2.crypto.Base64;
 import hudson.model.TaskListener;
-import org.apache.commons.io.FileUtils;
 
 import javax.crypto.Cipher;
 import javax.crypto.SecretKey;
@@ -33,21 +32,21 @@ public class SecretRewriter {
      */
     private int count;
 
-    /**
-     * If non-null the original file before rewrite gets in here.
-     */
-    private final File backupDirectory;
-
     /**
      * Canonical paths of the directories we are recursing to protect
      * against symlink induced cycles.
      */
     private Set<String> callstack = new HashSet<String>();
 
-    public SecretRewriter(File backupDirectory) throws GeneralSecurityException {
+    public SecretRewriter() throws GeneralSecurityException {
         cipher = Secret.getCipher("AES");
         key = Secret.getLegacyKey();
-        this.backupDirectory = backupDirectory;
+    }
+
+    /** @deprecated SECURITY-376: {@code backupDirectory} is ignored */
+    @Deprecated
+    public SecretRewriter(File backupDirectory) throws GeneralSecurityException {
+        this();
     }
 
     private String tryRewrite(String s) throws IOException, InvalidKeyException {
@@ -70,12 +69,14 @@ public class SecretRewriter {
             return s;
     }
 
-    /**
-     * @param backup
-     *      if non-null, the original file will be copied here before rewriting.
-     *      if the rewrite doesn't happen, no copying.
-     */
+    /** @deprecated SECURITY-376: {@code backup} is ignored */
+    @Deprecated
     public boolean rewrite(File f, File backup) throws InvalidKeyException, IOException {
+        return rewrite(f);
+    }
+
+    public boolean rewrite(File f) throws InvalidKeyException, IOException {
+
         AtomicFileWriter w = new AtomicFileWriter(f, "UTF-8");
         try {
             PrintWriter out = new PrintWriter(new BufferedWriter(w));
@@ -117,10 +118,6 @@ public class SecretRewriter {
             }
 
             if (modified) {
-                if (backup!=null) {
-                    backup.getParentFile().mkdirs();
-                    FileUtils.copyFile(f,backup);
-                }
                 w.commit();
             }
             return modified;
@@ -165,11 +162,7 @@ public class SecretRewriter {
                     if ((count++)%100==0)
                         listener.getLogger().println("Scanning "+child);
                     try {
-                        File backup = null;
-                        if (backupDirectory!=null)  backup = new File(backupDirectory,relative+'/'+ cn);
-                        if (rewrite(child,backup)) {
-                            if (backup!=null)
-                                listener.getLogger().println("Copied "+child+" to "+backup+" as a backup");
+                        if (rewrite(child)) {
                             listener.getLogger().println("Rewritten "+child);
                             rewritten++;
                         }
@@ -199,7 +192,6 @@ public class SecretRewriter {
         String n = dir.getName();
         return n.equals("workspace") || n.equals("artifacts")
             || n.equals("plugins") // no mutable data here
-            || n.equals("jenkins.security.RekeySecretAdminMonitor") // we don't want to rewrite backups
             || n.equals(".") || n.equals("..");
     }
 

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

 package jenkins.security;
 
 import hudson.Extension;
+import hudson.Util;
 import hudson.init.InitMilestone;
 import hudson.init.Initializer;
 import hudson.model.TaskListener;
@@ -50,6 +51,7 @@ public class RekeySecretAdminMonitor extends AsynchronousAdministrativeMonitor {
      */
     private final FileBoolean scanOnBoot = state("scanOnBoot");
 
+    @SuppressWarnings("OverridableMethodCallInConstructor") // should have been final
     public RekeySecretAdminMonitor() throws IOException {
         // if JENKINS_HOME existed <1.497, we need to offer rewrite
         // this computation needs to be done and the value be captured,
@@ -59,6 +61,7 @@ public class RekeySecretAdminMonitor extends AsynchronousAdministrativeMonitor {
         if (j.isUpgradedFromBefore(new VersionNumber("1.496.*"))
         &&  new FileBoolean(new File(j.getRootDir(),"secret.key.not-so-secret")).isOff())
             needed.on();
+        Util.deleteRecursive(new File(getBaseDir(), "backups")); // SECURITY-376: no longer used
     }
 
     @Override
@@ -133,7 +136,7 @@ public class RekeySecretAdminMonitor extends AsynchronousAdministrativeMonitor {
     protected void fix(TaskListener listener) throws Exception {
         LOGGER.info("Initiating a re-keying of secrets. See "+getLogFile());
 
-        SecretRewriter rewriter = new SecretRewriter(new File(getBaseDir(),"backups"));
+        SecretRewriter rewriter = new SecretRewriter();
 
         try {
             PrintStream log = listener.getLogger();

core/src/test/groovy/hudson/util/SecretRewriterTest.groovy

@@ -70,8 +70,7 @@ class SecretRewriterTest {
      */
     @Test
     void recursionDetection() {
-        def backup = tmp.newFolder("backup")
-        def sw = new SecretRewriter(backup);
+        def sw = new SecretRewriter();
         def st = StreamTaskListener.fromStdout()
 
         def o = encryptOld("Hello world")
@@ -101,7 +100,6 @@ class SecretRewriterTest {
 
         dirs.each { p->
             assert new File(t,"$p/foo.xml").text.trim()==answer
-            assert new File(backup,"$p/foo.xml").text.trim()==payload
         }
 
         // t2 is only reachable by following a symlink. this should be covered, too