Merge pull request #134 from canonical/resolv-conf-fixes

Fix handling of resolv.conf in the chroot

debian/changelog

@@ -1,4 +1,4 @@
-ubuntu-image (3.0+23.04ubuntu2) UNRELEASED; urgency=medium
+ubuntu-image (3.0+23.04ubuntu1) UNRELEASED; urgency=medium
 
   [ William 'jawn-smith' Wilson ]
   * Redesign classic image builds to use image definition file.
@@ -16,7 +16,11 @@ ubuntu-image (3.0+23.04ubuntu2) UNRELEASED; urgency=medium
   [ Alfonso Sanchez-Beato ]
   * Run patchelf so we get dynamic linker and libs from the core22 base.
 
- -- mjdonis <[email protected]>  Thu, 11 May 2023 12:29:08 +0200
+  [ Łukasz 'sil2100' Zemczak ]
+  * Fix handling of resolv.conf in chroot, making sure we don't leak the build
+    environment by accident.
+
+ -- Łukasz 'sil2100' Zemczak <[email protected]>  Wed, 12 Jul 2023 12:55:52 +0200
 
 ubuntu-image (2.2+22.10ubuntu1) kinetic; urgency=medium
 

internal/helper/helper.go

@@ -18,6 +18,10 @@ import (
 	"github.com/xeipuuv/gojsonschema"
 )
 
