[FIXED JENKINS-16936] Added SecureRequester extension point.

jenkinsci · Oct 16, 2013 · acff331 · acff331
1 parent 587c6fe
commit acff331
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 6 deletions.
diff --git a/changelog.html b/changelog.html
@@ -58,6 +58,9 @@
   <li class='rfe'>
     Upgrade bundled plugin versions: ssh-slaves to 1.5, and credentials to 1.9.1
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-20071">issue 20071</a>)
+  <li class=rfe>
+    Extension point for secure users of REST APIs (permitting JSONP and primitive XPath).
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-16936">issue 16936</a>)
   <li class=bug>
     Integer overflow could cause JavaScript functions to break in long-running Jenkins processes.
     (<a href="https://issues.jenkins-ci.org/browse/JENKINS-20085">issue 20085</a>)

diff --git a/core/src/main/java/hudson/model/Api.java b/core/src/main/java/hudson/model/Api.java
@@ -25,6 +25,8 @@
 
 import hudson.util.IOException2;
 import jenkins.model.Jenkins;
+import jenkins.security.SecureRequester;
+
 import org.dom4j.CharacterData;
 import org.dom4j.Document;
 import org.dom4j.DocumentException;
@@ -59,6 +61,7 @@
  *
  * @author Kohsuke Kawaguchi
  * @see Exported
+ * @see SecureRequester
  */
 public class Api extends AbstractModelObject {
     /**
@@ -155,12 +158,12 @@ public void doXml(StaplerRequest req, StaplerResponse rsp,
         OutputStream o = rsp.getCompressedOutputStream(req);
         try {
             if (result instanceof CharacterData || result instanceof String || result instanceof Number || result instanceof Boolean) {
-                if (INSECURE) {
+                if (permit(req)) {
                     rsp.setContentType("text/plain;charset=UTF-8");
                     String text = result instanceof CharacterData ? ((CharacterData) result).getText() : result.toString();
                     o.write(text.getBytes("UTF-8"));
                 } else {
-                    rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "primitive XPath result sets forbidden; can use -Dhudson.model.Api.INSECURE=true if you run without security");
+                    rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "primitive XPath result sets forbidden; implement jenkins.security.SecureRequester");
                 }
                 return;
             }
@@ -188,11 +191,11 @@ public void doSchema(StaplerRequest req, StaplerResponse rsp) throws IOException
      * Exposes the bean as JSON.
      */
     public void doJson(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException {
-        if (INSECURE || req.getParameter("jsonp") == null) {
-        setHeaders(rsp);
+        if (req.getParameter("jsonp") == null || permit(req)) {
+            setHeaders(rsp);
             rsp.serveExposedBean(req,bean, Flavor.JSON);
         } else {
-            rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "jsonp forbidden; can use -Dhudson.model.Api.INSECURE=true if you run without security");
+            rsp.sendError(HttpURLConnection.HTTP_FORBIDDEN, "jsonp forbidden; implement jenkins.security.SecureRequester");
         }
     }
 
@@ -204,13 +207,21 @@ public void doPython(StaplerRequest req, StaplerResponse rsp) throws IOException
         rsp.serveExposedBean(req,bean, Flavor.PYTHON);
     }
 
+    private boolean permit(StaplerRequest req) {
+        for (SecureRequester r : Jenkins.getInstance().getExtensionList(SecureRequester.class)) {
+            if (r.permit(req, bean)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private void setHeaders(StaplerResponse rsp) {
         rsp.setHeader("X-Jenkins", Jenkins.VERSION);
         rsp.setHeader("X-Jenkins-Session", Jenkins.SESSION_HASH);
     }
 
     private static final Logger LOGGER = Logger.getLogger(Api.class.getName());
     private static final ModelBuilder MODEL_BUILDER = new ModelBuilder();
-    private static final boolean INSECURE = "true".equals(System.getProperty("hudson.model.Api.INSECURE"));
 
 }
diff --git a/core/src/main/java/jenkins/security/SecureRequester.java b/core/src/main/java/jenkins/security/SecureRequester.java
@@ -0,0 +1,50 @@
+package jenkins.security;
+
+import hudson.Extension;
+import hudson.ExtensionPoint;
+import hudson.model.Api;
+import java.util.logging.Logger;
+import jenkins.model.Jenkins;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+import org.kohsuke.stapler.StaplerRequest;
+
+/**
+ * An extension point for authorizing REST API access to an object where an unsafe result type would be produced.
+ * Both JSONP and XPath with primitive result sets are considered unsafe due to CSRF attacks.
+ * A default implementation allows requests if a deprecated system property is set, or if Jenkins is unsecured anyway,
+ * but plugins may offer implementations which authorize scripted clients, requests from inside a trusted domain, etc.
+ * @see Api
+ * @since 1.537
+ */
+public interface SecureRequester extends ExtensionPoint {
+
+    /**
+     * Checks if a Jenkins object can be accessed by a given REST request.
+     * For instance, if the {@link StaplerRequest#getReferer} matches a given host, or
+     * anonymous read is allowed for the given object.
+     * @param req a request going through the REST API
+     * @param bean an exported object of some kind
+     * @return true if this requester should be trusted, false to reject
+     */
+    boolean permit(StaplerRequest req, Object bean);
+
+    @Restricted(NoExternalUse.class)
+    @Extension class Default implements SecureRequester {
+
+        private static final String PROP = "hudson.model.Api.INSECURE";
+        private static final boolean INSECURE = Boolean.getBoolean(PROP);
+        static {
+            if (INSECURE) {
+                Logger.getLogger(SecureRequester.class.getName()).warning(PROP + " system property is deprecated; implement SecureRequester instead");
+            }
+        }
+
+        @Override public boolean permit(StaplerRequest req, Object bean) {
+            return INSECURE || !Jenkins.getInstance().isUseSecurity();
+        }
+
+    }
+
+}
diff --git a/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java b/test/src/test/java/hudson/security/csrf/DefaultCrumbIssuerTest.java
@@ -11,6 +11,7 @@
 import java.net.HttpURLConnection;
 import org.jvnet.hudson.test.Bug;
 import org.jvnet.hudson.test.HudsonTestCase;
+import org.jvnet.hudson.test.recipes.PresetData;
 
 /**
  *
@@ -97,6 +98,7 @@ public void testProxyCompatibilityMode() throws Exception {
         submit(p.getFormByName("config"));
    }
 
+    @PresetData(PresetData.DataSet.ANONYMOUS_READONLY)
     public void testApiXml() throws Exception {
         WebClient wc = new WebClient();
         assertXPathValue(wc.goToXml("crumbIssuer/api/xml"), "//crumbRequestField", jenkins.getCrumbIssuer().getCrumbRequestField());
@@ -116,6 +118,7 @@ public void testApiXml() throws Exception {
         wc.assertFails("crumbIssuer/api/xml?xpath=concat(//crumbRequestField,'=',//crumb)", HttpURLConnection.HTTP_FORBIDDEN); // perhaps interpretable as JS number
     }
 
+    @PresetData(PresetData.DataSet.ANONYMOUS_READONLY)
     public void testApiJson() throws Exception {
         WebClient wc = new WebClient();
         String json = wc.goTo("crumbIssuer/api/json", "application/json").getWebResponse().getContentAsString();