improved the signature validation code

core/src/main/java/hudson/model/UpdateSite.java

@@ -25,52 +25,53 @@
 
 package hudson.model;
 
-import hudson.PluginWrapper;
+import com.trilead.ssh2.crypto.Base64;
 import hudson.PluginManager;
-import hudson.model.UpdateCenter.UpdateCenterJob;
+import hudson.PluginWrapper;
 import hudson.lifecycle.Lifecycle;
+import hudson.model.UpdateCenter.UpdateCenterJob;
+import hudson.util.FormValidation;
+import hudson.util.FormValidation.Kind;
 import hudson.util.IOUtils;
 import hudson.util.TextFile;
 import hudson.util.VersionNumber;
-import static hudson.util.TimeUnit2.DAYS;
-
 import jenkins.model.Jenkins;
 import net.sf.json.JSONObject;
+import org.apache.commons.io.output.NullOutputStream;
+import org.apache.commons.io.output.TeeOutputStream;
+import org.jvnet.hudson.crypto.CertificateUtil;
+import org.jvnet.hudson.crypto.SignatureOutputStream;
 import org.kohsuke.stapler.DataBoundConstructor;
-import org.kohsuke.stapler.Stapler;
 import org.kohsuke.stapler.StaplerRequest;
 import org.kohsuke.stapler.StaplerResponse;
-import org.jvnet.hudson.crypto.CertificateUtil;
-import org.jvnet.hudson.crypto.SignatureOutputStream;
-import org.apache.commons.io.output.NullOutputStream;
-import org.apache.commons.io.output.TeeOutputStream;
 
+import javax.servlet.ServletContext;
+import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.IOException;
-import java.io.ByteArrayInputStream;
 import java.io.OutputStreamWriter;
+import java.security.DigestOutputStream;
+import java.security.GeneralSecurityException;
+import java.security.MessageDigest;
+import java.security.Signature;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.TrustAnchor;
+import java.security.cert.X509Certificate;
 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.TreeMap;
-import java.util.HashMap;
 import java.util.Set;
+import java.util.TreeMap;
 import java.util.concurrent.Future;
 import java.util.logging.Level;
 import java.util.logging.Logger;
-import java.security.GeneralSecurityException;
-import java.security.MessageDigest;
-import java.security.DigestOutputStream;
-import java.security.Signature;
-import java.security.cert.X509Certificate;
-import java.security.cert.CertificateFactory;
-import java.security.cert.TrustAnchor;
 
-import com.trilead.ssh2.crypto.Base64;
-
-import javax.servlet.ServletContext;
+import static hudson.util.TimeUnit2.*;
 
 
 /**
@@ -137,81 +138,97 @@ public class UpdateSite {
     /**
      * This is the endpoint that receives the update center data file from the browser.
      */
