diff --git a/src/main/java/jenkins/metrics/api/Metrics.java b/src/main/java/jenkins/metrics/api/Metrics.java index 3c885f3..b50fc62 100644 --- a/src/main/java/jenkins/metrics/api/Metrics.java +++ b/src/main/java/jenkins/metrics/api/Metrics.java @@ -200,7 +200,7 @@ public static HealthCheckData getHealthCheckData() { */ @NonNull public static MetricRegistry metricRegistry() { - Metrics plugin = Jenkins.getInstance().getPlugin(Metrics.class); + Metrics plugin = Jenkins.get().getPlugin(Metrics.class); if (plugin == null || plugin.metricRegistry == null) { throw new AssertionError(Metrics.class.getName() + " is missing its MetricRegistry"); } @@ -209,7 +209,7 @@ public static MetricRegistry metricRegistry() { private static MetricsAccessKey.DescriptorImpl accessKeyDescriptorOrDie() { MetricsAccessKey.DescriptorImpl descriptor = - Jenkins.getInstance().getDescriptorByType(MetricsAccessKey.DescriptorImpl.class); + Jenkins.get().getDescriptorByType(MetricsAccessKey.DescriptorImpl.class); if (descriptor == null) { throw new IllegalStateException(); } @@ -296,7 +296,7 @@ public void start() throws Exception { @Initializer(after = InitMilestone.EXTENSIONS_AUGMENTED, before = InitMilestone.JOB_LOADED) public static void afterExtensionsAugmented() { LOGGER.log(Level.FINER, "Registering metric provider and health check provider extensions..."); - Jenkins jenkins = Jenkins.getInstance(); + Jenkins jenkins = Jenkins.get(); Metrics plugin = jenkins.getPlugin(Metrics.class); if (plugin == null) { LOGGER.log(Level.WARNING, "Could not register metrics providers or health check providers as " diff --git a/src/main/java/jenkins/metrics/api/MetricsAccessKey.java b/src/main/java/jenkins/metrics/api/MetricsAccessKey.java index 9f8b667..0300fde 100644 --- a/src/main/java/jenkins/metrics/api/MetricsAccessKey.java +++ b/src/main/java/jenkins/metrics/api/MetricsAccessKey.java @@ -30,9 +30,14 @@ import hudson.ExtensionList; import hudson.ExtensionPoint; import hudson.Util; +import hudson.XmlFile; import hudson.model.AbstractDescribableImpl; import hudson.model.Descriptor; +import hudson.util.HttpResponses; +import hudson.util.Secret; +import hudson.util.VersionNumber; import jenkins.model.Jenkins; +import jenkins.util.Timer; import net.sf.json.JSONObject; import org.acegisecurity.AccessDeniedException; import org.apache.commons.lang.StringUtils; @@ -41,26 +46,46 @@ import org.kohsuke.stapler.HttpResponse; import org.kohsuke.stapler.StaplerRequest; import org.kohsuke.stapler.StaplerResponse; +import org.kohsuke.stapler.interceptor.RequirePOST; +import org.xml.sax.Attributes; +import org.xml.sax.InputSource; +import org.xml.sax.Locator; +import org.xml.sax.SAXException; +import org.xml.sax.ext.Locator2; +import org.xml.sax.helpers.DefaultHandler; import javax.annotation.concurrent.GuardedBy; import javax.servlet.ServletException; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.parsers.SAXParserFactory; +import java.io.File; import java.io.IOException; +import java.io.InputStream; +import java.io.ObjectStreamException; import java.io.Serializable; import java.lang.reflect.Method; +import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.security.SecureRandom; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; /** * @author Stephen Connolly */ public class MetricsAccessKey extends AbstractDescribableImpl implements Serializable { private static final long serialVersionUID = 1L; + @Deprecated + private transient String key; @NonNull - private final String key; + private Secret secretKey; @CheckForNull private final String description; private final boolean canPing; @@ -75,15 +100,20 @@ public class MetricsAccessKey extends AbstractDescribableImpl private transient String[] originRegexs = null; public MetricsAccessKey(String description, String key) { - this(description, key, true, false, false, true, null); + this(description, Secret.fromString(key), true, false, false, true, null); } - @DataBoundConstructor + @Deprecated public MetricsAccessKey(String description, String key, boolean canPing, boolean canThreadDump, boolean canHealthCheck, boolean canMetrics, String origins) { + this(description, Secret.fromString(key), canPing, canThreadDump, canHealthCheck, canMetrics, origins); + } + + @DataBoundConstructor + public MetricsAccessKey(String description, Secret key, boolean canPing, boolean canThreadDump, + boolean canHealthCheck, boolean canMetrics, String origins) { this.description = Util.fixEmptyAndTrim(description); - key = Util.fixEmptyAndTrim(key); - this.key = key == null ? DescriptorImpl.generateKey() : key; + this.secretKey = key; this.canPing = canPing; this.canThreadDump = canThreadDump; this.canHealthCheck = canHealthCheck; @@ -177,8 +207,8 @@ public String getDescription() { } @NonNull - public String getKey() { - return key; + public Secret getKey() { + return secretKey; } public boolean isCanPing() { @@ -271,7 +301,7 @@ public boolean equals(Object o) { if (description != null ? !description.equals(that.description) : that.description != null) { return false; } - if (!key.equals(that.key)) { + if (!secretKey.equals(that.secretKey)) { return false; } if (origins != null ? !origins.equals(that.origins) : that.origins != null) { @@ -286,7 +316,7 @@ public boolean equals(Object o) { */ @Override public int hashCode() { - return key.hashCode(); + return secretKey.hashCode(); } /** @@ -295,7 +325,7 @@ public int hashCode() { @Override public String toString() { final StringBuilder sb = new StringBuilder("MetricsAccessKey{"); - sb.append("key='").append(key).append('\''); + sb.append("key='").append(StringUtils.isNotEmpty(Secret.toString(secretKey)) ? "****" : "NULL").append('\''); sb.append(", description='").append(description).append('\''); sb.append(", canPing=").append(canPing); sb.append(", canHealthCheck=").append(canHealthCheck); @@ -306,6 +336,22 @@ public String toString() { return sb.toString(); } + /** + * Called when object has been deserialized from a stream. + * + * @return {@code this}, or a replacement for {@code this}. + * @throws ObjectStreamException if the object cannot be restored. + * @see The Java Object Serialization Specification + */ + private Object readResolve() throws ObjectStreamException { + if (StringUtils.isNotEmpty(this.key)) { + this.secretKey = Secret.fromString(this.key); + this.key = null; + DescriptorImpl.keyConverted = true; + } + return this; + } + /** * An extension point that allows for plugins to provide their own set of access keys. */ @@ -330,19 +376,37 @@ public static class DescriptorImpl extends Descriptor { private static final SecureRandom entropy = new SecureRandom(); private static final char[] keyChars = "ABCEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_".toCharArray(); + /** + * Used by {@link #readResolve()} to indicate that a key has been converted from String to {@link Secret}. + */ + private static boolean keyConverted = false; @GuardedBy("this") private List accessKeys; private transient volatile Set accessKeySet; public DescriptorImpl() { super(); + keyIsNotConverted(); load(); + if (keyConverted) { + Logger.getLogger(MetricsAccessKey.class.getName()).info("Saving encrypted Metrics access key(s)"); + Timer.get().submit(this::save); + } + } + + /*package for testing*/ static boolean isKeyConverted() { + return keyConverted; + } + + private static void keyIsNotConverted() { + keyConverted = false; } @NonNull public static String generateKey() { - StringBuilder b = new StringBuilder(64); - for (int i = 0; i < 64; i++) { + final int keyLength = 64; + StringBuilder b = new StringBuilder(keyLength); + for (int i = 0; i < keyLength; i++) { b.append(keyChars[entropy.nextInt(keyChars.length)]); } return b.toString(); @@ -350,7 +414,13 @@ public static String generateKey() { @Override public synchronized boolean configure(StaplerRequest req, JSONObject json) throws FormException { - accessKeys = req.bindJSONToList(MetricsAccessKey.class, json.get("accessKeys")); + final List keys = req.bindJSONToList(MetricsAccessKey.class, json.get("accessKeys")); + for (MetricsAccessKey accessKey : keys) { + if (accessKey.getKey().getPlainText().isEmpty()) { + throw new FormException("Metrics access key cannot be empty. Make sure to generate it.", "accessKeys.key"); + } + } + this.accessKeys = keys; accessKeySet = null; save(); return true; @@ -366,29 +436,16 @@ public synchronized List getAccessKeys() { public void checkAccessKey(@CheckForNull String accessKey) { Set accessKeySet = this.accessKeySet; if (accessKeySet == null) { - accessKeySet = new HashSet(); - for (Provider p : ExtensionList.lookup(Provider.class)) { - for (MetricsAccessKey k : p.getAccessKeys()) { - accessKeySet.add(k.getKey()); - } - } - synchronized (this) { - if (accessKeys != null) { - for (MetricsAccessKey k : accessKeys) { - accessKeySet.add(k.getKey()); - } - } - this.accessKeySet = accessKeySet; // will be idempotent - } + reindexAccessKeys(); } - if (!accessKeySet.contains(accessKey)) { + if (accessKeySet != null && !accessKeySet.contains(accessKey)) { // slow check - for (Provider p : ExtensionList.lookup(Provider.class)) { - if (((!(p instanceof AbstractProvider) || ((AbstractProvider) p).isMayHaveOnDemandKeys()) - && p.getAccessKey(accessKey) != null)) { - return; - } + for (Provider p : ExtensionList.lookup(Provider.class)) { + if (((!(p instanceof AbstractProvider) || ((AbstractProvider) p).isMayHaveOnDemandKeys()) + && p.getAccessKey(accessKey) != null)) { + return; } + } throw new AccessDeniedException(Messages.MetricsAccessKey_invalidAccessKey(accessKey)); } } @@ -450,7 +507,7 @@ public MetricsAccessKey getAccessKey(String accessKey) { } synchronized (this) { for (MetricsAccessKey k : accessKeys) { - if (StringUtils.equals(accessKey, k.getKey())) { + if (StringUtils.equals(accessKey, Secret.toString(k.getKey()))) { return k; } } @@ -458,6 +515,14 @@ public MetricsAccessKey getAccessKey(String accessKey) { return null; } + @RequirePOST + public HttpResponse doGenerateNewToken() { + Jenkins.get().checkPermission(Jenkins.ADMINISTER); + + Map data = new HashMap<>(); + data.put("tokenValue", DescriptorImpl.generateKey()); + return HttpResponses.okJSON(data); + } /** * @@ -496,16 +561,16 @@ public String getDisplayName() { } public void reindexAccessKeys() { - Set accessKeySet = new HashSet(); + Set accessKeySet = new HashSet<>(); for (Provider p : ExtensionList.lookup(Provider.class)) { for (MetricsAccessKey k : p.getAccessKeys()) { - accessKeySet.add(k.getKey()); + accessKeySet.add(Secret.toString(k.getKey())); } } synchronized (this) { if (accessKeys != null) { for (MetricsAccessKey k : accessKeys) { - accessKeySet.add(k.getKey()); + accessKeySet.add(Secret.toString(k.getKey())); } } this.accessKeySet = accessKeySet; @@ -558,7 +623,7 @@ private boolean isMayHaveOnDemandKeys() { @CheckForNull public MetricsAccessKey getAccessKey(String accessKey) { for (MetricsAccessKey k : getAccessKeys()) { - if (StringUtils.equals(accessKey, k.getKey())) { + if (StringUtils.equals(accessKey, Secret.toString(k.getKey()))) { return k; } } diff --git a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.jelly b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.jelly index ebf93ea..9b55c13 100644 --- a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.jelly +++ b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.jelly @@ -23,23 +23,21 @@ --> - - - + + + + +
+ +
${%TokenDisplayedOnce}
+
+ + + + diff --git a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.properties b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.properties new file mode 100644 index 0000000..ab28086 --- /dev/null +++ b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.properties @@ -0,0 +1,4 @@ +GenerateNewToken=Generate a new token +TokenDisplayedOnce=Copy this token now, because it cannot be recovered in the future. +CopyToken=Copy the token value +NewTokenValueCopied=The new metrics access token has been copied diff --git a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/global.jelly b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/global.jelly index deb6597..eac84a6 100644 --- a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/global.jelly +++ b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/global.jelly @@ -24,8 +24,9 @@ - - + + + diff --git a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.css b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.css new file mode 100644 index 0000000..624893c --- /dev/null +++ b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.css @@ -0,0 +1,62 @@ +/* + * The MIT License + * + * Copyright (c) 2021, 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. + */ +.metrics-access-key-new-token-value { + width: 40%; + display: none; + font-family: monospace; + margin-right: 2px; + font-size: 1em; +} + +.metrics-access-key-new-token-value.visible { + display: block; +} + +.metrics-access-key-copy.copy-button { + float: right; +} + +.metrics-access-key-copy.copy-button .yui-button { + vertical-align: middle; +} + +.metrics-access-key-copy.copy-button button { + margin-left: 6px; +} + +.metrics-access-key-copy.invisible { + display: none; +} +.metrics-access-key-copy.visible { + display: block; +} + +.metrics-access-key-display-after-generation { + margin-top: 5px; + display: none; +} + +.metrics-access-key-display-after-generation.visible { + display: block; +} diff --git a/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.js b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.js new file mode 100644 index 0000000..82a1fbe --- /dev/null +++ b/src/main/resources/jenkins/metrics/api/MetricsAccessKey/resource.js @@ -0,0 +1,62 @@ +/* + * The MIT License + * + * Copyright (c) 2021, 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. + */ +function saveApiToken(button) { + if (button.hasClassName('request-pending')) { + return; + } + button.addClassName('request-pending'); + const repeatedChunk = button.up('.repeated-chunk'); + + const targetUrl = button.getAttribute('data-target-url'); + new Ajax.Request(targetUrl, { + method: 'post', + onSuccess: function (resp) { + const json = resp.responseJSON; + if (json.status === 'error') { + button.removeClassName('request-pending'); + } else { + const {tokenValue} = json.data; + // The visible part, so it can be copied + const tokenValueSpan = repeatedChunk.querySelector('span.metrics-access-key-new-token-value'); + tokenValueSpan.innerText = tokenValue; + tokenValueSpan.addClassName('visible'); + + // The input sent to save the configuration + repeatedChunk.querySelector('[name = "key"]').value = tokenValue; + + // Show the copy button + const tokenCopyButton = repeatedChunk.querySelector('.copy-button'); + tokenCopyButton.setAttribute('text', tokenValue); + tokenCopyButton.removeClassName('invisible') + tokenCopyButton.addClassName('visible'); + + // Show the warning message + repeatedChunk.querySelector('.metrics-access-key-display-after-generation') + .addClassName('visible'); + + button.remove(); + } + }, + }); +} diff --git a/src/test/java/jenkins/metrics/RoundTripJCascMetricsTest.java b/src/test/java/jenkins/metrics/RoundTripJCascMetricsTest.java index ff3739c..7e6c647 100644 --- a/src/test/java/jenkins/metrics/RoundTripJCascMetricsTest.java +++ b/src/test/java/jenkins/metrics/RoundTripJCascMetricsTest.java @@ -1,6 +1,7 @@ package jenkins.metrics; import hudson.ExtensionList; +import hudson.util.Secret; import io.jenkins.plugins.casc.misc.RoundTripAbstractTest; import jenkins.metrics.api.MetricsAccessKey; import org.jvnet.hudson.test.RestartableJenkinsRule; @@ -24,7 +25,7 @@ public void assertConfiguredAsExpected(RestartableJenkinsRule j, String configCo MetricsAccessKey accessKey = accessKeys.get(0); - assertEquals("tDdG5Vsv-2-WDdHfI3QFPiU9-hcvKmWd2HL4CfVIFvUumQzz3qf6c0qt_HU4_lUh", accessKey.getKey()); + assertEquals("tDdG5Vsv-2-WDdHfI3QFPiU9-hcvKmWd2HL4CfVIFvUumQzz3qf6c0qt_HU4_lUh", Secret.toString(accessKey.getKey())); assertEquals("JCasC key", accessKey.getDescription()); assertFalse(accessKey.isCanPing()); assertTrue(accessKey.isCanThreadDump()); @@ -35,6 +36,6 @@ public void assertConfiguredAsExpected(RestartableJenkinsRule j, String configCo @Override public String stringInLogExpected() { - return "MetricsAccessKey.key = tDdG5Vsv-2-WDdHfI3QFPiU9-hcvKmWd2HL4CfVIFvUumQzz3qf6c0qt_HU4_lUh"; + return "MetricsAccessKey.key = ****"; } } diff --git a/src/test/java/jenkins/metrics/api/MetricsAccessKeyTest.java b/src/test/java/jenkins/metrics/api/MetricsAccessKeyTest.java new file mode 100644 index 0000000..ce49e27 --- /dev/null +++ b/src/test/java/jenkins/metrics/api/MetricsAccessKeyTest.java @@ -0,0 +1,57 @@ +package jenkins.metrics.api; + +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.RuleChain; +import org.jvnet.hudson.test.Issue; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.LoggerRule; +import org.jvnet.hudson.test.RealJenkinsRule; +import org.jvnet.hudson.test.recipes.LocalData; + +import java.util.logging.Level; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasProperty; +import static org.hamcrest.Matchers.startsWith; +import static org.hamcrest.core.IsNot.not; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class MetricsAccessKeyTest { + public JenkinsRule j = new JenkinsRule(); + public LoggerRule l = new LoggerRule().record("jenkins.metrics.api.MetricsAccessKey", Level.ALL).capture(1000); + @Rule + public RuleChain chain = RuleChain.outerRule(j).around(l); + + @Test + @Issue("SECURITY-1624") + @LocalData + public void oldKeysAreConvertedAfterStartup() throws Exception { + j.waitUntilNoActivityUpTo(5000); + assertThat(l.getRecords(), hasItem( + allOf( + hasProperty("level", equalTo(Level.INFO)), + hasProperty("message", startsWith("Saving encrypted Metrics access key")) + ) + )); + assertTrue(MetricsAccessKey.DescriptorImpl.isKeyConverted()); + } + + @Test + @Issue("SECURITY-1624") + @LocalData + public void newKeysStayTheSame() throws Exception { + j.waitUntilNoActivityUpTo(5000); + assertThat(l.getRecords(), not(hasItem( + allOf( + hasProperty("level", equalTo(Level.INFO)), + hasProperty("message", startsWith("Saving encrypted Metrics access key")) + ) + ))); + assertFalse(MetricsAccessKey.DescriptorImpl.isKeyConverted()); + } +} diff --git a/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/newKeysStayTheSame/jenkins.metrics.api.MetricsAccessKey.xml b/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/newKeysStayTheSame/jenkins.metrics.api.MetricsAccessKey.xml new file mode 100644 index 0000000..9b51838 --- /dev/null +++ b/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/newKeysStayTheSame/jenkins.metrics.api.MetricsAccessKey.xml @@ -0,0 +1,14 @@ + + + + + hello + frt + true + true + false + true + * + + + \ No newline at end of file diff --git a/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/oldKeysAreConvertedAfterStartup/jenkins.metrics.api.MetricsAccessKey.xml b/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/oldKeysAreConvertedAfterStartup/jenkins.metrics.api.MetricsAccessKey.xml new file mode 100644 index 0000000..097ba24 --- /dev/null +++ b/src/test/resources/jenkins/metrics/api/MetricsAccessKeyTest/oldKeysAreConvertedAfterStartup/jenkins.metrics.api.MetricsAccessKey.xml @@ -0,0 +1,14 @@ + + + + + hello + frt + true + true + false + true + * + + + \ No newline at end of file