diff --git a/core/pom.xml b/core/pom.xml index b5913b102f38..d9ffd0294c2c 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -895,5 +895,16 @@ THE SOFTWARE. true + + all-tests + + + !test + + + + true + + diff --git a/core/src/main/java/hudson/util/HistoricalSecrets.java b/core/src/main/java/hudson/util/HistoricalSecrets.java index 7b1d0ee3bb85..ed0bbf9d4de5 100644 --- a/core/src/main/java/hudson/util/HistoricalSecrets.java +++ b/core/src/main/java/hudson/util/HistoricalSecrets.java @@ -81,9 +81,15 @@ public class HistoricalSecrets { */ @Deprecated /*package*/ static SecretKey getLegacyKey() throws GeneralSecurityException { - String secret = Secret.SECRET; - if(secret==null) return Jenkins.get().getSecretKeyAsAES128(); - return Util.toAes128Key(secret); + if (Secret.SECRET != null) { + return Util.toAes128Key(Secret.SECRET); + } + Jenkins j = Jenkins.getInstanceOrNull(); + if (j != null) { + return j.getSecretKeyAsAES128(); + } else { + return Util.toAes128Key("mock"); + } } 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 d53054cec9f0..af17928d21a3 100644 --- a/core/src/main/java/hudson/util/Secret.java +++ b/core/src/main/java/hudson/util/Secret.java @@ -31,7 +31,6 @@ import com.thoughtworks.xstream.io.HierarchicalStreamWriter; import jenkins.util.SystemProperties; import java.util.Arrays; -import jenkins.model.Jenkins; import hudson.Util; import jenkins.security.CryptoConfidentialKey; import org.kohsuke.stapler.Stapler; @@ -289,8 +288,10 @@ public Object unmarshal(HierarchicalStreamReader reader, final UnmarshallingCont private static final String PROVIDER = SystemProperties.getString(Secret.class.getName()+".provider"); /** - * For testing only. Override the secret key so that we can test this class without {@link Jenkins}. + * For testing only. + * @deprecated Normally unnecessary. */ + @Deprecated /*package*/ static String SECRET = null; /** diff --git a/core/src/main/java/jenkins/security/ConfidentialStore.java b/core/src/main/java/jenkins/security/ConfidentialStore.java index e9b79e058176..1062486e089f 100644 --- a/core/src/main/java/jenkins/security/ConfidentialStore.java +++ b/core/src/main/java/jenkins/security/ConfidentialStore.java @@ -10,10 +10,14 @@ import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import java.io.IOException; +import java.security.NoSuchAlgorithmException; import java.security.SecureRandom; import java.util.Iterator; +import java.util.Map; +import java.util.Random; import java.util.ServiceConfigurationError; import java.util.ServiceLoader; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; @@ -51,6 +55,9 @@ public abstract class ConfidentialStore { */ protected abstract @CheckForNull byte[] load(ConfidentialKey key) throws IOException; + // TODO consider promoting to public, and offering a default implementation of randomBytes (via the usual Util.isOverridden binary compat trick) + abstract SecureRandom secureRandom(); + /** * Works like {@link SecureRandom#nextBytes(byte[])}. * @@ -64,7 +71,10 @@ public abstract class ConfidentialStore { public static @Nonnull ConfidentialStore get() { if (TEST!=null) return TEST.get(); - Jenkins j = Jenkins.get(); + Jenkins j = Jenkins.getInstanceOrNull(); + if (j == null) { + return Mock.INSTANCE; + } Lookup lookup = j.lookup; ConfidentialStore cs = lookup.get(ConfidentialStore.class); if (cs==null) { @@ -91,10 +101,60 @@ public abstract class ConfidentialStore { return cs; } - /** - * Testing only. Used for testing {@link ConfidentialKey} without {@link Jenkins} + /** + * @deprecated No longer needed. */ + @Deprecated /*package*/ static ThreadLocal TEST = null; + static final class Mock extends ConfidentialStore { + + static final Mock INSTANCE = new Mock(); + + private final SecureRandom rand; + + private final Map data = new ConcurrentHashMap<>(); + + Mock() { + // Use a predictable seed to make tests more reproducible. + try { + rand = SecureRandom.getInstance("SHA1PRNG"); + } catch (NoSuchAlgorithmException x) { + throw new AssertionError("https://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#SecureRandom", x); + } + rand.setSeed(new byte[] {1, 2, 3, 4}); + } + + void clear() { + data.clear(); + } + + @Override + protected void store(ConfidentialKey key, byte[] payload) throws IOException { + LOGGER.fine(() -> "storing " + key.getId() + " " + hudson.Util.getDigestOf(hudson.Util.toHexString(payload))); + data.put(key.getId(), payload); + } + + @Override + protected byte[] load(ConfidentialKey key) throws IOException { + byte[] payload = data.get(key.getId()); + LOGGER.fine(() -> "loading " + key.getId() + " " + (payload != null ? hudson.Util.getDigestOf(hudson.Util.toHexString(payload)) : "null")); + return payload; + } + + @Override + SecureRandom secureRandom() { + return rand; + } + + @Override + public byte[] randomBytes(int size) { + byte[] random = new byte[size]; + rand.nextBytes(random); + return random; + } + + } + private static final Logger LOGGER = Logger.getLogger(ConfidentialStore.class.getName()); } diff --git a/core/src/main/java/jenkins/security/DefaultConfidentialStore.java b/core/src/main/java/jenkins/security/DefaultConfidentialStore.java index 5a16cefc7074..4d18357b32b8 100644 --- a/core/src/main/java/jenkins/security/DefaultConfidentialStore.java +++ b/core/src/main/java/jenkins/security/DefaultConfidentialStore.java @@ -140,6 +140,11 @@ private File getFileFor(ConfidentialKey key) { return new File(rootDir, key.getId()); } + @Override + SecureRandom secureRandom() { + return sr; + } + public byte[] randomBytes(int size) { byte[] random = new byte[size]; sr.nextBytes(random); diff --git a/core/src/main/java/jenkins/security/RSAConfidentialKey.java b/core/src/main/java/jenkins/security/RSAConfidentialKey.java index fe30c622de06..8c5f64b31748 100644 --- a/core/src/main/java/jenkins/security/RSAConfidentialKey.java +++ b/core/src/main/java/jenkins/security/RSAConfidentialKey.java @@ -30,7 +30,6 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.PrivateKey; -import java.security.SecureRandom; import java.security.interfaces.RSAPrivateCrtKey; import java.security.interfaces.RSAPrivateKey; import java.security.interfaces.RSAPublicKey; @@ -79,7 +78,7 @@ protected synchronized RSAPrivateKey getPrivateKey() { byte[] payload = load(); if (payload == null) { KeyPairGenerator gen = KeyPairGenerator.getInstance("RSA"); - gen.initialize(2048, new SecureRandom()); // going beyond 2048 requires crypto extension + gen.initialize(2048, cs.secureRandom()); // going beyond 2048 requires crypto extension KeyPair keys = gen.generateKeyPair(); priv = (RSAPrivateKey) keys.getPrivate(); pub = (RSAPublicKey) keys.getPublic(); diff --git a/core/src/test/java/hudson/util/MockSecretRule.java b/core/src/test/java/hudson/util/MockSecretRule.java deleted file mode 100644 index f3c11ea381de..000000000000 --- a/core/src/test/java/hudson/util/MockSecretRule.java +++ /dev/null @@ -1,33 +0,0 @@ -package hudson.util; - -import hudson.Util; -import org.junit.rules.ExternalResource; - -import java.security.SecureRandom; - -/** - * JUnit rule that cleans that sets a temporary {@link Secret#SECRET} value. - * - * @author Kohsuke Kawaguchi - */ -public class MockSecretRule extends ExternalResource { - - private String value; - - @Override - protected void before() throws Throwable { - byte[] random = new byte[32]; - sr.nextBytes(random); - value = Util.toHexString(random); - Secret.SECRET = value; - } - - @Override - protected void after() { - if (!Secret.SECRET.equals(value)) - throw new IllegalStateException("Someone tinkered with Secret.SECRET"); - Secret.SECRET = null; - } - - private static final SecureRandom sr = new SecureRandom(); -} diff --git a/core/src/test/java/hudson/util/SecretRewriterTest.java b/core/src/test/java/hudson/util/SecretRewriterTest.java index 881f278518f7..bb7e6adf68fa 100644 --- a/core/src/test/java/hudson/util/SecretRewriterTest.java +++ b/core/src/test/java/hudson/util/SecretRewriterTest.java @@ -22,9 +22,6 @@ public class SecretRewriterTest { - @Rule - public MockSecretRule mockSecretRule = new MockSecretRule(); - @Rule public ConfidentialStoreRule confidentialStoreRule = new ConfidentialStoreRule(); diff --git a/core/src/test/java/hudson/util/SecretTest.java b/core/src/test/java/hudson/util/SecretTest.java index 32b8d86cebec..19fec48e7068 100644 --- a/core/src/test/java/hudson/util/SecretTest.java +++ b/core/src/test/java/hudson/util/SecretTest.java @@ -43,9 +43,6 @@ public class SecretTest { @Rule public ConfidentialStoreRule confidentialStore = new ConfidentialStoreRule(); - @Rule - public MockSecretRule mockSecretRule = new MockSecretRule(); - private static final Pattern ENCRYPTED_VALUE_PATTERN = Pattern.compile("\\{?[A-Za-z0-9+/]+={0,2}}?"); @Test diff --git a/core/src/test/java/jenkins/security/ConfidentialStoreRule.java b/core/src/test/java/jenkins/security/ConfidentialStoreRule.java index e060cfb3f6c2..5fbc242a7f8f 100644 --- a/core/src/test/java/jenkins/security/ConfidentialStoreRule.java +++ b/core/src/test/java/jenkins/security/ConfidentialStoreRule.java @@ -2,28 +2,15 @@ import org.junit.rules.ExternalResource; -import org.junit.rules.TemporaryFolder; /** - * Test rule that injects a temporary {@link DefaultConfidentialStore} - * @author Kohsuke Kawaguchi + * Test rule that makes {@link ConfidentialStore#get} be reset for each test. */ public class ConfidentialStoreRule extends ExternalResource { - private final TemporaryFolder tmp = new TemporaryFolder(); @Override protected void before() throws Throwable { - tmp.create(); - ConfidentialStore.TEST.set(new DefaultConfidentialStore(tmp.getRoot())); + ConfidentialStore.Mock.INSTANCE.clear(); } - @Override - protected void after() { - ConfidentialStore.TEST.set(null); - tmp.delete(); - } - - static { - ConfidentialStore.TEST = new ThreadLocal<>(); - } } diff --git a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java index 4a84630e2de1..1b744c29952b 100644 --- a/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java +++ b/test/src/test/java/jenkins/security/RekeySecretAdminMonitorTest.java @@ -19,9 +19,12 @@ import java.io.File; import java.io.IOException; import java.lang.annotation.Annotation; +import java.nio.charset.StandardCharsets; import java.util.Base64; import java.util.regex.Pattern; import java.util.stream.Stream; +import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; /** * @author Kohsuke Kawaguchi @@ -81,7 +84,7 @@ private void putSomeOldData(File dir) throws Exception { private void verifyRewrite(File dir) throws Exception { File xml = new File(dir, "foo.xml"); Pattern pattern = Pattern.compile(""+plain_regex_match+""); - assertTrue(pattern.matcher(FileUtils.readFileToString(xml).trim()).matches()); + MatcherAssert.assertThat(FileUtils.readFileToString(xml, StandardCharsets.UTF_8).trim(), Matchers.matchesRegex(pattern)); } // TODO sometimes fails: "Invalid request submission: {json=[Ljava.lang.String;@2c46358e, .crumb=[Ljava.lang.String;@35661457}"