Skip to content

[JENKINS-70870] Save libraries as JAR files rather than unpacked #57

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.

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

Draft
wants to merge 46 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
62562ac
Sketch of saving libraries as JAR files rather than unpacked
jglick Mar 3, 2023
c02ed47
Incremental deployment of https://github.com/jenkinsci/workflow-cps-p…
jglick Mar 3, 2023
e5e95d0
Avoiding an extraneous empty dir
jglick Mar 3, 2023
8598f5e
Nicer name for temp checkout dir
jglick Mar 3, 2023
10155ea
Simpler handling of `LoadedClasses.srcUrl`; no apparent need for `can…
jglick Mar 3, 2023
0a2ed44
e5e95d0d2f92f5091f1646915f82a88e7e2a5590 & 8598f5e4e032a64c48ae41df12…
jglick Mar 3, 2023
fec1f4e
Support for resuming builds
jglick Mar 3, 2023
3cd3102
Fixing `ResourceStep`
jglick Mar 3, 2023
e2b6276
Clearer name for `GLOBAL_LIBRARIES_DIR`
jglick Mar 3, 2023
f9be590
Switching `*-name.txt` to a manifest attribute; and fixes related to …
jglick Mar 3, 2023
a14ed45
Giving up on https://github.com/jenkinsci/workflow-cps-plugin/pull/66…
jglick Mar 4, 2023
1783fb0
May as well pick up https://github.com/jenkinsci/workflow-cps-plugin/…
jglick Mar 6, 2023
3062a97
Comment obsolete as of a14ed45
jglick Mar 6, 2023
859dad1
Properly migrating running builds crossing the update line
jglick Mar 6, 2023
6ba9d43
Restoring SECURITY-2479 defense (SECURITY-2476 is no longer vulnerabl…
jglick Mar 6, 2023
fa82612
`SCMSourceRetrieverTest.lease` highlighted that `dir2Jar` was imprope…
jglick Mar 6, 2023
75f2cae
Incremental build of https://github.com/jenkinsci/workflow-cps-plugin…
jglick Mar 6, 2023
5bbb1ed
Restoring support for `INCLUDE_SRC_TEST_IN_LIBRARIES`, as well as war…
jglick Mar 6, 2023
0a7ccb4
Adapting `cloneModeExcludeSrcTest` to revised message
jglick Mar 6, 2023
20729d2
Fixing `vars/*.txt`
jglick Mar 6, 2023
29a8848
`safeSymlinks` broken by requirement to have at least one source file
jglick Mar 6, 2023
af8414f
Fixing `LoadedLibraries`, though not yet actual replay
jglick Mar 6, 2023
894fbbb
https://github.com/jenkinsci/workflow-cps-plugin/pull/672 released
jglick Mar 6, 2023
38e609a
Removing comment satisfied by 859dad14fa0893fae45aecbd5532ed178c9f0d87
jglick Mar 6, 2023
2c181cc
Fixing replay
jglick Mar 6, 2023
f5b0866
Fixing `LibraryCachingConfigurationTest.clearCache`
jglick Mar 6, 2023
80395ee
`LibraryAdderTest.parallelBuildsDontInterfereWithExpiredCache` simila…
jglick Mar 6, 2023
c2c711e
Similarly `ResourceStepTest.cachingRefresh`
jglick Mar 6, 2023
81ca5a6
`symlinksInLibraryResourcesAreNotAllowedToEscapeWorkspaceContext` att…
jglick Mar 6, 2023
d1f3aca
SpotBugs
jglick Mar 6, 2023
840cd60
`LibraryCachingCleanup` was unreliable as it sometimes deleted the la…
jglick Mar 6, 2023
0aa22f4
Merged `doClone` back into `doRetrieve` for clarity
jglick Mar 6, 2023
743db17
Introduced `SCMBasedRetriever`, permitting `SCMRetriever` to support …
jglick Mar 6, 2023
444bde7
Also need `class="${descriptor.clazz}"`
jglick Mar 7, 2023
992eeaa
Skip new symlink unit tests on Windows
jglick Mar 7, 2023
b7bd774
Pick up https://github.com/jenkinsci/workflow-cps-plugin/pull/673
jglick Mar 7, 2023
10623dd
Also recommend `CloneOption.noTags`
jglick Mar 8, 2023
698ae68
`CloneOption.honorRefspec` can also be useful
jglick Mar 8, 2023
77aea3e
Migrate `LoadedClasses.srcUrl`, and switching persisted field to `Lib…
jglick Mar 9, 2023
2fb8ed9
SpotBugs
jglick Mar 9, 2023
6acba02
https://github.com/jenkinsci/workflow-cps-plugin/pull/673 released
jglick Mar 9, 2023
23125c9
Also delete `lastReadFile` when clearing cache
jglick Mar 13, 2023
68d2a33
Merge branch 'SCMSourceRetriever.clone' into dir2Jar
jglick Mar 24, 2023
d2c5eff
Merge branch 'dir2Jar' of https://github.com/jglick/pipeline-groovy-l…
jglick Mar 24, 2023
157d6f0
Removing some minor differences to `SCMSourceRetriever.clone`
jglick Mar 24, 2023
d608664
Merge branch 'master' of https://github.com/jenkinsci/pipeline-groovy…
jglick Apr 11, 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
6 changes: 6 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,12 @@
<scope>import</scope>
<type>pom</type>
</dependency>
<!-- TODO pending inclusion in BOM -->
<dependency>
<groupId>org.jenkins-ci.plugins.workflow</groupId>
<artifactId>workflow-cps</artifactId>
<version>3637.v63b_c17e0ed5b_</version>
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,31 +1,40 @@
package org.jenkinsci.plugins.workflow.cps.global;

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import groovy.lang.Binding;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import org.apache.commons.io.FileUtils;
import jenkins.model.Jenkins;
import org.apache.commons.io.IOUtils;
import org.codehaus.groovy.control.MultipleCompilationErrorsException;
import org.jenkinsci.plugins.workflow.cps.CpsCompilationErrorsException;
import org.jenkinsci.plugins.workflow.cps.CpsScript;
import org.jenkinsci.plugins.workflow.cps.CpsThread;
import org.jenkinsci.plugins.workflow.cps.GlobalVariable;

import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import java.io.File;
import java.io.IOException;
import jenkins.model.Jenkins;

/**
* Global variable backed by user-supplied script.
*
* @author Kohsuke Kawaguchi
*/
// not @Extension because these are instantiated programmatically
public class UserDefinedGlobalVariable extends GlobalVariable {
private final File help;
private final URI help;
private final String name;

/**
* @deprecated use {@link #UserDefinedGlobalVariable(String, URI)}
*/
@Deprecated
public UserDefinedGlobalVariable(String name, File help) {
this(name, help.toURI());
}

public UserDefinedGlobalVariable(String name, URI help) {
this.name = name;
this.help = help;
}
Expand Down Expand Up @@ -69,12 +78,14 @@ public Object getValue(@NonNull CpsScript script) throws Exception {
* Loads help from user-defined file, if available.
*/
public @CheckForNull String getHelpHtml() throws IOException {
if (!help.exists()) return null;

return Jenkins.get().getMarkupFormatter().translate(
FileUtils.readFileToString(help, StandardCharsets.UTF_8).
// Util.escape translates \n but not \r, and we do not know what platform the library will be checked out on:
replace("\r\n", "\n"));
try {
return Jenkins.get().getMarkupFormatter().translate(
Copy link
Member Author

Choose a reason for hiding this comment

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

(suppress whitespace changes when reviewing)

IOUtils.toString(help, StandardCharsets.UTF_8).
// Util.escape translates \n but not \r, and we do not know what platform the library will be checked out on:
replace("\r\n", "\n"));
} catch (FileNotFoundException x) {
return null;
}
}

@Override
Expand Down
172 changes: 89 additions & 83 deletions src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryAdder.java

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.io.IOException;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import jenkins.model.Jenkins;

import jenkins.util.SystemProperties;

Expand All @@ -27,41 +28,37 @@ public LibraryCachingCleanup() {

@Override protected void execute(TaskListener listener) throws IOException, InterruptedException {
FilePath globalCacheDir = LibraryCachingConfiguration.getGlobalLibrariesCacheDir();
for (FilePath library : globalCacheDir.list()) {
if (!removeIfExpiredCacheDirectory(library)) {
// Prior to the SECURITY-2586 fix, library caches had a two-level directory structure.
// These caches will never be used again, so we delete any that we find.
for (FilePath version: library.list()) {
if (version.child(LibraryCachingConfiguration.LAST_READ_FILE).exists()) {
library.deleteRecursive();
break;
}
}
}
for (FilePath libJar : globalCacheDir.list("*.jar")) {
removeIfExpiredCacheJar(libJar, listener);
}
// Old cache directory; format has changed, so just delete it:
Jenkins.get().getRootPath().child("global-libraries-cache").deleteRecursive();
}

/**
* Delete the specified cache directory if it is outdated.
* @return true if specified directory is a cache directory, regardless of whether it was outdated. Used to detect
* whether the cache was created before or after the fix for SECURITY-2586.
* Delete the specified cache JAR if it is outdated.
*/
private boolean removeIfExpiredCacheDirectory(FilePath library) throws IOException, InterruptedException {
final FilePath lastReadFile = new FilePath(library, LibraryCachingConfiguration.LAST_READ_FILE);
if (lastReadFile.exists()) {
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(library.getName());
private void removeIfExpiredCacheJar(FilePath libJar, TaskListener listener) throws IOException, InterruptedException {
final FilePath lastReadFile = libJar.sibling(libJar.getBaseName() + "." + LibraryCachingConfiguration.LAST_READ_FILE);
if (lastReadFile != null && lastReadFile.exists()) {
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(libJar.getBaseName());
retrieveLock.writeLock().lockInterruptibly();
try {
if (System.currentTimeMillis() - lastReadFile.lastModified() > TimeUnit.DAYS.toMillis(EXPIRE_AFTER_READ_DAYS)) {

library.deleteRecursive();
library.withSuffix("-name.txt").delete();
listener.getLogger().println("Deleting " + libJar);
try {
libJar.delete();
} finally {
lastReadFile.delete();
}
} else {
listener.getLogger().println(lastReadFile + " is sufficiently recent");
}
} finally {
retrieveLock.writeLock().unlock();
}
return true;
} else {
listener.getLogger().println("No such file " + lastReadFile);
}
return false;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,15 @@
import org.kohsuke.stapler.QueryParameter;

import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.jar.JarFile;
import java.util.logging.Level;
import java.util.logging.Logger;

import org.apache.commons.io.IOUtils;
import org.apache.commons.lang.StringUtils;

public final class LibraryCachingConfiguration extends AbstractDescribableImpl<LibraryCachingConfiguration> {
Expand All @@ -31,7 +29,7 @@ public final class LibraryCachingConfiguration extends AbstractDescribableImpl<L
private String excludedVersionsStr;

private static final String VERSIONS_SEPARATOR = " ";
public static final String GLOBAL_LIBRARIES_DIR = "global-libraries-cache";
public static final String GLOBAL_LIBRARIES_DIR = "pipeline-groovy-lib-cache";
public static final String LAST_READ_FILE = "last_read";

@DataBoundConstructor public LibraryCachingConfiguration(int refreshTimeMinutes, String excludedVersionsStr) {
Expand Down Expand Up @@ -94,26 +92,22 @@ public FormValidation doClearCache(@QueryParameter String name, @QueryParameter

try {
if (LibraryCachingConfiguration.getGlobalLibrariesCacheDir().exists()) {
for (FilePath libraryNamePath : LibraryCachingConfiguration.getGlobalLibrariesCacheDir().list("*-name.txt")) {
for (FilePath libraryCachePath : LibraryCachingConfiguration.getGlobalLibrariesCacheDir().list("*.jar")) {
// Libraries configured in distinct locations may have the same name. Since only admins are allowed here, this is not a huge issue, but it is probably unexpected.
String cacheName;
try (InputStream stream = libraryNamePath.read()) {
cacheName = IOUtils.toString(stream, StandardCharsets.UTF_8);
try (JarFile jf = new JarFile(libraryCachePath.getRemote())) {
cacheName = jf.getManifest().getMainAttributes().getValue(LibraryRetriever.ATTR_LIBRARY_NAME);
}
if (cacheName.equals(name)) {
FilePath libraryCachePath = LibraryCachingConfiguration.getGlobalLibrariesCacheDir()
.child(libraryNamePath.getName().replace("-name.txt", ""));
if (forceDelete) {
LOGGER.log(Level.FINER, "Force deleting cache for {0}", name);
libraryCachePath.deleteRecursive();
libraryNamePath.delete();
libraryCachePath.delete();
} else {
LOGGER.log(Level.FINER, "Safe deleting cache for {0}", name);
ReentrantReadWriteLock retrieveLock = LibraryAdder.getReadWriteLockFor(libraryCachePath.getName());
if (retrieveLock.writeLock().tryLock(10, TimeUnit.SECONDS)) {
try {
libraryCachePath.deleteRecursive();
libraryNamePath.delete();
libraryCachePath.delete();
} finally {
retrieveLock.writeLock().unlock();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ public String getName() {
}

/**
* Returns a partially unique name that can be safely used as a directory name.
* Returns a partially unique name that can be safely used as a directory or JAR base name.
*
* Uniqueness is based on the library name, version, whether it is trusted, and the source of the library.
* {@link LibraryRetriever}-specific information such as the SCM is not used to produce this name.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,128 @@
import hudson.util.FormValidation;
import edu.umd.cs.findbugs.annotations.CheckForNull;
import edu.umd.cs.findbugs.annotations.NonNull;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import hudson.slaves.WorkspaceList;
import hudson.util.DirScanner;
import hudson.util.FileVisitor;
import hudson.util.io.ArchiverFactory;
import java.io.File;
import java.io.IOException;
import java.io.OutputStream;
import java.nio.file.Path;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.jar.Attributes;
import java.util.jar.JarFile;
import java.util.jar.Manifest;

/**
* A way in which a library can be physically obtained for use in a build.
*/
public abstract class LibraryRetriever extends AbstractDescribableImpl<LibraryRetriever> implements ExtensionPoint {

/**
* JAR manifest attribute giving original library name.
*/
static final String ATTR_LIBRARY_NAME = "Jenkins-Library-Name";

@SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Non-final for write access via the Script Console")
public static boolean INCLUDE_SRC_TEST_IN_LIBRARIES = Boolean.getBoolean(SCMSourceRetriever.class.getName() + ".INCLUDE_SRC_TEST_IN_LIBRARIES");

/**
* Obtains library sources.
* @param name the {@link LibraryConfiguration#getName}
* @param version the version of the library, such as from {@link LibraryConfiguration#getDefaultVersion} or an override
* @param changelog whether to include changesets in the library in jobs using it from {@link LibraryConfiguration#getIncludeInChangesets}
* @param target a JAR file in which to stash sources; should contain {@code **}{@code /*.groovy} (sources at top level will be considered vars), and optionally also {@code resources/}
* @param run a build which will use the library
* @param listener a way to report progress
* @throws Exception if there is any problem (use {@link AbortException} for user errors)
*/
public void retrieveJar(@NonNull String name, @NonNull String version, boolean changelog, @NonNull FilePath target, @NonNull Run<?,?> run, @NonNull TaskListener listener) throws Exception {
if (Util.isOverridden(LibraryRetriever.class, getClass(), "retrieve", String.class, String.class, boolean.class, FilePath.class, Run.class, TaskListener.class)) {
FilePath tmp = target.sibling(target.getBaseName() + "-checkout");
if (tmp == null) {
throw new IOException();
}
try {
retrieve(name, version, changelog, tmp, run, listener);
dir2Jar(name, tmp, target, listener);
} finally {
tmp.deleteRecursive();
FilePath tmp2 = WorkspaceList.tempDir(tmp);
if (tmp2 != null) {
tmp2.deleteRecursive();
}
}
} else {
throw new AbstractMethodError("Implement retrieveJar");
}
}

/**
* Translates a historical directory with {@code src/} and/or {@code vars/} and/or {@code resources/} subdirectories
* into a JAR file with Groovy in classpath orientation and {@code resources/} as a ZIP folder.
*/
static void dir2Jar(@NonNull String name, @NonNull FilePath dir, @NonNull FilePath jar, @NonNull TaskListener listener) throws IOException, InterruptedException {
lookForBadSymlinks(dir, dir);
FilePath mf = jar.withSuffix(".mf");
try {
try (OutputStream os = mf.write()) {
Manifest m = new Manifest();
m.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0");
// Informational debugging aid, since the hex JAR basename will be meaningless:
m.getMainAttributes().putValue(ATTR_LIBRARY_NAME, name);
m.write(os);
}
try (OutputStream os = jar.write()) {
dir.archive(ArchiverFactory.ZIP, os, new DirScanner() {
@Override public void scan(File dir, FileVisitor visitor) throws IOException {
scanSingle(new File(mf.getRemote()), JarFile.MANIFEST_NAME, visitor);
String excludes;
if (!INCLUDE_SRC_TEST_IN_LIBRARIES && new File(dir, "src/test").isDirectory()) {
excludes = "test/";
listener.getLogger().println("Excluding src/test/ so that library test code cannot be accessed by Pipelines.");
listener.getLogger().println("To remove this log message, move the test code outside of src/. To restore the previous behavior that allowed access to files in src/test/, pass -D" + SCMSourceRetriever.class.getName() + ".INCLUDE_SRC_TEST_IN_LIBRARIES=true to the java command used to start Jenkins.");
} else {
excludes = null;
}
AtomicBoolean found = new AtomicBoolean();
FileVisitor verifyingVisitor = visitor.with(pathname -> {
if (pathname.getName().endsWith(".groovy")) {
found.set(true);
}
return true;
});
new DirScanner.Glob("**/*.groovy", excludes).scan(new File(dir, "src"), verifyingVisitor);
new DirScanner.Glob("*.groovy,*.txt", null).scan(new File(dir, "vars"), verifyingVisitor);
if (!found.get()) {
throw new AbortException("Library " + name + " expected to contain at least one of src or vars directories");
}
new DirScanner.Glob("resources/", null).scan(dir, visitor);
}
});
}
} finally {
mf.delete();
}
}

private static void lookForBadSymlinks(FilePath root, FilePath dir) throws IOException, InterruptedException {
for (FilePath child : dir.list()) {
if (child.isDirectory()) {
lookForBadSymlinks(root, child);
} else {
String link = child.readLink();
if (link != null) {
Path target = Path.of(dir.getRemote(), link).toRealPath();
if (!target.startsWith(Path.of(root.getRemote()))) {
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

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

Does this behave as expected if JENKINS_HOME itself is a symlink? See for example jenkinsci/workflow-cps-global-lib-plugin#139. Also, toRealPath will throw an exception if the link target does not exist, which may be undesirable from a security standpoint.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does this behave as expected if JENKINS_HOME itself is a symlink?

No idea offhand; I suppose we have no test coverage for such cases.

toRealPath will throw an exception if the link target does not exist, which may be undesirable

I think it is fine. If there is no such target, we want to fail one way or the other.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose we have no test coverage for such cases.

I don't think so. Based on the number of reports I got from the related security fix though, it is a surprisingly common scenario. Safest to use File.getCanonicalFile / File.getCanonicalPath on both the link and root.

If there is no such target, we want to fail one way or the other.

Yeah, but the interesting thing about toRealPath vs just comparing the canonical paths is that toRealPath allows an attacker to probe the existence of arbitrary files on the controller's file system.

throw new SecurityException(child + " → " + target + " is not inside " + root);
}
}
}
}
}

/**
* Obtains library sources.
* @param name the {@link LibraryConfiguration#getName}
Expand All @@ -51,7 +167,14 @@ public abstract class LibraryRetriever extends AbstractDescribableImpl<LibraryRe
* @param listener a way to report progress
* @throws Exception if there is any problem (use {@link AbortException} for user errors)
*/
public abstract void retrieve(@NonNull String name, @NonNull String version, boolean changelog, @NonNull FilePath target, @NonNull Run<?,?> run, @NonNull TaskListener listener) throws Exception;
@Deprecated
public void retrieve(@NonNull String name, @NonNull String version, boolean changelog, @NonNull FilePath target, @NonNull Run<?,?> run, @NonNull TaskListener listener) throws Exception {
if (Util.isOverridden(LibraryRetriever.class, getClass(), "retrieve", String.class, String.class, FilePath.class, Run.class, TaskListener.class)) {
retrieve(name, version, target, run, listener);
} else {
throw new AbstractMethodError("Implement retrieveJar");
}
}

/**
* Obtains library sources.
Expand All @@ -62,8 +185,10 @@ public abstract class LibraryRetriever extends AbstractDescribableImpl<LibraryRe
* @param listener a way to report progress
* @throws Exception if there is any problem (use {@link AbortException} for user errors)
*/
// TODO this should have been made nonabstract and deprecated and delegated to the new version; may be able to use access-modifier to help
public abstract void retrieve(@NonNull String name, @NonNull String version, @NonNull FilePath target, @NonNull Run<?,?> run, @NonNull TaskListener listener) throws Exception;
@Deprecated
public void retrieve(@NonNull String name, @NonNull String version, @NonNull FilePath target, @NonNull Run<?,?> run, @NonNull TaskListener listener) throws Exception {
throw new AbstractMethodError("Implement retrieveJar");
}

@Deprecated
public FormValidation validateVersion(@NonNull String name, @NonNull String version) {
Expand Down
Loading