From fc63914426300b5b8f11ac36506689ae7de4e61f Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Tue, 10 Sep 2024 11:06:28 +0200 Subject: [PATCH] GrpcRemoteDownloader: optionally propagate credentials to remote server In a multi-tenancy server deployment setup, the clients might want to treat the remote downloader server as a pull-through proxy and use it to download from private storage systems. Currently, we do support it via --remote_downloader_headers. However this scheme does not apply to the specific URL, while credentials and authentication could sometimes be host/domain specific. Add a flag to let users opt-in to credentials propagation to the remote server. This is off by default as not all remote servers can be trusted. When the flag is enabled, URL-specific credentials from Netrc or a custom credentials helper can be propagated to the remote server. The server implementation needs to support the new http_header_url:: qualifier where the url-index is a 0-based position of the URL inside the FetchBlobRequest's uris field. This new qualifier is modeled after the existing http_header qualifier. --- .../downloader/GrpcRemoteDownloader.java | 40 ++++++++- .../lib/remote/options/RemoteOptions.java | 11 +++ .../build/lib/remote/downloader/BUILD | 1 + .../downloader/GrpcRemoteDownloaderTest.java | 88 ++++++++++++++++++- 4 files changed, 136 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java index 12350ebdca5b7c..3671e64f6d0bda 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java @@ -43,6 +43,7 @@ import io.grpc.StatusRuntimeException; import java.io.IOException; import java.io.OutputStream; +import java.net.URISyntaxException; import java.net.URL; import java.util.List; import java.util.Map; @@ -83,6 +84,9 @@ public class GrpcRemoteDownloader implements AutoCloseable, Downloader { // delimit the qualifier prefix which denotes an HTTP header qualifer from the // header name itself. private static final String QUALIFIER_HTTP_HEADER_PREFIX = "http_header:"; + // Same as HTTP_HEADER_PREFIX, but only apply for a specific URL. + // The index starts from 0 and corresponds to the URL index in the request. + private static final String QUALIFIER_HTTP_HEADER_URL_PREFIX = "http_header_url:"; public GrpcRemoteDownloader( String buildRequestId, @@ -135,7 +139,14 @@ public void download( final FetchBlobRequest request = newFetchBlobRequest( - options.remoteInstanceName, urls, checksum, canonicalId, digestFunction, headers); + options.remoteInstanceName, + options.remoteDownloaderPropagateCredentials, + urls, + checksum, + canonicalId, + digestFunction, + headers, + credentials); try { FetchBlobResponse response = retrier.execute( @@ -180,17 +191,40 @@ public void download( @VisibleForTesting static FetchBlobRequest newFetchBlobRequest( String instanceName, + boolean remoteDownloaderPropagateCredentials, List urls, Optional checksum, String canonicalId, DigestFunction.Value digestFunction, - Map> headers) { + Map> headers, + Credentials credentials) + throws IOException { FetchBlobRequest.Builder requestBuilder = FetchBlobRequest.newBuilder() .setInstanceName(instanceName) .setDigestFunction(digestFunction); - for (URL url : urls) { + for (int i = 0; i < urls.size(); i++) { + var url = urls.get(i); requestBuilder.addUris(url.toString()); + + if (!remoteDownloaderPropagateCredentials) { + continue; + } + + try { + var metadata = credentials.getRequestMetadata(url.toURI()); + for (var entry : metadata.entrySet()) { + for (var value : entry.getValue()) { + requestBuilder.addQualifiers( + Qualifier.newBuilder() + .setName(QUALIFIER_HTTP_HEADER_URL_PREFIX + i + ":" + entry.getKey()) + .setValue(value) + .build()); + } + } + } catch (URISyntaxException e) { + throw new IOException(e); + } } if (checksum.isPresent()) { diff --git a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java index d88aea890df295..3219faf1a35d42 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java +++ b/src/main/java/com/google/devtools/build/lib/remote/options/RemoteOptions.java @@ -147,6 +147,17 @@ public final class RemoteOptions extends CommonRemoteOptions { help = "Whether to fall back to the local downloader if remote downloader fails.") public boolean remoteDownloaderLocalFallback; + @Option( + name = "experimental_remote_downloader_propagate_credentials", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.REMOTE, + effectTags = {OptionEffectTag.UNKNOWN}, + help = + "Whether to propagate credentials from netrc and credential helper to the remote" + + " downloader server. This requires the server implementation to support Bazel's" + + " `http_header_url` Qualifier.") + public boolean remoteDownloaderPropagateCredentials; + @Option( name = "remote_header", converter = Converters.AssignmentConverter.class, diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD index d49a5aa46aff52..c24c29f46e15f3 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/BUILD @@ -35,6 +35,7 @@ java_library( "//src/test/java/com/google/devtools/build/lib/remote/util", "//src/test/java/com/google/devtools/build/lib/testutil", "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", + "//third_party:auth", "//third_party:guava", "//third_party:jsr305", "//third_party:junit4", diff --git a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java index dbacc73c918c66..08ab269ae2882c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloaderTest.java @@ -17,10 +17,12 @@ import static com.google.common.truth.Truth.assertThat; import static com.google.devtools.build.lib.remote.util.Utils.getFromFuture; import static java.nio.charset.StandardCharsets.UTF_8; +import static java.util.Collections.singletonList; import static org.junit.Assert.assertThrows; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import build.bazel.remote.asset.v1.FetchBlobRequest; import build.bazel.remote.asset.v1.FetchBlobResponse; @@ -29,6 +31,7 @@ import build.bazel.remote.execution.v2.Digest; import build.bazel.remote.execution.v2.RequestMetadata; import build.bazel.remote.execution.v2.ServerCapabilities; +import com.google.auth.Credentials; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.io.ByteStreams; @@ -69,6 +72,7 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -348,6 +352,7 @@ public void testFetchBlobRequest() throws Exception { FetchBlobRequest request = GrpcRemoteDownloader.newFetchBlobRequest( "instance name", + false, ImmutableList.of( new URL("http://example.com/a"), new URL("http://example.com/b"), @@ -359,7 +364,8 @@ public void testFetchBlobRequest() throws Exception { DIGEST_UTIL.getDigestFunction(), ImmutableMap.of( "Authorization", ImmutableList.of("Basic Zm9vOmJhcg=="), - "X-Custom-Token", ImmutableList.of("foo", "bar"))); + "X-Custom-Token", ImmutableList.of("foo", "bar")), + StaticCredentials.EMPTY); assertThat(request) .isEqualTo( @@ -385,4 +391,84 @@ public void testFetchBlobRequest() throws Exception { .setValue("foo,bar")) .build()); } + + @Test + public void testFetchBlobRequest_withCredentialsPropagation() throws Exception { + var shouldPropagateCredentials = true; + var url = new URL("http://example.com/a"); + + Credentials credentials = mock(Credentials.class); + when(credentials.hasRequestMetadata()).thenReturn(true); + Map> headers = new HashMap<>(); + headers.put("CredKey", singletonList("CredValue")); + when(credentials.getRequestMetadata(url.toURI())).thenReturn(headers); + + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + shouldPropagateCredentials, + ImmutableList.of(url), + Optional.of( + Checksum.fromSubresourceIntegrity( + "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")), + "canonical ID", + DIGEST_UTIL.getDigestFunction(), + ImmutableMap.of(), + credentials); + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .addUris("http://example.com/a") + .addQualifiers( + Qualifier.newBuilder().setName("http_header_url:0:CredKey").setValue("CredValue")) + .addQualifiers( + Qualifier.newBuilder() + .setName("checksum.sri") + .setValue("sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")) + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .build()); + } + + @Test + public void testFetchBlobRequest_withoutCredentialsPropagation() throws Exception { + var shouldPropagateCredentials = false; + var url = new URL("http://example.com/a"); + + Credentials credentials = mock(Credentials.class); + when(credentials.hasRequestMetadata()).thenReturn(true); + Map> headers = new HashMap<>(); + headers.put("CredKey", singletonList("CredValue")); + when(credentials.getRequestMetadata(url.toURI())).thenReturn(headers); + + FetchBlobRequest request = + GrpcRemoteDownloader.newFetchBlobRequest( + "instance name", + shouldPropagateCredentials, + ImmutableList.of(url), + Optional.of( + Checksum.fromSubresourceIntegrity( + "sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")), + "canonical ID", + DIGEST_UTIL.getDigestFunction(), + ImmutableMap.of(), + credentials); + + assertThat(request) + .isEqualTo( + FetchBlobRequest.newBuilder() + .setInstanceName("instance name") + .setDigestFunction(DIGEST_UTIL.getDigestFunction()) + .addUris("http://example.com/a") + .addQualifiers( + Qualifier.newBuilder() + .setName("checksum.sri") + .setValue("sha256-AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")) + .addQualifiers( + Qualifier.newBuilder().setName("bazel.canonical_id").setValue("canonical ID")) + .build()); + } }