+// define some functions that can be mocked by test cases
+var osRename = os.Rename
+var osRemove = os.Remove
+
 // CaptureStd returns an io.Reader to read what was printed, and teardown
 func CaptureStd(toCap **os.File) (io.Reader, func(), error) {
 	stdCap, stdCapW, err := os.Pipe()
@@ -434,10 +438,21 @@ func BackupAndCopyResolvConf(chroot string) error {
 // version that was backed up by BackupAndCopyResolvConf
 func RestoreResolvConf(chroot string) error {
 	if osutil.FileExists(filepath.Join(chroot, "etc", "resolv.conf.tmp")) {
-		src := filepath.Join(chroot, "etc", "resolv.conf.tmp")
-		dest := filepath.Join(chroot, "etc", "resolv.conf")
-		if err := os.Rename(src, dest); err != nil {
-			return fmt.Errorf("Error moving file \"%s\" to \"%s\": %s", src, dest, err.Error())
+		if osutil.IsSymlink(filepath.Join(chroot, "etc", "resolv.conf")) {
+			// As per what live-build does, handle the case where some package
+			// in the install_packages phase converts resolv.conf into a
+			// symlink. In such case we don't restore our backup but instead
+			// remove it, leaving the symlink around.
+			backup := filepath.Join(chroot, "etc", "resolv.conf.tmp")
+			if err := osRemove(backup); err != nil {
+				return fmt.Errorf("Error removing file \"%s\": %s", backup, err.Error())
+			}
+		} else {
+			src := filepath.Join(chroot, "etc", "resolv.conf.tmp")
+			dest := filepath.Join(chroot, "etc", "resolv.conf")
+			if err := osRename(src, dest); err != nil {
+				return fmt.Errorf("Error moving file \"%s\" to \"%s\": %s", src, dest, err.Error())
+			}
 		}
 	}
 	return nil

internal/helper/helper_test.go

@@ -0,0 +1,119 @@
+package helper
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/google/uuid"
+	"github.com/snapcore/snapd/osutil"
+)
+
+// define some mocked versions of go package functions
+func mockRemove(string) error {
+	return fmt.Errorf("Test Error")
+}
+func mockRename(string, string) error {
+	return fmt.Errorf("Test Error")
+}
+
+// TestRestoreResolvConf tests if resolv.conf is restored correctly
+func TestRestoreResolvConf(t *testing.T) {
+	t.Run("test_restore_resolv_conf", func(t *testing.T) {
+		asserter := Asserter{T: t}
+		// Prepare temporary directory
+		workDir := filepath.Join("/tmp", "ubuntu-image-"+uuid.NewString())
+		err := os.Mkdir(workDir, 0755)
+		asserter.AssertErrNil(err, true)
+		defer os.RemoveAll(workDir)
+
+		// Create test objects for a regular backup
+		err = os.MkdirAll(filepath.Join(workDir, "etc"), 0755)
+		asserter.AssertErrNil(err, true)
+		mainConfPath := filepath.Join(workDir, "etc", "resolv.conf")
+		mainConf, err := os.Create(mainConfPath)
+		asserter.AssertErrNil(err, true)
+		testData := []byte("Main")
+		_, err = mainConf.Write(testData)
+		asserter.AssertErrNil(err, true)
+		mainConf.Close()
+		backupConfPath := filepath.Join(workDir, "etc", "resolv.conf.tmp")
+		backupConf, err := os.Create(backupConfPath)
+		asserter.AssertErrNil(err, true)
+		testData = []byte("Backup")
+		_, err = backupConf.Write(testData)
+		asserter.AssertErrNil(err, true)
+		backupConf.Close()
+
+		err = RestoreResolvConf(workDir)
+		asserter.AssertErrNil(err, true)
+		if osutil.FileExists(backupConfPath) {
+			t.Errorf("Backup resolv.conf.tmp has not been removed")
+		}
+		checkData, err := os.ReadFile(mainConfPath)
+		asserter.AssertErrNil(err, true)
+		if string(checkData) != "Backup" {
+			t.Errorf("Main resolv.conf has not been restored")
+		}
+
+		// Now check if the symlink case also works
+		_, err = os.Create(backupConfPath)
+		asserter.AssertErrNil(err, true)
+		err = os.Remove(mainConfPath)
+		asserter.AssertErrNil(err, true)
+		err = os.Symlink("resolv.conf.tmp", mainConfPath)
+		asserter.AssertErrNil(err, true)
+
+		err = RestoreResolvConf(workDir)
+		asserter.AssertErrNil(err, true)
+		if osutil.FileExists(backupConfPath) {
+			t.Errorf("Backup resolv.conf.tmp has not been removed when dealing with as symlink")
+		}
+		if !osutil.IsSymlink(mainConfPath) {
+			t.Errorf("Main resolv.conf should have remained a symlink, but it is not")
+		}
+	})
+}
+
+// TestFailedRestoreResolvConf tests all resolv.conf error cases
+func TestFailedRestoreResolvConf(t *testing.T) {
+	t.Run("test_failed_restore_resolv_conf", func(t *testing.T) {
+		asserter := Asserter{T: t}
+		// Prepare temporary directory
+		workDir := filepath.Join("/tmp", "ubuntu-image-"+uuid.NewString())
+		err := os.Mkdir(workDir, 0755)
+		asserter.AssertErrNil(err, true)
+		defer os.RemoveAll(workDir)
+
+		// Create test environment
+		err = os.MkdirAll(filepath.Join(workDir, "etc"), 0755)
+		asserter.AssertErrNil(err, true)
+		mainConfPath := filepath.Join(workDir, "etc", "resolv.conf")
+		_, err = os.Create(mainConfPath)
+		asserter.AssertErrNil(err, true)
+		backupConfPath := filepath.Join(workDir, "etc", "resolv.conf.tmp")
+		_, err = os.Create(backupConfPath)
+		asserter.AssertErrNil(err, true)
+
+		// Mock the os.Rename failure
+		osRename = mockRename
+		defer func() {
+			osRename = os.Rename
+		}()
+		err = RestoreResolvConf(workDir)
+		asserter.AssertErrContains(err, "Error moving file")
+
+		// Mock the os.Remove failure
+		err = os.Remove(mainConfPath)
+		asserter.AssertErrNil(err, true)
+		err = os.Symlink("resolv.conf.tmp", mainConfPath)
+		asserter.AssertErrNil(err, true)
+		osRemove = mockRemove
+		defer func() {
+			osRemove = os.Remove
+		}()
+		err = RestoreResolvConf(workDir)
+		asserter.AssertErrContains(err, "Error removing file")
+	})
+}

internal/statemachine/classic_states.go

@@ -513,6 +513,14 @@ func (stateMachine *StateMachine) createChroot() error {
 	hostname := filepath.Join(stateMachine.tempDirs.chroot, "etc", "hostname")
 	hostnameFile, _ := os.OpenFile(hostname, os.O_TRUNC|os.O_WRONLY, 0644)
 	hostnameFile.WriteString("ubuntu\n")
+	hostnameFile.Close()
+
+	// debootstrap also copies /etc/resolv.conf from build environment; truncate it
+	// as to not leak the host files into the built image
+	resolvConf := filepath.Join(stateMachine.tempDirs.chroot, "etc", "resolv.conf")
+	if err := osTruncate(resolvConf, 0); err != nil {
+		return fmt.Errorf("Error truncating resolv.conf: %s", err.Error())
+	}
 
 	// add any extra apt sources to /etc/apt/sources.list
 	aptSources := classicStateMachine.ImageDef.GeneratePocketList()

internal/statemachine/classic_test.go

@@ -2652,6 +2652,14 @@ func TestCreateChroot(t *testing.T) {
 			t.Errorf("Expected hostname to be \"ubuntu\", but is \"%s\"", string(hostnameData))
 		}
 
+		// check that the resolv.conf file was truncated
+		resolvConfFile := filepath.Join(stateMachine.tempDirs.chroot, "etc", "resolv.conf")
+		resolvConfData, err := os.ReadFile(resolvConfFile)
+		asserter.AssertErrNil(err, true)
+		if string(resolvConfData) != "" {
+			t.Errorf("Expected resolv.conf to be empty, but is \"%s\"", string(resolvConfData))
+		}
+
 		// check that security, updates, and proposed were added to /etc/apt/sources.list
 		sourcesList := filepath.Join(stateMachine.tempDirs.chroot, "etc", "apt", "sources.list")
 		sourcesListData, err := os.ReadFile(sourcesList)
@@ -2711,6 +2719,28 @@ func TestFailedCreateChroot(t *testing.T) {
 		asserter.AssertErrContains(err, "Error running debootstrap command")
 		execCommand = exec.Command
 
+		// Check if failure of truncation is detected
+
+		// Clean the work directory
+		os.RemoveAll(stateMachine.stateMachineFlags.WorkDir)
+		err = stateMachine.makeTemporaryDirectories()
+		asserter.AssertErrNil(err, true)
+
+		// Prepare a fallthrough debootstrap
+		testCaseName = "TestFailedCreateChrootSkip"
+		execCommand = fakeExecCommand
+		defer func() {
+			execCommand = exec.Command
+		}()
+		osTruncate = mockTruncate
+		defer func() {
+			osTruncate = os.Truncate
+		}()
+		err = stateMachine.createChroot()
+		asserter.AssertErrContains(err, "Error truncating resolv.conf")
+		osTruncate = os.Truncate
+		execCommand = exec.Command
+
 		os.RemoveAll(stateMachine.stateMachineFlags.WorkDir)
 	})
 }

internal/statemachine/state_machine_test.go

@@ -242,6 +242,8 @@ func TestExecHelperProcess(t *testing.T) {
 			os.Exit(1)
 		}
 		break
+	case "TestFailedCreateChrootSkip":
+		fallthrough
 	case "TestFailedRunLiveBuild":
 		// Do nothing so we don't have to wait for actual lb commands
 		break

snapcraft.yaml

@@ -3,7 +3,7 @@ summary: Create Ubuntu images
 description: |
   Official tool for building Ubuntu images, currently supporing Ubuntu Core
   snap-based images and preinstalled Ubuntu classic images.
-version: 3.0+snap4
+version: 3.0+snap5
 grade: stable
 confinement: classic
 base: core22