From 957e2f8b7f2072b47a31ed121456615a7dde8ea0 Mon Sep 17 00:00:00 2001 From: Moritz Halbritter Date: Fri, 11 Oct 2024 15:23:29 +0200 Subject: [PATCH] Add warning if sensitive container paths are bound Closes gh-41643 --- .../platform/build/AbstractBuildLog.java | 8 +++ .../buildpack/platform/build/BuildLog.java | 8 +++ .../buildpack/platform/build/Builder.java | 10 ++++ .../platform/docker/type/Binding.java | 51 +++++++++++++++++- .../platform/build/BuilderTests.java | 21 ++++++++ .../platform/docker/type/BindingTests.java | 53 ++++++++++++++++++- 6 files changed, 149 insertions(+), 2 deletions(-) diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/AbstractBuildLog.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/AbstractBuildLog.java index a75f72d659..d7b25cccbd 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/AbstractBuildLog.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/AbstractBuildLog.java @@ -21,6 +21,7 @@ import java.util.function.Consumer; import org.springframework.boot.buildpack.platform.docker.LogUpdateEvent; import org.springframework.boot.buildpack.platform.docker.TotalProgressEvent; +import org.springframework.boot.buildpack.platform.docker.type.Binding; import org.springframework.boot.buildpack.platform.docker.type.Image; import org.springframework.boot.buildpack.platform.docker.type.ImagePlatform; import org.springframework.boot.buildpack.platform.docker.type.ImageReference; @@ -118,6 +119,13 @@ public abstract class AbstractBuildLog implements BuildLog { log(); } + @Override + public void sensitiveTargetBindingDetected(Binding binding) { + log("Warning: Binding '%s' uses a container path which is used by buildpacks while building. Binding to it can cause problems!" + .formatted(binding)); + log(); + } + private String getDigest(Image image) { List digests = image.getDigests(); return (digests.isEmpty() ? "" : digests.get(0)); diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildLog.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildLog.java index 633886f55f..3c2f5bf282 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildLog.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/BuildLog.java @@ -21,6 +21,7 @@ import java.util.function.Consumer; import org.springframework.boot.buildpack.platform.docker.LogUpdateEvent; import org.springframework.boot.buildpack.platform.docker.TotalProgressEvent; +import org.springframework.boot.buildpack.platform.docker.type.Binding; import org.springframework.boot.buildpack.platform.docker.type.Image; import org.springframework.boot.buildpack.platform.docker.type.ImagePlatform; import org.springframework.boot.buildpack.platform.docker.type.ImageReference; @@ -125,6 +126,13 @@ public interface BuildLog { */ void failedCleaningWorkDir(Cache cache, Exception exception); + /** + * Log that a binding with a sensitive target has been detected. + * @param binding the binding + * @since 3.4.0 + */ + void sensitiveTargetBindingDetected(Binding binding); + /** * Factory method that returns a {@link BuildLog} the outputs to {@link System#out}. * @return a build log instance that logs to system out diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/Builder.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/Builder.java index 8d90e84a84..09baf1db32 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/Builder.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/build/Builder.java @@ -28,6 +28,7 @@ import org.springframework.boot.buildpack.platform.docker.UpdateListener; import org.springframework.boot.buildpack.platform.docker.configuration.DockerConfiguration; import org.springframework.boot.buildpack.platform.docker.configuration.ResolvedDockerHost; import org.springframework.boot.buildpack.platform.docker.transport.DockerEngineException; +import org.springframework.boot.buildpack.platform.docker.type.Binding; import org.springframework.boot.buildpack.platform.docker.type.Image; import org.springframework.boot.buildpack.platform.docker.type.ImagePlatform; import org.springframework.boot.buildpack.platform.docker.type.ImageReference; @@ -98,6 +99,7 @@ public class Builder { public void build(BuildRequest request) throws DockerEngineException, IOException { Assert.notNull(request, "Request must not be null"); this.log.start(request); + validateBindings(request.getBindings()); String domain = request.getBuilder().getDomain(); PullPolicy pullPolicy = request.getPullPolicy(); ImageFetcher imageFetcher = new ImageFetcher(domain, getBuilderAuthHeader(), pullPolicy, @@ -125,6 +127,14 @@ public class Builder { } } + private void validateBindings(List bindings) { + for (Binding binding : bindings) { + if (binding.usesSensitiveContainerPath()) { + this.log.sensitiveTargetBindingDetected(binding); + } + } + } + private BuildRequest withRunImageIfNeeded(BuildRequest request, BuilderMetadata metadata) { if (request.getRunImage() != null) { return request; diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/Binding.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/Binding.java index 8930f4fffd..4ba2509c40 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/Binding.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/main/java/org/springframework/boot/buildpack/platform/docker/type/Binding.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2022 the original author or authors. + * Copyright 2012-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -16,7 +16,10 @@ package org.springframework.boot.buildpack.platform.docker.type; +import java.util.ArrayList; +import java.util.List; import java.util.Objects; +import java.util.Set; import org.springframework.util.Assert; @@ -24,10 +27,17 @@ import org.springframework.util.Assert; * Volume bindings to apply when creating a container. * * @author Scott Frederick + * @author Moritz Halbritter * @since 2.5.0 */ public final class Binding { + /** + * Sensitive container paths, which lead to problems if used in a binding. + */ + private static final Set SENSITIVE_CONTAINER_PATHS = Set.of("/cnb", "/layers", "/workspace", "c:\\cnb", + "c:\\layers", "c:\\workspace"); + private final String value; private Binding(String value) { @@ -55,6 +65,45 @@ public final class Binding { return this.value; } + /** + * Returns the container destination path. + * @return the container destination path + */ + String getContainerDestinationPath() { + List parts = split(this.value, ':', '\\'); + // Format is ::[] + Assert.state(parts.size() >= 2, () -> "Expected 2 or more parts, but found %d".formatted(parts.size())); + return parts.get(1); + } + + private List split(String input, char delimiter, char notFollowedBy) { + Assert.state(notFollowedBy != '\0', "notFollowedBy must not be the null terminator"); + List parts = new ArrayList<>(); + StringBuilder accumulator = new StringBuilder(); + for (int i = 0; i < input.length(); i++) { + char c = input.charAt(i); + char nextChar = (i + 1 < input.length()) ? input.charAt(i + 1) : '\0'; + if (c == delimiter && nextChar != notFollowedBy) { + parts.add(accumulator.toString()); + accumulator.setLength(0); + } + else { + accumulator.append(c); + } + } + parts.add(accumulator.toString()); + return parts; + } + + /** + * Whether the binding uses a sensitive container path. + * @return whether the binding uses a sensitive container path + * @since 3.4.0 + */ + public boolean usesSensitiveContainerPath() { + return SENSITIVE_CONTAINER_PATHS.contains(getContainerDestinationPath()); + } + /** * Create a {@link Binding} with the specified value containing a host source, * container destination, and options. diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderTests.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderTests.java index 78d2ea24c0..239143aa47 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/build/BuilderTests.java @@ -32,6 +32,7 @@ import org.springframework.boot.buildpack.platform.docker.DockerApi.VolumeApi; import org.springframework.boot.buildpack.platform.docker.TotalProgressPullListener; import org.springframework.boot.buildpack.platform.docker.configuration.DockerConfiguration; import org.springframework.boot.buildpack.platform.docker.transport.DockerEngineException; +import org.springframework.boot.buildpack.platform.docker.type.Binding; import org.springframework.boot.buildpack.platform.docker.type.ContainerReference; import org.springframework.boot.buildpack.platform.docker.type.ContainerStatus; import org.springframework.boot.buildpack.platform.docker.type.Image; @@ -521,6 +522,26 @@ class BuilderTests { .withMessageContaining("not found in builder"); } + @Test + void logsWarningIfBindingWithSensitiveTargetIsDetected() throws IOException { + TestPrintStream out = new TestPrintStream(); + DockerApi docker = mockDockerApi(); + Image builderImage = loadImage("image.json"); + Image runImage = loadImage("run-image.json"); + given(docker.image() + .pull(eq(ImageReference.of(BuildRequest.DEFAULT_BUILDER_IMAGE_REF)), isNull(), any(), isNull())) + .willAnswer(withPulledImage(builderImage)); + given(docker.image() + .pull(eq(ImageReference.of("docker.io/cloudfoundry/run:base-cnb")), eq(ImagePlatform.from(builderImage)), + any(), isNull())) + .willAnswer(withPulledImage(runImage)); + Builder builder = new Builder(BuildLog.to(out), docker, null); + BuildRequest request = getTestRequest().withBindings(Binding.from("/host", "/cnb")); + builder.build(request); + assertThat(out.toString()).contains( + "Warning: Binding '/host:/cnb' uses a container path which is used by buildpacks while building. Binding to it can cause problems!"); + } + private DockerApi mockDockerApi() throws IOException { return mockDockerApi(null); } diff --git a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/BindingTests.java b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/BindingTests.java index 3f0ec68cbe..94ba710f47 100644 --- a/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/BindingTests.java +++ b/spring-boot-project/spring-boot-tools/spring-boot-buildpack-platform/src/test/java/org/springframework/boot/buildpack/platform/docker/type/BindingTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2012-2023 the original author or authors. + * Copyright 2012-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,14 +17,18 @@ package org.springframework.boot.buildpack.platform.docker.type; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; /** * Tests for {@link Binding}. * * @author Scott Frederick + * @author Moritz Halbritter */ class BindingTests { @@ -70,4 +74,51 @@ class BindingTests { .withMessageContaining("SourceVolume must not be null"); } + @Test + void shouldReturnContainerDestinationPath() { + Binding binding = Binding.from("/host", "/container"); + assertThat(binding.getContainerDestinationPath()).isEqualTo("/container"); + } + + @Test + void shouldReturnContainerDestinationPathWithOptions() { + Binding binding = Binding.of("/host:/container:ro"); + assertThat(binding.getContainerDestinationPath()).isEqualTo("/container"); + } + + @Test + void shouldReturnContainerDestinationPathOnWindows() { + Binding binding = Binding.from("C:\\host", "C:\\container"); + assertThat(binding.getContainerDestinationPath()).isEqualTo("C:\\container"); + } + + @Test + void shouldReturnContainerDestinationPathOnWindowsWithOptions() { + Binding binding = Binding.of("C:\\host:C:\\container:ro"); + assertThat(binding.getContainerDestinationPath()).isEqualTo("C:\\container"); + } + + @Test + void shouldFailIfBindingIsMalformed() { + Binding binding = Binding.of("some-invalid-binding"); + assertThatIllegalStateException().isThrownBy(binding::getContainerDestinationPath) + .withMessage("Expected 2 or more parts, but found 1"); + } + + @ParameterizedTest + @CsvSource(textBlock = """ + /cnb, true + /layers, true + /workspace, true + /something, false + c:\\cnb, true + c:\\layers, true + c:\\workspace, true + c:\\something, false + """) + void shouldDetectSensitiveContainerPaths(String containerPath, boolean sensitive) { + Binding binding = Binding.from("/host", containerPath); + assertThat(binding.usesSensitiveContainerPath()).isEqualTo(sensitive); + } + }