From d00f7ab9b62d1524bb286c4bb7be9274adae29cd Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Mon, 17 Mar 2014 12:48:24 -0400 Subject: [PATCH] Removed DownloadSettings.checkSignature; back to using a plain system property for this. (cherry picked from commit df9c92e461443415be06743f0f8fc0b3937f2408) --- core/src/main/java/hudson/PluginManager.java | 3 +- .../java/hudson/model/DownloadService.java | 11 ++++-- .../main/java/hudson/model/UpdateCenter.java | 3 +- .../main/java/hudson/model/UpdateSite.java | 2 +- .../{security => model}/DownloadSettings.java | 37 +------------------ .../hudson/PluginManager/advanced.jelly | 2 +- .../PageDecoratorImpl/footer.jelly | 2 +- .../model/DownloadSettings/config.groovy | 9 +++++ .../DownloadSettings/help-useBrowser.html | 0 .../security/DownloadSettings/config.groovy | 14 ------- .../DownloadSettings/help-checkSignature.html | 4 -- .../jenkins/security/Messages.properties | 4 +- .../hudson/model/DownloadServiceTest.java | 5 +-- 13 files changed, 26 insertions(+), 70 deletions(-) rename core/src/main/java/jenkins/{security => model}/DownloadSettings.java (66%) create mode 100644 core/src/main/resources/jenkins/model/DownloadSettings/config.groovy rename core/src/main/resources/jenkins/{security => model}/DownloadSettings/help-useBrowser.html (100%) delete mode 100644 core/src/main/resources/jenkins/security/DownloadSettings/config.groovy delete mode 100644 core/src/main/resources/jenkins/security/DownloadSettings/help-checkSignature.html diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java index 40f577f4e5..c444105b96 100644 --- a/core/src/main/java/hudson/PluginManager.java +++ b/core/src/main/java/hudson/PluginManager.java @@ -111,7 +111,6 @@ import static hudson.init.InitMilestone.*; import hudson.model.DownloadService; import hudson.util.FormValidation; import static java.util.logging.Level.WARNING; -import jenkins.security.DownloadSettings; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -783,7 +782,7 @@ public abstract class PluginManager extends AbstractModelObject implements OnMas @Restricted(NoExternalUse.class) @RequirePOST public HttpResponse doCheckUpdatesServer() throws IOException { for (UpdateSite site : Jenkins.getInstance().getUpdateCenter().getSites()) { - FormValidation v = site.updateDirectlyNow(DownloadSettings.get().isCheckSignature()); + FormValidation v = site.updateDirectlyNow(DownloadService.signatureCheck); if (v.kind != FormValidation.Kind.OK) { // TODO crude but enough for now return v; diff --git a/core/src/main/java/hudson/model/DownloadService.java b/core/src/main/java/hudson/model/DownloadService.java index 2e592d8222..bedf45db0f 100644 --- a/core/src/main/java/hudson/model/DownloadService.java +++ b/core/src/main/java/hudson/model/DownloadService.java @@ -37,7 +37,7 @@ import java.io.InputStream; import java.net.URL; import java.net.URLEncoder; import java.util.logging.Logger; -import jenkins.security.DownloadSettings; +import jenkins.model.DownloadSettings; import jenkins.model.Jenkins; import jenkins.util.JSONSignatureValidator; import net.sf.json.JSONException; @@ -302,7 +302,7 @@ public class DownloadService extends PageDecorator { private FormValidation load(String json, long dataTimestamp) throws IOException { JSONObject o = JSONObject.fromObject(json); - if (DownloadSettings.get().isCheckSignature()) { + if (signatureCheck) { FormValidation e = new JSONSignatureValidator("downloadable '"+id+"'").verifySignature(o); if (e.kind!= Kind.OK) { return e; @@ -346,7 +346,12 @@ public class DownloadService extends PageDecorator { public static boolean neverUpdate = Boolean.getBoolean(DownloadService.class.getName()+".never"); - /** Now used only to set default value of, and enable UI switching of, {@link DownloadSettings#setIgnoreSignature}. */ + /** + * May be used to temporarily disable signature checking on {@link DownloadService} and {@link UpdateCenter}. + * Useful when upstream signatures are broken, such as due to expired certificates. + * Should only be used when {@link DownloadSettings#isUseBrowser}; + * disabling signature checks for in-browser downloads is very dangerous as unprivileged users could submit spoofed metadata! + */ public static boolean signatureCheck = !Boolean.getBoolean(DownloadService.class.getName()+".noSignatureCheck"); } diff --git a/core/src/main/java/hudson/model/UpdateCenter.java b/core/src/main/java/hudson/model/UpdateCenter.java index b7ec3a4e30..040c288f75 100644 --- a/core/src/main/java/hudson/model/UpdateCenter.java +++ b/core/src/main/java/hudson/model/UpdateCenter.java @@ -88,7 +88,6 @@ import java.util.jar.JarFile; import java.util.logging.Level; import java.util.logging.Logger; import javax.annotation.CheckForNull; -import jenkins.security.DownloadSettings; import org.acegisecurity.context.SecurityContextHolder; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; @@ -631,7 +630,7 @@ public class UpdateCenter extends AbstractModelObject implements Saveable, OnMas public List updateAllSites() throws InterruptedException, ExecutionException { List > futures = new ArrayList>(); for (UpdateSite site : getSites()) { - Future future = site.updateDirectly(DownloadSettings.get().isCheckSignature()); + Future future = site.updateDirectly(DownloadService.signatureCheck); if (future != null) { futures.add(future); } diff --git a/core/src/main/java/hudson/model/UpdateSite.java b/core/src/main/java/hudson/model/UpdateSite.java index 1681aabab1..006b8a003b 100644 --- a/core/src/main/java/hudson/model/UpdateSite.java +++ b/core/src/main/java/hudson/model/UpdateSite.java @@ -56,7 +56,7 @@ import org.apache.commons.io.IOUtils; import javax.annotation.CheckForNull; import javax.annotation.Nonnull; import jenkins.model.Jenkins; -import jenkins.security.DownloadSettings; +import jenkins.model.DownloadSettings; import jenkins.util.JSONSignatureValidator; import net.sf.json.JSONException; import net.sf.json.JSONObject; diff --git a/core/src/main/java/jenkins/security/DownloadSettings.java b/core/src/main/java/jenkins/model/DownloadSettings.java similarity index 66% rename from core/src/main/java/jenkins/security/DownloadSettings.java rename to core/src/main/java/jenkins/model/DownloadSettings.java index 9961e64a06..0cf410c28b 100644 --- a/core/src/main/java/jenkins/security/DownloadSettings.java +++ b/core/src/main/java/jenkins/model/DownloadSettings.java @@ -22,24 +22,19 @@ * THE SOFTWARE. */ -package jenkins.security; +package jenkins.model; import hudson.Extension; -import hudson.PluginManager; import hudson.model.AsyncPeriodicWork; import hudson.model.DownloadService; import hudson.model.TaskListener; import hudson.model.UpdateSite; import hudson.util.FormValidation; import java.io.IOException; -import jenkins.model.GlobalConfiguration; -import jenkins.model.GlobalConfigurationCategory; -import jenkins.model.Jenkins; import net.sf.json.JSONObject; import org.kohsuke.accmod.Restricted; import org.kohsuke.accmod.restrictions.NoExternalUse; import org.kohsuke.stapler.HttpResponse; -import org.kohsuke.stapler.QueryParameter; import org.kohsuke.stapler.StaplerRequest; /** @@ -55,17 +50,11 @@ import org.kohsuke.stapler.StaplerRequest; } private boolean useBrowser = true; // historical default, not necessarily recommended - @SuppressWarnings("deprecation") - private boolean checkSignature = DownloadService.signatureCheck; public DownloadSettings() { load(); } - @Override public GlobalConfigurationCategory getCategory() { - return GlobalConfigurationCategory.get(GlobalConfigurationCategory.Security.class); - } - @Override public boolean configure(StaplerRequest req, JSONObject json) throws FormException { req.bindJSON(this, json); return true; @@ -80,30 +69,6 @@ import org.kohsuke.stapler.StaplerRequest; save(); } - public boolean isCheckSignature() { - return checkSignature; - } - - public void setCheckSignature(boolean checkSignature) { - if (!checkSignature) { - // Just to be on the safe side. Normally this is implied by ADMINISTER, needed to configure the security screen anyway, - // but in case ADMINISTER but not CONFIGURE_UPDATECENTER is somehow granted, make sure signature checking cannot be disabled. - Jenkins.getInstance().checkPermission(PluginManager.CONFIGURE_UPDATECENTER); - } - this.checkSignature = checkSignature; - save(); - } - - public FormValidation doCheckCheckSignature(@QueryParameter boolean value, @QueryParameter boolean useBrowser) { - if (value) { - return FormValidation.ok(); - } else if (useBrowser) { - return FormValidation.warningWithMarkup(Messages.DownloadSettings_disabling_signature_checks_for_in_browse()); - } else { - return FormValidation.warningWithMarkup(Messages.DownloadSettings_disabling_signature_checks_is_not_recomm()); - } - } - @Extension public static final class DailyCheck extends AsyncPeriodicWork { public DailyCheck() { diff --git a/core/src/main/resources/hudson/PluginManager/advanced.jelly b/core/src/main/resources/hudson/PluginManager/advanced.jelly index 79dc056fbc..b6ade3c2d5 100644 --- a/core/src/main/resources/hudson/PluginManager/advanced.jelly +++ b/core/src/main/resources/hudson/PluginManager/advanced.jelly @@ -96,7 +96,7 @@ THE SOFTWARE.
- +
${%lastUpdated(app.updateCenter.lastUpdatedString)} diff --git a/core/src/main/resources/hudson/model/UpdateCenter/PageDecoratorImpl/footer.jelly b/core/src/main/resources/hudson/model/UpdateCenter/PageDecoratorImpl/footer.jelly index 1751517592..777f1192c7 100644 --- a/core/src/main/resources/hudson/model/UpdateCenter/PageDecoratorImpl/footer.jelly +++ b/core/src/main/resources/hudson/model/UpdateCenter/PageDecoratorImpl/footer.jelly @@ -31,7 +31,7 @@ THE SOFTWARE. --> - + diff --git a/core/src/main/resources/jenkins/model/DownloadSettings/config.groovy b/core/src/main/resources/jenkins/model/DownloadSettings/config.groovy new file mode 100644 index 0000000000..9c8987f561 --- /dev/null +++ b/core/src/main/resources/jenkins/model/DownloadSettings/config.groovy @@ -0,0 +1,9 @@ +package jenkins.security.DownloadSettings + +def f = namespace(lib.FormTagLib) + +f.section(title: _("Download Preferences")) { + f.entry(title: _("Use Browser"), field: "useBrowser") { + f.checkbox() + } +} diff --git a/core/src/main/resources/jenkins/security/DownloadSettings/help-useBrowser.html b/core/src/main/resources/jenkins/model/DownloadSettings/help-useBrowser.html similarity index 100% rename from core/src/main/resources/jenkins/security/DownloadSettings/help-useBrowser.html rename to core/src/main/resources/jenkins/model/DownloadSettings/help-useBrowser.html diff --git a/core/src/main/resources/jenkins/security/DownloadSettings/config.groovy b/core/src/main/resources/jenkins/security/DownloadSettings/config.groovy deleted file mode 100644 index 90c81a0dab..0000000000 --- a/core/src/main/resources/jenkins/security/DownloadSettings/config.groovy +++ /dev/null @@ -1,14 +0,0 @@ -package jenkins.security.DownloadSettings - -def f = namespace(lib.FormTagLib) - -f.section(title: _("Download Preferences")) { - f.entry(title: _("Use Browser"), field: "useBrowser") { - f.checkbox() - } - if (!instance.checkSignature || !hudson.model.DownloadService.signatureCheck) { // do not display this option by default - f.entry(title: _("Check Signatures"), field: "checkSignature") { - f.checkbox() - } - } -} diff --git a/core/src/main/resources/jenkins/security/DownloadSettings/help-checkSignature.html b/core/src/main/resources/jenkins/security/DownloadSettings/help-checkSignature.html deleted file mode 100644 index d639bbaf78..0000000000 --- a/core/src/main/resources/jenkins/security/DownloadSettings/help-checkSignature.html +++ /dev/null @@ -1,4 +0,0 @@ -
- Uncheck to skip the signature check on downloaded metadata. - This is generally dangerous and should only be used as an emergency measure when dealing with broken upstream metadata. -
diff --git a/core/src/main/resources/jenkins/security/Messages.properties b/core/src/main/resources/jenkins/security/Messages.properties index 2c93ecc382..fc71097d03 100644 --- a/core/src/main/resources/jenkins/security/Messages.properties +++ b/core/src/main/resources/jenkins/security/Messages.properties @@ -22,6 +22,4 @@ ApiTokenProperty.DisplayName=API Token ApiTokenProperty.ChangeToken.Success=
Updated
-DownloadSettings.disabling_signature_checks_for_in_browse=Disabling signature checks for in-browser downloads is very dangerous as unprivileged users could submit spoofed metadata! -DownloadSettings.disabling_signature_checks_is_not_recomm=Disabling signature checks is not recommended except as a temporary measure when upstream metadata is broken, such as due to expired certificates. -RekeySecretAdminMonitor.DisplayName=Re-keying +RekeySecretAdminMonitor.DisplayName=Re-keying \ No newline at end of file diff --git a/test/src/test/java/hudson/model/DownloadServiceTest.java b/test/src/test/java/hudson/model/DownloadServiceTest.java index 50ff400070..0b01c00c9f 100644 --- a/test/src/test/java/hudson/model/DownloadServiceTest.java +++ b/test/src/test/java/hudson/model/DownloadServiceTest.java @@ -5,7 +5,6 @@ import java.io.IOException; import java.net.URL; import java.util.Set; import java.util.TreeSet; -import jenkins.security.DownloadSettings; import net.sf.json.JSONObject; import org.jvnet.hudson.test.Bug; import org.jvnet.hudson.test.HudsonTestCase; @@ -41,13 +40,13 @@ public class DownloadServiceTest extends HudsonTestCase { assertNull(job.getData()); // and now it should work - DownloadSettings.get().setCheckSignature(false); + DownloadService.signatureCheck = false; try { createWebClient().goTo("/self/testPost"); JSONObject d = job.getData(); assertEquals(hashCode(),d.getInt("hello")); } finally { - DownloadSettings.get().setCheckSignature(true); + DownloadService.signatureCheck = true; } // TODO: test with a signature