Merge pull request #555 from jglick/ApprovedWhitelist

Avoid race condition in `ApprovedWhitelist` reconfiguration
jenkinsci · Feb 2, 2024 · a73c079 · a73c079
2 parents 13ba786 + 83df236
commit a73c079
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 36 deletions.
diff --git a/Jenkinsfile b/Jenkinsfile
@@ -1,6 +1,8 @@
+configurations = [[platform: 'linux', jdk: 21]]
+if (env.CHANGE_ID == null) { // TODO https://github.com/jenkins-infra/helpdesk/issues/3931 workaround
+  configurations += [platform: 'windows', jdk: 17]
+}
 buildPlugin(
   useContainerAgent: true,
-  configurations: [
-    [platform: 'linux', jdk: 21],
-    [platform: 'windows', jdk: 17],
-])
+  configurations: configurations
+)
diff --git a/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java b/src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
@@ -58,7 +58,6 @@
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
@@ -76,7 +75,6 @@
 
 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
-import java.util.concurrent.atomic.AtomicBoolean;
 import jenkins.model.Jenkins;
 import net.sf.json.JSON;
 import org.jenkinsci.plugins.scriptsecurity.sandbox.Whitelist;
@@ -533,7 +531,11 @@ public synchronized void load() {
         if (changed) {
             save();
         }
-        ApprovedWhitelist.configurationChanged();
+        try {
+            configurationChanged();
+        } catch (IOException x) {
+            LOG.log(Level.SEVERE, "Malformed signature entry in scriptApproval.xml: '" + x.getMessage() + "'");
+        }
     }
 
     private void clear() {
@@ -967,32 +969,30 @@ public synchronized String[] getApprovedScriptHashes() {
         return approvedScriptHashes.toArray(new String[approvedScriptHashes.size()]);
     }
 
+    private synchronized void configurationChanged() throws IOException {
+        // Do not use lookupSingleton: ScriptApprovalLoadingTest.dynamicLoading
+        ApprovedWhitelist instance = ExtensionList.lookup(Whitelist.class).get(ApprovedWhitelist.class);
+        if (instance == null) {
+            throw new IllegalStateException("Failed to find ApprovedWhitelist");
+        }
+        LOG.fine("resetting");
+        synchronized (instance) {
+            instance.pendingDelegate = new AclAwareWhitelist(new StaticWhitelist(approvedSignatures), new StaticWhitelist(aclApprovedSignatures));
+        }
+    }
+
     @Restricted(NoExternalUse.class) // implementation
     @Extension public static final class ApprovedWhitelist extends ProxyWhitelist {
 
-        static void configurationChanged() {
-            // Do not use lookupSingleton: ScriptApprovalLoadingTest.dynamicLoading
-            ApprovedWhitelist instance = ExtensionList.lookup(Whitelist.class).get(ApprovedWhitelist.class);
-            if (instance == null) {
-                throw new IllegalStateException("Failed to find ApprovedWhitelist");
-            }
-            instance.initialized.set(false);
-        }
-
-        private final AtomicBoolean initialized = new AtomicBoolean();
+        private @CheckForNull Whitelist pendingDelegate;
 
-        @Override protected void beforePermits() {
-            if (initialized.compareAndSet(false, true)) {
-                try {
-                    ScriptApproval instance = ScriptApproval.get();
-                    Whitelist delegate;
-                    synchronized (instance) {
-                        delegate = new AclAwareWhitelist(new StaticWhitelist(instance.approvedSignatures), new StaticWhitelist(instance.aclApprovedSignatures));
-                    }
-                    reset(Set.of(delegate));
-                } catch (IOException e) {
-                    LOG.log(Level.SEVERE, "Malformed signature entry in scriptApproval.xml: '" + e.getMessage() + "'");
-                }
+        @Override protected synchronized void beforePermits() {
+            if (pendingDelegate != null) {
+                LOG.fine("refreshing");
+                reset(Set.of(pendingDelegate));
+                pendingDelegate = null;
+            } else {
+                LOG.finer("no need to refresh");
             }
         }
 
@@ -1136,8 +1136,8 @@ public Set<PendingSignature> getPendingSignatures() {
         return pendingSignatures;
     }
 
-    private String[][] reconfigure() throws IOException {
-        ApprovedWhitelist.configurationChanged();
+    private synchronized String[][] reconfigure() throws IOException {
+        configurationChanged();
         return new String[][] {getApprovedSignatures(), getAclApprovedSignatures(), getDangerousApprovedSignatures()};
     }
 

diff --git a/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java b/src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java
@@ -49,6 +49,7 @@
 import java.util.logging.Level;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItemInArray;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
@@ -57,7 +58,7 @@
 
 public class ScriptApprovalTest extends AbstractApprovalTest<ScriptApprovalTest.Script> {
     @Rule
-    public LoggerRule logging = new LoggerRule();
+    public LoggerRule logging = new LoggerRule().record(ScriptApproval.class, Level.FINER).capture(100);
 
     private static final String CLEAR_ALL_ID = "approvedScripts-clear";
 
@@ -75,11 +76,9 @@ public class ScriptApprovalTest extends AbstractApprovalTest<ScriptApprovalTest.
     @Test
     @LocalData("malformedScriptApproval")
     public void malformedScriptApproval() throws Exception {
-        logging.record(ScriptApproval.class, Level.FINER).capture(100);
         assertThat(Whitelist.all().permitsMethod(Jenkins.class.getMethod("get"), null, null), is(false));
-        assertThat(logging.getRecords(), Matchers.hasSize(Matchers.equalTo(1)));
-        assertEquals("Malformed signature entry in scriptApproval.xml: ' new java.lang.Exception java.lang.String'",
-                logging.getRecords().get(0).getMessage());
+        assertThat(logging.getRecords().stream().map(r -> r.getMessage()).toArray(String[]::new),
+            hasItemInArray("Malformed signature entry in scriptApproval.xml: ' new java.lang.Exception java.lang.String'"));
     }
 
     @Test @LocalData("dangerousApproved") public void dangerousApprovedSignatures() {