Skip to content

Commit

Permalink
HttpConnector.connect: request a function instead of fixed set of hea…
Browse files Browse the repository at this point in the history 
…ders

Fix the signature of HttpConnector.connect. As the connect method
handles redirects, we cannot have a fixed set of headers, once we
support authentication, as the authentication headers depend on
the host and that might change in a redirect.

This change fixes the header generation in the HttpDownloader,
but does not yet re-add the usage of the auth parmeter in the
Starlark-provided download functions.

Related #9327

Change-Id: Id12aa6a9a790fac6133a4da64baff58f02d36ef4
PiperOrigin-RevId: 269300472
  • Loading branch information
@aehlig @copybara-github
aehlig authored and copybara-github committed Sep 16, 2019
1 parent 25caae2 commit e38d838
Show file tree
Hide file tree
Showing 4 changed files with 130 additions and 88 deletions.
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.devtools.build.lib.bazel.repository.downloader;

import com.google.common.base.Ascii;
import com.google.common.base.Function;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
Expand Down Expand Up @@ -87,9 +88,8 @@ private int scale(int unscaled) {
return Math.round(unscaled * timeoutScaling);
}

URLConnection connect(
URL originalUrl, ImmutableMap<String, String> requestHeaders)
throws IOException {
URLConnection connect(URL originalUrl, Function<URL, ImmutableMap<String, String>> requestHeaders)
throws IOException {

if (Thread.interrupted()) {
throw new InterruptedIOException();
Expand All @@ -111,7 +111,7 @@ URLConnection connect(
COMPRESSED_EXTENSIONS.contains(HttpUtils.getExtension(url.getPath()))
|| COMPRESSED_EXTENSIONS.contains(HttpUtils.getExtension(originalUrl.getPath()));
connection.setInstanceFollowRedirects(false);
for (Map.Entry<String, String> entry : requestHeaders.entrySet()) {
for (Map.Entry<String, String> entry : requestHeaders.apply(url).entrySet()) {
if (isAlreadyCompressed && Ascii.equalsIgnoreCase(entry.getKey(), "Accept-Encoding")) {
// We're not going to ask for compression if we're downloading a file that already
// appears to be compressed.
Expand Down
52 changes: 33 additions & 19 deletions ...a/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

import static com.google.common.collect.ImmutableSortedSet.toImmutableSortedSet;

import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.base.Predicates;
import com.google.common.collect.ImmutableMap;
Expand Down Expand Up @@ -313,21 +314,29 @@ private void tellParentThreadWeAreDone() {
private HttpStream establishConnection(
final URL url, Optional<Checksum> checksum, Map<URI, Map<String, String>> additionalHeaders)
throws IOException {
ImmutableMap<String, String> headers = REQUEST_HEADERS;
try {
if (additionalHeaders.containsKey(url.toURI())) {
headers =
ImmutableMap.<String, String>builder()
.putAll(headers)
.putAll(additionalHeaders.get(url.toURI()))
.build();
}
} catch (URISyntaxException e) {
// If we can't convert the URL to a URI (because it is syntactically malformed), still try to
// do the connection, not adding authentication information as we cannot look it up.
}
final URLConnection connection = connector.connect(url, headers);
final Map<String, String> allHeaders = headers;
final Function<URL, ImmutableMap<String, String>> headerFunction =
new Function<URL, ImmutableMap<String, String>>() {
@Override
public ImmutableMap<String, String> apply(URL url) {
ImmutableMap<String, String> headers = REQUEST_HEADERS;
try {
if (additionalHeaders.containsKey(url.toURI())) {

This comment has been minimized.

Sign in to view

Copy link
@artem-zinnatullin

artem-zinnatullin Sep 17, 2019

Contributor

I think this results in different behavior from before and from intended one.

I'd assume that we'd want to append auth headers to same "host" within a redirect, this commit however does matching by entire URI which will change and we won't get auth headers at all after a redirect.

Consider following examples:

https://myserver.com/path1 -> https://myserver.com/path2
http://myserver.com/path1  -> https://myserver.com/path1

In similar CVE in CURL, they implemented same host policy, this commit implements same URI policy which I think won't work in real life redirects 🤔

See https://curl.haxx.se/docs/CVE-2018-1000007.html

I might be missing something though, wdyt?

This comment has been minimized.

Sign in to view

Copy link
@aehlig

aehlig Sep 17, 2019

Author Contributor

I think this results in different behavior from before and from intended one.

The behavior is certainly different than the one before, as we wanted to move away from the behavior that was considered a security issue. The behavior, however, is safe, as only credentials are sent explicitly authorized by the user for that URL.

I'm not sure what the "intended" behavior is, as ...

I'd assume that we'd want to append auth headers to same "host" within a redirect, this commit however does matching by entire URI which will change and we won't get auth headers at all after a redirect.

... a "same host" policy does not fit with the authentication interface for ctx.download that was the outcome of a lengthy committee meeting, as it allows requests like the following.

ctx.download(
  urls = ["https://hosting.example.com/user/foo/datarepo/data.txt",
         ],
  sha256 = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
  auth = {"https://hosting.example.com/user/foo/datarepo/data.txt" :
             {"type" : "basic",
              "login" : "alice",
              "password" : "mypassforfoo",
              },
           "https://hosting.example.com/user/bar/datarepo/data.txt" :
             {"type" : "basic",
              "login" : "alice",
              "password" : "mypassforbar",
              },
           })

Now, if https://hosting.example.com/user/foo/datarepo/data.txt redirects to https://hosting.example.com/user/bar/datarepo/data.txt, should the same-host credentials be used, or the provided ones? And what if urls contained both those URLs?

So, I think, if we want to go for a same-host policy, a new discussion about the bazel-provided interface would be necessary (making it a bazel 2.0 feature).
headers =
ImmutableMap.<String, String>builder()
.putAll(headers)
.putAll(additionalHeaders.get(url.toURI()))
.build();
}
} catch (URISyntaxException e) {
// If we can't convert the URL to a URI (because it is syntactically malformed), still
// try to
// do the connection, not adding authentication information as we cannot look it up.
}
return headers;
}
};

final URLConnection connection = connector.connect(url, headerFunction);
return httpStreamFactory.create(
connection,
url,
Expand All @@ -340,10 +349,15 @@ public URLConnection connect(Throwable cause, ImmutableMap<String, String> extra
Event.progress(String.format("Lost connection for %s due to %s", url, cause)));
return connector.connect(
connection.getURL(),
new ImmutableMap.Builder<String, String>()
.putAll(allHeaders)
.putAll(extraHeaders)
.build());
new Function<URL, ImmutableMap<String, String>>() {
@Override
public ImmutableMap<String, String> apply(URL url) {
return new ImmutableMap.Builder<String, String>()
.putAll(headerFunction.apply(url))
.putAll(extraHeaders)
.build();
}
});
}
});
}
Expand Down
79 changes: 41 additions & 38 deletions ...m/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;

import com.google.common.base.Function;
import com.google.common.base.Optional;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.bazel.repository.cache.RepositoryCache.KeyType;
import com.google.devtools.build.lib.bazel.repository.downloader.RetryingInputStream.Reconnector;
import com.google.devtools.build.lib.events.EventHandler;
Expand Down Expand Up @@ -97,9 +97,9 @@ public class HttpConnectorMultiplexerTest {

@Before
public void before() throws Exception {
when(connector.connect(eq(URL1), any(ImmutableMap.class))).thenReturn(connection1);
when(connector.connect(eq(URL2), any(ImmutableMap.class))).thenReturn(connection2);
when(connector.connect(eq(URL3), any(ImmutableMap.class))).thenReturn(connection3);
when(connector.connect(eq(URL1), any(Function.class))).thenReturn(connection1);
when(connector.connect(eq(URL2), any(Function.class))).thenReturn(connection2);
when(connector.connect(eq(URL3), any(Function.class))).thenReturn(connection3);
when(streamFactory.create(
same(connection1), any(URL.class), any(Optional.class), any(Reconnector.class)))
.thenReturn(stream1);
Expand Down Expand Up @@ -151,7 +151,7 @@ public void run() {
@Test
public void singleUrl_justCallsConnector() throws Exception {
assertThat(toByteArray(multiplexer.connect(asList(URL1), DUMMY_CHECKSUM))).isEqualTo(data1);
verify(connector).connect(eq(URL1), any(ImmutableMap.class));
verify(connector).connect(eq(URL1), any(Function.class));
verify(streamFactory)
.create(
any(URLConnection.class), any(URL.class), eq(DUMMY_CHECKSUM), any(Reconnector.class));
Expand All @@ -160,11 +160,11 @@ public void singleUrl_justCallsConnector() throws Exception {

@Test
public void multipleUrlsFail_throwsIOException() throws Exception {
when(connector.connect(any(URL.class), any(ImmutableMap.class))).thenThrow(new IOException());
when(connector.connect(any(URL.class), any(Function.class))).thenThrow(new IOException());
IOException e =
assertThrows(IOException.class, () -> multiplexer.connect(asList(URL1, URL2, URL3), null));
assertThat(e).hasMessageThat().contains("All mirrors are down");
verify(connector, times(3)).connect(any(URL.class), any(ImmutableMap.class));
verify(connector, times(3)).connect(any(URL.class), any(Function.class));
verify(sleeper, times(2)).sleepMillis(anyLong());
verifyNoMoreInteractions(sleeper, connector, streamFactory);
}
Expand All @@ -179,12 +179,12 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
return null;
}
}).when(sleeper).sleepMillis(anyLong());
when(connector.connect(eq(URL1), any(ImmutableMap.class))).thenThrow(new IOException());
when(connector.connect(eq(URL1), any(Function.class))).thenThrow(new IOException());
assertThat(toByteArray(multiplexer.connect(asList(URL1, URL2), DUMMY_CHECKSUM)))
.isEqualTo(data2);
assertThat(clock.currentTimeMillis()).isEqualTo(1000L);
verify(connector).connect(eq(URL1), any(ImmutableMap.class));
verify(connector).connect(eq(URL2), any(ImmutableMap.class));
verify(connector).connect(eq(URL1), any(Function.class));
verify(connector).connect(eq(URL2), any(Function.class));
verify(streamFactory)
.create(
any(URLConnection.class), any(URL.class), eq(DUMMY_CHECKSUM), any(Reconnector.class));
Expand All @@ -196,14 +196,15 @@ public Void answer(InvocationOnMock invocation) throws Throwable {
public void twoSuccessfulUrlsAndFirstWins_returnsFirstAndInterruptsSecond() throws Exception {
final CyclicBarrier barrier = new CyclicBarrier(2);
final AtomicBoolean wasInterrupted = new AtomicBoolean(true);
when(connector.connect(eq(URL1), any(ImmutableMap.class))).thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
return connection1;
}
});
when(connector.connect(eq(URL1), any(Function.class)))
.thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
return connection1;
}
});
doAnswer(
new Answer<Void>() {
@Override
Expand All @@ -225,26 +226,28 @@ public void parentThreadGetsInterrupted_interruptsChildrenThenThrowsIe() throws
final AtomicBoolean wasInterrupted1 = new AtomicBoolean(true);
final AtomicBoolean wasInterrupted2 = new AtomicBoolean(true);
final AtomicBoolean wasInterrupted3 = new AtomicBoolean(true);
when(connector.connect(eq(URL1), any(ImmutableMap.class))).thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
TimeUnit.MILLISECONDS.sleep(10000);
wasInterrupted1.set(false);
throw new RuntimeException();
}
});
when(connector.connect(eq(URL2), any(ImmutableMap.class))).thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
TimeUnit.MILLISECONDS.sleep(10000);
wasInterrupted2.set(false);
throw new RuntimeException();
}
});
when(connector.connect(eq(URL1), any(Function.class)))
.thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
TimeUnit.MILLISECONDS.sleep(10000);
wasInterrupted1.set(false);
throw new RuntimeException();
}
});
when(connector.connect(eq(URL2), any(Function.class)))
.thenAnswer(
new Answer<URLConnection>() {
@Override
public URLConnection answer(InvocationOnMock invocation) throws Throwable {
barrier.await();
TimeUnit.MILLISECONDS.sleep(10000);
wasInterrupted2.set(false);
throw new RuntimeException();
}
});
Thread task =
new Thread(
new Runnable() {
Expand Down
Loading

0 comments on commit e38d838

Please sign in to comment.