Skip to content

Commit

Permalink
[SECURITY-1624]
Browse files Browse the repository at this point in the history
  • Loading branch information
@rsandell @alecharp
rsandell authored and alecharp committed Dec 15, 2021
1 parent 06a8665 commit 9810480
Show file tree
Hide file tree
Showing 11 changed files with 340 additions and 62 deletions.
6 changes: 3 additions & 3 deletions src/main/java/jenkins/metrics/api/Metrics.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Expand All @@ -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();
}
Expand Down Expand Up @@ -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 "
Expand Down
141 changes: 103 additions & 38 deletions src/main/java/jenkins/metrics/api/MetricsAccessKey.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<MetricsAccessKey> 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;
Expand All @@ -75,15 +100,20 @@ public class MetricsAccessKey extends AbstractDescribableImpl<MetricsAccessKey>
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;
Expand Down Expand Up @@ -177,8 +207,8 @@ public String getDescription() {
}

@NonNull
public String getKey() {
return key;
public Secret getKey() {
return secretKey;
}

public boolean isCanPing() {
Expand Down Expand Up @@ -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) {
Expand All @@ -286,7 +316,7 @@ public boolean equals(Object o) {
*/
@Override
public int hashCode() {
return key.hashCode();
return secretKey.hashCode();
}

/**
Expand All @@ -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);
Expand All @@ -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 <a href="http://download.oracle.com/javase/1.3/docs/guide/serialization/spec/input.doc6.html">The Java Object Serialization Specification</a>
*/
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.
*/
Expand All @@ -330,27 +376,51 @@ public static class DescriptorImpl extends Descriptor<MetricsAccessKey> {
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<MetricsAccessKey> accessKeys;
private transient volatile Set<String> 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();
}

@Override
public synchronized boolean configure(StaplerRequest req, JSONObject json) throws FormException {
accessKeys = req.bindJSONToList(MetricsAccessKey.class, json.get("accessKeys"));
final List<MetricsAccessKey> 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;
Expand All @@ -366,29 +436,16 @@ public synchronized List<MetricsAccessKey> getAccessKeys() {
public void checkAccessKey(@CheckForNull String accessKey) {
Set<String> accessKeySet = this.accessKeySet;
if (accessKeySet == null) {
accessKeySet = new HashSet<String>();
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));
}
}
Expand Down Expand Up @@ -450,14 +507,22 @@ 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;
}
}
}
return null;
}

@RequirePOST
public HttpResponse doGenerateNewToken() {
Jenkins.get().checkPermission(Jenkins.ADMINISTER);

Map<String, Object> data = new HashMap<>();
data.put("tokenValue", DescriptorImpl.generateKey());
return HttpResponses.okJSON(data);
}

/**
*
Expand Down Expand Up @@ -496,16 +561,16 @@ public String getDisplayName() {
}

public void reindexAccessKeys() {
Set<String> accessKeySet = new HashSet<String>();
Set<String> 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;
Expand Down Expand Up @@ -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;
}
}
Expand Down
32 changes: 15 additions & 17 deletions src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,21 @@
-->
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:l="/lib/layout" xmlns:st="jelly:stapler">
<f:entry title="${%Key}" field="key">
<input class="setting-input metrics-access-key"
name="_.key"
value="${instance.key}"
type="text"
placeholder="${%Will be generated on save or click Generate to generate now}"
readonly="true" style="width: 80ex;"
onclick="this.select();"/>
<input type="button" value="${%Generate}..." onclick="(function(){
var src = event.target || event.srcElement;
var fld = findPreviousFormItem(src,'key');
var text = '';
var possible = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_';
for( var i=0; i &lt; 64; i++ )
text += possible.charAt(Math.floor(Math.random() * possible.length));
fld.value = text;
})(this); return false;" class="submit-button"/>
<f:invisibleEntry>
<f:password name="key" field="key"/>
</f:invisibleEntry>
<f:entry title="${%Key}">
<div>
<span class="metrics-access-key-new-token-value"><!-- For javascript --></span>
<div class="warning metrics-access-key-display-after-generation">${%TokenDisplayedOnce}</div>
</div>
<span class="yui-button">
<button type="button" tabindex="0" data-target-url="${descriptor.descriptorFullUrl}/generateNewToken"
onclick="saveApiToken(this)">
${%GenerateNewToken}
</button>
</span>
<l:copyButton message="${%NewTokenValueCopied}" clazz="metrics-access-key-copy invisible" tooltip="${%CopyToken}"/>
</f:entry>
<f:entry title="${%Description}" field="description">
<f:textbox/>
Expand Down
4 changes: 4 additions & 0 deletions src/main/resources/jenkins/metrics/api/MetricsAccessKey/config.properties
View file
Original file line number Diff line number Diff line change
@@ -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
5 changes: 3 additions & 2 deletions src/main/resources/jenkins/metrics/api/MetricsAccessKey/global.jelly
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@
<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:f="/lib/form" xmlns:l="/lib/layout" xmlns:st="jelly:stapler">
<f:section title="${%Metrics}">
<f:entry title="${%Access keys}" field="accessKeys">
<f:repeatableProperty field="accessKeys"/>
<st:adjunct includes="jenkins.metrics.api.MetricsAccessKey.resource"/>
<f:entry title="${%Access keys}" field="accessKeys" clazz="metrics-access-keys">
<f:repeatableProperty field="accessKeys" add="${%Add new access key}" />
</f:entry>
</f:section>
</j:jelly>
Loading

0 comments on commit 9810480

Please sign in to comment.