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

[JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP servers #8814

Merged
merged 7 commits into from
Jan 2, 2024
Merged

[JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP servers #8814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9922cfd
[JENKINS-72469] Avoid repeated tool downloads from misconfigured HTTP…
MarkEWaite Dec 31, 2023
cd0487e
Use equals instead of contains to check ETag
MarkEWaite Dec 31, 2023
df05da5
Use weak comparison for ETag values
MarkEWaite Dec 31, 2023
cc50570
Test comparison of weak and strong validators
MarkEWaite Dec 31, 2023
b068176
Do not duplicate test args, more readable
MarkEWaite Jan 1, 2024
056ec5d
Use better variable names in test
MarkEWaite Jan 1, 2024
aa3578d
Fix variable declaration order
MarkEWaite Jan 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 40 additions & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import static hudson.Util.fileToPath;
import static hudson.Util.fixEmpty;
import static hudson.Util.fixEmptyAndTrim;

import com.google.common.annotations.VisibleForTesting;
import com.jcraft.jzlib.GZIPInputStream;
Expand Down Expand Up @@ -962,7 +963,7 @@
* </ul>
*
* @param archive
* The resource that represents the tgz/zip file. This URL must support the {@code Last-Modified} header.
* The resource that represents the tgz/zip file. This URL must support the {@code Last-Modified} header or the {@code ETag} header.
* (For example, you could use {@link ClassLoader#getResource}.)
* @param listener
* If non-null, a message will be printed to this listener once this method decides to
Expand All @@ -984,12 +985,18 @@
try {
FilePath timestamp = this.child(".timestamp");
long lastModified = timestamp.lastModified();
// https://httpwg.org/specs/rfc9110.html#field.etag is the ETag specification
// Read previously stored ETag if timestamp is available
String etag = timestamp.exists() ? fixEmptyAndTrim(timestamp.readToString()) : null;
URLConnection con;
try {
con = ProxyConfiguration.open(archive);
if (lastModified != 0) {
con.setIfModifiedSince(lastModified);
}
if (etag != null) {
con.setRequestProperty("If-None-Match", etag);
}
con.connect();
} catch (IOException x) {
if (this.exists()) {
Expand All @@ -1016,7 +1023,7 @@
return false;
}
}
if (lastModified != 0) {
if (lastModified != 0 || etag != null) {
if (responseCode == HttpURLConnection.HTTP_NOT_MODIFIED) {
return false;
} else if (responseCode != HttpURLConnection.HTTP_OK) {
Expand All @@ -1027,8 +1034,12 @@
}

long sourceTimestamp = con.getLastModified();
String resultEtag = fixEmptyAndTrim(con.getHeaderField("ETag"));

if (this.exists()) {
if (equalETags(etag, resultEtag)) {
return false; // already up to date
}
if (lastModified != 0 && sourceTimestamp == lastModified)
return false; // already up to date
this.deleteContents();
Expand All @@ -1042,6 +1053,10 @@
// First try to download from the agent machine.
try {
act(new Unpack(archive));
if (resultEtag != null && !equalETags(etag, resultEtag)) {
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");

Check warning on line 1058 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 1056-1058 are not covered by tests
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException x) {
Expand All @@ -1061,13 +1076,36 @@
throw new IOException(String.format("Failed to unpack %s (%d bytes read of total %d)",
archive, cis.getByteCount(), con.getContentLength()), e);
}
if (resultEtag != null && !equalETags(etag, resultEtag)) {

Check warning on line 1079 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1079 is only partially covered, one branch is missing
/* Store the ETag value in the timestamp file for later use */
timestamp.write(resultEtag, "UTF-8");
}
timestamp.touch(sourceTimestamp);
return true;
} catch (IOException e) {
throw new IOException("Failed to install " + archive + " to " + remote, e);
}
}

/* Return true if etag1 equals etag2 as defined by the etag specification
https://httpwg.org/specs/rfc9110.html#field.etag
*/
private boolean equalETags(String etag1, String etag2) {
if (etag1 == null || etag2 == null) {

Check warning on line 1094 in core/src/main/java/hudson/FilePath.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Partially covered line 

Line 1094 is only partially covered, one branch is missing
return false;
}
if (etag1.equals(etag2)) {
return true;
}
/* Weak tags are identified by leading characters "W/" as a marker */
/* Weak tag marker must not be considered in tag comparison.
This implements the weak comparison in the specification at
https://httpwg.org/specs/rfc9110.html#field.etag */
String opaqueTag1 = etag1.startsWith("W/") ? etag1.substring(2) : etag1;
String opaqueTag2 = etag2.startsWith("W/") ? etag2.substring(2) : etag2;
return opaqueTag1.equals(opaqueTag2);
}

// this reads from arbitrary URL
private static final class Unpack extends MasterToSlaveFileCallable<Void> {
private final URL archive;
Expand Down
71 changes: 71 additions & 0 deletions core/src/test/java/hudson/FilePathTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,77 @@ private static void assertValidateAntFileMask(String expected, FilePath d, Strin
assertTrue(d.installIfNecessaryFrom(url, null, message));
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongValidator() throws Exception {
String strongValidator = "\"An-ETag-strong-validator\"";
installIfNecessaryWithoutLastModified(strongValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongValidatorNoQuotes() throws Exception {
// This ETag is a violation of the spec at https://httpwg.org/specs/rfc9110.html#field.etag
// However, better safe to handle without quotes as well, just in case
String strongValidator = "An-ETag-strong-validator-without-quotes";
installIfNecessaryWithoutLastModified(strongValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedWeakValidator() throws Exception {
String weakValidator = "W/\"An-ETag-weak-validator\"";
installIfNecessaryWithoutLastModified(weakValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedStrongAndWeakValidators() throws Exception {
String strongValidator = "\"An-ETag-validator\"";
String weakValidator = "W/" + strongValidator;
installIfNecessaryWithoutLastModified(strongValidator, weakValidator);
}

@Issue("JENKINS-72469")
@Test public void installIfNecessaryWithoutLastModifiedWeakAndStrongValidators() throws Exception {
String strongValidator = "\"An-ETag-validator\"";
String weakValidator = "W/" + strongValidator;
installIfNecessaryWithoutLastModified(weakValidator, strongValidator);
}

private void installIfNecessaryWithoutLastModified(String validator) throws Exception {
installIfNecessaryWithoutLastModified(validator, validator);
}

private void installIfNecessaryWithoutLastModified(String validator, String alternateValidator) throws Exception {
final HttpURLConnection con = mock(HttpURLConnection.class);
// getLastModified == 0 when last-modified header is not returned
when(con.getLastModified()).thenReturn(0L);
// An Etag is provided by Azul CDN without last-modified header
when(con.getHeaderField("ETag")).thenReturn(validator);
when(con.getInputStream()).thenReturn(someZippedContent());

final URL url = someUrlToZipFile(con);

File tmp = temp.getRoot();
final FilePath d = new FilePath(tmp);

/* Initial download expected to occur */
assertTrue(d.installIfNecessaryFrom(url, null, "message if failed first download"));

/* Timestamp last modified == 0 means the header was not provided */
assertThat(d.child(".timestamp").lastModified(), is(0L));

/* Second download should not occur if JENKINS-72469 is fixed and NOT_MODIFIED is returned */
when(con.getResponseCode()).thenReturn(HttpURLConnection.HTTP_NOT_MODIFIED);
when(con.getInputStream()).thenReturn(someZippedContent());
when(con.getHeaderField("ETag")).thenReturn(alternateValidator);
assertFalse(d.installIfNecessaryFrom(url, null, "message if failed second download"));

/* Third download should not occur if JENKINS-72469 is fixed and OK is returned with matching ETag */
/* Unexpected to receive an OK and a matching ETag from a real web server, but check for safety */
when(con.getResponseCode()).thenReturn(HttpURLConnection.HTTP_OK);
when(con.getInputStream()).thenReturn(someZippedContent());
when(con.getHeaderField("ETag")).thenReturn(alternateValidator);
assertFalse(d.installIfNecessaryFrom(url, null, "message if failed third download"));
}

private URL someUrlToZipFile(final URLConnection con) throws IOException {

final URLStreamHandler urlHandler = new URLStreamHandler() {
Expand Down