diff --git a/pkg/daemon/command_runner.go b/pkg/daemon/command_runner.go new file mode 100644 index 0000000000..ee5a8c9e51 --- /dev/null +++ b/pkg/daemon/command_runner.go @@ -0,0 +1,64 @@ +package daemon + +import ( + "fmt" + "os/exec" + "strings" + + "k8s.io/klog/v2" +) + +// CommandRunner abstracts command execution for testing and flexibility. +type CommandRunner interface { + // RunGetOut executes a command and returns its stdout output. + // On error, stderr is included in the error message. + RunGetOut(command string, args ...string) ([]byte, error) +} + +// CommandRunnerOS is the production implementation that executes real OS commands. +type CommandRunnerOS struct{} + +// RunGetOut executes a command, logs it, and returns stdout. +// On error, stderr is included in the error message (truncated to 256 chars). +func (r *CommandRunnerOS) RunGetOut(command string, args ...string) ([]byte, error) { + klog.Infof("Running captured: %s %s", command, strings.Join(args, " ")) + cmd := exec.Command(command, args...) + rawOut, err := cmd.Output() + if err != nil { + errtext := "" + if e, ok := err.(*exec.ExitError); ok { + // Trim to max of 256 characters + errtext = fmt.Sprintf("\n%s", truncate(string(e.Stderr), 256)) + } + return nil, fmt.Errorf("error running %s %s: %s%s", command, strings.Join(args, " "), err, errtext) + } + return rawOut, nil +} + +// TODO: Delete this function to always consume the interface instance +// Tracking story: https://issues.redhat.com/browse/MCO-1924 +// +// Conserved the old signature to avoid a big footprint bugfix with this change +func runGetOut(command string, args ...string) ([]byte, error) { + return (&CommandRunnerOS{}).RunGetOut(command, args...) +} + +// MockCommandRunner is a test implementation that returns pre-configured outputs. +type MockCommandRunner struct { + outputs map[string][]byte + errors map[string]error +} + +// RunGetOut returns pre-configured output or error based on the command string. +// It matches commands using "command arg1 arg2..." as the key. +// Returns an error if no matching output or error is found. +func (m *MockCommandRunner) RunGetOut(command string, args ...string) ([]byte, error) { + key := command + " " + strings.Join(args, " ") + if out, ok := m.outputs[key]; ok { + return out, nil + } + if err, ok := m.errors[key]; ok { + return nil, err + } + return nil, fmt.Errorf("no output for command %s found", command) +} diff --git a/pkg/daemon/command_runner_test.go b/pkg/daemon/command_runner_test.go new file mode 100644 index 0000000000..c027d6bb4c --- /dev/null +++ b/pkg/daemon/command_runner_test.go @@ -0,0 +1,93 @@ +package daemon + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestCommandRunnerOS_RunGetOut(t *testing.T) { + runner := &CommandRunnerOS{} + + // Test successful command with no output + o, err := runner.RunGetOut("true") + assert.Nil(t, err) + assert.Equal(t, len(o), 0) + + // Test failed command + o, err = runner.RunGetOut("false") + assert.NotNil(t, err) + + // Test successful command with output + o, err = runner.RunGetOut("echo", "hello") + assert.Nil(t, err) + assert.Equal(t, string(o), "hello\n") + + // Test command that outputs to stderr and exits with error + // base64 encode "oops" so we can't match on the command arguments + o, err = runner.RunGetOut("/bin/sh", "-c", "echo hello; echo b29vcwo== | base64 -d 1>&2; exit 1") + assert.Error(t, err) + errtext := err.Error() + assert.Contains(t, errtext, "exit status 1\nooos\n") + + // Test command that doesn't exist + o, err = runner.RunGetOut("/usr/bin/test-failure-to-exec-this-should-not-exist", "arg") + assert.Error(t, err) +} + +func TestMockCommandRunner_RunGetOut(t *testing.T) { + // Test mock with pre-configured output + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "echo hello": []byte("hello\n"), + "rpm-ostree status": []byte("State: idle\n"), + }, + errors: map[string]error{}, + } + + o, err := mock.RunGetOut("echo", "hello") + assert.Nil(t, err) + assert.Equal(t, []byte("hello\n"), o) + + o, err = mock.RunGetOut("rpm-ostree", "status") + assert.Nil(t, err) + assert.Equal(t, []byte("State: idle\n"), o) + + // Test mock with pre-configured error + mock = &MockCommandRunner{ + outputs: map[string][]byte{}, + errors: map[string]error{ + "cmd --fail": assert.AnError, + }, + } + + o, err = mock.RunGetOut("cmd", "--fail") + assert.Error(t, err) + assert.Equal(t, assert.AnError, err) + + // Test mock with no matching command (should return error) + mock = &MockCommandRunner{ + outputs: map[string][]byte{}, + errors: map[string]error{}, + } + + o, err = mock.RunGetOut("unknown", "command") + assert.Error(t, err) + assert.Contains(t, err.Error(), "no output for command") +} + +func TestMockCommandRunner_Priority(t *testing.T) { + // Test that outputs take priority over errors + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "test cmd": []byte("output\n"), + }, + errors: map[string]error{ + "test cmd": assert.AnError, + }, + } + + o, err := mock.RunGetOut("test", "cmd") + assert.Nil(t, err) + assert.Equal(t, []byte("output\n"), o) +} diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index 85e13b2b04..e0130dbd7a 100644 --- a/pkg/daemon/daemon.go +++ b/pkg/daemon/daemon.go @@ -165,6 +165,12 @@ type Daemon struct { // Ensures that only a single syncOSImagePullSecrets call can run at a time. osImageMux *sync.Mutex + + // Abstraction for running commands against the OS + cmdRunner CommandRunner + + // Bare minimal podman client + podmanInterface PodmanInterface } // CoreOSDaemon protects the methods that should only be called on CoreOS variants @@ -307,10 +313,11 @@ func New( } var nodeUpdaterClient *RpmOstreeClient - + cmdRunner := &CommandRunnerOS{} + podmanInterface := NewPodmanExec(cmdRunner) // Only pull the osImageURL from OSTree when we are on RHCOS or FCOS if hostos.IsCoreOSVariant() { - nodeUpdaterClientVal := NewNodeUpdaterClient() + nodeUpdaterClientVal := NewNodeUpdaterClient(cmdRunner, podmanInterface) nodeUpdaterClient = &nodeUpdaterClientVal err := nodeUpdaterClient.Initialize() if err != nil { @@ -348,6 +355,8 @@ func New( configDriftMonitor: NewConfigDriftMonitor(), osImageMux: &sync.Mutex{}, irreconcilableReporter: NewNoOpIrreconcilableReporterImpl(), + cmdRunner: cmdRunner, + podmanInterface: podmanInterface, }, nil } diff --git a/pkg/daemon/image_manager_helper.go b/pkg/daemon/image_manager_helper.go index 33ad45ff08..6848d523ca 100644 --- a/pkg/daemon/image_manager_helper.go +++ b/pkg/daemon/image_manager_helper.go @@ -5,13 +5,8 @@ import ( "errors" "fmt" "os" - "os/exec" "path/filepath" - "strings" - "time" - "github.com/opencontainers/go-digest" - pivotutils "github.com/openshift/machine-config-operator/pkg/daemon/pivot/utils" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" ) @@ -33,21 +28,6 @@ const ( type imageSystem string -// imageInspection is a public implementation of -// https://github.com/containers/skopeo/blob/82186b916faa9c8c70cfa922229bafe5ae024dec/cmd/skopeo/inspect.go#L20-L31 -type imageInspection struct { - Name string `json:",omitempty"` - Tag string `json:",omitempty"` - Digest digest.Digest - RepoDigests []string - Created *time.Time - DockerVersion string - Labels map[string]string - Architecture string - Os string - Layers []string -} - // BootedImageInfo stores MCO interested bootec image info type BootedImageInfo struct { OSImageURL string @@ -55,54 +35,6 @@ type BootedImageInfo struct { BaseChecksum string } -func podmanInspect(imgURL string) (imgdata *imageInspection, err error) { - // Pull the container image if not already available - var authArgs []string - if _, err := os.Stat(kubeletAuthFile); err == nil { - authArgs = append(authArgs, "--authfile", kubeletAuthFile) - } - args := []string{"pull", "-q"} - args = append(args, authArgs...) - args = append(args, imgURL) - _, err = pivotutils.RunExt(numRetriesNetCommands, "podman", args...) - if err != nil { - return - } - - inspectArgs := []string{"inspect", "--type=image"} - inspectArgs = append(inspectArgs, fmt.Sprintf("%s", imgURL)) - var output []byte - output, err = runGetOut("podman", inspectArgs...) - if err != nil { - return - } - var imagedataArray []imageInspection - err = json.Unmarshal(output, &imagedataArray) - if err != nil { - err = fmt.Errorf("unmarshaling podman inspect: %w", err) - return - } - imgdata = &imagedataArray[0] - return - -} - -// runGetOut executes a command, logging it, and return the stdout output. -func runGetOut(command string, args ...string) ([]byte, error) { - klog.Infof("Running captured: %s %s", command, strings.Join(args, " ")) - cmd := exec.Command(command, args...) - rawOut, err := cmd.Output() - if err != nil { - errtext := "" - if e, ok := err.(*exec.ExitError); ok { - // Trim to max of 256 characters - errtext = fmt.Sprintf("\n%s", truncate(string(e.Stderr), 256)) - } - return nil, fmt.Errorf("error running %s %s: %s%s", command, strings.Join(args, " "), err, errtext) - } - return rawOut, nil -} - // truncate a string using runes/codepoints as limits. // This specifically will avoid breaking a UTF-8 value. func truncate(input string, limit int) string { diff --git a/pkg/daemon/podman.go b/pkg/daemon/podman.go new file mode 100644 index 0000000000..e66bda81bf --- /dev/null +++ b/pkg/daemon/podman.go @@ -0,0 +1,89 @@ +package daemon + +import ( + "encoding/json" + "fmt" +) + +// PodmanStorageConfig contains storage configuration from Podman. +type PodmanStorageConfig struct { + GraphDriverName string `json:"graphDriverName"` + GraphRoot string `json:"graphRoot"` +} + +// PodmanInfo contains system information from Podman. +type PodmanInfo struct { + Store PodmanStorageConfig `json:"store"` +} + +// PodmanImageInfo contains image metadata from Podman. +type PodmanImageInfo struct { + ID string `json:"Id"` + Digest string `json:"Digest"` + RepoDigests []string `json:"RepoDigests"` + RepoDigest string `json:"-"` // Filled with matching digest from RepoDigests +} + +// PodmanInterface abstracts podman operations for testing and flexibility. +type PodmanInterface interface { + // GetPodmanImageInfoByReference retrieves image info for the given reference. + // Returns nil if no image is found. + GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error) + // GetPodmanInfo retrieves Podman system information. + GetPodmanInfo() (*PodmanInfo, error) +} + +// PodmanExecInterface is the production implementation that executes real podman commands. +type PodmanExecInterface struct { + cmdRunner CommandRunner +} + +// NewPodmanExec creates a new PodmanExecInterface with the given command runner. +func NewPodmanExec(commandRunner CommandRunner) *PodmanExecInterface { + return &PodmanExecInterface{cmdRunner: commandRunner} +} + +// GetPodmanImageInfoByReference retrieves image info for the given reference. +// It executes 'podman images --format=json --filter reference='. +// Returns nil if no image is found. +// The RepoDigest field is populated if the reference matches one of the RepoDigests. +func (p *PodmanExecInterface) GetPodmanImageInfoByReference(reference string) (*PodmanImageInfo, error) { + output, err := p.cmdRunner.RunGetOut("podman", "images", "--format=json", "--filter", "reference="+reference) + if err != nil { + return nil, err + } + var podmanInfos []PodmanImageInfo + if err := json.Unmarshal(output, &podmanInfos); err != nil { + return nil, fmt.Errorf("failed to decode podman image ls output: %v", err) + } + if len(podmanInfos) == 0 { + + return nil, nil + } + + info := &podmanInfos[0] + // Fill the custom RepoDigest field with the digest that matches the + // requested reference as it's convenient to be used by the caller + for _, digest := range info.RepoDigests { + if digest == reference { + info.RepoDigest = digest + break + } + } + + return info, nil +} + +// GetPodmanInfo retrieves Podman system information. +// It executes 'podman system info --format=json'. +func (p *PodmanExecInterface) GetPodmanInfo() (*PodmanInfo, error) { + output, err := p.cmdRunner.RunGetOut("podman", "system", "info", "--format=json") + if err != nil { + return nil, err + } + var podmanInfo PodmanInfo + if err := json.Unmarshal(output, &podmanInfo); err != nil { + return nil, fmt.Errorf("failed to decode podman system info output: %v", err) + } + return &podmanInfo, nil +} diff --git a/pkg/daemon/podman_test.go b/pkg/daemon/podman_test.go new file mode 100644 index 0000000000..0741a72ada --- /dev/null +++ b/pkg/daemon/podman_test.go @@ -0,0 +1,221 @@ +package daemon + +import ( + "encoding/json" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetPodmanImageInfoByReference_Success(t *testing.T) { + reference := "quay.io/openshift/test:latest" + imageInfo := []PodmanImageInfo{ + { + ID: "abc123", + Digest: "sha256:1234567890abcdef", + RepoDigests: []string{ + "quay.io/openshift/test@sha256:1234567890abcdef", + reference, + }, + }, + } + + jsonOutput, err := json.Marshal(imageInfo) + require.Nil(t, err) + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman images --format=json --filter reference=" + reference: jsonOutput, + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Nil(t, err) + assert.NotNil(t, info) + assert.Equal(t, "abc123", info.ID) + assert.Equal(t, "sha256:1234567890abcdef", info.Digest) + assert.Equal(t, reference, info.RepoDigest) + assert.Len(t, info.RepoDigests, 2) +} + +func TestGetPodmanImageInfoByReference_NoMatch(t *testing.T) { + reference := "quay.io/openshift/nonexistent:latest" + // If there are no matches podman returns an empty string + jsonOutput := []byte("[]") + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman images --format=json --filter reference=" + reference: jsonOutput, + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Nil(t, err) + assert.Nil(t, info) +} + +func TestGetPodmanImageInfoByReference_CommandError(t *testing.T) { + reference := "quay.io/openshift/test:latest" + + mock := &MockCommandRunner{ + outputs: map[string][]byte{}, + errors: map[string]error{ + "podman images --format=json --filter reference=" + reference: fmt.Errorf("podman command failed"), + }, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Error(t, err) + assert.Nil(t, info) + assert.Contains(t, err.Error(), "podman command failed") +} + +func TestGetPodmanImageInfoByReference_InvalidJSON(t *testing.T) { + reference := "quay.io/openshift/test:latest" + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman images --format=json --filter reference=" + reference: []byte("invalid json"), + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Error(t, err) + assert.Nil(t, info) + assert.Contains(t, err.Error(), "failed to decode podman image ls output") +} + +func TestGetPodmanImageInfoByReference_RepoDigestMatching(t *testing.T) { + reference := "quay.io/openshift/test@sha256:specific" + imageInfo := []PodmanImageInfo{ + { + ID: "xyz789", + Digest: "sha256:specific", + RepoDigests: []string{ + "quay.io/openshift/test@sha256:other", + reference, + "quay.io/openshift/test@sha256:another", + }, + }, + } + + jsonOutput, err := json.Marshal(imageInfo) + require.Nil(t, err) + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman images --format=json --filter reference=" + reference: jsonOutput, + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Nil(t, err) + assert.NotNil(t, info) + assert.Equal(t, reference, info.RepoDigest) +} + +func TestGetPodmanImageInfoByReference_NoRepoDigestMatch(t *testing.T) { + reference := "quay.io/openshift/test:tag" + imageInfo := []PodmanImageInfo{ + { + ID: "xyz789", + Digest: "sha256:specific", + RepoDigests: []string{ + "quay.io/openshift/test@sha256:other", + }, + }, + } + + jsonOutput, err := json.Marshal(imageInfo) + require.Nil(t, err) + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman images --format=json --filter reference=" + reference: jsonOutput, + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanImageInfoByReference(reference) + + assert.Nil(t, err) + assert.NotNil(t, info) + // RepoDigest should be empty since reference doesn't match any RepoDigests + assert.Equal(t, "", info.RepoDigest) +} + +func TestGetPodmanInfo_Success(t *testing.T) { + podmanInfo := PodmanInfo{ + Store: PodmanStorageConfig{ + GraphDriverName: "overlay", + GraphRoot: "/var/lib/containers/storage", + }, + } + + jsonOutput, err := json.Marshal(podmanInfo) + require.Nil(t, err) + + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman system info --format=json": jsonOutput, + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanInfo() + + assert.Nil(t, err) + assert.NotNil(t, info) + assert.Equal(t, "overlay", info.Store.GraphDriverName) + assert.Equal(t, "/var/lib/containers/storage", info.Store.GraphRoot) +} + +func TestGetPodmanInfo_CommandError(t *testing.T) { + mock := &MockCommandRunner{ + outputs: map[string][]byte{}, + errors: map[string]error{ + "podman system info --format=json": fmt.Errorf("podman system info failed"), + }, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanInfo() + + assert.Error(t, err) + assert.Nil(t, info) + assert.Contains(t, err.Error(), "podman system info failed") +} + +func TestGetPodmanInfo_InvalidJSON(t *testing.T) { + mock := &MockCommandRunner{ + outputs: map[string][]byte{ + "podman system info --format=json": []byte("not valid json"), + }, + errors: map[string]error{}, + } + + podman := NewPodmanExec(mock) + info, err := podman.GetPodmanInfo() + + assert.Error(t, err) + assert.Nil(t, info) + assert.Contains(t, err.Error(), "failed to decode podman system info output") +} diff --git a/pkg/daemon/rpm-ostree.go b/pkg/daemon/rpm-ostree.go index 312f9da77c..8972132d46 100644 --- a/pkg/daemon/rpm-ostree.go +++ b/pkg/daemon/rpm-ostree.go @@ -1,28 +1,40 @@ package daemon import ( + "encoding/json" "fmt" "os" "path/filepath" + "reflect" "strings" + "github.com/containers/image/v5/signature" rpmostreeclient "github.com/coreos/rpmostree-client-go/pkg/client" "gopkg.in/yaml.v2" "k8s.io/klog/v2" ) +const imagePolicyTransportContainerStorage = "containers-storage" +const imagePolicyFilePath = "/etc/containers/policy.json" +const rpmOstreeTemporalDropinFile = "/run/systemd/system/rpm-ostreed.service.d/temporal-policy-binding.conf" +const rpmOstreeTemporalPolicyFile = "/run/tmp-rpm-ostree-policy.json" + // RpmOstreeClient provides all RpmOstree related methods in one structure. // This structure implements DeploymentClient // // TODO(runcom): make this private to pkg/daemon!!! type RpmOstreeClient struct { - client rpmostreeclient.Client + client rpmostreeclient.Client + commandRunner CommandRunner + podmanInterface PodmanInterface } // NewNodeUpdaterClient is a wrapper to create an RpmOstreeClient -func NewNodeUpdaterClient() RpmOstreeClient { +func NewNodeUpdaterClient(commandRunner CommandRunner, podmanInterface PodmanInterface) RpmOstreeClient { return RpmOstreeClient{ - client: rpmostreeclient.NewClient("machine-config-daemon"), + client: rpmostreeclient.NewClient("machine-config-daemon"), + commandRunner: commandRunner, + podmanInterface: podmanInterface, } } @@ -61,6 +73,12 @@ func (r *RpmOstreeClient) Initialize() error { if err := bug2111817Workaround(); err != nil { return err } + // Ensure the temporal ostree dropin doesn't exist + // It shouldn't, but it's possible if the MCD container + // suddenly died before rpm-ostree rebase finished + if err := cleanupTemporalOstreePolicyFileDropin(); err != nil { + return err + } // Commands like update and rebase need the pull secrets to pull images and manifests, // make sure we get access to them when we Initialize @@ -94,7 +112,7 @@ func (r *RpmOstreeClient) GetBootedAndStagedDeployment() (*rpmostreeclient.Deplo // GetStatus returns multi-line human-readable text describing system status func (r *RpmOstreeClient) GetStatus() (string, error) { - output, err := runGetOut("rpm-ostree", "status") + output, err := r.commandRunner.RunGetOut("rpm-ostree", "status") if err != nil { return "", err } @@ -147,8 +165,8 @@ type RpmOstreeVersionData struct { } // RpmOstreeVersion returns the running rpm-ostree version number -func rpmOstreeVersion() (*VersionData, error) { - buf, err := runGetOut("rpm-ostree", "--version") +func (r *RpmOstreeClient) rpmOstreeVersion() (*VersionData, error) { + buf, err := r.commandRunner.RunGetOut("rpm-ostree", "--version") if err != nil { return nil, err } @@ -162,7 +180,7 @@ func rpmOstreeVersion() (*VersionData, error) { } func (r *RpmOstreeClient) IsNewEnoughForLayering() (bool, error) { - verdata, err := rpmOstreeVersion() + verdata, err := r.rpmOstreeVersion() if err != nil { return false, err } @@ -185,11 +203,168 @@ func (r *RpmOstreeClient) RebaseLayered(imgURL string) error { } // RebaseLayeredFromContainerStorage rebases the system from an existing local container storage image. -func (r *RpmOstreeClient) RebaseLayeredFromContainerStorage(imgURL string) error { +func (r *RpmOstreeClient) RebaseLayeredFromContainerStorage(podmanImageInfo *PodmanImageInfo) error { // Try to re-link the merged pull secrets if they exist, since it could have been populated without a daemon reboot if err := useMergedPullSecrets(rpmOstreeSystem); err != nil { return fmt.Errorf("Error while ensuring access to pull secrets: %w", err) } - klog.Infof("Executing local container storage rebase to %s", imgURL) - return runRpmOstree("rebase", "--experimental", "ostree-unverified-image:containers-storage:"+imgURL) + + defer func() { + // Call the cleanup always, just in case there are left-overs of + // a previous killed MCD (unlikely, but possible) + if err := cleanupTemporalOstreePolicyFileDropin(); err != nil { + klog.Errorf("Error deleting temporary MCD temporal policy %v", err) + } + }() + // Temporary patch the containers policies to allow rpm-ostree to pull + // an image from local storage. Only required if the policies are + // restrictive and won't allow containers-storage transport pulls. + if err := r.patchPoliciesForContainerStorage(podmanImageInfo); err != nil { + // Swallow the error and let it fail in case the user/default defined policies + // avoids pulling the image + klog.Errorf("Error writing temporal policy files %v", err) + } + + klog.Infof("Executing local container storage rebase to %s", podmanImageInfo.RepoDigest) + return runRpmOstree("rebase", "--experimental", "ostree-unverified-image:containers-storage:"+podmanImageInfo.RepoDigest) +} + +// patchPoliciesForContainerStorage temporarily overrides the container image policy visible +// to rpm-ostreed to ensure pulls from the "containers-storage" transport are allowed for the +// given image. +// +// This is necessary for tools like rpm-ostree to function correctly with locally +// stored images, especially in environments with restrictive security policies. A common +// scenario is a user removing the default "insecureAcceptAnything" policy without +// adding an explicit rule for local storage, which is an easily missed implementation detail. +// +// The function is idempotent and will not modify the policy if it's already permissive +// enough for local storage pulls. +// +// To avoid modifying the system's policy file, this function creates a temporary policy file +// at rpmOstreeTemporalPolicyFile and uses a systemd drop-in to bind-mount it over the actual +// policy file for the rpm-ostreed service. The drop-in and temporary policy file are cleaned +// up after the rpm-ostree operation completes. +func (r *RpmOstreeClient) patchPoliciesForContainerStorage(podmanImageInfo *PodmanImageInfo) error { + url, err := r.generateTransportPolicyKeyForReference(podmanImageInfo) + if err != nil { + return err + } + _, err = os.Stat(imagePolicyFilePath) + if err != nil { + if os.IsNotExist(err) { + klog.Warningf("No policy file found at %s. Skipping temporal policy generation.", imagePolicyFilePath) + return nil + } + return err + } + + policyOriginalContent, err := os.ReadFile(imagePolicyFilePath) + if err != nil { + return err + } + + policy, err := signature.NewPolicyFromBytes(policyOriginalContent) + if err != nil { + return err + } + + _, containerStoragePoliciesPresent := policy.Transports[imagePolicyTransportContainerStorage] + if (reflect.DeepEqual(policy.Default[0], signature.PolicyRequirements{signature.NewPRInsecureAcceptAnything()}) && !containerStoragePoliciesPresent) { + // Temporary patching the policies.json file can be skipped, with warranties, and without re-implementing the + // logic that evaluates the policies or importing it under the following circumstances (must match all): + // 1. The default policy should be "insecureAcceptAnything" + // 2. Transport-specific policies for containers-storage shouldn't be in place. + return nil + } + + // At this point there's no warranty the policy will allow rpm-ostree to fetch the image + // from local storage -> Add a specific rule to allow the image + if !containerStoragePoliciesPresent { + policy.Transports[imagePolicyTransportContainerStorage] = make(map[string]signature.PolicyRequirements) + } + policy.Transports[imagePolicyTransportContainerStorage][url] = signature.PolicyRequirements{ + signature.NewPRInsecureAcceptAnything(), + } + + // Prepare the json patched content + policyJSON, err := json.MarshalIndent(policy, "", " ") + if err != nil { + return err + } + + // The temporal policy is written atomically + if err := writeFileAtomicallyWithDefaults(rpmOstreeTemporalPolicyFile, policyJSON); err != nil { + return err + } + + if err := writeTemporalOstreePolicyFileDropin(); err != nil { + return err + } + + klog.Infof("Temporal allow policy added for URL %s", url) + return nil +} + +// generateTransportPolicyKeyForReference creates the reference string used as a key in the +// image policy file for the "containers-storage" transport. +// +// The format of this string is not officially documented and was determined by reverse-engineering +// the policy evaluation logic. It is structured as follows: +// "[@]@" +// +// The storage driver and graph root details are retrieved from the running Podman +// instance at runtime. +func (r *RpmOstreeClient) generateTransportPolicyKeyForReference(podmanImageInfo *PodmanImageInfo) (string, error) { + podmanInfo, err := r.podmanInterface.GetPodmanInfo() + if err != nil { + return "", fmt.Errorf("failed to get podman info for storage configuration gathering: %w", err) + } + return fmt.Sprintf("[%s@%s]%s@%s", podmanInfo.Store.GraphDriverName, podmanInfo.Store.GraphRoot, podmanImageInfo.RepoDigest, podmanImageInfo.ID), nil +} + +// writeTemporalOstreePolicyFileDropin creates a systemd drop-in configuration that +// bind-mounts the temporary policy file over the actual policy file for rpm-ostreed. +// This allows rpm-ostree to use the patched policy without modifying the system's +// policy file directly. After writing the drop-in, the function reloads systemd and +// restarts rpm-ostreed to apply the changes. +func writeTemporalOstreePolicyFileDropin() error { + // Create a temporal dropin to mount the temporal policy into rpm-ostreed process + if err := writeFileAtomicallyWithDefaults( + rpmOstreeTemporalDropinFile, + []byte( + fmt.Sprintf( + "[Service]\nBindReadOnlyPaths=%s:%s", rpmOstreeTemporalPolicyFile, imagePolicyFilePath), + ), + ); err != nil { + return err + } + return systemdRpmOstreeReload() +} + +// cleanupTemporalOstreePolicyFileDropin removes the systemd drop-in configuration +// created by writeTemporalOstreePolicyFileDropin, restoring rpm-ostreed to use the +// original system policy file. After removing the drop-in, the function reloads +// systemd and restarts rpm-ostreed to apply the changes. +func cleanupTemporalOstreePolicyFileDropin() error { + if err := os.Remove(rpmOstreeTemporalDropinFile); err != nil { + if os.IsNotExist(err) { + // Nothing to do + return nil + } + return err + } + return systemdRpmOstreeReload() +} + +// systemdRpmOstreeReload notifies systemd of unit configuration changes and restarts +// the rpm-ostreed service if it's currently running. +func systemdRpmOstreeReload() error { + // Tell systemd that there are changes in the units + if err := runCmdSync("systemctl", "daemon-reload"); err != nil { + return err + } + + // In case rpm-ostreed is running restart it to take the latest config + return runCmdSync("systemctl", "try-restart", "rpm-ostreed") } diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index e7c91edcd3..ab12474c50 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -447,48 +447,30 @@ func podmanRemove(cid string) { exec.Command("podman", "rm", "-f", cid).Run() } -// return true if the image is present -func isImagePresent(imgURL string) (bool, error) { - // search the image - var imageSearch []byte - imageSearch, err := runGetOut("podman", "images", "-q", "--filter", fmt.Sprintf("reference=%s", imgURL)) - if err != nil { - return false, fmt.Errorf("error searching the image: %w", err) +func pullExtensionsImage(podmanInterface PodmanInterface, imgURL string) error { + // Check if the image is present + podmanImageInfo, err := podmanInterface.GetPodmanImageInfoByReference(imgURL) + if err != nil || podmanImageInfo != nil { + // The image exists (ok) or an error happened + return err } - if strings.TrimSpace(string(imageSearch)) == "" { - return false, nil + + // The image is not present, pull it + var authArgs []string + if _, err := os.Stat(kubeletAuthFile); err == nil { + authArgs = append(authArgs, "--authfile", kubeletAuthFile) } - return true, nil + args := []string{"pull", "-q"} + args = append(args, authArgs...) + args = append(args, imgURL) + _, err = pivotutils.RunExtBackground(numRetriesNetCommands, "podman", args...) + return err } func podmanCopy(imgURL, osImageContentDir string) (err error) { - // arguments used in external commands - var args []string - // make sure that osImageContentDir doesn't exist os.RemoveAll(osImageContentDir) - // Check if the image is present - imagePresent, err := isImagePresent(imgURL) - if err != nil { - return - } - - // Pull the container image - if !imagePresent { - var authArgs []string - if _, err := os.Stat(kubeletAuthFile); err == nil { - authArgs = append(authArgs, "--authfile", kubeletAuthFile) - } - args = []string{"pull", "-q"} - args = append(args, authArgs...) - args = append(args, imgURL) - _, err = pivotutils.RunExtBackground(numRetriesNetCommands, "podman", args...) - if err != nil { - return - } - } - // create a container var cidBuf []byte containerName := pivottypes.PivotNamePrefix + string(uuid.NewUUID()) @@ -502,7 +484,7 @@ func podmanCopy(imgURL, osImageContentDir string) (err error) { // copy the content from create container locally into a temp directory under /run/ cid := strings.TrimSpace(string(cidBuf)) - args = []string{"cp", fmt.Sprintf("%s:/", cid), osImageContentDir} + args := []string{"cp", fmt.Sprintf("%s:/", cid), osImageContentDir} _, err = pivotutils.RunExtBackground(numRetriesNetCommands, "podman", args...) if err != nil { return @@ -520,7 +502,7 @@ func podmanCopy(imgURL, osImageContentDir string) (err error) { // ExtractExtensionsImage extracts the OS extensions content in a temporary directory under /run/machine-os-extensions // and returns the path on successful extraction -func ExtractExtensionsImage(imgURL string) (osExtensionsImageContentDir string, err error) { +func (dn *CoreOSDaemon) ExtractExtensionsImage(imgURL string) (osExtensionsImageContentDir string, err error) { if err = os.MkdirAll(osExtensionsContentBaseDir, 0o755); err != nil { err = fmt.Errorf("error creating directory %s: %w", osExtensionsContentBaseDir, err) return @@ -529,7 +511,9 @@ func ExtractExtensionsImage(imgURL string) (osExtensionsImageContentDir string, if osExtensionsImageContentDir, err = os.MkdirTemp(osExtensionsContentBaseDir, "os-extensions-content-"); err != nil { return } - + if err := pullExtensionsImage(dn.podmanInterface, imgURL); err != nil { + return osExtensionsImageContentDir, fmt.Errorf("error pulling extensions image %s: %w", imgURL, err) + } // Extract the image using `podman cp` return osExtensionsImageContentDir, podmanCopy(imgURL, osExtensionsImageContentDir) } @@ -2586,9 +2570,9 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error { // If PIS is configured check if the image is locally present. If so, rebase using // the local image - isOsImagePresent := false + var podmanImageInfo *PodmanImageInfo if isPisConfigured { - if isOsImagePresent, err = isImagePresent(newURL); err != nil { + if podmanImageInfo, err = dn.podmanInterface.GetPodmanImageInfoByReference(newURL); err != nil { return err } } @@ -2603,8 +2587,8 @@ func (dn *Daemon) updateLayeredOS(config *mcfgv1.MachineConfig) error { } } - if isOsImagePresent { - if err := dn.NodeUpdaterClient.RebaseLayeredFromContainerStorage(newURL); err != nil { + if podmanImageInfo != nil { + if err := dn.NodeUpdaterClient.RebaseLayeredFromContainerStorage(podmanImageInfo); err != nil { return fmt.Errorf("failed to update OS from local storage: %s: %w", newURL, err) } } else { @@ -2704,6 +2688,11 @@ func (dn *Daemon) isPinnedImageSetConfigured() (bool, error) { return false, nil } +// TODO: Delete this function to always consume the CommandRunner interface instance +// Tracking story: https://issues.redhat.com/browse/MCO-1925 +// +// Conserved the old signature to avoid a big footprint bugfix with this change +// // Synchronously invoke a command, writing its stdout to our stdout, // and gathering stderr into a buffer which will be returned in err // in case of error. @@ -2880,7 +2869,7 @@ func (dn *CoreOSDaemon) applyLayeredOSChanges(mcDiff machineConfigDiff, oldConfi } } - if osExtensionsContentDir, err = ExtractExtensionsImage(newConfig.Spec.BaseOSExtensionsContainerImage); err != nil { + if osExtensionsContentDir, err = dn.ExtractExtensionsImage(newConfig.Spec.BaseOSExtensionsContainerImage); err != nil { // Report ImagePulledFromRegistry condition as false (failed) if imageModeStatusReportingEnabled { err := upgrademonitor.GenerateAndApplyMachineConfigNodes( diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index 63e83b4144..40228b95b9 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -737,45 +737,6 @@ func TestOriginalFileBackupRestore(t *testing.T) { assert.True(t, os.IsNotExist(err)) } -func TestIsImagePresent(t *testing.T) { - // Create a temporary directory for the mock "podman" command. - tmpDir, err := os.MkdirTemp("", "*.test") - require.Nil(t, err) - defer os.RemoveAll(tmpDir) - - // Create the script that mocks the "podman" command: - podmanScript := filepath.Join(tmpDir, "podman") - err = os.WriteFile( - podmanScript, - []byte( - "#!/bin/bash\n"+ - "if [[ \"$4\" =~ test-true ]]; then\n"+ - " echo '1b57f04f6da8'\n"+ - "else\n"+ - " echo ''\n"+ - "fi\n"+ - "exit 0\n", - ), - 0700, - ) - require.Nil(t, err) - - // Change the "PATH" environment variable for the execution of the rest of the test: - oldPath := os.Getenv("PATH") - defer os.Setenv("PATH", oldPath) - newPath := fmt.Sprintf("%s%c%s", tmpDir, os.PathListSeparator, oldPath) - os.Setenv("PATH", newPath) - - // Test the function: - imagePresent, err := isImagePresent("quay.io/openshift-release-dev/test-true") - assert.Nil(t, err) - assert.True(t, imagePresent) - - imagePresent, err = isImagePresent("quay.io/openshift-release-dev/test-false") - assert.Nil(t, err) - assert.False(t, imagePresent) -} - func TestFindClosestFilePolicyPathMatch(t *testing.T) { policyActions := map[string][]opv1.NodeDisruptionPolicyStatusAction{