-    public void doPostBack(StaplerRequest req, StaplerResponse rsp) throws IOException, GeneralSecurityException {
+    public FormValidation doPostBack(StaplerRequest req) throws IOException, GeneralSecurityException {
         dataTimestamp = System.currentTimeMillis();
         String json = IOUtils.toString(req.getInputStream(),"UTF-8");
         JSONObject o = JSONObject.fromObject(json);
 
         int v = o.getInt("updateCenterVersion");
-        if(v !=1) {
-            LOGGER.warning("Unrecognized update center version: "+v);
-            return;
+        if(v !=1)
+            throw new IllegalArgumentException("Unrecognized update center version: "+v);
+
+        if (signatureCheck) {
+            FormValidation e = verifySignature(o);
+            if (e.kind!=Kind.OK)
+                LOGGER.severe(e.renderHtml());
+            return e;
         }
 
-        if (signatureCheck)
-            verifySignature(o);
-
         LOGGER.info("Obtained the latest update center data file for UpdateSource " + id);
         getDataFile().write(json);
-        rsp.setContentType("text/plain");  // So browser won't try to parse response
+        return FormValidation.ok();
+    }
+
+    public FormValidation doVerifySignature() throws IOException {
+        return verifySignature(getJSONObject());
     }
 
     /**
      * Verifies the signature in the update center data file.
      */
-    private boolean verifySignature(JSONObject o) throws GeneralSecurityException, IOException {
-        JSONObject signature = o.getJSONObject("signature");
-        if (signature.isNullObject()) {
-            LOGGER.severe("No signature block found");
-            return false;
-        }
-        o.remove("signature");
-
-        List<X509Certificate> certs = new ArrayList<X509Certificate>();
-        {// load and verify certificates
-            CertificateFactory cf = CertificateFactory.getInstance("X509");
-            for (Object cert : signature.getJSONArray("certificates")) {
-                X509Certificate c = (X509Certificate) cf.generateCertificate(new ByteArrayInputStream(Base64.decode(cert.toString().toCharArray())));
-                c.checkValidity();
-                certs.add(c);
+    private FormValidation verifySignature(JSONObject o) throws IOException {
+        try {
+            FormValidation warning = null;
+
+            JSONObject signature = o.getJSONObject("signature");
+            if (signature.isNullObject()) {
+                return FormValidation.error("No signature block found in update center '"+id+"'");
             }
+            o.remove("signature");
+
+            List<X509Certificate> certs = new ArrayList<X509Certificate>();
+            {// load and verify certificates
+                CertificateFactory cf = CertificateFactory.getInstance("X509");
+                for (Object cert : signature.getJSONArray("certificates")) {
+                    X509Certificate c = (X509Certificate) cf.generateCertificate(new ByteArrayInputStream(Base64.decode(cert.toString().toCharArray())));
+                    try {
+                        c.checkValidity();
+                    } catch (CertificateExpiredException e) { // even if the certificate isn't valid yet, we'll proceed it anyway
+                        warning = FormValidation.warning(e,String.format("Certificate %s has expired in update center '%s'",cert.toString(),id));
+                    } catch (CertificateNotYetValidException e) {
+                        warning = FormValidation.warning(e,String.format("Certificate %s is not yet valid in update center '%s'",cert.toString(),id));
+                    }
+                    certs.add(c);
+                }
 
-            // all default root CAs in JVM are trusted, plus certs bundled in Jenkins
-            Set<TrustAnchor> anchors = new HashSet<TrustAnchor>(); // CertificateUtil.getDefaultRootCAs();
-            ServletContext context = Jenkins.getInstance().servletContext;
-            for (String cert : (Set<String>) context.getResourcePaths("/WEB-INF/update-center-rootCAs")) {
-                if (cert.endsWith(".txt"))  continue;       // skip text files that are meant to be documentation
-                anchors.add(new TrustAnchor((X509Certificate)cf.generateCertificate(context.getResourceAsStream(cert)),null));
+                // all default root CAs in JVM are trusted, plus certs bundled in Jenkins
+                Set<TrustAnchor> anchors = new HashSet<TrustAnchor>(); // CertificateUtil.getDefaultRootCAs();
+                ServletContext context = Jenkins.getInstance().servletContext;
+                for (String cert : (Set<String>) context.getResourcePaths("/WEB-INF/update-center-rootCAs")) {
+                    if (cert.endsWith(".txt"))  continue;       // skip text files that are meant to be documentation
+                    anchors.add(new TrustAnchor((X509Certificate)cf.generateCertificate(context.getResourceAsStream(cert)),null));
+                }
+                CertificateUtil.validatePath(certs,anchors);
             }
-            CertificateUtil.validatePath(certs,anchors);
-        }
 
-        // this is for computing a digest to check sanity
-        MessageDigest sha1 = MessageDigest.getInstance("SHA1");
-        DigestOutputStream dos = new DigestOutputStream(new NullOutputStream(),sha1);
+            // this is for computing a digest to check sanity
+            MessageDigest sha1 = MessageDigest.getInstance("SHA1");
+            DigestOutputStream dos = new DigestOutputStream(new NullOutputStream(),sha1);
 
-        // this is for computing a signature
-        Signature sig = Signature.getInstance("SHA1withRSA");
-        sig.initVerify(certs.get(0));
-        SignatureOutputStream sos = new SignatureOutputStream(sig);
+            // this is for computing a signature
+            Signature sig = Signature.getInstance("SHA1withRSA");
+            sig.initVerify(certs.get(0));
+            SignatureOutputStream sos = new SignatureOutputStream(sig);
 
-        o.writeCanonical(new OutputStreamWriter(new TeeOutputStream(dos,sos),"UTF-8"));
+            o.writeCanonical(new OutputStreamWriter(new TeeOutputStream(dos,sos),"UTF-8"));
 
-        // did the digest match? this is not a part of the signature validation, but if we have a bug in the c14n
-        // (which is more likely than someone tampering with update center), we can tell
-        String computedDigest = new String(Base64.encode(sha1.digest()));
-        String providedDigest = signature.getString("digest");
-        if (!computedDigest.equalsIgnoreCase(providedDigest)) {
-            LOGGER.severe("Digest mismatch: "+computedDigest+" vs "+providedDigest);
-            return false;
-        }
+            // did the digest match? this is not a part of the signature validation, but if we have a bug in the c14n
+            // (which is more likely than someone tampering with update center), we can tell
+            String computedDigest = new String(Base64.encode(sha1.digest()));
+            String providedDigest = signature.getString("digest");
+            if (!computedDigest.equalsIgnoreCase(providedDigest)) {
+                return FormValidation.error("Digest mismatch: "+computedDigest+" vs "+providedDigest+" in update center '"+id+"'");
+            }
 
-        if (!sig.verify(Base64.decode(signature.getString("signature").toCharArray()))) {
-            LOGGER.severe("Signature in the update center doesn't match with the certificate");
-            return false;
-        }
+            if (!sig.verify(Base64.decode(signature.getString("signature").toCharArray()))) {
+                return FormValidation.error("Signature in the update center doesn't match with the certificate in update center '"+id+"'");
+            }
 
-        return true;
+            if (warning!=null)  return warning;
+            return FormValidation.ok();
+        } catch (GeneralSecurityException e) {
+            return FormValidation.error(e,"Signature verification failed in the update center '"+id+"'");
+        }
     }
 
     /**
@@ -233,10 +250,19 @@ public class UpdateSite {
      * @return  null if no data is available.
      */
     public Data getData() {
+        JSONObject o = getJSONObject();
+        if (o!=null)    return new Data(o);
+        return null;
+    }
+
+    /**
+     * Gets the raw update center JSON data.
+     */
+    public JSONObject getJSONObject() {
         TextFile df = getDataFile();
         if(df.exists()) {
             try {
-                return new Data(JSONObject.fromObject(df.read()));
+                return JSONObject.fromObject(df.read());
             } catch (IOException e) {
                 LOGGER.log(Level.SEVERE,"Failed to parse "+df,e);
                 df.delete(); // if we keep this file, it will cause repeated failures
@@ -246,7 +272,7 @@ public class UpdateSite {
             return null;
         }
     }
-    
+
     /**
      * Returns a list of plugins that should be shown in the "available" tab.
      * These are "all plugins - installed plugins".
@@ -662,5 +688,5 @@ public class UpdateSite {
     /**
      * Off by default until we know this is reasonably working.
      */
-    public static boolean signatureCheck = Boolean.getBoolean(UpdateCenter.class.getName()+".signatureCheck");
+    public static boolean signatureCheck = true; // Boolean.getBoolean(UpdateCenter.class.getName()+".signatureCheck");
 }