Skip to content

Keep the build context inside the build directory - #611

Open
halogenandtoast wants to merge 1 commit into
upserve:masterfrom
halogenandtoast:contain-build-context
Open

halogenandtoast wants to merge 1 commit into
upserve:masterfrom
halogenandtoast:contain-build-context

Conversation

@halogenandtoast

Copy link
Copy Markdown

Docker::Util can pull files from outside the build directory into the build context, in two independent ways.

create_relative_dir_tar stats and opens each path with File.stat and File.open, both of which follow symlinks, so a link sitting in the build directory is packed with its target's bytes under the link's own name. Separately, docker_context joins each .dockerignore negation pattern onto the build directory with File.join and hands the result to Dir.glob with nothing checking that it stays underneath, so a pattern containing ../ resolves outside. Either way the file lands in the tar sent to the daemon and can be read from inside the build.

docker build does not work this way. Moby matches .dockerignore against paths it gets from walking the context root, so a pattern cannot name anything outside it, and a COPY that leaves the context is refused. Anyone assuming this gem behaved like the CLI would be wrong about what their build can reach.

Affected: the symlink path since 1.20.0, the .dockerignore path since 2.2.0. Both present in 2.4.0 and on master.

The fix

Archive symlinks as symlinks instead of following them — which is what #530 has proposed since 2018 — and check that regular files really resolve under the build directory.

Both halves are needed. lstat alone does not stop a ../ pattern, and validating the pattern string alone does not stop a symlink, or a file reached through a symlinked directory.

The containment check sits in the tar loop rather than in docker_context so that file_hash_from_paths is covered too, which reaches the same code through Container#archive_in and Image#insert_local.

Using lstat also clears up the Errno::ENOENT that a dangling symlink previously raised out of build_from_dir. The Docker CLI archives a dangling link without complaint, so a tree that builds fine with docker build used to fail hard here.

Behaviour change worth flagging

In-context symlinks now ship as symlink entries rather than as regular files holding a copy of the target's bytes. That matches what docker build produces, but it is a visible change for anyone who was relying on the old dereferencing.

Tests

Four regression tests in spec/docker/util_spec.rb, covering both mechanisms, the composed symlinked-directory case, and the dangling-link crash. The suite had no symlink coverage at all before, which is why nothing pinned this.

SingleCov.covered! uncovered: 71 is unchanged and needs no edit: master measures 70 uncovered lines in util.rb, the patch alone takes it to 74, and the four tests bring it back to exactly 71.

bundle exec rspec spec/docker/util_spec.rb
# 37 examples, 0 failures, no coverage warning

Verified on Ruby 3.2.2. Note that the suite cannot load on Ruby 3.4 — lib/docker.rb requires base64, which left the default gems in 3.4 — but that is pre-existing and unrelated to this change.

network_spec.rb and connection_spec.rb have four failures against a modern daemon (Docker 29.8.1). They are identical on master with this branch reverted, so they are pre-existing and not from this change.

Cost

The added realpath call is roughly 18 µs per file: a 1,000-file context goes 76 ms to 99 ms, a 5,000-file context 259 ms to 352 ms, measured over 5 runs each with a warm cache. Proportionally real, negligible next to tarring and uploading the context.

Reproduction

Exits 1 on master, 0 with this branch. No Docker daemon needed — it only inspects the tar the gem builds.

require 'docker'
require 'tmpdir'
require 'stringio'
require 'rubygems/package'
require 'fileutils'

def entries(dir)
  tar = Docker::Util.create_dir_tar(dir)
  data = tar.read
  tar.close
  File.unlink(tar.path) if File.exist?(tar.path)
  found = []
  Gem::Package::TarReader.new(StringIO.new(data)) do |t|
    t.each { |e| found << [e.full_name, e.read.to_s] }
  end
  found
end

outside = Dir.mktmpdir('outside-the-build-context')
File.write(File.join(outside, 'secret'), 'CANARY-NOT-IN-BUILD-CONTEXT')
leaks = []

Dir.mktmpdir('ctx') do |dir|
  File.write(File.join(dir, 'Dockerfile'), "FROM scratch\n")
  File.symlink(File.join(outside, 'secret'), File.join(dir, 'innocent.txt'))
  entries(dir).each { |n, b| leaks << "symlink: #{n}" if b.include?('CANARY') }
end

Dir.mktmpdir('ctx') do |dir|
  File.write(File.join(dir, 'Dockerfile'), "FROM scratch\n")
  File.write(File.join(dir, '.dockerignore'),
             "!#{'../' * 12}#{outside[1..]}/secret\n")
  entries(dir).each { |n, b| leaks << ".dockerignore: #{n}" if b.include?('CANARY') }
end

FileUtils.rm_rf(outside)

if leaks.empty?
  puts 'OK: nothing outside the build directory reached the build context.'
  exit 0
else
  puts 'PRESENT: files outside the build directory were packed in:'
  leaks.each { |l| puts "  #{l}" }
  exit 1
end

Found by AI-assisted analysis, reviewed and reproduced by hand before sending.

This overlaps #530, which fixes the symlink half. Happy to rebase onto it if you would rather land that one first.

Docker::Util could pull files from outside the build directory into the
build context in two ways.

create_relative_dir_tar stats and opens each path with File.stat and
File.open, both of which follow symlinks, so a link sitting in the build
directory was packed with its target's bytes under the link's own name.
Separately, docker_context joins each .dockerignore negation pattern onto
the build directory with File.join and hands the result to Dir.glob with
nothing checking that it stays underneath, so a pattern containing ../
resolved outside. Either way the file landed in the tar sent to the
daemon and could be read from inside the build.

docker build does not work this way: moby matches .dockerignore against
paths it gets from walking the context root, so a pattern cannot name
anything outside it, and a COPY that leaves the context is refused.

Archive symlinks as symlinks instead of following them, as PR upserve#530 has
proposed since 2018, and check that regular files really resolve under
the build directory. Both are needed: lstat does not stop a ../ pattern,
and validating the pattern string does not stop a symlink or a file
reached through a symlinked directory.

The check sits in the tar loop rather than in docker_context so that
file_hash_from_paths is covered too, which reaches the same code through
Container#archive_in and Image#insert_local. Using lstat also clears up
the Errno::ENOENT that a dangling symlink previously raised out of
build_from_dir.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant