From f72bcaca0dd14312092a0bcbdd8e4792d6e56d44 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 14 Dec 2016 17:45:35 -0500 Subject: [PATCH 01/17] [SECURITY-382] Sign console notes with a MAC. --- .../main/java/hudson/console/ConsoleNote.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index c27fec6364..3e8767ad22 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -51,6 +51,7 @@ import java.util.Collection; import java.util.List; import com.jcraft.jzlib.GZIPInputStream; import com.jcraft.jzlib.GZIPOutputStream; +import jenkins.security.HMACConfidentialKey; /** * Data that hangs off from a console output. @@ -121,6 +122,9 @@ import com.jcraft.jzlib.GZIPOutputStream; * @since 1.349 */ public abstract class ConsoleNote implements Serializable, Describable>, ExtensionPoint { + + private static final HMACConfidentialKey MAC = new HMACConfidentialKey(ConsoleNote.class, "MAC"); + /** * When the line of a console output that this annotation is attached is read by someone, * a new {@link ConsoleNote} is de-serialized and this method is invoked to annotate that line. @@ -182,6 +186,9 @@ public abstract class ConsoleNote implements Serializable, Describable implements Serializable, Describable Date: Tue, 20 Dec 2016 10:11:00 -0500 Subject: [PATCH 02/17] Making tests fail more meaningfully. --- .../main/java/jenkins/security/ConfidentialStore.java | 6 ++++-- core/src/test/java/hudson/model/TaskActionTest.java | 10 ++++++++-- .../java/jenkins/security/ConfidentialStoreRule.java | 7 ++----- .../test/java/hudson/console/ConsoleAnnotatorTest.java | 4 ++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/core/src/main/java/jenkins/security/ConfidentialStore.java b/core/src/main/java/jenkins/security/ConfidentialStore.java index 9b3efdbec6..3e8e3b4b4d 100644 --- a/core/src/main/java/jenkins/security/ConfidentialStore.java +++ b/core/src/main/java/jenkins/security/ConfidentialStore.java @@ -61,7 +61,9 @@ public abstract class ConfidentialStore { * Retrieves the currently active singleton instance of {@link ConfidentialStore}. */ public static @Nonnull ConfidentialStore get() { - if (TEST!=null) return TEST.get(); + if (TEST != null) { + return TEST; + } Jenkins j = Jenkins.getInstance(); if (j == null) { @@ -95,7 +97,7 @@ public abstract class ConfidentialStore { /** * Testing only. Used for testing {@link ConfidentialKey} without {@link Jenkins} */ - /*package*/ static ThreadLocal TEST = null; + /*package*/ static ConfidentialStore TEST = null; private static final Logger LOGGER = Logger.getLogger(ConfidentialStore.class.getName()); } diff --git a/core/src/test/java/hudson/model/TaskActionTest.java b/core/src/test/java/hudson/model/TaskActionTest.java index ec31498e07..8b86e0ca93 100644 --- a/core/src/test/java/hudson/model/TaskActionTest.java +++ b/core/src/test/java/hudson/model/TaskActionTest.java @@ -1,13 +1,16 @@ package hudson.model; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import java.io.ByteArrayOutputStream; import hudson.console.AnnotatedLargeText; import hudson.security.ACL; import hudson.security.Permission; +import jenkins.security.ConfidentialStoreRule; import org.acegisecurity.Authentication; +import static org.hamcrest.CoreMatchers.startsWith; +import org.junit.Rule; import org.junit.Test; /** @@ -15,6 +18,9 @@ import org.junit.Test; */ public class TaskActionTest { + @Rule + public ConfidentialStoreRule confidentialStoreRule = new ConfidentialStoreRule(); + private static class MyTaskThread extends TaskThread { MyTaskThread(TaskAction taskAction) { super(taskAction, ListenerAndText.forMemory(taskAction)); @@ -63,6 +69,6 @@ public class TaskActionTest { } ByteArrayOutputStream os = new ByteArrayOutputStream(); annotatedText.writeLogTo(0, os); - assertTrue(os.toString("UTF-8").startsWith("a linkCompleted")); + assertThat(os.toString("UTF-8"), startsWith("a linkCompleted")); } } diff --git a/core/src/test/java/jenkins/security/ConfidentialStoreRule.java b/core/src/test/java/jenkins/security/ConfidentialStoreRule.java index db46255ba6..c42c8e6b7b 100644 --- a/core/src/test/java/jenkins/security/ConfidentialStoreRule.java +++ b/core/src/test/java/jenkins/security/ConfidentialStoreRule.java @@ -14,16 +14,13 @@ public class ConfidentialStoreRule extends ExternalResource { @Override protected void before() throws Throwable { tmp.create(); - ConfidentialStore.TEST.set(new DefaultConfidentialStore(tmp.getRoot())); + ConfidentialStore.TEST = new DefaultConfidentialStore(tmp.getRoot()); } @Override protected void after() { - ConfidentialStore.TEST.set(null); + ConfidentialStore.TEST = null; tmp.delete(); } - static { - ConfidentialStore.TEST = new ThreadLocal(); - } } diff --git a/test/src/test/java/hudson/console/ConsoleAnnotatorTest.java b/test/src/test/java/hudson/console/ConsoleAnnotatorTest.java index 83827f4860..03de67226c 100644 --- a/test/src/test/java/hudson/console/ConsoleAnnotatorTest.java +++ b/test/src/test/java/hudson/console/ConsoleAnnotatorTest.java @@ -27,6 +27,7 @@ import java.net.URL; import java.util.HashMap; import java.util.Map; import java.util.concurrent.Future; +import static org.hamcrest.Matchers.containsString; import static org.junit.Assert.*; import org.junit.Rule; import org.junit.Test; @@ -117,8 +118,7 @@ public class ConsoleAnnotatorTest { // make sure raw console output doesn't include the garbage TextPage raw = (TextPage)r.createWebClient().goTo(b.getUrl()+"consoleText","text/plain"); - System.out.println(raw.getContent()); - assertTrue(raw.getContent().contains("\nabc\ndef\n")); + assertThat(raw.getContent(), containsString("\nabc\ndef\n")); } From be28bbd9556fea74fdd1b6494e67aff46e6c33ac Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 20 Dec 2016 10:26:25 -0500 Subject: [PATCH 03/17] Existing tests caught some mistakes. --- .../main/java/hudson/console/ConsoleNote.java | 34 +++++++++++++------ 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index 3e8767ad22..46a02a4f56 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -222,23 +222,30 @@ public abstract class ConsoleNote implements Serializable, Describable 0) { // new format + mac = new byte[macSz]; + decoded.readFully(mac); + sz = decoded.readInt(); + } else { + mac = null; + sz = - macSz; } - byte[] mac = new byte[macSz]; - decoded.readFully(mac); - int sz = decoded.readInt(); byte[] buf = new byte[sz]; decoded.readFully(buf); - if (!MAC.checkMac(buf, mac)) { - throw new IOException("MAC mismatch"); - } byte[] postamble = new byte[POSTAMBLE.length]; in.readFully(postamble); if (!Arrays.equals(postamble,POSTAMBLE)) return null; // not a valid postamble + if (mac == null) { + throw new IOException("Refusing to deserialize unsigned note from an old log."); + } else if (!MAC.checkMac(buf, mac)) { + throw new IOException("MAC mismatch"); + } + ObjectInputStream ois = new ObjectInputStreamEx( new GZIPInputStream(new ByteArrayInputStream(buf)), Jenkins.getInstance().pluginManager.uberClassLoader); try { @@ -263,8 +270,15 @@ public abstract class ConsoleNote implements Serializable, Describable 0) { // new format + IOUtils.skip(decoded, macSz); + int sz = decoded.readInt(); + IOUtils.skip(decoded, sz); + } else { // old format + int sz = -macSz; + IOUtils.skip(decoded, sz); + } byte[] postamble = new byte[POSTAMBLE.length]; in.readFully(postamble); From 2d7e0d96b03ff4aeba4a27bc1dec1355c59fe91c Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 20 Dec 2016 10:34:02 -0500 Subject: [PATCH 04/17] Deleting claimed ability in Javadoc which is no longer supportable. --- core/src/main/java/hudson/console/ConsoleNote.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index 46a02a4f56..1e9b411e98 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -95,8 +95,6 @@ import jenkins.security.HMACConfidentialKey; * {@link ConsoleNote} always sticks to a particular point in the console output. * *

- * This design allows descendant processes of Hudson to emit {@link ConsoleNote}s. For example, Ant forked - * by a shell forked by Hudson can put an encoded note in its stdout, and Hudson will correctly understands that. * The preamble and postamble includes a certain ANSI escape sequence designed in such a way to minimize garbage * if this output is observed by a human being directly. * From 0e3f66f583d1b948b13136045391bafa46c2bda3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 20 Dec 2016 11:24:12 -0500 Subject: [PATCH 05/17] Added test coverage for various ConsoleNote deserialization scenarios. --- .../main/java/hudson/console/ConsoleNote.java | 10 +- .../console/AnnotatedLargeTextTest.java | 116 ++++++++++++++++++ 2 files changed, 121 insertions(+), 5 deletions(-) create mode 100644 core/src/test/java/hudson/console/AnnotatedLargeTextTest.java diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index 1e9b411e98..4c3785c95a 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -51,6 +51,7 @@ import java.util.Collection; import java.util.List; import com.jcraft.jzlib.GZIPInputStream; import com.jcraft.jzlib.GZIPOutputStream; +import hudson.remoting.ClassFilter; import jenkins.security.HMACConfidentialKey; /** @@ -244,12 +245,11 @@ public abstract class ConsoleNote implements Serializable, Describable text = new AnnotatedLargeText<>(buf, Charsets.UTF_8, true, null); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + text.writeLogTo(0, baos); + assertEquals("Some text.\nGo back to your home.\nMore text.\n", baos.toString()); + StringWriter w = new StringWriter(); + text.writeHtmlTo(0, w); + assertEquals("Some text.\nGo back to your home.\nMore text.\n", w.toString()); + } + + @Issue("SECURITY-382") + @Test + public void oldDeserialization() throws Exception { + ByteBuffer buf = new ByteBuffer(); + buf.write(("hello" + ConsoleNote.PREAMBLE_STR + "AAAAwR+LCAAAAAAAAP9dzLEOwVAUxvHThtiNprYxsGiMQhiwNSIhMR/tSZXr3Lr3oJPwPt7FM5hM3gFh8i3/5Bt+1yeUrYH6ap9Yza1Ys9WKWuMiR05wqWhEgpmyEy306Jxvwb19ccGNoBJjLplmgWq0xgOGCjkNZ2IyTrsRlFayVTs4gVMYqP3pw28/JnznuABF/rYWyIyeJfLQe1vxZiDQ7NnYZLn0UZGRRjA9MiV+0OyFv3+utadQyH8B+aJxVM4AAAA=" + ConsoleNote.POSTAMBLE_STR + "there\n").getBytes()); + AnnotatedLargeText text = new AnnotatedLargeText<>(buf, Charsets.UTF_8, true, null); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + text.writeLogTo(0, baos); + assertEquals("hellothere\n", baos.toString()); + StringWriter w = new StringWriter(); + text.writeHtmlTo(0, w); + assertEquals("hellothere\n", w.toString()); + // TODO expect log record with message "Failed to resurrect annotation" and IOException with message "Refusing to deserialize unsigned note from an old log." + } + + @Issue("SECURITY-382") + @Test + public void badMac() throws Exception { + ByteBuffer buf = new ByteBuffer(); + buf.write(("Go back to " + ConsoleNote.PREAMBLE_STR + "////4ByIhqPpAc43AbrEtyDUDc1/UEOXsoY6LeoHSeSlb1d7AAAAlR+LCAAAAAAAAP9b85aBtbiIQS+jNKU4P08vOT+vOD8nVc8xLy+/JLEkNcUnsSg9NSS1oiQktbhEBUT45ZekCpys9xWo8J3KxMDkycCWk5qXXpLhw8BcWpRTwiDkk5VYlqifk5iXrh9cUpSZl25dUcQghWaBM4QGGcYAAYxMDAwVBUAGZwkDq35Rfn4JABmN28qcAAAA" + ConsoleNote.POSTAMBLE_STR + "your home.\n").getBytes()); + AnnotatedLargeText text = new AnnotatedLargeText<>(buf, Charsets.UTF_8, true, null); + ByteArrayOutputStream baos = new ByteArrayOutputStream(); + text.writeLogTo(0, baos); + assertEquals("Go back to your home.\n", baos.toString()); + StringWriter w = new StringWriter(); + text.writeHtmlTo(0, w); + assertEquals("Go back to your home.\n", w.toString()); + // TODO expect log record with message "Failed to resurrect annotation" and IOException with message "MAC mismatch" + } + + /** Simplified version of {@link HyperlinkNote}. */ + static class TestNote extends ConsoleNote { + private final String url; + private final int length; + TestNote(String url, int length) { + this.url = url; + this.length = length; + } + @Override + public ConsoleAnnotator annotate(Void context, MarkupText text, int charPos) { + text.addMarkup(charPos, charPos + length, "", ""); + return null; + } + static String encodeTo(String url, String text) throws IOException { + return new TestNote(url, text.length()).encode() + text; + } + } + +} From a572450f039fdb99410fcf6eb0ba307bd69ea458 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 21 Dec 2016 16:21:38 -0500 Subject: [PATCH 06/17] [SECURITY-376] Remove backup directory for RekeySecretAdminMonitor. --- .../main/java/hudson/util/SecretRewriter.java | 38 ++++++++----------- .../security/RekeySecretAdminMonitor.java | 5 ++- .../hudson/util/SecretRewriterTest.groovy | 4 +- 3 files changed, 20 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/hudson/util/SecretRewriter.java b/core/src/main/java/hudson/util/SecretRewriter.java index 37dc9fb2ed..6350adf0f3 100644 --- a/core/src/main/java/hudson/util/SecretRewriter.java +++ b/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 callstack = new HashSet(); - 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(".."); } diff --git a/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java b/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java index bf5d361060..3fe47c80f5 100644 --- a/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java +++ b/core/src/main/java/jenkins/security/RekeySecretAdminMonitor.java @@ -1,6 +1,7 @@ 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(); diff --git a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy b/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy index 9285f0bcdd..f198bdd8a3 100644 --- a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy +++ b/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 From 1ce5df04e5c65e164f56755f3470cb445978c7d2 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Jan 2017 17:05:28 -0500 Subject: [PATCH 07/17] @daniel-beck requests a kill switch for ConsoleNote.MAC. --- core/src/main/java/hudson/console/ConsoleNote.java | 10 +++++++++- .../java/hudson/console/AnnotatedLargeTextTest.java | 11 ++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index 4c3785c95a..dc90310b4c 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -123,6 +123,12 @@ import jenkins.security.HMACConfidentialKey; public abstract class ConsoleNote implements Serializable, Describable>, ExtensionPoint { private static final HMACConfidentialKey MAC = new HMACConfidentialKey(ConsoleNote.class, "MAC"); + /** + * Allows historical build records with unsigned console notes to be displayed, at the expense of any security. + * Disables checking of {@link #MAC} so do not set this flag unless you completely trust all users capable of affecting build output, + * which in practice means that all SCM committers as well as all Jenkins users with any non-read-only access are consider administrators. + */ + static /* nonfinal for tests & script console */ boolean LENIENT_MAC = Boolean.getBoolean(ConsoleNote.class.getName() + ".LENIENT_MAC"); // TODO 2.x use SystemProperties /** * When the line of a console output that this annotation is attached is read by someone, @@ -240,7 +246,9 @@ public abstract class ConsoleNote implements Serializable, Describable")); + } finally { + ConsoleNote.LENIENT_MAC = false; + } } @Issue("SECURITY-382") From cc6539285cfe803bcda4645a3b4efdb3c5712247 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Jan 2017 17:12:26 -0500 Subject: [PATCH 08/17] @daniel-beck suggests a scarier name. --- core/src/main/java/hudson/console/ConsoleNote.java | 2 +- core/src/test/java/hudson/console/AnnotatedLargeTextTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index dc90310b4c..eeac73fd8f 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -128,7 +128,7 @@ public abstract class ConsoleNote implements Serializable, Describable")); } finally { - ConsoleNote.LENIENT_MAC = false; + ConsoleNote.INSECURE = false; } } From b93e2e1751f81970e898e37a2fed7b78f2bafa71 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Tue, 10 Jan 2017 17:15:44 -0500 Subject: [PATCH 09/17] Oops, forgot to rename a usage. --- core/src/main/java/hudson/console/ConsoleNote.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index eeac73fd8f..f44b7b6db6 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -246,7 +246,7 @@ public abstract class ConsoleNote implements Serializable, Describable Date: Thu, 12 Jan 2017 16:34:49 -0500 Subject: [PATCH 10/17] Merge pull request #80 from jenkinsci-cert/SECURITY-362 [SECURITY-362] Do not persist User in OfflineCause.UserCause --- .../main/java/hudson/slaves/OfflineCause.java | 46 ++++++++++++--- .../test/java/hudson/model/ComputerTest.java | 29 ++++++++++ .../config.xml | 34 +++++++++++ .../nodes/deserialized/config.xml | 56 +++++++++++++++++++ .../users/username/config.xml | 38 +++++++++++++ 5 files changed, 195 insertions(+), 8 deletions(-) create mode 100644 test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/config.xml create mode 100644 test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/nodes/deserialized/config.xml create mode 100644 test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/users/username/config.xml diff --git a/core/src/main/java/hudson/slaves/OfflineCause.java b/core/src/main/java/hudson/slaves/OfflineCause.java index 41319f01e9..3b94e99cd9 100644 --- a/core/src/main/java/hudson/slaves/OfflineCause.java +++ b/core/src/main/java/hudson/slaves/OfflineCause.java @@ -24,7 +24,6 @@ package hudson.slaves; -import jenkins.model.Jenkins; import hudson.Functions; import hudson.model.Computer; import hudson.model.User; @@ -33,7 +32,10 @@ import org.jvnet.localizer.Localizable; import org.kohsuke.stapler.export.ExportedBean; import org.kohsuke.stapler.export.Exported; +import javax.annotation.CheckForNull; import javax.annotation.Nonnull; +import java.io.ObjectStreamException; +import java.util.Collections; import java.util.Date; /** @@ -128,21 +130,49 @@ public abstract class OfflineCause { /** * Taken offline by user. + * * @since 1.551 */ public static class UserCause extends SimpleOfflineCause { - private final User user; + @Deprecated + private transient User user; + // null when unknown + private /*final*/ @CheckForNull String userId; - public UserCause(User user, String message) { - super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy( - user!=null ? user.getId() : Jenkins.ANONYMOUS.getName(), + public UserCause(@CheckForNull User user, @CheckForNull String message) { + this( + user != null ? user.getId() : null, message != null ? " : " + message : "" - )); - this.user = user; + ); + } + + private UserCause(String userId, String message) { + super(hudson.slaves.Messages._SlaveComputer_DisconnectedBy(userId, message)); + this.userId = userId; } public User getUser() { - return user; + return userId == null + ? User.getUnknown() + : User.getById(userId, true) + ; + } + + // Storing the User in a filed was a mistake, switch to userId + @SuppressWarnings("deprecation") + private Object readResolve() throws ObjectStreamException { + if (user != null) { + String id = user.getId(); + if (id != null) { + userId = id; + } else { + // The user field is not properly deserialized so id may be missing. Look the user up by fullname + User user = User.get(this.user.getFullName(), true, Collections.emptyMap()); + userId = user.getId(); + } + this.user = null; + } + return this; } } diff --git a/test/src/test/java/hudson/model/ComputerTest.java b/test/src/test/java/hudson/model/ComputerTest.java index b58b00f45e..ffe67ddb15 100644 --- a/test/src/test/java/hudson/model/ComputerTest.java +++ b/test/src/test/java/hudson/model/ComputerTest.java @@ -23,16 +23,22 @@ */ package hudson.model; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.*; + import java.io.File; +import com.gargoylesoftware.htmlunit.xml.XmlPage; +import hudson.slaves.OfflineCause; import jenkins.model.Jenkins; import hudson.slaves.DumbSlave; import org.junit.Rule; import org.junit.Test; import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.recipes.LocalData; public class ComputerTest { @@ -52,4 +58,27 @@ public class ComputerTest { assertTrue("Slave log should be kept", keep.toComputer().getLogFile().exists()); } + + @Test + public void doNotShowUserDetailsInOfflineCause() throws Exception { + DumbSlave slave = j.createOnlineSlave(); + final Computer computer = slave.toComputer(); + computer.setTemporarilyOffline(true, new OfflineCause.UserCause(User.get("username"), "msg")); + verifyOfflineCause(computer); + } + + @Test @LocalData + public void removeUserDetailsFromOfflineCause() throws Exception { + Computer computer = j.jenkins.getComputer("deserialized"); + verifyOfflineCause(computer); + } + + private void verifyOfflineCause(Computer computer) throws Exception { + XmlPage page = j.createWebClient().goToXml("computer/" + computer.getName() + "/config.xml"); + String content = page.getWebResponse().getContentAsString("UTF-8"); + assertThat(content, containsString("temporaryOfflineCause")); + assertThat(content, containsString("username")); + assertThat(content, not(containsString("ApiTokenProperty"))); + assertThat(content, not(containsString("apiToken"))); + } } diff --git a/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/config.xml b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/config.xml new file mode 100644 index 0000000000..1a1cd9abbb --- /dev/null +++ b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/config.xml @@ -0,0 +1,34 @@ + + + + 1.0 + 2 + NORMAL + true + + + false + + ${JENKINS_HOME}/workspace/${ITEM_FULLNAME} + ${ITEM_ROOTDIR}/builds + + + + + 0 + + + + All + false + false + + + + All + 0 + + + + true + diff --git a/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/nodes/deserialized/config.xml b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/nodes/deserialized/config.xml new file mode 100644 index 0000000000..b093f82a5c --- /dev/null +++ b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/nodes/deserialized/config.xml @@ -0,0 +1,56 @@ + + + + 1479196265920 + + + hudson.slaves.Messages + + SlaveComputer.DisconnectedBy + + username + : msg + + + + username + + + wPfVKd4HGJzRoEpazbTu35nXXfI34cguPjm+5JPO7pZDFLFgpFLviQsS3NdJndax + + + + + + + All + false + false + + + + + + + + + false + + + + + deserialized + dummy + ... + 1 + NORMAL + + + + + + + + + SYSTEM + diff --git a/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/users/username/config.xml b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/users/username/config.xml new file mode 100644 index 0000000000..9b55a52743 --- /dev/null +++ b/test/src/test/resources/hudson/model/ComputerTest/removeUserDetailsFromOfflineCause/users/username/config.xml @@ -0,0 +1,38 @@ + + + username + username + + + + qykj8q6EqvMg9LPu+lCqLiXBZvEVdCTWoYJwmicXgH+yh1ZUm85iHe29grd+g3QG + + + + + + + + + + + + + All + false + false + + + + + + + + + + + + false + + + From 92964da7b22b0cbb57734b9929f5be8293609c0d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 12 Jan 2017 16:35:34 -0500 Subject: [PATCH 11/17] Merge pull request #102 from jenkinsci-cert/security-349 [SECURITY-349] Use updated stapler-adjunct-timeline for newer jquery --- core/pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/pom.xml b/core/pom.xml index 94726b9737..a191e899c1 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -157,7 +157,7 @@ THE SOFTWARE. org.kohsuke.stapler stapler-adjunct-timeline - 1.4 + 1.5-20170112.000031-1 org.kohsuke.stapler From a814154695e23dc37542af7d40cacc129cf70722 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 12 Jan 2017 16:36:43 -0500 Subject: [PATCH 12/17] Merge pull request #101 from jenkinsci-cert/security-383-simpler [SECURITY-383] Additional XStream2-specific class blacklisting --- pom.xml | 2 +- .../hudson/util/XStream2Security383Test.java | 120 ++++++++++++++++++ .../util/XStream2Security383Test/config.xml | 54 ++++++++ 3 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 test/src/test/java/hudson/util/XStream2Security383Test.java create mode 100644 test/src/test/resources/hudson/util/XStream2Security383Test/config.xml diff --git a/pom.xml b/pom.xml index f57a3b6345..f01684c3f1 100644 --- a/pom.xml +++ b/pom.xml @@ -179,7 +179,7 @@ THE SOFTWARE. org.jenkins-ci.main remoting - 2.53.4 + 2.53.5-20170111.201445-1 diff --git a/test/src/test/java/hudson/util/XStream2Security383Test.java b/test/src/test/java/hudson/util/XStream2Security383Test.java new file mode 100644 index 0000000000..1e3897a5f2 --- /dev/null +++ b/test/src/test/java/hudson/util/XStream2Security383Test.java @@ -0,0 +1,120 @@ +package hudson.util; + +import hudson.model.Items; +import org.apache.commons.io.FileUtils; +import org.apache.commons.io.IOUtils; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.kohsuke.stapler.StaplerRequest; +import org.kohsuke.stapler.StaplerResponse; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import javax.servlet.ServletInputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.Assert.assertFalse; +import static org.mockito.Mockito.when; + +public class XStream2Security383Test { + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Rule + public TemporaryFolder f = new TemporaryFolder(); + + @Mock + private StaplerRequest req; + + @Mock + private StaplerResponse rsp; + + @Before + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + } + + @Test + @Issue("SECURITY-383") + public void testXmlLoad() throws Exception { + File exploitFile = f.newFile(); + try { + // be extra sure there's no file already + if (exploitFile.exists() && !exploitFile.delete()) { + throw new IllegalStateException("file exists and cannot be deleted"); + } + File tempJobDir = new File(j.jenkins.getRootDir(), "security383"); + + String exploitXml = IOUtils.toString( + XStream2Security383Test.class.getResourceAsStream( + "/hudson/util/XStream2Security383Test/config.xml"), "UTF-8"); + + exploitXml = exploitXml.replace("@TOKEN@", exploitFile.getAbsolutePath()); + + FileUtils.write(new File(tempJobDir, "config.xml"), exploitXml); + + try { + Items.load(j.jenkins, tempJobDir); + } catch (Exception e) { + // ignore + } + assertFalse("no file should be created here", exploitFile.exists()); + } finally { + exploitFile.delete(); + } + } + + @Test + @Issue("SECURITY-383") + public void testPostJobXml() throws Exception { + File exploitFile = f.newFile(); + try { + // be extra sure there's no file already + if (exploitFile.exists() && !exploitFile.delete()) { + throw new IllegalStateException("file exists and cannot be deleted"); + } + File tempJobDir = new File(j.jenkins.getRootDir(), "security383"); + + String exploitXml = IOUtils.toString( + XStream2Security383Test.class.getResourceAsStream( + "/hudson/util/XStream2Security383Test/config.xml"), "UTF-8"); + + exploitXml = exploitXml.replace("@TOKEN@", exploitFile.getAbsolutePath()); + + when(req.getMethod()).thenReturn("POST"); + when(req.getInputStream()).thenReturn(new Stream(IOUtils.toInputStream(exploitXml))); + when(req.getContentType()).thenReturn("application/xml"); + when(req.getParameter("name")).thenReturn("foo"); + + try { + j.jenkins.doCreateItem(req, rsp); + } catch (Exception e) { + // don't care + } + + assertFalse("no file should be created here", exploitFile.exists()); + } finally { + exploitFile.delete(); + } + } + + private static class Stream extends ServletInputStream { + private final InputStream inner; + + public Stream(final InputStream inner) { + this.inner = inner; + } + + @Override + public int read() throws IOException { + return inner.read(); + } + } +} diff --git a/test/src/test/resources/hudson/util/XStream2Security383Test/config.xml b/test/src/test/resources/hudson/util/XStream2Security383Test/config.xml new file mode 100644 index 0000000000..d7edab3764 --- /dev/null +++ b/test/src/test/resources/hudson/util/XStream2Security383Test/config.xml @@ -0,0 +1,54 @@ + + + + + + + + 0 + -1 + 0 + + + foo + + + + + + + + 0 + -1 + 0 + + + + + touch + @TOKEN@ + + false + + + + + + + java.lang.ProcessBuilder + start + + + foo + + + 101575 + foo + foo + + + + + + + \ No newline at end of file From b0ed9669bc00dbccf1be6896bb527b4cf2e7687d Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 12 Jan 2017 17:18:53 -0500 Subject: [PATCH 13/17] Merge pull request #104 from jenkinsci-cert/SECURITY-392 [SECURITY-392] JDKInstaller.DescriptorImpl.doPostCredential was unprotected --- core/src/main/java/hudson/tools/JDKInstaller.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/main/java/hudson/tools/JDKInstaller.java b/core/src/main/java/hudson/tools/JDKInstaller.java index 8248d416a4..8b99de1a53 100644 --- a/core/src/main/java/hudson/tools/JDKInstaller.java +++ b/core/src/main/java/hudson/tools/JDKInstaller.java @@ -77,6 +77,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import static hudson.tools.JDKInstaller.Preference.*; +import org.kohsuke.stapler.interceptor.RequirePOST; /** * Install JDKs from java.sun.com. @@ -781,7 +782,9 @@ public class JDKInstaller extends ToolInstaller { /** * Submits the Oracle account username/password. */ + @RequirePOST public HttpResponse doPostCredential(@QueryParameter String username, @QueryParameter String password) throws IOException, ServletException { + Jenkins.getInstance().checkPermission(Jenkins.ADMINISTER); this.username = username; this.password = Secret.fromString(password); save(); From e6aa166246d1734f4798a9e31f78842f4c85c28b Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 18 Jan 2017 13:21:54 -0500 Subject: [PATCH 14/17] Merge pull request #105 from jenkinsci-cert/SECURITY-304-t3 [SECURITY-304] Encrypt new secrets with CBC and random IV instead of ECB --- core/src/main/java/hudson/model/Item.java | 2 +- .../java/hudson/util/HistoricalSecrets.java | 84 ++++++++++ core/src/main/java/hudson/util/Secret.java | 147 +++++++++++------- .../main/java/hudson/util/SecretRewriter.java | 4 +- .../security/CryptoConfidentialKey.java | 90 ++++++++++- .../hudson/util/SecretRewriterTest.groovy | 33 ++-- .../test/groovy/hudson/util/SecretTest.groovy | 27 +++- .../java/hudson/util/SecretCompatTest.java | 121 ++++++++++++++ .../security/RekeySecretAdminMonitorTest.java | 10 +- test/src/test/java/lib/form/PasswordTest.java | 27 +++- .../canReadPreSec304Secrets/config.xml | 34 ++++ .../jobs/OldSecret/config.xml | 27 ++++ .../secrets/hudson.util.Secret | Bin 0 -> 272 bytes .../secrets/master.key | 1 + 14 files changed, 518 insertions(+), 89 deletions(-) create mode 100644 core/src/main/java/hudson/util/HistoricalSecrets.java create mode 100644 test/src/test/java/hudson/util/SecretCompatTest.java create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/config.xml create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret create mode 100644 test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key diff --git a/core/src/main/java/hudson/model/Item.java b/core/src/main/java/hudson/model/Item.java index 834cf97b48..7c2aeba5e2 100644 --- a/core/src/main/java/hudson/model/Item.java +++ b/core/src/main/java/hudson/model/Item.java @@ -229,7 +229,7 @@ public interface Item extends PersistenceRoot, SearchableModelObject, AccessCont Permission DISCOVER = new Permission(PERMISSIONS, "Discover", Messages._AbstractProject_DiscoverPermission_Description(), READ, PermissionScope.ITEM); /** * Ability to view configuration details. - * If the user lacks {@link CONFIGURE} then any {@link Secret}s must be masked out, even in encrypted form. + * If the user lacks {@link #CONFIGURE} then any {@link Secret}s must be masked out, even in encrypted form. * @see Secret#ENCRYPTED_VALUE_PATTERN */ Permission EXTENDED_READ = new Permission(PERMISSIONS,"ExtendedRead", Messages._AbstractProject_ExtendedReadPermission_Description(), CONFIGURE, Boolean.getBoolean("hudson.security.ExtendedReadPermission"), new PermissionScope[]{PermissionScope.ITEM}); diff --git a/core/src/main/java/hudson/util/HistoricalSecrets.java b/core/src/main/java/hudson/util/HistoricalSecrets.java new file mode 100644 index 0000000000..37a6fa39e1 --- /dev/null +++ b/core/src/main/java/hudson/util/HistoricalSecrets.java @@ -0,0 +1,84 @@ +/* + * The MIT License + * + * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package hudson.util; + +import com.trilead.ssh2.crypto.Base64; +import hudson.Util; +import jenkins.model.Jenkins; +import jenkins.security.CryptoConfidentialKey; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; + +import javax.crypto.Cipher; +import javax.crypto.SecretKey; +import java.io.IOException; +import java.security.GeneralSecurityException; + +import static java.nio.charset.StandardCharsets.UTF_8; + +/** + * Historical algorithms for decrypting {@link Secret}s. + */ +@Restricted(NoExternalUse.class) +public class HistoricalSecrets { + + /*package*/ static Secret decrypt(String data, CryptoConfidentialKey key) throws IOException, GeneralSecurityException { + 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 = Secret.getCipher("AES"); + cipher.init(Cipher.DECRYPT_MODE, getLegacyKey()); + return tryDecrypt(cipher, in); + } + + /*package*/ static Secret tryDecrypt(Cipher cipher, byte[] in) { + 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; // if the key doesn't match with the bytes, it can result in BadPaddingException + } + } + + /** + * 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. + */ + @Deprecated + /*package*/ static SecretKey getLegacyKey() throws GeneralSecurityException { + String secret = Secret.SECRET; + if(secret==null) return Jenkins.getInstance().getSecretKeyAsAES128(); + return Util.toAes128Key(secret); + } + + private static final String MAGIC = "::::MAGIC::::"; +} diff --git a/core/src/main/java/hudson/util/Secret.java b/core/src/main/java/hudson/util/Secret.java index 9a4dcb750a..0ea02d7d47 100644 --- a/core/src/main/java/hudson/util/Secret.java +++ b/core/src/main/java/hudson/util/Secret.java @@ -2,6 +2,7 @@ * The MIT License * * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. * * Permission is hereby granted, free of charge, to any person obtaining a copy * of this software and associated documentation files (the "Software"), to deal @@ -29,12 +30,12 @@ import com.thoughtworks.xstream.converters.UnmarshallingContext; import com.thoughtworks.xstream.io.HierarchicalStreamReader; import com.thoughtworks.xstream.io.HierarchicalStreamWriter; import com.trilead.ssh2.crypto.Base64; +import java.util.Arrays; import jenkins.model.Jenkins; import hudson.Util; import jenkins.security.CryptoConfidentialKey; import org.kohsuke.stapler.Stapler; -import javax.crypto.SecretKey; import javax.crypto.Cipher; import java.io.Serializable; import java.io.UnsupportedEncodingException; @@ -44,6 +45,8 @@ import java.util.regex.Pattern; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; +import static java.nio.charset.StandardCharsets.UTF_8; + /** * Glorified {@link String} that uses encryption in the persisted form, to avoid accidental exposure of a secret. * @@ -58,15 +61,22 @@ import org.kohsuke.accmod.restrictions.NoExternalUse; * @author Kohsuke Kawaguchi */ public final class Secret implements Serializable { + private static final byte PAYLOAD_V1 = 1; /** * Unencrypted secret text. */ private final String value; + private byte[] iv; - private Secret(String value) { + /*package*/ Secret(String value) { this.value = value; } + /*package*/ Secret(String value, byte[] iv) { + this.value = value; + this.iv = iv; + } + /** * Obtains the secret in a plain text. * @@ -100,20 +110,6 @@ public final class Secret implements Serializable { return value.hashCode(); } - /** - * 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. - */ - @Deprecated - /*package*/ static SecretKey getLegacyKey() throws GeneralSecurityException { - String secret = SECRET; - if(secret==null) return Jenkins.getInstance().getSecretKeyAsAES128(); - return Util.toAes128Key(secret); - } - /** * Encrypts {@link #value} and returns it in an encoded printable form. * @@ -121,56 +117,95 @@ public final class Secret implements Serializable { */ public String getEncryptedValue() { try { - 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")))); + synchronized (this) { + if (iv == null) { //if we were created from plain text or other reason without iv + iv = KEY.newIv(); + } + } + Cipher cipher = KEY.encrypt(iv); + byte[] encrypted = cipher.doFinal(this.value.getBytes(UTF_8)); + byte[] payload = new byte[1 + 8 + iv.length + encrypted.length]; + int pos = 0; + // For PAYLOAD_V1 we use this byte shifting model, V2 probably will need DataOutput + payload[pos++] = PAYLOAD_V1; + payload[pos++] = (byte)(iv.length >> 24); + payload[pos++] = (byte)(iv.length >> 16); + payload[pos++] = (byte)(iv.length >> 8); + payload[pos++] = (byte)(iv.length); + payload[pos++] = (byte)(encrypted.length >> 24); + payload[pos++] = (byte)(encrypted.length >> 16); + payload[pos++] = (byte)(encrypted.length >> 8); + payload[pos++] = (byte)(encrypted.length); + System.arraycopy(iv, 0, payload, pos, iv.length); + pos+=iv.length; + System.arraycopy(encrypted, 0, payload, pos, encrypted.length); + return "{"+new String(Base64.encode(payload))+"}"; } catch (GeneralSecurityException e) { throw new Error(e); // impossible - } catch (UnsupportedEncodingException e) { - throw new Error(e); // impossible } } /** - * Pattern matching a possible output of {@link #getEncryptedValue}. - * Basically, any Base64-encoded value. - * You must then call {@link #decrypt} to eliminate false positives. + * Pattern matching a possible output of {@link #getEncryptedValue} + * Basically, any Base64-encoded value optionally wrapped by {@code {}}. + * You must then call {@link #decrypt(String)} to eliminate false positives. + * @see #ENCRYPTED_VALUE_PATTERN */ @Restricted(NoExternalUse.class) - public static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("[A-Za-z0-9+/]+={0,2}"); + public static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("\\{?[A-Za-z0-9+/]+={0,2}}?"); /** * Reverse operation of {@link #getEncryptedValue()}. Returns null * if the given cipher text was invalid. */ 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; + if (data == null) return null; - // try our historical key for backward compatibility - Cipher cipher = getCipher("AES"); - cipher.init(Cipher.DECRYPT_MODE, getLegacyKey()); - return tryDecrypt(cipher, in); - } catch (GeneralSecurityException e) { - return null; - } catch (UnsupportedEncodingException e) { - throw new Error(e); // impossible - } catch (IOException e) { - return null; - } - } - - /*package*/ 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; // if the key doesn't match with the bytes, it can result in BadPaddingException + if (data.startsWith("{") && data.endsWith("}")) { //likely CBC encrypted/containing metadata but could be plain text + byte[] payload; + try { + payload = Base64.decode(data.substring(1, data.length()-1).toCharArray()); + } catch (IOException e) { + return null; + } + switch (payload[0]) { + case PAYLOAD_V1: + // For PAYLOAD_V1 we use this byte shifting model, V2 probably will need DataOutput + int ivLength = ((payload[1] & 0xff) << 24) + | ((payload[2] & 0xff) << 16) + | ((payload[3] & 0xff) << 8) + | (payload[4] & 0xff); + int dataLength = ((payload[5] & 0xff) << 24) + | ((payload[6] & 0xff) << 16) + | ((payload[7] & 0xff) << 8) + | (payload[8] & 0xff); + if (payload.length != 1 + 8 + ivLength + dataLength) { + // not valid v1 + return null; + } + byte[] iv = Arrays.copyOfRange(payload, 9, 9 + ivLength); + byte[] code = Arrays.copyOfRange(payload, 9+ivLength, payload.length); + String text; + try { + text = new String(KEY.decrypt(iv).doFinal(code), UTF_8); + } catch (GeneralSecurityException e) { + // it's v1 which cannot be historical, but not decrypting + return null; + } + return new Secret(text, iv); + default: + return null; + } + } else { + try { + return HistoricalSecrets.decrypt(data, KEY); + } catch (GeneralSecurityException e) { + return null; + } catch (UnsupportedEncodingException e) { + throw new Error(e); // impossible + } catch (IOException e) { + return null; + } } } @@ -228,8 +263,6 @@ public final class Secret implements Serializable { } } - private static final String MAGIC = "::::MAGIC::::"; - /** * Workaround for JENKINS-6459 / http://java.net/jira/browse/GLASSFISH-11862 * @see #getCipher(String) @@ -246,6 +279,14 @@ public final class Secret implements Serializable { */ private static final CryptoConfidentialKey KEY = new CryptoConfidentialKey(Secret.class.getName()); + /** + * Reset the internal secret key for testing. + */ + @Restricted(NoExternalUse.class) + /*package*/ static void resetKeyForTest() { + KEY.resetForTest(); + } + private static final long serialVersionUID = 1L; static { diff --git a/core/src/main/java/hudson/util/SecretRewriter.java b/core/src/main/java/hudson/util/SecretRewriter.java index 6350adf0f3..8b8a574e6a 100644 --- a/core/src/main/java/hudson/util/SecretRewriter.java +++ b/core/src/main/java/hudson/util/SecretRewriter.java @@ -40,7 +40,7 @@ public class SecretRewriter { public SecretRewriter() throws GeneralSecurityException { cipher = Secret.getCipher("AES"); - key = Secret.getLegacyKey(); + key = HistoricalSecrets.getLegacyKey(); } /** @deprecated SECURITY-376: {@code backupDirectory} is ignored */ @@ -62,7 +62,7 @@ public class SecretRewriter { return s; // not a valid base64 } cipher.init(Cipher.DECRYPT_MODE, key); - Secret sec = Secret.tryDecrypt(cipher, in); + Secret sec = HistoricalSecrets.tryDecrypt(cipher, in); if(sec!=null) // matched return sec.getEncryptedValue(); // replace by the new encrypted value else // not encrypted with the legacy key. leave it unmodified diff --git a/core/src/main/java/jenkins/security/CryptoConfidentialKey.java b/core/src/main/java/jenkins/security/CryptoConfidentialKey.java index dd1dad9e37..f402706013 100644 --- a/core/src/main/java/jenkins/security/CryptoConfidentialKey.java +++ b/core/src/main/java/jenkins/security/CryptoConfidentialKey.java @@ -1,9 +1,15 @@ package jenkins.security; +import hudson.Main; import hudson.util.Secret; +import jenkins.model.Jenkins; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.DoNotUse; +import org.kohsuke.accmod.restrictions.NoExternalUse; import javax.crypto.Cipher; import javax.crypto.SecretKey; +import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; import java.io.IOException; import java.security.GeneralSecurityException; @@ -15,6 +21,9 @@ import java.security.GeneralSecurityException; * @since 1.498 */ public class CryptoConfidentialKey extends ConfidentialKey { + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public static final int DEFAULT_IV_LENGTH = 16; + private volatile SecretKey secret; public CryptoConfidentialKey(String id) { super(id); @@ -35,7 +44,7 @@ public class CryptoConfidentialKey extends ConfidentialKey { store(payload); } // Due to the stupid US export restriction JDK only ships 128bit version. - secret = new SecretKeySpec(payload,0,128/8, ALGORITHM); + secret = new SecretKeySpec(payload,0,128/8, KEY_ALGORITHM); } } } @@ -47,10 +56,12 @@ public class CryptoConfidentialKey extends ConfidentialKey { /** * Returns a {@link Cipher} object for encrypting with this key. + * @deprecated use {@link #encrypt(byte[])} */ + @Deprecated public Cipher encrypt() { try { - Cipher cipher = Secret.getCipher(ALGORITHM); + Cipher cipher = Secret.getCipher(KEY_ALGORITHM); cipher.init(Cipher.ENCRYPT_MODE, getKey()); return cipher; } catch (GeneralSecurityException e) { @@ -59,11 +70,67 @@ public class CryptoConfidentialKey extends ConfidentialKey { } /** - * Returns a {@link Cipher} object for decrypting with this key. + * Returns a {@link Cipher} object for encrypting with this key using the provided initialization vector. + * @param iv the initialization vector + * @return the cipher */ - public Cipher decrypt() { + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public Cipher encrypt(byte[] iv) { try { Cipher cipher = Secret.getCipher(ALGORITHM); + cipher.init(Cipher.ENCRYPT_MODE, getKey(), new IvParameterSpec(iv)); + return cipher; + } catch (GeneralSecurityException e) { + throw new AssertionError(e); + } + } + + /** + * Returns a {@link Cipher} object for decrypting with this key using the provided initialization vector. + * @param iv the initialization vector + * @return the cipher + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public Cipher decrypt(byte[] iv) { + try { + Cipher cipher = Secret.getCipher(ALGORITHM); + cipher.init(Cipher.DECRYPT_MODE, getKey(), new IvParameterSpec(iv)); + return cipher; + } catch (GeneralSecurityException e) { + throw new AssertionError(e); + } + } + + /** + * Generates a new Initialization Vector. + * @param length the length of the salt + * @return some random bytes + * @see #encrypt(byte[]) + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public byte[] newIv(int length) { + return ConfidentialStore.get().randomBytes(length); + } + + /** + * Generates a new Initialization Vector of default length. + * @return some random bytes + * @see #newIv(int) + * @see #encrypt(byte[]) + */ + @Restricted(NoExternalUse.class) //TODO remove when in mainline + public byte[] newIv() { + return newIv(DEFAULT_IV_LENGTH); + } + + /** + * Returns a {@link Cipher} object for decrypting with this key. + * @deprecated use {@link #decrypt(byte[])} + */ + @Deprecated + public Cipher decrypt() { + try { + Cipher cipher = Secret.getCipher(KEY_ALGORITHM); cipher.init(Cipher.DECRYPT_MODE, getKey()); return cipher; } catch (GeneralSecurityException e) { @@ -72,5 +139,18 @@ public class CryptoConfidentialKey extends ConfidentialKey { } - private static final String ALGORITHM = "AES"; + private static final String KEY_ALGORITHM = "AES"; + private static final String ALGORITHM = "AES/CBC/PKCS5Padding"; + + /** + * Reset the internal secret key for testing. + */ + @Restricted(NoExternalUse.class) + public void resetForTest() { + if (Main.isUnitTest) { + this.secret = null; + } else { + throw new IllegalStateException("Only for testing"); + } + } } diff --git a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy b/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy index f198bdd8a3..b748703ac1 100644 --- a/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy +++ b/core/src/test/groovy/hudson/util/SecretRewriterTest.groovy @@ -23,42 +23,49 @@ class SecretRewriterTest { @Rule public TemporaryFolder tmp = new TemporaryFolder() + def FOO_PATTERN = /\{[A-Za-z0-9+\/]+={0,2}}<\/foo>/ + def MSG_PATTERN = /\{[A-Za-z0-9+\/]+={0,2}}<\/msg>/ + def FOO_PATTERN2 = /(\{[A-Za-z0-9+\/]+={0,2}}<\/foo>){2}/ + def ABC_FOO_PATTERN = /\s\{[A-Za-z0-9+\/]+={0,2}}<\/foo>\s<\/abc>/ + @Test void singleFileRewrite() { def o = encryptOld('foobar') // old def n = encryptNew('foobar') // new roundtrip "${o}", - "${n}" + {assert it ==~ FOO_PATTERN} + roundtrip "${o}${o}", - "${n}${n}" + {assert it ==~ FOO_PATTERN2} roundtrip "${n}", - "${n}" + {assert it == "${n}"} roundtrip " thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret ", - " thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret " + {assert it == "thisIsLegalBase64AndLongEnoughThatItCouldLookLikeSecret"} // to be rewritten, it needs to be between a tag - roundtrip "$o", "$o" - roundtrip "$o", "$o" + roundtrip "$o", {assert it == "$o"} + roundtrip "$o", {assert it == "$o"} // - roundtrip "\n$o\n", "\n$n\n" + roundtrip "\n$o\n", {assert it ==~ ABC_FOO_PATTERN} } - void roundtrip(String before, String after) { + void roundtrip(String before, Closure check) { def sr = new SecretRewriter(null); def f = File.createTempFile("test", "xml", tmp.root) f.text = before sr.rewrite(f,null) - assert after.replaceAll(System.getProperty("line.separator"), "\n").trim()==f.text.replaceAll(System.getProperty("line.separator"), "\n").trim() + check(f.text.replaceAll(System.getProperty("line.separator"), "\n").trim()) + //assert after.replaceAll(System.getProperty("line.separator"), "\n").trim()==f.text.replaceAll(System.getProperty("line.separator"), "\n").trim() } String encryptOld(str) { def cipher = Secret.getCipher("AES"); - cipher.init(Cipher.ENCRYPT_MODE, Secret.legacyKey); - return new String(Base64.encode(cipher.doFinal((str + Secret.MAGIC).getBytes("UTF-8")))) + cipher.init(Cipher.ENCRYPT_MODE, HistoricalSecrets.legacyKey); + return new String(Base64.encode(cipher.doFinal((str + HistoricalSecrets.MAGIC).getBytes("UTF-8")))) } String encryptNew(str) { @@ -99,11 +106,11 @@ class SecretRewriterTest { assert 6==sw.rewriteRecursive(t, st) dirs.each { p-> - assert new File(t,"$p/foo.xml").text.trim()==answer + assert new File(t,"$p/foo.xml").text.trim() ==~ MSG_PATTERN } // t2 is only reachable by following a symlink. this should be covered, too - assert new File(t2,"foo.xml").text.trim()==answer.trim(); + assert new File(t2,"foo.xml").text.trim() ==~ MSG_PATTERN } } diff --git a/core/src/test/groovy/hudson/util/SecretTest.groovy b/core/src/test/groovy/hudson/util/SecretTest.groovy index f7a0ba3ad0..8f39ae7ec0 100644 --- a/core/src/test/groovy/hudson/util/SecretTest.groovy +++ b/core/src/test/groovy/hudson/util/SecretTest.groovy @@ -31,7 +31,8 @@ import org.junit.Rule import org.junit.Test import java.util.Random; -import javax.crypto.Cipher; +import javax.crypto.Cipher +import java.util.regex.Pattern; /** * @author Kohsuke Kawaguchi @@ -43,6 +44,8 @@ public class SecretTest { @Rule public MockSecretRule mockSecretRule = new MockSecretRule() + static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("\\{?[A-Za-z0-9+/]+={0,2}}?"); + @Test void testEncrypt() { def secret = Secret.fromString("abc"); @@ -54,6 +57,11 @@ public class SecretTest { // can we round trip? assert secret==Secret.fromString(secret.encryptedValue); + + //Two consecutive encryption requests of the same object should result in the same encrypted value - SECURITY-304 + assert secret.encryptedValue == secret.encryptedValue + //Two consecutive encryption requests of different objects with the same value should not result in the same encrypted value - SECURITY-304 + assert secret.encryptedValue != Secret.fromString(secret.plainText).encryptedValue } @Test @@ -62,9 +70,16 @@ public class SecretTest { String plaintext = RandomStringUtils.random(new Random().nextInt(i)); String ciphertext = Secret.fromString(plaintext).getEncryptedValue(); //println "${plaintext} → ${ciphertext}" - assert Secret.ENCRYPTED_VALUE_PATTERN.matcher(ciphertext).matches(); + assert ENCRYPTED_VALUE_PATTERN.matcher(ciphertext).matches(); } - assert !Secret.ENCRYPTED_VALUE_PATTERN.matcher("hello world").matches(); + //Not "plain" text + assert !ENCRYPTED_VALUE_PATTERN.matcher("hello world").matches(); + //Not "plain" text + assert !ENCRYPTED_VALUE_PATTERN.matcher("helloworld!").matches(); + //legacy key + assert ENCRYPTED_VALUE_PATTERN.matcher("abcdefghijklmnopqr0123456789").matches(); + //legacy key + assert ENCRYPTED_VALUE_PATTERN.matcher("abcdefghijklmnopqr012345678==").matches(); } @Test @@ -77,7 +92,7 @@ public class SecretTest { def s = Secret.fromString("Mr.Jenkins"); def xml = Jenkins.XSTREAM.toXML(s); assert !xml.contains(s.plainText) - assert xml.contains(s.encryptedValue) + assert xml ==~ /\{[A-Za-z0-9+\/]+={0,2}}<\/hudson\.util\.Secret>/ def o = Jenkins.XSTREAM.fromXML(xml); assert o==s : xml; @@ -104,11 +119,11 @@ public class SecretTest { */ @Test void migrationFromLegacyKeyToConfidentialStore() { - def legacy = Secret.legacyKey + def legacy = HistoricalSecrets.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 old = new String(Base64.encode(cipher.doFinal((str + HistoricalSecrets.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" diff --git a/test/src/test/java/hudson/util/SecretCompatTest.java b/test/src/test/java/hudson/util/SecretCompatTest.java new file mode 100644 index 0000000000..8c440c4ff9 --- /dev/null +++ b/test/src/test/java/hudson/util/SecretCompatTest.java @@ -0,0 +1,121 @@ +/* + * The MIT License + * + * Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi + * Copyright (c) 2016, CloudBees Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package hudson.util; + +import hudson.model.FreeStyleProject; +import hudson.model.ParameterDefinition; +import hudson.model.ParametersDefinitionProperty; +import hudson.model.PasswordParameterDefinition; +import org.hamcrest.core.Is; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.recipes.LocalData; + +import java.io.IOException; +import java.util.regex.Pattern; + +import static org.hamcrest.core.Is.isA; +import static org.hamcrest.core.IsNot.not; +import static org.hamcrest.core.StringContains.containsString; +import static org.junit.Assert.*; + +/** + * Tests {@link Secret}. + */ +public class SecretCompatTest { + + @Rule + public JenkinsRule j = new JenkinsRule() { + @Override + public void before() throws Throwable { + Secret.resetKeyForTest(); //As early as possible + super.before(); + } + }; + + @After + public void after() { + Secret.resetKeyForTest(); + } + + + @Test + @Issue("SECURITY-304") + public void encryptedValueStaysTheSameAfterRoundtrip() throws Exception { + FreeStyleProject project = j.createFreeStyleProject(); + project.addProperty(new ParametersDefinitionProperty(new PasswordParameterDefinition("p", "s3cr37", "Keep this a secret"))); + project = j.configRoundtrip(project); + String round1 = project.getConfigFile().asString(); + project = j.configRoundtrip(project); + String round2 = project.getConfigFile().asString(); + assertEquals(round1, round2); + + + //But reconfiguring will make it a new value + project = j.jenkins.getItemByFullName(project.getFullName(), FreeStyleProject.class); + project.removeProperty(ParametersDefinitionProperty.class); + project.addProperty(new ParametersDefinitionProperty(new PasswordParameterDefinition("p", "s3cr37", "Keep this a secret"))); + project = j.configRoundtrip(project); + String round3 = project.getConfigFile().asString(); + assertNotEquals(round2, round3); + //Saving again will produce the same + project = j.configRoundtrip(project); + String round4 = project.getConfigFile().asString(); + assertEquals(round3, round4); + } + + @Test + @Issue("SECURITY-304") + @LocalData + public void canReadPreSec304Secrets() throws Exception { + FreeStyleProject project = j.jenkins.getItemByFullName("OldSecret", FreeStyleProject.class); + String oldxml = project.getConfigFile().asString(); + //It should be unchanged on disk + assertThat(oldxml, containsString("z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo=")); + ParametersDefinitionProperty property = project.getProperty(ParametersDefinitionProperty.class); + ParameterDefinition definition = property.getParameterDefinitions().get(0); + assertTrue(definition instanceof PasswordParameterDefinition); + Secret secret = ((PasswordParameterDefinition) definition).getDefaultValueAsSecret(); + assertEquals("theSecret", secret.getPlainText()); + + //OK it was read correctly from disk, now the first roundtrip should update the encrypted value + + project = j.configRoundtrip(project); + String newXml = project.getConfigFile().asString(); + assertNotEquals(oldxml, newXml); //This could have changed because Jenkins has moved on, so not really a good check + assertThat(newXml, not(containsString("z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo="))); + Pattern p = Pattern.compile("\\{[A-Za-z0-9+/]+={0,2}}"); + assertTrue(p.matcher(newXml).find()); + + //But the next roundtrip should result in the same data + project = j.configRoundtrip(project); + String round2 = project.getConfigFile().asString(); + assertEquals(newXml, round2); + } +} diff --git a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java index ea7a08ba45..413b4a6eed 100644 --- a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java +++ b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java @@ -11,6 +11,7 @@ import hudson.Util; import hudson.util.Secret; import hudson.util.SecretHelper; import org.apache.commons.io.FileUtils; +import org.hamcrest.CoreMatchers; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.recipes.Recipe.Runner; import org.xml.sax.SAXException; @@ -20,6 +21,9 @@ import javax.inject.Inject; import java.io.File; import java.io.IOException; import java.lang.annotation.Annotation; +import java.util.regex.Pattern; + +import static org.junit.Assert.assertThat; /** * @author Kohsuke Kawaguchi @@ -28,6 +32,8 @@ public class RekeySecretAdminMonitorTest extends HudsonTestCase { @Inject RekeySecretAdminMonitor monitor; + final String plain_regex_match = ".*\\{[A-Za-z0-9+/]+={0,2}}.*"; + @Override protected void setUp() throws Exception { SecretHelper.set(TEST_KEY); @@ -76,8 +82,8 @@ public class RekeySecretAdminMonitorTest extends HudsonTestCase { private void verifyRewrite(File dir) throws Exception { File xml = new File(dir, "foo.xml"); - assertEquals("" + encryptNew(TEST_KEY) + "".trim(), - FileUtils.readFileToString(xml).trim()); + Pattern pattern = Pattern.compile(""+plain_regex_match+""); + assertTrue(pattern.matcher(FileUtils.readFileToString(xml).trim()).matches()); } // TODO sometimes fails: "Invalid request submission: {json=[Ljava.lang.String;@2c46358e, .crumb=[Ljava.lang.String;@35661457}" diff --git a/test/src/test/java/lib/form/PasswordTest.java b/test/src/test/java/lib/form/PasswordTest.java index 595b0e7e85..0a9f5eb149 100644 --- a/test/src/test/java/lib/form/PasswordTest.java +++ b/test/src/test/java/lib/form/PasswordTest.java @@ -43,10 +43,13 @@ import java.io.PrintStream; import java.util.Arrays; import java.util.Collections; import java.util.Locale; +import java.util.regex.Pattern; + import jenkins.model.Jenkins; import org.acegisecurity.Authentication; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.not; +import static org.hamcrest.core.Is.is; import static org.junit.Assert.assertThat; import org.jvnet.hudson.test.HudsonTestCase; import org.jvnet.hudson.test.Issue; @@ -78,10 +81,19 @@ public class PasswordTest extends HudsonTestCase implements Describable" + sEnc + "")); + + assertThat(Pattern.compile("" + xml_regex_match + "").matcher(xmlAdmin).find(), is(true)); assertThat(xmlAdmin, containsString("" + p.getDisplayName() + "")); assertThat(xmlAdmin, containsString("" + p.getDescription() + "")); // CLICommandInvoker does not work here, as it sets up its own SecurityRealm + AuthorizationStrategy. @@ -131,11 +144,11 @@ public class PasswordTest extends HudsonTestCase implements Describable + + + 1.625.4-SNAPSHOT (private-12/16/2016 18:04 GMT-rsandell) + 2 + NORMAL + true + + + false + + ${ITEM_ROOTDIR}/workspace + ${ITEM_ROOTDIR}/builds + + + + + 5 + 0 + + + + All + false + false + + + + All + 0 + + + + \ No newline at end of file diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml new file mode 100644 index 0000000000..e094ad77fd --- /dev/null +++ b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/jobs/OldSecret/config.xml @@ -0,0 +1,27 @@ + + + + + false + + + + + alice + theSecret + z/Dd3qrHdQ6/C5lR7uEafM/jD3nQDrGprw3XsfZ/0vo= + + + + + + true + false + false + false + + false + + + + \ No newline at end of file diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/hudson.util.Secret new file mode 100644 index 0000000000000000000000000000000000000000..cdec3920fbc06ca5f07e25e884ea246c80f898d4 GIT binary patch literal 272 zcmV+r0q_2*6%{s0ht|6$Z8z))t|l)&1ny;IB`WdC+cNvem;y-ic$|YUZcP z7)L*-wH8Ml#ULpjze85UAdBs5{CyvOY1|MBD9#gu-M-3yCnC>9c}H&cV&KMWsX`xd zvgnC7{{@5o4f7ZkQ=z8rXE;Ztvx2o_XNBer_PlM$sL~1(njgRLp(=QJ+8C*6+wlE` zaK`~?T4b6>p-#u>XiKswSqwa literal 0 HcmV?d00001 diff --git a/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key new file mode 100644 index 0000000000..2572203a97 --- /dev/null +++ b/test/src/test/resources/hudson/util/SecretCompatTest/canReadPreSec304Secrets/secrets/master.key @@ -0,0 +1 @@ +4311ab5e95e3da7b9b1360b52cac6f0f666db7b48f9f701296cc07c8f00612b451f1874e584d49560810619e8a6ff6b19f8f58ae1305c515fc62a7b60ea3a69e6058cad16b2c8df317952b749fdaaecab013431da55bb4ea4b8eee754fa043261b51a99a2b537fd57f867cdcb1e209f3bba735a8672dbfc3f10b0e2209a81683 \ No newline at end of file From 1b76edd98702b34248625d5dc349d57ae7ad52e6 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Thu, 12 Jan 2017 17:24:54 -0500 Subject: [PATCH 15/17] Compilable against Servlet 3.1. --- .../java/hudson/util/XStream2Security383Test.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/src/test/java/hudson/util/XStream2Security383Test.java b/test/src/test/java/hudson/util/XStream2Security383Test.java index 1e3897a5f2..7e089e4cc4 100644 --- a/test/src/test/java/hudson/util/XStream2Security383Test.java +++ b/test/src/test/java/hudson/util/XStream2Security383Test.java @@ -14,6 +14,7 @@ import org.kohsuke.stapler.StaplerResponse; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import javax.servlet.ReadListener; import javax.servlet.ServletInputStream; import java.io.File; import java.io.IOException; @@ -116,5 +117,17 @@ public class XStream2Security383Test { public int read() throws IOException { return inner.read(); } + @Override + public boolean isFinished() { + throw new UnsupportedOperationException(); + } + @Override + public boolean isReady() { + throw new UnsupportedOperationException(); + } + @Override + public void setReadListener(ReadListener readListener) { + throw new UnsupportedOperationException(); + } } } From 10193b80a9c4de14a5b4ebdd7663b7c753641440 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Wed, 11 Jan 2017 19:16:57 -0500 Subject: [PATCH 16/17] Switching to SystemProperties. --- core/src/main/java/hudson/console/ConsoleNote.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/console/ConsoleNote.java b/core/src/main/java/hudson/console/ConsoleNote.java index f44b7b6db6..1f6caad754 100644 --- a/core/src/main/java/hudson/console/ConsoleNote.java +++ b/core/src/main/java/hudson/console/ConsoleNote.java @@ -53,6 +53,7 @@ import com.jcraft.jzlib.GZIPInputStream; import com.jcraft.jzlib.GZIPOutputStream; import hudson.remoting.ClassFilter; import jenkins.security.HMACConfidentialKey; +import jenkins.util.SystemProperties; /** * Data that hangs off from a console output. @@ -128,7 +129,7 @@ public abstract class ConsoleNote implements Serializable, Describable Date: Thu, 19 Jan 2017 10:51:01 -0500 Subject: [PATCH 17/17] encryptedValueStaysTheSameAfterRoundtrip was failing in Jenkins 2 since the sidepanel is no longer displayed on the configuration screen. --- test/src/test/java/hudson/util/SecretCompatTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/src/test/java/hudson/util/SecretCompatTest.java b/test/src/test/java/hudson/util/SecretCompatTest.java index 8c440c4ff9..dc15312bfb 100644 --- a/test/src/test/java/hudson/util/SecretCompatTest.java +++ b/test/src/test/java/hudson/util/SecretCompatTest.java @@ -70,6 +70,7 @@ public class SecretCompatTest { public void encryptedValueStaysTheSameAfterRoundtrip() throws Exception { FreeStyleProject project = j.createFreeStyleProject(); project.addProperty(new ParametersDefinitionProperty(new PasswordParameterDefinition("p", "s3cr37", "Keep this a secret"))); + project.getAllActions(); // initialize Actionable.actions; otherwise first made nonnull while rendering sidepanel after redirect after round #1 has been saved, so only round #2 has project = j.configRoundtrip(project); String round1 = project.getConfigFile().asString(); project = j.configRoundtrip(project);