Ensure WORKDIR from images is created

A recent crun change stopped the creation of the container's
working directory if it does not exist. This is arguably correct
for user-specified directories, to protect against typos; it is
definitely not correct for image WORKDIR, where the image author
definitely intended for the directory to be used.

This makes Podman create the working directory and chown it to
container root, if it does not already exist, and only if it was
specified by an image, not the user.

Signed-off-by: Matthew Heon <matthew.heon@pm.me>

5 files changed, 161 insertions, 25 deletions

diff --git a/libpod/container_config.go b/libpod/container_config.go
index 301b867fc..5f89395c1 100644
--- a/libpod/container_config.go
+++ b/libpod/container_config.go
@@ -13,25 +13,52 @@ import (
 
 // ContainerConfig contains all information that was used to create the
 // container. It may not be changed once created.
-// It is stored, read-only, on disk
+// It is stored, read-only, on disk in Libpod's State.
+// Any changes will not be written back to the database, and will cause
+// inconsistencies with other Libpod instances.
 type ContainerConfig struct {
+	// Spec is OCI runtime spec used to create the container. This is passed
+	// in when the container is created, but it is not the final spec used
+	// to run the container - it will be modified by Libpod to add things we
+	// manage (e.g. bind mounts for /etc/resolv.conf, named volumes, a
+	// network namespace prepared by CNI or slirp4netns) in the
+	// generateSpec() function.
 	Spec *spec.Spec `json:"spec"`
 
+	// ID is a hex-encoded 256-bit pseudorandom integer used as a unique
+	// identifier for the container. IDs are globally unique in Libpod -
+	// once an ID is in use, no other container or pod will be created with
+	// the same one until the holder of the ID has been removed.
+	// ID is generated by Libpod, and cannot be chosen or influenced by the
+	// user (except when restoring a checkpointed container).
+	// ID is guaranteed to be 64 characters long.
 	ID string `json:"id"`
 
+	// Name is a human-readable name for the container. All containers must
+	// have a non-empty name. Name may be provided when the container is
+	// created; if no name is chosen, a name will be auto-generated.
 	Name string `json:"name"`
 
-	// Full ID of the pood the container belongs to
+	// Pod is the full ID of the pod the container belongs to. If the
+	// container does not belong to a pod, this will be empty.
+	// If this is not empty, a pod with this ID is guaranteed to exist in
+	// the state for the duration of this container's existence.
 	Pod string `json:"pod,omitempty"`
 
-	// Namespace the container is in
+	// Namespace is the libpod Namespace the container is in.
+	// Namespaces are used to divide containers in the state.
 	Namespace string `json:"namespace,omitempty"`
 
-	// ID of this container's lock
+	// LockID is the ID of this container's lock. Each container, pod, and
+	// volume is assigned a unique Lock (from one of several backends) by
+	// the libpod Runtime. This lock will belong only to this container for
+	// the duration of the container's lifetime.
 	LockID uint32 `json:"lockID"`
 
-	// CreateCommand is the full command plus arguments of the process the
-	// container has been created with.
+	// CreateCommand is the full command plus arguments that were used to
+	// create the container. It is shown in the output of Inspect, and may
+	// be used to recreate an identical container for automatic updates or
+	// portable systemd unit files.
 	CreateCommand []string `json:"CreateCommand,omitempty"`
 
 	// RawImageName is the raw and unprocessed name of the image when creating
@@ -40,10 +67,13 @@ type ContainerConfig struct {
 	// name and not some normalized instance of it.
 	RawImageName string `json:"RawImageName,omitempty"`
 
-	// UID/GID mappings used by the storage
+	// IDMappings are UID/GID mappings used by the container's user
+	// namespace. They are used by the OCI runtime when creating the
+	// container, and by c/storage to ensure that the container's files have
+	// the appropriate owner.
 	IDMappings storage.IDMappingOptions `json:"idMappingsOptions,omitempty"`
 
-	// IDs of dependency containers.
+	// Dependencies are the IDs of dependency containers.
 	// These containers must be started before this container is started.
 	Dependencies []string
 
@@ -59,45 +89,92 @@ type ContainerConfig struct {
 // ContainerRootFSConfig is an embedded sub-config providing config info
 // about the container's root fs.
 type ContainerRootFSConfig struct {
-	RootfsImageID   string `json:"rootfsImageID,omitempty"`
+	// RootfsImageID is the ID of the image used to create the container.
+	// If the container was created from a Rootfs, this will be empty.
+	// If non-empty, Podman will create a root filesystem for the container
+	// based on an image with this ID.
+	// This conflicts with Rootfs.
+	RootfsImageID string `json:"rootfsImageID,omitempty"`
+	// RootfsImageName is the (normalized) name of the image used to create
+	// the container. If the container was created from a Rootfs, this will
+	// be empty.
 	RootfsImageName string `json:"rootfsImageName,omitempty"`
-	// Rootfs to use for the container, this conflicts with RootfsImageID
+	// Rootfs is a directory to use as the container's root filesystem.
+	// If RootfsImageID is set, this will be empty.
+	// If this is set, Podman will not create a root filesystem for the
+	// container based on an image, and will instead use the given directory
+	// as the container's root.
+	// Conflicts with RootfsImageID.
 	Rootfs string `json:"rootfs,omitempty"`
-	// Src path to be mounted on /dev/shm in container.
+	// ShmDir is the path to be mounted on /dev/shm in container.
+	// If not set manually at creation time, Libpod will create a tmpfs
+	// with the size specified in ShmSize and populate this with the path of
+	// said tmpfs.
 	ShmDir string `json:"ShmDir,omitempty"`
-	// Size of the container's SHM.
+	// ShmSize is the size of the container's SHM. Only used if ShmDir was
+	// not set manually at time of creation.
 	ShmSize int64 `json:"shmSize"`
 	// Static directory for container content that will persist across
 	// reboot.
+	// StaticDir is a persistent directory for Libpod files that will
+	// survive system reboot. It is not part of the container's rootfs and
+	// is not mounted into the container. It will be removed when the
+	// container is removed.
+	// Usually used to store container log files, files that will be bind
+	// mounted into the container (e.g. the resolv.conf we made for the
+	// container), and other per-container content.
 	StaticDir string `json:"staticDir"`
-	// Mounts list contains all additional mounts into the container rootfs.
-	// These include the SHM mount.
+	// Mounts contains all additional mounts into the container rootfs.
+	// It is presently only used for the container's SHM directory.
 	// These must be unmounted before the container's rootfs is unmounted.
 	Mounts []string `json:"mounts,omitempty"`
-	// NamedVolumes lists the named volumes to mount into the container.
+	// NamedVolumes lists the Libpod named volumes to mount into the
+	// container. Each named volume is guaranteed to exist so long as this
+	// container exists.
 	NamedVolumes []*ContainerNamedVolume `json:"namedVolumes,omitempty"`
 	// OverlayVolumes lists the overlay volumes to mount into the container.
 	OverlayVolumes []*ContainerOverlayVolume `json:"overlayVolumes,omitempty"`
+	// CreateWorkingDir indicates that Libpod should create the container's
+	// working directory if it does not exist. Some OCI runtimes do this by
+	// default, but others do not.
+	CreateWorkingDir bool `json:"createWorkingDir,omitempty"`
 }
 
 // ContainerSecurityConfig is an embedded sub-config providing security configuration
 // to the container.
 type ContainerSecurityConfig struct {
-	// Whether the container is privileged
+	// Pirivileged is whether the container is privileged. Privileged
+	// containers have lessened security and increased access to the system.
+	// Note that this does NOT directly correspond to Podman's --privileged
+	// flag - most of the work of that flag is done in creating the OCI spec
+	// given to Libpod. This only enables a small subset of the overall
+	// operation, mostly around mounting the container image with reduced
+	// security.
 	Privileged bool `json:"privileged"`
-	// SELinux process label for container
+	// ProcessLabel is the SELinux process label for the container.
 	ProcessLabel string `json:"ProcessLabel,omitempty"`
-	// SELinux mount label for root filesystem
+	// MountLabel is the SELinux mount label for the container's root
+	// filesystem. Only used if the container was created from an image.
+	// If not explicitly set, an unused random MLS label will be assigned by
+	// containers/storage (but only if SELinux is enabled).
 	MountLabel string `json:"MountLabel,omitempty"`
-	// LabelOpts are options passed in by the user to setup SELinux labels
+	// LabelOpts are options passed in by the user to setup SELinux labels.
+	// These are used by the containers/storage library.
 	LabelOpts []string `json:"labelopts,omitempty"`
-	// User and group to use in the container
-	// Can be specified by name or UID/GID
+	// User and group to use in the container. Can be specified as only user
+	// (in which case we will attempt to look up the user in the container
+	// to determine the appropriate group) or user and group separated by a
+	// colon.
+	// Can be specified by name or UID/GID.
+	// If unset, this will default to UID and GID 0 (root).
 	User string `json:"user,omitempty"`
-	// Additional groups to add
+	// Groups are additional groups to add the container's user to. These
+	// are resolved within the container using the container's /etc/passwd.
 	Groups []string `json:"groups,omitempty"`
-	// AddCurrentUserPasswdEntry indicates that the current user passwd entry
-	// should be added to the /etc/passwd within the container
+	// AddCurrentUserPasswdEntry indicates that Libpod should ensure that
+	// the container's /etc/passwd contains an entry for the user running
+	// Libpod - mostly used in rootless containers where the user running
+	// Libpod wants to retain their UID inside the container.
 	AddCurrentUserPasswdEntry bool `json:"addCurrentUserPasswdEntry,omitempty"`
 }
 
diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 4cfe992ea..9fb9738dc 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -159,7 +159,32 @@ func (c *Container) prepare() error {
 	}
 
 	// Save changes to container state
-	return c.save()
+	if err := c.save(); err != nil {
+		return err
+	}
+
+	// Ensure container entrypoint is created (if required)
+	if c.config.CreateWorkingDir {
+		workdir, err := securejoin.SecureJoin(c.state.Mountpoint, c.WorkingDir())
+		if err != nil {
+			return errors.Wrapf(err, "error creating path to container %s working dir", c.ID())
+		}
+		rootUID := c.RootUID()
+		rootGID := c.RootGID()
+
+		if err := os.MkdirAll(workdir, 0755); err != nil {
+			if os.IsExist(err) {
+				return nil
+			}
+			return errors.Wrapf(err, "error creating container %s working dir", c.ID())
+		}
+
+		if err := os.Chown(workdir, rootUID, rootGID); err != nil {
+			return errors.Wrapf(err, "error chowning container %s working directory to container root", c.ID())
+		}
+	}
+
+	return nil
 }
 
 // cleanupNetwork unmounts and cleans up the container's network
diff --git a/libpod/options.go b/libpod/options.go
index b98ef2221..16b05d9b6 100644
--- a/libpod/options.go
+++ b/libpod/options.go
@@ -1451,6 +1451,19 @@ func WithCreateCommand(cmd []string) CtrCreateOption {
 	}
 }
 
+// WithCreateWorkingDir tells Podman to create the container's working directory
+// if it does not exist.
+func WithCreateWorkingDir() CtrCreateOption {
+	return func(ctr *Container) error {
+		if ctr.valid {
+			return define.ErrCtrFinalized
+		}
+
+		ctr.config.CreateWorkingDir = true
+		return nil
+	}
+}
+
 // Volume Creation Options
 
 // WithVolumeName sets the name of the volume.
diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go
index 9dfb35be3..b61ac2c30 100644
--- a/pkg/specgen/generate/container_create.go
+++ b/pkg/specgen/generate/container_create.go
@@ -238,6 +238,17 @@ func createContainerOptions(ctx context.Context, rt *libpod.Runtime, s *specgen.
 	if s.Entrypoint != nil {
 		options = append(options, libpod.WithEntrypoint(s.Entrypoint))
 	}
+	// If the user did not set an workdir but the image did, ensure it is
+	// created.
+	if s.WorkDir == "" && img != nil {
+		newWD, err := img.WorkingDir(ctx)
+		if err != nil {
+			return nil, err
+		}
+		if newWD != "" {
+			options = append(options, libpod.WithCreateWorkingDir())
+		}
+	}
 	if s.StopSignal != nil {
 		options = append(options, libpod.WithStopSignal(*s.StopSignal))
 	}
diff --git a/test/e2e/run_test.go b/test/e2e/run_test.go
index 1f9cc3cb0..b666f114b 100644
--- a/test/e2e/run_test.go
+++ b/test/e2e/run_test.go
@@ -1123,4 +1123,14 @@ USER mail`
 		Expect(session.ExitCode()).To(Not(Equal(0)))
 		Expect(session.ErrorToString()).To(ContainSubstring("Invalid umask"))
 	})
+
+	It("podman run makes entrypoint from image", func() {
+		dockerfile := `FROM busybox
+WORKDIR /madethis`
+		podmanTest.BuildImage(dockerfile, "test", "false")
+		session := podmanTest.Podman([]string{"run", "--rm", "test", "pwd"})
+		session.WaitWithDefaultTimeout()
+		Expect(session.ExitCode()).To(Equal(0))
+		Expect(session.OutputToString()).To(ContainSubstring("/madethis"))
+	})
 })