Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix reloading configuration from disk #545

Merged
merged 4 commits into from
Nov 27, 2023
Merged

Fix reloading configuration from disk #545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0899757
Fix reloading configuration from disk
Vlatombe Nov 22, 2023
fc513f0
Fix milestone and add logging
Vlatombe Nov 23, 2023
2ddc35a
Relax the log check
Vlatombe Nov 23, 2023
ed5b0bf
Clean up unused import
Vlatombe Nov 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 46 additions & 15 deletions src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -268,7 +269,7 @@
/** 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);
}

Expand Down Expand Up @@ -503,16 +504,12 @@
@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;
Expand All @@ -536,6 +533,40 @@
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();
Comment on lines +539 to +540
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is what is causing issues with the tests, most specifically with the hot plugin load.

} catch (IOException e) {
LOG.log(Level.WARNING, e, () -> "Failed to reconfigure ScriptApproval");

Check warning on line 542 in src/main/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApproval.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 541-542 are not covered by tests
}
} 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)
Expand Down Expand Up @@ -571,7 +602,7 @@
}

/** Nothing has ever been approved or is pending. */
boolean isEmpty() {
synchronized boolean isEmpty() {
return approvedScriptHashes.isEmpty() &&
approvedSignatures.isEmpty() &&
aclApprovedSignatures.isEmpty() &&
Expand Down Expand Up @@ -1198,7 +1229,7 @@

@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) {
Expand All @@ -1222,7 +1253,7 @@

@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) {
Expand All @@ -1234,7 +1265,7 @@

@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();
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/HasherScriptApprovalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.")));

Expand Down
23 changes: 23 additions & 0 deletions src/test/java/org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Comment on lines +190 to +192
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps besides the point since you approved the whole script anyway, and if you had not, you would also get an error on method jenkins.model.Jenkins getLabels, but OK.

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);
}
Expand Down
14 changes: 14 additions & 0 deletions ...org/jenkinsci/plugins/scriptsecurity/scripts/ScriptApprovalTest/reload/scriptApproval.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>
Loading