Merge pull request #545 from Vlatombe/fix-load

Fix reloading configuration from disk
jenkinsci · Nov 27, 2023 · 99333c0 · 99333c0
2 parents 48317e9 + ed5b0bf
commit 99333c0
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 18 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
@@ -25,6 +25,7 @@
 package org.jenkinsci.plugins.scriptsecurity.scripts;
 
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import hudson.init.InitMilestone;
 import hudson.model.BallColor;
 import hudson.model.PageDecorator;
 import hudson.security.ACLContext;
@@ -268,7 +269,7 @@ String hashClasspathEntry(URL entry) throws IOException {
     /** All external classpath entries allowed used for scripts. */
     private /*final*/ TreeSet<ApprovedClasspathEntry> approvedClasspathEntries;
 
-    /* for test */ void addApprovedClasspathEntry(ApprovedClasspathEntry acp) {
+    /* for test */ synchronized void addApprovedClasspathEntry(ApprovedClasspathEntry acp) {
         approvedClasspathEntries.add(acp);
     }
 
@@ -503,16 +504,12 @@ private PendingClasspathEntry getPendingClasspathEntry(@NonNull String hash) {
     @DataBoundConstructor
     public ScriptApproval() {
         load();
-        /* can be null when upgraded from old versions.*/
-        if (aclApprovedSignatures == null) {
-            aclApprovedSignatures = new TreeSet<>();
-        }
-        if (approvedClasspathEntries == null) {
-            approvedClasspathEntries = new TreeSet<>();
-        }
-        if (pendingClasspathEntries == null) {
-            pendingClasspathEntries = new TreeSet<>();
-        }
+    }
+
+    @Override
+    public synchronized void load() {
+        clear();
+        super.load();
         // Check for loaded class directories
         boolean changed = false;
         int dcp = 0;
@@ -536,6 +533,40 @@ public ScriptApproval() {
         if (changed) {
             save();
         }
+        // only call on subsequent load to avoid cycle
+        if (Jenkins.get().getInitLevel() == InitMilestone.COMPLETED) {
+            try {
+                LOG.log(Level.FINE, "Reconfiguring ScriptApproval after loading configuration from disk");
+                reconfigure();
+            } catch (IOException e) {
+                LOG.log(Level.WARNING, e, () -> "Failed to reconfigure ScriptApproval");
+            }
+        } else {
+            LOG.log(Level.FINE, "Skipping reconfiguration of ScriptApproval during Jenkins startup sequence");
+        }
+    }
+
+    private void clear() {
+        approvedScriptHashes.clear();
+        approvedSignatures.clear();
+        pendingScripts.clear();
+        pendingSignatures.clear();
+        /* can be null when upgraded from old versions.*/
+        if (aclApprovedSignatures == null) {
+            aclApprovedSignatures = new TreeSet<>();
+        } else {
+            aclApprovedSignatures.clear();
+        }
+        if (approvedClasspathEntries == null) {
+            approvedClasspathEntries = new TreeSet<>();
+        } else {
+            approvedClasspathEntries.clear();
+        }
+        if (pendingClasspathEntries == null) {
+            pendingClasspathEntries = new TreeSet<>();
+        } else {
+            pendingClasspathEntries.clear();
+        }
     }
 
     @Restricted(NoExternalUse.class)
@@ -571,7 +602,7 @@ public synchronized boolean hasDeprecatedApprovedClasspathHashes() {
     }
 
     /** Nothing has ever been approved or is pending. */
-    boolean isEmpty() {
+    synchronized boolean isEmpty() {
         return approvedScriptHashes.isEmpty() &&
                approvedSignatures.isEmpty() &&
                aclApprovedSignatures.isEmpty() &&
@@ -1201,7 +1232,7 @@ public JSON getClasspathRenderInfo() {
 
     @Restricted(NoExternalUse.class) // for use from AJAX
     @JavaScriptMethod
-    public JSON approveClasspathEntry(String hash) throws IOException {
+    public synchronized JSON approveClasspathEntry(String hash) throws IOException {
         Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         URL url = null;
         synchronized (this) {
@@ -1225,7 +1256,7 @@ public JSON approveClasspathEntry(String hash) throws IOException {
 
     @Restricted(NoExternalUse.class) // for use from AJAX
     @JavaScriptMethod
-    public JSON denyClasspathEntry(String hash) throws IOException {
+    public synchronized JSON denyClasspathEntry(String hash) throws IOException {
         Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         PendingClasspathEntry cp = getPendingClasspathEntry(hash);
         if (cp != null) {
@@ -1237,7 +1268,7 @@ public JSON denyClasspathEntry(String hash) throws IOException {
 
     @Restricted(NoExternalUse.class) // for use from AJAX
     @JavaScriptMethod
-    public JSON denyApprovedClasspathEntry(String hash) throws IOException {
+    public synchronized JSON denyApprovedClasspathEntry(String hash) throws IOException {
         Jenkins.get().checkPermission(Jenkins.ADMINISTER);
         if (approvedClasspathEntries.remove(new ApprovedClasspathEntry(hash, null))) {
             save();

diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/HasherScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/HasherScriptApprovalTest.java
@@ -15,9 +15,9 @@
 import java.util.logging.Level;
 
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsInRelativeOrder;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasItem;
 import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
@@ -51,7 +51,7 @@ public void warnsAndClearsDeprecatedScriptHashes() throws Throwable {
         session.then(r -> {
             final ScriptApproval approval = ScriptApproval.get();
             assertEquals(2, approval.countDeprecatedApprovedScriptHashes());
-            assertThat(log.getMessages(), containsInAnyOrder(
+            assertThat(log.getMessages(), hasItem(
                     containsString("There are 2 deprecated approved script hashes " +
                             "and 0 deprecated approved classpath hashes.")));
             approval.clearDeprecatedApprovedScripts();
@@ -102,7 +102,7 @@ public void testConvertApprovedClasspathEntries() throws Throwable {
             final ScriptApproval approval = ScriptApproval.get();
             assertEquals(2, approval.countDeprecatedApprovedClasspathHashes());
 
-            assertThat(log.getMessages(), containsInAnyOrder(
+            assertThat(log.getMessages(), hasItem(
                     containsString("There are 0 deprecated approved script hashes " +
                             "and 2 deprecated approved classpath hashes.")));
 

diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java
@@ -176,6 +176,29 @@ public void upgradeSmokes() throws Exception {
                 r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get()));
     }
 
+    @LocalData // Some scriptApproval.xml with existing signatures approved
+    @Test
+    public void reload() throws Exception {
+        configureSecurity();
+        ScriptApproval sa = ScriptApproval.get();
+
+        FreeStyleProject p = r.createFreeStyleProject();
+        p.getPublishersList().add(new TestGroovyRecorder(
+                new SecureGroovyScript("jenkins.model.Jenkins.instance", true, null)));
+        p.getPublishersList().add(new TestGroovyRecorder(
+                new SecureGroovyScript("println(jenkins.model.Jenkins.instance.getLabels())", false, null)));
+        r.assertLogNotContains("org.jenkinsci.plugins.scriptsecurity.sandbox.RejectedAccessException: "
+                        + "Scripts not permitted to use staticMethod jenkins.model.Jenkins getInstance",
+                r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get()));
+        r.assertLogNotContains("org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException: script not yet approved for use",
+                r.assertBuildStatus(Result.SUCCESS, p.scheduleBuild2(0).get()));
+
+        ScriptApproval.get().getConfigFile().delete();
+        sa.load();
+        r.assertLogContains("org.jenkinsci.plugins.scriptsecurity.scripts.UnapprovedUsageException: script not yet approved for use",
+                r.assertBuildStatus(Result.FAILURE, p.scheduleBuild2(0).get()));
+    }
+
     private Script script(String groovy) {
         return new Script(groovy);
     }

diff --git a/...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/reload/scriptApproval.xml b/...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/reload/scriptApproval.xml
@@ -0,0 +1,14 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<scriptApproval plugin="[email protected]">
+  <approvedScriptHashes>
+    <string>fccae58c5762bdd15daca97318e9d74333203106</string>
+  </approvedScriptHashes>
+  <approvedSignatures>
+    <string>staticMethod jenkins.model.Jenkins getInstance</string>
+  </approvedSignatures>
+  <aclApprovedSignatures/>
+  <approvedClasspathEntries/>
+  <pendingScripts/>
+  <pendingSignatures/>
+  <pendingClasspathEntries/>
+</scriptApproval>